refactor(opencode): fail the execute tool on program failure (#35180)
This commit is contained in:
@@ -192,6 +192,8 @@ const layer = Layer.effect(
|
|||||||
status: "error",
|
status: "error",
|
||||||
input: match.part.state.input,
|
input: match.part.state.input,
|
||||||
error: errorMessage(error),
|
error: errorMessage(error),
|
||||||
|
// Keep metadata streamed while running so failures retain progress detail (e.g. execute's child calls).
|
||||||
|
metadata: match.part.state.metadata,
|
||||||
time: { start: match.part.state.time.start, end: Date.now() },
|
time: { start: match.part.state.time.start, end: Date.now() },
|
||||||
},
|
},
|
||||||
})
|
})
|
||||||
|
|||||||
@@ -287,35 +287,37 @@ export const CodeModeTool = Tool.define(
|
|||||||
|
|
||||||
const result = yield* Effect.raceFirst(runtime.execute(params.code), abort.pipe(Effect.map(cancelled)))
|
const result = yield* Effect.raceFirst(runtime.execute(params.code), abort.pipe(Effect.map(cancelled)))
|
||||||
const logs = result.logs ?? []
|
const logs = result.logs ?? []
|
||||||
const attached = attachments.length > 0 ? { attachments } : {}
|
const withLogs = (text: string) => {
|
||||||
const hints = result.ok
|
if (logs.length === 0) return text
|
||||||
? []
|
return text.length > 0 ? `${text}\n\nLogs:\n${logs.join("\n")}` : `Logs:\n${logs.join("\n")}`
|
||||||
: (result.error.suggestions ?? []).filter((hint) => !result.error.message.includes(hint))
|
|
||||||
const metadata: Metadata = result.ok ? { toolCalls: calls } : { toolCalls: calls, error: true }
|
|
||||||
let output: string
|
|
||||||
if (result.ok) {
|
|
||||||
if (typeof result.value === "string") output = result.value
|
|
||||||
else if (result.value === undefined) output = "undefined"
|
|
||||||
else {
|
|
||||||
try {
|
|
||||||
output = JSON.stringify(result.value, null, 2) ?? String(result.value)
|
|
||||||
} catch {
|
|
||||||
output = String(result.value)
|
|
||||||
}
|
|
||||||
}
|
|
||||||
} else {
|
|
||||||
output = [result.error.message, ...hints].join("\n")
|
|
||||||
}
|
}
|
||||||
if (logs.length > 0)
|
|
||||||
output = output.length > 0 ? `${output}\n\nLogs:\n${logs.join("\n")}` : `Logs:\n${logs.join("\n")}`
|
if (!result.ok) {
|
||||||
|
if (ctx.abort.aborted) {
|
||||||
|
return {
|
||||||
|
title: CODE_MODE_TOOL,
|
||||||
|
metadata: { toolCalls: calls, error: true },
|
||||||
|
output: "Execution cancelled.",
|
||||||
|
} satisfies Tool.ExecuteResult<Metadata>
|
||||||
|
}
|
||||||
|
const hints = (result.error.suggestions ?? []).filter((hint) => !result.error.message.includes(hint))
|
||||||
|
return yield* Effect.fail(new Error(withLogs([result.error.message, ...hints].join("\n"))))
|
||||||
|
}
|
||||||
|
|
||||||
|
// The interpreter validates returned values as plain JSON, so stringify cannot throw;
|
||||||
|
// it yields undefined only for a program that returns undefined.
|
||||||
|
const output =
|
||||||
|
typeof result.value === "string"
|
||||||
|
? result.value
|
||||||
|
: (JSON.stringify(result.value, null, 2) ?? String(result.value))
|
||||||
|
|
||||||
return {
|
return {
|
||||||
title: CODE_MODE_TOOL,
|
title: CODE_MODE_TOOL,
|
||||||
metadata,
|
metadata: { toolCalls: calls },
|
||||||
output,
|
output: withLogs(output),
|
||||||
...attached,
|
...(attachments.length > 0 ? { attachments } : {}),
|
||||||
} satisfies Tool.ExecuteResult<Metadata>
|
} satisfies Tool.ExecuteResult<Metadata>
|
||||||
}),
|
}, Effect.orDie),
|
||||||
}
|
}
|
||||||
return init
|
return init
|
||||||
}),
|
}),
|
||||||
|
|||||||
@@ -17,7 +17,7 @@ import {
|
|||||||
ListToolsRequestSchema,
|
ListToolsRequestSchema,
|
||||||
type Tool as MCPToolDef,
|
type Tool as MCPToolDef,
|
||||||
} from "@modelcontextprotocol/sdk/types.js"
|
} from "@modelcontextprotocol/sdk/types.js"
|
||||||
import { Effect, Layer } from "effect"
|
import { Cause, Effect, Exit, Layer } from "effect"
|
||||||
|
|
||||||
const PNG = "iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVR42mNkYPhfDwAChwGA60e6kgAAAABJRU5ErkJggg=="
|
const PNG = "iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVR42mNkYPhfDwAChwGA60e6kgAAAABJRU5ErkJggg=="
|
||||||
|
|
||||||
@@ -160,6 +160,12 @@ async function buildTool() {
|
|||||||
}
|
}
|
||||||
|
|
||||||
const run = (code: string) => Effect.runPromise(tool.execute({ code }, ctx))
|
const run = (code: string) => Effect.runPromise(tool.execute({ code }, ctx))
|
||||||
|
// Program failures die at the tool boundary; recover the defect for message assertions.
|
||||||
|
const runFailed = async (code: string) => {
|
||||||
|
const exit = await Effect.runPromise(tool.execute({ code }, ctx).pipe(Effect.exit))
|
||||||
|
if (Exit.isSuccess(exit)) throw new Error("expected the tool to fail")
|
||||||
|
return Cause.squash(exit.cause) as Error
|
||||||
|
}
|
||||||
|
|
||||||
beforeAll(async () => {
|
beforeAll(async () => {
|
||||||
const built = await buildTool()
|
const built = await buildTool()
|
||||||
@@ -248,9 +254,8 @@ describe("code mode integration (real MCP server)", () => {
|
|||||||
})
|
})
|
||||||
|
|
||||||
test("an uncaught MCP error surfaces as a failed execution", async () => {
|
test("an uncaught MCP error surfaces as a failed execution", async () => {
|
||||||
const out = await run("await tools.fixtures.boom({}); return 'unreachable'")
|
const error = await runFailed("await tools.fixtures.boom({}); return 'unreachable'")
|
||||||
expect(out.metadata.error).toBe(true)
|
expect(error.message).toContain("kaboom")
|
||||||
expect(out.output).toContain("kaboom")
|
|
||||||
})
|
})
|
||||||
|
|
||||||
test("console output is captured and appended as a Logs section after the result", async () => {
|
test("console output is captured and appended as a Logs section after the result", async () => {
|
||||||
@@ -265,14 +270,13 @@ describe("code mode integration (real MCP server)", () => {
|
|||||||
})
|
})
|
||||||
|
|
||||||
test("console output is preserved on the error path", async () => {
|
test("console output is preserved on the error path", async () => {
|
||||||
const out = await run(`
|
const error = await runFailed(`
|
||||||
console.log("before the throw")
|
console.log("before the throw")
|
||||||
await tools.fixtures.boom({})
|
await tools.fixtures.boom({})
|
||||||
return "unreachable"
|
return "unreachable"
|
||||||
`)
|
`)
|
||||||
expect(out.metadata.error).toBe(true)
|
expect(error.message).toContain("kaboom")
|
||||||
expect(out.output).toContain("kaboom")
|
expect(error.message).toContain("Logs:\nbefore the throw")
|
||||||
expect(out.output).toContain("Logs:\nbefore the throw")
|
|
||||||
})
|
})
|
||||||
|
|
||||||
test("a program that logs nothing gets no Logs section", async () => {
|
test("a program that logs nothing gets no Logs section", async () => {
|
||||||
|
|||||||
@@ -10,7 +10,7 @@ import { Session } from "@/session/session"
|
|||||||
import { Tool } from "@/tool/tool"
|
import { Tool } from "@/tool/tool"
|
||||||
import * as Truncate from "@/tool/truncate"
|
import * as Truncate from "@/tool/truncate"
|
||||||
import { MessageID, SessionID } from "@/session/schema"
|
import { MessageID, SessionID } from "@/session/schema"
|
||||||
import { Effect, Layer, Schema } from "effect"
|
import { Cause, Effect, Exit, Layer, Schema } from "effect"
|
||||||
|
|
||||||
const ctx: Tool.Context = {
|
const ctx: Tool.Context = {
|
||||||
sessionID: SessionID.make("ses_code-mode"),
|
sessionID: SessionID.make("ses_code-mode"),
|
||||||
@@ -86,6 +86,13 @@ function describeFor(mcpTools: Record<string, MCP.McpTool>, servers?: string[],
|
|||||||
return describeCatalog(Permission.visibleTools(mcpTools, permission), serverNames(mcpTools, servers))
|
return describeCatalog(Permission.visibleTools(mcpTools, permission), serverNames(mcpTools, servers))
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Program failures die at the tool boundary; recover the defect for message assertions.
|
||||||
|
async function failure(effect: Effect.Effect<unknown>) {
|
||||||
|
const exit = await Effect.runPromise(effect.pipe(Effect.exit))
|
||||||
|
if (Exit.isSuccess(exit)) throw new Error("expected the tool to fail")
|
||||||
|
return Cause.squash(exit.cause) as Error
|
||||||
|
}
|
||||||
|
|
||||||
describe("code mode execute", () => {
|
describe("code mode execute", () => {
|
||||||
test("defines execute input with an Effect schema", async () => {
|
test("defines execute input with an Effect schema", async () => {
|
||||||
const decode = Schema.decodeUnknownEffect(Parameters)
|
const decode = Schema.decodeUnknownEffect(Parameters)
|
||||||
@@ -313,18 +320,16 @@ describe("code mode execute", () => {
|
|||||||
expect(output.metadata.toolCalls.every((c) => c.status === "completed")).toBe(true)
|
expect(output.metadata.toolCalls.every((c) => c.status === "completed")).toBe(true)
|
||||||
})
|
})
|
||||||
|
|
||||||
test("returns a readable error when the program throws", async () => {
|
test("a program failure fails the tool with a readable error", async () => {
|
||||||
const tool = await build({})
|
const tool = await build({})
|
||||||
const output = await Effect.runPromise(tool.execute({ code: "throw new Error('boom')" }, ctx))
|
const error = await failure(tool.execute({ code: "throw new Error('boom')" }, ctx))
|
||||||
expect(output.output).toBe("Uncaught: boom")
|
expect(error.message).toBe("Uncaught: boom")
|
||||||
expect(output.metadata.error).toBe(true)
|
|
||||||
})
|
})
|
||||||
|
|
||||||
test("reports an unknown tool as a failed execution", async () => {
|
test("reports an unknown tool as a failed execution", async () => {
|
||||||
const tool = await build({ known_tool: mcpTool("tool", () => "ok") })
|
const tool = await build({ known_tool: mcpTool("tool", () => "ok") })
|
||||||
const output = await Effect.runPromise(tool.execute({ code: "return await tools.known.missing({})" }, ctx))
|
const error = await failure(tool.execute({ code: "return await tools.known.missing({})" }, ctx))
|
||||||
expect(output.metadata.error).toBe(true)
|
expect(error.message).toContain("Unknown tool 'known.missing'")
|
||||||
expect(output.output).toContain("Unknown tool 'known.missing'")
|
|
||||||
})
|
})
|
||||||
|
|
||||||
test("propagates an MCP tool error into the program as a catchable failure", async () => {
|
test("propagates an MCP tool error into the program as a catchable failure", async () => {
|
||||||
@@ -576,8 +581,8 @@ describe("code mode execute", () => {
|
|||||||
|
|
||||||
test("isolates the sandbox from host globals", async () => {
|
test("isolates the sandbox from host globals", async () => {
|
||||||
const tool = await build({})
|
const tool = await build({})
|
||||||
const output = await Effect.runPromise(tool.execute({ code: "return process.env" }, ctx))
|
const error = await failure(tool.execute({ code: "return process.env" }, ctx))
|
||||||
expect(output.metadata.error).toBe(true)
|
expect(error.message).toContain("process")
|
||||||
})
|
})
|
||||||
|
|
||||||
test("cancelling via ctx.abort interrupts the running program", async () => {
|
test("cancelling via ctx.abort interrupts the running program", async () => {
|
||||||
@@ -627,12 +632,9 @@ describe("code mode execute", () => {
|
|||||||
)
|
)
|
||||||
expect(ok.output).toBe("done\n\nLogs:\nstep one\n[warn] careful")
|
expect(ok.output).toBe("done\n\nLogs:\nstep one\n[warn] careful")
|
||||||
|
|
||||||
const err = await Effect.runPromise(
|
const error = await failure(tool.execute({ code: "console.log('before the throw'); throw new Error('boom')" }, ctx))
|
||||||
tool.execute({ code: "console.log('before the throw'); throw new Error('boom')" }, ctx),
|
expect(error.message).toContain("Uncaught: boom")
|
||||||
)
|
expect(error.message).toContain("Logs:\nbefore the throw")
|
||||||
expect(err.metadata.error).toBe(true)
|
|
||||||
expect(err.output).toContain("Uncaught: boom")
|
|
||||||
expect(err.output).toContain("Logs:\nbefore the throw")
|
|
||||||
})
|
})
|
||||||
})
|
})
|
||||||
|
|
||||||
@@ -677,12 +679,9 @@ describe("code mode permission visibility", () => {
|
|||||||
[deny("github_create_issue")],
|
[deny("github_create_issue")],
|
||||||
)
|
)
|
||||||
|
|
||||||
const denied = await Effect.runPromise(
|
const denied = await failure(tool.execute({ code: "return await tools.github.create_issue({ title: 'x' })" }, ctx))
|
||||||
tool.execute({ code: "return await tools.github.create_issue({ title: 'x' })" }, ctx),
|
expect(denied.message).toContain("Unknown tool 'github.create_issue'")
|
||||||
)
|
expect(denied.message).not.toContain("permission")
|
||||||
expect(denied.metadata.error).toBe(true)
|
|
||||||
expect(denied.output).toContain("Unknown tool 'github.create_issue'")
|
|
||||||
expect(denied.output).not.toContain("permission")
|
|
||||||
expect(called).toEqual([])
|
expect(called).toEqual([])
|
||||||
|
|
||||||
const allowed = await Effect.runPromise(tool.execute({ code: "return await tools.github.list_issues({})" }, ctx))
|
const allowed = await Effect.runPromise(tool.execute({ code: "return await tools.github.list_issues({})" }, ctx))
|
||||||
|
|||||||
Reference in New Issue
Block a user