fix(agent): stop recovery after exposed reasoning - #3735
Conversation
Reasoning events are visible and persisted before the runtime can decide whether to retry a truncated local tool call. Once substantive reasoning is exposed, fail closed instead of reconstructing the batch and duplicating that reasoning in the client and history. Constraint: Recovery remains bounded to wholly uncommitted output that can be reconciled safely. Rejected: Buffer and reconcile reasoning events | broadens the replay protocol and delays already-supported reasoning delivery. Confidence: high Scope-risk: narrow Directive: Do not allow interrupted recovery after visible reasoning without end-to-end replay reconciliation. Tested: refresh regression plus four-suite runtime expansion, fmt, lint, typecheck, diff-check. Not-tested: Live provider reasoning transport.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe runtime now declines interrupted local tool-batch recovery when persisted reasoning is present. It preserves reasoning and assistant text, announces buffered tool input, and emits terminal tool errors. New tests cover truncated and placeholder tool calls. ChangesInterrupted tool recovery
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The change stops interrupted-tool recovery after substantive reasoning has reached the client and avoids duplicate reasoning persistence; the supplied checks pass, and no actionable merge-blocking risk remains beyond normal checks and review. Sequence Diagram(s)sequenceDiagram
participant Runtime
participant ChatStreamHandler
participant Client
Runtime->>Runtime: Detect persisted reasoning
Runtime->>ChatStreamHandler: Announce buffered tool input
ChatStreamHandler->>Client: Emit tool-input-start and input deltas
Runtime->>Client: Emit incomplete-tool error
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
kwakayama
left a comment
kwakayama
left a comment
There was a problem hiding this comment.
Review — 76/100 (sound, but three things worth fixing before merge)
The predicate is correct and the test is a genuine red test. My concerns are about blast radius, observability, and what the user sees on the newly-common fail-closed path — not about the 5 lines themselves.
1. Blast radius: this disables #3726 on essentially every hosted path — P1
src/agent/runtime/index.ts:2259-2264
Reasoning is not opt-in. In src/provider/veryfront-cloud/model-catalog.ts:
- All four Anthropic catalog entries set
thinkingBudgetTokens(opus 4.8 / opus 4.6 / sonnet 4.6 = 2048, haiku 4.5 = 1024). - Nine more entries set
thinking: true, includinggpt-5.4-nano, which isDEFAULT_VERYFRONT_CLOUD_MODEL_ID(model-catalog.ts:48). resolveVeryfrontCloudModelThinkingturns both into{ enabled: true }, andresolveHostedRuntimeRequestConfigfalls back to it wheneveragentConfig.thinkingis undefined (src/agent/hosted/runtime-request-config.ts:223-225). Soreasoningis passed tostreamTextby default.claude-opus-4-7/4-8returnundefinedfromresolveVeryfrontCloudReasoningOption(adaptive), but still getthinking: { type: "adaptive", display: "summarized" }viaresolveVeryfrontCloudThinkingProviderOptions— so they emit reasoning too.
Anthropic emits a thinking block before tool use as a matter of course. So for the default hosted chat, invoke_agent, and schedules, hasExposedReasoning will be true on virtually every step that has a tool call — meaning the interrupted-batch recovery that merged one commit ago (#3726) is inert in production.
That may well be the right call (a replayed thinking block with a stale signature is a correctness problem, not a cosmetic one). But it should be an explicit, tracked decision, not an inference a reader has to make.
Suggested fix: open a follow-up issue for the reasoning replay reconciliation protocol and link it from the code comment at 2259, so the next person knows the gate is a stopgap and can find the design work.
2. The decline is completely silent — no log, no span event, no metric — P1
src/agent/runtime/index.ts:2259-2264
Every other abnormal path in this loop is instrumented: logger.warn("Streamed tool call terminated before tool-call event", …) at 2299, addSpanEvent(loopSpan, "max_steps_reached", …) at 1829. Declining recovery has nothing. Given finding 1, this is the difference between "we know how often this fires" and "we find out from a customer".
Suggested fix: inside the hasExposedReasoning && streamedToolCalls.some(isInterruptedClientToolCall) case, emit a span event / warn with { step, toolName, reasoningPartCount }. That also gives you the data to decide whether the reconciliation protocol is worth building.
3. The resulting failure mode is a silently truncated response, not a surfaced error — P1
Trace of shouldContinue === false with an interrupted client tool call:
index.ts:2402—recordIncompleteLocalToolError(toolCall)for each streamed call.index.ts:2393— it callsrecordToolErrorwithemitSse: toolCall.inputAnnounced === true.inputAnnouncedis initialisedfalseatchat-stream-handler.ts:1040and only set true insideannounceToolInputStart(chat-stream-handler.ts:795), whose four call sites (761, 854, 1076, 1177) all sit on commit /tool-input-end/tool-callpaths. An interrupted call never reaches any of them.- So
inputAnnouncedis false → notool-output-errorSSE is sent. Notool-input-startwas ever sent either. step-end,break, finish withfinishReason: "tool-calls".
Net effect for a streaming client: a reasoning block, then the stream ends. No text, no tool call, no error. The user sees the agent think and then say nothing.
This path existed before (maxSteps exhaustion, a second interruption, a provider-executed tool in the same step), so it is not new — but this PR routes the common case into it. A latent gap becomes the default outcome.
Suggested fix: when recovery is declined and the run terminates with an interrupted local tool call, surface something the client can render — either force emitSse: true for this case, or emit an error part with a "the model's response was interrupted" message. Silence is the worst of the three options.
4. The predicate is a copy of one that already exists — P2
src/agent/runtime/index.ts:2259-2261 vs src/agent/runtime/streamed-assistant-message.ts:20-26:
// streamed-assistant-message.ts — skip condition
reasoningPart.text.length === 0 && !reasoningPart.signature && !reasoningPart.redactedData
// index.ts — new gate
part.text.length > 0 || part.signature || part.redactedDataThese are exact logical negations. That is not a coincidence — it is why the predicate is right: it means precisely "would this reasoning part have been persisted into the assistant message?", which is exactly the replay-duplication condition. An id-only part from a bare reasoning-start with no deltas is skipped by the builder and correctly allowed to recover. Good choice.
But it is now stated twice in two modules. If StreamingReasoningPart grows a field (encrypted reasoning / providerMetadata is the obvious one — OpenAI encrypted reasoning items carry no text and no signature) and only the builder is updated, this gate silently keeps permitting replay of reasoning that is now persisted. The failure is invisible: nothing throws, recovery just becomes wrong again.
Suggested fix: export isPersistedReasoningPart(part: StreamingReasoningPart): boolean from streamed-assistant-message.ts (or tool-result-continuation.ts), use it in the builder's skip check and here as state.reasoningParts.some(isPersistedReasoningPart). One definition, one place to update.
Related wording nit: the PR body and reviewer notes say "substantive reasoning". The code means "persisted reasoning" — text.length > 0 counts a whitespace-only part, unlike this repo's hasSubstantiveAssistantText (tool-result-continuation.ts:123). Do not switch to .trim(): that would desync from the builder, which persists " ". Just fix the wording so the next reader does not "fix" it.
5. Test gap: the new test never exercises the preserveRecoverablePlaceholderToolCalls interaction — P2
src/agent/runtime/refresh.test.ts:3033
The test streams '{"suggestions":[' — deliberately a non-placeholder partial input, so isRecoverablePlaceholderToolCall is false and preserveRecoverablePlaceholderToolCalls is irrelevant to it.
But this PR's whole effect is to make !shouldContinue true in a new situation, and index.ts:2278 reads shouldRecoverInterruptedLocalToolBatch || !shouldContinue. For a bare {} placeholder plus substantive assistant text plus exposed reasoning, the placeholder is now preserved in the assistant message and an incomplete-tool error message is written to memory — where the pre-#3726 design deliberately omitted it (shouldOmitRecoverablePlaceholderToolCall). That error then goes back to the model as context on the next turn. I believe this is acceptable (it matches the maxSteps-exhaustion path), but it is a newly-common path with zero coverage.
Suggested fix: add a sibling test with delta: "{}" and reasoning, asserting callCount === 1 and what the persisted assistant message / finishedResponse.toolCalls contain.
Second gap in the same test: it asserts the retry does not happen, but asserts nothing about the terminal state — no finishReason, no assertion that the failure is surfaced (or, per finding 3, that it isn't). Add one, so the day someone fixes finding 3 the test tells them what changed.
6. Nit — .some() predicate returns string | boolean | undefined
src/agent/runtime/index.ts:2259-2261. Typechecks because Array.prototype.some takes => unknown, but the intent reads better as an explicit boolean:
const hasExposedReasoning = state.reasoningParts.some((part) =>
part.text.length > 0 || part.signature !== undefined || part.redactedData !== undefined
);(Also tightens the empty-string case, though no provider is known to send signature: "".)
Verified
const state = createStreamState()is inside the per-step loop (index.ts:2028) —reasoningPartscannot leak across steps. Confirmed.- The gate feeds
canRecoverInterruptedLocalToolBatchonly, which reachesshouldContinueAfterStreamStepsolely throughcanRecoverInterruptedClientToolCall(tool-result-continuation.ts:165-169), which is only consulted whenhasIncompleteToolCallis true. No effect on normal continuation. Confirmed. - The test is genuinely red without the production change. Step 0,
maxSteps: 3, sostep + 1 < maxSteps.'{"suggestions":['failstryParseToolInputObject, so the call stays incomplete; no finalized client call, no provider-executed call. Without the gate,canRecoverInterruptedClientToolCallis true →shouldContinuetrue → seconddoStream→callCount === 2. The assertion fails. - Negative coverage already exists. Roughly eleven tests between
refresh.test.ts:1891and:3032assertcallCount === 2with no reasoning in the first stream, so an over-broad predicate that killed recovery outright would be caught. The one test with reasoning (:2435) puts it in the second stream, which the per-step gate correctly ignores. - All 34 checks pass or skip; zero review threads. Confirmed.
Not verified
- Did not run the branch's tests. The local clone is dirty and on
main; per instructions I did not touch the working tree. Relied on CI plus the static trace above. - No live provider verification — the PR body already says as much for reasoning transport.
- Did not check how Studio renders the terminated stream in finding 3. My claim is derived from the SSE emitted (none), not from the UI.
Recovery declined after exposed reasoning terminated the run with an interrupted local tool call whose tool-input-start was still buffered. recordToolError gates its tool-output-error on inputAnnounced, which is never true for such a call, so the stream ended after the reasoning block with no text, no tool call and no error. Flush the buffered tool-input-start on that path so the failure renders as a tool card matching what the assistant message already persists. Log the declined recovery with step, tool name and persisted reasoning part count. The check re-asks shouldContinueAfterStreamStep with the gate lifted, so it fires only when a batch would actually have been replayed. Move the "was this reasoning persisted?" predicate into isPersistedReasoningPart, exported from streamed-assistant-message.ts and used by both the message builder's skip check and the recovery gate. It was stated twice as exact logical negations; a new StreamingReasoningPart field could have updated one and silently re-permitted replay. Reasoning is default-on across the hosted catalog, so the gate makes #3726 inert on most hosted paths. Track the reconciliation protocol that would lift it in #3736 and reference it from the gate.
|
Thanks — all six findings addressed. Details below, one per finding. 1. Blast radius — documented and trackedOpened #3736, "Reasoning replay reconciliation protocol for interrupted tool batches". It records the catalog analysis (four Anthropic entries with The gate now carries a comment ending Wording drift fixed. The PR body no longer says "substantive reasoning" and now spells out why the gate is not 2. The decline is no longer silent
The condition is No span event: this streaming loop has no span handle ( Observed in the test run: 3. Silently truncated response — fixed by announcing the buffered tool-input-startI checked both options you suggested against this repo's own client before picking, and both are inert there:
So the actual gap is upstream of the error event: the Fix: extract that closure into an exported Scope.
Neither is the failure mode you described, so I left both contracts intact rather than rewriting existing assertions. The pre-existing silent paths (maxSteps exhaustion, second interruption) are unchanged; widening to them is a separate call. Worth recording: for an incomplete call, 4. Predicate deduplicated
Nit 6 applied: the predicate returns a real boolean, 5. Test gaps closedBoth added to
Both are genuinely red without the production change. Forcing Counts
The ~11 6. NitApplied, see finding 4. |
The generated reference pins each declaration to a source line, so the four import lines added by the previous commit made it stale and reddened ci (lint).
|
Follow-up on CI for the two commits above.
Also ran the rest of the Final state: every check green or skipping; CodeQL |
|
@coderabbitai review No CodeRabbit review exists for this PR at any commit — the check reports SUCCESS but the earlier pass was skipped while it was a draft. Please review current head |
|
🧠 Learnings used
|
kwakayama
left a comment
kwakayama
left a comment
There was a problem hiding this comment.
Re-review at 5fdd19d00 — 87/100, no merge blocker
Second pass over the fix round (8a9d542c1 → c77956b70 → 5fdd19d00). All six findings from the 76/100 pass are addressed at the layer that ships, and I verified the important one end to end locally rather than trusting the description. Two things keep this off 90: the extracted predicate is not the pure refactor it is presented as, and CodeRabbit has still never reviewed this PR at any commit.
Verification I ran (detached worktree at 5fdd19d00, Deno 2.7.7 — nothing pushed)
src/agent/runtime/refresh.test.ts: 1 passed (43 steps), 0 failed. Matches the description's count.- Negative control, reproduced independently. Forcing
announceInput: falseatindex.ts:2443turns exactly the two new tests red (0 passed (41 steps) | 1 failed (2 steps)), bothAssertionError. The announce is load-bearing, not decorative. - Wire shape on the declined path, asserted directly. Added throwaway assertions to the truncated-non-placeholder test: the SSE body contains exactly
tool-input-start×1,tool-input-delta×1,tool-input-error×0,tool-output-error×1. So the client gets a renderable, errored tool card and is not double-reported.
Per-finding status
P1 #1 — blast radius: CLOSED
#3736 ("Reasoning replay reconciliation protocol for interrupted tool batches") exists, is open, and the gate comment at index.ts:2267 points at it (See #3736 for the reconciliation protocol that would let it run again). The comment states the stopgap nature in the code, not only in the PR body — which is what I asked for, since the PR body is not what the next reader sees.
The issue itself is good: it carries the catalog analysis, five concrete protocol requirements, and an acceptance criterion that explicitly permits "decide failing closed is permanent, backed by the log volume". No client names, no incident narrative, no customer identifiers — it reads as a platform design note, which is correct for a public repo.
Wording: the gate comment now says persisted, and the PR body has a dedicated "Wording" section distinguishing it from hasSubstantiveAssistantText (which trims). Correct distinction, correctly drawn. See new finding N1 for where that section overstates its case.
Nit: #3736 has no labels. Worth one so it is findable next to the rest of the runtime backlog.
P1 #2 — silent decline: CLOSED
logger.warn("Declined interrupted local tool batch recovery after exposed reasoning", { step, toolName, reasoningPartCount }) at index.ts:2292. logger.warn is the right instrument here — it is what all three sibling abnormal paths in this same loop use (index.ts:2327, :2344, :2585), and this file emits no span events at all, so a span event would have been the odd one out.
Gating is provably exact, which was my worry. declinedRecoveryForExposedReasoning requires shouldContinueAfterStreamStep(state, { recoverInterruptedToolCalls: true }). In tool-result-continuation.ts:150, once hasIncompleteToolCall is true both the finishReason === "tool-calls" and the "stop" branch return canRecoverInterruptedClientToolCall and short-circuit everything else — and that constant is false whenever options.recoverInterruptedToolCalls !== true. So declined === true implies shouldContinue === false. The warn cannot fire on a step that goes on to continue, and it cannot fire on a step that merely carried reasoning: it also requires streamedToolCalls.some(isInterruptedClientToolCall). Both new tests show exactly one warn line each in the captured output.
The second shouldContinueAfterStreamStep call is free of side effects (pure read of state) and sits last behind three cheap conditions. Fine.
P1 #3 — silently truncated response: CLOSED for the path this PR creates
announceStreamedToolCallInput is lifted out of the processStreamInternal closure (chat-stream-handler.ts:91) and called from recordIncompleteLocalToolError when announceInput is set. Traced end to end:
- Guards run before the announce —
providerExecuted,!isStreamedToolCallIncomplete,finalToolResults.has(id)all returnfalsefirst, so a completed or provider-executed call is never announced. - The announce flushes
tool-input-start+ bufferedtool-input-deltas and setsinputAnnounced = true. recordToolErrorthen readsemitSse: toolCall.inputAnnounced === true— evaluated after the mutation, so it is nowtrue. Ordering is correct, and it is the existing gate rather than a bypass, which is the right shape.announceStreamedToolCallInputearly-returns oninputAnnounced === true, so a call surfaced upstream is not announced twice.
Downstream both encoders handle the pair: ag-ui/runtime-chat-stream-encoder.ts:523 forwards tool-output-error by toolCallId, and ag-ui/browser-encoder.ts:893 maps it to an error ToolCallResult. tool-input-start → tool-output-error with no intervening tool-input-available is already an established shape in this codebase (hosted/child-fork-run-context.test.ts:255,365 assert exactly that chunk sequence), so this is not a new protocol invention.
No double-report, and I checked the one place it could have happened. The pre-existing tool-input-error emit at index.ts:2332 is gated on tc.inputAnnounced === true, and it runs in the materialize loop before the terminal block, i.e. before the announce. So it stays dead on this path. Verified empirically: zero tool-input-error in the stream body. See N2 — nothing locks that ordering.
No change to the pre-existing terminal paths. I checked each one the task called out:
- maxSteps exhaustion →
step + 1 < maxStepsfalse →declinedfalse →announceInput: false→ wire behaviour identical. - second interruption →
recoveredInterruptedLocalToolBatchtrue →declinedfalse → identical. - provider-executed tool in the same step →
hasProviderExecutedToolCallmakescanRecoverInterruptedClientToolCallfalse →declinedfalse → identical. - a finalized local sibling in the same step → same, via
hasFinalizedClientToolCall.
That scoping is the conservative choice and I agree with it for this PR. It does leave the inconsistency in N3.
P2 #4 — duplicated predicate: CLOSED
One exported isPersistedReasoningPart in streamed-assistant-message.ts:19, consumed by the builder's skip check (:39), the recovery gate (index.ts:2269) and the log's reasoningPartCount (index.ts:2295). Grepping the branch, there is no surviving copy of the text.length > 0 || signature || redactedData shape in the runtime. The two sites that were an exact logical negation of each other cannot drift now.
P2 #5 — test gaps: CLOSED
preserveRecoverablePlaceholderToolCallsinteraction:"preserves a bare placeholder tool call when reasoning blocks recovery"covers the bare{}case, and asserts the call survives into the assistant message, gets exactly onetool-resultpart, and reaches the client astool-input-start+tool-output-error.- Terminal state:
message-finish×1,finishReason: "tool-calls",toolCalls=[["studio_suggestions", "error"]], plus the literal error text. - The 17
assertEquals(callCount, 2)assertions are unchanged in count betweenorigin/mainand this head, and pass locally — the negative coverage proving the gate is not over-broad is intact.
Nit #6 — .some() return type: CLOSED
isPersistedReasoningPart returns boolean.
New findings
N1 (P2) — the builder change is not a pure refactor, and the PR body says it is
The extraction did not preserve the builder's semantics. Two conditions flipped from truthiness to !== undefined:
- !reasoningPart.signature &&
- !reasoningPart.redactedData
+ return part.text.length > 0 || part.signature !== undefined || part.redactedData !== undefined;
and the same flip was applied to the emitted part's spread. Verified empirically against this head:
| input | old builder | new builder |
|---|---|---|
{ text: "", signature: "" } |
part dropped | {"type":"reasoning","signature":""} |
signature: "" is reachable: chat-stream-handler.ts:1002 assigns on typeof typedPart.signature === "string", which accepts the empty string, and :1005 does the same for redactedData.
Three problems with it:
- The stated rationale is inverted. The PR body defends the predicate as keeping the gate in sync with
buildStreamedAssistantMessage. But the builder was changed to match a newly-written predicate, not the other way round. At the empty-string edge the "existing" behaviour being preserved never existed. - The doc comment and the code disagree. The comment says a part with "no text, no signature and no redacted data" is an id-only shell that is dropped — but
signature: ""is no signature by any reading, and it is now kept. - It is the only place in the repo with these semantics. Every sibling treats empty as absent:
message-adapter.ts:361(!text && !signature && !redactedData),chat-stream-handler.ts:731-732,conversation.ts:375,message-part-parsing.ts:226(getNonEmptyStringField),message-prep.ts:991(hasNonEmptyStringField),hosted/chat-request.ts:131.
Blast radius is genuinely small — getAgentRuntimeReasoningPart drops the empty-signature part again on the replay path, so it should not reach a provider — and the gate direction is fail-closed. But it is an undisclosed semantic change riding in on a refactor, and the asymmetry (text.length > 0 rejects "", signature !== undefined accepts it) is arbitrary. Either make it text.length > 0 || (signature?.length ?? 0) > 0 || (redactedData?.length ?? 0) > 0, which is both a true refactor and consistent with the rest of the repo, or keep it and say plainly in the body that the builder's empty-string handling changed.
N2 (P3) — two surfacing mechanisms for one condition, separated only by statement order
tool-input-error (index.ts:2332) and tool-output-error (via recordToolError's emitSse) are both keyed on inputAnnounced. They do not both fire today only because the announce happens in the terminal block, after the materialize loop. Move the announce earlier — a plausible future refactor, e.g. to cover N3 — and the client gets both events for the same call. Neither new test asserts tool-input-error count is 0; I had to add that assertion myself to confirm it. One line in each new test would lock it.
N3 (P3) — the same failure now renders differently depending on whether the step had reasoning
After this PR, a truncated local tool call on the declined-recovery path renders an errored tool card, while the identical truncation on maxSteps exhaustion or a second interruption still ends the stream with nothing rendered. That inconsistency is pre-existing in origin and correctly out of scope here, but it is now visible: two runs of the same agent differ only by whether the model emitted thinking. Worth a follow-up to make the terminal announce unconditional for incomplete local calls, with the existing assertions updated deliberately rather than by accident.
N4 (nit) — #3736 carries no labels.
CI and CodeRabbit
CI: all green at 5fdd19d00. Run 31867362082, head_sha 5fdd19d001d1d3a916542aad9c1108fff7945fc6, run_attempt 1, conclusion success.
The tests (bun) failure is genuinely gone, and this is a fresh run rather than a retry. The bun job (94970447530) reports run_attempt: 1 on the new head — not a "re-run failed jobs" on the old one. Combined with the diagnosis (link-observer.test.ts:485, a 102 ms timing assertion in the browser prefetch link observer, no path from an agent-runtime change), I am satisfied it is unrelated to this PR. It is still a wall-clock assertion and will flake again on someone else's PR; that is a separate cleanup, not this one's.
ci (lint) fix is correct and minimal. 5fdd19d00 is a one-line docs/api-reference/veryfront/agent.md update moving AgentRuntime from #L840 to #L844, matching the four import lines the fix added. Nothing else in that commit.
CodeRabbit has still not reviewed this PR at any commit. The check reports pass with duration 0 and the note "Review rate limited" — vacuous, exactly the failure mode worth naming. Its only substantive comment (00:26Z) is the rate-limit warning: "you've reached your PR review limit, so we couldn't start this review". pulls/3735/reviews returns one review, mine. A re-review was requested at 05:48:45Z and the bot acknowledged it 14 seconds later; as of 05:50Z it has not posted. Let that pass land before merging — this is the first automated look this branch will get.
Verdict
87/100. Not blocked. The fix round is thorough and the important finding is closed at the wire, with a negative control I reproduced myself. Holding under 90 for two reasons, both cheap to clear: N1 is a real behaviour change presented as a refactor and contradicted by its own doc comment, and CodeRabbit's first-ever pass on this branch is still in flight. Land after that pass returns clean, ideally with N1 resolved one way or the other and N2's one-line assertions added.
Verified: test suite green at head, negative control red, declined-path SSE shape, gate exclusivity via shouldContinueAfterStreamStep, no change to the four pre-existing terminal paths, callCount === 2 coverage intact, issue #3736 content, CI run identity and attempt number.
Not verified: live provider reasoning transport (the author says the same), and how a real Studio client renders tool-input-start → tool-output-error without an intervening tool-input-available — inferred from the in-repo precedent at child-fork-run-context.test.ts:255, not observed in a browser.
Extracting isPersistedReasoningPart flipped the builder's skip check from
truthiness to `!== undefined`, which silently changed behaviour at the
empty-string edge: `{ text: "", signature: "" }` used to be dropped and was
being kept as `{"type":"reasoning","signature":""}`. That input is reachable
— `reasoning-end` assigns on `typeof … === "string"` — and the SSE emission
for the same part still treats "" as absent, so the persisted message had
started disagreeing with the wire. It also diverged from every sibling
(message-adapter, conversation, message-part-parsing, message-prep,
hosted/chat-request), all of which read empty as absent.
Restore the original semantics with an explicit boolean, and drop the
matching `!== undefined` spreads so an empty signature cannot be emitted on
a part kept for its text. The extraction is now a true refactor.
Pin the edge in streamed-assistant-message.test.ts, covering the builder
output and the predicate directly. The test fails under the `!== undefined`
form.
Also assert `tool-input-error` count is 0 in both interrupted-reasoning
tests. That event and the terminal `tool-output-error` are both keyed on
`inputAnnounced`, and only statement order stops both from firing, so the
no-double-report invariant is now pinned rather than incidental.
|
@coderabbitai full review |
|
|
Round-2 findings addressed in N1 — you're right, and the PR body was wrong. Took the preferred fix.This was mine and I'd mis-described it. The predicate is now: part.text.length > 0 ||
(part.signature?.length ?? 0) > 0 ||
(part.redactedData?.length ?? 0) > 0and the two Two things reinforce your call beyond what you cited:
On the round-1 nit that produced this: it asked for a real boolean and suggested Pinned, not just reverted. New test in Verified red under the form it replaces: swapping back to PR body corrected — it now states plainly that an intermediate revision changed the empty-string handling, rather than letting it ride in as a refactor. N2 — invariant pinned
N3 — recorded as #3737#3737 — "Surface interrupted local tool calls on every terminal path, not just declined reasoning recovery". Names the three paths still silent (maxSteps exhaustion, second interruption, exposed sibling / provider-executed call), records exactly which two existing assertions block making the flush unconditional and why neither is the silent-truncation case, and lists the three points that need deciding (placeholder visibility, second-card noise, whether maxSteps deserves its own message). Labeled Your framing is the right one: two runs of the same agent differing only by whether the model emitted thinking is not a defensible distinction to expose. N4 — done#3736 labeled Verification on
|
| suite | result |
|---|---|
src/agent/runtime/ |
504 passed, 663 steps, 0 failed |
streamed-assistant-message.test.ts + chat-stream-handler.test.ts |
2 passed, 97 steps, 0 failed |
refresh.test.ts |
43 steps, 0 failed |
deno fmt --check, deno lint, deno check clean on all changed files. docs:api-reference:check reports current — checked before committing this time rather than after, since that gate is in lint:ci but not in the pre-push hook. Full pre-push suite passed; pushed without --no-verify, no force-push.
|
@coderabbitai review |
|
|
@codex review |
kojiwakayama
left a comment
kojiwakayama
left a comment
There was a problem hiding this comment.
Exact-head re-review at 5851ad24a: 95/100
No blocking findings.
Both items that held the prior review below 90 are closed. isPersistedReasoningPart again treats empty signature and redacted payload values as absent, matching the pre-extraction builder, the SSE path, and sibling runtime adapters. The two interrupted-reasoning regressions now explicitly assert zero tool-input-error events, pinning the no-double-report invariant. The PR body records the intermediate semantic mistake and the final behavior accurately.
Verified locally on the exact head:
- streamed-message and refresh tests: 2 suites, 46 steps, 0 failures
- formatting, lint, typecheck, and diff checks: clean
- hosted checks: complete and green at review time
- unresolved review threads: none
Live provider reasoning transport was not exercised, but the persisted-message and emitted-event seams are covered directly. Merge confidence: 95%, conditional on the requested fresh automated review returning without a new substantive finding.
![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: 5851ad24a9
ℹ️ 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 terminal announce is only reachable when no commit event arrived for the call: `tool-call` is what can supersede a streamed name, and it also sets `inputAvailable`, which fails the guard in recordIncompleteLocalToolError. So the buffered name is the only name such a call will ever have, and it is the same one written to the assistant message and the tool-error record. Both cases now have coverage, since the existing tests used a plain local tool whose name is trivially final: - a delegated remote tool truncated after reasoning is announced under its exact namespaced name and matches what is persisted - a `tool-call` carrying a different name than its `tool-input-start` announces the finalized name once, and the superseded name never reaches the wire Correct the comment on announceStreamedToolCallInput: it attributed the buffering to a namespace-resolution concern from a specific PR, but that attribution traced to a squashed whole-repo import commit and is not supported. Describe the mechanism that is verifiable instead, and state at the call site why the name cannot be superseded there.
Exact-head re-review at ac1e432: 95/100No blocking findings. The only delta from the previously reviewed 5851ad2 head is focused coverage for the terminal tool-name path plus comments documenting why a superseding tool-call event cannot reach that branch. The new tests prove a truncated delegated tool keeps its namespaced name across the live stream and persisted history, while a finalized rename is announced once under the final name with no fabricated error. Verified locally on the exact head:
Merge confidence: 95%. |
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).
2 participants
Summary
{}placeholder siblingWording
Earlier revisions of this description said "substantive reasoning". The gate does not mean that. It means persisted reasoning — the parts
buildStreamedAssistantMessagekeeps:A whitespace-only
textcounts, deliberately. This is nothasSubstantiveAssistantText(tool-result-continuation.ts), which trims; trimming here would desync the gate from the builder, which persists" ". The predicate is defined once, asisPersistedReasoningPartinsrc/agent/runtime/streamed-assistant-message.ts, and both the builder's skip check and the recovery gate read it.Correction, and a note on how this predicate got its final form. An intermediate revision of this PR wrote the predicate as
signature !== undefined || redactedData !== undefinedand changed the builder to match. That was described here as a pure refactor. It was not: the builder previously used truthiness, so{ text: "", signature: "" }went from dropped to kept as{"type":"reasoning","signature":""}. That input is reachable —reasoning-endassigns ontypeof … === "string"— and the SSE emission for the same part still treats""as absent, so the persisted message had begun disagreeing with the wire. It also diverged from every sibling in the repo (message-adapter.ts,conversation.ts,message-part-parsing.ts,message-prep.ts,hosted/chat-request.ts), all of which read empty as absent.The
(x?.length ?? 0) > 0form above restores the original semantics while still returning a real boolean, and the!== undefinedspreads in the builder were reverted with it so an empty signature cannot be emitted on a part kept for its text. The extraction is now a genuine refactor with no behaviour change.streamed-assistant-message.test.tspins the edge against both the builder output and the predicate directly; that test fails under the!== undefinedform.Why the announced name is safe
The terminal announce is only reachable when no commit event arrived for the call.
tool-callis what can supersede a streamed tool name, and it also setsinputAvailable: true, which fails the guard inrecordIncompleteLocalToolError. So reaching the announce means notool-callarrived and the buffered name is the only name that call will ever have — it is also the name written to the persisted assistant message and the tool-error record, so the card matches a reload.Failure mode when recovery is declined
Declining recovery ends the run with an interrupted local tool call. That call never reached
tool-input-end, so itstool-input-startwas still buffered (buffering is deliberate — a delegated remote tool must not be announced under an unresolved namespace, #3123) andinputAnnouncedwasfalse.recordToolErrorgates itstool-output-erroroninputAnnounced === true, so nothing was emitted: the stream ended after the reasoning block with no text, no tool call and no error.The terminal path now flushes the buffered
tool-input-startfor exactly this case, so the existingemitSsegate evaluates true and the failure renders as a tool card. Announcing is idempotent, so a call already surfaced upstream is not reported twice. The flush is scoped to the declined-recovery step, so the pre-existing terminal paths (maxSteps exhaustion, a second interruption, an exposed local sibling) keep their current wire behaviour and their assertions.Blast radius
Reasoning is default-on across the hosted catalog: all four Anthropic entries set
thinkingBudgetTokens, nine more setthinking: trueincludinggpt-5.4-nano(DEFAULT_VERYFRONT_CLOUD_MODEL_ID), andresolveHostedRuntimeRequestConfigfalls back to the catalog value wheneveragentConfig.thinkingis undefined. So this gate makes #3726's recovery inert on essentially every hosted path.That is an accepted trade — replaying a thinking block with a stale signature is a correctness bug — but it is a stopgap, not the end state. #3736 tracks the reasoning replay reconciliation protocol that would let recovery run again, and the code comment at the gate points to it. The new
logger.warnis the data that decides whether that protocol is worth building.Related
Verification
src/agent/runtime/refresh.test.ts: 43 steps, 0 failed (41 before this PR's tests)src/agent/runtime/: 504 tests, 665 steps, 0 failedstreamed-assistant-message.test.ts+chat-stream-handler.test.ts: 2 tests, 97 steps, 0 failedannounceInput: falsefails both reasoning tests!== undefinedpredicate it replacestool-input-errorcount is 0, pinning the no-double-report invariant that currently rests only on statement ordertool-callcarrying a different name than itstool-input-startannounces the finalized name once with the superseded name never reaching the wireReviewer notes
Reasoning is already visible and persisted before continuation is decided. Recovery therefore fails closed after persisted reasoning until the runtime has an end-to-end reasoning replay reconciliation protocol (#3736).
Summary by CodeRabbit
Bug Fixes
Tests