From 903cb5daa6264791a1e738f6b9841a4b62283612 Mon Sep 17 00:00:00 2001 From: marcelo Date: Mon, 24 Aug 2026 20:02:45 -0700 Subject: [PATCH] eval: type toolCallId at the persistence boundary instead of casting it Tool-policy enforcement gave the runner a reason to carry the provider's `toolCallId` on every extracted tool call -- `extractToolCallsExcludingPolicyBlocks` matches blocked calls by it. The field was attached with an `as ToolCall` cast at all three push sites, and `type ToolCall` was never widened to admit it. That cast is exactly what suppressed TypeScript's excess-property check. The same array is what gets persisted as `updateTestIteration.actualToolCalls`, where a Convex object validator that had never heard of `toolCallId` rejected it outright -- so every eval iteration that called a tool failed to record its result. Only prose-only iterations, which send `[]`, got through. This change is type-only and alters no runtime behavior; the runtime fix is the matching backend widening (MCPJam/mcpjam-backend#1134), which has to deploy first. What it buys is that the boundary cannot silently drift again: `ToolCall` and `finalize-iteration`'s `ToolCallRecord` both name `toolCallId`, the three casts are gone, and adding another undeclared field to a persisted tool call is now a compile error rather than a production ArgumentValidationError that surfaces sixty seconds later as "Worker heartbeat lost". Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_0143FPiXSfkYoPJBSQJ17v8Q --- .../eval-tool-call-id-typed-at-boundary.md | 9 +++++ .../server/services/evals-runner.ts | 36 +++++++++++++------ .../services/evals/finalize-iteration.ts | 15 ++++++-- 3 files changed, 47 insertions(+), 13 deletions(-) create mode 100644 .changeset/eval-tool-call-id-typed-at-boundary.md diff --git a/.changeset/eval-tool-call-id-typed-at-boundary.md b/.changeset/eval-tool-call-id-typed-at-boundary.md new file mode 100644 index 0000000000..bc2862cb19 --- /dev/null +++ b/.changeset/eval-tool-call-id-typed-at-boundary.md @@ -0,0 +1,9 @@ +--- +"@mcpjam/inspector": patch +--- + +`toolCallId` is now part of the runner's tool-call type instead of a cast, so the eval persistence boundary is type-checked again. + +Tool-policy enforcement gave the runner a reason to carry the provider's `toolCallId` on every extracted tool call — `extractToolCallsExcludingPolicyBlocks` matches blocked calls by it. The field was attached with an `as ToolCall` cast at all three push sites, and `type ToolCall` was never widened to admit it. That cast is precisely what suppressed TypeScript's excess-property check, and the same array is what gets persisted as `updateTestIteration.actualToolCalls` — where a Convex object validator that had never heard of `toolCallId` rejected it outright. Every eval iteration that called a tool failed to record its result; only prose-only iterations, which send `[]`, got through. + +This change is type-only and alters no runtime behavior — the runtime fix is the matching backend widening, which has to deploy first. What it buys is that the boundary can't silently drift again: `ToolCall` and `finalize-iteration`'s `ToolCallRecord` now both name `toolCallId`, the three casts are gone, and adding another undeclared field to a persisted tool call is a compile error rather than a production `ArgumentValidationError` that surfaces sixty seconds later as `Worker heartbeat lost`. diff --git a/mcpjam-inspector/server/services/evals-runner.ts b/mcpjam-inspector/server/services/evals-runner.ts index 15b5526b61..926e2a6c2e 100644 --- a/mcpjam-inspector/server/services/evals-runner.ts +++ b/mcpjam-inspector/server/services/evals-runner.ts @@ -521,7 +521,23 @@ function delay(ms: number): Promise { } type ToolSet = Record; -type ToolCall = { toolName: string; arguments: Record }; +type ToolCall = { + toolName: string; + arguments: Record; + /** + * The provider's id for this call, when the source part carried one. + * + * Declared here rather than smuggled in behind a cast: this value is BOTH + * read locally (`extractToolCallsExcludingPolicyBlocks` matches blocked + * calls by it) and persisted (`updateTestIteration.actualToolCalls`), and + * the casts that used to hide it are how it reached a Convex validator that + * had never been told about it — an unknown field there is a hard + * ArgumentValidationError, so every tool-calling iteration failed to + * finalize. Keeping it on the type is what makes the persistence boundary + * type-checked again. + */ + toolCallId?: string; +}; type TraceSnapshotKind = "step_finish" | "turn_finish" | "failure"; function getServerLabelForEvalError( @@ -972,7 +988,7 @@ function extractToolCallsFromConversation(params: { ...(typeof call.toolCallId === "string" ? { toolCallId: call.toolCallId } : {}), - } as ToolCall); + }); } } } @@ -999,7 +1015,7 @@ function extractToolCallsFromConversation(params: { ...(typeof item.toolCallId === "string" ? { toolCallId: item.toolCallId } : {}), - } as ToolCall); + }); } } } @@ -1024,7 +1040,7 @@ function extractToolCallsFromConversation(params: { ...(typeof call.toolCallId === "string" ? { toolCallId: call.toolCallId } : {}), - } as ToolCall); + }); } } } @@ -1041,13 +1057,11 @@ function extractToolCallsExcludingPolicyBlocks( }, blockedToolCallIds: ReadonlySet ): ToolCall[] { - return extractToolCallsFromConversation(params).filter((toolCall) => { - const toolCallId = (toolCall as ToolCall & { toolCallId?: unknown }) - .toolCallId; - return ( - typeof toolCallId !== "string" || !blockedToolCallIds.has(toolCallId) - ); - }); + return extractToolCallsFromConversation(params).filter( + (toolCall) => + toolCall.toolCallId === undefined || + !blockedToolCallIds.has(toolCall.toolCallId) + ); } function toolCallIdentity(toolCall: ToolCall): string { diff --git a/mcpjam-inspector/server/services/evals/finalize-iteration.ts b/mcpjam-inspector/server/services/evals/finalize-iteration.ts index 56b59828e9..efd1a389ee 100644 --- a/mcpjam-inspector/server/services/evals/finalize-iteration.ts +++ b/mcpjam-inspector/server/services/evals/finalize-iteration.ts @@ -67,7 +67,18 @@ import { isTerminalIterationStatus } from "./run-status.js"; */ type IterationStatus = ContractIterationStatus; -type ToolCallRecord = { toolName: string; arguments: Record }; +type ToolCallRecord = { + toolName: string; + arguments: Record; + /** + * Mirrors the runner's `ToolCall.toolCallId`. This type describes exactly + * what goes over the wire as `updateTestIteration.actualToolCalls`, so it + * has to name every field the runner actually sends — the whole reason + * `toolCallId` reached a validator that rejected it is that no type on this + * path admitted the field existed. + */ + toolCallId?: string; +}; type PolicyBlockRecord = { reason?: unknown }; /** @@ -596,7 +607,7 @@ export type FinalizeEvalIterationParams = { convexClient: ConvexHttpClient; iterationId?: string; passed: boolean; - toolsCalled: Array<{ toolName: string; arguments: Record }>; + toolsCalled: ToolCallRecord[]; usage: UsageTotals; messages: ModelMessage[]; /** Effective model used by the iteration; persisted on the eval session. */