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
This commit is contained in:
Brandon Webb
committed by
YeonGyu-Kim
YeonGyu-Kim
parent
76c5356a80
commit
da91c53536
@@ -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())
|
||||
);
|
||||
|
||||
@@ -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<Record<string, unknown>>) {
|
||||
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<Record<string, unknown>>
|
||||
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 {}
|
||||
@@ -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];
|
||||
|
||||
@@ -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 {}
|
||||
@@ -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",
|
||||
|
||||
Reference in New Issue
Block a user