Skip to content

fix(claude): end a message Claude started but never confirmed, and keep Claude running while it holds one - #23898

Merged
brennanb2025 merged 16 commits into
mainfrom
brennanb2025/claude-lifecycle-pr2
Sep 30, 2026
Merged

brennanb2025 merged 16 commits into
mainfrom
brennanb2025/claude-lifecycle-pr2

Conversation

@brennanb2025

@brennanb2025 brennanb2025 commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor
Files Added Deleted Net
Test 4 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​338 $\color{#cf222e}{\Huge{\mathbf{−}}}$​12 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​326
Prod 18 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​196 $\color{#cf222e}{\Huge{\mathbf{−}}}$​71 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​125

ELI5

When you send a Claude chat a message, Orca marks it "sent, waiting" until Claude confirms it. Sometimes Claude starts on a message and then ends that turn early, or goes quiet, without ever confirming it. That message then stayed "waiting" until the Claude process exited, and the chat kept saying Working. Now Orca reads what Claude reports about each message and gives it an honest ending. It never calls a message withdrawn if Claude may already have acted on it.

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:

What Changed

The problem. After #23862, three cases still had no ending:

  • A message Claude had started but never repeated back, and then ended, whether a Stop interrupted it or the turn failed. It stayed pending until the process exited, which in practice meant the idle cleanup stopping Claude about 30 minutes later. The chat said Working throughout.
  • Claude's other per-message end reports, discarded and refused, were ignored. Only cancelled was read.
  • The idle cleanup could stop a Claude process that was still working on a message it had started, for example during a silent rate-limit retry. The cleanup only counted rows in the chat, and a message that is still pending adds none.

Scope. Claude chats, on the desktop app and on SSH hosts: the host runs Claude and reads its reports. It needs a Claude CLI that sends per-message reports (2.1.261 or newer). Claude CLIs older than 2.1.261 keep #23862's behaviour, where the idle cleanup stopping the process is what ends a held message. Codex chats are not affected.

Before / After

Claude reports, for a message it never repeated back Before After
started, then cancelled, discarded or refused stays "waiting"; chat says Working until the process exits "Message delivery is unconfirmed." with Retry; the chat stops saying Working. Nothing is re-sent automatically, because Claude may already have acted on it
started, then Claude goes idle with no end report (a turn that failed) same same as above
discarded before it started (Claude ended its session with it still queued) stays "waiting" until exit "This message was not delivered." with Retry
refused before it started stays "waiting" until exit "The provider did not accept this message." with Retry, marked Failed
cancelled before it started withdrawn (#23862) unchanged
still only queued when Claude goes idle stays "waiting" unchanged on purpose: Claude 2.1.280's code reports idle before it checks its queue again, so the message may still start

If Claude's repeat-back of the message arrives after Orca called it unconfirmed, the message is still marked delivered.

The idle cleanup no longer stops a Claude process while it holds a message it has taken, queued or started, and not yet confirmed.

Mechanism

  • claude-command-lifecycle.ts: a terminal report (cancelled, discarded or refused) for a message that never started rejects it with the matching reason. After started, the same report means only that its turn ended, so the message is released as doubt through the new releaseClaudeDispatchWaitersInDoubt (claude-structured-dispatch.ts). The pairing record is retired, not dropped, so a late repeat-back still marks it delivered. The host writes the result through the existing late-settlement path as "unknown, recovered", so it is never re-sent on its own.
  • claude-replay-turn-resolution.ts: when Claude reports its session idle, a message still marked started is released the same way.
  • claudeHoldsDispatch is derived on every check from the in-memory pairing records: true while one is queued or started and unconfirmed. Releasing at idle still covers only started. It reaches the idle cleanup's "work still owed" check (structured-agent-session-idle-sweep.ts) through the Claude adapter and the router.
  • Nothing is saved, and nothing changes on the wire. The "unknown, recovered" state and its messages already exist.

Why

These reports are Claude's own per-message facts, so every held message ends from something Claude said, not from a timer or a guess. A message Claude started may already be in the conversation, so it is never called withdrawn or refused. It is "unconfirmed", the same word Orca already uses when it cannot prove delivery.

Why nothing can be held forever. The hold is derived, not stored. Claude's per-message schema gives every taken command queued, then started, then exactly one end state; its one documented stuck-queued case is a peer message, which Orca never sends, and every queued command in the captured runs ends. A pairing record stops holding on any of these:

  • its repeat-back;
  • a terminal report;
  • Claude going idle;
  • a failed write;
  • the process exiting.

A Claude process that has taken a message (queued or started) but not yet repeated it back is now kept running for as long as it stays in that state, for example while it retries a rate-limited request before its first reply. Before, the idle cleanup stopped it after 30 minutes of silence. This trade is intended: Claude is still working on the message. Stop still works, and Claude going idle or exiting ends the hold. In every capture and in the 2.1.280 code, Claude repeats a message back before it can go idle, and a turn that fails without going idle happens only while background tasks run, which already keep the process running.

Alternatives considered:

  • For older CLIs without per-message reports, release messages written before a Stop at the next idle, using the order of Claude's input. This was built and then removed in review. On those CLIs, a message released this way that is never repeated back stops Orca matching later messages by content, so every later message would stay waiting. Orca has never seen the failure it was meant to fix.
  • Release a still-queued message at idle too. This would wrongly mark a message unconfirmed when it is about to start. In the 2.1.280 code, Claude reports idle first and checks its queue after. That order is read from the code, since no capture caught a message in that window.
  • A timeout on a waiting message. A timer cannot tell "Claude is slow" from "Claude dropped it".

Differences from the common pattern

  • A common approach ends the Claude session on Stop and treats these per-message reports as noise. Orca keeps the process running across Stop, so the reports are its only per-message fact once a message is handed over.

Linked Issue

No issue. Step 2 of the plan above, following #23553, #23026 and #23862.

Visual Proof

Live on a second Mac (the M4 Air) with the real Claude CLI 2.1.285, one run per arm. Before is current main (26bb7c23f19, which includes #23862). After is this PR (5471ca443bf; case 2 again at 8dd0f818ace). Both cases need Claude to go quiet, or to end a turn, at a precise moment, which the app can't trigger. So both arms carry the same two QA-only changes, never committed:

  • A shorter idle cleanup. The idle window is 20 s instead of 30 minutes, set through a QA-only ORCA_QA_IDLE_MS environment variable. This applies to the first case only.
  • A filter on Claude's output. The filter sits between Orca and the real Claude process. It acts only on a message containing QAHOLD. Once Claude reports that message started, the filter holds back Claude's replies, and passes through only Claude's per-message reports and control frames.

1. Claude stays quiet for 45 s while working on a message (as in a silent rate-limit retry).
The filter releases everything after 45 s.

Before: Claude's process ended during the silence, before the 45 s release, so the filter never delivered its held output. With the 20 s idle window, what ends it there is the idle cleanup, which counted only chat rows and saw none owed. The message ends "Message delivery is unconfirmed." and its answer never arrives.

Before, main: after 45 s of Claude working silently, the follow-up reads "Message delivery is unconfirmed." with Retry, and no answer arrived

After: Claude kept running while it held the message. When the output was released at 45 s, the answer landed.

After, this PR: the same follow-up is answered, "Worked for 45s" then "follow-up answered"

2. Stop on a message Claude started but never repeated back.
The filter throws away Claude's repeat-back and replies. Claude still reports the message cancelled.

Before: 98 s after Claude reported the message cancelled, the chat still says Working, with no ending for the message.

Before, main: after Stop, "Cancellation requested." and the chat still says Working

After: the message reads "Message delivery is unconfirmed." with Retry, and the chat stops saying Working 251 ms after the Stop. It is not called withdrawn, because Claude had started on it. This case was re-run on the head after merging main (8dd0f818ace), because that merge brought in #23861's Stop changes; the result was the same as on 5471ca443bf.

After, this PR at 8dd0f818ace: the message reads "Message delivery is unconfirmed." with Retry, and the send button is back

A steer sent while Claude was running tool calls still folded into the running turn on this PR, and Claude answered it in the same reply.

discarded, refused, and a turn that fails without an end report were not run live: the CLI can't be made to produce them. They are covered by the tests below.

Testing

  • I manually tested these changes locally

  • Automated tests added/updated, or explained why not below

  • claude-command-lifecycle.test.ts:

    • replays the captured 2.1.280 frames, with account identifiers scrubbed;
    • started, not repeated back, then cancelled: unconfirmed, never withdrawn, and a late repeat-back still marks it delivered;
    • discarded and refused, rewritten from the captures, since neither can be produced from the app. Before started, they give not delivered and provider rejected. Claude 2.1.280 sends only cancelled after a start, so the tests with these after started just check that any end there is still unconfirmed, never withdrawn;
    • started, then idle with no end report: unconfirmed;
    • queued, then idle, then started, then repeat-back: delivered, never unconfirmed;
    • a message that is only queued holds the process and is not released at idle; it stops holding once it starts and is repeated back, or ends cancelled.
  • structured-agent-session-idle-sweep.test.ts: the cleanup stays off while a taken message is unconfirmed, and runs again once it ends.

  • claude-structured-idle-releases-unknown.test.ts runs the real runtime, router and host. A started message that is then cancelled, or left behind when Claude goes idle, is recorded as unknown and recovered. It holds the process from the cleanup until then, and afterwards the chat stops reading Working.

  • Every new behaviour test fails on fix(claude): a queued message Claude withdrew is settled from Claude's own cancelled event #23862's head. Each behaviour was also removed in turn, and its test failed by assertion. That makes 10 removals, including counting a queued message at idle and in the hold.

  • 73 related test files pass: 691 tests.

  • tc:node and tc:cli, oxlint, check:code-quality:changed, check:react-doctor:changed and the anti-slop audit pass.

AI Disclosure

Review

Two review loops.

  • Loop 1 found a regression in an extra path for Claude CLIs older than 2.1.261: releasing messages written before a Stop at the next idle stopped Orca matching later messages by content. That path was removed. Loop 1 also found that counting a still-queued message at idle wrongly marked a message unconfirmed in the moment before it starts, so only started counts now.
  • Loop 2 found no code issues, only comments and this description claiming more than the evidence shows. Those were corrected: which end states follow a start, which idle order comes from Claude's code rather than a capture, and the longer hold before Claude's first reply.
  • Follow-up to match the common idle cleanup: the hold now covers a queued message as well as a started one, while releasing at idle stays started-only. Four new tests fail without it.

Agent skill upstream boundary

  • Not applicable, or this change follows docs/reference/agent-skill-sharing-upstream-boundary.md and copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.

Notes

Known limits:

  • A Stop that lands while Claude runs its hooks before a turn starts: Claude reports the message started and then cancelled, and then its Stop reply lists the message as withdrawn. The chat briefly shows "unconfirmed", then "withdrawn". The final state is right.
  • A message Claude only ever reports as queued and never starts still ends when the process exits.
  • Claude CLIs older than 2.1.261 send no per-message reports and keep fix(claude): a queued message Claude withdrew is settled from Claude's own cancelled event #23862's behaviour.

Same on macOS, Linux, Windows and SSH hosts: the host reads its own Claude process.

Checklist

  • This PR is small and focused
  • I explained what changed and why (ELI5, the user-facing before/after, the mechanism, and why over the alternatives)
  • Before/after screenshots or videos attached for UI changes, or N/A with reason
  • Self-reviewed for correctness, security, and performance
  • Cross-platform, SSH/remote, and path/shortcut impact considered (or N/A)
  • pnpm lint, pnpm typecheck, pnpm test, and pnpm build pass (or CI will cover; local preferred)

Author: @BrennanKB5

…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`.
…never echoed

A send the CLI started, then cancelled before any echo, stayed pending: it may
already be in the conversation, so it is released as doubt (unknown, recovered),
never withdrawn and never re-sent. A late echo still accepts it.

The 2.1.280 schema has two more terminal states. `discarded` (the CLI ended its
session with the send still queued) settles as not delivered; `refused`
(declined before it queued) settles as not accepted by the provider. After
`started`, either one is doubt, as `cancelled` is.

The late-settlement path gains an `unknown` outcome, which the host records as
released doubt.
…goes idle

`session_state_changed idle` comes only once the CLI's queue has drained, so a
send it took that is still unanswered there got no echo and never will: a turn
that throws can leave `started` with no terminal state. Idle releases it as
doubt.

What proves the CLI took a send is its lifecycle frame. On a CLI that reports no
lifecycle, it is the send's place on stdin: one whose write finished before an
interrupt went out was read before the interrupt was, so the first idle after
that interrupt releases it too. A send armed ahead of the interrupt but written
after it is left alone, since the CLI may still run it.
… send

A Claude retrying a rate-limited request has taken the send but echoes nothing,
so no turn row exists yet and the sweep rested the child after the idle window,
turning the send into doubt. The adapter now reports whether the CLI holds a
send (lifecycle `queued` or `started`, not yet echoed or ended), derived from
the live waiters, and owed work counts it.

Nothing is stored: every held send leaves the live set on its echo, its
terminal lifecycle state, the CLI's idle, or the child's exit, so the hold ends
with the send.
2.1.280's end-of-turn cleanup can report idle before it re-reads its queue, so
a send read in that window goes queued, idle, started. Releasing every taken
send at idle doubted that live send and dropped Working. Only a `started` send
is released at idle or keeps the child from the idle sweep; a `queued` one ends
by starting and echoing, by a terminal lifecycle frame, or with the child.

The stdin-order path for CLIs without lifecycle frames is removed: a doubted
send retired there disables content matching on CLIs that mint their own echo
ids, and no Orca failure called for it. Those CLIs keep the earlier behaviour.

Comments that said only a failed write or child exit ends a waiter, or that
idle comes only once the queue has drained, now say what ends one.
@brennanb2025

Copy link
Copy Markdown
Contributor Author

Status: ready for review (step 2 of 2, stacked on #23862)

Head a71b3ecdde6. CI is green: 19 checks passed, none failed or pending. Review this after #23862, which it builds on.

What it fixes: a Claude message that Claude started but never confirmed, then ended or failed, stayed "waiting" with the chat saying Working until the process exited. It now shows "Message delivery is unconfirmed." with Retry. Claude's discarded and refused reports for a message it never started now show "not delivered" and "not accepted". The idle cleanup no longer stops Claude while it is working on a message it has started. Claude CLIs older than 2.1.261 keep #23862's behaviour.

Review: two loops.

  • Loop 1 found one regression, in an extra path for older CLIs, and removed that path. It also changed the rule so that only a started message counts at idle.
  • Loop 2 found no code issues. It corrected comments and this description where they claimed more than the evidence showed.

Validation:

Not done:

  • No live Electron check. These endings need Claude to end or crash a turn at a precise moment, which can't be triggered from the app, so they are covered by replaying Claude 2.1.280's captured reports.
  • discarded and refused can't be produced from the app, so their tests rewrite captured frames.
  • Claude reports idle before it checks its queue again. That order is read from Claude 2.1.280's code; no capture shows it.

Known limits: listed in the description.

  • A Stop during Claude's pre-turn hooks shows "unconfirmed" briefly, then "withdrawn".
  • A message that is only ever queued still ends when the process exits.
  • A Claude process that has started a message but not yet confirmed it now stays running until Claude goes idle, exits, or is stopped.

Not merged. It's ready for a maintainer's decision.

@greptile-apps

greptile-apps Bot commented Sep 29, 2026 •

Copy link
Copy Markdown

RetriggerConfidence Score: 5/5

[Medium risk] Refactors how Claude handles unconfirmed messages and idle state.

The PR appears safe to merge; no new actionable issue was identified in its changeset.

Summary

This PR settles unanswered Claude messages from lifecycle and idle reports, and keeps a Claude child running while it holds an unconfirmed message.

  • Started messages that end without an echo become unconfirmed rather than withdrawn.
  • Queued or started messages prevent idle cleanup from stopping the child until the hold ends.
  • A late echo can still confirm a message previously marked unconfirmed.
Diagram
%%{init: {'theme': 'neutral'}}%%
flowchart LR
  A[Claude takes message] --> B{Echo received?}
  B -->|Yes| C[Delivered]
  B -->|No| D{Lifecycle end or idle after start?}
  D -->|Yes| E[Unconfirmed; retain identity for late echo]
  D -->|No| F[Keep child running while message is held]
  E -->|Late echo| C
Loading

Reviews (8) · Last reviewed commit: "Merge origin/main into brennanb2025/clau..."

Comment thread src/main/claude/claude-command-lifecycle.ts

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes

  • Terminal lifecycle frames now settle unstarted sends — claude-command-lifecycle.ts reads cancelled/discarded/refused; before started each rejects with its matching fact, and after started any of them releases the send as doubt rather than withdrawing it.
  • Idle releases a started, unechoed send — resolveClaudeReplayTurn calls releaseClaudeDispatchesUnansweredAtIdle on session_state_changed idle, retiring only waiters still marked started (a still-queued one is left, per 2.1.280 idling before re-reading its queue).
  • The idle sweep holds a Claude child that has taken a send — holdsDispatch is derived per check from the in-memory pairing records and plumbed adapter → router → conversation-lifetime → StructuredAgentSessionIdleSweep.owesWork.
  • New doubt settlement outcome — settleStructuredAgentSessionLateDispatch writes {state:'unknown', reason} as recovered: true, so the send is retired without any automatic re-send.
  • Tests — capture-replay coverage for each new ending, a production-runtime test, and an idle-sweep hold test.

I read the full diff and traced the hold/release paths. The hold is derived and bounded: every started waiter leaves dispatchWaiters via an echo, a terminal frame, idle, a failed write, or child exit, so it cannot latch. The only place one send is settled twice (unknown then rejected) is the pre-existing Stop path that ends the started-during-hooks case, which the PR already documents. I ran the three changed test files (claude-command-lifecycle.test.ts, structured-agent-session-idle-sweep.test.ts, claude-structured-idle-releases-unknown.test.ts): 44 tests pass.

One trade worth keeping in mind, already stated in the PR body: the hold has no timer, so a CLI that starts a command and never emits idle nor exits keeps its child until Stop or tab close. Nothing in the captured frames or the 2.1.280 code shows that state reachable, so it reads as intentional.

Pullfrog  | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

@brennanb2025
brennanb2025 changed the base branch from brennanb2025/claude-lifecycle-settles-pending to main September 29, 2026 20:30
@coderabbitai

coderabbitai Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: fa273b2c-98e1-47a2-aefd-98e5db858368

📥 Commits

Reviewing files that changed from the base of the PR and between 5471ca4 and 8dd0f81.

📒 Files selected for processing (3)
  • src/main/native-chat/agent-session-wire/structured-agent-session-adapter.ts
  • src/main/native-chat/agent-session-wire/structured-agent-session-host-mutations.ts
  • src/main/native-chat/agent-session-wire/structured-agent-session-rest-test-rig.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.


📝 Walkthrough

Walkthrough

Claude dispatch settlement now distinguishes sends that started from sends that did not. Started sends can be released in doubt when the CLI ends a command or reports idle; a later echo can still accept a send. Unknown outcomes are recorded as recovered submissions. Adapter hold state also prevents the idle sweep from stopping a session while a dispatch remains unanswered.

Priority: ➖ Normal

Merge Risk: ⚪ Minimal · up to 8dd0f

The change ends unanswered started Claude messages without automatically resending them and protects provider-held work from idle shutdown. No concrete merge-blocking risk is established; merge after normal checks pass.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 8dd0f

The changes conservatively distinguish messages that may have run from messages that were never delivered, and prevent idle shutdown while work remains. No new external access or permissions were identified. Deployment exposure and some recovery guarantees remain incompletely verified.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The demonstrated exposure is the lifetime and dispatch journal of an already owned session. The hold query does not itself create a process, submit work, or grant tool permissions; broader tenant, asset, and environment exposure cannot be established from the supplied deployment context.

Trust Boundaries and Controls

  • observed — Provider lifecycle reports affect waiters correlated within the owning session. Late outcomes cross into host-owned journal mutation through a callback that supplies the request session ID, followed by session lookup and conversation-fence recording. Existing cancellation checks also compare session identity, runtime fence, acquisition generation, and current dispatch admission.

Resilience and Maintainability Implications

  • observed — Potentially executed messages remain uncertain rather than becoming eligible for automatic resend through these settlement paths. Preserving that distinction contains duplicate-execution risk, while bounded retired correlation state permits late acceptance without reopening a turn.
🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 43.48% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 23 functions across 22 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check ⚠️ Warning The description is detailed and covers the change, user impact, rationale, testing, visual proof, limitations, and review notes. However, the required Linked Issue section explicitly states that no is… Add an actual linked issue in the Linked Issue section, using the required issue format such as Fixes #<issue-number>. Then update the description before merging.
✅ Passed checks (3 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly summarizes the two main changes: resolving unconfirmed Claude messages and keeping Claude running while it holds a message.
Full details: Description check

Explanation

The description is detailed and covers the change, user impact, rationale, testing, visual proof, limitations, and review notes. However, the required Linked Issue section explicitly states that no issue exists, despite the template requiring an actual issue link.

✨ Finishing Touches 💡 2
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
🛠️ Fix failing CI checks 💡
  • Commit to this branch
  • Create a new PR

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes

  • Merged origin/main into the branch — the only commit added since the prior review; it brings in step 1 (#23862, now merged) and unrelated main work.
  • Reconciled three PR files to origin/main — claude-structured-session-adapter.ts adopts main's compaction-to-dispatchClaudeCommand refactor, claude-structured-session-state.ts re-points the journal-translator import to its contract module, and claude-structured-dispatch.ts takes main's optional sentUuid on dispatchClaudeTurn.
  • No PR-authored behavior changed — every other touched file is byte-identical to the previously reviewed head a71b3ec; the holdsDispatch plumbing and lifecycle release logic survived the merge verbatim.

I confirmed the delta by diffing a71b3ec..72b6481 across the 20 PR files, then ran pnpm tc:node and six affected test files (76 tests: claude-command-lifecycle, structured-agent-session-idle-sweep, claude-structured-idle-releases-unknown, structured-agent-session-conversation-close, claude-structured-control-actions, structured-agent-session-command-readers) — all pass. The hold/release semantics are unchanged from the prior review, so the earlier analysis still stands.

Pullfrog  | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes

  • Merged origin/main into the branch — the only commit added since the prior review (e97c0ed), bringing in main CI fix #23923. Every file it touches is main-side (the constants.ts split, onboarding defaults, native-chat turn-fold/transcript slots, i18n); no PR-authored file changed.

git diff 72b6481..e97c0ed -- <PR source paths> is empty, so the lifecycle, hold, and idle-release logic already reviewed at a71b3ec/72b6481 survives the merge verbatim. I re-ran the three key test files on the merged tree (claude-command-lifecycle, structured-agent-session-idle-sweep, claude-structured-idle-releases-unknown): 44 tests pass, and the head SHA has no failing check-runs. The prior review's conclusions still stand.

Pullfrog  | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

@brennanb2025

Copy link
Copy Markdown
Contributor Author

Status update: #23862 merged, so this PR now targets main with main merged in (head e97c0edfc1f), CI green (22 passed). Its diff against main is the same set of files as before. Not merged.

…only started

The sweep rested a child whose CLI had queued a follow-up behind a turn, dropping
the send it had already taken. The hold now spans the CLI reporting it took the
send until its echo, a terminal lifecycle state, or the child's exit. The idle
release still covers only started sends: 2.1.280 can report idle before it
re-reads its queue.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes

  • The idle-sweep hold now spans a queued send, not only a started one — claudeHoldsDispatch (claude-command-lifecycle.ts:70) returns true for any waiter carrying a non-terminal commandLifecycle (queued or started), so a follow-up the CLI queued behind a running turn keeps its child off the idle sweep until it is echoed, withdrawn, or started and then released. The idle release itself stays started-only, matching 2.1.280 idling before it re-reads its queue.
  • Tests for the widened hold — two capture-replay cases (queued until withdrawn, queued until start-and-echo), the auth-failed hold expectation flipped to true, a production-runtime case where a queued send survives idle as still-pending, and an idle-sweep test that a provider-held send blocks the stop and grants a fresh window once released.
  • Docs — the holdsDispatch JSDoc and the adapter/waiter comments now say "took a send" rather than "started a send".

The delta is one commit (6f2c729c1a). I verified it is load-bearing: reverting claudeHoldsDispatch to === 'started' fails 4 tests across the two touched suites, and the change passes all 47 tests in the three affected suites. pnpm tc:node passes. Terminal-frame handling, the started-only idle release, and the wire shape are untouched by this delta.

Pullfrog  | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes

  • Extracted the provider-proven late-dispatch settlement into its own module — settleStructuredAgentSessionLateDispatch moved verbatim from structured-agent-session-host-mutations.ts to a new structured-agent-session-late-dispatch.ts; structured-agent-session-host.ts imports it from there, and the now-unused imports were dropped. Signature and behavior are unchanged.
  • Merged origin/main — brings in main's mid-turn queued-messages work (#23726) and the one-journal-database-per-host refactor (#23613), reconciling the PR's host wiring (structured-agent-session-host{,-mutations}.ts, structured-agent-session-conversation-lifetime.ts, structured-agent-session-runtime.ts, structured-agent-session-idle-sweep.ts) against them. No PR-authored lifecycle, hold, or idle-release logic changed in the merge.

The delta is one PR-authored refactor commit plus the merge. I confirmed the moved function has a single import site and no duplicate export, ran pnpm tc:node, and ran the affected suites (claude-command-lifecycle, structured-agent-session-idle-sweep, claude-structured-idle-releases-unknown, structured-agent-session-conversation-close, claude-api-retry-idle-sweep): 59 tests pass. The hold/release semantics reviewed at a71b3ec/6f2c729 are untouched.

Pullfrog  | Fix it ➔ | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes

  • Pinned a Stop that interrupts an already-started steer as doubt, not withdrawal — a new capture-replay case in claude-command-lifecycle.test.ts replays the batch-lead fixture's client-C steer (queued while the lead's turn runs, then started as its own turn), omits every frame after the steer's started, drives cancelTurn with an interrupt that leaves nothing queued, then injects the CLI's cancelled and idle frames. It asserts client-C settles as unknown / provider_ended_before_acknowledgement — never withdrawn — and that holdsDispatch goes true then false.

The delta since the prior review (ff9d0a45ec) is one test-only commit (5471ca443b, +46 lines, one file); no production code changed. I confirmed the test is load-bearing: disabling the waiter.commandLifecycle === 'started' doubt branch in claude-command-lifecycle.ts:52 fails this test along with the five sibling started-terminal cases, and the full suite (31 tests) passes on the restored tree. The prior review's hold/release conclusions still stand.

Pullfrog  | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

@brennanb2025

brennanb2025 commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor Author

Status: live QA done, ready for review. Head 8dd0f818ace (main merged in).

Live QA ran on a second Mac with the real Claude CLI 2.1.285, current main against this PR, with the same QA-only idle window and output filter on both arms (details in Visual Proof):

  • Claude silent for 45 s while working on a message: on main the process ended during the silence and the message ended unconfirmed with no answer. On this PR, Claude kept running and the answer landed at 45 s.
  • Stop on a message Claude started but never repeated back: on main the chat still said Working 98 s after Claude reported it cancelled. On this PR, the message reads "Message delivery is unconfirmed." with Retry, and Working clears.
  • Re-run on the merge head 8dd0f818ace: the Stop case was run again because merging main brought in fix(claude): a Stop naming the running turn acts at once, and a Stop in the gap is no longer lost #23861's Stop changes. Same result: Claude reported the message cancelled 40 ms after the Stop, and 251 ms after the Stop the message read unconfirmed with Retry and Working had cleared. The Visual Proof image is from this run.
  • A steer during tool calls still folds into the running turn on this PR.

Not run live: discarded, refused, and a turn that fails without an end report. The CLI can't be made to produce them; the tests cover them.

Also traced, with no bug found: drafts queued in Orca while Claude is busy are never counted as Claude's pending messages, so this PR's hold and release never touch them. A Stop with the queue paused still settles every message.

@brennanb2025
brennanb2025 merged commit 007b7c0 into main Sep 30, 2026
35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant