fix(athena): address 9 council-audit findings — dead code, bugs, and hardening

Fixes from multi-model council audit (7 members, 19 findings, 9 selected): - Use parseModelString() for cross-provider Anthropic thinking config (#3) - Update stale AGENTS.md athena directory listing (#4) - Replace prompt in appendMissingCouncilPrompt instead of appending (#5) - Extract duplicated session cleanup logic in agent-switch hook (#6) - Surface skipped council members when >=2 valid members exist (#9) - Expand fallback handoff regex with negation guards (#11) - Remove dead council-member agent from agentSources and tests (#12) - Make runtime council member duplicate check case-insensitive (#14) - Fix false-positive schema tests by adding required name field (#18)
2026-02-20 16:10:08 +01:00
parent f0d0658eae
commit 11a4d457bf
12 changed files with 99 additions and 46 deletions
--- a/assets/oh-my-opencode.schema.json
+++ b/assets/oh-my-opencode.schema.json
@@ -3541,7 +3541,8 @@
                      },
                      "name": {
                        "type": "string",
-                        "minLength": 1
+                        "minLength": 1,
+                        "pattern": "^[a-zA-Z0-9][a-zA-Z0-9 .\\-]*$"
                      },
                      "temperature": {
                        "type": "number",
--- a/src/agents/AGENTS.md
+++ b/src/agents/AGENTS.md
@@ -51,11 +51,10 @@ agents/
 ├── momus.ts               # Plan review
 ├── atlas/agent.ts         # Todo orchestrator
 ├── athena/                # Multi-model council orchestrator
-│   ├── agent.ts           # Athena agent factory
+│   ├── agent.ts           # Athena agent factory + system prompt
 │   ├── council-member-agent.ts  # Council member agent factory
-│   ├── model-parser.ts    # Model string parser
-│   ├── types.ts           # Council types
-│   └── index.ts           # Barrel exports
+│   ├── model-thinking-config.ts  # Per-provider thinking/reasoning config
+│   └── model-thinking-config.test.ts  # Tests for thinking config
 ├── types.ts               # AgentFactory, AgentMode
 ├── agent-builder.ts       # buildAgent() composition
 ├── utils.ts               # Agent utilities
--- a/src/agents/athena/model-thinking-config.test.ts
+++ b/src/agents/athena/model-thinking-config.test.ts
@@ -52,4 +52,30 @@ describe("applyModelThinkingConfig", () => {
      expect(result).toBe(BASE_CONFIG)
    })
  })
+
+  describe("given a Claude model through a non-Anthropic provider", () => {
+    it("returns thinking config for github-copilot/claude-opus-4-6", () => {
+      const result = applyModelThinkingConfig(BASE_CONFIG, "github-copilot/claude-opus-4-6")
+      expect(result).toEqual({
+        ...BASE_CONFIG,
+        thinking: { type: "enabled", budgetTokens: 32000 },
+      })
+    })
+
+    it("returns thinking config for opencode/claude-opus-4-6", () => {
+      const result = applyModelThinkingConfig(BASE_CONFIG, "opencode/claude-opus-4-6")
+      expect(result).toEqual({
+        ...BASE_CONFIG,
+        thinking: { type: "enabled", budgetTokens: 32000 },
+      })
+    })
+
+    it("returns thinking config for opencode/claude-sonnet-4-6", () => {
+      const result = applyModelThinkingConfig(BASE_CONFIG, "opencode/claude-sonnet-4-6")
+      expect(result).toEqual({
+        ...BASE_CONFIG,
+        thinking: { type: "enabled", budgetTokens: 32000 },
+      })
+    })
+  })
 })
--- a/src/agents/athena/model-thinking-config.ts
+++ b/src/agents/athena/model-thinking-config.ts
@@ -1,4 +1,5 @@
 import type { AgentConfig } from "@opencode-ai/sdk"
+import { parseModelString } from "../../tools/delegate-task/model-string-parser"
 import { isGptModel } from "../types"

 export function applyModelThinkingConfig(base: AgentConfig, model: string): AgentConfig {
@@ -6,10 +7,12 @@ export function applyModelThinkingConfig(base: AgentConfig, model: string): Agen
    return { ...base, reasoningEffort: "medium" }
  }

