fix(agent): surface interrupted local tool calls on every terminal path - #3782
Conversation
The terminal announce for an incomplete local tool call was gated on
`declinedRecoveryForExposedReasoning`, so the buffered `tool-input-start`
was only flushed when the step happened to carry a persisted reasoning
block. `inputAnnounced` stays false for exactly this class of call, so
`emitSse: toolCall.inputAnnounced === true` suppressed the
`tool-output-error` on every other terminal path — maxSteps exhaustion, a
second interruption in the same run, and an exposed local sibling or
provider-executed call. The stream ended with no text, no tool call and
no error while history recorded a failure.
Pass `announceInput: true` unconditionally in the `!shouldContinue` loop.
Whether recovery was declined for reasoning, a spent step budget or an
exposed sibling is invisible to the user, so it must not decide whether
the failure renders. `recordIncompleteLocalToolError` still guards on
`providerExecuted`, incompleteness and a final result, so only genuinely
truncated local calls are announced, and `announceStreamedToolCallInput`
is idempotent for anything already surfaced upstream.
Exactly one failure event per call still reaches the wire. The
`tool-input-error` emission at index.ts:~2332 is gated on the same
`inputAnnounced` flag and runs before this loop, so it stays dead on this
path; all four affected tests pin `tool-input-error` at 0 alongside a
`tool-output-error` count of 1.
Issue point 1 (bare `{}` placeholders): they now surface on the terminal
path. `preserveRecoverablePlaceholderToolCalls` already keeps such a call
in the assistant message and `recordIncompleteLocalToolError` already
writes a matching tool-result error and an errored `toolCalls` entry, so
withholding it from the wire made persisted history and the live stream
disagree about the same run. A placeholder that is still recoverable is
unaffected: it is re-asked rather than terminalized, so it never enters
this loop.
Two existing assertions in refresh.test.ts changed:
- "fails closed after a local sibling was exposed, with or without a
final result": `tool-output-error` count 0 -> 1. Failing closed means
not re-running the batch, not hiding the truncation. The exposed
sibling is complete, so it never enters the incomplete branch and
contributes no error; the single error belongs to
`truncated-agent-after-exposure`, which the same step already writes
into history. Tightened to assert the toolCallId on both the announce
and the error, plus `tool-input-error` 0.
- "recovers a placeholder after assistant text only once":
`tool-output-error` count 0 -> 1. The test already asserts that the
step-2 placeholder `toolu_repeated_placeholder` is kept in the
assistant message with a tool-result error and an errored entry in
`toolCalls` — that is the history/wire disagreement above, in the same
test. The step-1 placeholder `toolu_placeholder_after_text` is
recovered rather than terminalized, and a new assertion pins its
announce count at 0 so the once-only recovery guarantee is still
covered.
Added "surfaces a truncated local tool call when maxSteps exhaustion ends
the run", which fails on the pre-fix runtime with 0 announces for the
truncated call.
Closes #3737
|
Warning Review limit reached
Next review available in: 4 minutes Limit details: You’ve used all 3 included reviews currently available under your plan. 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: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
Comment |
📦 Client bundle boundary
A server module in a client graph aborts hydration in the browser. New leaks fail CI; known leaks are tracked in |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 17cf097ef3
ℹ️ 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".
| // only genuinely truncated local calls are announced, and | ||
| // `announceStreamedToolCallInput` is idempotent for any already | ||
| // surfaced upstream. | ||
| await recordIncompleteLocalToolError(toolCall, { announceInput: true }); |
There was a problem hiding this comment.
When this newly widened branch handles a truncated call without exposed reasoning, such as maxSteps exhaustion or a second interruption, it emits tool-input-start and partial deltas followed directly by tool-output-error. The AG-UI mapping in src/agent/ag-ui/browser-encoder.ts:926-932 converts an output error only to ToolCallResult, unlike tool-input-error and tool-input-available, which call completeToolInput first. Consequently AG-UI clients receive ToolCallStart, ToolCallArgs, and ToolCallResult without the required ToolCallEnd, leaving the tool-input lifecycle open. Terminalizing this path must either emit an input terminal event or teach the AG-UI encoder to close an incomplete input before its result.
Useful? React with 👍 / 👎.
Addresses the review finding on #3782. A truncated local tool call terminalizes as `tool-input-start` (plus any partial deltas) and then straight to `tool-output-error`. In the browser encoder, `tool-input-available` and `tool-input-error` both close the input through `completeToolInput`, which emits `ToolCallEnd`; the `tool-output-error` branch only emitted `ToolCallResult`. AG-UI clients were therefore left with ToolCallStart, ToolCallArgs and ToolCallResult and no ToolCallEnd, holding the tool-input lifecycle open. This predates #3782 — the declined-reasoning path #3735 added already produced it — but widening the announce to every terminal path takes it from one rare case to all of them, so it is fixed here rather than filed. Fixed in the encoder rather than by emitting an extra runtime event, so the lifecycle closes for any producer of `tool-output-error`, not just this path. `openToolCallIds` tracks calls whose `ToolCallStart` has been emitted and not yet closed; it is distinct from `streamedToolInputIds`, which records whether args were streamed rather than whether the call is open. No synthetic args are emitted on this path. `completeToolInput` back-fills a `ToolCallArgs` when none streamed, but here the model never committed any input, and writing `{}` would claim it did. Red: "closes an open tool input before emitting its output error" fails with ToolCallResult alone. A second test pins that a call already closed by `tool-input-available` does not get a duplicate ToolCallEnd — that one passes before and after, and exists to stop the fix over-reaching. Green: browser-encoder 3 passed (24 steps); full ag-ui suite 29 passed (157 steps); runtime refresh.test.ts 2 passed (56 steps).
Two consequences of closing the tool input on `tool-output-error`. `src/internal-agents/ag-ui-sse.test.ts` opened `tool-1` with `tool-input-start` and went straight to `tool-output-error`, asserting the result alone. That expectation was encoding the defect: the sequence it described leaves the client holding an open tool-input lifecycle for the rest of the run. It now expects `ToolCallEnd` before `ToolCallResult`, with the reason recorded inline. This is the only stale expectation. Every suite in `src/` that references `tool-output-error` was run together to check — 31 passed, 298 steps, 0 failed — rather than discovering them one CI round at a time. `docs/api-reference/veryfront/agent.md` is regenerated: `openToolCallIds` is a documented field on the exported `AgUiBrowserEncoderState`, so `deno task docs` output moved with it and `ci (lint)` fails on a stale reference.
|
@coderabbitai review @codex review Head has moved to 15b4728 since the last review. Two commits since: the AG-UI |
|
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 15b47289fe
ℹ️ 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".
| * a `ToolCallEnd`. Distinct from `streamedToolInputIds`, which tracks | ||
| * whether any args were streamed, not whether the call is still open. | ||
| */ | ||
| openToolCallIds: Set<string>; |
There was a problem hiding this comment.
When a consumer passes a manually constructed AgUiBrowserEncoderState using the previously valid public shape, mapping tool-input-start or tool-output-error now calls add or delete on an undefined openToolCallIds and crashes. This state type and the mapping function are re-exported from veryfront/agent, and the adjacent reasoningSpanIndex is already optional specifically to preserve older state objects, so make this field optional and initialize it lazily rather than requiring every existing caller to adopt it immediately.
AGENTS.md reference: AGENTS.md:L9-L10
Useful? React with 👍 / 👎.
…urvive Review finding on the head commit, and correct. `AgUiBrowserEncoderState` is re-exported from `veryfront/agent` (src/agent/index.ts:752), so a consumer can hold a state object built against the shape the type had before this tracker existed. As a required field, the first `tool-input-start` called `.add` on undefined and crashed. Optional and populated lazily now, which is exactly why `reasoningSpanIndex` directly above it is optional — its comment already says "Optional so a state object built before this counter existed stays valid". `closeOpenToolInput` checks `?.delete(...) !== true` rather than `!...delete()` so an absent set reads as "nothing open" instead of throwing. Covered by a test that deletes the field from a freshly created state and drives the same start/output-error sequence, asserting it still closes the lifecycle rather than crashing.
|
@codex review Head is now 4ae6408. Since your last pass: |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '== HEAD =='
git rev-parse HEAD
git show -s --format='%H%n%s%n%b' HEAD
printf '%s\n' '== Changed files since previous head =='
git diff --name-status 15b47289f..HEAD
printf '%s\n' '== Diff statistics =='
git diff --stat 15b47289f..HEAD
printf '%s\n' '== Relevant diff =='
git diff --find-renames --find-copies 15b47289f..HEAD -- \
src/internal-agents/ag-ui-sse.ts \
src/internal-agents/ag-ui-sse.test.ts \
docsLength of output: 20301 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '== Source outline =='
ast-grep outline src/agent/ag-ui/browser-encoder.ts --items all
printf '%s\n' '== Test outline =='
ast-grep outline src/agent/ag-ui/browser-encoder.test.ts --items all
printf '%s\n' '== Implementation and test diff =='
git diff --find-renames --find-copies 15b47289f..HEAD -- \
src/agent/ag-ui/browser-encoder.ts \
src/agent/ag-ui/browser-encoder.test.ts
printf '%s\n' '== All openToolCallIds references =='
rg -n -C 5 '\bopenToolCallIds\b' src/agent/ag-uiLength of output: 12745 🏁 Script executed: #!/bin/bash
set -euo pipefail
base="$(git merge-base HEAD origin/main 2>/dev/null || git merge-base HEAD main)"
printf '== Merge base ==\n%s\n' "$base"
printf '%s\n' '== Full PR changed files =='
git diff --name-status "$base"..HEAD
printf '%s\n' '== Full implementation diff =='
git diff --find-renames "$base"..HEAD -- src/agent/ag-ui/browser-encoder.ts
printf '%s\n' '== Full test diff =='
git diff --find-renames "$base"..HEAD -- src/agent/ag-ui/browser-encoder.test.ts
printf '%s\n' '== Lifecycle implementation context =='
sed -n '540,625p' src/agent/ag-ui/browser-encoder.ts
sed -n '850,940p' src/agent/ag-ui/browser-encoder.ts
sed -n '1020,1095p' src/agent/ag-ui/browser-encoder.ts
printf '%s\n' '== Lifecycle test context =='
sed -n '880,1055p' src/agent/ag-ui/browser-encoder.test.ts
rg -n -C 4 '"ToolCallEnd"|tool-output-error|tool-input-start' src/agent/ag-ui/browser-encoder.test.tsLength of output: 26163 🏁 Script executed: #!/bin/bash
set -euo pipefail
base="$(git merge-base HEAD origin/main 2>/dev/null || git merge-base HEAD main)"
printf '%s\n' '== Runtime implementation diff =='
git diff --find-renames "$base"..HEAD -- src/agent/runtime/index.ts
printf '%s\n' '== Runtime regression-test diff =='
git diff --find-renames "$base"..HEAD -- src/agent/runtime/refresh.test.ts
printf '%s\n' '== AG-UI SSE regression-test diff =='
git diff --find-renames "$base"..HEAD -- src/internal-agents/ag-ui-sse.test.ts
printf '%s\n' '== Runtime terminal-path context =='
rg -n -C 12 'announceInput|recordIncompleteLocalToolError|declinedRecoveryForExposedReasoning' \
src/agent/runtime/index.ts
printf '%s\n' '== Changed integration-test context =='
rg -n -C 16 'ToolCallEnd|ToolCallResult|tool-output-error|truncated' \
src/internal-agents/ag-ui-sse.test.tsLength of output: 25527
The optional, lazy The AG-UI encoder emits one The new regression tests cover the legacy state shape and the corrected lifecycle. I found no new issues. 🧠 Learnings used
|
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Review: 93/100 — mergingWhat was verified, not assumedThe riskiest part of this PR is that it changes two existing assertions from 0 to 1. That is the classic way to fake a green, so it got the most scrutiny:
Regression evidence: full Two defects found during review, both fixedUnclosed tool-input lifecycle (Codex P2). That fix then surfaced a third thing: Required field on a public type (Codex P2). Deductions−4, issue point 3 left undecided. The issue asks whether −3, blast radius beyond the issue. The encoder change affects every Points 1 and 2 of the issue are settled explicitly in the commit message: placeholders now surface (history and wire no longer disagree), and reasoning-vs-budget-vs-sibling no longer decides visibility. VerdictGreen at head ( |
1 participant
fix(agent): surface interrupted local tool calls on every terminal path
The terminal announce for an incomplete local tool call was gated on
declinedRecoveryForExposedReasoning, so the bufferedtool-input-startwas only flushed when the step happened to carry a persisted reasoning
block.
inputAnnouncedstays false for exactly this class of call, soemitSse: toolCall.inputAnnounced === truesuppressed thetool-output-erroron every other terminal path — maxSteps exhaustion, asecond interruption in the same run, and an exposed local sibling or
provider-executed call. The stream ended with no text, no tool call and
no error while history recorded a failure.
Pass
announceInput: trueunconditionally in the!shouldContinueloop.Whether recovery was declined for reasoning, a spent step budget or an
exposed sibling is invisible to the user, so it must not decide whether
the failure renders.
recordIncompleteLocalToolErrorstill guards onproviderExecuted, incompleteness and a final result, so only genuinelytruncated local calls are announced, and
announceStreamedToolCallInputis idempotent for anything already surfaced upstream.
Exactly one failure event per call still reaches the wire. The
tool-input-erroremission at index.ts:~2332 is gated on the sameinputAnnouncedflag and runs before this loop, so it stays dead on thispath; all four affected tests pin
tool-input-errorat 0 alongside atool-output-errorcount of 1.Issue point 1 (bare
{}placeholders): they now surface on the terminalpath.
preserveRecoverablePlaceholderToolCallsalready keeps such a callin the assistant message and
recordIncompleteLocalToolErroralreadywrites a matching tool-result error and an errored
toolCallsentry, sowithholding it from the wire made persisted history and the live stream
disagree about the same run. A placeholder that is still recoverable is
unaffected: it is re-asked rather than terminalized, so it never enters
this loop.
Two existing assertions in refresh.test.ts changed:
"fails closed after a local sibling was exposed, with or without a
final result":
tool-output-errorcount 0 -> 1. Failing closed meansnot re-running the batch, not hiding the truncation. The exposed
sibling is complete, so it never enters the incomplete branch and
contributes no error; the single error belongs to
truncated-agent-after-exposure, which the same step already writesinto history. Tightened to assert the toolCallId on both the announce
and the error, plus
tool-input-error0."recovers a placeholder after assistant text only once":
tool-output-errorcount 0 -> 1. The test already asserts that thestep-2 placeholder
toolu_repeated_placeholderis kept in theassistant message with a tool-result error and an errored entry in
toolCalls— that is the history/wire disagreement above, in the sametest. The step-1 placeholder
toolu_placeholder_after_textisrecovered rather than terminalized, and a new assertion pins its
announce count at 0 so the once-only recovery guarantee is still
covered.
Added "surfaces a truncated local tool call when maxSteps exhaustion ends
the run", which fails on the pre-fix runtime with 0 announces for the
truncated call.
Closes #3737
Reviewer: read the assertion changes first
This PR changes two existing assertions from 0 to 1. That is the deliberate behaviour decision #3737 asks for, not a test being relaxed to go green, and it is the thing to scrutinise. Both are justified line by line in the commit message above.
The invariant the issue calls out as a hard requirement — exactly one failure event on the wire — is preserved and now pinned in all four affected tests:
tool-output-errorcount 1 alongsidetool-input-errorcount 0.Red evidence: the new
maxStepstest fails on the pre-fix runtime with 0 announces for the truncated call.Closes #3737