fix(bridge): cancel open tool calls on terminal error (#765) - #781
fix(bridge): cancel open tool calls on terminal error (#765)#781Wibias wants to merge 11 commits into
Conversation
|
Warning Review limit reached
Next review available in: 8 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR validates streamed and batch tool-call arguments as JSON. Malformed or unfinished calls now emit incomplete or failed responses instead of completion events. Anthropic streaming maintains independent argument buffers for each tool block. ChangesTool-call hardening
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 70031f4704
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The branch refactors log parsing to logsFromApiBody but omitted the helper file.
|
[shipping-github] Merge-ready |
Streams that deliver content but omit message_stop now surface truncation errors; only message_delta.stop_reason remains a narrow EOF completion path.
Give the tab-visible makeup fetch time to run on slower Ubuntu runners after merging latest dev.
The extra delay did not fix the makeup fetch and could mask real regressions.
…oundary Windows CI died with `panic(...): Internal assertion failure` and no `(fail)` line — four runs across `dev`, #771, #780 and #781. The crash header names the shape: `workers_spawned(9) workers_terminated(8)`, one worker never reclaimed, and it lands exactly at the `api-storage-policy` -> `api-storage` file boundary. That suite is the only one that spawns policy workers. `Worker.terminate()` returns void and does not wait for the thread to go away. Under `bun test --isolate` the harness reclaims a file's realm at the boundary, so a worker still exiting at that instant trips a Bun-internal assertion and takes the whole run down. macOS and Linux tolerate the same race, which is why it reads as a Windows-only flake. `worker-lifecycle.ts` tracks live workers and terminates them through the `close` event, so callers can await the thread actually being gone rather than a timer they hope is long enough; a 5s cap keeps a wedged worker from hanging teardown. Both storage jobs settle their run promise after that await, and the policy suite's `afterEach` now drains. devlog `_plan/260730_remote_issue_merge_round/150` reproduced this panic twice against an unchanged tree, confirmed the same tree passed in PR context, and left this defence as the follow-up if it recurred. It recurred. Evidence: 6190 pass / 0 fail across 445 files, typecheck clean, privacy scan green. The regression test asserts a worker was really spawned before checking it is gone, so it cannot pass vacuously.
EOF completion already keys off stop_reason; remove dead tracking.
Verdict: approve-comment
|
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/adapters/anthropic.ts`:
- Around line 255-258: Update the baseUrl validation error in the Anthropic
adapter’s URL-parsing try/catch to use a static message and omit the raw baseUrl
entirely. Preserve throwing an error when new URL(baseUrl) fails, ensuring
credentials, tokens, or OAuth data cannot be logged or serialized.
- Around line 265-267: Update both streamed and non-streamed tool-block call
sites to treat blank or whitespace-only IDs as missing, using the shared
synthesizeToolUseId fallback rather than nullish coalescing alone. Preserve
valid non-blank IDs, and add regressions covering tool blocks with id: "" for
both paths. Flag any resulting provider/adapter contract drift under src/**.
- Around line 260-262: Update the Anthropic base-URL normalization logic around
the visible `trimmed`/`root` construction to parse the URL and normalize its
`pathname` before appending `/v1/messages`. Explicitly reject URLs with nonempty
`search` or `hash`, while preserving valid host and path normalization; this
prevents query strings or fragments from being treated as part of the Messages
endpoint.
In `@tests/anthropic-stream-hardening.test.ts`:
- Around line 27-37: Add a focused test alongside the existing EOF hardening
test that emits a message_delta with stop_reason "provider_specific_stop" and
then reaches EOF without message_stop. Assert exactly one done event and verify
its stopReason is "provider_specific_stop", covering the successful completion
path in createAnthropicAdapter(...).parseStream.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b013f89c-2b21-4b76-8d9f-61185f61002b
📒 Files selected for processing (5)
src/adapters/anthropic.tstests/anthropic-stream-hardening.test.tstests/claude-messages-endpoint.test.tstests/claude-native-passthrough.test.tstests/helpers/logs-api.ts
Use pathname-based /v1/messages normalization without query/hash, static malformed-baseUrl errors, usableToolUseId for blank IDs, and EOF+stop_reason test coverage.
|
Most of this landed on Landed: the Two additions on top. One thing deliberately not taken. The topic commit also swapped three Also worth recording: I tried to go further and backed out. After landing your fix I noticed the streaming path still forwards malformed So #765 stays open for that remainder, and this PR stays open as its reference point. |
The dev checkout is mid-optimization, so this round stages on codex/260731-pr-merge-round instead of dev. The user merges it into dev and releases once that unit lands. Four reviewers triaged every open PR against origin/dev=356924263 on disjoint slices, and the load-bearing finding is that seven PRs are already fixed on dev by different commits. Merging those heads now would REVERT the newer work. lidge-jun#736 is the clearest case: dev carries decodeSchtasksOutput() at src/service.ts:364-393 because schtasks /query /xml emits UTF-16LE, and the PR head deletes that block and restores encoding: "utf8" -- landing it reopens lidge-jun#722. A clean merge-tree is not permission to merge. An adversarial audit then returned five blockers, all folded in rather than argued away. Two changed what actually gets merged. lidge-jun#744 was routed as a routine catalog fix, but 59d95c0 and 39543a3 change OAuth reconciliation, persist provider settings, and move token resolution around the static branch -- security review per MAINTAINERS.md, so it left the batch. lidge-jun#781's topic commit also swaps three /api/logs test files onto a logsFromApiBody helper that accepts both the array and the {logs} envelope; adopting it would pre-accept the very contract batch B rejects, so only the Anthropic file and its own test come across. The audit also caught three PRs missing from a matrix that claimed to be complete (lidge-jun#750, lidge-jun#746, lidge-jun#644), and lidge-jun#644 additionally carries .codexclaw/goalplans/** and two .DS_Store files. Nothing is merged yet. Batch A is six PRs, batch B rebuilds three whose implementations no longer fit the tree, and 20+ are held for security review, provider evidence, or their own cycle -- each with what would unblock it recorded.
Owner left lidge-jun#781 open as the lidge-jun#765 reference after most of the adapter fix landed via decb08b. Merge tip, drop the duplicated adapter/test delta and the rejected logsFromApiBody helper. Remaining lidge-jun#765 work is bridge-level terminal-error mode (out of scope here).
|
[shipping-github] Addressed owner feedback — synced onto tip, dropped the landed adapter/test overlap and the rejected |
Adapter stream paths can already surface malformed tool arguments, but the bridge was still closing the open call as completed before response.failed. Add a cancel path (status incomplete, no arguments.done), refuse to complete unparseable assembled args at tool_call_end, and restore Anthropic stream validation that errors instead of ending the call.
Move the new stream-path cases into their own describe so they are not nested under the non-stream tool_use input suite.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/bridge.ts`:
- Around line 443-468: Normalize unusable tool-call arguments to "{}" in both
failCurrentToolCall() and flushToolCall(), covering function, tool-search, and
freeform incomplete-call payloads without exposing malformed JSON. Ensure open
calls use the incomplete-response path when stopReason is "max_tokens" or
"content_filter", including the batch flow around the completed-condition logic,
so replayed responses remain valid. Add streaming and non-streaming regression
assertions confirming arguments is "{}".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2ab92137-7c86-4b5b-a9f8-8db288e42888
📒 Files selected for processing (5)
src/adapters/anthropic.tssrc/bridge.tsstructure/01_runtime.mdtests/anthropic-stream-hardening.test.tstests/responses-stream-tool-events.test.ts
| const failCurrentToolCall = () => { | ||
| if (!currentToolCall) return; | ||
| const argsStr = currentToolCall.args || "{}"; | ||
| const item = currentToolCall.toolSearch | ||
| ? { | ||
| type: "tool_search_call", id: currentToolCall.itemId, | ||
| call_id: currentToolCall.callId, execution: "client", | ||
| arguments: parseArgsObj(currentToolCall.args), status: "incomplete", | ||
| } | ||
| : currentToolCall.freeform | ||
| ? { | ||
| type: "custom_tool_call", id: currentToolCall.itemId, | ||
| call_id: currentToolCall.callId, name: currentToolCall.name, | ||
| input: freeformInput(currentToolCall.args), status: "incomplete", | ||
| } | ||
| : { | ||
| type: "function_call", id: currentToolCall.itemId, | ||
| call_id: currentToolCall.callId, name: currentToolCall.name, | ||
| arguments: argsStr, status: "incomplete", | ||
| ...(currentToolCall.namespace ? { namespace: currentToolCall.namespace } : {}), | ||
| }; | ||
| emit("response.output_item.done", { output_index: currentToolCall.outputIndex, item }); | ||
| finishedItems.push(item as OutputItem); | ||
| outputIndex++; | ||
| currentToolCall = null; | ||
| }; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm whether "incomplete" status function_call items get echoed back into
# subsequent-turn history/replay (the mechanism that would make this leak harmful).
rg -n "previous_response_id" -g '*.ts' -A5 -B5 src | head -100
rg -n 'status.*"incomplete"' -g '*.ts' src/server src/codex 2>/dev/null | head -50Repository: lidge-jun/opencodex
Length of output: 7362
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- bridge relevant definitions and call sites ---'
sed -n '380,475p' src/bridge.ts
sed -n '690,735p' src/bridge.ts
sed -n '1085,1140p' src/bridge.ts
sed -n '1180,1210p' src/bridge.ts
sed -n '1235,1265p' src/bridge.ts
printf '%s\n' '--- replay and response-state handling ---'
sed -n '360,445p' src/server/responses/core.ts
rg -n "rememberResponseState|_replayPrefixLen|output\\.|function_call|incomplete" src/server src -g '*.ts' | head -180
printf '%s\n' '--- relevant tests ---'
rg -n "malformed|incomplete|arguments|tool_call_end" tests -g '*.ts' | head -160Repository: lidge-jun/opencodex
Length of output: 49612
🏁 Script executed:
#!/bin/bash
set -e
python3 - <<'PY'
from pathlib import Path
p = Path("src/bridge.ts")
text = p.read_text()
for needle in ["toolCallArgumentsUsable", "failCurrentToolCall", "flushToolCall", "rememberResponseState"]:
print(f"--- {needle} ---")
for i, line in enumerate(text.splitlines(), 1):
if needle in line:
print(i, line.strip())
PY
printf '%s\n' '--- response state call graph excerpts ---'
rg -n "rememberResponseState|onCompletedResponse|_previousResponseInputExpanded|_replayPrefixLen|previousResponseId" src -g '*.ts' -A8 -B8 | head -260Repository: lidge-jun/opencodex
Length of output: 20534
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- argument usability helper ---'
sed -n '55,90p' src/bridge.ts
printf '%s\n' '--- response-state policy and replay construction ---'
sed -n '405,490p' src/responses/state.ts
sed -n '1395,1440p' src/server/responses/core.ts
sed -n '1635,1675p' src/server/responses/core.ts
sed -n '1860,1905p' src/server/responses/core.ts
sed -n '2010,2090p' src/server/responses/core.ts
printf '%s\n' '--- malformed tool-event tests ---'
sed -n '1,115p' tests/responses-stream-tool-events.test.ts
printf '%s\n' '--- bridge response construction ---'
sed -n '780,850p' src/bridge.ts
sed -n '895,970p' src/bridge.ts
sed -n '1265,1305p' src/bridge.tsRepository: lidge-jun/opencodex
Length of output: 28073
🏁 Script executed:
#!/bin/bash
set -e
python3 - <<'PY'
from pathlib import Path
text = Path("src/server/responses/core.ts").read_text().splitlines()
needles = (
"bridgeToResponsesSSE(",
"buildResponseJSON(",
"onCompletedResponse",
"rememberResponseState(",
)
for i, line in enumerate(text, 1):
if any(n in line for n in needles):
lo, hi = max(1, i - 10), min(len(text), i + 18)
print(f"--- core.ts:{i} ---")
for j in range(lo, hi + 1):
print(f"{j}:{text[j-1]}")
PY
printf '%s\n' '--- all response.failed persistence and replay references ---'
rg -n "response\\.failed|status === \"failed\"|status.*failed|rememberResponseState" src/responses src/server -g '*.ts' -A6 -B6 | head -260Repository: lidge-jun/opencodex
Length of output: 38332
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- response-state policy and replay construction ---'
sed -n '405,490p' src/responses/state.ts
sed -n '1395,1440p' src/server/responses/core.ts
sed -n '1635,1675p' src/server/responses/core.ts
sed -n '1860,1905p' src/server/responses/core.ts
sed -n '2010,2090p' src/server/responses/core.ts
printf '%s\n' '--- malformed tool-event tests ---'
sed -n '1,115p' tests/responses-stream-tool-events.test.tsRepository: lidge-jun/opencodex
Length of output: 18444
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- incomplete adapter-event producers and reasons ---'
rg -n "type:\s*[\"']incomplete[\"']|yield\s*\{[^}]*incomplete|reason:\s*[\"'](max|content|adapter|upstream)" src/adapters src -g '*.ts' -A4 -B4 | head -300
printf '%s\n' '--- AdapterEvent definition ---'
rg -n "type AdapterEvent|interface.*AdapterEvent|incomplete.*reason" src/types.ts src -g '*.ts' -A35 -B8 | head -180
printf '%s\n' '--- non-stream bridge callers and tests for max-output partials ---'
rg -n "max_output_tokens|max_tokens|stopReason|incompleteEvent|buildResponseJSON" tests src -g '*.ts' -A8 -B8 | head -320Repository: lidge-jun/opencodex
Length of output: 44051
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- incomplete adapter-event producers and reasons ---'
rg -n "type:\s*[\"']incomplete[\"']|yield\s*\{[^}]*incomplete|reason:\s*[\"'](max|content|adapter|upstream)" src/adapters src -g '*.ts' -A4 -B4 | head -300
printf '%s\n' '--- AdapterEvent definition ---'
rg -n "type AdapterEvent|interface.*AdapterEvent|incomplete.*reason" src/types.ts src -g '*.ts' -A35 -B8 | head -180Repository: lidge-jun/opencodex
Length of output: 19747
Normalize malformed arguments on every incomplete tool-call path.
The failure paths at src/bridge.ts:443 and src/bridge.ts:1101 expose malformed arguments, but failed responses are not replayed. The replay risk occurs when max_tokens ends with an open tool call:
src/bridge.ts:780callscloseCurrentToolCall()before emitting an incomplete response.src/bridge.ts:1253flushes the batch call as"completed"becausestopReasonis not included in the condition.rememberResponseState()cachesmax_output_tokensincomplete responses.
If the truncated buffer is invalid JSON, the cached replay contains invalid function_call.arguments. Use "{}" for unusable arguments in failCurrentToolCall() and flushToolCall(). Route open calls through the incomplete path when stopReason is "max_tokens" or "content_filter". Add streaming and non-streaming regression assertions for arguments: "{}".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/bridge.ts` around lines 443 - 468, Normalize unusable tool-call arguments
to "{}" in both failCurrentToolCall() and flushToolCall(), covering function,
tool-search, and freeform incomplete-call payloads without exposing malformed
JSON. Ensure open calls use the incomplete-response path when stopReason is
"max_tokens" or "content_filter", including the batch flow around the
completed-condition logic, so replayed responses remain valid. Add streaming and
non-streaming regression assertions confirming arguments is "{}".
Continues 260731_pr_merge_round at dev 7132828. 000 freezes the inventory and records the method; 010 dispositions every open PR. Two findings changed how the round reads CI and PR state: Windows red is three infra classes, not one. The launcher ENOENT the merge round fixed, a Bun runtime panic it did not, and unrelated flaky tests. Zero of the 27 open PRs has a Windows failure traceable to what it changes, so a blanket 'CI broken' reading would have been wrong on every row. #781 is not empty. gh pr diff showed its first force-pushed patch; git diff origin/dev...head shows +275/-9 including 113 lines in src/bridge.ts adding failCurrentToolCall(). That is the bridge-layer fix the previous round concluded #658 actually needs. Closing it as a stale reference would have discarded the work. MERGE-NOW is zero. Several PRs are technically clean, but none clears both gates MAINTAINERS.md requires: maintainer approval and required CI against current dev.
Three close cleanly: #759 (video modality filtered at three write paths), #754 (init EOF rejects pending prompts), #767 (native slugs withheld across every Desktop writer). Three lane errors are recorded rather than quietly dropped, because each would have produced a wrong row: A reported typecheck failure was an empty node_modules in a fresh worktree, not a tree defect. After bun install it exits 0. #570 came back both FIXED and PARTIAL. Both readings are correct about different questions: the port-remap failure it reported is gone at auth-cors.ts:37-51, while non-loopback host aliases stay rejected at :85 -- a request the issue never made. Closing on the reported mode, splitting the alias question out. #764's covering PR merged remotely but its merge commit is not an ancestor of the frozen HEAD, so the reading stands for this round. #658 now has a located fix site. The adapter fails closed at anthropic.ts:866-881 whenever EOF arrives without stop_reason, and relaxing it there was already reverted once. bridge.ts:771 is where it belongs -- the same place PR #781 adds failCurrentToolCall(). Acceptance-boundary analysis stays in gitignored scratch per AGENTS.md; only neutral pointers are here.
Selection rule is narrow on purpose: an entry must trace to a triage row and must not be waiting on maintainer approval or third-party evidence. That is what keeps the list short -- most open PRs are blocked by approval, not by code. F1 is first because three other stream items sit on it. closeCurrentToolCall() at bridge.ts:378-417 hardcodes status: "completed", and both failure paths call it unconditionally (:776 on adapter error, :835 on implicit EOF). So a stream that ends in failure still hands the client a completed tool call, with response.failed arriving afterward. That is why the adapter-side attempt at #658 was reverted in 95d8ed7, and it is what PR #781 already fixes with failCurrentToolCall(). F2 must not land before F1. Relaxing EOF without a cancel path would let a truncated tool call through as success. Also recorded: bridge.ts:436-441 still says it synthesizes response.completed, but :838 emits response.incomplete. The comment outlived the code. Three tests already on dev would survive reverting the behavior they claim to cover. With #790 and #758 last round, that is a pattern rather than three accidents, so it gets its own issue.
…g it
closeCurrentToolCall() hardcodes status: "completed" and emits
function_call_arguments.done. Every failure path called it
unconditionally, so a stream ending in error still handed the client a
completed tool call and delivered response.failed afterward. The client
had already seen an issued call.
failCurrentToolCall() cancels instead: no *.done argument frames,
status: "incomplete", same shape an in-flight web_search_call already
uses when it closes as failed. Empty args still serialize as "{}" so an
echoed item cannot poison the next turn through JSON.parse("").
Assembled arguments are now validated at tool_call_end. Fragments
already streamed cannot be repaired the way a non-stream adapter
degrades a bad payload to {}, so unusable arguments cancel the item and
fail the turn rather than completing it.
This is why the adapter-side attempt at #658 was reverted in 95d8ed7:
the adapter was never the right layer, because bridge completed the open
call before the failure reached the client.
Both mechanisms were ablated separately. Reverting the five failure-path
call sites fails "terminal error cancels an open tool call instead of
completing it"; disabling only the tool_call_end guard fails "malformed
assembled arguments at tool_call_end fail the turn without completing".
Each test dies with its own mechanism and not the other.
Also corrects a comment that claimed the implicit-EOF path synthesizes
response.completed. It emits response.incomplete with reason
adapter_eof, and has for some time.
Cherry-picked from PR #781 by @Wibias (bridge and adapter halves), which
addresses the #765 stream remainder.
Co-authored-by: Wibias <37517432+Wibias@users.noreply.github.com>
|
The bridge and adapter halves landed on A note on how close this came to being closed as empty:
This also explains why the adapter-side attempt at #658 was reverted in Both mechanisms were ablated separately — reverting the five call sites fails the terminal-error test, disabling only the Closing since it is on |
Summary
Fixes #765
Most of the Anthropic adapter side of #765 already landed on
dev(baseUrl/v1/messagesnormalization,input_json_deltascoping,toolu_id synthesis, EOF stop-reason fallback, and non-streamtoolUseArguments→{}).This PR covers the remaining defect: the streaming path still forwarded malformed
partial_json, and when the adapter errored the bridge closed the open tool call asstatus: "completed"(withfunction_call_arguments.done) beforeresponse.failed— so the client still saw an issued call.Bridge terminal-error mode
error/ incomplete / stall / EOF (and thrown upstream), cancel the open tool call asstatus: "incomplete"without emittingfunction_call_arguments.done/custom_tool_call_input.done.tool_call_end, refuse to complete non-freeform calls whose assembled arguments do not parse; cancel the item and fail the turn.buildResponseJSONmatches: open calls on failed/incomplete turns land asincomplete, and malformedtool_call_endfails the turn.Anthropic stream path
input_json_deltaforwarding (bridge contract).content_block_stop; on invalid JSON emiterrorinstead oftool_call_end(bridge cancel path handles the open call).Test plan
bun test tests/anthropic-stream-hardening.test.ts tests/responses-stream-tool-events.test.ts tests/bridge.test.ts tests/bridge-terminal-singleness.test.ts tests/bridge-lifecycle.test.tsbun run typecheckbun run testSummary by CodeRabbit
Bug Fixes
Documentation