-  const slashIndex = model.indexOf("/")
-  const provider = slashIndex > 0 ? model.substring(0, slashIndex).toLowerCase() : ""
+  const parsed = parseModelString(model)
+  if (!parsed) {
+    return base
+  }

-  if (provider === "anthropic") {
+  if (parsed.providerID.toLowerCase() === "anthropic" || parsed.modelID.startsWith("claude")) {
    return { ...base, thinking: { type: "enabled", budgetTokens: 32000 } }
  }

--- a/src/agents/builtin-agents.ts
+++ b/src/agents/builtin-agents.ts
@@ -13,7 +13,6 @@ import { createAtlasAgent, atlasPromptMetadata } from "./atlas"
 import { createMomusAgent, momusPromptMetadata } from "./momus"
 import { createHephaestusAgent } from "./hephaestus"
 import { createAthenaAgent, ATHENA_PROMPT_METADATA } from "./athena/agent"
-import { createCouncilMemberAgent } from "./athena/council-member-agent"
 import type { AvailableCategory } from "./dynamic-agent-prompt-builder"
 import {
  fetchAvailableModels,
@@ -45,7 +44,6 @@ const agentSources: Partial<Record<BuiltinAgentName, AgentSource>> = {
  metis: createMetisAgent,
  momus: createMomusAgent,
  athena: createAthenaAgent,
-  "council-member": createCouncilMemberAgent,
  // Note: Atlas is handled specially in createBuiltinAgents()
  // because it needs OrchestratorContext, not just a model string
  atlas: createAtlasAgent as AgentFactory,
@@ -211,7 +209,14 @@ export async function createBuiltinAgents(

    if (registeredKeys.length > 0) {
      const memberList = registeredKeys.map((key) => `- "${key}"`).join("\n")
-      const councilTaskInstructions = `\n\n## Registered Council Members\n\nUse these as subagent_type in task calls:\n\n${memberList}`
+      let councilTaskInstructions = `\n\n## Registered Council Members\n\nUse these as subagent_type in task calls:\n\n${memberList}`
+
+      if (skippedMembers.length > 0) {
+        const skipDetails = skippedMembers.map((m) => `- **${m.name}**: ${m.reason}`).join("\n")
+        councilTaskInstructions += `\n\n> **Note**: Some configured council members were skipped:\n${skipDetails}`
+        log("[builtin-agents] Some council members were skipped during registration", { skippedMembers })
+      }
+
      result["athena"] = {
        ...result["athena"],
        prompt: (result["athena"].prompt ?? "") + councilTaskInstructions,
--- a/src/agents/builtin-agents/athena-council-guard.ts
+++ b/src/agents/builtin-agents/athena-council-guard.ts
@@ -39,14 +39,15 @@ Each member requires \`model\` (\`"provider/model-id"\` format) and \`name\` (di
 After informing the user, **end your turn**. Do NOT try to work around this by using generic agents, the council-member agent, or any other fallback.`

 /**
- * Replaces Athena's prompt with a guard that tells the user to configure council members.
+ * Replaces Athena's orchestration prompt with a guard that tells the user to configure council members.
+ * The original prompt is discarded to avoid contradictory instructions.
 * Used when Athena is registered but no valid council config exists.
 */
 export function appendMissingCouncilPrompt(
  athenaConfig: AgentConfig,
  skippedMembers?: Array<{ name: string; reason: string }>,
 ): AgentConfig {
-  let prompt = (athenaConfig.prompt ?? "") + MISSING_COUNCIL_PROMPT
+  let prompt = MISSING_COUNCIL_PROMPT

  if (skippedMembers && skippedMembers.length > 0) {
    const skipDetails = skippedMembers.map((m) => `- **${m.name}**: ${m.reason}`).join("\n")
--- a/src/agents/builtin-agents/council-member-agents.ts
+++ b/src/agents/builtin-agents/council-member-agents.ts
@@ -27,6 +27,7 @@ export function registerCouncilMemberAgents(
  const agents: Record<string, AgentConfig> = {}
  const registeredKeys: string[] = []
  const skippedMembers: SkippedMember[] = []
+  const registeredNamesLower = new Set<string>()

  for (const member of councilConfig.members) {
    const parsed = parseModelString(member.model)
@@ -40,16 +41,16 @@ export function registerCouncilMemberAgents(
    }

    const key = getCouncilMemberAgentKey(member)
+    const nameLower = member.name.toLowerCase()

-    if (agents[key]) {
+    if (registeredNamesLower.has(nameLower)) {
      skippedMembers.push({
        name: member.name,
-        reason: `Duplicate name: '${member.name}' already registered`,
+        reason: `Duplicate name: '${member.name}' already registered (case-insensitive match)`,
      })
      log("[council-member-agents] Skipping duplicate council member name", {
        name: member.name,
        model: member.model,
-        existingModel: agents[key].model ?? "unknown",
      })
      continue
    }
@@ -66,6 +67,7 @@ export function registerCouncilMemberAgents(
    }

    registeredKeys.push(key)
+    registeredNamesLower.add(nameLower)

    log("[council-member-agents] Registered council member agent", {
      key,
--- a/src/config/schema/athena.test.ts
+++ b/src/config/schema/athena.test.ts
@@ -43,7 +43,7 @@ describe("CouncilMemberSchema", () => {

  test("rejects model string without provider/model separator", () => {
    //#given
-    const config = { model: "invalid-model" }
+    const config = { model: "invalid-model", name: "test-member" }

    //#when
    const result = CouncilMemberSchema.safeParse(config)
@@ -54,7 +54,7 @@ describe("CouncilMemberSchema", () => {

  test("rejects model string with empty provider", () => {
    //#given
-    const config = { model: "/gpt-5.3-codex" }
+    const config = { model: "/gpt-5.3-codex", name: "test-member" }

    //#when
    const result = CouncilMemberSchema.safeParse(config)
@@ -65,7 +65,7 @@ describe("CouncilMemberSchema", () => {

  test("rejects model string with empty model ID", () => {
    //#given
-    const config = { model: "openai/" }
+    const config = { model: "openai/", name: "test-member" }

    //#when
    const result = CouncilMemberSchema.safeParse(config)
@@ -151,7 +151,7 @@ describe("CouncilMemberSchema", () => {

  test("rejects temperature below 0", () => {
    //#given
-    const config = { model: "openai/gpt-5.3-codex", temperature: -0.1 }
+    const config = { model: "openai/gpt-5.3-codex", name: "test-member", temperature: -0.1 }

    //#when
    const result = CouncilMemberSchema.safeParse(config)
@@ -162,7 +162,7 @@ describe("CouncilMemberSchema", () => {

  test("rejects temperature above 2", () => {
    //#given
-    const config = { model: "openai/gpt-5.3-codex", temperature: 2.1 }
+    const config = { model: "openai/gpt-5.3-codex", name: "test-member", temperature: 2.1 }

    //#when
    const result = CouncilMemberSchema.safeParse(config)
@@ -173,7 +173,7 @@ describe("CouncilMemberSchema", () => {

  test("rejects member config with unknown fields", () => {
    //#given
-    const config = { model: "openai/gpt-5.3-codex", unknownField: true }
+    const config = { model: "openai/gpt-5.3-codex", name: "test-member", unknownField: true }

    //#when
    const result = CouncilMemberSchema.safeParse(config)
--- a/src/hooks/agent-switch/fallback-handoff.ts
+++ b/src/hooks/agent-switch/fallback-handoff.ts
@@ -51,17 +51,34 @@ export function extractTextPartsFromMessageResponse(response: unknown): string {
    .join("\n")
 }

-export function detectFallbackHandoffTarget(messageText: string): "atlas" | "prometheus" | undefined {
+const HANDOFF_TARGETS = ["prometheus", "atlas"] as const
+type HandoffTarget = (typeof HANDOFF_TARGETS)[number]
+
+const HANDOFF_VERBS = [
+  "switching",
+  "handing\\s+off",
+  "delegating",
+  "routing",
+  "transferring",
+  "passing",
+]
+
+function buildHandoffPattern(target: string): RegExp {
+  const verbGroup = HANDOFF_VERBS.join("|")
+  return new RegExp(
+    `(?<!\\bnot\\s+)(?<!\\bdon'?t\\s+)(?<!\\bnever\\s+)(?:${verbGroup})\\s+(?:(?:control|this|it|work)\\s+)?to\\s+\\*{0,2}\\s*${target}\\b`
+  )
+}
+
+export function detectFallbackHandoffTarget(messageText: string): HandoffTarget | undefined {
  if (!messageText) return undefined

  const normalized = messageText.toLowerCase()

-  if (/switching\s+to\s+\*{0,2}\s*prometheus\b/.test(normalized) || /handing\s+off\s+to\s+\*{0,2}\s*prometheus\b/.test(normalized)) {
-    return "prometheus"
-  }
-
-  if (/switching\s+to\s+\*{0,2}\s*atlas\b/.test(normalized) || /handing\s+off\s+to\s+\*{0,2}\s*atlas\b/.test(normalized)) {
-    return "atlas"
+  for (const target of HANDOFF_TARGETS) {
+    if (buildHandoffPattern(target).test(normalized)) {
+      return target
+    }
  }

  return undefined
--- a/src/hooks/agent-switch/hook.ts
+++ b/src/hooks/agent-switch/hook.ts
@@ -14,6 +14,15 @@ import {
 const processedFallbackMessages = new Set<string>()
 const MAX_PROCESSED_FALLBACK_MARKERS = 500

+function clearFallbackMarkersForSession(sessionID: string): void {
+  clearPendingSwitchRuntime(sessionID)
+  for (const key of Array.from(processedFallbackMessages)) {
+    if (key.startsWith(`${sessionID}:`)) {
+      processedFallbackMessages.delete(key)
+    }
+  }
+}
+
 function getSessionIDFromStatusEvent(input: { event: { properties?: Record<string, unknown> } }): string | undefined {
  const props = input.event.properties as Record<string, unknown> | undefined
  const fromProps = typeof props?.sessionID === "string" ? props.sessionID : undefined
@@ -46,12 +55,7 @@ export function createAgentSwitchHook(ctx: PluginInput) {
        const info = props?.info as Record<string, unknown> | undefined
        const deletedSessionID = info?.id
        if (typeof deletedSessionID === "string") {
-          clearPendingSwitchRuntime(deletedSessionID)
-          for (const key of Array.from(processedFallbackMessages)) {
-            if (key.startsWith(`${deletedSessionID}:`)) {
-              processedFallbackMessages.delete(key)
-            }
-          }
+          clearFallbackMarkersForSession(deletedSessionID)
        }
        return
      }
@@ -61,12 +65,7 @@ export function createAgentSwitchHook(ctx: PluginInput) {
        const info = props?.info as Record<string, unknown> | undefined
        const erroredSessionID = info?.id ?? props?.sessionID
        if (typeof erroredSessionID === "string") {
-          clearPendingSwitchRuntime(erroredSessionID)
-          for (const key of Array.from(processedFallbackMessages)) {
-            if (key.startsWith(`${erroredSessionID}:`)) {
-              processedFallbackMessages.delete(key)
-            }
-          }
+          clearFallbackMarkersForSession(erroredSessionID)
        }
        return
      }
--- a/src/plugin/tool-execute-before.test.ts
+++ b/src/plugin/tool-execute-before.test.ts
@@ -16,7 +16,7 @@ describe("createToolExecuteBeforeHandler", () => {

    const backgroundManager = {
      getTasksByParentSession: () => [
-        { agent: "council-member", status: "running" },
+        { agent: "Council: Claude Opus 4.6", status: "running" },
      ],
    }

@@ -50,7 +50,7 @@ describe("createToolExecuteBeforeHandler", () => {

    const backgroundManager = {
      getTasksByParentSession: () => [
-        { agent: "council-member", status: "pending" },
+        { agent: "Council: GPT 5.2", status: "pending" },
      ],
    }

@@ -84,8 +84,8 @@ describe("createToolExecuteBeforeHandler", () => {

    const backgroundManager = {
      getTasksByParentSession: () => [
-        { agent: "council-member", status: "completed" },
-        { agent: "council-member", status: "cancelled" },
+        { agent: "Council: Claude Opus 4.6", status: "completed" },
+        { agent: "Council: GPT 5.2", status: "cancelled" },
      ],
    }

--- a/src/plugin/tool-execute-before.ts
+++ b/src/plugin/tool-execute-before.ts
@@ -28,7 +28,7 @@ export function createToolExecuteBeforeHandler(args: {

    const tasks = backgroundManager.getTasksByParentSession(sessionID)
    return tasks.some((task) =>
-      (task.agent === "council-member" || task.agent.startsWith(COUNCIL_MEMBER_KEY_PREFIX)) &&
+      task.agent.startsWith(COUNCIL_MEMBER_KEY_PREFIX) &&
      (task.status === "pending" || task.status === "running")
    )
  }