fix(ai): preserve executable Anthropic tool calls on terminal stop - #4759
Conversation
snowykr
left a comment
There was a problem hiding this comment.
Verdict
CHANGES_REQUESTED
Summary
This narrowly targets the correct regression—normally completed Anthropic tool_use calls should remain executable after transient partial JSON. However, the new terminal-stop relaxation also allows genuinely incomplete or integrity-compromised tool calls to cross the existing execution guard. The stream must remain fail-closed unless the tool JSON is strictly complete and the content-block lifecycle is valid.
Findings / Required Changes
-
[P1] Keep incomplete tool JSON non-executable even when a terminal stop reason is present.
packages/ai/src/providers/anthropic.ts:2405-2423, 2906-2924records incompleteinput_json_deltabuffers, but the final marking now runs only forlengthor a missing terminal stop reason. An explicittool_use(and also other explicit stops) can therefore leave best-effort-repaired incomplete JSON withoutincompleteArguments, bypassing the agent loop's execution guard. Mark every syntactically incomplete or unclosed tool JSON buffer asincompleteArguments: trueregardless of terminal reason; relax only for strictly complete JSON. Add closed- and unclosed-buffer cases with explicit terminal stops that assert non-execution. -
[P1] Do not publish executable replacement calls after a duplicate active content-block index.
packages/ai/src/providers/anthropic.ts:2055-2068, 2421, 2928now defers the duplicate-index error until finalization. That lets the replacement block emittoolcall_endbefore the stream fails, whereas the previous implementation rejected the malformed envelope immediately. Fail at duplicate-index detection, or suppress all subsequent executable tool lifecycle events and mark exposed calls non-executable before reporting the integrity error. -
[P2] Ensure affected-path PR CI runs the Anthropic streaming regression suites.
scripts/ci-dev-affected.ts:1038-1053selects same-basename tests for provider source changes, butanthropic.tshas no such test; the relevant coverage is inpackages/ai/test/anthropic-truncated-toolcall.test.tsandpackages/ai/test/anthropic-stream-envelope.test.ts. Add an explicit behavioral-owner/risk-canary mapping so this path runs those suites before merge.
CI / Verification
- No project code, tests, linters, or formatters were executed as part of this review.
- The verification review inspected the affected-path CI selection and relevant test sources; it found that the provider change does not schedule the focused Anthropic stream suites.
- No needs-human-ci failure was considered.
Axis Coverage
| Axis | Verdict | Coverage |
|---|---|---|
| A1. Intent / Policy / Contract | APPROVED | The change is narrow and aligns with the stated regression fix; no public-contract or scope finding. |
| A2. Architecture / Correctness / Failure | CHANGES_REQUESTED | Incomplete JSON can bypass the execution guard after an explicit terminal stop. |
| A3. Security / Privacy / Trust | CHANGES_REQUESTED | Best-effort parsing of malformed tool arguments can reach tool execution. |
| A4. Verification / Tests / CI | CHANGES_REQUESTED | Affected-path PR CI does not select the focused Anthropic stream regression suites. |
| A5. Context / Compatibility / Platform | CHANGES_REQUESTED | Deferred duplicate-index failure can expose replacement tool lifecycle events before the integrity error. |
|
@snowykr all three closed at [P1] Incomplete JSON stays non-executable regardless of stop reason. You were right that I relaxed on the wrong axis — I keyed on the stop reason when the question is JSON completeness. Incomplete buffers are now flagged before Negative control: re-gating incomplete-buffer marking on [P1] No executable replacement after a duplicate active index. Duplicate indices throw immediately again. The replacement block is marked ambiguous and non-executable before the integrity error, so nothing executable escapes ahead of the failure. Envelope coverage now asserts the exact sequence: 1 [P2] Provider changes now schedule the Anthropic stream suites. This is the one that matters most, and thank you for finding it — it is the systemic cause of the whole incident.
Negative control: removing the mapping fails that planner test (90/1) with only root-check and native selected.
For context on why this PR exists: I reviewed, approved and merged #4612, and I did not run the suite named after the behavior it changed. #4757 is the same mistake on a different PR. Your P2 fixes the class rather than the instance. |
|
Fix-forward pushed as 2fc51b2. The three prior blockers are addressed: incomplete/unclosed JSON remains non-executable under explicit stops; duplicate active indexes fail before replacement lifecycle publication and quarantine both complete orphan and replacement calls; anthropic.ts behavioral-owner mapping schedules anthropic-truncated-toolcall.test.ts and anthropic-stream-envelope.test.ts. Local evidence: bun --cwd=packages/ai run check; 50 Anthropic regression tests pass; 66 Codex guard tests pass; 72 downstream managed/execution tests pass; 91 affected-selector tests pass. Please re-review this exact head. |
Yeachan-Heo
left a comment
There was a problem hiding this comment.
APPROVE exact head 2fc51b2. Resolved snowykr review 4980233998: explicit terminal stops execute only strictly complete, closed JSON; incomplete/unclosed buffers remain incompleteArguments; duplicate active indices abort before replacement lifecycle publication and quarantine complete orphan/replacement calls; packages/ai affected-path mapping selects both Anthropic stream suites. Verified current head with package AI check, 50 Anthropic regression tests, 66 Codex guard tests, 72 downstream execution/managed tests, 91 affected-selector tests, and base/original-patch negative controls.
A tool call enters the truncation set while its JSON is partial, even when a terminal tool_use turn subsequently completes normally. Restore terminal-stop-aware finalization while retaining incomplete evidence for genuinely orphaned or unfinalized calls, so valid calls remain executable.
Duplicate content-block indexes invalidate the block lifecycle even when the orphan already has complete JSON. Mark both the orphan and replacement non-executable before surfacing the integrity error. Confidence: high Scope-risk: narrow Reversibility: revertable Tested: Anthropic truncation, envelope, increment guard, and downstream execution suites Not-tested: Codex reviewer unavailable because no openai-codex API key
2fc51b2 to
6c993cf
Compare
|
Rebased the reviewed fix-forward onto current origin/dev 861b000. New exact head: 6c993cf; merge-base is current dev; exact diff digest remains 843ef7f2076048c196efdce6b02b216d53ca7a07ecbc9f4749104b9f5996c7f8. The prior approval is stale by policy. Please re-review this exact head; the needs-human verdict is intentionally reset until that review lands. |
Yeachan-Heo
left a comment
There was a problem hiding this comment.
APPROVE exact rebased head 6c993cf against current dev 861b000. The three requested blockers are closed: explicit-stop execution requires strictly complete closed JSON; duplicate active indices abort before replacement publication and quarantine complete orphan/replacement calls; affected-path mapping selects both Anthropic stream suites. Local evidence: bun --cwd=packages/ai run check; 50 Anthropic regression tests; 66 Codex guard tests; 72 downstream managed/execution tests; 91 affected-selector tests; 141 combined exact-head focused/selector assertions; base and original-patch negative controls.
What
Restores execution of Anthropic tool calls that stream partially and then finish normally.
devis red because of this.Why
#4612 (
d97b79ef) removed terminal-stop-aware finalization:Membership in
truncatedToolCallsrecords that a block passed through transient partial JSON while streaming. It is not evidence that the finished call is incomplete. Flagging unconditionally marks completed, valid tool callsincompleteArguments: true, and that blocks them from executing — so the agent silently loses the ability to run those tools on an explicittool_usestop.#4612 also changed duplicate active-index handling to fail immediately, which lost orphan finalization evidence.
Measured against the commit immediately before
d97b79ef:anthropic-truncated-toolcall.test.tswent from 10 pass / 0 fail to 5 / 5.How
Terminal stop reasons now suppress truncation flags for normally finalized partial calls, while two cases are still flagged because they genuinely never completed:
Duplicate replacement events are finalized before that error, so #4612's envelope failure behavior is preserved.
#4612's hardening is untouched: malformed and non-string
toolcall_deltaare still rejected, and terminal Codex/function inputs are still validated. This only changes what counts as evidence of incompleteness.Source only — no test file was modified.
Testing
anthropic-truncated-toolcallanthropic-stream-envelopemanaged-attempt-transactionWhole
packages/aisuite is back to its exact pre-#4612 failure set — same 12 names, no new entries. (Those 12 predate #4612 and are tracked separately in #4722.)Negative control: reverting only the source diff reproduces exactly the five named failures (5 pass / 5 fail); reapplying returns 10/0.
Note
I reviewed, approved and merged #4612, and I did not run this suite — it is named after the behavior the change affected. Same miss as #4757. I am now checking for suites named after the behavior a PR removes or changes before approving.
GJC verdict
dev## [Unreleased]