fix(review): address owner review feedback on PR #1728

Author: potb

.github/workflows/ci.yml

@@ -44,14 +44,16 @@ jobs:
         env:
           BUN_INSTALL_ALLOW_SCRIPTS: "@ast-grep/napi"
 
-      - name: Run mock-heavy tests (isolated)
-        run: |
-          # These files use mock.module() which pollutes module cache
-          # Run them in separate processes to prevent cross-file contamination
-          bun test src/plugin-handlers
-          bun test src/hooks/atlas
-          bun test src/hooks/compaction-context-injector
-          bun test src/features/tmux-subagent
+       - name: Run mock-heavy tests (isolated)
+         run: |
+           # These files use mock.module() which pollutes module cache
+           # Run them in separate processes to prevent cross-file contamination
+           bun test src/plugin-handlers
+           bun test src/hooks/atlas
+           bun test src/hooks/compaction-context-injector
+           bun test src/features/tmux-subagent
+           bun test src/shared/session-utils.test.ts
+           bun test src/tools/skill/tools.test.ts
 
       - name: Run remaining tests
         run: |

src/config/schema/oh-my-opencode-config.ts

@@ -32,9 +32,10 @@ export const OhMyOpenCodeConfigSchema = z.object({
   disabled_hooks: z.array(HookNameSchema).optional(),
   disabled_commands: z.array(BuiltinCommandNameSchema).optional(),
   /** Disable specific tools by name (e.g., ["todowrite", "todoread"]) */
-  disabled_tools: z.array(z.string()).optional(),
-  agent_display_names: z.record(z.string(), z.string()).optional(),
-  agents: AgentOverridesSchema.optional(),
+   disabled_tools: z.array(z.string()).optional(),
+   /** Map agent canonical names to custom display names (e.g., { "sisyphus": "Builder" }) */
+   agent_display_names: z.record(z.string(), z.string()).optional(),
+   agents: AgentOverridesSchema.optional(),
   categories: CategoriesConfigSchema.optional(),
   claude_code: ClaudeCodeConfigSchema.optional(),
   sisyphus_agent: SisyphusAgentConfigSchema.optional(),

src/index.ts

@@ -10,7 +10,6 @@ import { createPluginInterface } from "./plugin-interface"
 import { loadPluginConfig } from "./plugin-config"
 import { createModelCacheState } from "./plugin-state"
 import { createFirstMessageVariantGate } from "./shared/first-message-variant"
-import { initializeAgentDisplayNames } from "./shared/agent-display-names"
 import { injectServerAuthIntoClient, log } from "./shared"
 import { startTmuxCheck } from "./tools"
 
@@ -24,10 +23,6 @@ const OhMyOpenCodePlugin: Plugin = async (ctx) => {
 
   const pluginConfig = loadPluginConfig(ctx.directory, ctx)
 
-  if (pluginConfig.agent_display_names) {
-    initializeAgentDisplayNames(pluginConfig.agent_display_names)
-  }
-  
   const disabledHooks = new Set(pluginConfig.disabled_hooks ?? [])
 
   const isHookEnabled = (hookName: HookName): boolean => !disabledHooks.has(hookName)

src/plugin-config.ts

@@ -54,7 +54,7 @@ export function mergeConfigs(
   return {
     ...base,
     ...override,
-    agent_display_names: deepMerge(base.agent_display_names ?? {}, override.agent_display_names ?? {}),
+    agent_display_names: { ...(base.agent_display_names ?? {}), ...(override.agent_display_names ?? {}) },
     agents: deepMerge(base.agents, override.agents),
     categories: deepMerge(base.categories, override.categories),
     disabled_agents: [

src/plugin-handlers/agent-display-name-rekeyer.test.ts

@@ -3,15 +3,18 @@
 import { describe, test, expect, spyOn, beforeEach, afterEach } from "bun:test"
 import { rekeyAgentsByDisplayNames } from "./agent-display-name-rekeyer"
 import * as aliasModule from "../shared/agent-name-aliases"
+import * as displayNamesModule from "../shared/agent-display-names"
 import * as shared from "../shared"
 
 beforeEach(() => {
   aliasModule.resetAgentNameAliases()
+  displayNamesModule.resetAgentDisplayNames()
   spyOn(shared, "log" as any).mockImplementation(() => {})
 })
 
 afterEach(() => {
   aliasModule.resetAgentNameAliases()
+  displayNamesModule.resetAgentDisplayNames()
   ;(shared.log as any)?.mockRestore?.()
 })
 

src/plugin-handlers/agent-display-name-rekeyer.ts

@@ -1,4 +1,5 @@
 import { log } from "../shared"
+import { initializeAgentDisplayNames } from "../shared/agent-display-names"
 import {
   initializeAgentNameAliases,
   getCanonicalToRegisteredMap,
@@ -13,6 +14,8 @@ export function rekeyAgentsByDisplayNames(params: {
 
   if (!displayNames || Object.keys(displayNames).length === 0) return
 
+  initializeAgentDisplayNames(displayNames)
+
   const { warnings } = initializeAgentNameAliases(
     displayNames,
     Object.keys(agentResult),

src/shared/agent-config-integration.test.ts

@@ -1,4 +1,4 @@
-import { describe, test, expect, afterEach } from "bun:test"
+import { describe, test, expect, afterEach, beforeEach } from "bun:test"
 import { migrateAgentNames } from "./migration"
 import { getAgentDisplayName, initializeAgentDisplayNames, resetAgentDisplayNames } from "./agent-display-names"
 import { AGENT_MODEL_REQUIREMENTS } from "./model-requirements"
@@ -84,6 +84,10 @@ describe("Agent Config Integration", () => {
   })
 
   describe("Display name resolution", () => {
+    beforeEach(() => {
+      resetAgentDisplayNames()
+    })
+
     test("returns correct display names for all builtin agents", () => {
       // given - lowercase config keys
       const agents = ["sisyphus", "atlas", "prometheus", "metis", "momus", "oracle", "librarian", "explore", "multimodal-looker"]
@@ -169,6 +173,10 @@ describe("Agent Config Integration", () => {
   })
 
   describe("End-to-end config flow", () => {
+    beforeEach(() => {
+      resetAgentDisplayNames()
+    })
+
     test("old config migrates and displays correctly", () => {
       // given - old format config
       const oldConfig = {

src/shared/agent-display-names.test.ts

@@ -217,15 +217,15 @@ describe("config-aware display names", () => {
     expect(result).toBe("Sisyphus (Ultraworker)")
   })
 
-  it("clears overrides after reset", () => {
-    // given override
-    initializeAgentDisplayNames({ sisyphus: "Builder" })
+   it("clears overrides after reset", () => {
+     // given override
+     initializeAgentDisplayNames({ sisyphus: "Builder" })
 
-    // when resetAgentDisplayNames called
-    resetAgentDisplayNames()
+     // when resetAgentDisplayNames called
+     resetAgentDisplayNames()
 
-    // then getAgentDisplayName returns default
-    const result = getAgentDisplayName("sisyphus")
-    expect(result).toBe("Sisyphus (Ultraworker)")
-  })
-})
+     // then getAgentDisplayName returns default
+     const result = getAgentDisplayName("sisyphus")
+     expect(result).toBe("Sisyphus (Ultraworker)")
+   })
+})

src/shared/agent-display-names.ts

@@ -1,3 +1,8 @@
+/**
+ * Agent config keys to display names mapping.
+ * Config keys are lowercase (e.g., "sisyphus", "atlas").
+ * Display names include suffixes for UI/logs (e.g., "Sisyphus (Ultraworker)").
+ */
 export const AGENT_DISPLAY_NAMES: Record<string, string> = {
   sisyphus: "Sisyphus (Ultraworker)",
   atlas: "Atlas (Plan Execution Orchestrator)",
@@ -13,14 +18,28 @@ export const AGENT_DISPLAY_NAMES: Record<string, string> = {
 
 let userOverrides: Record<string, string> = {}
 
+/**
+ * Initialize agent display name overrides from configuration.
+ * @param overrides - Map of agent config keys to custom display names
+ */
 export function initializeAgentDisplayNames(overrides: Record<string, string>): void {
   userOverrides = overrides
 }
 
+/**
+ * Reset agent display name overrides to empty state.
+ */
 export function resetAgentDisplayNames(): void {
   userOverrides = {}
 }
 
+/**
+ * Get display name for an agent config key.
+ * Uses case-insensitive lookup for backward compatibility.
+ * Returns original key if not found.
+ * @param configKey - Agent config key (e.g., "sisyphus", "atlas")
+ * @returns Display name with UI/log suffix, or original key if not found
+ */
 export function getAgentDisplayName(configKey: string): string {
   const lowerKey = configKey.toLowerCase()
 

src/tools/call-omo-agent/tools.ts

@@ -33,16 +33,12 @@ export function createCallOmoAgent(
       log(`[call_omo_agent] Starting with agent: ${args.subagent_type}, background: ${args.run_in_background}`)
 
        // Case-insensitive agent validation - allows "Explore", "EXPLORE", "explore" etc.
-       if (
-         !ALLOWED_AGENTS.some(
-           (name) => name.toLowerCase() === toCanonical(args.subagent_type).toLowerCase(),
-         )
-       ) {
-        return `Error: Invalid agent type "${args.subagent_type}". Only ${ALLOWED_AGENTS.join(", ")} are allowed.`
-      }
+       const canonicalAgent = toCanonical(args.subagent_type)
+       if (!ALLOWED_AGENTS.includes(canonicalAgent as AllowedAgentType)) {
+         return `Error: Invalid agent type "${args.subagent_type}". Only ${ALLOWED_AGENTS.join(", ")} are allowed.`
+       }
 
-      const normalizedAgent = toCanonical(args.subagent_type).toLowerCase() as AllowedAgentType
-      args = { ...args, subagent_type: normalizedAgent }
+       args = { ...args, subagent_type: canonicalAgent as AllowedAgentType }
 
       if (args.run_in_background) {
         if (args.session_id) {