From da91c535365e6754c16980799664dba54e5094d6 Mon Sep 17 00:00:00 2001 From: Brandon Webb Date: Thu, 26 Mar 2026 10:31:57 -0400 Subject: [PATCH] fix(call-omo-agent): address cubic review findings and add requirement-based tests - Fix agent-resolver.ts: add defensive validation on agent name (typeof, trim, filter) - Fix tools.test.ts: correct mock to return {data: agents} matching SDK contract - Fix agent-config-handler.ts: include opencode global/project agents in customAgentSummaries - Add agent-resolver.test.ts: 14 requirement-based tests covering R1-R7 behavioral specs - Add tools-edge-cases.test.ts: 5 integration tests for rollback, whitespace, dedup, session_id --- src/plugin-handlers/agent-config-handler.ts | 7 +- .../call-omo-agent/agent-resolver.test.ts | 236 ++++++++++++++++++ src/tools/call-omo-agent/agent-resolver.ts | 4 +- .../call-omo-agent/tools-edge-cases.test.ts | 222 ++++++++++++++++ src/tools/call-omo-agent/tools.test.ts | 2 +- 5 files changed, 465 insertions(+), 6 deletions(-) create mode 100644 src/tools/call-omo-agent/agent-resolver.test.ts create mode 100644 src/tools/call-omo-agent/tools-edge-cases.test.ts diff --git a/src/plugin-handlers/agent-config-handler.ts b/src/plugin-handlers/agent-config-handler.ts index d644f68c6..91c711b19 100644 --- a/src/plugin-handlers/agent-config-handler.ts +++ b/src/plugin-handlers/agent-config-handler.ts @@ -96,6 +96,8 @@ export async function applyAgentConfig(params: { const includeClaudeAgents = params.pluginConfig.claude_code?.agents ?? true; const userAgents = includeClaudeAgents ? loadUserAgents() : {}; const projectAgents = includeClaudeAgents ? loadProjectAgents(params.ctx.directory) : {}; + const opencodeGlobalAgents = loadOpencodeGlobalAgents(); + const opencodeProjectAgents = loadOpencodeProjectAgents(params.ctx.directory); const rawPluginAgents = params.pluginComponents.agents; const pluginAgents = Object.fromEntries( @@ -113,6 +115,8 @@ export async function applyAgentConfig(params: { ...Object.entries(configAgent ?? {}), ...Object.entries(userAgents), ...Object.entries(projectAgents), + ...Object.entries(opencodeGlobalAgents), + ...Object.entries(opencodeProjectAgents), ...Object.entries(pluginAgents).filter(([, config]) => config !== undefined), ] .filter(([, config]) => config != null) @@ -139,9 +143,6 @@ export async function applyAgentConfig(params: { disableOmoEnv, ); - const opencodeGlobalAgents = loadOpencodeGlobalAgents(); - const opencodeProjectAgents = loadOpencodeProjectAgents(params.ctx.directory); - const disabledAgentNames = new Set( (migratedDisabledAgents ?? []).map(a => a.toLowerCase()) ); diff --git a/src/tools/call-omo-agent/agent-resolver.test.ts b/src/tools/call-omo-agent/agent-resolver.test.ts new file mode 100644 index 000000000..cadd0c99f --- /dev/null +++ b/src/tools/call-omo-agent/agent-resolver.test.ts @@ -0,0 +1,236 @@ +/** + * Requirement-based tests for resolveCallableAgents(). + * + * These tests are derived from behavioral requirements in the PR description + * and feature spec, NOT from reading the implementation: + * + * R1: ALLOWED_AGENTS always present as baseline + * R2: Dynamic agents from client.app.agents() merged into the result + * R3: Primary-mode agents excluded from callable list + * R4: Falls back to ALLOWED_AGENTS alone when client.app.agents() fails + * R5: All output names are lowercase + * R6: No duplicate agent names in output + * R7: Malformed agent entries (null, missing name, non-string name, whitespace-only) are skipped gracefully + */ +const { describe, test, expect, mock } = require("bun:test") +const { resolveCallableAgents } = require("./agent-resolver") +const { ALLOWED_AGENTS } = require("./constants") + +function createMockClient(agents: Array>) { + return { + app: { + agents: mock(() => Promise.resolve({ data: agents })), + }, + } +} + +function createFailingClient(error: Error = new Error("API unavailable")) { + return { + app: { + agents: mock(() => Promise.reject(error)), + }, + } +} + +describe("resolveCallableAgents", () => { + describe("#given the SDK returns agents successfully", () => { + describe("#when only built-in agents exist", () => { + test("#then every ALLOWED_AGENT appears in the result", async () => { + const builtinAgents = ALLOWED_AGENTS.map((name: string) => ({ + name, + mode: "subagent", + })) + const client = createMockClient(builtinAgents) + + const result = await resolveCallableAgents(client) + + for (const agent of ALLOWED_AGENTS) { + expect(result).toContain(agent) + } + }) + }) + + describe("#when dynamic custom agents are present alongside built-ins", () => { + test("#then custom agents are included in the result", async () => { + const agents = [ + ...ALLOWED_AGENTS.map((name: string) => ({ name, mode: "subagent" })), + { name: "bug-fixer", mode: "subagent" }, + { name: "code-reviewer", mode: "subagent" }, + ] + const client = createMockClient(agents) + + const result = await resolveCallableAgents(client) + + expect(result).toContain("bug-fixer") + expect(result).toContain("code-reviewer") + }) + + test("#then ALLOWED_AGENTS are still present", async () => { + const agents = [{ name: "custom-agent", mode: "subagent" }] + const client = createMockClient(agents) + + const result = await resolveCallableAgents(client) + + for (const agent of ALLOWED_AGENTS) { + expect(result).toContain(agent) + } + }) + }) + + describe("#when an agent has mode=primary", () => { + test("#then it is excluded from the callable list", async () => { + const agents = [ + { name: "sisyphus", mode: "primary" }, + { name: "explore", mode: "subagent" }, + ] + const client = createMockClient(agents) + + const result = await resolveCallableAgents(client) + + expect(result).not.toContain("sisyphus") + expect(result).toContain("explore") + }) + }) + + describe("#when agent names have mixed case", () => { + test("#then all output names are lowercase", async () => { + const agents = [ + { name: "Bug-Fixer", mode: "subagent" }, + { name: "CODE-REVIEWER", mode: "subagent" }, + ] + const client = createMockClient(agents) + + const result = await resolveCallableAgents(client) + + expect(result).toContain("bug-fixer") + expect(result).toContain("code-reviewer") + for (const name of result) { + expect(name).toBe(name.toLowerCase()) + } + }) + }) + + describe("#when duplicate agent names exist across sources", () => { + test("#then no duplicates appear in the result", async () => { + const agents = [ + { name: "explore", mode: "subagent" }, + { name: "explore", mode: "subagent" }, + { name: "Explore", mode: "subagent" }, + ] + const client = createMockClient(agents) + + const result = await resolveCallableAgents(client) + + const exploreCount = result.filter((n: string) => n === "explore").length + expect(exploreCount).toBe(1) + }) + }) + + describe("#when agent entries are malformed", () => { + test("#then entries with null name are skipped", async () => { + const agents = [ + { name: null, mode: "subagent" }, + { name: "explore", mode: "subagent" }, + ] + const client = createMockClient(agents) + + const result = await resolveCallableAgents(client) + + expect(result).toContain("explore") + expect(result.length).toBeGreaterThanOrEqual(ALLOWED_AGENTS.length) + }) + + test("#then entries with numeric name are skipped", async () => { + const agents = [ + { name: 42, mode: "subagent" }, + { name: "explore", mode: "subagent" }, + ] + const client = createMockClient(agents) + + const result = await resolveCallableAgents(client) + + expect(result).not.toContain("42") + expect(result).toContain("explore") + }) + + test("#then entries with whitespace-only name are skipped", async () => { + const agents = [ + { name: " ", mode: "subagent" }, + { name: "explore", mode: "subagent" }, + ] + const client = createMockClient(agents) + + const result = await resolveCallableAgents(client) + + expect(result).not.toContain("") + expect(result).not.toContain(" ") + expect(result).toContain("explore") + }) + + test("#then entries with missing name property are skipped", async () => { + const agents = [ + { mode: "subagent" }, + { name: "explore", mode: "subagent" }, + ] + const client = createMockClient(agents) + + const result = await resolveCallableAgents(client) + + expect(result).toContain("explore") + expect(result.length).toBeGreaterThanOrEqual(ALLOWED_AGENTS.length) + }) + + test("#then entries that are undefined/null themselves are skipped", async () => { + const agents = [ + null, + undefined, + { name: "explore", mode: "subagent" }, + ] as unknown as Array> + const client = createMockClient(agents) + + const result = await resolveCallableAgents(client) + + expect(result).toContain("explore") + }) + }) + + describe("#when SDK returns an empty list", () => { + test("#then ALLOWED_AGENTS still appear as the baseline", async () => { + const client = createMockClient([]) + + const result = await resolveCallableAgents(client) + + for (const agent of ALLOWED_AGENTS) { + expect(result).toContain(agent) + } + expect(result.length).toBe(ALLOWED_AGENTS.length) + }) + }) + }) + + describe("#given the SDK call fails", () => { + describe("#when client.app.agents() throws an error", () => { + test("#then it falls back to ALLOWED_AGENTS", async () => { + const client = createFailingClient(new Error("Network error")) + + const result = await resolveCallableAgents(client) + + expect(result.length).toBe(ALLOWED_AGENTS.length) + for (const agent of ALLOWED_AGENTS) { + expect(result).toContain(agent) + } + }) + + test("#then custom agents are NOT available in fallback mode", async () => { + const client = createFailingClient() + + const result = await resolveCallableAgents(client) + + expect(result).not.toContain("bug-fixer") + expect(result).not.toContain("custom-agent") + }) + }) + }) +}) + +export {} diff --git a/src/tools/call-omo-agent/agent-resolver.ts b/src/tools/call-omo-agent/agent-resolver.ts index d417be189..9f87feb79 100644 --- a/src/tools/call-omo-agent/agent-resolver.ts +++ b/src/tools/call-omo-agent/agent-resolver.ts @@ -29,8 +29,8 @@ export async function resolveCallableAgents( }); const dynamicAgents = agents - .filter((a) => a.mode !== "primary") - .map((a) => a.name.toLowerCase()); + .filter((a) => a && typeof a.name === "string" && a.name.trim().length > 0 && a.mode !== "primary") + .map((a) => a.name.trim().toLowerCase()); const merged = new Set([...ALLOWED_AGENTS, ...dynamicAgents]); return [...merged]; diff --git a/src/tools/call-omo-agent/tools-edge-cases.test.ts b/src/tools/call-omo-agent/tools-edge-cases.test.ts new file mode 100644 index 000000000..7e766c28f --- /dev/null +++ b/src/tools/call-omo-agent/tools-edge-cases.test.ts @@ -0,0 +1,222 @@ +/** + * Requirement-based integration tests for createCallOmoAgent edge cases + * introduced by the dev rebase and dynamic agent resolution feature. + * + * R1: Spawn reservation is rolled back when execution fails after reservation + * R2: Agent names with leading/trailing whitespace are trimmed before matching + * R3: An agent present in both ALLOWED_AGENTS and dynamic list is callable (no conflict) + * R4: session_id continuation rejects in background mode when session already exists + */ +const { describe, test, expect, mock, beforeEach } = require("bun:test") +const { createCallOmoAgent } = require("./tools") + +type PluginInput = { client: any; directory: string } + +function createMockCtx(agents: Array<{ name: string; mode?: string }> = []): PluginInput { + return { + client: { + app: { + agents: mock(() => Promise.resolve({ data: agents })), + }, + }, + directory: "/test", + } as unknown as PluginInput +} + +const DEFAULT_AGENTS = [ + { name: "explore", mode: "subagent" }, + { name: "librarian", mode: "subagent" }, + { name: "oracle", mode: "subagent" }, + { name: "hephaestus", mode: "subagent" }, + { name: "metis", mode: "subagent" }, + { name: "momus", mode: "subagent" }, + { name: "multimodal-looker", mode: "subagent" }, +] + +const reserveCommitMock = mock(() => 1) +const reserveRollbackMock = mock(() => {}) +const reserveSubagentSpawnMock = mock(() => Promise.resolve({ + spawnContext: { rootSessionID: "root-session", parentDepth: 0, childDepth: 1 }, + descendantCount: 1, + commit: reserveCommitMock, + rollback: reserveRollbackMock, +})) + +const toolCtx = { + sessionID: "test", + messageID: "msg", + agent: "test", + abort: new AbortController().signal, +} + +beforeEach(() => { + reserveSubagentSpawnMock.mockClear() + reserveCommitMock.mockClear() + reserveRollbackMock.mockClear() +}) + +describe("createCallOmoAgent edge cases", () => { + describe("#given spawn reservation succeeds but sync execution fails", () => { + test("#then rollback is called to release the reservation", async () => { + const mockCtx = createMockCtx(DEFAULT_AGENTS) + reserveSubagentSpawnMock.mockResolvedValueOnce({ + spawnContext: { rootSessionID: "root-session", parentDepth: 0, childDepth: 1 }, + descendantCount: 1, + commit: reserveCommitMock, + rollback: reserveRollbackMock, + }) + const mockManager = { + assertCanSpawn: mock(() => Promise.resolve(undefined)), + reserveSubagentSpawn: reserveSubagentSpawnMock, + launch: mock(() => Promise.resolve()), + getTask: mock(() => undefined), + } + const toolDef = createCallOmoAgent(mockCtx, mockManager, []) + const executeFunc = toolDef.execute as Function + + const result = await executeFunc( + { + description: "Test", + prompt: "Test prompt", + subagent_type: "explore", + run_in_background: false, + }, + toolCtx, + ) + + expect(reserveRollbackMock).toHaveBeenCalled() + expect(result).toContain("Error:") + }) + }) + + describe("#given agent names with extra whitespace from SDK", () => { + test("#then whitespace-padded names are trimmed and matched correctly", async () => { + const agents = [ + ...DEFAULT_AGENTS, + { name: " bug-fixer ", mode: "subagent" }, + ] + const mockCtx = createMockCtx(agents) + const mockManager = { + assertCanSpawn: mock(() => Promise.resolve(undefined)), + reserveSubagentSpawn: reserveSubagentSpawnMock, + launch: mock(() => Promise.resolve({ + id: "task-id", + sessionID: "ses-1", + description: "Test", + agent: "bug-fixer", + status: "pending", + })), + getTask: mock(() => ({ status: "pending", sessionID: "ses-1" })), + } + const toolDef = createCallOmoAgent(mockCtx, mockManager, []) + const executeFunc = toolDef.execute as Function + + const result = await executeFunc( + { + description: "Test", + prompt: "Fix bug", + subagent_type: "bug-fixer", + run_in_background: true, + }, + toolCtx, + ) + + expect(result).not.toContain("Invalid agent type") + }) + }) + + describe("#given an agent exists in both ALLOWED_AGENTS and dynamic results", () => { + test("#then the agent is callable without conflict", async () => { + const agents = [ + ...DEFAULT_AGENTS, + { name: "explore", mode: "subagent" }, + ] + const mockCtx = createMockCtx(agents) + const mockManager = { + assertCanSpawn: mock(() => Promise.resolve(undefined)), + reserveSubagentSpawn: reserveSubagentSpawnMock, + launch: mock(() => Promise.resolve({ + id: "task-id", + sessionID: "ses-1", + description: "Test", + agent: "explore", + status: "pending", + })), + getTask: mock(() => ({ status: "pending", sessionID: "ses-1" })), + } + const toolDef = createCallOmoAgent(mockCtx, mockManager, []) + const executeFunc = toolDef.execute as Function + + const result = await executeFunc( + { + description: "Test", + prompt: "Search codebase", + subagent_type: "explore", + run_in_background: true, + }, + toolCtx, + ) + + expect(result).not.toContain("Invalid agent type") + }) + }) + + describe("#given a disabled custom agent from dynamic resolution", () => { + test("#then disabled_agents check takes precedence over dynamic availability", async () => { + const agents = [ + ...DEFAULT_AGENTS, + { name: "bug-fixer", mode: "subagent" }, + ] + const mockCtx = createMockCtx(agents) + const mockManager = { + assertCanSpawn: mock(() => Promise.resolve(undefined)), + reserveSubagentSpawn: reserveSubagentSpawnMock, + launch: mock(() => Promise.resolve()), + getTask: mock(() => undefined), + } + const toolDef = createCallOmoAgent(mockCtx, mockManager, ["Bug-Fixer"]) + const executeFunc = toolDef.execute as Function + + const result = await executeFunc( + { + description: "Test", + prompt: "Fix bug", + subagent_type: "bug-fixer", + run_in_background: true, + }, + toolCtx, + ) + + expect(result).toContain("disabled via disabled_agents") + }) + }) + + describe("#given session_id is provided in background mode", () => { + test("#then the request is rejected with a clear error", async () => { + const mockCtx = createMockCtx(DEFAULT_AGENTS) + const mockManager = { + assertCanSpawn: mock(() => Promise.resolve(undefined)), + reserveSubagentSpawn: reserveSubagentSpawnMock, + launch: mock(() => Promise.resolve()), + getTask: mock(() => undefined), + } + const toolDef = createCallOmoAgent(mockCtx, mockManager, []) + const executeFunc = toolDef.execute as Function + + const result = await executeFunc( + { + description: "Test", + prompt: "Continue work", + subagent_type: "explore", + run_in_background: true, + session_id: "ses-existing-123", + }, + toolCtx, + ) + + expect(result).toContain("session_id is not supported in background mode") + }) + }) +}) + +export {} diff --git a/src/tools/call-omo-agent/tools.test.ts b/src/tools/call-omo-agent/tools.test.ts index 2d0d4be2f..6bbadfa28 100644 --- a/src/tools/call-omo-agent/tools.test.ts +++ b/src/tools/call-omo-agent/tools.test.ts @@ -13,7 +13,7 @@ function createMockCtx(agents: Array<{ name: string; mode?: string }> = []): Plu return { client: { app: { - agents: mock(() => Promise.resolve(agents)), + agents: mock(() => Promise.resolve({ data: agents })), }, }, directory: "/test",