From e503697d9239665d9d9c4abdba84b84e760a0a2f Mon Sep 17 00:00:00 2001 From: ismeth Date: Tue, 24 Feb 2026 01:48:18 +0100 Subject: [PATCH] =?UTF-8?q?fix(athena):=20council=20review=20fixes=20?= =?UTF-8?q?=E2=80=94=20delegation=20bug,=20dead=20code,=20test=20coverage?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add background_output to council-member allowlist (fixes delegation deadlock) - Replace empty catch with error logging in prepare-council-prompt - Remove unnecessary type assertion in agent.ts - Remove dead hasAgentToolRestrictions function - Fix incorrect test assertions (undefined vs false semantics) - Add barrel export for athena module - Add guard function test coverage (5 tests) - Add parity test for triple-sync restrictions (9 tests) --- src/agents/athena/agent.ts | 2 +- src/agents/athena/council-member-agent.ts | 1 + src/agents/athena/index.ts | 3 + src/agents/builtin-agents.ts | 2 +- .../athena-council-guard.test.ts | 85 +++++++++++ .../builtin-agents/council-member-agents.ts | 2 +- .../agent-tool-restrictions-parity.test.ts | 140 ++++++++++++++++++ src/shared/agent-tool-restrictions.test.ts | 51 ++++--- src/shared/agent-tool-restrictions.ts | 7 +- src/tools/prepare-council-prompt/tools.ts | 6 +- 10 files changed, 268 insertions(+), 31 deletions(-) create mode 100644 src/agents/athena/index.ts create mode 100644 src/agents/builtin-agents/athena-council-guard.test.ts create mode 100644 src/shared/agent-tool-restrictions-parity.test.ts diff --git a/src/agents/athena/agent.ts b/src/agents/athena/agent.ts index a11d752d4..bd8ff38f1 100644 --- a/src/agents/athena/agent.ts +++ b/src/agents/athena/agent.ts @@ -238,7 +238,7 @@ export function createAthenaAgent(model: string): AgentConfig { // question permission is set by tool-config-handler.ts based on CLI mode (allow/deny) const permission = { ...restrictions.permission, - } as AgentConfig["permission"] + } const base = { description: diff --git a/src/agents/athena/council-member-agent.ts b/src/agents/athena/council-member-agent.ts index fa070bb66..335e1953b 100644 --- a/src/agents/athena/council-member-agent.ts +++ b/src/agents/athena/council-member-agent.ts @@ -52,6 +52,7 @@ export function createCouncilMemberAgent(model: string): AgentConfig { "lsp_diagnostics", "ast_grep_search", "call_omo_agent", + "background_output", ]) // Explicitly deny TodoWrite/TodoRead even though `*: deny` should catch them. diff --git a/src/agents/athena/index.ts b/src/agents/athena/index.ts new file mode 100644 index 000000000..b3c148222 --- /dev/null +++ b/src/agents/athena/index.ts @@ -0,0 +1,3 @@ +export { createAthenaAgent, ATHENA_PROMPT_METADATA } from "./agent" +export { createCouncilMemberAgent, COUNCIL_MEMBER_PROMPT, COUNCIL_DELEGATION_ADDENDUM } from "./council-member-agent" +export { applyModelThinkingConfig } from "./model-thinking-config" diff --git a/src/agents/builtin-agents.ts b/src/agents/builtin-agents.ts index 1d96e5d96..f0b63adc0 100644 --- a/src/agents/builtin-agents.ts +++ b/src/agents/builtin-agents.ts @@ -12,7 +12,7 @@ import { createMetisAgent, metisPromptMetadata } from "./metis" import { createAtlasAgent, atlasPromptMetadata } from "./atlas" import { createMomusAgent, momusPromptMetadata } from "./momus" import { createHephaestusAgent } from "./hephaestus" -import { createAthenaAgent, ATHENA_PROMPT_METADATA } from "./athena/agent" +import { createAthenaAgent, ATHENA_PROMPT_METADATA } from "./athena" import type { AvailableCategory } from "./dynamic-agent-prompt-builder" import { fetchAvailableModels, diff --git a/src/agents/builtin-agents/athena-council-guard.test.ts b/src/agents/builtin-agents/athena-council-guard.test.ts new file mode 100644 index 000000000..72eae0901 --- /dev/null +++ b/src/agents/builtin-agents/athena-council-guard.test.ts @@ -0,0 +1,85 @@ +import { describe, expect, test } from "bun:test" +import { applyMissingCouncilGuard } from "./athena-council-guard" +import type { AgentConfig } from "@opencode-ai/sdk" + +describe("applyMissingCouncilGuard", () => { + describe("#given an athena agent config with no skipped members", () => { + test("#when applying the guard #then replaces prompt with missing council message", () => { + //#given + const athenaConfig: AgentConfig = { + model: "anthropic/claude-opus-4-6", + prompt: "original orchestration prompt", + temperature: 0.1, + } + //#when + const result = applyMissingCouncilGuard(athenaConfig) + //#then + expect(result.prompt).not.toBe("original orchestration prompt") + expect(result.prompt).toContain("No Council Members Configured") + }) + }) + + describe("#given an athena agent config with skipped members", () => { + test("#when applying the guard #then includes skipped member names and reasons", () => { + //#given + const athenaConfig: AgentConfig = { + model: "anthropic/claude-opus-4-6", + prompt: "original orchestration prompt", + } + const skippedMembers = [ + { name: "GPT", reason: "invalid model format" }, + { name: "Gemini", reason: "duplicate name" }, + ] + //#when + const result = applyMissingCouncilGuard(athenaConfig, skippedMembers) + //#then + expect(result.prompt).toContain("GPT") + expect(result.prompt).toContain("invalid model format") + expect(result.prompt).toContain("Gemini") + expect(result.prompt).toContain("duplicate name") + expect(result.prompt).toContain("Why Council Failed") + }) + }) + + describe("#given an athena agent config", () => { + test("#when applying the guard #then preserves model and other agent properties", () => { + //#given + const athenaConfig: AgentConfig = { + model: "anthropic/claude-opus-4-6", + prompt: "original prompt", + temperature: 0.1, + } + //#when + const result = applyMissingCouncilGuard(athenaConfig) + //#then + expect(result.model).toBe("anthropic/claude-opus-4-6") + expect(result.temperature).toBe(0.1) + }) + + test("#when applying the guard #then prompt includes configuration instructions", () => { + //#given + const athenaConfig: AgentConfig = { + model: "anthropic/claude-opus-4-6", + prompt: "original prompt", + } + //#when + const result = applyMissingCouncilGuard(athenaConfig) + //#then + expect(result.prompt).toContain("oh-my-opencode") + expect(result.prompt).toContain("council") + expect(result.prompt).toContain("members") + }) + + test("#when applying the guard with empty skipped members array #then does not include why council failed section", () => { + //#given + const athenaConfig: AgentConfig = { + model: "anthropic/claude-opus-4-6", + prompt: "original prompt", + } + //#when + const result = applyMissingCouncilGuard(athenaConfig, []) + //#then + expect(result.prompt).not.toContain("Why Council Failed") + }) + }) +}) diff --git a/src/agents/builtin-agents/council-member-agents.ts b/src/agents/builtin-agents/council-member-agents.ts index f43bdf22a..a79d758da 100644 --- a/src/agents/builtin-agents/council-member-agents.ts +++ b/src/agents/builtin-agents/council-member-agents.ts @@ -1,6 +1,6 @@ import type { AgentConfig } from "@opencode-ai/sdk" import type { CouncilConfig, CouncilMemberConfig } from "../../config/schema/athena" -import { createCouncilMemberAgent } from "../athena/council-member-agent" +import { createCouncilMemberAgent } from "../athena" import { parseModelString } from "../../tools/delegate-task/model-string-parser" import { log } from "../../shared/logger" diff --git a/src/shared/agent-tool-restrictions-parity.test.ts b/src/shared/agent-tool-restrictions-parity.test.ts new file mode 100644 index 000000000..5dfe2e968 --- /dev/null +++ b/src/shared/agent-tool-restrictions-parity.test.ts @@ -0,0 +1,140 @@ +/** + * Parity test: verifies Athena and council-member tool restrictions stay in sync + * across the 3 definition surfaces. + * + * Surface 1: src/agents/athena/agent.ts — createAgentToolRestrictions() deny-list + * Surface 2: src/shared/agent-tool-restrictions.ts — AGENT_RESTRICTIONS boolean map + * Surface 3: src/agents/athena/council-member-agent.ts — createAgentToolAllowlist() array + * + * This test FAILS if someone adds/removes a restriction in one surface without updating the others. + */ + +import { describe, expect, it } from "bun:test" +import { getAgentToolRestrictions } from "./agent-tool-restrictions" + +// Surface 1: Athena deny-list from src/agents/athena/agent.ts +// createAgentToolRestrictions(["write", "edit", "call_omo_agent"]) +const ATHENA_DENY_LIST = ["write", "edit", "call_omo_agent"] + +// Surface 3: Council-member allowlist from src/agents/athena/council-member-agent.ts +// createAgentToolAllowlist([...]) +const COUNCIL_MEMBER_ALLOWLIST = [ + "read", + "grep", + "glob", + "lsp_goto_definition", + "lsp_find_references", + "lsp_symbols", + "lsp_diagnostics", + "ast_grep_search", + "call_omo_agent", + "background_output", +] + +// Tools granted to Athena by tool-config-handler.ts (not in deny-list, not in AGENT_RESTRICTIONS) +const ATHENA_HANDLER_GRANTS = ["task", "prepare_council_prompt"] + +describe("agent tool restrictions parity", () => { + describe("given Athena restrictions", () => { + describe("#when comparing deny-list (agent.ts) with boolean map (agent-tool-restrictions.ts)", () => { + it("every tool in the deny-list has a matching false entry in AGENT_RESTRICTIONS", () => { + const athenaRestrictions = getAgentToolRestrictions("athena") + + for (const tool of ATHENA_DENY_LIST) { + expect( + athenaRestrictions[tool], + `Tool "${tool}" is in the deny-list (agent.ts) but not false in AGENT_RESTRICTIONS["athena"]` + ).toBe(false) + } + }) + + it("every false entry in AGENT_RESTRICTIONS is in the deny-list", () => { + const athenaRestrictions = getAgentToolRestrictions("athena") + const deniedInMap = Object.entries(athenaRestrictions) + .filter(([, value]) => value === false) + .map(([key]) => key) + + for (const tool of deniedInMap) { + expect( + ATHENA_DENY_LIST, + `Tool "${tool}" is false in AGENT_RESTRICTIONS["athena"] but missing from deny-list (agent.ts)` + ).toContain(tool) + } + }) + + it("deny-list and AGENT_RESTRICTIONS false-entries have the same length", () => { + const athenaRestrictions = getAgentToolRestrictions("athena") + const deniedInMap = Object.entries(athenaRestrictions) + .filter(([, value]) => value === false) + .map(([key]) => key) + + expect(deniedInMap.length).toBe(ATHENA_DENY_LIST.length) + }) + }) + + describe("#when checking handler grants do not conflict with deny-list", () => { + it("tools granted by tool-config-handler are NOT in the deny-list", () => { + for (const tool of ATHENA_HANDLER_GRANTS) { + expect( + ATHENA_DENY_LIST, + `Tool "${tool}" is granted by tool-config-handler but also in the deny-list — conflict!` + ).not.toContain(tool) + } + }) + + it("tools granted by tool-config-handler are NOT false in AGENT_RESTRICTIONS", () => { + const athenaRestrictions = getAgentToolRestrictions("athena") + + for (const tool of ATHENA_HANDLER_GRANTS) { + expect( + athenaRestrictions[tool], + `Tool "${tool}" is granted by tool-config-handler but is false in AGENT_RESTRICTIONS["athena"]` + ).not.toBe(false) + } + }) + }) + }) + + describe("given council-member restrictions", () => { + describe("#when comparing allowlist (council-member-agent.ts) with boolean map (agent-tool-restrictions.ts)", () => { + it("every tool in the allowlist has a matching true entry in AGENT_RESTRICTIONS", () => { + const councilRestrictions = getAgentToolRestrictions("council-member") + + for (const tool of COUNCIL_MEMBER_ALLOWLIST) { + expect( + councilRestrictions[tool], + `Tool "${tool}" is in the allowlist (council-member-agent.ts) but not true in AGENT_RESTRICTIONS["council-member"]` + ).toBe(true) + } + }) + + it("every true entry in AGENT_RESTRICTIONS is in the allowlist", () => { + const councilRestrictions = getAgentToolRestrictions("council-member") + const allowedInMap = Object.entries(councilRestrictions) + .filter(([key, value]) => key !== "*" && value === true) + .map(([key]) => key) + + for (const tool of allowedInMap) { + expect( + COUNCIL_MEMBER_ALLOWLIST, + `Tool "${tool}" is true in AGENT_RESTRICTIONS["council-member"] but missing from allowlist (council-member-agent.ts)` + ).toContain(tool) + } + }) + + it("allowlist and AGENT_RESTRICTIONS true-entries have the same length", () => { + const councilRestrictions = getAgentToolRestrictions("council-member") + const allowedInMap = Object.entries(councilRestrictions) + .filter(([key, value]) => key !== "*" && value === true) + .map(([key]) => key) + + expect(allowedInMap.length).toBe(COUNCIL_MEMBER_ALLOWLIST.length) + }) + + it("AGENT_RESTRICTIONS has wildcard deny (*: false) for council-member", () => { + const councilRestrictions = getAgentToolRestrictions("council-member") + expect(councilRestrictions["*"]).toBe(false) + }) + }) + }) +}) diff --git a/src/shared/agent-tool-restrictions.test.ts b/src/shared/agent-tool-restrictions.test.ts index 14151d3b8..74c7fe1b5 100644 --- a/src/shared/agent-tool-restrictions.test.ts +++ b/src/shared/agent-tool-restrictions.test.ts @@ -1,8 +1,5 @@ import { describe, expect, test } from "bun:test" -import { - getAgentToolRestrictions, - hasAgentToolRestrictions, -} from "./agent-tool-restrictions" +import { getAgentToolRestrictions } from "./agent-tool-restrictions" describe("agent-tool-restrictions", () => { test("athena restrictions include call_omo_agent", () => { @@ -20,9 +17,19 @@ describe("agent-tool-restrictions", () => { //#when const restrictions = getAgentToolRestrictions("council-member") //#then - expect(restrictions.call_omo_agent).toBe(false) - expect(restrictions.switch_agent).toBe(false) - expect(restrictions.background_wait).toBe(false) + // Wildcard deny key + expect(restrictions["*"]).toBe(false) + // Explicitly allowed tools + expect(restrictions.read).toBe(true) + expect(restrictions.grep).toBe(true) + expect(restrictions.call_omo_agent).toBe(true) + expect(restrictions.background_output).toBe(true) + // Explicitly denied tools + expect(restrictions.todowrite).toBe(false) + expect(restrictions.todoread).toBe(false) + // Unlisted tools are undefined (SDK applies wildcard at runtime) + expect(restrictions.switch_agent).toBeUndefined() + expect(restrictions.background_wait).toBeUndefined() }) test("#given dynamic council member name #when getAgentToolRestrictions #then returns council-member restrictions", () => { @@ -31,19 +38,21 @@ describe("agent-tool-restrictions", () => { //#when const restrictions = getAgentToolRestrictions(dynamicName) //#then - expect(restrictions.write).toBe(false) - expect(restrictions.edit).toBe(false) - expect(restrictions.task).toBe(false) - expect(restrictions.call_omo_agent).toBe(false) - expect(restrictions.switch_agent).toBe(false) - expect(restrictions.background_wait).toBe(false) - }) - - test("hasAgentToolRestrictions returns true for athena", () => { - //#given - //#when - const result = hasAgentToolRestrictions("athena") - //#then - expect(result).toBe(true) + // Wildcard deny key + expect(restrictions["*"]).toBe(false) + // Explicitly allowed tools + expect(restrictions.read).toBe(true) + expect(restrictions.grep).toBe(true) + expect(restrictions.call_omo_agent).toBe(true) + expect(restrictions.background_output).toBe(true) + // Explicitly denied tools + expect(restrictions.todowrite).toBe(false) + expect(restrictions.todoread).toBe(false) + // Unlisted tools are undefined (SDK applies wildcard at runtime) + expect(restrictions.switch_agent).toBeUndefined() + expect(restrictions.write).toBeUndefined() + expect(restrictions.edit).toBeUndefined() + expect(restrictions.task).toBeUndefined() + expect(restrictions.background_wait).toBeUndefined() }) }) diff --git a/src/shared/agent-tool-restrictions.ts b/src/shared/agent-tool-restrictions.ts index ca348b661..3fae82f9f 100644 --- a/src/shared/agent-tool-restrictions.ts +++ b/src/shared/agent-tool-restrictions.ts @@ -70,6 +70,7 @@ const AGENT_RESTRICTIONS: Record> = { lsp_diagnostics: true, ast_grep_search: true, call_omo_agent: true, + background_output: true, todowrite: false, todoread: false, }, @@ -85,8 +86,4 @@ export function getAgentToolRestrictions(agentName: string): Record key.toLowerCase() === agentName.toLowerCase())?.[1] - return restrictions !== undefined && Object.keys(restrictions).length > 0 -} + diff --git a/src/tools/prepare-council-prompt/tools.ts b/src/tools/prepare-council-prompt/tools.ts index eb8540ea3..d13259e6e 100644 --- a/src/tools/prepare-council-prompt/tools.ts +++ b/src/tools/prepare-council-prompt/tools.ts @@ -3,7 +3,7 @@ import { randomUUID } from "node:crypto" import { writeFile, unlink, mkdir } from "node:fs/promises" import { join } from "node:path" import { log } from "../../shared/logger" -import { COUNCIL_MEMBER_PROMPT, COUNCIL_DELEGATION_ADDENDUM } from "../../agents/athena/council-member-agent" +import { COUNCIL_MEMBER_PROMPT, COUNCIL_DELEGATION_ADDENDUM } from "../../agents/athena" const CLEANUP_DELAY_MS = 30 * 60 * 1000 const COUNCIL_TMP_DIR = ".sisyphus/tmp" @@ -49,7 +49,9 @@ ${args.prompt}` await writeFile(filePath, content, "utf-8") setTimeout(() => { - unlink(filePath).catch(() => {}) + unlink(filePath).catch((err) => { + log("[prepare-council-prompt] Failed to clean up temp file", { filePath, error: String(err) }) + }) }, CLEANUP_DELAY_MS) log("[prepare-council-prompt] Saved prompt", { filePath, length: args.prompt.length, mode })