fix(think,ai-chat,agents): harden recovery, transcript integrity & compaction under deploy churn

[threepointone]

Summary

This PR hardens Think (and, where the machinery is shared, AIChatAgent and the agents Session memory / sub-agent layer) against the failure modes a customer hit while running long, tool-heavy agent turns through repeated wrangler deploy churn. A mid-turn deploy resets the Durable Object ("reset because its code was updated"), and under rapid redeploys a single turn can be interrupted several times in a row. The bugs below combined to make turns that actually completed look failed, make completed tool calls re-run, make interrupted tool calls vanish, make malformed tool inputs 400 forever, make auto-compaction silently no-op on tool-heavy histories, and make sub-agent interruptions look like terminal failures.

All fixes are covered by unit tests plus new e2e/real-deploy churn harnesses (examples/deploy-churn, Think e2e tool-rollback + task-amplification).

Changesets included:

• recovery-submission-status-under-churn.md — @cloudflare/think + @cloudflare/ai-chat (patch)
• preserve-interrupted-tool-calls.md — @cloudflare/think (patch)
• compaction-tokencounter-drives-boundary.md — agents (patch, Auto-compaction trigger fires, but compaction doesn't run due to internal token counting #1593)
• agent-tool-structured-failure.md — agents (minor)

---

1. Recovery falsely erroring a completed turn under repeated mid-turn deploys

When several deploys interrupt one turn, recovery runs a chain of continuations. Three bugs combined to leave the durable submission in error even though every step completed:

• Lost ownership — the submission link (recoveredRequestId) was derived from each continuation's own fresh requestId, so chained continuations dropped it; the continuation that finally finished the turn could no longer mark the submission completed.
• Stale-continuation clobber — a superseded continuation tripped the conversation_changed guard because the leaf had advanced via recovery's own forward progress (a new assistant message), not a new user turn, and overwrote the still-running submission to error.
• Premature stable_timeout — a timeout while waiting for the isolate to settle (common with a deploy in flight) failed the turn terminally on the first attempt.

Fix: submission ownership is keyed off the stable recovery root and threaded through the entire continuation chain; a superseded continuation skips benignly (only a genuinely newer user turn marks the submission skipped, never error); and a stable-state timeout reschedules within the maxAttempts budget instead of abandoning. @cloudflare/ai-chat shares the recovery machinery but has no durable-submission layer, so it gets the stable_timeout reschedule fix only.

Commits: ff8a4340, d59a7287.

2. Interrupted tool calls disappeared and were silently re-run

Transcript repair previously removed a tool call with no recorded output (interrupted mid-execution by a deploy, or an ask_user-style tool answered by the user's next message) before the next turn. That made the call vanish from the broadcast transcript and let the model re-run it — duplicating non-idempotent side effects.

Fix: the orphan is now flipped to state: "output-error" with an explanatory message, so the user-visible record survives, the model sees the tool errored instead of re-running it blind, and the provider still receives a valid tool-result (no AI_MissingToolResultsError). As a last-line backstop, convertToModelMessages is now called with ignoreIncompleteToolCalls: true.

Commits: aad63224, bfec18c2, 5ca0e378.

3. Malformed tool inputs 400 the provider forever

Repair parsed stringified-JSON tool inputs, but a tool call with a missing or null input was left unrepaired — Anthropic rejects a tool_use block whose input is absent, so the turn 400s on every retry.

Fix: a new _normalizeToolInput helper parses stringified JSON and defaults a missing/null input to {}, applied on both the orphan-healing and settled-part paths.

Commit: 5c57ac52.

4. Auto-compaction fired every turn but silently no-op'd on tool-heavy histories (#1593)

Session.compactAfter(threshold, { tokenCounter }) fired the onCompaction callback, but the bundled createCompactFunction boundary walk still used the Workers-safe chars/4 heuristic. On tool-heavy histories that under-counts ~4–5×, so the configured tail budget covered the entire history, compressEnd <= compressStart, and the function returned null — compaction fired every turn but never shortened anything.

Fix: the Session passes its counter to the compaction function via a new CompactContext argument; createCompactFunction uses it for the tail-budget walk when no explicit CompactOptions.tokenCounter was given. A single tokenCounter on compactAfter() now drives both "should we compact?" and "what should we compact?". A trigger that still returns null logs a one-time warning (re-armed on the next successful compaction).

Commits: ce06564e, 52e69855.

5. Sub-agent interruptions looked like terminal failures

agentTool() collapsed every non-completed sub-agent run to an opaque { ok: false, error: string }. A child reset/superseded by a deploy or parent recovery (interrupted) was indistinguishable from a genuine failure or an intentional cancellation, so the parent model would often parrot the interruption text back to the user as final.

Fix: failures now return a structured AgentToolFailure { ok: false, status, error, retryable }. interrupted → retryable: true (and surfaces the interruption reason); aborted and error → retryable: false. Backward compatible for consumers reading ok/error; AgentToolFailure is exported from agents so an orchestration harness (or a parent prompt convention) can re-dispatch an interrupted sub-agent instead of reporting it as final.

Commit: 1f62e9ff.

6. Test harnesses (no shipped behavior)

• examples/deploy-churn — a real wrangler deploy churn harness (scripts/deploy-rollback.ts).
• packages/think/src/e2e-tests/tool-rollback.test.ts — settled-tool durability under recovery.
• packages/think/src/e2e-tests/task-amplification.test.ts — proves sub-agent (task) re-runs stay bounded under churn (no parent↔child budget amplification).
• Unit coverage: think-session.test.ts, message-reconciliation.test.ts (orphan-preserved + stringified and missing input), agent-tools-failure.test.ts (the new failure envelope), durable-chat-recovery.test.ts, session.test.ts.

Commits: 1fc27c2e, ce07fccc.

---

7. Silent-hang watchdog for the streaming read loop

A model stream that parked without ever throwing (no chunk, no error, no done) left the chat read loop waiting forever — an infinite spinner with no terminal state. There was no detection for a silently hung turn (only the recovery-path stable_timeout, which guards recovery scheduling, not a live stream).

Fix: opt-in chatStreamStallTimeoutMs (default 0 = off). If no UI-message-stream chunk arrives within the window, the watchdog aborts the turn so the loop exits with a terminal stream error (routed through onChatError with stage: "stream") and emits a new chat:stream:stalled observability event. Applies to both the WebSocket turn loop and the chat() / sub-agent callback loop. It measures inter-chunk inactivity (which includes server-side tool execution), so it must be set above the slowest expected model TTFT and tool latency — hence opt-in, not a surprising default.

Commit: 2d5f9f2d. Changeset: think-stream-stall-watchdog.md.

Test plan

• npm run check (sherif + export checks + oxfmt + oxlint + typecheck) — all 91 projects typecheck, lint + format clean
• think message-reconciliation (5/5), think/ai-chat agent-tools reconciliation (14/14, 19/19), agents agentTool envelope (4/4), agents session (74/74)
• npx nx affected -t test (full affected suite in CI)
• Think e2e: tool-rollback, task-amplification, chat-recovery
• Manual: examples/deploy-churn real-deploy churn — turn ends completed, no duplicate tool execution, no disappearing tool calls

Out of scope (separate follow-ups)

These remain genuine gaps that no engine owns yet — intentionally not in this PR:

• Prompt caching + dynamic per-turn cached prefix (withCachedPrompt is opt-in and the cached prefix freezes until refreshSystemPrompt(); no Anthropic cacheControl breakpoints in core).
• Silent mid-stream hang watchdog — addressed in this PR (see section 7).

[changeset-bot]

🦋 Changeset detected

Latest commit: be29d60

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages

Name	Type
agents	Minor
@cloudflare/think	Minor
@cloudflare/ai-chat	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[threepointone]

/bonk run npm run format and commit and push the changes.

[ask-bonk]

Done. Ran npm run format (via npx oxfmt --write .) which reformatted two changeset files — oxfmt normalized *emphasis* to _emphasis_ in the markdown. Committed as chore: format changeset files with oxfmt and pushed to general-fixes.

github run

[threepointone]

/bonk do a deep review on this PR, look for edge cases, what do you think

[ask-bonk]

Deep Review — PR #1623

I've read every changed file and diff carefully. This is a serious hardening PR for a real customer failure mode (repeated wrangler deploy churn during long, tool-heavy agent turns). The problem analysis is excellent and the three-bug decomposition is precise. Here's my detailed review, focused on edge cases, correctness concerns, and things that could bite.

---

1. Recovery Submission Ownership (recoveryRootRequestId)

The fix is correct and well-motivated. Keying submission ownership off recoveryRootRequestId instead of the per-continuation requestId is the right design. The threading through _runChatRecoveryFiber → snapshot → _chatRecoveryContinue looks clean.

Edge case concern — _activeChatRecoveryRootRequestId initialization: In _runChatRecoveryFiber (think.ts:1970), the root is set as this._activeChatRecoveryRootRequestId ?? requestId. This means the FIRST turn in a recovery chain gets its own requestId as the root, and subsequent continuations inherit it. This is correct for the happy path. But consider: if two independent recovery incidents overlap (e.g., a user sends a new message that triggers a new fiber while an old recovery is still draining), _activeChatRecoveryRootRequestId is a single instance field — the second turn would inherit the first's root. The try/finally restore in _chatRecoveryContinue:7455 mitigates this, but it's worth noting the field is inherently not concurrency-safe. The turn queue serialization makes this safe in practice, but it's a subtle invariant worth a comment.

2. Superseded Continuation Skip Logic

This is the most important behavioral change and the analysis is correct. The old code marked superseded continuations as error, which was wrong. The new logic at think.ts:7386-7411 is:

• Leaf is an assistant message but not the target → recovery's own progress advanced, skip benignly, don't touch the submission (correct!)
• Leaf is a user message → genuinely new turn, mark submission skipped (correct!)

Edge case — lastLeaf is null: If getLatestLeaf() returns null (empty conversation after a clear), lastLeaf?.role === "user" is false, so supersededByNewerUserTurn is false. The continuation is skipped but the submission stays running. This is debatable — a cleared conversation arguably should terminate the submission. However, this is a pre-existing behavior (the clear path resets the turn queue generation, which would cause the continuation's continueLastTurn to return skipped anyway), so it's not a regression.

3. Stable Timeout Reschedule

The idempotent: false insight is a gem. The comment explaining WHY idempotent would fail here (the currently-executing one-shot row gets deleted by alarm() after return, taking an idempotent dedup with it) is one of the most important in this PR. The test at think-session.test.ts that pre-inserts a schedule row to verify count === 2 is exactly the right test.

Edge case — unbounded retries via progress: The recovery incident has a progress field (high-water mark of assistant messages) used elsewhere to reset the attempt budget when forward progress is detected. The stable timeout retry increments attempt but does not check CHAT_RECOVERY_MAX_WINDOW_MS. However, the wall-clock ceiling IS checked in _beginChatRecoveryIncident, not here. If a deploy churn storm produces stable timeouts for 15+ minutes, the attempt counter would exhaust naturally (6 attempts × 3s delay = 18s, well within the window). So this is fine in practice, but worth noting that the wall-clock guard is only checked at incident creation, not at retry time.

Code duplication: The stable timeout reschedule logic is inlined identically in _chatRecoveryRetry and _chatRecoveryContinue in think.ts (two ~25-line blocks). ai-chat correctly extracts this into _rescheduleRecoveryAfterStableTimeout. Think should arguably do the same. Not a correctness issue, but a maintainability concern — a future fix to one copy but not the other would be a bug.

4. Interrupted Tool Call Preservation

The behavioral change from "delete" to "error" is clearly correct. The old behavior was wrong in two ways: disappearing tool calls and silent re-execution. The new output-error state with an explanatory message is well-chosen.

Edge case — JSON parsing of stringified inputs: The input normalization at think.ts:1499-1510 tries JSON.parse on string inputs that start with {. This is a best-effort heuristic. Consider:

• Input '{ invalid json' — caught by the try/catch, falls back to the original string. ✓
• Input '{"key": "value"}' — correctly parsed to object. ✓
• Input that is a string representation of a number like '42' — doesn't start with {, so no parse attempt. ✓
• Input '{"key": "val"}extra' — JSON.parse would throw, falls back to original. ✓

This looks robust. The same normalization runs for both orphaned and completed tool parts, which is a nice consistency.

Edge case — ignoreIncompleteToolCalls: true as backstop: The backstop at think.ts:2485-2488 is a good defense-in-depth layer. However, ignoreIncompleteToolCalls silently drops tool calls, which could mask bugs in the repair logic. The repair is supposed to catch everything — if incomplete calls reach the backstop, it means the repair missed something. A debug-level log when the backstop fires would help surface these cases without breaking the turn.

5. Compaction TokenCounter Flow-Through

The fix is structurally sound. The CompactContext type is backward-compatible (optional second argument to the compaction function), and the session counter adapter correctly feeds individual messages with empty system/context.

Edge case — the per-message caveat is well-documented but could be confusing: The comment at compaction-helpers.ts:496-504 correctly warns that a usage-only counter (one that returns a fixed total regardless of which messages are passed) degrades tailTokenBudget to minTailMessages. This is important but only documented in code comments. Users who wire usage.inputTokens as their counter will get compaction that "works" but with a degraded tail budget — and they'll get a console warning every turn that compaction returned null. The one-time warning at session.ts:551-554 helps surface this, but the message suggests "configure a tokenCounter" — which they already did, just the wrong kind. Consider making the warning message more specific about per-message vs. whole-prompt counters.

Edge case — _warnedCompactionNoOp is instance-level: The warning flag at session.ts:78 is an instance field, not persisted. After DO hibernation + wake, the warning resets and fires again. This is actually fine — a fresh isolate should surface the issue — but worth noting it's by design.

6. Test Coverage Assessment

The test coverage is thorough and well-structured:

• Chain ownership test (think-session.test.ts) — verifies recoveredRequestId in the scheduled payload is the root, not the continuation's id
• Superseded continuation — verifies submission stays running when an assistant leaf supersedes
• New user turn supersede — verifies submission goes to skipped
• Stable timeout reschedule — verifies non-idempotent scheduling creates 2 rows
• Budget exhaustion — verifies terminal failure when attempts are spent
• Idempotent re-recovery — verifies no duplicate assistant messages
• CompactContext flow — verifies session counter is used when no explicit counter
• Orphan tool preservation — verifies output-error state and input normalization

Gap I notice: There's no test for the _chatRecoveryRetry stable timeout reschedule path in Think (only _chatRecoveryContinue). The ai-chat tests cover both, but Think's _chatRecoveryRetry has its own inlined copy of the reschedule logic. Given the duplication concern above, a test for the retry path would be valuable.

7. deploy-churn Example

The harness is well-designed — deterministic mock model, real wrangler deploy churn, non-idempotent tool ledger for rollback detection, and structured JSONL timeline output. The CLEAN/MINIMAL/ROLLBACK classification is a nice touch.

Note: The harness uses @callable() decorators, which require the agents vite plugin for decorator transforms. The vite.config.ts isn't in the changed files — worth verifying it includes agents().

Summary

This is a high-quality fix for a genuinely difficult problem. The three-bug decomposition is precise, the fixes are well-motivated, and the test coverage is strong. The main areas I'd want to see addressed:

1. Extract the stable timeout reschedule logic in Think into a helper method (like ai-chat does) to eliminate the inlined duplication
2. Add a test for _chatRecoveryRetry stable timeout in Think
3. Minor: consider a debug log when the ignoreIncompleteToolCalls backstop fires

[ask-bonk]

Review posted on PR #1623. Here's a summary of what I found:

Overall: This is a high-quality hardening PR. The three-bug decomposition (lost ownership, stale-continuation clobber, premature stable_timeout) is precise, and the fixes are well-motivated by a real customer failure mode.

Key findings:

1. Recovery root ownership fix is correct — keying off recoveryRootRequestId instead of per-continuation requestId is the right design. Noted a subtle invariant that the _activeChatRecoveryRootRequestId instance field relies on turn queue serialization for safety.

2. Superseded continuation logic is the most important behavioral change — correctly distinguishes between recovery's own progress (skip benignly, don't touch submission) vs. a genuinely new user turn (mark skipped). Noted an edge case where lastLeaf is null after a clear.

3. idempotent: false for stable timeout reschedule is a gem-level insight with excellent test coverage proving the non-dedup behavior with a pre-inserted schedule row.

4. Tool call preservation (delete → error) is clearly correct and the JSON input normalization is robust.

5. CompactContext flow-through is backward-compatible and structurally sound.

Three actionable suggestions:

• Extract the duplicated stable timeout reschedule logic in Think into a helper (like ai-chat already does)
• Add a test for _chatRecoveryRetry stable timeout in Think (currently only _chatRecoveryContinue is tested)
• Consider a debug log when ignoreIncompleteToolCalls backstop fires, to surface repair gaps

github run

[pkg-pr-new]

Open in StackBlitz

agents

npm i https://pkg.pr.new/agents@1623

@cloudflare/ai-chat

npm i https://pkg.pr.new/@cloudflare/ai-chat@1623

@cloudflare/codemode

npm i https://pkg.pr.new/@cloudflare/codemode@1623

hono-agents

npm i https://pkg.pr.new/hono-agents@1623

@cloudflare/shell

npm i https://pkg.pr.new/@cloudflare/shell@1623

@cloudflare/think

npm i https://pkg.pr.new/@cloudflare/think@1623

@cloudflare/voice

npm i https://pkg.pr.new/@cloudflare/voice@1623

@cloudflare/worker-bundler

npm i https://pkg.pr.new/@cloudflare/worker-bundler@1623

commit: be29d60

[threepointone]

Update — review-driven fixes + a terminal-state sweep

Since the deep review, I've addressed both review findings and run a focused sweep for the bug class they exposed ("is every terminal tool/stream state enumerated as terminal?"). All of the below are on the branch; npm run check is green (91/91 typecheck, lint + format clean) and every fix has a regression test verified to fail before the fix.

Review findings

• Submission abandon-paths used the per-continuation requestId (1e36ea3a). _markRecoveredSubmissionInterrupted was called with the per-continuation id in both terminal abandon paths — recovery exhaustion (_exhaustChatRecovery) and { continue: false }. Under chained continuations the submission row still carries the recovery-root id, so the lookup missed it and left the submission stuck running. Now keyed off the recovery root (stored on the incident record). The reviewer flagged the continue_disabled path; the exhaustion path had the identical defect and is fixed too.
• output-error not treated as settled in transcript repair (c3b71cdb). _repairToolTranscriptParts.hasOutput omitted output-error, so an already-healed (or legitimately errored) tool call was re-flipped every turn — clobbering a real errorText with the generic "interrupted" message and emitting spurious chat:transcript:repaired events/writes/broadcasts for the life of the conversation. Now settled.

Sweep fixes (same bug class, additional instances)

• output-denied not recognized as a settled tool state in Think (5077de94). The AI SDK converts output-denied (a user-denied approval) into a denial tool-result, but Think's repair, backstop detector, and immediate-flush check all omitted it — so repair turned denials into generic errors and the denial wasn't durably flushed. Centralized the terminal-state check into one shared _toolPartHasSettledResult predicate so repair and the backstop can't drift again, and added tool-output-denied to the immediate-flush set.
• Stall-watchdog wrapper leaked the source stream on early exit (3d77ff4e). The inactivity-watchdog generator (from the watchdog feature in this PR) didn't forward .return() to the source on an early consumer break (notably an in-band stream error, where the abort signal isn't set), leaving the underlying ReadableStream un-cancelled. Now cancels on early termination, except after a watchdog-triggered abort (which already tore it down).
• Shared reconciler clobbered server-resolved errors/denials (9b522fd7). reconcileMessages (run at persist by both Think and AIChatAgent) only protected output-available from a stale client part — so a client persisting a stale input-available for a tool the server had resolved to output-error/output-denied overwrote the resolved result. Now protects all three terminal states, overlaying the matching field (output/errorText/approval).

Reviewed and confirmed complete (no change needed)

_isTerminalSubmissionStatus, streamIsTerminal, _messageHasPendingInteraction, shouldMarkSkippedAfterGenerationChange, and AIChatAgent's own tool-state handling (it's the HITL home and already enumerates output-denied everywhere it matters).

Also addressed from the review

• Extracted the stable-timeout reschedule into a shared _rescheduleRecoveryAfterStableTimeout helper (mirrors @cloudflare/ai-chat) + added the missing _chatRecoveryRetry reschedule test (90655e3e).
• Docs aligned with the behavior changes, incl. the new chat:stream:stalled event and chatStreamStallTimeoutMs (893643c7, 4b1d7f73).

Deferred (tracked, intentionally out of scope)

• discussion: first-class prompt caching + a cacheable-but-dynamic system prefix for Think #1624 — first-class prompt caching / cacheable-but-dynamic system prefix.
• discussion: route a stream-stall watchdog abort into bounded recovery instead of a terminal error #1626 — route a stream-stall abort into bounded recovery instead of a terminal error.

[threepointone]

/bonk do another deep review of this PR, look at the existing replies and updates, look for edge cases and production worthiness

[ask-bonk]

Deep Review #2 — PR #1623

I've read every changed file against the current HEAD (9b522fd), the first review's findings, and the follow-up fix commits. This review focuses on production-worthiness: edge cases that survived the first review pass, latent correctness issues in the new code, and things that could bite in long-running production deploys.

---

Overall Assessment

The follow-up commits (1e36ea3a through 9b522fd7) addressed the first review's findings well — the stable timeout helper was deduplicated, the output-error/output-denied terminal-state gaps were closed, and the reconciler now protects all three terminal tool states. The centralized _toolPartHasSettledResult predicate is the right design. The test coverage is unusually thorough for a fix PR of this scope.

The PR is production-worthy with the items below addressed or acknowledged.

---

1. Dead Code in _repairTranscriptForProvider (think.ts:1632-1638)

The removedIds branch is now dead code. _repairToolTranscriptParts no longer removes messages — it always pushes every message to repaired (line 1613: repaired.push(messageChanged ? { ...message, parts } : message)), so repaired.length === messages.length always. The repairedIds set will contain every ID from the input, and removedIds will always be empty.

This is harmless but misleading — it suggests messages can still be deleted by repair, which is no longer true (the whole point of this fix). Consider removing the removedIds branch to keep the code aligned with its behavior, or at minimum add a comment explaining it's vestigial.

2. _completeRecoveredSubmission SELECT can find stale rows (think.ts:7520-7528)

After the UPDATE WHERE request_id = ${originalRequestId} AND status = 'running', the SELECT queries WHERE request_id = COALESCE(${requestId}, ${originalRequestId}). If the UPDATE matched and changed request_id from originalRequestId to requestId, this correctly finds the updated row.

But if the UPDATE matched zero rows (no running submission with that originalRequestId — possible if the submission was concurrently completed between the _hasRunningSubmission guard and this call), and requestId is non-null, the SELECT searches for request_id = ${requestId}. If a previous submission happened to use that same request ID, we'd find and emit a status event for a stale, unrelated submission.

In practice, request IDs are random-enough that collision is negligible, and the _isTerminalSubmissionStatus guard (line 7530) prevents most damage. But the gap is architecturally subtle. A tighter fix: do the UPDATE with RETURNING (D1/SQLite supports it) to atomically get the updated row, avoiding the two-query race entirely.

3. _normalizeToolInput doesn't handle array-typed inputs (think.ts:1492)

The heuristic checks raw.trim().startsWith("{") to decide whether to JSON-parse a stringified input. A stringified array input like '[{"key": "value"}]' won't be parsed — it starts with [, not {. If a tool receives array-typed args that were stringified (same way objects get stringified), they'd persist as a raw string forever.

This may be acceptable if array-typed tool inputs are not expected from any provider, but worth documenting the assumption. The fix is trivial: change startsWith("{") to startsWith("{") || startsWith("[").

4. Stall watchdog: timer accumulation on very-high-throughput streams

In _iterateWithStallWatchdog (think.ts:6300-6332), a new setTimeout is created for every chunk and cleared after the race. On a high-throughput stream (thousands of chunks/second from a fast model), this creates and clears thousands of timers per second. The Workers runtime handles this fine, but the pattern allocates a new Promise + setTimeout per chunk, which is more GC pressure than necessary.

An alternative: use a single setTimeout that is .refresh()'d on each chunk (Node 10+ / Workers support timer.refresh()). This avoids the per-chunk Promise allocation. Not a blocking concern — the current approach is correct — but a production optimization worth considering for high-volume streaming.

5. AgentToolRunInspection.status excludes "interrupted" (agent-tool-types.ts:84)

AgentToolRunInspection is typed as Exclude<AgentToolRunStatus, "interrupted">, but AgentToolRunState (line 131) and the database both include "interrupted" as a valid status. The child adapter's inspectAgentToolRun returns AgentToolRunInspection, but the agent-tool recovery code at index.ts:4788 reads row.status !== "interrupted" — meaning the database can have interrupted status, but the type system says the inspection result can't.

This looks like a pre-existing inconsistency that this PR inherits rather than introduces. But since this PR adds the AgentToolFailure type and exports it, it's worth flagging: a consumer who calls inspectAgentToolRun and exhaustively switches on status won't handle "interrupted" because the type excludes it, even though the runtime value can be "interrupted".

6. AIChatAgent _chatRecoveryContinue doesn't distinguish assistant-leaf vs. user-leaf supersession

Think's _chatRecoveryContinue (think.ts:7607-7625) carefully distinguishes:

• Leaf is assistant → recovery's own progress, skip benignly, don't touch submission
• Leaf is user → genuinely new turn, mark submission skipped

AIChatAgent's equivalent (ai-chat/index.ts:3417-3424) just marks skipped/conversation_changed regardless. This is correct because AIChatAgent has no submission layer, but it means the two implementations have different semantics for the same concept name (conversation_changed). If AIChatAgent ever gains submissions, this will silently clobber them. A comment noting this divergence would prevent a future regression.

7. Recovery progress marker: forward progress can be false-positive after compaction

_chatRecoveryProgressMarker() (used in _beginChatRecoveryIncident at line 6833) likely counts assistant messages. After a compaction, the message count drops. If recovery fires post-compaction, currentProgress < prevProgress even though the turn actually advanced — madeProgress would be false, and the attempt counter wouldn't reset.

The inverse is also possible: if compaction happens during a recovery chain (auto-compaction fires on appendMessage), the progress marker jumps down, making the next interruption look like no progress was made. This would be rare (compaction + churn simultaneously) but could cause premature budget exhaustion.

If the progress marker is based on persisted row IDs (auto-incrementing) rather than count, this concern doesn't apply — worth confirming.

8. Test gap: concurrent tool output + repair race

The repair path (_repairTranscriptForProvider) runs at the start of _runInferenceLoop (line 2562). If a HITL tool-output arrives via WebSocket between the repair check and convertToModelMessages (line 2584), the repair would have already flipped that tool to output-error, but the real output wouldn't be in the repaired message. The ignoreIncompleteToolCalls backstop would then drop it.

This race window is small (synchronous code between lines 2562 and 2584), so it's likely safe in practice because the turn queue serializes chat turns. But addToolOutput from a WebSocket handler doesn't go through the turn queue — it's a direct mutation. A test that calls addToolOutput concurrently with a turn start would verify the invariant holds.

9. Minor: removedToolCalls counter name is now misleading

In _repairToolTranscriptParts, the removedToolCalls counter (line 1557) is incremented when a tool call is flipped to errored — but nothing is removed. The name is a holdover from the old delete behavior. It flows through to the chat:transcript:repaired observability event. The semantic mismatch between the field name and behavior could confuse observability consumers.

Consider renaming to repairedToolCalls or erroredToolCalls for clarity. Not blocking, but a low-effort improvement.

10. Reconciler: mergeServerToolOutputs field-overlay could carry stale output onto an output-error part

In message-reconciler.ts:131-137, the overlay carries output, errorText, and approval from the server part when the server state is terminal. But it applies all three fields unconditionally via spread:

...("output" in server ? { output: server.output } : {}),
...("errorText" in server ? { errorText: server.errorText } : {}),
...("approval" in server ? { approval: server.approval } : {})

If a server part has state: "output-error" and a leftover output field (e.g., from a partial output before the error), both output and errorText would be overlaid onto the client part. The AI SDK might interpret the presence of output as a valid result despite the output-error state. This depends on whether the SDK checks state or output first. If the server-side tool state is always clean (no leftover output on output-error), this is fine.

---

Summary

The PR is solid. The five original bugs are correctly fixed, the follow-up sweep caught real additional instances of the same class, and test coverage is strong. The items above are mostly "hardening the hardening" — none are likely to cause production incidents, but items 1, 3, and 9 are low-effort improvements worth addressing before merge. Items 2 and 7 are worth confirming the invariants hold.

[ask-bonk]

Review posted on PR #1623. Here's a summary of what I found:

Overall verdict: The PR is production-worthy. The original five bugs are correctly fixed, the follow-up terminal-state sweep was thorough, and test coverage is strong.

10 findings, ranked by priority:

1. Dead code — _repairTranscriptForProvider's removedIds branch is now unreachable since repair no longer deletes messages. Misleading but harmless.

2. Stale row risk in _completeRecoveredSubmission — The two-query UPDATE+SELECT pattern has a subtle TOCTOU window where the SELECT could find an unrelated historical submission if the UPDATE matched zero rows. Low probability due to random request IDs, but UPDATE ... RETURNING would close it.

3. _normalizeToolInput ignores array inputs — The startsWith("{") heuristic won't parse stringified arrays like '[{"key": "val"}]'. Trivial fix: add || startsWith("[").

4. Stall watchdog timer churn — Creates a new Promise+setTimeout per stream chunk. A timer.refresh() pattern would reduce GC pressure on high-throughput streams. Correct as-is.

5. AgentToolRunInspection type excludes "interrupted" — Pre-existing inconsistency, but worth flagging since this PR exports the new AgentToolFailure type alongside it.

6. AIChatAgent supersession semantics differ from Think — AIChatAgent doesn't distinguish assistant-leaf vs. user-leaf in conversation_changed. Correct today (no submission layer), but a future regression risk.

7. Progress marker could interact badly with compaction — If _chatRecoveryProgressMarker() counts messages, compaction can make progress appear to go backwards, preventing attempt budget reset.

8. Test gap: concurrent HITL output + repair — addToolOutput doesn't go through the turn queue, so a WebSocket tool output arriving during _runInferenceLoop could race with repair.

9. removedToolCalls counter name is misleading — Should be repairedToolCalls since nothing is removed anymore. Flows to observability events.

10. Reconciler overlay carries all fields — mergeServerToolOutputs overlays output+errorText+approval unconditionally from server parts, which could put both output and errorText on an output-error part if the server data isn't clean.

Items 1, 3, and 9 are low-effort improvements worth addressing before merge. Items 2 and 7 need invariant confirmation.

github run

[threepointone]

Deep review #2 — disposition

Triaged all 10 items against HEAD. Landed the clear wins in be29d607; confirmed the rest with reasons; filed one genuine follow-up. Details below so anyone can pick this up later.

Landed (be29d607)

• make an mcp guide #1 Dead code — removed the unreachable removedIds/deleteMessages branch in _repairTranscriptForProvider (repair preserves every message now, never deletes). Comment added.
• Version Packages #3 Stringified-array inputs — _normalizeToolInput now parses a stringified [...] input, not just {...}. + test (message-reconciliation.test.ts).
• better error handling everywhere #10 Reconciler overlay — mergeServerToolOutputs overlay is now state-driven: only the field matching the server's terminal state is carried (output for output-available, errorText for output-error, approval for output-denied), so a stray output on an output-error part can't ride along. + test asserting output is dropped.
• Version Packages #6 ai-chat divergence — added a comment in _chatRecoveryContinue explaining AIChatAgent intentionally does NOT split assistant-leaf vs user-leaf on conversation_changed (no durable-submission layer to protect), and to mirror Think if submissions are ever added.

Confirmed — not changing (with reasons)

• ☂️ agents-sdk roadmap #2 Two-query SELECT race — negligible: it's a crypto.randomUUID() collision risk. In the realistic path the SELECT re-finds the same submission (whose request_id the UPDATE set), and _isTerminalSubmissionStatus gates the emit. RETURNING is used in exactly one other place in the codebase; not worth diverging the submission code's style for a UUID-collision-only risk.
• Version Packages #5 AgentToolRunInspection excludes "interrupted" — valid but pre-existing and unrelated to this PR's diffs. Should be a separate type-consistency issue, not folded into this PR. (Flagging here for tracking; happy to file if wanted.)
• add .state sync to the normal agent client #9 removedToolCalls name — misleading, but it's a field on the already-released chat:transcript:repaired event, so renaming is a breaking change, not free cleanup. Docs already clarify the semantics ("counts orphaned tool calls healed — preserved as errored, not deleted"). Left as-is deliberately.
• Version Packages #4 Watchdog per-chunk timer — acknowledged micro-opt. .refresh() doesn't compose cleanly with the race-and-abandon pattern (loses the per-iteration reject handle); it's an opt-in watchdog and the runtime handles the allocation fine. Not worth the added complexity/risk.
• Version Packages #8 addToolOutput-vs-repair race — acknowledged. The turn queue serializes turns; the residual is a direct addToolOutput WS mutation during one of repair's awaits. Real but narrow, and a deterministic test would be flaky. Tracked as a known invariant.

Deferred — filed as a follow-up

• Version Packages #7 Progress marker is count-based — confirmed real. _chatRecoveryProgressMarker() counts current assistant messages and progress is stored as a Math.max high-water mark, so auto-compaction (which collapses messages into a summary overlay) drops the count → madeProgress reads false → the attempt budget isn't reset → premature exhaustion under simultaneous compaction + deploy churn. The wall-clock ceiling still bounds it (no infinite loop), so it's a premature-failure risk. Proper fix is a monotonic progress signal (persisted ever-incrementing assistant-persist counter, or a non-decreasing ordinal), applied to both Think and ai-chat. Full detail + repro in bug: chat-recovery progress marker is count-based, so compaction can cause premature budget exhaustion #1628.

Verification

npm run check green (91/91 typecheck, lint + format clean). Reconciler unit (30), Think reconciliation (8), ai-chat reconciliation (41) suites pass.