luna/oh-my-opencode
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

fix(tools): return structured error objects from skill tool validation failures

Browse Source 
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 <clio-agent@sisyphuslabs.ai>
This commit is contained in:
YeonGyu-Kim
2026-05-25 10:56:51 +09:00
parent 4906d9207a
commit 19ed1b9b83
4 changed files with 139 additions and 73 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/tools/call-omo-agent/tools.test.ts
+24 -12
View File
@@ -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")
})
})
src/tools/call-omo-agent/tools.ts
+12 -3
View File
@@ -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({
src/tools/skill-mcp/tools.test.ts
+44 -33
View File
@@ -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")
})
})
src/tools/skill-mcp/tools.ts
+59 -25
View File
@@ -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<string, unknown>
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<string, string> = {}
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<string, string> = {}
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)