[brennanb2025]

	Files	Added	Deleted	Net
Test	12	$\color{#1a7f37}{\Huge{\mathbf{+}}}$​1085	$\color{#cf222e}{\Huge{\mathbf{−}}}$​63	$\color{#1a7f37}{\Huge{\mathbf{+}}}$​1022
Prod	23	$\color{#1a7f37}{\Huge{\mathbf{+}}}$​430	$\color{#cf222e}{\Huge{\mathbf{−}}}$​95	$\color{#1a7f37}{\Huge{\mathbf{+}}}$​335

Scope: which chat this affects

• Structured chat only. This PR changes the experimental structured native chat, which runs the agent through its SDK. It is behind the experimentalStructuredNativeChat setting, which is off by default (src/shared/default-global-settings.ts:137).
• Default chat view: no change. The default chat view, backed by a terminal and reading the agent's transcript, does not use this code.
• Terminals: no change. Plain terminals and terminal-backed chats behave exactly as before.

ELI5

Orca kept treating an agent as the chat's agent after it already knew that agent could no longer take messages. It did this because it waited to prove the agent's process had exited before letting go. When that proof never came, the chat was stuck: every message went to a dead connection. Now Orca lets go of the agent as soon as it can't take messages. The only thing still waiting on the proof is Orca's record of which process owns the chat, and Orca settles that record right away by checking the recorded process and stopping it if it is still running.

What a user sees:

• A stop Orca can't confirm no longer leaves the chat refusing every message until the app restarts. The next message starts a fresh agent.
• Retry right after an agent dies while starting works the first time. Before, the first Retry failed again with the same error and only a second Retry worked.
• A question or approval card closes when the agent behind it is gone, including after a failed re-attach.
• A message the old agent was handed but never answered shows as unconfirmed once a new agent starts, instead of staying "pending" until a restart.
• A leftover agent process is stopped right away, not when the user next opens or messages the chat.

Terms used below

• Ownership record (lease): Orca's durable note of which process owns a chat. It exists so a second agent is never started over one that might still be running, including on an SSH host where losing contact does not mean the process died.
• Recovery: the existing routine that settles an ownership record whose owner may still be running. It checks the recorded process by its identity (pid plus start time), stops it on the same machine if it is alive, and then releases the record. It never signals a process on another host.
• Shutdown sequence: the agent adapter's steps to close an agent (signal, wait, confirm exit). Its result is either "exit proven" or "exit not proven".
• Wind-down: Orca's steps after it stops an agent: stop, flush the last events into the chat's history, close out the dead agent's unfinished work, release the ownership record.

What Changed

Before

• When a stop could not prove the agent's process exited, the whole wind-down was abandoned. Orca kept the half-closed agent as the chat's agent and kept the ownership record marked live, so every later message was handed to a connection that could not take it. Nothing retried the stop for an idle chat, so the chat refused every message until the app restarted. This covered idle release, a user's Stop of a starting agent, and quit.
• When an agent died while starting, the chat showed the red "stopped before it finished starting" row at once, but Orca kept the dead agent as the chat's agent until the adapter's shutdown sequence finished. A Retry pressed in that window went to the dead agent and failed with the same words.
• The recovery after a failed history write did nothing at all when it could not prove the agent's exit.
• Ending an agent after a failed re-attach skipped the cleanup every other end runs, so a question or approval card stayed open with no agent behind it.
• If cleanup after a stop failed to write (for example, a full disk), the rest of the wind-down was abandoned, including releasing the ownership record, so the chat again refused every message.
• A record left waiting for recovery was only settled at the chat's next start or at app start, and a message the old agent had been handed could stay "pending" until a restart.

After

• An agent that can't take messages ends at once. A stop reports whether the exit was proven, and either way the agent is no longer the chat's agent. The proof decides only the ownership record: proven, and it is released; not proven, and it is handed to recovery.
• Recovery runs in the same step. When Orca hands the record for its own agent to recovery (a stop it can't confirm, a failed history write, or a failed attach), it runs the existing recovery routine right then: it checks the recorded process, stops it if it is alive and verified, and releases the record. A new message never waits on it, and no send or relaunch is needed to settle it.
• A start that dies is ended when Orca sees it die, not when the adapter's shutdown finishes, so a Retry in that window starts a fresh agent.
• One cleanup for every end. Every way an agent ends interrupts its running turn, cancels its open questions and approvals, and marks messages it was handed as unconfirmed. The failed re-attach and the failed history write now use it too.
• Cleanup never blocks the chat. After a stop, a failure to flush events or close out the dead agent's work is reported, and the ownership record is still released or handed to recovery. The next start redoes that cleanup from the chat's history: running turns and open cards are closed, and messages handed to the old agent are marked unconfirmed. If that cleanup can't be written and no new agent comes, the idle check (every 5 minutes) re-reads the chat's history and marks a message the old agent was handed as unconfirmed, so the chat can close and settle the rest when it is next opened, without the user sending again. Nothing extra is stored.
• Open chats learn the ownership change (a counter Orca bumps each time a chat's owning process changes) when Orca itself ends a start that died, just as they do when the agent's own exit event gets there first.
• Deliberate widening: recovery now treats a process check that throws like a check that could not verify the process. The record is released without signalling anything, and the error is reported. This applies everywhere recovery runs, including at chat start, where a check that kept throwing used to fail every message.
• With fix(native-chat): the conversation outlives its agent #22835's idle check. The idle check stops an agent that sat unused, and a stop there that can't be confirmed now ends the agent and settles its record in the same pass (as for any other stop), instead of being retried on the next pass. A stop that actually fails is still retried on the next pass.
• A start failure's reason is stored as the agent's own message, capped to the record's size limit. An overlong reason used to make the record unreadable, which silently undid its release.

Why

• The fact that matters is "can this agent take messages", and Orca already knew it. A stop that could not prove the exit still closed the connection, and a start seen to die had already dropped its session. Orca was the only part still treating them as live. Ending the agent where that fact becomes known removes the failure: no later path can hand a message to it.
• The ownership record needs the proof; the chat does not. The record stops a second agent from starting over a process that may still run, so an unproven exit keeps the record with its owner's identity until recovery has checked that exact process. The chat itself never waits on it.
• A record handed to recovery with nothing settling it is a latch. Orca used to settle it when the chat was opened or viewed, and that path is gone (fix(native-chat): the conversation outlives its agent #22835 no longer starts an agent just because a chat is viewed). Running recovery where the record is written means nothing is left owed to a future message.
• Cleanup is bookkeeping, and bookkeeping must not gate the user. Every other way an agent ends already released the record even when its cleanup could not be written. Stop was the one exception, and the next start can rebuild everything that cleanup would have written.
• Nothing new is stored. "Handed to recovery, owner kept" is an existing record state with an existing resolver, and a failed agent start already writes it.

Alternatives considered

• Let the start-failure check see the dying agent without ending it. That only fails the Retry again: Orca still has the dead agent, so nothing starts.
• Retry the adapter's shutdown on a timer. That adds a scheduler and still gates the user's message on a process that may never exit.
• Settle the record when the chat is opened, as before. fix(native-chat): the conversation outlives its agent #22835 removed that, and it would leave an orphaned process running until someone opens the chat.

Differences from the common pattern

• Matches: a stop that fails is reported and never blocks the next start, and a stopped process is no longer the chat's agent whatever its shutdown proved. The kill escalates within a bound and then finishes regardless.
• Deviation: an unproven exit is not forgotten. The common pattern logs a failed stop and moves on. Orca keeps the owner's identity until recovery has checked that exact process, and never signals a process on another host, because over SSH losing contact is not proof of death.
• Deviation: a Retry after a start death waits for the old agent's shutdown. The stop runs in the chat's ordered queue, so the Retry is accepted once the dead agent's shutdown and recovery finish. In live QA that was about 2 seconds.

Known weaknesses

• In a rare case a Retry after a failed start waits about 7 seconds instead of about 2. When the dying agent leaves a child process that ignores SIGTERM and started in the same second Orca took its process snapshot, Claude's adapter can't prove the exit on its first pass and runs its whole shutdown sequence a second time. The Retry still works the first time, and the chat says Claude is still starting meanwhile. Removing the second pass is the next small follow-up (F8).
• A Retry right after a start death waits for the dead agent's shutdown plus recovery. Each stop step is capped at 10 seconds; recovery's process check is bounded by its existing limits (4 checks 250 ms apart per signal, each check capped at 5 seconds). Typical cost is a single check.
• A stop step that throws or times out still keeps the agent (for example, Claude proves the connection closed and then fails to save its resume point). Follow-up F1.
• A step that times out keeps running outside the queue. Its late write is refused by the owner-generation check, so it cannot overwrite a newer owner, but a late kill signal can still reach the recorded pid for up to about 2 seconds. That only matters if the pid is reused in that window.
• A failed attach's recovery has no overall deadline of its own. It is bounded by the process check's limits and is normally under 250 ms.
• If the ownership record itself cannot be written, it stays waiting for recovery until the next start or app start settles it.
• A message handed to an agent whose cleanup failed is marked "provider exited" at the next start even if that agent was stopped rather than exited, because the next start cannot tell which. It still shows as unconfirmed with Retry.

Linked Issue

Part of the native-chat "a chat never gets stuck" series.

Visual Proof

Live desktop app on a separate Mac: an isolated dev instance with a hidden window, driven over the Chrome DevTools Protocol, running a structured Claude chat with real Claude Code. Before = this branch with its production changes removed. After = 5f972a4d43; the later head only merges main. Each run started from a fresh profile.

QA-only levers, identical in both builds, each off unless a flag file exists:

• A claude wrapper that makes a send-started Claude exit while starting and leave behind a child process that ignores SIGTERM, so the shutdown sequence stays open.
• For the stop Orca can't confirm: a wrapper whose root ignores SIGTERM, plus a patch that skips the shutdown sequence's SIGKILL for that one pid, so the real close path answers "exit not proven".
• A patch that fails the attach's final write once, to force a failed re-attach.
• The idle release shortened to 45 s.

	Before	After
Retry right after the agent dies while starting. Before, the first Retry is rejected with the same words and a second one is needed. After, the first Retry starts a new Claude and is answered; while it starts, the chat says Claude is still starting.
A stop Orca can't confirm (idle release). Before, the old process stays alive and every message is refused. After, about 3 seconds after the idle stop, with no message sent and nobody returning to the chat, recovery sent SIGTERM (ignored), then SIGKILL; the process was gone and its record released. The next message was answered.
An open question, then a failed re-attach. Before, the card stays open with no agent behind it. After, it closes as Cancelled.

Normal behaviour is unchanged (after): a Stop mid-turn ends the turn cleanly and the same Claude answers the next message; after a normal idle release the next message starts a new Claude and is answered. Neither shows an "unconfirmed" marker on earlier messages.

Stop mid-turn	Message after idle release

Testing

• I manually tested these changes locally
• Automated tests added/updated, or explained why not below

New tests run against a real host, record store and history journal, with a fake agent adapter and an injected process check (structured-agent-session-unproven-stop.test.ts, plus cases in the recovery, eviction and Claude integration tests):

• Idle release and a user's Stop whose exit Orca can't prove: the agent ends, recovery stops the recorded process in the same step, the record is released, and nothing was sent or relaunched. The next message then starts a fresh agent.
• A failed history write whose shutdown can't be proven: the agent ends, its open question is cancelled, and recovery settles the record.
• A failed attach (before or after the agent started) that can't prove the agent gone: recovery settles the record inside that same attach.
• An agent that dies while starting: it ends at once, a Retry starts a fresh agent, only the first message carries the start's error, and an open chat hears the new owner generation. The same with an unproven shutdown settles through recovery.
• The real Claude adapter over the RPC layer: a Retry sent while the dead agent's shutdown is held open starts a new Claude and is accepted.
• Process checks that can't answer or that throw: the record is released without signalling anything, the error is reported, and the next message goes through.
• Cleanup that fails after a stop: the failure is reported and the record still moves. At the next start, a message handed to the old agent is unconfirmed, its turn is closed and its card is cancelled. A reconnect to a still-running agent keeps its own pending message.
• A long start failure: its reason is capped, and the record still reads as released from a freshly opened store.

Each production change was removed in turn and its tests went red for that reason: the stop-and-recover step (12 fail), the failed-attach recovery (3 fail), the throwing-check mapping (4 fail), the loop's stop on a seen start death, the Claude exit-map read, the settlement on each end, the best-effort cleanup, the new-owner "unconfirmed" mark and its reconnect guard, the fence notice, and the reason cap.

Also green: the native-chat host suites, the history journal suites, the Claude and Codex structured-session suites, the cross-version send cases, tsc (node and web), oxlint, oxfmt and the changed-code quality gate.

Notes

• Mixed versions: host-only. No wire change and no new stored shape: "handed to recovery, owner kept" is a record state older builds already read and resolve, and the new fence notice reuses the existing snapshot message.
• SSH and folder workspaces: the stop, the process check and recovery all run on the host that owns the agent. Recovery never signals a process on another host, and an unproven exit is never recorded as a death, so losing contact is not read as the process dying. Folder workspaces take the same path.
• What callers that need proof now see: a worker release or worktree removal reads a chat whose exit wasn't proven as unverifiable rather than live, and only until recovery settles the record, which now happens in the same step.

Follow-ups

• F1: a stop step that throws or times out should end the agent as unproven. Not in this PR because it changes policy, not just code. Ending the agent inside a throwing stop step would skip the adapter's own close on the retry, which Claude relies on to save its resume point, and a timed-out stop keeps running, so its late callback would end the agent outside the chat's ordered queue. The stop's timeout also states the opposite policy today (a stuck app-server is kept rather than killed). Doing it properly means deciding "is there an agent to stop" from the owed wind-down instead of the agent.
• F3: the orchestration caller's wording for a record waiting on recovery ("changing owners on this host. Retry when that finishes.") now only shows when recovery itself failed to write, and could say that the next message revives the chat.
• F4: derive each end's cleanup from what ended the agent (cause, during start-up, reason), so no end can skip or mis-shape it.
• F5: let a message complete an owed wind-down, so a record release that could not be written does not wait for a Stop, close or quit.
• F6: a proven history-write-failure stop with no acquisition generation leaves the dead agent indexed (pre-existing edge case).
• F7: recovery's comment about releasing an owner it cannot verify says the owner's connection died with the runtime; that no longer describes a record handed to recovery by a stop in a running host.
• F8 (next PR): Claude's adapter should not run its shutdown sequence a second time once the first pass already saw the agent's root process exit, since the outcome is the same either way.

Review

Author: @BrennanKB5

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.

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)

[pullfrog]

✅ No new issues found.

Reviewed changes

Incremental re-review of the two commits since the prior pullfrog review (e0c3f03d25), both on top of the base-sync merge 5ab8deaae0.

• A delivery-observed start death now tells readers the fence its release moved the lease to. The stop's wind-down publishes the new fence when its cause is an observed exit, closing the case where the stop released the lease before the provider's own exit event was handled — the exit event then found no child and published nothing, so an open reader kept the pre-release fence. Whichever path releases first publishes; the other sees session.child === null and returns early, so the fence is announced exactly once.
• The lease release now reports whether it happened. releaseStoredStructuredAgentSessionOwner returns true only on the proven-release branch; the childless, stale/unreleasable, and recovering branches return false, so no fence is published for a transition that kept it.
• Refactor, no behavior change. The inline publishFence lambda passed to event recovery became a private class field reused by the lifetime context; class-field initialization runs before the constructor body, so it is defined when the recovery constructor reads it.
• Test teardown drains delivery starts. abandonHost in structured-agent-session-refusal-retry.test.ts awaits tracked attaches before closing, matching quit, so the store directory is not removed under an in-flight delivery start.

Verified locally: both touched test files green (15 tests, including the new fence-publication case) and pnpm tc:node clean. The new test fails without the fix — the reader would observe only the pre-release fence. The prior informational note about the limited "at app start" sweep remains open and non-blocking; no prior inline threads are open.

  ^{｜ View workflow run ｜ Using DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏}

[coderabbitai]

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: 1349c891-5415-44a5-b83f-ac4a7e052128

📥 Commits

Reviewing files that changed from the base of the PR and between fcbc590 and d683bd1.

📒 Files selected for processing (15)

• src/main/native-chat/agent-session-wire/structured-agent-session-conversation-close.test.ts
• src/main/native-chat/agent-session-wire/structured-agent-session-conversation-lifetime.ts
• src/main/native-chat/agent-session-wire/structured-agent-session-eviction.ts
• src/main/native-chat/agent-session-wire/structured-agent-session-host-lifetime.ts
• src/main/native-chat/agent-session-wire/structured-agent-session-idle-sweep.test.ts
• src/main/native-chat/agent-session-wire/structured-agent-session-idle-sweep.ts
• src/main/native-chat/agent-session-wire/structured-agent-session-unproven-stop.test.ts
• src/main/runtime/rpc/methods/orchestration-structured-worker-lifecycle.test.ts
• src/main/runtime/rpc/methods/orchestration-structured-worker-lifecycle.ts
• src/main/runtime/rpc/methods/structured-chat-tab-table.test.ts
• src/main/runtime/structured-agent-session-close.ts
• src/main/runtime/structured-session-worktree-teardown.test.ts
• src/main/runtime/structured-session-worktree-teardown.ts
• src/main/runtime/structured-worker-at-rest.test.ts
• src/main/runtime/structured-worker-authority.ts

🚧 Files skipped from review as they are similar to previous changes (1)

• src/main/native-chat/agent-session-wire/structured-agent-session-eviction.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 0 remain after this review.

---

📝 Walkthrough

Walkthrough

Startup waits now return structured failure details, and the delivery loop can stop a child that remains in the starting state. Attachment and shutdown paths settle pending work and distinguish confirmed child exits from unproven stops. Unproven stops hand ownership to recovery, which handles indeterminate probes and reports probe errors. Eviction continues through selected best-effort failures, while failures in other teardown steps still stop the sequence.

Priority: ➖ Normal

Merge Risk: 🟡 Moderate · up to d683b

A journal write failure can prevent Retry from starting a fresh agent, and a failed attachment can leave its operation unsettled. Address these failure paths before merging.

Security Architecture Review

Security architecture risk: 🟡 Moderate · up to d683b

Recovery is better defined for an agent that cannot accept messages, but two error paths can still interrupt cleanup and leave a chat unable to replace its agent promptly. The affected chat mode is experimental; no cross-user exposure was established.

Retained concerns

• Medium · reliability · inferred: A journal-write failure while recording a failed start bypasses the new delivery loop’s stop of the still-matching starting child. A later retry can encounter that same child rather than create a replacement, unless another cleanup path ends it.
• Medium · reliability · inferred: If the newly introduced post-acquisition child-cleanup callback rejects, attachment failure exits before settling its reserved operation. The enclosing attach cleanup does not independently perform that settlement, leaving recovery dependent on another path.

Security review details

Security Blast Radius

• inferred — The demonstrated failure modes concern the affected structured-chat session and its provider child or ownership record. The reviewed evidence does not establish cross-tenant reachability or a wider service privilege change.

Trust Boundaries and Controls

• observed — The new idle-sweep callback is injected by the internal conversation lifetime and runs after session lookup within per-session serialization. Export metadata alone does not establish an attacker-accessible request path.

Resilience and Maintainability Implications

• inferred — The exceptional paths matter to security containment because stopping a failed child and settling ownership should not depend solely on preceding journal or callback operations succeeding. There is no evidence here that either path grants an attacker additional authority.

Hardening Proposals

• proposed — Make failed-child wind-down and durable attachment-operation settlement independent of best-effort journal reporting and callback publication, while retaining fence and generation checks.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 40.68% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 59 functions across 45 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly identifies the native-chat fix: dropping an agent that cannot accept messages and settling an unproven exit. It is specific and related to the main change, although somewhat long.
Description check	✅ Passed	The description is largely complete. It includes the user impact, scope, before-and-after behavior, rationale, alternatives, visual proof, testing, known weaknesses, follow-ups, compatibility notes, a…
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.

• Fix all pre-merge checks with AI

✨ Finishing Touches 💡 2

📝 Generate docstrings 💡

• Commit to this branch
• Create a new PR

⚔️ Resolve merge conflicts 💡

• Resolve merge conflict in branch brennanb2025/chat-child-ends-when-unusable

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

Actionable comments posted: 2

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: bc793980-f71c-46f7-bea4-52f120a65d00

📥 Commits

Reviewing files that changed from the base of the PR and between bfe476f and f525b75.

📒 Files selected for processing (28)

• src/main/claude/claude-structured-session-adapter.ts
• src/main/claude/claude-structured-session-startup-state.ts
• src/main/native-chat/agent-session-wire/structured-agent-session-adapter-router.ts
• src/main/native-chat/agent-session-wire/structured-agent-session-adapter.ts
• src/main/native-chat/agent-session-wire/structured-agent-session-attach-failure.ts
• src/main/native-chat/agent-session-wire/structured-agent-session-attach-flow.ts
• src/main/native-chat/agent-session-wire/structured-agent-session-attach-orchestration.ts
• src/main/native-chat/agent-session-wire/structured-agent-session-attach-reconciliation.test.ts
• src/main/native-chat/agent-session-wire/structured-agent-session-attach.ts
• src/main/native-chat/agent-session-wire/structured-agent-session-dead-generation-settlement.ts
• src/main/native-chat/agent-session-wire/structured-agent-session-delivery-loop.ts
• src/main/native-chat/agent-session-wire/structured-agent-session-event-recovery.ts
• src/main/native-chat/agent-session-wire/structured-agent-session-eviction-deadline.ts
• src/main/native-chat/agent-session-wire/structured-agent-session-eviction.test.ts
• src/main/native-chat/agent-session-wire/structured-agent-session-eviction.ts
• src/main/native-chat/agent-session-wire/structured-agent-session-holds.test.ts
• src/main/native-chat/agent-session-wire/structured-agent-session-host-delivery.ts
• src/main/native-chat/agent-session-wire/structured-agent-session-host-lifetime.ts
• src/main/native-chat/agent-session-wire/structured-agent-session-host.ts
• src/main/native-chat/agent-session-wire/structured-agent-session-lease-release.ts
• src/main/native-chat/agent-session-wire/structured-agent-session-provider-child-record.test.ts
• src/main/native-chat/agent-session-wire/structured-agent-session-refusal-retry.test.ts
• src/main/native-chat/agent-session-wire/structured-agent-session-surface-lifetime.test.ts
• src/main/native-chat/agent-session-wire/structured-agent-session-unproven-stop.test.ts
• src/main/runtime/agent-session-surface-release-transition.ts
• src/main/runtime/claude-structured-fake-connection-test-fixture.ts
• src/main/runtime/claude-structured-session-integration.test.ts
• src/main/runtime/structured-agent-session-runtime-exit.test.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.

[coderabbitai]

🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Protect the durable settlement from a rejected onAcquisitionReleased callback.

The callback on Line 27 is now awaited, and it can return a Promise. If that Promise rejects, the function exits before settleFailedPostAcquisitionAttachment runs. The failed attachment's operation row and reservation then stay unsettled, and the caller receives the callback error in place of cleanupError. The async endReleasedChild path runs journal settlement and publishes status, so a rejection is possible. Catch the callback error, keep the settlement step, and include the callback error in the thrown error.

🛡️ Proposed fix

-  await input.onAcquisitionReleased?.(cause, { rootGone: exitProof !== 'unproven' })
+  try {
+    await input.onAcquisitionReleased?.(cause, { rootGone: exitProof !== 'unproven' })
+  } catch (releaseError) {
+    cleanupError = new AggregateError([cleanupError, releaseError], 'acquisition release callback failed')
+  }

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	await input.onAcquisitionReleased?.(cause, { rootGone: exitProof !== 'unproven' })
	try {
	await input.onAcquisitionReleased?.(cause, { rootGone: exitProof !== 'unproven' })
	} catch (releaseError) {
	cleanupError = new AggregateError([cleanupError, releaseError], 'acquisition release callback failed')
	}

[coderabbitai]

🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift

Stop the failed child even if failure recording rejects.

If this.fail() rejects while writing the journal, execution skips stopAgent. The outer catch attempts another journal write but does not end the child. A startup failure combined with a journal write failure can therefore leave the failed child indexed and prevent Retry from acquiring a new agent. Attempt the stop in a finally path for the still-matching child, and report the journal error separately.

[pullfrog]

✅ No new issues found.

Reviewed changes

Incremental re-review of 2dda86515e, the one commit since the prior pullfrog review (787c8ceb24); f525b750f5 in between was a base-sync merge. The change makes a lease the host hands to recovery conclude in the same serialized step rather than at the chat's next start.

• The stop's wind-down now runs recovery. STRUCTURED_AGENT_SESSION_EVICTION_STEPS gains a best-effort resolve-recovery step at the very end, after acknowledge-release so the adapter's route is already dropped. It is gated on a wind-down actually being owed. stopStructuredAgentSessionAgentUnderSerialize wires it to runtimeState.resolveRecovery and re-publishes the fence when the cause is an observed exit, so an unproven stop, a sink-failure end and a quit stop release the lease in-step instead of leaving it latched.
• A failed attach concludes its own unproven acquisition. runAttach reads recoveringBefore right after the initial resolveRecovery, then on a failed attempt runs recovery in its finally only when the lease was not already recovering. That distinguishes the attempt's own unproven acquisition from a lease a crash left behind, which the next start still owns; a recovery failure is reported and the attach keeps its own outcome.
• Tests cover the new triggers and the failure paths. The W40, sink-failure and W43 cases now assert the lease is released with no send and no new child; new cases cover the failed-attach trigger, an owner probe that cannot answer (released, nothing signalled), a probe that throws (reported, stop still finishes), and the fence an open reader sees when a concluding recovery moves the lease.

Verified locally: structured-agent-session-eviction.test.ts, structured-agent-session-holds.test.ts, structured-agent-session-unproven-stop.test.ts and structured-agent-session-recovery-exits.test.ts all pass (58 tests). The only prior inline thread (the sink-failure settlement) is resolved, and no pullfrog threads remain open. This also supersedes the earlier informational note about the recovering lease concluding at app start: it now concludes in-step for all three triggers, and only survives a recovery failure or a host crash.

  ^{｜ View workflow run ｜ Using DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏}

[pullfrog]

✅ No new issues found.

Reviewed changes

Incremental re-review of 5f972a4d43, the one commit since the prior pullfrog review (2dda86515e). It changes how recovery resolution reads an owner probe that throws.

• A throwing owner probe no longer strands the lease. New probeRecordOrIndeterminate wraps deps.probeRecord; the first throw within a resolution is reported through a new optional onProbeError (wired to onEventSinkError) and the probe reads as indeterminate. Recovery then takes the existing "identity cannot be verified" branch: it releases the lease without a signal rather than rejecting and leaving the lease latched recovering for a later retry.
• The stop ladder finishes through the same wrapper. A probe that starts throwing after a signal no longer aborts stopOwnerAndReprobe; it is read as indeterminate and the ladder proceeds through SIGTERM and SIGKILL.
• Tests pin the new reading. recovery-resolution adds a throwing-probe case (released, unsignalled, error reported once) and a mid-ladder throwing case; recovery-exits now forces the attach's latch by failing transitionHandoff instead of throwing the probe, and unproven-stop's two probe-failure cases assert release-without-signal and a plain next send.

This is consistent with releaseUnprovenAgentSessionOwner's documented contract ("a recorded owner whose identity cannot be verified"), and the indeterminate outcome already released this way — the commit makes a throw equivalent rather than a separate path. A conflicted terminal owner is still waited out, since an indeterminate probe is not proven dead. Verified locally: the three touched test files pass (34 tests).

  ^{｜ View workflow run ｜ Using DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏}

[brennanb2025]

Review status: ready for review

Head 023c2743c5 (this PR merged with current main). CI is green: 31 checks pass, none fail.

Review loops. Seven review loops ran, plus an independent architecture review after the same kind of issue came up twice. Each loop's fixes were pushed with a test that fails without them.

• Loop 1: the history-write-failure path now runs the same cleanup as every other way an agent ends; a start failure's reason is capped so the record stays readable; the reason is stored as the agent's own message.
• Loop 2: clean.
• Architecture review: cleanup failing after a stop could still leave the chat refusing messages, because Stop was the one end where cleanup blocked releasing the ownership record. Loop 3 fixed it. A new agent now marks messages handed to the old one as unconfirmed, so cleanup no longer has to succeed first, and cleanup after a stop no longer blocks the record.
• Loop 4: clean.
• Loop 5: fixed two CI failures. An open chat now learns the ownership change when Orca ends a start that died. A test teardown now waits for in-flight starts.
• Loops 6 and 7: an ownership record handed to recovery is now settled in the same step instead of waiting for the next message or an app restart. A process check that throws is treated like one that cannot verify the process.

Readiness checklist. Run at the start (two medium findings, both fixed above) and twice at the end. The final pass found no defects.

Live QA (desktop app on a separate Mac, real Claude Code, before and after builds):

• Retry right after an agent dies while starting works the first time.
• A stop Orca can't confirm no longer leaves the chat refusing messages. The leftover process is stopped about 3 seconds after the idle stop, with no message needed.
• An open question closes when a failed re-attach ends its agent.
• A normal Stop and idle release behave as before, with no "unconfirmed" markers.

Screenshots are in the PR body.

Not covered by live QA. Codex scenarios were not run live. Unit and integration tests cover the Codex paths. Live Codex QA was deferred until the rig can run without touching the machine's real Codex and Claude settings.

Known weaknesses and follow-ups are listed in the PR body. The next small PR (F8) removes the second shutdown pass that can make a Retry after a failed start wait about 7 seconds instead of about 2.

[coderabbitai]

🧹 Nitpick comments (1)

src/main/runtime/claude-structured-session-integration.test.ts (1)

417-418: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Keep the close gate shut until the Retry reaches the host.

send('retry') starts an asynchronous RPC, but openGate() runs before the test observes that the host received the Retry. The close operation can therefore complete first, allowing the test to pass without exercising Retry during blocked cleanup. Add a synchronization point that confirms the Retry reached the blocked path before opening the gate.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: 2a3b7eec-d91c-4801-b5ae-422165bb93b5

📥 Commits

Reviewing files that changed from the base of the PR and between 023c274 and fcbc590.

📒 Files selected for processing (18)

• src/main/native-chat/agent-session-wire/structured-agent-session-adapter.ts
• src/main/native-chat/agent-session-wire/structured-agent-session-attach-orchestration.ts
• src/main/native-chat/agent-session-wire/structured-agent-session-attach-reconciliation.test.ts
• src/main/native-chat/agent-session-wire/structured-agent-session-conversation-close.test.ts
• src/main/native-chat/agent-session-wire/structured-agent-session-dead-generation-settlement.ts
• src/main/native-chat/agent-session-wire/structured-agent-session-delivery-loop.ts
• src/main/native-chat/agent-session-wire/structured-agent-session-event-recovery.ts
• src/main/native-chat/agent-session-wire/structured-agent-session-eviction.test.ts
• src/main/native-chat/agent-session-wire/structured-agent-session-host-delivery.ts
• src/main/native-chat/agent-session-wire/structured-agent-session-host-lifetime.ts
• src/main/native-chat/agent-session-wire/structured-agent-session-host.ts
• src/main/native-chat/agent-session-wire/structured-agent-session-provider-child-record.test.ts
• src/main/native-chat/agent-session-wire/structured-agent-session-recovery-exits.test.ts
• src/main/native-chat/agent-session-wire/structured-agent-session-start-failure-writer.test.ts
• src/main/native-chat/agent-session-wire/structured-agent-session-surface-lifetime.test.ts
• src/main/native-chat/agent-session-wire/structured-agent-session-unproven-stop.test.ts
• src/main/runtime/claude-structured-session-integration.test.ts
• src/main/runtime/structured-agent-session-runtime-exit.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 4 remain after this review.

[pullfrog]

✅ No new issues found.

Reviewed changes

Incremental re-review of fcbc590d6c, the merge of origin/main into this branch. That merge landed PR 2 / #22835 ("the conversation outlives its agent"), which removed the holds and made the delivery loop the only starter, so the merge re-carried this PR's behaviour onto that refactor rather than being a mechanical base-sync. Reviewed the current PR diff against main (32 files) with the merge message as the map.

• The sink-failure unproven end now runs the stop's own wind-down. endUnprovenChild still endProviderChilds the force-closed child as host-stop / rootGone:false and records the owed wind-down, but instead of its own settle + release it delegates to stopStructuredAgentSessionAgentUnderSerialize through a new lifetime context. The release reads endedChildRootGone (false here), so the lease goes recovering and the wind-down's resolve-recovery step concludes it in the same step. No serialize re-entry: recovery takes only the store transaction and process.kill.
• The stop wind-down composes #22835's "the row belongs to the conversation" with this PR's pieces. Status is published right after the child ends, the fence is published on release only when released && cause === 'exit', and the new resolve-recovery step publishes it when concluding a resolved recovery for an observed exit.
• Deliveries keep their stop for a start seen to die. The host wires stopAgent alongside #22835's startedFor start; the loop still awaits fail(...) then stops the starting child that had no start failure value. awaitStarted now returns the provider's raw failure object and the loop words it once.
• The eviction deadline preserves best-effort. withStructuredAgentSessionEvictionDeadline spreads each step instead of copying only name, so drain-published, settle-dead-generation and resolve-recovery stay non-fatal through the wrapper. The assertion moved from the deleted holds test into the eviction test, and #22835's retry tests now fail their stop by throwing (an unproven stop is no longer a failure), with a new same-tick conclusion case.

Verified locally: pnpm tc:node is clean, and the full src/main/native-chat/agent-session-wire/ suite passes (114 files, 904 tests), along with the touched runtime tests (claude-structured-session-integration, structured-agent-session-runtime-exit). The single prior pullfrog inline thread is resolved. The two open threads on this head are CodeRabbit's, which are theirs to resolve and were not re-raised here.

  ^{｜ View workflow run ｜ Using DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏}

[pullfrog]

✅ No new issues found.

Reviewed changes

Incremental re-review of 9212fef938, the one commit since the prior pullfrog review (fcbc590d6c). It keeps a settlement that could not be written owed after the child ends, so an idle chat's next stop retries it instead of leaving a handed-over send pending until an attach that #22835 no longer triggers.

• A failed settlement stays owed across the stop. stopStructuredAgentSessionAgentUnderSerialize now tracks whether settleWork actually wrote, and releaseLease sets session.owesProviderChildWindDown = settled ? undefined : owed where it used to clear it unconditionally. The lease still moves — release and recovery run regardless of the settlement — so only the bookkeeping stays owed.
• The idle sweep's existing retry writes it. The gate at structured-agent-session-idle-sweep.ts:103 (owesProviderChildWindDown !== undefined && !session.child && !queuedOrDelivering) already re-runs stopAgent; the retry's release is a no-op against the already-released record and resolveRecovery reads not-applicable once the lease has left recovering, so the second pass repeats only the settlement, with no send and no new child.
• Test pins the no-send retry. The new case fails markPendingSubmissionsUnknown once, closes the chat, then ticks the sweep and asserts the submission reads unknown, the turn interrupted, the question cancelled, the handle still open, and host activity unchanged.

Verified locally: the touched file is green (20 tests). Reverting the one-line assignment reds the new test on dispatchState (pending instead of unknown), so it exercises the fix rather than a reopen. The two remaining open threads on this head are CodeRabbit's, which are theirs to resolve; no pullfrog threads are open.

  ^{｜ View workflow run ｜ Using DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏}

[pullfrog]

✅ No new issues found.

Reviewed changes

Incremental re-review of 97a7f00767, the one commit pushed after my prior review of 9212fef938. It replaces that commit's "keep the whole wind-down owed" approach with a narrower step in the idle sweep.

• The owed-wind-down revert. releaseLease goes back to clearing session.owesProviderChildWindDown unconditionally, and settleWork no longer records whether it wrote. A failed settlement therefore no longer keeps the handle pinned by the retry gate.
• The sweep marks a leftover send in doubt. New markLeftoverStructuredAgentSessionSendsUnknown no-ops when nothing is pending, otherwise marks the session's pending sends unknown (DISPATCH_DOUBT_PROVIDER_EXITED) and reports a write failure. tickUnderSerialize runs it for a childless session with nothing queued or delivering: a failure returns, a success lets the handle close, and the next open settles the turn and prompt through settleStaleStructuredAgentSessionState.
• Tests cover the sequence. The rewritten case fails the mark twice (the stop's settlement, then the first sweep) and advances the host clock each sweep: the send stays pending after sweep one, reads unknown after sweep two, and the handle closes on sweep three, with the turn no longer running and the question cancelled on reopen.

Verified locally: the three touched suites pass (44 tests) and pnpm tc:node is clean. Neutralizing the new helper reds the new test on dispatchState. Two things I checked and cleared: the sweep's mark uses the post-release conversation fence while the old settlement used owed.fence, but markJournalPendingSubmissionsUnknown filters on dispatchState only and merely stamps the passed fence on resolved rows, so no submission can be skipped; and the gate cannot race a just-accepted send because sendStructuredAgentSessionTurn calls wakeDelivery inside the same serialized step as the accept. Deferring the turn/prompt settlement to the next open is the intended design here.

  ^{｜ View workflow run ｜ Using DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏}

[pullfrog]

✅ No new issues found.

Reviewed changes

Incremental re-review of 5f6b8710aa, the one commit since my prior review of 97a7f00767. It is test-only: no production change.

• A case for the unexpected-exit trigger. structured-agent-session-unproven-stop.test.ts gains a case (:714) alongside the existing host-stop sibling (:678). It leaves a handed pending send, a running turn and a pending question, then drives an adapter ended (cause: 'unexpected-exit', reason 'killed') with the exit settlement's markPendingSubmissionsUnknown forced to reject once, so the settlement aborts before writing.
• The sweep lets the handle go, the reopen tells the exit. The send is asserted pending and the turn running after the failed settlement; sweep one marks the send unknown (handle stays open — the write resets the idle clock), sweep two closes the handle, and the reopen re-derives the turn as interrupted with the exit's own copy. acquire is unchanged, so no send or relaunch did it.
• The import only names the outcome helper the new assertion reads.

The prior finding (the sink-failure settlement) remains resolved, and no pullfrog threads are open. Verified locally: the file passes (21 tests), and neutralizing markLeftoverStructuredAgentSessionSendsUnknown reds the new case at :744, so it exercises the re-derive path rather than a reopen.

  ^{｜ View workflow run ｜ Using DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏}

[pullfrog]

Important

A close that reports stopped: false for a lease released without proof of exit still retires the chat tab, and the worktree sweep now refuses the removal on exactly that lease — so the workspace survives while the user's chat tab is dropped. Details inline.

Reviewed changes

Incremental re-review of the two commits since the prior pullfrog review (5f6b8710aa). Prior inline feedback (the sink-failure settlement) remains resolved; no pullfrog threads are open.

• stopped from a close now means a proven exit. closeStructuredAgentSessionChild returns stopped: false (with a reason) when the post-close observation is not exited, even though the lease is released and the close already retired the tab. structuredSessionCloseSettled keeps its broader "nothing left to act on" meaning for finishing the close.
• Worktree teardown refuses over a released-unproven lease. The sweep's re-read went from status === 'exited' || structuredSessionCloseSettled(...) to status === 'exited'; a lease recovery released without death evidence is unverifiable and now lands in unstopped, so an ordinary (non-force) removal refuses rather than counting the session closed.
• A worker release settles only on death evidence. stopStructuredWorkerForRelease already maps !stop.stopped to release_unknown; with the above, a released-unproven lease reports release_unknown instead of settling with no evidence.
• Tests pin the new verdicts. The release case asserts stopped: false while the tab retirement still runs, and settleWorkerTerminalRelease only on death evidence; the teardown case asserts the refusal; the close and at-rest cases assert not-stopped with the lease released/no evidence.

ℹ️ Nitpicks

• The PR body's "What callers that need proof now see" still says a worktree removal reads an unproven chat as unverifiable rather than live and settles in-step; with this commit an ordinary removal now refuses over that lease (force still proceeds). Worth correcting so the behavior change is visible in the writeup.

  ^{｜ Fix all ➔ ｜ Fix 👍s ➔ ｜ View workflow run ｜ Using DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏}

[pullfrog]

A close that reports stopped: false for a lease recovery released without proof of exit still retired the durable tab (the hide at line 77 was never put back, because the rollback at line 152 is keyed on structuredSessionCloseSettled, which is true for the same released lease) and retired the live snapshot tab at line 104. closeStructuredSessionsForWorktree now refuses an ordinary removal on exactly this lease, so the workspace survives while the user's chat tab is gone — the loss restoreTabOnUnprovenClose exists to undo. I confirmed the durable index is dropped (visible empty, stopped: false) with a throwaway test against this function.

Technical details

# A not-stopped close still drops the chat tab, and the worktree sweep refuses over the same lease it never un-dropped

## Affected sites
- `src/main/runtime/structured-agent-session-close.ts:104-112` — `retireSettledStructuredWorkerTab` runs before the new not-exited check, and the returned `stopped: false` no longer implies the caller will proceed.
- `src/main/runtime/structured-agent-session-close.ts:152` — `restorePersistedTabVisibility` skips the restore whenever `structuredSessionCloseSettled` is true, which is exactly the `unverifiable` + `released` lease this branch returns on.
- `src/main/runtime/structured-session-worktree-teardown.ts:277-293` — a `mayRefuse` removal passes `restoreTabOnUnprovenClose: true`, then refuses when the re-read status is not `exited`; nothing there restores the tab the close dropped.
- `src/main/runtime/structured-worker-authority.ts:134-141` — `structuredSessionCloseSettled` conflates "the close has nothing left to act on" with "safe to drop the tab".

## Required outcome
- When a removal refuses, the chat tab must survive it: do not retire the durable/live tab for a `stopped: false` close, or have the sweep restore the tab for a session it pushed to `unstopped`.
- Decide whether the orchestration release path (which reports `release_unknown` and stays retryable) should also keep the tab, or intentionally drop it.

## Suggested approach (optional)
- Move the `retireSettledStructuredWorkerTab` call behind the `status === 'exited'` check so a not-stopped close never retires, or make `closeStructuredSessionsForWorktree` restore the durable reference (and leave the snapshot tab) in its `unstopped` branch.

## Open questions for the human
- Is retiring the chat of a released-but-unproven process intended even when the removal refuses and the workspace stays?