fix(providers,agent-core): refuse tool calls whose final args are not complete JSON - #171
Closed
jasonkneen wants to merge 1 commit into
Closed
jasonkneen wants to merge 1 commit into
jasonkneen wants to merge 1 commit into
Conversation
… complete JSON
Invariant: tool-call arguments are validated as a complete JSON object
before dispatch, on every stop reason.
Cause: the adapters finalized tool-call arguments with parseStreamingJson,
which never throws and salvages partial JSON (`{"path":"a.ts","cont` ->
{ path: "a.ts" }). agent-loop only refused calls on stopReason "length", so
a tool_use/stop response with truncated or malformed argument JSON executed
with salvaged args whenever the required fields survived. Separately,
validateToolArguments returned the original unvalidated args for plain JSON
schema tools when root coercion changed a non-object value that still
failed the schema.
Fix: add parseToolCallArguments (strict JSON.parse + existing escape repair;
empty text is a valid {}), used at final tool-call finalization in
anthropic-messages, openai-completions and openai-responses-shared. On
failure or a non-object result the ToolCall gets a new optional
argumentsError (salvaged args kept for display only). prepareToolCall
returns an immediate error result for such calls before prepareArguments or
validation. Replay serializers only emit id/name/arguments, so the field
never reaches the wire (covered by tests). validateToolArguments now throws
instead of returning unvalidated args.
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The reviewed changes and tests support safe approval.
Review effort: Lite
Findings: None
What changed in this PR
This PR prevents incomplete or non-object tool-call arguments from being executed and strengthens validation.
Changes:
- Adds strict final argument parsing across providers.
- Rejects invalid tool calls before execution.
- Adds replay-safety and regression tests.
| File | Summary |
|---|---|
packages/providers/test/validation.test.ts |
Tests failed argument coercion validation. |
packages/providers/test/tool-call-arguments-strict.test.ts |
Tests strict parsing and replay behavior. |
packages/providers/src/utils/validation.ts |
Rejects invalid coerced values. |
packages/providers/src/utils/json-parse.ts |
Adds strict tool-argument parsing. |
packages/providers/src/types.ts |
Adds optional argument error metadata. |
packages/providers/src/api/openai-responses-shared.ts |
Applies strict parsing and safe replay serialization. |
packages/providers/src/api/openai-completions.ts |
Applies strict parsing and safe replay serialization. |
packages/providers/src/api/anthropic-messages.ts |
Applies strict parsing and safe replay serialization. |
packages/agent-core/test/agent-loop.test.ts |
Verifies invalid calls are not executed. |
packages/agent-core/src/agent-loop.ts |
Rejects invalid tool calls before execution. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Invariant: tool-call args are validated as a complete JSON object before dispatch, on every stop reason.
packages/providers/src/utils/json-parse.tsparseStreamingJsonnever throws; it falls back to partial-json salvage and finally{}. It was used for the FINAL tool-call arguments inanthropic-messages.ts(content_block_stop, ~661),openai-completions.ts(finishBlock, ~444) andopenai-responses-shared.ts(output_item.done, ~713).packages/agent-core/src/agent-loop.ts(~244) only refuses tool calls whenstopReason === "length".tool_use/tool_callsresponse with truncated argument JSON such as{"path":"a.ts","contexecuted with salvaged{ path: "a.ts" }whenever the required fields survived (e.g. a write whose content was cut off).packages/providers/src/utils/validation.ts(~332):return validator.Check(coerced) ? coerced : args;returned the original UNVALIDATED args for plain JSON schema (MCP) tools when root coercion changed a non-object value that still failed the schema.Fix
parseToolCallArguments()injson-parse.ts: strictparseJsonWithRepair(JSON.parse plus the existing escape-only repair). Empty or whitespace text is still a valid{}(no-arg tools; Anthropic streams emptypartial_json). A parse failure or a non-object result (null, array, primitive) returns anerror, with the lenient salvage kept for display.ToolCall.argumentsError?: string(providers/src/types.ts). All three adapters use the strict parser at finalization and set the field only on failure. Streaming deltas still use the lenient parser for the live UI.agent-loop.tsprepareToolCall: whenargumentsErroris set, it returns an immediateisErrorresult ("was not executed because its arguments were incomplete or invalid JSON ... Re-issue the tool call with complete arguments"). This runs beforeprepareArgumentsand validation.argumentsErrornever appears in the replayed request. Session persistence is plain JSON, so an extra optional field is fine.validation.ts: a coerced non-object value that fails the schema now falls through to the normal validation error and throws. The coerced success path is unchanged.Test
packages/providers/test/tool-call-arguments-strict.test.ts(new) runs across anthropic-messages, openai-completions and openai-responses with mocked streams:{"path":"a.ts","contplus a normal tool-use finish:argumentsErroris set and salvaged args are{ path: "a.ts" }{}with no error[1,2]: flaggedargumentsErroris not in the outgoing requestAssertionError: expected undefined to deeply equal Any<String>/expected undefined to be defined.packages/providers/test/validation.test.ts: args"5"against schema{type:["object","integer"],minimum:10}must throw. Before the fix:AssertionError: expected [Function] to throw an error.packages/agent-core/test/agent-loop.test.ts: atoolUsemessage whose toolCall hasargumentsErrorand schema-valid salvaged args must not call execute, and must emit an error result. Before the fix:AssertionError: expected [ 'hel' ] to deeply equal [].Risk / behaviour change
arguments: {}plus an error, instead of passing the raw array or primitive through.validateToolArgumentsnow throws in one edge case where it used to return unvalidated args.git grep 'type: "toolCall"'over coding-agent/src and agent-core/src finds no code that rebuilds a ToolCall from an explicit field list; the only hit isproxy.tstoolcall_start, and its toolcall_endObject.assigns the server's toolCall, so the field carries through. No persisted-message schema usesadditionalProperties: false.output_item.done, Anthropiccontent_block_stop). If a server sends the terminal event without that per-block end, the call keeps its lenient streaming args and is not flagged. Real endpoints always send the per-block end, so this is left out of scope.Merge note
This is one of five independent PRs that edit
packages/agent-core/src/agent-loop.ts(tool-result cap, strict tool args, leak-retry transcript, turn cap, agentLoop rejection). Each is based on currentmain; whichever merges later needs a small rebase.https://claude.ai/code/session_01GUdnnHEaDThHUATSwXBpV9