-
-
Notifications
You must be signed in to change notification settings - Fork 275
eval: type toolCallId at the persistence boundary instead of casting it #4351
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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`. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -521,7 +521,23 @@ function delay(ms: number): Promise<void> { | |
| } | ||
|
|
||
| type ToolSet = Record<string, any>; | ||
| type ToolCall = { toolName: string; arguments: Record<string, any> }; | ||
| type ToolCall = { | ||
| toolName: string; | ||
| arguments: Record<string, any>; | ||
| /** | ||
| * 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; | ||
|
Comment on lines
+524
to
+539
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win Add regression tests for the typed persistence path. The server changes add As per coding guidelines, 🤖 Prompt for AI AgentsSource: Coding guidelines |
||
| }; | ||
| 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<string> | ||
| ): 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 { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
Repository: MCPJam/inspector
Length of output: 38400
🏁 Script executed:
Repository: MCPJam/inspector
Length of output: 50372
🏁 Script executed:
Repository: MCPJam/inspector
Length of output: 50372
🏁 Script executed:
Repository: MCPJam/inspector
Length of output: 21964
🌐 Web query:
Search GitHub and public web sources for MCPJam inspector backend PR#1134, including its current status, merged state, and whether it changes the Convex validator for actualToolCalls/toolCallId.💡 Result:
There is no PR
#1134in the MCPJam/inspector repository [1][2]. Recent activity in that repository, such as PR#3083,#3069,#3202,#3050,#3013, and#3113, shows no connection to a PR numbered 1134 [3][4][5][6][7][8]. PR#1134was found in unrelated repositories such as panyam/mcpkit (a documentation-only PR) [9] and CoplayDev/unity-mcp [10]. Regarding the Convex validator fortoolCallIdin MCPJam, the project architecture uses hand-mirrored Zod or Convex schemas inmcpjam-backend/convex/lib/predicates.tsto maintain parity with the@mcpjam/sdk/predicatesimplementation, as the backend cannot directly import from the SDK [11]. While issues related totoolCallIdconsistency and validation have been discussed in the broader ecosystem (e.g., within theget-convex/agentrepository) [12][13][14][15], there is no evidence of a PR#1134in MCPJam modifying these validators [1][2].Citations:
🏁 Script executed:
Repository: MCPJam/inspector
Length of output: 315
🌐 Web query:
Search the exact URL and public references for https://github.com/MCPJam/mcpjam-backend/pull/1134. Report whether PR#1134exists, its title and merged state, and whether it changes the Convex eval persistence validator to accept toolCallId.💡 Result:
The pull request https://github.com/MCPJam/mcpjam-backend/pull/1134 does not exist [1][2][3][4][5][6][7][8]. Public technical references for the
mcpjam-backendrepository indicate that recent pull requests are numbered significantly lower (e.g., in the 600s–700s range) [2][3][4][6][7][8]. There is no information supporting the existence of PR#1134or any specific change related to a "Convex eval persistence validator" accepting atoolCallId.Citations:
Require the backend validator before publishing
@mcpjam/inspector.publish-packagesstill allows publication whendeploy_backend_prod=false; in that case,deploy-backend-prodis skipped. Make the backend deployment mandatory for this changeset, or requiredeploy_backend_prod=truefor this release, sotoolCallIdcannot reach npm before the Convex validator accepts it.🤖 Prompt for AI Agents