From 19ed1b9b831ece285dc317722c4beff02acd15d2 Mon Sep 17 00:00:00 2001 From: YeonGyu-Kim Date: Mon, 25 May 2026 10:56:51 +0900 Subject: [PATCH] fix(tools): return structured error objects from skill tool validation failures Convert Error throws and plain-string returns in skill-mcp and call-omo-agent tools into structured objects with { output, metadata: { kind } }. call-omo-agent uses 'unsupported_agents_action' kind for agent validation errors. skill-mcp uses 'unsupported_mcp_action' kind for MCP operation validation errors. This enables callers to distinguish error responses from successful ones by checking metadata.kind rather than parsing error strings. Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus --- src/tools/call-omo-agent/tools.test.ts | 36 +++++++---- src/tools/call-omo-agent/tools.ts | 15 ++++- src/tools/skill-mcp/tools.test.ts | 77 +++++++++++++---------- src/tools/skill-mcp/tools.ts | 84 ++++++++++++++++++-------- 4 files changed, 139 insertions(+), 73 deletions(-) diff --git a/src/tools/call-omo-agent/tools.test.ts b/src/tools/call-omo-agent/tools.test.ts index 49d968225..7bd46c3ed 100644 --- a/src/tools/call-omo-agent/tools.test.ts +++ b/src/tools/call-omo-agent/tools.test.ts @@ -87,7 +87,8 @@ describe("createCallOmoAgent", () => { toolCtx ) - expect(result).toContain("disabled via disabled_agents") + expect(typeof result === "object" ? result.metadata?.kind : null).toBe("unsupported_agents_action") + expect(typeof result === "object" ? result.output : result).toContain("disabled via disabled_agents") }) test("should reject agent in disabled_agents list with case-insensitive matching", async () => { @@ -100,7 +101,8 @@ describe("createCallOmoAgent", () => { toolCtx ) - expect(result).toContain("disabled via disabled_agents") + expect(typeof result === "object" ? result.metadata?.kind : null).toBe("unsupported_agents_action") + expect(typeof result === "object" ? result.output : result).toContain("disabled via disabled_agents") }) test("should allow agent not in disabled_agents list", async () => { @@ -141,7 +143,8 @@ describe("createCallOmoAgent", () => { toolCtx ) - expect(result).toContain("subagent_type is required") + expect(typeof result === "object" ? result.metadata?.kind : null).toBe("unsupported_agents_action") + expect(typeof result === "object" ? result.output : result).toContain("subagent_type is required") }) test("should reject general even when returned by client.app.agents()", async () => { @@ -155,8 +158,10 @@ describe("createCallOmoAgent", () => { toolCtx ) - expect(result).toContain("Invalid agent type") - expect(result).toContain("Only explore, librarian are allowed") + expect(typeof result === "object" ? result.metadata?.kind : null).toBe("unsupported_agents_action") + const output = typeof result === "object" ? result.output : result + expect(output).toContain("Invalid agent type") + expect(output).toContain("Only explore, librarian are allowed") }) test("should reject unknown non-allowed agents", async () => { @@ -169,7 +174,8 @@ describe("createCallOmoAgent", () => { toolCtx ) - expect(result).toContain("Invalid agent type") + expect(typeof result === "object" ? result.metadata?.kind : null).toBe("unsupported_agents_action") + expect(typeof result === "object" ? result.output : result).toContain("Invalid agent type") }) test("should perform case-insensitive matching for allowed agents", async () => { @@ -182,7 +188,8 @@ describe("createCallOmoAgent", () => { toolCtx ) - expect(result).not.toContain("Invalid agent type") + expect(typeof result === "object" ? result.metadata?.kind : null).not.toBe("unsupported_agents_action") + expect(typeof result === "object" ? result.output : result).not.toContain("Invalid agent type") }) test("should exclude primary-mode agents from callable list", async () => { @@ -199,7 +206,8 @@ describe("createCallOmoAgent", () => { toolCtx ) - expect(result).toContain("Invalid agent type") + expect(typeof result === "object" ? result.metadata?.kind : null).toBe("unsupported_agents_action") + expect(typeof result === "object" ? result.output : result).toContain("Invalid agent type") }) test("should fall back to ALLOWED_AGENTS when client.app.agents() fails", async () => { @@ -212,7 +220,8 @@ describe("createCallOmoAgent", () => { toolCtx ) - expect(result).not.toContain("Invalid agent type") + expect(typeof result === "object" ? result.metadata?.kind : null).not.toBe("unsupported_agents_action") + expect(typeof result === "object" ? result.output : result).not.toContain("Invalid agent type") }) test("should reject unknown agent even when client.app.agents() fails (fallback mode)", async () => { @@ -225,7 +234,8 @@ describe("createCallOmoAgent", () => { toolCtx ) - expect(result).toContain("Invalid agent type") + expect(typeof result === "object" ? result.metadata?.kind : null).toBe("unsupported_agents_action") + expect(typeof result === "object" ? result.output : result).toContain("Invalid agent type") }) test("should reject non-allowed agents before disabled_agents can make them appear callable", async () => { @@ -239,8 +249,10 @@ describe("createCallOmoAgent", () => { toolCtx ) - expect(result).toContain("Invalid agent type") - expect(result).not.toContain("disabled via disabled_agents") + expect(typeof result === "object" ? result.metadata?.kind : null).toBe("unsupported_agents_action") + const output = typeof result === "object" ? result.output : result + expect(output).toContain("Invalid agent type") + expect(output).not.toContain("disabled via disabled_agents") }) }) diff --git a/src/tools/call-omo-agent/tools.ts b/src/tools/call-omo-agent/tools.ts index 2d8f4f405..318f19ca8 100644 --- a/src/tools/call-omo-agent/tools.ts +++ b/src/tools/call-omo-agent/tools.ts @@ -141,7 +141,10 @@ export function createCallOmoAgent( ); if (typeof args.subagent_type !== "string" || args.subagent_type.trim() === "") { - return "Error: subagent_type is required." + return { + output: "Error: subagent_type is required.", + metadata: { kind: "unsupported_agents_action" }, + } } const callableAgents = await resolveCallableAgents(ctx.client); @@ -153,7 +156,10 @@ export function createCallOmoAgent( (name) => name.toLowerCase() === strippedAgentType.toLowerCase(), ) ) { - return `Error: Invalid agent type "${args.subagent_type}". Only ${callableAgents.join(", ")} are allowed.`; + return { + output: `Error: Invalid agent type "${args.subagent_type}". Only ${callableAgents.join(", ")} are allowed.`, + metadata: { kind: "unsupported_agents_action" }, + } } const normalizedAgent = strippedAgentType.toLowerCase(); @@ -161,7 +167,10 @@ export function createCallOmoAgent( // Check if agent is disabled if (disabledAgents.some((disabled) => stripInvisibleAgentCharacters(disabled).toLowerCase() === normalizedAgent)) { - return `Error: Agent "${normalizedAgent}" is disabled via disabled_agents configuration. Remove it from disabled_agents in your ${CONFIG_BASENAME}.json to use it.` + return { + output: `Error: Agent "${normalizedAgent}" is disabled via disabled_agents configuration. Remove it from disabled_agents in your ${CONFIG_BASENAME}.json to use it.`, + metadata: { kind: "unsupported_agents_action" }, + } } const { model: resolvedModel, fallbackChain } = resolveModelAndFallbackChain({ diff --git a/src/tools/skill-mcp/tools.test.ts b/src/tools/skill-mcp/tools.test.ts index 07ef60993..202622bad 100644 --- a/src/tools/skill-mcp/tools.test.ts +++ b/src/tools/skill-mcp/tools.test.ts @@ -42,7 +42,7 @@ describe("skill_mcp tool", () => { }) describe("parameter validation", () => { - it("throws when no operation specified", async () => { + it("returns unsupported_mcp_action when no operation specified", async () => { // given const tool = createSkillMcpTool({ manager, @@ -50,13 +50,15 @@ describe("skill_mcp tool", () => { getSessionID: () => sessionID, }) - // when / #then - await expect( - tool.execute({ mcp_name: "test-server" }, mockContext) - ).rejects.toThrow(/Missing operation/) + // when + const result = await tool.execute({ mcp_name: "test-server" }, mockContext) + + // then + expect(typeof result === "object" && result.metadata?.kind).toBe("unsupported_mcp_action") + expect(typeof result === "object" ? result.output : result).toContain("Missing operation") }) - it("throws when multiple operations specified", async () => { + it("returns unsupported_mcp_action when multiple operations specified", async () => { // given const tool = createSkillMcpTool({ manager, @@ -64,17 +66,19 @@ describe("skill_mcp tool", () => { getSessionID: () => sessionID, }) - // when / #then - await expect( - tool.execute({ - mcp_name: "test-server", - tool_name: "some-tool", - resource_name: "some://resource", - }, mockContext) - ).rejects.toThrow(/Multiple operations/) + // when + const result = await tool.execute({ + mcp_name: "test-server", + tool_name: "some-tool", + resource_name: "some://resource", + }, mockContext) + + // then + expect(typeof result === "object" && result.metadata?.kind).toBe("unsupported_mcp_action") + expect(typeof result === "object" ? result.output : result).toContain("Multiple operations") }) - it("throws when mcp_name not found in any skill", async () => { + it("returns unsupported_mcp_action when mcp_name not found in any skill", async () => { // given loadedSkills = [ createMockSkillWithMcp("test-skill", { @@ -87,13 +91,15 @@ describe("skill_mcp tool", () => { getSessionID: () => sessionID, }) - // when / #then - await expect( - tool.execute({ mcp_name: "unknown-server", tool_name: "some-tool" }, mockContext) - ).rejects.toThrow(/not found/) + // when + const result = await tool.execute({ mcp_name: "unknown-server", tool_name: "some-tool" }, mockContext) + + // then + expect(typeof result === "object" && result.metadata?.kind).toBe("unsupported_mcp_action") + expect(typeof result === "object" ? result.output : result).toContain("not found") }) - it("includes available MCP servers in error message", async () => { + it("includes available MCP servers in unsupported_mcp_action response", async () => { // given loadedSkills = [ createMockSkillWithMcp("db-skill", { @@ -109,13 +115,16 @@ describe("skill_mcp tool", () => { getSessionID: () => sessionID, }) - // when / #then - await expect( - tool.execute({ mcp_name: "missing", tool_name: "test" }, mockContext) - ).rejects.toThrow(/sqlite.*db-skill|rest-api.*api-skill/s) + // when + const result = await tool.execute({ mcp_name: "missing", tool_name: "test" }, mockContext) + + // then + expect(typeof result === "object" && result.metadata?.kind).toBe("unsupported_mcp_action") + const output = typeof result === "object" ? result.output : result + expect(output).toMatch(/sqlite.*db-skill|rest-api.*api-skill/s) }) - it("throws on invalid JSON arguments", async () => { + it("returns unsupported_mcp_action on invalid JSON arguments", async () => { // given loadedSkills = [ createMockSkillWithMcp("test-skill", { @@ -128,14 +137,16 @@ describe("skill_mcp tool", () => { getSessionID: () => sessionID, }) - // when / #then - await expect( - tool.execute({ - mcp_name: "test-server", - tool_name: "some-tool", - arguments: "not valid json", - }, mockContext) - ).rejects.toThrow(/Invalid arguments JSON/) + // when + const result = await tool.execute({ + mcp_name: "test-server", + tool_name: "some-tool", + arguments: "not valid json", + }, mockContext) + + // then + expect(typeof result === "object" && result.metadata?.kind).toBe("unsupported_mcp_action") + expect(typeof result === "object" ? result.output : result).toContain("Invalid arguments JSON") }) }) diff --git a/src/tools/skill-mcp/tools.ts b/src/tools/skill-mcp/tools.ts index 4aec4cfb1..0a19b1e00 100644 --- a/src/tools/skill-mcp/tools.ts +++ b/src/tools/skill-mcp/tools.ts @@ -115,28 +115,45 @@ export function createSkillMcpTool(options: SkillMcpToolOptions): ToolDefinition .describe("Regex pattern to filter output lines (only matching lines returned)"), }, async execute(args: SkillMcpArgs, toolContext: ToolContext) { - const operation = validateOperationParams(args) + let operation: OperationType + try { + operation = validateOperationParams(args) + } catch (error) { + const message = error instanceof Error ? error.message : String(error) + return { + output: message, + metadata: { kind: "unsupported_mcp_action" }, + } + } const skills = getLoadedSkills() const found = findMcpServer(args.mcp_name, skills) if (!found) { const builtinHint = formatBuiltinMcpHint(args.mcp_name) if (builtinHint) { - throw new Error(builtinHint) + return { + output: builtinHint, + metadata: { kind: "unsupported_mcp_action" }, + } } - throw new Error( - `MCP server "${args.mcp_name}" not found.\n\n` + + return { + output: + `MCP server "${args.mcp_name}" not found.\n\n` + `Available MCP servers in loaded skills:\n` + formatAvailableMcps(skills) + `\n\n` + `Hint: Load the skill first using the 'skill' tool, then call skill_mcp.`, - ) + metadata: { kind: "unsupported_mcp_action" }, + } } const sessionID = toolContext.sessionID || getSessionID?.() if (!sessionID) { - throw new Error("No active session available for skill MCP call.") + return { + output: "No active session available for skill MCP call.", + metadata: { kind: "unsupported_mcp_action" }, + } } const info: SkillMcpClientInfo = { @@ -152,28 +169,45 @@ export function createSkillMcpTool(options: SkillMcpToolOptions): ToolDefinition skillName: found.skill.name, } - const parsedArgs = parseSkillMcpArguments(args.arguments) + let parsedArgs: Record + try { + parsedArgs = parseSkillMcpArguments(args.arguments) + } catch (error) { + const message = error instanceof Error ? error.message : String(error) + return { + output: message, + metadata: { kind: "unsupported_mcp_action" }, + } + } let output: string - switch (operation.type) { - case "tool": { - const result = await manager.callTool(info, context, operation.name, parsedArgs) - output = JSON.stringify(result, null, 2) - break - } - case "resource": { - const result = await manager.readResource(info, context, operation.name) - output = JSON.stringify(result, null, 2) - break - } - case "prompt": { - const stringArgs: Record = {} - for (const [key, value] of Object.entries(parsedArgs)) { - stringArgs[key] = String(value) + try { + switch (operation.type) { + case "tool": { + const result = await manager.callTool(info, context, operation.name, parsedArgs) + output = JSON.stringify(result, null, 2) + break } - const result = await manager.getPrompt(info, context, operation.name, stringArgs) - output = JSON.stringify(result, null, 2) - break + case "resource": { + const result = await manager.readResource(info, context, operation.name) + output = JSON.stringify(result, null, 2) + break + } + case "prompt": { + const stringArgs: Record = {} + for (const [key, value] of Object.entries(parsedArgs)) { + stringArgs[key] = String(value) + } + const result = await manager.getPrompt(info, context, operation.name, stringArgs) + output = JSON.stringify(result, null, 2) + break + } + } + } catch (error) { + const message = error instanceof Error ? error.message : String(error) + return { + output: message, + metadata: { kind: "unsupported_mcp_action" }, } } return applyGrepFilter(output, args.grep)