Skip to content

fix(claude): open structured chat without a startup deadline, and make Retry start fresh - #22364

Merged
brennanb2025 merged 84 commits into
mainfrom
brennanb2025/claude-init-deadline
Sep 24, 2026
Merged

brennanb2025 merged 84 commits into
mainfrom
brennanb2025/claude-init-deadline

Conversation

@brennanb2025

@brennanb2025 brennanb2025 commented Sep 23, 2026 •

Copy link
Copy Markdown
Contributor
Files Added Deleted Net
Test 63 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​4510 $\color{#cf222e}{\Huge{\mathbf{−}}}$​349 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​4161
Prod 89 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​2960 $\color{#cf222e}{\Huge{\mathbf{−}}}$​1001 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​1959

ELI5

A structured Claude chat has two parts: the chat you see, and a Claude process running behind it that actually answers. This PR fixes three ways that pairing broke.

  1. Opening a chat could fail on a busy machine. Orca gave Claude 10 seconds to start. If it took longer, the chat said "Could not open Claude chat". Pressing Retry just replayed that stored failure instead of trying again.
  2. Once the Claude process was gone, the chat was stuck forever. Every message you sent came back "not ready yet". The app kept resending it, and nothing ever started Claude again.
  3. A chat you weren't looking at lost its Claude process after 15 seconds. Switching to another worktree and back, or sending to a chat in the background, usually paid for a full restart.

After this PR:

  • A chat opens as soon as Claude's process starts. Messages you type while it's starting are held and delivered in order once it's ready.
  • If Claude really can't start, the chat tells you why in Claude's own words, for example "Not signed in". Retry then genuinely tries again.
  • Sending a message to a chat whose Claude process is gone starts a new one first, then delivers the message. The two happen as one step.
  • A chat nobody is looking at keeps its Claude process for 30 quiet minutes instead of 15 seconds.

Why this PR is so big

It's 151 files, but most of that is tests, and the rest is spread thinly across a few layers.

It started as two stacked PRs. This PR removed the startup deadline. #22359 sat on top of it and made a send restart a missing Claude process. Both are now folded in here, because neither was safe to merge on its own:

Where the lines are:

Area Prod files Prod lines (+/−) Test files Test lines (+/−)
Session host (src/main/native-chat) 34 +1,493 / −546 25 +2,203 / −151
Claude adapter (src/main/claude) 16 +683 / −261 18 +571 / −87
Runtime wiring and operation ledger (src/main/runtime) 6 +200 / −88 11 +1,456 / −110
Desktop UI (src/renderer) 11 +111 / −16 10 +514 / −5
Shared wire types (src/shared) 7 +51 / −3 2 +50 / 0
Translations (5 locales plus English) 6 +38 / 0 — —
Codex adapter and observability (renames only) 3 +4 / −5 1 +3 / −4

About 4,800 of the 7,400 added lines are tests. The production change is about 2,900 lines added and 900 removed.

Why it touches so many files. "Is there a Claude process for this chat, and what state is it in?" is answered in several places: the Claude adapter, the session host, the operation ledger, the idle clock, the status feed and the desktop outbox. Making "starting" a real state, and making a send able to start a process, means each of those places has to agree on the new states. Most production files change by under 30 lines.

Why so many tests. Almost every bug here is a race: a send arriving while Claude is starting, a send arriving while it dies, a chat view opening while a send restarts it, or quitting mid-start. Each race gets a test that drives the real session host with a scripted Claude. Each test was also checked by deleting the code it covers and confirming it fails.

What Changed

1. Opening a Claude chat no longer races a clock

Before. Creating a chat spawned claude and then waited for its startup handshake: the reply to initialize, plus the model and settings reports. Past 10 seconds, the create failed. The failure was stored in the operation ledger under the request's id, so Retry, which reused that id, got the stored failure back in about 100 ms without starting anything.

After.

  • The chat is published when the process spawns. claude-structured-init-deadline.ts is deleted. Models, settings and saved options are read and applied when Claude reports them, in claude-structured-session-startup.ts, after the chat already exists.
  • Messages sent during startup are held, not lost. claude-structured-session-startup-gate.ts queues any message sent while Claude is starting. When startup lands, it writes them to Claude in the order they were sent. Holding them until then means the first turn never runs under defaults that the saved options were about to replace.
  • Held messages have a definite outcome if startup fails. Claude accepts no input before it initializes, so a held message was provably never written. If Claude exits first, the message is rejected with Claude's own reason, not left "unconfirmed". Stop withdraws held messages.
  • The chat says it's starting. The status feed carries a new optional field, hostExecutionPhase (starting or ready). While it's starting, the chat shows: "Claude is still starting. Messages wait until it is ready; close this chat to give up on it." Stop ends a start that hangs.
  • Restoring saved options can't fail the start. When a start restores saved options such as model or permission mode, the CLI may never answer a write. The SDK then gives up after its 30-second request timeout. Before, that timeout failed the whole start. Now that option is skipped: the running chat uses Claude's own value, and your saved choice is kept and tried again on the next start. Only an option Claude actively refuses, or one you change yourself, replaces the saved choice. This also holds across /clear and across later changes to a different option.

2. When Claude can't start, the chat says why, and Retry means retry

  • A failed create carries its cause. If the Claude process provably exited, including at spawn, the create is refused once with the CLI's own message and a new optional ownerVerdict: 'exited'. The desktop client treats exited as definite: it shows the reason next to Retry, and Retry sends a new create request with a new id. A chat whose first start wrote no transcript resumes as the same conversation instead of creating a second one.
  • A send into a dying start is rejected, not "unconfirmed". Before, a send that met a Claude process dying before startup read "Message delivery is unconfirmed", and pressing Retry made the Retry button disappear. Now it settles as "not sent". The red line under the composer names the cause, for example "The provider stopped before it finished starting: Not signed in.", and Retry stays available.
  • One failed attempt writes one row, whichever path noticed it. A failed start or restart always leaves the same status row in the transcript, whether the chat view, a send, or the host's own recovery hit it. A message that never ran no longer shows "Worked for 0s", which used to fold the explanation away behind a disclosure.

3. A send brings a missing Claude process back

Before. When Claude exited, the host released the chat's ownership lease. After that, every send was refused agent_session_ownership_unknown. Clients read that code as "not admitted yet", so they resent forever. Only opening the chat view could restart Claude, and that path ran once and silently ignored its own failure.

After. A send now makes sure its chat has a running Claude process, as part of the send itself. The rest of this section walks through what happens when a message arrives on the host.

  1. Sends to one chat take turns. The host already runs every change to a chat one at a time through a per-chat queue. The check below happens inside that queue, so nothing can slip in between "is there a process?" and "start one".
  2. The operation ledger answers first. If the host has already seen this exact message id, it replays the recorded answer and starts nothing. If the chat isn't loaded, the host loads its history read-only for the replay; it never starts Claude just to repeat an answer. Without this rule, a Claude that dies at startup would move the ownership fence. The client would resend the same message against the new fence, and each resend would start another Claude that dies the same way.
  3. The host restarts Claude only when the old one is provably gone. That means three things: the chat has no process, its lease was handed back cleanly and reconciled, and nothing about the chat would refuse the message anyway, such as an unfinished rewind or a cleared conversation. A lease the host can't verify, one being reserved, or one mid-handoff is never replaced. That send gets its normal retryable refusal instead.
  4. Everything that restarts Claude goes through one function. A chat view opening, a send, and the host's own recovery after an unexpected exit all call ensureProviderChild, inside the same per-chat queue. The first one starts Claude; the next finds it running and starts nothing. Before, these were separate paths that raced, and the loser was refused or silently stopped holding the chat.
  5. The message is delivered once, against the new process. Starting Claude moves the chat's ownership fence forward. The client sent its message against the old fence, and the restart was the only thing that moved it, so the host admits the message at the new fence. It doesn't bounce it back as stale. This matters most on mobile, which has no automatic resend.
  6. A failed restart gives a typed, final answer. The restart returns a result, not a thrown string. Each possible refusal code is classified in a table that fails to compile when a new code is added without a classification.
    • Transient, for example a lease that another operation is settling. The send carries on to the normal lease check and gets its usual retryable answer.
    • Failed, for example "Not signed in". The send is refused with the new code agent_session_owner_restart_failed, and a message naming the cause: "Claude couldn't restart: Not signed in." The same text is written into the chat once. The desktop outbox stops auto-retrying, shows the message, and offers Retry, which is a fresh attempt.
    • Unresumable, when the host has nothing to restart from. The message adds "Start a new chat to continue."

The chat view's hold still starts Claude when you open a chat, so the first message doesn't wait. The two paths share the one function above. The hold no longer swallows its own failure: it logs it.

4. A chat you aren't looking at keeps its Claude for 30 quiet minutes

Before. When the last view of a chat went away, its Claude process was stopped 15 seconds later.

After.

  • The window is 30 minutes. Every chat write restarts it: a send, streamed output, a turn ending.
  • Owed work defers the stop. A running turn, or a message sent but not yet taken by Claude, keeps the process past the window.
  • Views and quit behave as before. A chat you're looking at is never stopped. Quitting the app still stops every Claude process at once.

The 15-second grace existed so that closed chats wouldn't keep a process until quit. That still holds, just after 30 minutes. Startup still spawns nothing until you open a chat or send to one. The ownership lease is renewed about every 10 seconds for any live process, so a long idle window only means more renewals of a healthy lease.

Architecture: before and after

Five pieces of the design changed. For each one, this is how it worked on main, how it works now, and what that changes.

1. How a Claude chat starts

Before (main): one blocking sequence inside the create request.

create request ──► spawn claude ──► wait for initialize (10 s deadline) ──► read settings
                                                                             │
                   chat exists ◄── restore saved options ◄── publish chat ◄──┘

The chat didn't exist until every step had finished. If any step was slow, the create failed, and so did a saved-option write the CLI never answered. That failure was stored against the request id, so Retry got the stored failure back. A slow start and a broken start looked the same.

After: spawn, publish, then learn.

create request ──► spawn claude ──► publish chat (phase: starting) ──► create returns
                                          │
          Claude reports initialize, models, settings ──► apply them, restore saved options
                                          │                (a skipped write keeps the saved choice)
                   messages sent meanwhile are held ──► written in order ──► phase: ready

The create only has to prove that a process exists. Everything Claude reports later is applied to a chat that already exists (claude-structured-session-startup.ts). Anything sent before that is held by the adapter's startup gate (claude-structured-session-startup-gate.ts). A startup failure is now an event on an existing chat, reported with Claude's own exit reason, instead of a stored create outcome.

2. Who can start a Claude process for an existing chat

Before (main): two starters that didn't coordinate, and a send wasn't one of them.

Path Starts Claude? Coordinated how
Opening the chat view (a "hold") Yes, once Checked "is there a process?" outside the per-chat queue, then asked attach to start one
Host's recovery after an unexpected exit Yes, only if a view was holding the chat Inside the per-chat queue, but through its own path
Sending a message No. Refused agent_session_ownership_unknown Clients read that as "retry", so they resent forever

When two starters raced, the only tie-break was attach's fence check deep inside. The loser was refused, and a losing hold silently stopped holding the chat.

After: one function, one queue, three callers.

chat view opens ─┐
send arrives ────┼──► per-chat queue ──► ensureProviderChild ──► process running? ── yes ──► nothing to do
exit recovery ───┘                                                     │
                                                                       no ──► start one (attach)

All three paths call ensureProviderChild from inside the chat's existing per-chat queue, in structured-agent-session-holds.ts. They take turns: the first one starts Claude, and the next finds it running and starts nothing. There is no longer a second map of in-flight restarts, and no pre-check outside the queue.

3. How a message is admitted

Before (main): one step. The operation ledger and the ownership lease were checked together. With no owner, the answer was a refusal that told the client to wait, and nothing would ever change that answer.

After: two phases, with a preparation step between them (structured-agent-session-mutation-admission.ts, structured-agent-session-send-preparation.ts):

1. ledger decision ─┬─ already answered ──► load chat read-only if needed ──► replay the answer
                    │                        (never starts Claude)
                    └─ new message ──► 2. prepare: no process, lease cleanly released, nothing refuses the send?
                                           ── yes ──► ensureProviderChild (one attempt)
                                           ── no  ──► leave the lease alone
                                        3. place the ledger row, check lease and fence
                                           against the lease as it stands now ──► admit once

The ledger is still consulted first, as it always was. Only the gap between "the ledger says this is new" and "check the lease" has a new step in it, and it runs inside the same queued turn.

4. How a failed restart is described

Before (main): the restart threw new Error(code). Callers compared the message string, and the cause, the exit reason and whether the process was proven gone were all dropped.

After: the restart returns a typed result: { ok: true, fromFence } or { ok: false, refusal }, where the refusal carries its message and ownerVerdict. The send looks the refusal code up in a table keyed by the full list of refusal codes (transient / failed / unresumable). Adding a refusal code without classifying it is a compile error. The cause travels all the way to the user: "Claude couldn't restart: Not signed in."

5. How long an unwatched chat keeps its process

Before (main): 15 seconds after the last view left. Only a running turn could delay the stop.

After: 30 minutes, and any write to the chat restarts the window. A stop is also deferred by any work still owed: a running turn, or a message sent but not yet taken by Claude.

Why the new architecture is better

  • Each question has exactly one place that answers it.

    • "Is Claude ready yet?" is answered by the startup gate.
    • "Does this chat need a process?" is answered by ensureProviderChild, inside the per-chat queue.
    • "Has this message already been answered?" is answered by the operation ledger, which is consulted first.

    On main, two or three places answered each of these, and the bugs lived in the gaps between them. A hold and an exit recovery could both start Claude. A send couldn't start it at all. A timer, not an event, decided whether a start had failed.

  • Races are removed by structure, not guarded against. An earlier revision of this fix checked for a process before the send joined the per-chat queue. Each of three review rounds then found a new defect on that boundary:

    • two restarts racing each other
    • a copy of the ledger's "already answered" rule that read the wrong source
    • a guessed ownership fence

    Moving the check inside the queue made those classes impossible: nothing can run between "is there a process?" and "start one". By the architecture review's count, five of the seven defects the review rounds found could not occur in the final design.

  • Failure is reported by proof, not by a clock. A start fails when the process exits or Claude says why, never because a number was too small for this machine. And Retry starts over only when the host holds that proof.

  • New failure modes can't be silently misfiled. The restart outcome table covers every refusal code at compile time. An unclassified code can't quietly become "give up" or "retry forever".

  • One fix covers every client. The restart lives on the host that runs Claude. Desktop, chats on SSH-connected hosts and mobile all get it without client changes. That matters most on mobile, which has no automatic resend: the host restarts Claude and delivers the message in one round trip, instead of answering "stale, try again".

  • Every path ends somewhere the user can act. A send now ends in one of two states: delivered, or a single visible failure that names its cause and offers Retry. The old states with no way out are gone: "not ready yet" forever, "delivery unconfirmed" with no Retry, and a create failure that Retry could only replay.

  • It reuses the machinery that was already there. No new queue, lock or retry loop was added. The per-chat queue, the operation ledger, the ownership lease and fence, and the idle clock were already the host's. The new code is the preparation step between two existing admission checks, one shared start function, and a startup gate in the adapter, which already owned what gets written to Claude.

What it costs.

  • Memory. Idle chats keep a Claude process for up to 30 minutes.
  • Latency on some sends. A send to a chat whose process is gone waits for Claude to start before it's answered.
  • One path still runs in the renderer. See the open question below.

Known limits and open questions

  • A Claude process killed after a successful start isn't noticed. If the process is force-killed (kill -9) after it has answered at least once, the adapter never reports the exit to the host. The chat keeps a "live" lease pointing at a dead process, and the next send sits at "delivery unconfirmed". This happens on main too, and neither this PR nor the send restart can see it, because both start from an exit report that never arrives. The fix belongs in the adapter's exit handling. It will be a follow-up PR.
  • Nothing caps idle processes. Every chat touched in the last 30 minutes can keep one Claude process. That trades memory for no restart delay; there is no LRU cap yet.
  • One restart path still runs in the renderer. If a chat's very first create failed before the host published the chat, the host has no chat to restart. In that case a send from the chat view asks the renderer to run the create again (sendThroughRelaunch), and the message goes out when it publishes. Everything else restarts on the host. Whether this path belongs in this PR, or should move to the host later, is still an open question.

Linked Issue

No issue filed. Reported directly: "Could not open Claude chat" on a loaded machine, Retry not recovering, and a chat stuck resending after its agent stopped.

Visual Proof

No layout or styling changed. The visible differences are the "still starting" line in the existing status area, and a failed send showing its cause in the existing error strip and as one transcript row. Screenshots from the hidden-window Electron validation passes are in the comments on #22359.

Testing

On the combined head with current main merged in: pnpm tc passes. Vitest over src/main/claude, src/main/native-chat, src/main/runtime, src/shared and the affected renderer folders passes, apart from two tests that drive the real Claude binary (claude-structured-real-cli, claude-tui-resume-real-binary). Those fail the same way on main on the test machine. Oxlint, the changed-lines quality gate, React Doctor and the localization catalog checks pass. Each new test was checked by deleting the production code it covers and confirming it fails.

Manual: nine hidden-window Electron runs of the combined code, with a Claude CLI that can be made to fail at startup. The last run, after merging current main, passed every scenario:

  • a failed start shows its cause
  • a send while Claude is still broken is marked not sent and names the cause, with Retry
  • Retry after Claude is fixed delivers once
  • a hidden idle chat keeps its Claude process
  • /clear keeps the chosen model
  • quitting and reopening resumes the same conversation

New test files, grouped by what they pin:

  • Startup without a deadline, and held messages: claude-structured-session-startup.test.ts, claude-structured-send-held-for-startup.test.ts, claude-structured-startup-unanswered-control-request.test.ts, structured-agent-session-provider-started.test.ts, structured-agent-session-runtime-provider-started.test.ts, structured-agent-session-starting-release.test.ts
  • Failed starts and failed creates: structured-agent-session-failed-create-owner-verdict.test.ts, structured-agent-session-failed-create-sink-release.test.ts, structured-agent-session-startup-failure-exit.test.ts, claude-structured-failed-start-resume.test.ts, claude-structured-resumed-start-failure.test.ts
  • A send restarting a missing process: structured-agent-session-send-preparation.test.ts, structured-agent-session-send-restarts-failed-start.test.ts, claude-structured-send-restart-dies-before-dispatch.test.ts
  • Desktop UI: NativeChatStructuredSessionStatus.test.tsx, use-structured-agent-session-outbox-rejection-cause.test.tsx, structured-agent-session-launch-exited-owner.test.ts

Plus 43 updated test files, including structured-agent-session-holds.test.ts (the 30-minute window and its renewal), structured-agent-session-hold-resume-race.test.ts, structured-agent-session-refusal-retry.test.ts, NativeChatStructuredSession.launch-lifecycle.test.tsx and use-structured-agent-session-outbox.test.tsx.

AI Disclosure

Implemented with Claude (Anthropic).

Review

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

Author X: @BrennanKB5

  • Size. 151 files (+7,365 / −1,276) against main: 25 added, 1 deleted (claude-structured-init-deadline.ts), 125 modified; 68 are test files. fix(native-chat): a send with no live owner restarts it once #22359 was folded into this branch by a fast-forward, with current main already merged in.
  • Wire compatibility. hostExecutionPhase on the status feed and ownerVerdict on refusals are new optional fields on existing frames. An old client ignores them. A new client talking to an old host sees neither, so "starting" never shows and a missing verdict stays "could not confirm". agent_session_owner_restart_failed is a new value of the existing refusal code. The desktop outbox classifies it as settled-rejected, so it stops auto-retrying. An older desktop client treats the unknown code as a blocked send and shows the host's message, keeping the same message id. Mobile treats an unknown code as rejected and shows the message. Neither loops.
  • SSH, remote and folder workspaces. Recovery is keyed only on the session record held by the execution host, and an exit is judged where Claude ran. Nothing assumes a local git worktree.
  • Performance. The startup deadline timer is gone. Restoring a saved option during startup waits at most the CLI's 30-second request timeout per option, then skips it; it never fails the start. The idle clock re-arms on each chat write only while a stop is pending, which costs one timer reset. The real cost is memory: every chat touched in the last 30 minutes may keep one 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)

…e Retry start fresh

Publish the Claude session as soon as its process is spawned instead of racing
initialize against a fixed 10s deadline. Prompts sent before startup lands are
held and written in order once it does. An exit or sign-in failure before startup
ends the session with the reason and the CLI's stderr.

A create that failed because the process provably exited now carries
ownerVerdict 'exited', so the client marks the launch failed and Retry mints a
new operation instead of replaying the stored failure.
@coderabbitai

coderabbitai Bot commented Sep 23, 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
📝 Walkthrough

Walkthrough

Claude sessions now publish before startup completes, hold prompts until startup is proven, and report startup status through the host and renderer. Failed acquisition results include provider ownership verdicts that affect reservation and launch retry handling. Claude launch resolution also checks transcript existence when selecting resume options.

Priority: ➖ Normal

Merge Risk: 🟡 Moderate · up to 0709d

This change lets Claude chats open without a startup deadline and makes Retry start fresh after a failed start. Some paths can still leave a failed start marked as having an unknown owner, so Retry cannot start fresh. A prompt sent during startup can be accepted but never delivered, and a session can keep showing "starting" after the host has given it up. These should be fixed or explicitly accepted before merging.

🚥 Pre-merge checks | ✅ 2 | ❌ 3

❌ Failed checks (3 warnings)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 26.36% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 110 functions across 86 files. (1 skipped… Write docstrings for the functions missing them to satisfy the coverage threshold.
Linked Issues check ⚠️ Warning The description states that no issue was filed, but the repository template requires an issue link in the Linked Issue section. Add the relevant issue reference and use the required Fixes #<issue-number> format.
Description check ⚠️ Warning The description is detailed and covers the change, rationale, testing, risks, and compatibility. However, it does not provide the required linked issue and does not clearly provide the required visual… Add a valid issue reference after Fixes #. Attach before-and-after screenshots or a video for the startup and failure-state UI changes, or explain why the visual proof requirement does not apply.
✅ Passed checks (2 passed)
Check name Status Explanation
Out of Scope Changes check ✅ Passed The changes are broad but support the stated startup, retry, failed-create, session-recovery, and user-status objectives. The description explains the cross-layer changes and their relationship to the…
Title check ✅ Passed The title clearly summarizes the main changes: removing the Claude startup deadline and making Retry start a fresh attempt.
Full details: Docstring Coverage

Explanation

Docstring coverage is 26.36% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 110 functions across 86 files. (1 skipped: 1 unsupported.)

Full details: Description check

Explanation

The description is detailed and covers the change, rationale, testing, risks, and compatibility. However, it does not provide the required linked issue and does not clearly provide the required visual proof for the documented behavior changes.

✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • 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.

@coderabbitai coderabbitai 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.

Actionable comments posted: 3


ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: 91139556-98f6-43db-b60c-71c347a783e0

📥 Commits

Reviewing files that changed from the base of the PR and between dfff391 and 0610f76.

📒 Files selected for processing (52)
  • src/main/claude/claude-agent-sdk-control-requests.ts
  • src/main/claude/claude-stream-json-connection.test.ts
  • src/main/claude/claude-structured-dispatch-test-support.ts
  • src/main/claude/claude-structured-dispatch.ts
  • src/main/claude/claude-structured-effort-reporting.test.ts
  • src/main/claude/claude-structured-init-deadline.ts
  • src/main/claude/claude-structured-launch-resolution.test.ts
  • src/main/claude/claude-structured-launch-resolution.ts
  • src/main/claude/claude-structured-model-confirmation.test.ts
  • src/main/claude/claude-structured-model-preflight.test.ts
  • src/main/claude/claude-structured-option-confirmation.test.ts
  • src/main/claude/claude-structured-options.test.ts
  • src/main/claude/claude-structured-options.ts
  • src/main/claude/claude-structured-prompt-ownership.ts
  • src/main/claude/claude-structured-real-cli.test.ts
  • src/main/claude/claude-structured-session-acquisition-processless.test.ts
  • src/main/claude/claude-structured-session-acquisition.ts
  • src/main/claude/claude-structured-session-adapter.test.ts
  • src/main/claude/claude-structured-session-adapter.ts
  • src/main/claude/claude-structured-session-close.ts
  • src/main/claude/claude-structured-session-journal-control.ts
  • src/main/claude/claude-structured-session-options.ts
  • src/main/claude/claude-structured-session-publication.ts
  • src/main/claude/claude-structured-session-reading-control.test.ts
  • src/main/claude/claude-structured-session-recovery.test.ts
  • src/main/claude/claude-structured-session-startup-gate.ts
  • src/main/claude/claude-structured-session-startup.test.ts
  • src/main/claude/claude-structured-session-startup.ts
  • src/main/claude/claude-structured-session-state.ts
  • src/main/claude/claude-structured-session-test-support.ts
  • src/main/native-chat/agent-session-wire/structured-agent-session-adapter.ts
  • src/main/native-chat/agent-session-wire/structured-agent-session-attach-flow.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-failed-create-owner-verdict.test.ts
  • src/main/native-chat/agent-session-wire/structured-agent-session-failed-create-refusal.ts
  • src/main/native-chat/agent-session-wire/structured-agent-session-startup-failure-exit.test.ts
  • src/main/native-chat/agent-session-wire/structured-agent-session-unexpected-exit.ts
  • src/main/runtime/agent-session-reservation-admission.test.ts
  • src/main/runtime/agent-session-reservation-admission.ts
  • src/main/runtime/claude-structured-session-integration.test.ts
  • src/main/runtime/structured-claude-runtime-adapter.ts
  • src/renderer/src/components/native-chat/NativeChatLaunchRetry.tsx
  • src/renderer/src/components/native-chat/NativeChatStructuredSession.launch-lifecycle.test.tsx
  • src/renderer/src/components/native-chat/NativeChatStructuredSession.test-harness.tsx
  • src/renderer/src/components/native-chat/NativeChatStructuredSession.tsx
  • src/renderer/src/components/native-chat/use-native-chat-provisional-launch.ts
  • src/renderer/src/lib/launch-structured-agent-session.ts
  • src/renderer/src/lib/structured-agent-session-launch-exited-owner.test.ts
  • src/renderer/src/lib/structured-agent-session-launch-registry.ts
  • src/renderer/src/lib/structured-agent-session-launch.ts
  • src/shared/agent-session-lease-adjudication.ts
  • src/shared/agent-session-wire-refusals.ts
💤 Files with no reviewable changes (1)
  • src/main/claude/claude-structured-init-deadline.ts

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

Comment on lines +72 to +83
// Startup may have failed while the admission barrier ran.
const failed = claudeStartupFailureReason(session)
if (failed) {
return { state: 'rejected', reason: failed }
}
const { waiter } = input.arm()
session.startup.held.push({
waiter,
message: input.message,
...(input.settleLate ? { settleLate: input.settleLate } : {})
})
return { state: 'admitted' }

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.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

A prompt queued after startup finishes draining is never written.

holdClaudeStartupWrite checks only for failure after await input.beforeDispatch(). It does not check whether the gate is still holding writes. The failing sequence is:

  1. dispatchClaudeTurn enters the hold path while state === 'pending'.
  2. beforeDispatch (the host admission barrier) is awaited.
  3. During that wait, settleClaudeSessionStartup calls openClaudeStartupGate. The gate drains an empty held queue, then sets state = 'proven' and draining = false.
  4. beforeDispatch resolves. The entry is pushed onto gate.held.

After step 4, nothing drains gate.held again. openClaudeStartupGate returns early because the state is not pending. The caller receives { state: 'admitted' }, but the message is never sent. The armed waiter stays in session.dispatchWaiters indefinitely and counts toward MAX_ACTIVE_DISPATCH_WAITERS. The same gap opens if draining finishes while an earlier prompt's beforeDispatch is still pending.

Suggested fix: move the drain loop into a helper. Call it from both openClaudeStartupGate and the hold path when the gate is proven and idle. This keeps write order and routes failures through settleLate.

🐛 Proposed fix
   const { waiter } = input.arm()
   session.startup.held.push({
     waiter,
     message: input.message,
     ...(input.settleLate ? { settleLate: input.settleLate } : {})
   })
+  // Startup may have landed and drained while the admission barrier ran.
+  if (!claudeStartupHoldsWrites(session)) {
+    void drainClaudeStartupWrites(session)
+  }
   return { state: 'admitted' }
 }
 
 export async function openClaudeStartupGate(session: ClaudeSession): Promise<void> {
   const gate = session.startup
   if (gate.state !== 'pending') {
     return
   }
   gate.state = 'proven'
+  await drainClaudeStartupWrites(session)
+}
+
+async function drainClaudeStartupWrites(session: ClaudeSession): Promise<void> {
+  const gate = session.startup
+  if (gate.state !== 'proven' || gate.draining) {
+    return
+  }
   gate.draining = true
   try {
     for (let held = gate.held.shift(); held; held = gate.held.shift()) {
       await writeHeld(session, held)
     }
   } finally {
     gate.draining = false
   }
 }

Add a test that uses a beforeDispatch that resolves only after drainStartup settles. The test should assert that connection.sent receives the message.

})
if (replay.decision === 'refuse') {
return { ok: false, refusal: replay.refusal }
return failedCreateRefusal(replay.refusal, reserved.operationRow.outcome.status, record)

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.

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

🔎 Supported by static analysis

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect acquisition refusal construction and adapter acquire implementations.
rg -n -C5 \
  'new AgentSessionAcquisitionRefusal|AgentSessionAcquisitionRefusal|acquire\s*[:=(]' \
  src/main

Repository: stablyai/orca

Length of output: 45543


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- target flow and helper references ---'
rg -n -C6 'failedCreateRefusal|AgentSessionAcquisitionRefusal|settleFailedAcquisition|rewindRefusal' \
  src/main/native-chat/agent-session-wire/structured-agent-session-attach-flow.ts \
  src/main/native-chat/agent-session-wire src/main/claude

printf '%s\n' '--- target flow ---'
cat -n src/main/native-chat/agent-session-wire/structured-agent-session-attach-flow.ts | sed -n '1,250p'

printf '%s\n' '--- structured adapter acquire declarations and implementations ---'
rg -n -C12 'acquire\s*=\s*\(|async acquire|function acquire|AgentSessionAcquisition' \
  src/main/claude --glob '!*.test.ts' --glob '!*-support.ts'

Repository: stablyai/orca

Length of output: 42960


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- failed-create helper ---'
cat -n src/main/native-chat/agent-session-wire/structured-agent-session-failed-create-refusal.ts

printf '%s\n' '--- acquisition flow ---'
cat -n src/main/claude/claude-structured-session-acquisition.ts | sed -n '43,240p'

printf '%s\n' '--- acquisition error resolution and cleanup ---'
cat -n src/main/claude/claude-structured-session-close.ts | sed -n '30,145p'
cat -n src/main/claude/claude-structured-session-close.ts | sed -n '270,300p'

printf '%s\n' '--- settlement types and exit-proof consumer ---'
rg -n -C8 'exitProof|ownerVerdict|failedCreateRefusal|settleFailedAcquisition' \
  src/main/native-chat/agent-session-wire src/main src/shared \
  --glob '*.ts' --glob '!*.test.ts' | head -n 500

Repository: stablyai/orca

Length of output: 42034


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- remaining acquisition implementation ---'
cat -n src/main/claude/claude-structured-session-acquisition.ts | sed -n '205,330p'

printf '%s\n' '--- attach settlement and proof mapping ---'
cat -n src/main/native-chat/agent-session-wire/structured-agent-session-attach-flow.ts | sed -n '160,245p'

printf '%s\n' '--- init proof and startup facts error path ---'
rg -n -C10 'claudeInitializationAuthError|readClaudeStartupFacts|resolveClaudeAcquisitionError|throw .*Refusal|initProof' \
  src/main/claude/claude-structured-init-proof.ts \
  src/main/claude/claude-structured-startup-facts.ts \
  src/main/claude/claude-structured-session-acquisition.ts

Repository: stablyai/orca

Length of output: 25419


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- startup refusal propagation ---'
rg -n -C12 'claudeInitializationAuthError|readClaudeStartupFacts|throw .*auth|throw .*refusal' \
  src/main/claude/claude-structured-session-startup.ts

printf '%s\n' '--- publication instrumentation contract ---'
rg -n -C12 'function withAgentSessionCreatePhase|export .*withAgentSessionCreatePhase|withAgentSessionCreatePhase' \
  src/main/observability src/main/claude/claude-structured-session-publication.ts

printf '%s\n' '--- structured adapter implementations ---'
rg -l 'implements StructuredAgentSessionAdapter|StructuredAgentSessionAdapter' src/main \
  --glob '*.ts' --glob '!*.test.ts' --glob '!*-support.ts' | sort

Repository: stablyai/orca

Length of output: 6896


🏁 Script executed:

#!/bin/bash
set -euo pipefail
rg -n -C12 'function agentSessionLeaseOwnerVerdict|agentSessionLeaseOwnerVerdict\s*=|export .*agentSessionLeaseOwnerVerdict' \
  src/shared/agent-session-lease-adjudication.ts

Repository: stablyai/orca

Length of output: 1183


Default post-publication acquisition failures to unproven.

If publication sets sessions before a later publication callback fails, the acquisition catch skips provider cleanup because the session already references the attempt. The ordinary error then selects exitProof: 'exit-proven', releases the lease, and permits a retry while the published provider can remain alive.

Use unproven for errors without explicit exit proof. Preserve exit-proven only for an error type that records successful cleanup.

Suggested proof-default fix
-            : 'exit-proven'
+            : 'unproven'

Comment on lines +43 to +48
function exitReasonDetail(reason: string | undefined): string | undefined {
return reason
?.slice(0, MAX_UNEXPECTED_EXIT_REASON_CHARS)
.trim()
.replace(/[.\s]+$/, '')
}

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.

🔒 Security & Privacy | 🛡️ Detected with Advanced Tier | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Trace diagnostic producers and existing redaction utilities.
rg -n -C4 \
  'startupUnproven|unexpectedExitReason|failureReason|stderr|redact|sanitize|scrub' \
  src/main src/renderer src/shared

Repository: stablyai/orca

Length of output: 45602


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- dead-generation settlement ---'
sed -n '1,180p' src/main/native-chat/agent-session-wire/structured-agent-session-dead-generation-settlement.ts

printf '%s\n' '--- launch producer and nearby state ---'
sed -n '1,230p' src/renderer/src/lib/structured-agent-session-launch.ts

printf '%s\n' '--- launch failure consumers ---'
rg -n -C6 'failureReason|useStructuredAgentSessionLaunchFailureReason|structured-agent-session-launch' src/renderer/src --glob '*.ts' --glob '*.tsx'

printf '%s\n' '--- exact redaction candidates ---'
rg -n -C3 'redact|sanitize|scrub|boundJournalStatusText|boundInlineText|DEFAULT_JOURNAL_PAYLOAD_LIMITS' \
  src/main/native-chat/agent-session-wire/structured-agent-session-dead-generation-settlement.ts \
  src/main/native-chat/agent-session-journal \
  src/renderer/src/lib/structured-agent-session-launch.ts \
  src/renderer/src/lib/structured-agent-session-launch-registry.ts \
  src/renderer/src --glob '*agent*session*' --glob '*redact*' --glob '*sanitize*'

Repository: stablyai/orca

Length of output: 41877


🏁 Script executed:

sed -n '1,180p' src/main/native-chat/agent-session-wire/structured-agent-session-dead-generation-settlement.ts; sed -n '1,230p' src/renderer/src/lib/structured-agent-session-launch.ts; rg -n -C6 'failureReason|useStructuredAgentSessionLaunchFailureReason' src/renderer/src; rg -n -C3 'redact|sanitize|scrub|boundJournalStatusText|boundInlineText' src/main/native-chat/agent-session-wire/structured-agent-session-dead-generation-settlement.ts src/main/native-chat/agent-session-journal src/renderer/src/lib/structured-agent-session-launch.ts src/renderer/src/lib/structured-agent-session-launch-registry.ts

Repository: stablyai/orca

Length of output: 41886


Sensitive Data Exposure

CWE: CWE-200 — Exposure of Sensitive Information to an Unauthorized Actor

Reachability path
● Entry
  src/main/native-chat/agent-session-wire/structured-agent-session-startup-failure-exit.test.ts:75
  settleUnexpectedStructuredAgentSessionExit
│
▼
● Hop
  src/main/native-chat/agent-session-wire/structured-agent-session-unexpected-exit.ts:187
  retryUnexpectedExitSettlement
│
▼
● Sink
  src/main/native-chat/agent-session-wire/structured-agent-session-dead-generation-settlement.ts

Redact provider and launch diagnostics before persistence or display.

boundJournalStatusText only limits journal text size. exitReasonDetail only trims the provider reason, and the renderer stores error.message directly before displaying it through NativeChatLaunchRetry. Use one shared redaction function at both boundaries to remove credentials and personal data.

📍 Affects 2 files
  • src/main/native-chat/agent-session-wire/structured-agent-session-dead-generation-settlement.ts#L43-L48 (this comment)
  • src/renderer/src/lib/structured-agent-session-launch.ts#L138-L138

Source: Learnings

@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.

Important

One race can leave a restart continuation admitted but never sent. Everything else in the diff checks out — details inline.

Reviewed changes

  • Startup is now untimed. claude-structured-init-deadline.ts is deleted; initializationResult runs with timeoutMs: null and the session is published at spawn, with init facts applied asynchronously by the new settleClaudeSessionStartup.
  • New held-write gate. claude-structured-session-startup-gate.ts parks prompts sent before startup and writes them in order once init facts and the saved-option restore land; failure rejects everything held.
  • Failure ends the session. A start that exits, proves unauthenticated, proves a foreign session id, or fails the option restore faults the published session through the exit path instead of failing the create.
  • Retry starts fresh. Create refusals carry an optional ownerVerdict (live/unverifiable/exited); a durably failed create over a released, never-bound record can be re-created, and the client treats exited as definite and retries under a new operation id.
  • Leafless Claude heads consult the transcript before choosing --resume vs a fresh --session-id.
  • Renderer surfaces the failure reason beside Retry; dead-generation settlement gets exitedDuringStartup.

I traced the recreate path (agentSessionLeaseOwnerVerdict → recreatable → failedCreateRefusal) against every producer of a released lease: each is gated on processless or first-hand root-exit observation, so it cannot admit a second writer while the first provider root runs.

ℹ️ Nitpicks

  • mobile/src/session/mobile-structured-agent-session-launch.ts ignores ownerVerdict, so a Claude create failure the host marks exited still classifies as unknown on mobile. Retry logic may differ there, but the UI copy will not match desktop.

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

Comment thread src/main/claude/claude-structured-session-startup-gate.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

  • Sending into a failed launch restarts it. use-native-chat-provisional-launch.ts adds sendThroughRelaunch, which lets the outbox accept the message and then retries the launch when its lifecycle is failed; the queued message is delivered when the restart publishes.
  • The composer transport routes through it. NativeChatStructuredSession.tsx wraps controller.send in sendThroughRelaunch, so a typed message is both queued and triggers the fresh create under a new operation.
  • Tests cover the paths that matter. The new cases assert the relaunch fires once and the message is delivered after publish, that a second failure keeps the message queued with the reason shown beside Retry, and that a visibility-unknown launch is left parked without relaunching.

Approval is withheld only because the earlier startup-gate admission race (claude-structured-session-startup-gate.ts) remains open and is unchanged by this commit.

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

A provider child that timed out or exited hands its lease back, and every
later send was refused agent_session_ownership_unknown. Clients read that
code as "not admitted yet" and resend forever, while only a surface hold
could make a new child, once per mount, with its failure swallowed.

The send now routes to a live owner, otherwise restarts one from the
persisted resume state where resume eligibility allows it (single-flight
per session), otherwise refuses with the new settled
agent_session_owner_unrecoverable. Unverifiable, reserved and handed-off
leases are left alone. The desktop hold now logs its failure.
A child that dies between spawn and journal attach can still write through
the host's event sink, which attach unbound in onAcquiring and never re-bound
because onAttached never ran. The orchestration released that sink only when
performAttach returned a refusal; a thrown failure (the root-exit path) kept
the sink cached with its queued write, so the next attach's drain barrier and
runtime shutdown's flush waited forever.

Also pins the publish-on-root-exit clause for a start that never proved:
deleting it reddened nothing before.

@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

  • A thrown create releases its event sink. structured-agent-session-attach-orchestration.ts extracts discardUnattachedSink and now calls it from a .catch() around performAttach, so the root-exit path that throws (through classifyStoreFailure's rethrow) no longer leaves the unbound sink cached with a queued write that wedges the next attach's drain and runtime shutdown.
  • Regression coverage for the stranded sink. New structured-agent-session-failed-create-sink-release.test.ts proves a second attach succeeds and flushAllStreamedEvents() resolves after a throwing create; I confirmed it fails with the .catch() removed.
  • Root-exit settlement pinned. A new claude-structured-session-startup.test.ts case asserts a start whose root exit was observed first-hand still ends with cause: 'unexpected-exit' when its descendants are unverifiable.

Approval is withheld only because the earlier startup-gate admission race (claude-structured-session-startup-gate.ts) remains open and is untouched by these commits.

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

@coderabbitai coderabbitai 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.

Actionable comments posted: 2


ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: 7715464b-71ca-48e0-8d99-18bc1d6edfe2

📥 Commits

Reviewing files that changed from the base of the PR and between a0aa442 and 2b0d5cf.

📒 Files selected for processing (3)
  • src/main/claude/claude-structured-session-startup.test.ts
  • src/main/native-chat/agent-session-wire/structured-agent-session-attach-orchestration.ts
  • src/main/native-chat/agent-session-wire/structured-agent-session-failed-create-sink-release.test.ts

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

…a send joining a restart rebases from the fence it replaced
…ists only proven options

A publish-first create used to read the session's options before Claude had
answered initialize. With startup pending that read fell back to the built-in
catalog's default, so `record.options.model` was persisted as `sonnet` for
every user whose CLI default is something else; an owner handoff or a reopen
then replayed `set_model('sonnet')` and silently switched their model.

The adapter now reports `started` once startup facts are applied and saved
options restored. The host keeps a `providerChildPhase` on the session it
owns: a starting child hands over nothing but the saved options as intent,
and the `started` event re-reads the options as fact and persists them through
the same record write a user's option change takes. The status summary carries
`hostExecutionPhase` (optional, wire-safe), and the chat pane says the agent is
still starting instead of showing nothing.

A child whose exit already reached the adapter before acquire returns is no
longer handed over as live; the create fails with the CLI's diagnostic.
…e restart, and a send the ledger already holds restarts nothing
…once at the boundary

A create whose Claude process was seen to exit answered twice in two shapes:
the first call threw a generic runtime error, and only the replay of the same
operation carried the `ownerVerdict: 'exited'` refusal that lets a client
retry under a new operation. Three sites stamped the verdict and the store
failure path stamped nothing.

The first-hand root exit is now returned as the refusal on the first call,
with the provider's own diagnostic as its message. The verdict is stamped in
one place, at the boundary of the attach, from the durable row the operation
settled to, so every refusal shape answers the same fact and no site can
forget it. The per-site stamps are gone.

@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 critical issues — one minor suggestion inline.

Reviewed changes

Since the prior pullfrog review (2b0d5cf), one commit landed (d3e47d7), fixing the pre-startup options read:

  • A publish-first Claude create no longer persists a guessed model. acquireClaudeSession now emits a new started event after startup facts are applied and saved options restored, and hands the child over as providerChildPhase: 'starting'; a child whose exit already reached handleExit before acquire returns now fails the create with the CLI's diagnostic instead of being handed over as live.
  • The host persists only proven options. acquireOwner and acquireNativeHandoffOwner skip the pre-startup options read while starting, and the new settleStructuredAgentSessionProviderStarted re-reads the options as fact and writes them through the same record write a user's option change takes.
  • Status carries the startup phase. hostExecutionPhase (starting/ready) is a new optional field on AgentSessionStatusSummary, and the chat pane says the agent is still starting while it is starting.
  • Lifecycle plumbing widened. StructuredAgentSessionLifecycleEvent becomes a started | ended union (with StructuredAgentSessionEndedEvent for exit handling), and the Claude runtime adapter's onUnexpectedExit becomes onLifecycleEvent.

I traced the new starting phase through a re-attach and the native handoff path: resolveClaudeAcquisitionLaunch closes any previously published Claude session before spawning, so every handover runs its own settleClaudeSessionStartup and re-emits started — there is no path where an already-proven child is adopted and left starting.

The earlier startup-gate admission race (claude-structured-session-startup-gate.ts) is untouched by this commit and stays open.

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

? {
hostExecutionOwned: true as const,
...(session.providerChildPhase
? { hostExecutionPhase: session.providerChildPhase }

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.

hostExecutionPhase is documented as meaningful only alongside hostExecutionOwned, but revokeLive (line 178) destructures out hostExecutionOwned alone, so a session closed while starting keeps hostExecutionPhase: 'starting' in the retained published cache and in the broadcast/client snapshot. NativeChatStructuredSession reads summary.hostExecutionPhase without checking ownership, so that stale phase renders "Claude is still starting" for a session this host no longer runs. Strip the phase in revokeLive too (the renderer's own revokeSnapshotOwnership already does), or gate the pane's read on hostExecutionOwned.

Technical details
# `hostExecutionPhase` outlives `hostExecutionOwned`

## Affected sites
- `src/main/native-chat/agent-session-wire/structured-agent-session-status-feed.ts:178` — `revokeLive` drops only `hostExecutionOwned` from the retained summary, leaving `hostExecutionPhase` (`:263` is where it is added).
- `src/renderer/src/components/native-chat/NativeChatStructuredSession.tsx:43-45` — reads `summary.hostExecutionPhase` with no `hostExecutionOwned` guard.
- `src/renderer/src/runtime/structured-agent-session-status-feed.ts:98` — the client's `revokeSnapshotOwnership` already strips both, confirming the intended pairing.

## Reachability
- `host.close` (`structured-agent-session-host.ts:202`) → `clientDelivery.closeSession` (`structured-agent-session-client-delivery.ts:66`) → `statusFeed.close` → `revokeLive`. Closing a chat mid-start is exactly the action the new copy invites ("close this chat to give up on it").

## Required outcome
- A summary without `hostExecutionOwned` carries no `hostExecutionPhase`, so the pane never shows a startup message for a session this host does not own.

## Suggested approach
- In `revokeLive`, destructure both fields: `const { hostExecutionOwned: _owned, hostExecutionPhase: _phase, ...retained } = previous`; and/or gate the pane's read on `summary.hostExecutionOwned`.

@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 newest commit (6c0393a) reworks how a failed Claude create reports its owner verdict:

  • The first call answers in the replay's shape. performAttach now catches AgentSessionAcquisitionRootExitObservedError and returns it as a refusal (agent_session_operation_invalid, carrying the provider's own message) instead of letting the first call throw a generic error that only the replay resolved.
  • The verdict is stamped once, at the attach boundary. The per-site failedCreateRefusal(...) calls are gone; the new stampFailedCreateOwnerVerdict wraps performAttach's result in attachStructuredAgentSession and reads the durable operation row (via the new AgentSessionRecordStore.getOperationRow) and the record, so every refusal shape answers the same fact and the store-failure path can no longer skip it.
  • Coverage for the first-call path. New host and renderer tests (structured-agent-session-failed-create-owner-verdict.test.ts, structured-agent-session-launch-exited-owner.test.ts) assert the first call already carries ownerVerdict: 'exited', its replay is identical, and a fresh operation retries.

I checked the details that could break this: AgentSessionAcquisitionRootExitObservedError extends Error, so the catch's ordering (RewindRefusal → AcquisitionRefusal → root exit) is correct; getOperationRow uses the same agentSessionOperationKey(callerKey, operationId) that settleFailedAcquisition writes; and performAttach has no production caller besides attachStructuredAgentSession, so the boundary stamp covers every production path (direct callers are tests only).

Approval is withheld only because the earlier startup-gate admission race and the revokeLive phase-retention note remain open.

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

… before admission

A session that published and then lost its Claude child before startup (not
signed in, for one) keeps a released lease and a chat the user can still type
into. The send was refused as ownership-unknown, the outbox parked it as
pending admission, and nothing ever restarted the child: the message sat
there until the user closed and reopened the tab.

A send reaching a session with no provider child now runs the same resume a
surface's first hold runs, before the write is admitted. The resume reserves
a new fence, so that send is answered stale with the published fence and the
client's outbox re-drives under it, as after any fence change. A resume that
fails is not this send's answer; admission reports the lease as it stands.

@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

Since the prior pullfrog review (6c0393a), one commit landed (39234e4):

  • A send into a childless session restarts it before admission. StructuredAgentSessionHolds.resumeForWrite runs the same resume a surface's first hold runs, without taking a holder; resumeBeforeSend in sendStructuredAgentSessionTurn calls it (only for an attached, childless session whose record isResumableStructuredAgentSessionRecord) before the write is admitted.
  • A failed resume is not the send's answer. Its error is swallowed so admission reports the lease as it stands; the resume publishes a new fence, so the send is refused agent_session_checkpoint_stale with currentFence and the client's outbox re-drives under it.
  • Coverage. New structured-agent-session-send-after-failed-start.test.ts asserts the resume acquires a second child, the first send is refused stale with the published fence, the same body is admitted once the fence is current, and a send against a live child never resumes. Both cases pass locally.

I traced the re-drive the code comment relies on: the stale refusal sets currentFence (agent-session-mutation-envelope.ts:114-130), maps to pending-admission (agent-session-refusal-retry.ts), and structured-agent-session-outbox.ts requeues the same client message id — while the refused operation's ledger row is deleted (agent-session-operation-admission.ts:108), so the re-drive admits fresh rather than replaying the refusal. The restart continuation and launch-prompt senders reach the same path with a live child, so resumeBeforeSend no-ops there.

Approval is withheld because the two earlier threads remain open and are untouched by this commit: the startup-gate admission race (claude-structured-session-startup-gate.ts) and the revokeLive hostExecutionPhase retention (structured-agent-session-status-feed.ts).

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

…b the status feed in the delivery test

An exit the adapter observes before acquire returns now fails the acquire
with the CLI's diagnostic instead of handing over a dead child; the
published-then-ended path stays pinned by the slow-init startup case. The
delivery test renders the pane, which now activates the host status feed.
…me, and a replay reopen goes on the idle clock

@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

Since the prior pullfrog review (39234e4), two commits landed — both test-only or housekeeping, no production code:

  • Stray lockfile entry reverted. pnpm-lock.yaml is restored to origin/main, dropping the unrelated @pnpm/exe@12.0.0 addition, so the lockfile no longer appears in the PR diff at all.
  • Pre-handover exit pinned as a failed acquire. claude-structured-session-adapter.test.ts now asserts acquire rejects with the CLI's own diagnostic and that, after drainObservedExits(), the first-hand exit is still reported as an ended event (unexpected-exit, startupUnproven) — matching the publish-first behavior d3e47d7 introduced.
  • Delivery test stubs the status feed. NativeChatStructuredSessionDelivery.test.tsx adds a subscribeStructuredAgentSessionStatus mock (return shape matches structured-agent-session-client.ts:110) that the pane now needs while starting; no existing assertion was weakened.

I ran both changed suites (46 tests pass). Two details I checked rather than took on faith: the new closeCount assertion is >= 1 because the actual count is 2 (the fixture's exit close plus the acquire-failure cleanup close), the same count this suite already documents for first-hand-exit cleanup at claude-structured-session-adapter.test.ts:608 — so it is not masking a double-close; and the slow-init published-then-ended path is still covered by claude-structured-session-startup.test.ts:115,140.

Approval remains withheld because the two earlier threads are open and untouched by these commits: the startup-gate admission race (claude-structured-session-startup-gate.ts) and the revokeLive hostExecutionPhase retention (structured-agent-session-status-feed.ts).

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

…d of being replaced by the CLI's value

A start skips an option write the CLI does not answer within the request
deadline, and then persisted what the CLI reported in its place, so a slow
answer silently replaced the user's saved model or dropped their saved
permission mode. Silence is not a refusal: the unanswered option is now
recorded apart from a rejected one, the live child keeps running on the
CLI's value, and the saved choice stays on the record for the next start to
retry. An option the CLI rejects is still dropped as before.
…y it failed is not folded away

A send whose restarted child died before starting is rejected, and the
exit writes a row naming the cause. The chat's local clock had watched the
send go pending and stop, so it gave the message "Worked for 0s"; that
settled a turn that never ran, and the fold hid every non-prose row after
the message behind it, including the one naming the cause. The row only
appeared when a later send moved the turn anchor, which read as two rows
for one Retry. The host's journal already says the send was rejected; it
now answers that such a message opened no turn, which outranks the local
clock on desktop and mobile alike. A rejected send whose journal does
record a turn keeps its duration.
…row as any start that died

One failed attempt already leaves one row, but which row depended on when
the child died. A child that died after the send was admitted left "The
provider stopped before it finished starting: <cause>."; one that died
before the send was admitted left "Claude couldn't restart: <cause>." So the
same failure read two ways from one Retry to the next. When the refused
restart proved its child exited, the send now writes the startup-failure row
itself, as its comment always said it did. The refusal under the composer
still says the restart failed; a restart that failed for a reason other than
a child exiting keeps its own wording.
…es the cause under the composer

When the child a send was admitted against died before starting, the host
rejected the send with the child's diagnostic behind the internal transport
marker. The client rightly hides that marker's detail, so the red line read
"Couldn't reach the agent" while the cause sat in the record. A startup
death is not a failed write: the host now words that rejection the way the
chat row does, "The provider stopped before it finished starting: <cause>.",
at every site that rejects for it. Desktop and mobile show a reason in words
verbatim already, and older clients do too, so no client change is needed.
Real write failures keep the marker and the generic copy.
…hange to a different option

The saved choice a start could not apply was kept on the record, but the next
option the user set persisted only what the child had applied, so changing the
permission mode or effort, or clearing the chat, silently dropped the saved
model. The adapter now reports which saved options are still unanswered, every
option write keeps those saved values, and a write the child accepts for that
option retires it.
…es that exit's cause

When the child a send was admitted against died starting and its exit
finished settling before the send reached it, the send was rejected with
"no live claude stream-json session for <id>", now shown under the composer
as the cause. The adapter keeps a settled exit's diagnostic until the chat is
acquired or closed again, so that send names what the CLI said. A refused
restart whose child died at spawn or while its start time was read is pinned
to leave one row in the words any failed start uses.
…tarted send with

The startup gate and the dispatch reject a send first in every existing
scenario, so the exit settlement's own rejection had no test of its wording.
…t the child applied

A write that lands already puts its option in the session's applied set, so
the unanswered list is that list minus what has since been applied, rather
than a second copy every option write must remember to edit. Session
fixtures built without the new set no longer throw on an ordinary write.
… never answered

Clearing a chat seeded the replacement from the values the child reported,
so a saved model or effort whose restore write the CLI never answered was
replaced by the CLI's own value in the new chat, even though the retired
record kept it. The replacement now keeps those saved values too, and its
start retries them.
…he exit settles during the close

The diagnostic was dropped when the close began, but closing over an exit
that was still settling finishes that settlement, which kept it again, so a
closed or deleted chat held it until its next acquire. It is now dropped once
the close finishes. Pins that an acquire and a close each retire it.
…anted value, not a list beside it

A restore cleared the session's wanted options and added back only the writes
the CLI answered, so an unanswered one lost the user's value and every later
writer had to be told to put it back: the start report, each option change and
/clear each carried a list of unanswered keys. The restore now keeps the saved
value as wanted and unconfirmed, so what the session reports and persists
already carries it, and the list, its adapter method and the started-event
field are gone. A refused option is still dropped.

/clear now starts the replacement from the record's options instead of reading
the child's live values, which can be a model the CLI fell back to.
Main resumes a Claude chat by session id alone and removes Claude rewind; the
stack publishes a Claude start at spawn and resumes a chat whose first start
failed as the same conversation. Both hold in the result:

- claude-structured-launch-resolution.ts: keep the stack's transcript check; a resume passes only the session id (no resumeSessionAt), and the inherited env drops CLAUDE_CONFIG_DIR.
- claude-structured-session-acquisition.ts: drop the rewind observer and proof-before-publish path; every start publishes at spawn and reads its facts after publish; publication carries main's turn-end leaf.
- claude-structured-session-publication.ts: keep the stack's providerSessionId input; drop claudeConfigDir, which the session no longer holds.
- claude-structured-session-test-support.ts: keep the stack's requestTimeoutMs; drop the removed readTranscriptLeaf parameter.
- claude-structured-rewind.ts and its test: deleted as on main; the stack's resumed -> resumesTranscript rename there is moot.
- structured-rewind-claude-owner.ts: deleted as on main; the stack's serialized-attach call there is moot.
- structured-agent-session-acquisition.ts: keep the stack's providerChildPhase result; drop the rewind proofs.
- structured-agent-session-attach-flow.ts: keep the stack's failed-create refusal; drop its rewind-refusal branch.
- structured-agent-session-attach-orchestration.ts: keep the stack's serialized attach; drop the rewind option so a pending rewind is always settled on attach.

Main's new tests use the stack's launch fields and runtime lifecycle hook; the
pending-rewind test seeds the transcript a resume continues.
…model

The record holds the saved model from creation, so waiting for it returned at
once and the option write could reach Claude while it was still starting,
which refuses it. Wait for the effort the finished start reports instead.
The notice that a structured chat is still starting was only in English.
The merge re-pointed this test at a child that exits after publish, where the
exit path also releases the binding, so it passed with the acquisition's release
deleted. A child that exits before publish leaves only that release. Also drops
the create 'init' phase, which lost its last producer when rewind stopped
proving before publish.
Brings main's re-pin of the mobile session route closure (4219) with its
revert of #18790; no conflicts.
Conflicts:
- claude-agent-sdk-control-requests.ts: kept this branch's untimed initialize and added
  main's get_context_usage request beside it.
- claude-structured-session-integration.test.ts: main moved the fake Claude into
  claude-structured-fake-connection-test-fixture.ts; this branch's one fixture change (close
  reports success once the child's tree has exited) now lives there.
- fr.json, and ja/ko/zh.json which auto-merged into two sibling "native" objects: this
  branch's still-starting string is folded into main's native.chat block.

Also gives the scripted test runtime main's new getContextUsage request.

@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 in this delta.

Reviewed changes

Since the prior pullfrog review (0ddf885), the branch fast-forwarded onto #22359's structured-send-recovery tip and then merged origin/main:

  • Folded #22359's send recovery into the branch. The host now restarts a gone owner inside the send's own serialized admission (ensureProviderChild), the Claude adapter holds a message sent against an unproven child and rejects it with the child's diagnostic if it dies first, a failed restart refuses agent_session_owner_restart_failed with its cause and a single status row, and an unheld owner idles for 30 minutes with activity renewing the clock.
  • Merged origin/main (1985c1b11e). Reconciled main's new context-usage feature (getContextUsage, recordsContextUsage, AgentSessionContextUsage, bindClaudeConnectionJournalControls, contextUsage on the options result) with the branch's publish-first startup, lifecycle-event split, hostExecutionPhase, and rejectPendingSubmissions. The 26 conflict-resolved files are a clean union — no PR-authored line was dropped, and the branch's unanswered-option skip survives on top of main's heldBeforeRestore/CONTEXT_WINDOW_KEYS.
  • Retired two earlier threads. The recovery test now waits for the stable reserved lease (with the spawn hanging) instead of the transient released it skipped past, and holdClaudeStartupWrite re-drains a prompt queued after the gate already drained.

Approval is withheld on the four earlier threads this delta does not touch.

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

Conflict: structured-agent-session-dead-generation-settlement.test.ts, where main (#22532)
and this branch each added a test at the same spot. Both tests are kept whole, with this
branch's import and journal-stub changes.

@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 in this delta.

Reviewed changes

Since the prior pullfrog review (b1e91e4), the branch merged origin/main (98584332) and resolved one conflict:

  • Main's Codex journal-producer work and ipynb-venv work came in. 98584332 (#22532, record which Codex agent produced each journal row) and 5610b117 (#22710, create a .venv when pip is locked out) are now reachable from the branch; both are already on main, so the PR diff against the new base excludes them.
  • Both tests kept in structured-agent-session-dead-generation-settlement.test.ts. Main's keeps a subagent's settled rows the subagent's test and this branch's rejects a send a child that never started left pending test were added at the same spot; the resolution keeps both whole and retains the branch's dispatchRejectionReasonIsInternal import and rejectPendingSubmissions journal stub.
  • No PR-authored production change. The range-diff isolates the delta to that one test file, and git diff b1e91e4 c2d0b33 -- structured-agent-session-dead-generation-settlement.ts is empty.

I verified the merged suite (10 tests), pnpm tc, and the broader agent-session-journal + codex suites (214 passed, 4 skipped) green. The one failing suite, codex-app-server-teardown.integration.test.ts, is not in the PR diff and fails in this environment on real-process descendant reaping, so it is unrelated to this delta.

Approval is withheld on the four earlier threads this delta does not touch: the stale started-on-shared-chain comment (structured-agent-session-runtime-provider-started.test.ts:35), the revokeLive phase retention (structured-agent-session-status-feed.ts:179), the startup-options snapshot overwriting a later setOption (structured-agent-session-provider-started.ts:71-80), and the release-clock rationale comment (structured-agent-session-release-clock.ts:11-12).

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

…le module

The adapter crossed the 300-line limit once main's context-usage change
landed beside this branch's growth. The two methods that turn a Claude
process exit into an ended event now live next to the existing exit
helpers; behavior is unchanged.

@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 in this delta.

Reviewed changes

Since the prior pullfrog review (c2d0b33), one commit landed — a behavior-preserving refactor with no new behavior:

  • Claude unexpected-exit handling moved into the exit-lifecycle module. handleExit and the private settleUnexpectedExit are extracted from ClaudeStructuredSessionAdapter into claude-structured-session-exit-lifecycle.ts as observeClaudeSessionExit / settleClaudeUnexpectedExit, and the adapter's acquire and releaseAcquisition callbacks now call those module functions.
  • One shared ClaudeExitLifecycle object carries the state. It bundles the adapter's sessions / exits / settledExitErrors maps, a Pick<ClaudeStructuredSessionAdapterDeps, 'persistHandle' | 'now'> deps slice, and the emit callback, built once in the adapter's constructor.

I read the extraction line-for-line against the code it replaced: the map identities, the emit callback, the deps slice, the startupFailed && claudeRootExitObserved failed-start bar, the settledExitErrors write, and the unexpected-exit ended event are all unchanged. pnpm tc:node passes, and the claude-structured-session-adapter, -session-startup, and -session-recovery suites pass (58 tests).

Approval is withheld only because the four earlier threads remain open and are untouched by this refactor: the stale started-on-shared-chain comment, the revokeLive hostExecutionPhase retention, the startup-options snapshot overwriting a later setOption, and the release-clock rationale comment.

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

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