Repository navigation
fix(claude): a queued message Claude withdrew is settled from Claude's own cancelled event - #23862
Conversation
…lled frame Claude reports each uuid-stamped command's lifecycle (queued, started, completed, cancelled). A send it withdraws from its queue gets `cancelled` before the interrupt or cancel_async_message answer, so a lost or failed answer no longer leaves that send pending: it settles as withdrawn, with the same reason and words as the receipt path. A command the CLI already started also ends `cancelled` when its turn is interrupted or fails, so `cancelled` after `started` is not a withdrawal; an echoed send has left the waiter lists and is never reached. Tests replay real 2.1.280 captures, scrubbed.
…idle A Claude send whose write ended in doubt is recorded `unknown`, and a live `unknown` reads as work still owed, so the chat showed Working until the child exited. Claude sends `session_state_changed idle` only once its whole queue has drained, so it can no longer be holding that send. The runtime now routes that report to the host's existing release, the same one Codex's thread-stopped report uses; it retires `unknown` only, never `pending`.
 |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 📝 WalkthroughWalkthroughClaude command-lifecycle frames now update dispatch waiter state and can settle a queued command when it is cancelled before starting. Claude session-idle notifications now trigger release of unanswered dispatches. Added replay captures and tests cover command cancellation, withdrawal, idle notifications, and recovery of unknown dispatches. Priority: ⚪ Not assessed Merge Risk: 🔵 Low · up to Repeated Claude idle notifications can add redundant recovery history, and retired-waiter cancellation still lacks replay coverage. The known effects are bounded, but should be addressed or accepted with owner awareness. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change appears confined to an existing Claude session and improves recovery of cancelled or unanswered messages. Repeated idle notifications could cause redundant recovery writes, but no new external entrypoint or verified security vulnerability was identified. Retained concerns
Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 6.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 15 functions across 8 files. (4 skipped: 4 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ No critical issues — one coverage suggestion inline.
Reviewed changes
- Claude
command_lifecyclesettlement (newclaude-command-lifecycle.ts):resolveClaudeReplayTurnroutes each per-command frame toobserveClaudeCommandLifecycle, which recordsqueued/startedon the in-memory waiter and settles acancelledsend that never reachedstartedas withdrawn through the existingsettleCancelledClaudeDispatchWaiters. - Idle release for doubted sends:
session_state_changed idlenow fires a newonSessionIdledep, wired in the runtime to the samereleaseUnansweredDispatchespath Codex already uses; onlyunknownsubmissions are released, neverpending. - Runtime refactor: the Codex inline release callback becomes the shared
releaseUnansweredDispatches;structured-claude-runtime-adapter.tsthreadsonSessionIdlethrough. - Tests and fixtures: four scrubbed Claude 2.1.280 captures plus
claude-command-lifecycle.test.ts(15 tests) andclaude-structured-idle-releases-unknown.test.ts(real runtime/host) — 20/20 pass locally.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
| const waiter = [...session.dispatchWaiters, ...session.retiredDispatchWaiters].find( | ||
| (candidate) => candidate.sentUuid === commandUuid | ||
| ) |
There was a problem hiding this comment.
The retiredDispatchWaiters half of this lookup is load-bearing but untested: a send whose write ended in doubt can still have been queued and later withdrawn by the CLI, and this branch is what upgrades that lingering unknown to a proven rejected. The four replay fixtures all dispatch successfully, so they exercise only the live-waiter path.
Technical details
# Untested retired-waiter settlement path
## Affected sites
- `src/main/claude/claude-command-lifecycle.ts:20` — `retiredDispatchWaiters` lookup; no test reaches it
- `src/main/claude/claude-command-lifecycle.test.ts` — every replayed dispatch resolves `{ state: 'admitted' }`, so waiters stay live
## Required outcome
A test in which `connection.send` throws after `beforeDispatch` (dispatch recorded `unknown`, waiter retired), the CLI then emits `command_lifecycle queued` and `command_lifecycle cancelled` for the same `command_uuid`, and the send settles as `rejected`/`cancelled` rather than staying `unknown`.
## Suggested approach
Reuse the `claude-command-lifecycle.test.ts` replay harness or the host harness in `claude-structured-idle-releases-unknown.test.ts`; make `send` throw (as the idle test does) and deliver the two captured lifecycle frames.… queued; fixtures name msg_lifecycle_v1
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🟡 Minor · Serialize idle recovery at the host boundary. · structured-agent-session-runtime.ts:218-230
src/main/runtime/structured-agent-session-runtime.ts:218-230
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winSerialize idle recovery at the host boundary.
onSessionIdlecan invoke the shared callback for multiple idle frames. The runtime does not await the host release. The host scans the unrecovered entries beforeJournalWriteQueuecommits the deferred row. Two idle frames can therefore select the same entry. Each call inserts a new recovered dispatch row with a new sequence. The reducer keeps one current submission, but the journal retains duplicate recovery transitions and emits an extra commit.Serialize the scan and writes in the host operation:
Suggested fix
diff --git a/src/main/native-chat/agent-session-wire/structured-agent-session-unanswered-dispatch-release.ts b/src/main/native-chat/agent-session-wire/structured-agent-session-unanswered-dispatch-release.ts @@ export async function releaseStructuredAgentSessionUnansweredDispatches( - context: Pick<StructuredAgentSessionMutationContext, 'sessions'> & { + context: Pick<StructuredAgentSessionMutationContext, 'serialize' | 'sessions'> & { deps: { store: Pick<StructuredAgentSessionHostDeps['store'], 'getRecord'> } }, input: { sessionId: string; reason: string } ): Promise<void> { - const session = context.sessions.get(input.sessionId) - if (!session) { - return - } - const stranded = session.journal - .submissions() - .filter((entry) => entry.dispatchState === 'unknown' && entry.recovered !== true) - if (stranded.length === 0) { - return - } - for (const entry of stranded) { - await session.journal.resolveDispatch({ - clientMessageId: entry.clientMessageId, - state: 'unknown', - // The earlier reason names a sharper fact than this one does. - reason: entry.reason ?? input.reason, - fence: structuredAgentSessionConversationFence(context.deps.store, input.sessionId), - recovered: true + return context.serialize(input.sessionId, async () => { + const session = context.sessions.get(input.sessionId) + if (!session) { + return + } + const stranded = session.journal + .submissions() + .filter((entry) => entry.dispatchState === 'unknown' && entry.recovered !== true) + if (stranded.length === 0) { + return + } + for (const entry of stranded) { + await session.journal.resolveDispatch({ + clientMessageId: entry.clientMessageId, + state: 'unknown', + // The earlier reason names a sharper fact than this one does. + reason: entry.reason ?? input.reason, + fence: structuredAgentSessionConversationFence(context.deps.store, input.sessionId), + recovered: true + }) + } + }) - }) - } } diff --git a/src/main/native-chat/agent-session-wire/structured-agent-session-unanswered-dispatch-release.test.ts b/src/main/native-chat/agent-session-wire/structured-agent-session-unanswered-dispatch-release.test.ts @@ const context = { sessions: new Map([['s-1', { journal }]]), + serialize: async (_sessionId: string, task: () => Promise<unknown>) => task(), deps: { store: { getRecord: () => ({ lease: { runtimeFence: FENCE } }) } } } as unknown as ReleaseContext
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: 08a55e9e-7fb3-4e0c-acbb-4338037406cc
📒 Files selected for processing (6)
src/main/claude/__fixtures__/claude-lifecycle-capture-auth-failed.jsonlsrc/main/claude/__fixtures__/claude-lifecycle-capture-batch-lead.jsonlsrc/main/claude/__fixtures__/claude-lifecycle-capture-cancel-async.jsonlsrc/main/claude/__fixtures__/claude-lifecycle-capture-interrupt-lost.jsonlsrc/main/claude/claude-command-lifecycle.test.tssrc/main/claude/claude-command-lifecycle.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 3 remain after this review.
There was a problem hiding this comment.
ℹ️ No new issues in the new commits — the forward-only lifecycle guard and its test look correct. One prior-coverage thread is still outstanding, so this stays unapproved rather than routing an auto-fix.
Reviewed changes
- Renamed the fixture capability token
msg_scrubbed01_v1→msg_lifecycle_v1, the token actually observed from Claude CLI 2.1.280; no production code reads either string. - Made
observeClaudeCommandLifecycle's mark forward-only: a redeliveredqueuedno longer downgrades a waiter already markedstarted, so a latercancelledcannot falsely settle a command the CLI had started. - Added an
afterFramehook to the replay harness and a test injecting aqueuedframe afterstarted. I confirmed the test fails against the pre-guard logic (the send settlesrejected), so it is not theatre.
The prior review's one open suggestion — direct coverage for the retiredDispatchWaiters branch — is unchanged by these commits, so that thread stays open.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
StatusReady. Head The failure: a queued Claude follow-up that Claude withdrew at a Stop stayed "sent, waiting", and the chat read Working, whenever Orca missed Claude's reply to the Stop. Nothing else recorded the withdrawal. The fix: Claude reports each message's progress by the id Orca sent. A message Claude cancels before starting it is now settled as withdrawn from that event, through the same function the Stop reply uses. A message Claude already started or repeated back is never withdrawn this way. Claude's idle report now releases a message whose outcome was unknown, the way Codex already does. Nothing new is saved, and there is no wire change. Review: one focused loop, CLEAN. Two small fixes came from its notes: the started mark only moves forward, and the fixtures name the real capability. Validation:
Known, next PR: a message Claude started but never repeated back, then cancelled, still waits until the process exits. |
1 participant
ELI5
When you stopped a Claude chat that had a follow-up message queued, the follow-up could be left marked "sent" forever and the chat kept saying Working, if Orca missed Claude's reply to the Stop. Claude also announces, message by message, when it drops one. Orca now listens for that and marks the dropped follow-up withdrawn, so the chat settles.
The plan this belongs to
Goal: every Claude message Orca hands over has a way to end, with no timers and nothing new saved. It ships as two PRs:
discardedandrefusedreports are read; and the idle cleanup no longer stops Claude while it is working on a message.What Changed
Before
After
Mechanism
claude-replay-turn-resolution.tsroutes eachcommand_lifecycleframe toclaude-command-lifecycle.ts.queuedandstartedmark the matching send in memory.cancelledfor a send that never reachedstartedgoes through the existingsettleCancelledClaudeDispatchWaiters.claude-structured-session-acquisition.ts:session_state_changedidle calls the runtime's existing idle release, which Codex already uses (structured-agent-session-runtime.ts).Why
Claude's own per-message event is the provider's fact about what it dropped, so settling from it gives every such message a way to end without a timer and without guessing. The existing Stop-reply path already settles the same way, so both now write through one function.
Alternatives considered:
Differences from the common pattern
Linked Issue
No issue. Follow-up from #23553 and #23026: every pending Claude message should have a way to end.
Visual Proof
Live check on a second Mac with the real Claude CLI (2.1.283), at head
45212a99100, through the app's own Stop button.cancelledevent came before Claude's reply to the Stop.cancelledevent alone, before the held reply arrived; the chat looked the same as in the other runsThe run with Claude's reply held back: the reply stops and the follow-up's text is back in the message box:
Not exercised live, covered by the captured-frame tests: the one-at-a-time cancel (the app does not send it), and releasing an unknown-outcome message on Claude's idle report (it cannot be produced from the app).
Testing
I manually tested these changes locally
Automated tests added/updated, or explained why not below
The frames were captured live from Claude Code 2.1.280, with account identifiers scrubbed, as
__fixtures__/claude-lifecycle-capture-{interrupt-lost,cancel-async,batch-lead,auth-failed}.jsonl.claude-command-lifecycle.test.ts(16 tests) replays them:claude-structured-idle-releases-unknown.test.ts(the real runtime and host): idle releases an unknown message, and a pending one is untouched.11 of the 14 new behaviour tests fail on
main; the other three pin behaviour that already held. Each behaviour was also removed in turn, and its test failed by assertion.pnpm tc:nodeandtc:cli, oxlint,check:code-quality:changed,check:react-doctor:changedand the anti-slop audit pass.AI Disclosure
Review
One focused review loop: CLEAN. It read the 2.1.280 lifecycle code and found no way for a
cancelledevent to withdraw a message the model received. Two small fixes from its notes: a message's started mark now only moves forward (a redelivered command can re-emit "queued"), and the fixtures name the realmsg_lifecycle_v1capability.Agent skill upstream boundary
docs/reference/agent-skill-sharing-upstream-boundary.mdand copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.Notes
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)Author: @BrennanKB5