diff --git a/.claude/skills/prod-telemetry/SKILL.md b/.claude/skills/prod-telemetry/SKILL.md index 47f79bbc75..33dae496d5 100644 --- a/.claude/skills/prod-telemetry/SKILL.md +++ b/.claude/skills/prod-telemetry/SKILL.md @@ -29,6 +29,15 @@ join the same traces via traceparent). **Span names worth querying** (and their custom attrs): +- `mcp.execute` / `mcp.execute.resume` — `mcp.execute.mode` + (`pausable`/`inline`), `mcp.execute.code_length`, and + `mcp.execute.outcome` (`ok`/`fail`/`paused`) with, on failures, + `mcp.execute.error_kind` (`type_error` | `reference_error` | + `syntax_error` | `range_error` | `tool_error` | `timeout` | + `resource_limit` | `serialization_error` | `thrown` | `unknown`). + Sandbox script failures ride the MCP success channel, so `status.code` + stays OK — filter on these attributes, not span status. Spans from + before the attributes shipped carry neither; absence is not success. - `executor.tool.execute` — `mcp.tool.name` (full address), and since PR #992: `executor.tool.outcome` (`ok`/`fail`), `executor.tool.error_code`, `executor.tool.error_status`, diff --git a/packages/core/execution/src/engine.ts b/packages/core/execution/src/engine.ts index e5d2b07587..fe10b3d7a9 100644 --- a/packages/core/execution/src/engine.ts +++ b/packages/core/execution/src/engine.ts @@ -62,6 +62,25 @@ export type ResumeResponse = { // caller is itself the human approver (the operator-facing Run/Test panel). const acceptAllHandler: ElicitationHandler = () => Effect.succeed({ action: "accept" }); +/** + * Stamp the current `mcp.execute` / `mcp.execute.resume` span with how the + * execution ended. Sandbox failures ride the success channel as + * `ExecuteResult.error`, so without this the span reads OK and the failure + * class is unqueryable. Attributes stay enumerable identifiers — never the + * error message itself. + */ +const annotateExecuteOutcome = (result: ExecuteResult) => + Effect.annotateCurrentSpan( + result.error + ? { "mcp.execute.outcome": "fail", "mcp.execute.error_kind": result.errorKind ?? "unknown" } + : { "mcp.execute.outcome": "ok" }, + ); + +const annotateExecutionOutcome = (execution: ExecutionResult) => + execution.status === "paused" + ? Effect.annotateCurrentSpan({ "mcp.execute.outcome": "paused" }) + : annotateExecuteOutcome(execution.result); + // --------------------------------------------------------------------------- // Result formatting // --------------------------------------------------------------------------- @@ -529,6 +548,7 @@ export const createExecutionEngine = Effect.gen(function* () { recordSettledOutcome(executionId, exit); @@ -649,6 +675,8 @@ export const createExecutionEngine = { + it("maps built-in JS error names to their kinds", () => { + expect(classifyThrownExecuteError("SyntaxError")).toBe("syntax_error"); + expect(classifyThrownExecuteError("TypeError")).toBe("type_error"); + expect(classifyThrownExecuteError("ReferenceError")).toBe("reference_error"); + expect(classifyThrownExecuteError("RangeError")).toBe("range_error"); + expect(classifyThrownExecuteError("DataCloneError")).toBe("serialization_error"); + }); + + it("maps runtime-tagged names to their kinds", () => { + expect(classifyThrownExecuteError("ExecutionToolError")).toBe("tool_error"); + expect(classifyThrownExecuteError("ExecutionTimeoutError")).toBe("timeout"); + }); + + it("treats anything else as the script's own throw", () => { + expect(classifyThrownExecuteError("Error")).toBe("thrown"); + expect(classifyThrownExecuteError("CustomDomainError")).toBe("thrown"); + expect(classifyThrownExecuteError(null)).toBe("thrown"); + expect(classifyThrownExecuteError(undefined)).toBe("thrown"); + }); +}); diff --git a/packages/kernel/core/src/error-kind.ts b/packages/kernel/core/src/error-kind.ts new file mode 100644 index 0000000000..64361ffe3d --- /dev/null +++ b/packages/kernel/core/src/error-kind.ts @@ -0,0 +1,37 @@ +/** + * Enumerable classification of a failed sandbox execution, derived from the + * structured error the runtime already holds (the thrown error's `name`, or + * the runtime's own typed failure). Carried on `ExecuteResult.errorKind` + * beside the descriptive `error` string so telemetry can count failure + * classes as identifiers without ever recording message content. + */ +export type ExecuteErrorKind = + | "syntax_error" + | "type_error" + | "reference_error" + | "range_error" + | "tool_error" + | "timeout" + | "resource_limit" + | "serialization_error" + | "thrown"; + +const KIND_BY_ERROR_NAME: Readonly> = { + SyntaxError: "syntax_error", + TypeError: "type_error", + ReferenceError: "reference_error", + RangeError: "range_error", + ExecutionToolError: "tool_error", + ExecutionTimeoutError: "timeout", + DataCloneError: "serialization_error", +}; + +/** + * Classify an error thrown inside the sandbox by its `name`. Runtimes tag + * their own conditions with dedicated names (`ExecutionToolError` for a + * failed tool-dispatch rethrow, `ExecutionTimeoutError` for the in-sandbox + * deadline); anything unrecognized is the script's own `throw`. + */ +export const classifyThrownExecuteError = ( + errorName: string | null | undefined, +): ExecuteErrorKind => (errorName != null ? KIND_BY_ERROR_NAME[errorName] : undefined) ?? "thrown"; diff --git a/packages/kernel/core/src/index.ts b/packages/kernel/core/src/index.ts index dcc792fa93..49c969af7a 100644 --- a/packages/kernel/core/src/index.ts +++ b/packages/kernel/core/src/index.ts @@ -1,4 +1,5 @@ export * from "./types"; +export * from "./error-kind"; export * from "./validation"; export * from "./json-schema"; export * from "./effect-errors"; diff --git a/packages/kernel/core/src/types.ts b/packages/kernel/core/src/types.ts index 14679424d7..1480b27b9a 100644 --- a/packages/kernel/core/src/types.ts +++ b/packages/kernel/core/src/types.ts @@ -3,6 +3,7 @@ import type * as Cause from "effect/Cause"; import type * as Effect from "effect/Effect"; import type { CodeExecutionError } from "./effect-errors"; +import type { ExecuteErrorKind } from "./error-kind"; /** Branded tool path */ export type ToolPath = string & { readonly __toolPath: unique symbol }; @@ -42,6 +43,8 @@ export type ExecuteResult = { result: unknown; output?: ExecuteOutputItem[]; error?: string; + /** Enumerable failure class for telemetry; never carries message content. */ + errorKind?: ExecuteErrorKind; logs?: string[]; }; diff --git a/packages/kernel/runtime-dynamic-worker/src/executor.ts b/packages/kernel/runtime-dynamic-worker/src/executor.ts index 7128e2e971..e03f2aca8c 100644 --- a/packages/kernel/runtime-dynamic-worker/src/executor.ts +++ b/packages/kernel/runtime-dynamic-worker/src/executor.ts @@ -14,12 +14,14 @@ import * as Data from "effect/Data"; import * as Effect from "effect/Effect"; import { + classifyThrownExecuteError, CodeCompilationError, recoverExecutionBody, SandboxHostTimeoutError, SandboxRuntimeError, stripTypeScript, type CodeExecutor, + type ExecuteErrorKind, type ExecuteOutputItem, type ExecuteResult, type SandboxToolInvoker, @@ -231,6 +233,23 @@ const RUNTIME_SIGNATURES = [ export type SandboxFailureKind = "compilation" | "runtime" | "internal"; +/** + * Signatures of the serialization subset of `RUNTIME_SIGNATURES`, used to + * split `SandboxRuntimeError` into its two telemetry classes: the user + * returned something that can't cross the sandbox boundary vs the isolate + * hit a CPU/memory/capacity limit. + */ +const SERIALIZATION_SIGNATURES = [ + "could not be cloned", + "does not support serialization", + "Could not serialize", +] as const; + +const sandboxRuntimeErrorKind = (message: string): ExecuteErrorKind => + SERIALIZATION_SIGNATURES.some((signature) => message.includes(signature)) + ? "serialization_error" + : "resource_limit"; + /** * Classify a sandbox rejection so the runtime knows whether to surface * its message descriptively (the user's mistake or a transient, @@ -785,6 +804,9 @@ const evaluate = ( return { result: error ? null : response.result, error, + ...(response.error + ? { errorKind: classifyThrownExecuteError(serializedErrorName(response.error.primary)) } + : {}), output: Array.isArray(response.output) && response.output.length > 0 ? response.output : undefined, logs: response.logs, @@ -812,16 +834,28 @@ const runInDynamicWorker = ( // `DynamicWorkerExecutionError` and remain opaque. Effect.catchTags({ CodeCompilationError: (error) => - Effect.succeed({ result: null, error: error.message } satisfies ExecuteResult), + Effect.succeed({ + result: null, + error: error.message, + errorKind: "syntax_error", + } satisfies ExecuteResult), SandboxRuntimeError: (error) => - Effect.succeed({ result: null, error: error.message } satisfies ExecuteResult), + Effect.succeed({ + result: null, + error: error.message, + errorKind: sandboxRuntimeErrorKind(error.message), + } satisfies ExecuteResult), // A wedged isolate that the in-sandbox timer failed to catch is // unrecoverable, but reporting it as a delivered, descriptive error beats // the original symptom (open-ended silence). Fold it into the success // channel like the other safe-to-report conditions so it reaches the // model instead of collapsing to an opaque internal error. SandboxHostTimeoutError: (error) => - Effect.succeed({ result: null, error: error.message } satisfies ExecuteResult), + Effect.succeed({ + result: null, + error: error.message, + errorKind: "timeout", + } satisfies ExecuteResult), }), Effect.withSpan("executor.code.exec.dynamic_worker", { attributes: { "executor.runtime": "dynamic-worker" }, diff --git a/packages/kernel/runtime-dynamic-worker/src/invocation.test.ts b/packages/kernel/runtime-dynamic-worker/src/invocation.test.ts index 336866780e..17c47486d9 100644 --- a/packages/kernel/runtime-dynamic-worker/src/invocation.test.ts +++ b/packages/kernel/runtime-dynamic-worker/src/invocation.test.ts @@ -283,6 +283,7 @@ describe("makeDynamicWorkerExecutor", () => { ); expect(result.error).toBe('{"code":"bad_request","detail":"team missing"}'); + expect(result.errorKind).toBe("thrown"); expect(result.result).toBeNull(); }); @@ -304,6 +305,7 @@ describe("makeDynamicWorkerExecutor", () => { ); expect(result.error).toBeUndefined(); + expect(result.errorKind).toBeUndefined(); expect(result.result).toBe(7); }); @@ -316,6 +318,25 @@ describe("makeDynamicWorkerExecutor", () => { ); expect(result.error).toBe("Internal tool error"); + expect(result.errorKind).toBe("tool_error"); + }); + + it("classifies a wrong-shape property access as a type error", async () => { + const executor = makeDynamicWorkerExecutor({ loader }); + const invoker = makeInvoker(() => ({ ok: true, data: {} })); + + // The wrong-shape-guess signature: the script assumes a response field + // that isn't there and dereferences undefined. + const result = await Effect.runPromise( + executor.execute( + "async () => { const r = await tools.issues.list({}); return r.issues.length; }", + invoker, + ), + ); + + expect(result.result).toBeNull(); + expect(result.error).toContain("undefined"); + expect(result.errorKind).toBe("type_error"); }); it("surfaces a syntax error with the parser's descriptive message, not an opaque generic", async () => { @@ -335,6 +356,7 @@ describe("makeDynamicWorkerExecutor", () => { expect(result.error).not.toBe("Internal tool error"); expect(result.error).not.toContain("Internal tool error"); expect(result.error?.toLowerCase()).toContain("unexpected"); + expect(result.errorKind).toBe("syntax_error"); }); it("surfaces smart-quote paste errors descriptively", async () => { @@ -369,6 +391,7 @@ describe("makeDynamicWorkerExecutor", () => { expect(result.error).not.toBe("Internal tool error"); expect(result.error).not.toContain("Internal tool error"); expect(result.error?.toLowerCase()).toContain("could not be cloned"); + expect(result.errorKind).toBe("serialization_error"); }); it("preserves public ExecutionToolError messages across the worker bridge", async () => { @@ -390,6 +413,7 @@ describe("makeDynamicWorkerExecutor", () => { expect(result.error).toBe( "tools.search expects an object: { query?: string; namespace?: string; limit?: number; offset?: number }", ); + expect(result.errorKind).toBe("tool_error"); }); it("does not expose host error stack details to sandbox error handlers", async () => { @@ -518,6 +542,7 @@ describe("makeDynamicWorkerExecutor", () => { expect(result.result).toBeNull(); expect(result.error).toBe(`Execution timed out after ${timeoutMs}ms`); + expect(result.errorKind).toBe("timeout"); }); it("resets the execution deadline after a tool dispatch returns", async () => { @@ -540,6 +565,7 @@ describe("makeDynamicWorkerExecutor", () => { expect(result.result).toBeNull(); expect(result.error).toBe(`Execution timed out after ${timeoutMs}ms`); + expect(result.errorKind).toBe("timeout"); }); it("returns an execution error for circular tool args", async () => { diff --git a/packages/kernel/runtime-dynamic-worker/src/module-template.ts b/packages/kernel/runtime-dynamic-worker/src/module-template.ts index 199a2c14c7..bdecdacfd3 100644 --- a/packages/kernel/runtime-dynamic-worker/src/module-template.ts +++ b/packages/kernel/runtime-dynamic-worker/src/module-template.ts @@ -207,7 +207,7 @@ export const buildExecutorModule = (body: string, timeoutMs: number): string => " __inFlight -= 1;", " __lastReturnedAt = Date.now();", " }", - " if (!data.ok) throw new Error(__publicToolErrorMessage(data.error) || 'Internal tool error');", + " if (!data.ok) throw Object.assign(new Error(__publicToolErrorMessage(data.error) || 'Internal tool error'), { name: 'ExecutionToolError' });", " return __decodeBinary(data.result);", " })();", " },", @@ -219,7 +219,7 @@ export const buildExecutorModule = (body: string, timeoutMs: number): string => " __watchdogInterval = setInterval(() => {", " const now = Date.now();", ` if (__inFlight === 0 && now - Math.max(__start, __lastReturnedAt) >= ${timeoutMs}) {`, - ` reject(new Error("Execution timed out after ${timeoutMs}ms"));`, + ` reject(Object.assign(new Error("Execution timed out after ${timeoutMs}ms"), { name: "ExecutionTimeoutError" }));`, " }", " }, 1000);", " });",