Repository navigation
Offer to reconnect native chats that were working when Orca restarted - #21096
Merged
Merged
Conversation
Teardown records a marker for every session this host was genuinely running a turn for, derived from the LIVE runtime rather than a persisted status row, so a stale `running` row left by an older crash can never trigger a resume. On the next launch a modal lists exactly which chats would resume and resumes them via native continuation (Claude resume/resumeSessionAt, Codex thread id) — never by re-sending the prompt, which is what makes an agent redo finished work. A session resumes only when all of these hold: a teardown marker exists and has not expired, the record's lease is released and reconciled, a provider resume cursor exists and still matches the marker, the journal's own turn record names the same turn, and the marker has not already been spent. Markers are consumed before the resume is submitted, so a crash mid-resume cannot double-fire, and an admission gate refuses a second concurrent resume for one session. Resumes are staggered three at a time rather than spawning every provider at once. The modal's "Don't ask again" checkbox writes the nativeChatResumeWorkOnRestart setting, which Settings can turn back off; automatic mode runs the identical predicate and staggering and reports what it did. Declining consumes the markers so the prompt cannot return every launch — nothing is lost, because opening a chat still re-acquires it at the same cursor.
…resume Four defects QA found in the restart-resume offer, fixed together because the first two interact: shipping the root fix without the state fix would convert a silent no-op into actively offering finished chats. 1. Claude was never offered (0/4). The marker recorded agentSessionProviderHandleKey, which embeds Claude's leaf uuid — a branch cursor. The adapter's own close path appends a `resumed` link with an advanced leaf during the SAME teardown, so the marker went stale seconds after it was written and the drift guard refused every Claude session forever. Record and compare agentSessionProviderHandleRoot instead: the root is the part a resume must preserve, and changing it is a fork, which is exactly what this guard is for. Codex is unaffected (its thread id is the whole key) but uses the root too, so the rule is uniform. 2. The predicate compared turn IDENTITY but discarded turn STATE, so a `completed` turn satisfied it as readily as an interrupted one. Eviction rewrites `running` to `interrupted` and never to `completed`, so the state is what separates work that was cut off from work that finished. Require `interrupted` or `unverifiable`. 3. A chat blocked on a pending approval or question was marked as working, because the teardown reader accepted any `running` turn while the product's own projection calls that state `attention`. Teardown now defers to that projection: an agent waiting on the USER is not interrupted work. 4. "Resume all" could silently no-op. The modal fetched candidates at mount; by click time the chat's own pane may have bound and taken the hold, moving the lease to `live` so the predicate dropped it and the call returned no results, leaving the dialog open behind a dead button. Re-derive at click time and settle an already-live session as resumed — it is running, which is what the user asked for. Test fakes now model the Claude close path that advances the leaf, which is why no unit test could previously exhibit defect 1. Ablation covers all eleven guards.
… predicate Two follow-ups from re-QA, both cases of a rule stated by intent rather than by discriminator. 1. The already-live path bypassed the predicate. "Resume all" sends no session ids, so the fallback's target set was every marker, and it was gated only on the session having a live provider child. A chat the predicate had refused -- a completed turn, say -- whose pane happened to own the lease was therefore settled as `already_live` and had its marker spent, inflating the "Resumed N" count with chats that were never eligible. No provider spawned and no tokens were spent, but a marker the predicate rejected must never be consumed. The resumable set now takes an explicit `leaseState`. The already-live path derives a second set with ONLY the released-lease clause relaxed, and settles a session just when it is in that set. Every other clause still applies. 2. The `attention` rule was one-sided. Teardown refuses to mint a marker for a chat blocked on the user, but the set predicate had no equivalent, so a marker arriving by any other route was offered once eviction rewrote its turn to `interrupted` -- the same asymmetry the completed-turn case had. Gated on projectStructuredAgentSessionStatus === 'attention'. That projection tests for a pending approval or question BEFORE it looks at turn state, so it still reports `attention` after the turn is settled, which makes it the durable signal and keeps one source of truth with teardown. Ablation now covers thirteen guards, including one for each of the above.
…iving it
The awaits-user clause could never fire. It asked the live projection for
`attention`, which needs a prompt whose resolution is still `pending` -- but
teardown CANCELS that prompt a few phases after it writes the marker. By the next
launch the evidence is gone, for precisely the sessions the clause was written
for. QA measured the injection still being offered and then resumed.
This is the same shape as the leaf-drift bug: state read after teardown is not the
state that justified the marker. The discriminator, now applied across the whole
predicate:
- a fact teardown itself destroys or mutates must be CAPTURED on the marker
while it is still true;
- a fact that evolves on its own must be RE-DERIVED at read time, never
snapshotted.
So `awaitsUser` is now recorded at teardown and the predicate reads the recorded
value. Teardown still declines to mint a marker for such a session, so the
recorded flag is the second line rather than the only one.
Audit of every other clause against the same test:
- turn id (captured) -- teardown rewrites turn STATE but never the id. Correct.
- provider handle root (captured) -- the close path appends a resumed link, and
appendAgentSessionProviderHandleLink refuses one that changes the root, so the
root is invariant under exactly the mutation that broke the key. Correct.
- turn state (re-derived) -- DELIBERATE exception, stated here rather than left
implicit: we are not reading the state that justified the marker, we are
reading teardown's receipt that it settled the turn. A turn still `running`
means eviction never finished, and we refuse. Correct, and intentionally so.
- lease reconciled / released / handoff stage (re-derived) -- these answer a
different, launch-time question: may this host take the lease NOW. The
teardown-time value would be meaningless, and `unreconciled` is cleared by
this launch's own reconciliation. Correct.
- adapter support, marker TTL, marker consumption (re-derived) -- all evolve
independently of teardown. Correct.
Only awaitsUser was on the wrong side.
The captured flag was dead code. `awaitsUser` could only be true when the projected status was `attention`, and `attention` hits the `continue` above the push -- so every marker teardown can ever write carries `false` (QA measured 22 of 22 across two real teardowns). The predicate clause reading it was unreachable by any production path. A flag that is structurally always false is worse than no flag: it reads as a safeguard, so the next person to touch this trusts it. The asymmetry it was added to close was only ever reachable by fault injection, because teardown is the sole writer of markers and already refuses attention sessions. Removing it also drops an upgrade discontinuity: as a required field it made a marker written by the previous build fail validation and be silently discarded, costing a resume offer on precisely the upgrade where the user was mid-turn. Markers predating the providerHandleRoot rename still will not parse, but those carry a leaf-sensitive key the predicate would refuse anyway, so nothing usable is lost. In its place the teardown gate now states that `status !== 'working'` is the SINGLE gate for awaiting-user sessions, why a predicate-side mirror would be unreachable, and why it could not even re-derive the fact -- so the reasoning is inherited rather than rediscovered. Ablation is back to twelve guards; every other clause is unchanged.
Two changes, both independent of the parked continuation decision.
1. The copy claimed something QA disproved. "Resuming continues each agent where
it left off" is false: reconnection restores the session at the point it
stopped, with full context and without re-sending the prompt, but the
interrupted reply does not continue on its own. The toast's "Resumed N chats"
implied work had restarted.
Audited every user-facing string against the rule that none may claim work
continues or that a reply resumes -- which caught more than the three strings
the fix started from. The title, the row button, "Resume all", "Resuming...",
the not-now hint ("picks it up where it left off"), the checkbox and its hint
("resume on their own"), the list's aria-label and the Settings row all made
the same claim. The user-facing verb is now reconnect throughout; the body and
update variant state outright that the interrupted reply will not continue.
en.json synced, runtime boot catalog regenerated.
If we later decide to send a continuation instruction, this is one commit to
change back. Shipping text we know to be false was the worse option.
2. Rows now show each offer's age. The TTL is 24 hours and a stale offer looked
identical to a fresh one. The marker already carried `recordedAt`, so this is
a render change plus one field on the renderer's candidate type, formatted
with the existing formatUiRelativeTime helper rather than a new one.
The clock is stamped once when the list arrives rather than read during render:
ages then stay stable across re-renders, and the render stays pure, which the
react(purity) rule requires.
Guards, predicate and RPC are untouched; ablation still covers twelve.
A row read `codex · folder:8f3a1c22-… · 8 hours ago`. Recognising which chats would reconnect is the entire point of the list, and at twenty rows a UUID identifies nothing. No RPC or host change was needed: the renderer can already resolve this id. Resolved the way automation dispatch resolves the same id space (resolveAutomationDispatchWorkspace) -- a folder workspace by its full `folder:<uuid>` key via getKnownWorktreeById, a git worktree by its bare `repoId::path` id via allWorktrees. Both return a Worktree, whose displayName is a required field, and DetectedWorktree extends Worktree so either shape answers. Falls back to the id when nothing resolves, which is what the row showed before and also covers the window before the worktree store has hydrated. The lookup lives in a per-row subcomponent because a hook cannot run inside `map`, and its selector returns a primitive string so repeated selector runs cannot churn referential equality.
…in continuation
Grouping. Rows are now grouped under a worktree heading with the repo glyph and
an agent count, using the sidebar's own collapse mechanics. Only presentational
pieces are reused -- RepoIconGlyph, CompactAgentExpansion, AgentIcon and
formatShortTimeAgo. The sidebar's agent row cannot be: worktree-card-compact-agent-row
imports DashboardAgentRow, the dashboard's own type, so both surfaces render one
live-agent model requiring a pane, tab and status entry. Every chat offered here
is by definition stopped, so supplying that would mean inventing live state.
Two things I had assumed were reusable and were not:
- DashboardHostBadge returns null unless hostKind is ssh or remote. Structured
chat is local-only, so it would always render nothing. The host line is
omitted rather than faked; the badge is the right element to add if and when
structured chat gains remote support.
- No state dot. Every AgentDotState misleads here: idle and unverifiable both
presuppose a live pane, interrupted renders red like an error, done green,
working a spinner. A missing dot beats one saying these agents are running.
One worktree renders flat with no heading -- a name, count and chevron around a
single group says nothing the dialog has not already said.
The age column now uses formatShortTimeAgo for sidebar consistency. It takes
(timestamp, now) and subtracts internally rather than taking a delta, so the call
is (recordedAt, listedAt); passing the old delta would have rendered plausible
nonsense. The clock is still stamped once into state, so ages stay stable and the
render stays pure.
Continuation. A secondary "Reconnect and continue" action sends one message, from
a single shared constant, identical for both providers. Reconnect is unchanged and
still sends nothing. An info popover quotes the literal message read from that
same constant, so what is shown cannot drift from what is sent.
Ablation now covers fourteen guards. Two are new: continuation only follows a
reconnect that actually happened, and -- inversely -- a send injected into the
reconnect path must turn the test red, since "don't ask again" rests on reconnect
never sending.
…quality gate
The modal lists stopped chats with no way to tell that CLI agents are fine, and
the true state of the world is counterintuitive: the terminal sessions survived
the restart and the chats did not. One line now says so, next to the heading
where it frames the list rather than as a footnote at the bottom.
Wording follows the app's own vocabulary rather than inventing a term: the
catalog settles on "terminal sessions" (terminalSessionCount, "Terminal sessions
are grouped by workspace", "No terminal sessions yet"), and UpdateCard already
reassures with "Your terminal sessions won't be interrupted during the update" in
the same text-xs text-muted-foreground treatment. "kept running" rather than
"were restored" -- nothing reconnected them, they never stopped, and the line
says nothing about why.
Also clears check:code-quality:changed, which I had not been running -- oxlint
alone covers neither the design-system nor the casting audit, so 18 findings had
accumulated across the branch.
- design system (4): Button spacing hand-rolled as gap-1/px-2 is just size="xs";
PopoverContent and DialogTitle own their typography and spacing, so the
text-xs moved to the popover's own children and the title's icon gap moved to
a plain wrapper.
- casting (14): production code loses its assertions outright via Reflect.get,
the idiom already used in managed-hook-detection-commands and
worktree-name-retirement. The marker validator reads each field through
Reflect.get and now checks recordedAt is a number rather than asserting it;
the store-file parse uses the existing `file` shape instead of a second
assertion; the runner narrows the admission error's owner with typeof.
Test fixtures keep their assertions behind the line-specific SAFETY:
rationale the repo mandates for exactly this case.
One trap worth recording: the audit reports an assertion at the line its
EXPRESSION OPENS, not where `as` appears, so a disable-next-line above the
closing brace of a multi-line literal is inert and silently changes nothing.
Guards unchanged; ablation re-proved 14/14 at this head.
…e name Every row rendered the provider as a bare AgentIcon, whose svg carries no aria-label, title or alt. With a Claude chat and a Codex chat in one worktree the two rows were identical to any non-visual consumer, and the dialog offered several identically-named "Reconnect" buttons with nothing to tell them apart. A regression from 233e37b, where the row read `${agent} · ${workspace} · …` as text. Moving the workspace name into the group heading was right; dropping the provider to an unlabelled glyph is what lost the information. AgentIcon takes no label prop, so the icon is wrapped the way NativeChatSupportedAgents already names it: a span with role="img" and an aria-label from formatAgentTypeLabel, the same labeller the sidebar and dashboard rows use. The per-row button also names its agent now ("Reconnect Claude chat"). The identical buttons were half the reported harm, and an accessible name that opens with the visible word keeps WCAG 2.5.3 satisfied. Say so if you would rather ship only the icon label -- it is one attribute and one catalog key to drop. Age code untouched, as asked: formatShortTimeAgo still takes (timestamp, now) and is still called with (recordedAt, listedAt).
…e-on-restart # Conflicts: # src/renderer/src/i18n/en-runtime-required.json # src/shared/structured-agent-session-live-turn.ts
…eal dispatch Three defects in the restart-resume path, all of which could resume a session that was not genuinely working or claim one was continued when it was not. Launch scoping. A durable marker with a 24h TTL is a write-ahead latch: a teardown write that failed or timed out, or a store restored from its backup, left a previous generation's marker actionable, and automatic reconnect would have acted on it silently. Markers now carry the id of the launch that wrote them, and only the launch immediately after may claim them. The launch id lives in its own file with no backup mechanism, so it cannot roll back in step with the markers it is proving adjacency for. Startup claims the previous launch's markers into launch-scoped memory and deletes every durable copy in the same step, so the durable fact dies at claim time rather than at use time. Both halves fail closed: an unprovable predecessor and a clear that throws each claim nothing. Dispatch states. The send layer answers ok as soon as Orca owns the message; the provider's own answer lives in the submission. Continuation read only the envelope, so a rejected turn/start was reported as continued and stamped the journal saying the agent had been asked to carry on. All four states are now preserved, and only an accepted dispatch appends the attribution note. Claude pre-echo sends. Claude cannot write a running turn until the SDK echoes the user message back, which is seconds on a real journal, so a turn-id-only marker dropped exactly the sessions that were working hardest. A send that has not become a turn now carries its own identity, and the launch-side predicate asks the journal about that submission's dispatch state instead.
…ore judging Two defects found in QA, both reproduced twice. Follow the submission forward. The launch-side predicate accepted a submission-shaped marker only while its dispatch was pending or unknown, but the window in which work is submission-shaped is precisely the window in which the dispatch is about to be accepted: the send settles during teardown and the turn it opened is then cut off as interrupted. Judgement was frozen at the moment the marker was written, so the predicate refused the very sessions this was built for and fired only when the send never reached the provider. An accepted submission is now followed to the turn it opened -- matched through the user item key a turn names and a submission is aliased by -- and that turn is judged by the existing turn rule. Accepted alone still proves nothing: without the link, or with a turn that completed, this refuses as before. Settle before judging. A send resolves as soon as Orca owns the message, while its dispatch is still pending; that is the ordinary successful path. Reading the dispatch off the send result therefore reported every delivered continuation as pending and never wrote the attribution note. The outcome is now decided on the settled submission, through the host's existing settlement waiter, with the send result as fallback when nothing settles in time. The failed-note path no longer swallows its error. It stays best effort -- a journal that refuses the note must not turn a delivered continuation into a failure -- but the failure is reported through the host's error sink instead of being discarded, so it cannot regress unseen again. The surface's send is typed against the wire result rather than a hand-written subset, which is what let a test assert a shape the host never returns. Binding the surface to the host moves into its own file: the host was one line under the line cap, and the bindings carry decisions that belong beside their consumer.
…bar does The offer is a list of workspaces, so it should read like the one users already know. Rows are now three tiers -- repo or project, then workspace, then the agent sessions inside it -- and each agent carries a checkbox rather than its own button, checked by default, with the footer acting on whatever is ticked. Reused rather than rebuilt. The host chip is the sidebar's own: its markup lived inline in the card's meta row, so it moves to a shared component both surfaces render, and the label comes from getHostContextLabel, which is where "Local Mac" has always come from. The repo glyph is RepoIconGlyph; a group with no repo uses the FolderTree the sidebar's own project-group metadata uses. The agent row reuses AgentIcon, the agent-type label helpers, formatShortTimeAgo and the same model treatment. Two things could NOT be reused, and both are deliberate. The sidebar's CompactAgentRow needs a live pane, tab and status entry, and every chat here is stopped by definition. And the sidebar has no git-worktree-vs-folder glyph resolver at all -- both kinds render the same card, and the difference people read is its status lane choosing GitBranch when a workspace has branch identity; that single precedent is what the workspace glyph follows. The model, the execution host and the workspace kind now travel with each offered chat. All three are read off the durable record the predicate already holds -- the model through the same normalizer the status feed uses -- so the glyph is never inferred from a name and no new data source appears. They are optional on the wire, so an older host still renders a row. Selection changes which ELIGIBLE chats are acted on, never what is eligible. Ids are seeded from the host's own answer and intersected back against it before any call, and the host re-derives the predicate regardless of what it is sent. Continuing still requires an explicit click, and the automatic path still calls the reconnect method, which contains no send. The badge's treatment becomes a variant instead of a pile of overrides, which is what the design-system gate asks for once the markup is somewhere it can see it.
A folder workspace's synthetic worktree borrows the `repoId` slot to name the project group it belongs to, so that field is NEVER null. The reconnect offer read a non-null `repoId` as proof of a git repo, looked it up in the repos list, found nothing, and rendered the raw `folder-workspace:<uuid>` string as the group header. The project glyph written for the no-repo case was unreachable for the one workspace kind it was meant for, and the string fallback behind it was dead for the same reason. The project group name was available all along and the sidebar already titles these with it, which is what this list is meant to mirror. Recognising the id now lives beside the code that mints it, so the two cannot drift: there was no such helper, only forward constructions of the same prefix in five places. The header choice itself moved into a pure resolver, so the branch that was wrong is now the branch under test. The dead fallback string is gone, along with its catalog entries.
…turn for QA: a chat that was genuinely working was silently dropped from the offer. The discriminator was how far the send had progressed -- it was the last chat prompted before quitting, reachable by quitting a second or two after sending. Mechanism, reproduced against the predicate. The marker was written while the send was still pending, so it is submission-shaped. During teardown the dispatch then settled to `accepted`, which took it out of the pending/unknown branch and into the follow-forward branch. But the provider died before writing a turn row for that send, so there was no turn to follow forward TO, and the branch demanded a proved link before it would answer. Both the no-turn-at-all case and the newest-turn-belongs-to-an-earlier-exchange case therefore refused. An accepted send that never became a turn cannot be finished work, because finishing writes a turn row. The marked send is also the newest work in the session, so any turn it opened would be the newest turn. That makes the link unnecessary to prove for a safe answer. When the newest turn is interrupted or unverifiable the two readings agree: if the row really is this send's under a key we failed to match, it was cut off; if it belongs to an earlier exchange, this send opened no turn at all. Either way the work was interrupted. A journal with no turn row at all is the same case with nothing to disagree about. The readings only diverge on a `completed` row, where an unmatched one might be this very send's finished turn under a key we did not recognise. That stays refused. Ambiguity resolves to no, because resuming finished work is the one outcome never worth risking.
Five separators in the reconnect-offer redesign were written as raw NUL bytes instead of the `\0` escape. The runtime strings were correct and the app behaved, but git classifies a file containing a NUL as binary -- so the two central files of that redesign rendered as "Binary file not shown" in review, and `rg` skipped them silently, returning no matches rather than an error. The escape produces the identical string, so the NUL separator is kept: the previous separator was a space, and a workspace id containing one would corrupt the join/split pair this grouping depends on. Nothing could have caught this. Typecheck, lint, the quality gate, the localization verifiers and the full suite all passed throughout, because none of them look at file encoding. So this adds a check that does, wired into the pre-commit hook where it costs nothing and catches the next one at the moment it is written. Two files already on main carry a raw NUL for the same reason -- one a template separator, one a deliberately tricky test alphabet whose neighbours are all written as escapes. They are grandfathered rather than fixed here, since they belong to their own change, and the gate fails if the list ever grows or goes stale.
…e-on-restart # Conflicts: # src/main/runtime/agent-session-claim-key-retention.ts # src/main/runtime/agent-session-record-store.ts
brennanb2025
marked this pull request as ready for review
September 17, 2026 02:07
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Important
Two issues worth addressing before merge: the auto-reconnect preference can reconnect chats the user explicitly unchecked, and the already-live settlement branch in the resume path does not enforce consume-once.
Reviewed changes
- Durable resume markers — teardown writes a marker (work identity, provider-handle root, launch id, TTL) for every session with a live provider child and a working projection, stored beside the records in one whole-file transaction.
- Launch-generation adjacency — a new backup-less
agent-session-launch.jsonis rotated once per launch; only markers stamped by the immediately preceding launch are claimed, andclaimMarkersclears every durable marker in the same step. - Eligibility predicate — a marker and the journal's own turn/submission state must independently agree the work was cut off; lease, cursor-root, and expiry checks all fail closed.
- Resume runner — consumes the marker before taking a resume-capable hold, behind per-session admission and a concurrency of 3.
- Continuation path — a separate
agentSession.restartContinuesends one fixed message only on explicit click, judged from the settled dispatch state; the automaticrestartResumepath contains no send. - RPC + renderer surface — four new
agentSession.*methods, a grouped reconnect modal, a settings toggle, and a sharedhostContextbadge variant extracted from the sidebar meta row. - Supporting changes — teardown phase ordering/triggers, a store read/write file split, and a new
check:no-nul-bytesgate.
ℹ️ The launch-adjacency clock advances on host install, not on app launch
rotateAgentSessionLaunchGeneration has exactly one production caller — install() in structured-agent-session-runtime.ts:198. A launch that never constructs the structured host (setting off with no persisted-store startup restore, or a degraded/renderer-failed startup) leaves both the launch id file and the durable markers untouched. The next launch that does install then reads the older id and computes previous as the launch before it, so markers written two app launches back can become claimable again — the exact case the tests pin as refused. Impact is bounded (24h TTL, and those sessions were genuinely working at the teardown that wrote them), but the documented invariant "only the launch immediately after it may act on it" is not what the wiring enforces.
Technical details
# Launch id is coupled to runtime install
## Affected sites
- `src/main/runtime/structured-agent-session-runtime.ts:198` — the only `rotateAgentSessionLaunchGeneration` call, inside `install()`.
- `src/main/runtime/agent-session-launch-generation.ts:18-20` — documents rotation as once per launch and rollback as under-offer only.
- `src/main/native-chat/agent-session-wire/structured-agent-session-restart-resume-host.ts:127-136` — filters by `launchId === launch.previous`.
## Sequence
1. L1 installs the host (id A), quits, writes marker M stamped `launchId: A`.
2. L2 never reaches `install()`; the id file still holds A and M stays durable because claim is lazy and host-gated.
3. L3 installs: reads A, computes `previous = A`, claims M. M is two app launches old but reported adjacent.
## Required outcome
The adjacency clock should advance per app launch (e.g. rotate at startup, outside the host install),
or the marker claim should additionally verify that the generating launch was the immediately
preceding app launch, not merely the preceding install.
## Open questions for the human
Is "launch" here intended to mean "app launch" or "structured-host install"? The comments and tests
say app launch; the wiring says install.DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
…ethods Two CI failures, both ours. Static analysis. `Reflect.get` was adopted to clear the casting audit, and the anti-slop rule forbids it -- the two gates disagree, and the rule text says what both want: parse dynamic input into a named type once, then read typed fields off it. Markers re-enter from a file this process may not have written and decide whether an agent is handed a provider child, so they now go through a single zod parse. Unknown keys still pass, and a malformed marker is still dropped rather than thrown, so a bad entry cannot make a user's sessions unreadable. The launch stamp is parsed the same way, the resume-admission refusal becomes a named error carrying a typed `owner` instead of a bag assigned onto `new Error`, and the test harness gets a named journal type instead of reaching into `unknown`. Cross-version wire. The four restart methods are added to the manifest rather than the count being bumped, so the suite now exercises them in both skews. They are bare additions, not capability-negotiated: an unknown RPC method answers `method_not_found`, which is explicit and visible during negotiation, unlike a stream opcode that is dropped in silence. The whole `agentSession.*` surface already sits behind its runtime capability, so an old client is told it does not exist and never reaches a host method. The stub's spies stay a flat map because callers iterate it asserting each entry is a spy that did not run; a composer reassembles the member the host really exposes. The manifest and its params builders move to their own module, which is what keeps the suite under its line cap as the surface grows.
Contributor
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change adds restart recovery for structured native-chat sessions. Teardown records working sessions in a durable recovery capsule. Startup derives eligible candidates, consumes markers, and resumes sessions with bounded concurrency. Users can reconnect or continue sessions through new RPC methods and a renderer modal. Settings, notifications, translations, workspace grouping, and cross-version test fixtures are also updated. Priority: ➖ Normal Merge Risk: 🟡 Moderate · up to Restart recovery can silently omit interrupted chats, and a recovery failure during shutdown can leave cleanup incomplete. Address these paths before merging; baseline-cleanup and platform-test risks also remain unresolved. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 37.25% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 102 functions across 59 files. (2 skipped: 2 unsupported.) Full details: Description checkExplanation The description covers the user impact, implementation, rationale, visual proof, testing, platform scope, and checklist. However, the required Linked Issue section contains “N/A”, although the template states that a linked issue must always be provided. ✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: b59bc1d9-0753-480e-a199-6f2e28387845
⛔ Files ignored due to path filters (1)
src/shared/rpc-contract/rpc-params-catalog.generated.tsis excluded by!**/*.generated.*
📒 Files selected for processing (57)
config/nul-byte-baseline.txtconfig/scripts/check-no-nul-bytes.mjspackage.jsonsrc/main/native-chat/agent-session-wire/structured-agent-session-host-teardown.tssrc/main/native-chat/agent-session-wire/structured-agent-session-host-types.tssrc/main/native-chat/agent-session-wire/structured-agent-session-host.tssrc/main/native-chat/agent-session-wire/structured-agent-session-restart-claim.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-restart-continuation.tssrc/main/native-chat/agent-session-wire/structured-agent-session-restart-resume-host.tssrc/main/native-chat/agent-session-wire/structured-agent-session-restart-resume-runner.tssrc/main/native-chat/agent-session-wire/structured-agent-session-restart-resume-set.tssrc/main/native-chat/agent-session-wire/structured-agent-session-restart-resume-test-harness.tssrc/main/native-chat/agent-session-wire/structured-agent-session-restart-resume-wiring.tssrc/main/native-chat/agent-session-wire/structured-agent-session-restart-resume.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-teardown-handoff-drain.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-working-at-teardown.tssrc/main/runtime/agent-session-launch-generation.test.tssrc/main/runtime/agent-session-launch-generation.tssrc/main/runtime/agent-session-operation-admission.tssrc/main/runtime/agent-session-record-store-file.tssrc/main/runtime/agent-session-record-store-write.tssrc/main/runtime/agent-session-record-store.tssrc/main/runtime/agent-session-recovery-publish-fault.test.tssrc/main/runtime/agent-session-reservation-admission.test.tssrc/main/runtime/agent-session-resume-marker-file.tssrc/main/runtime/agent-session-resume-marker-store.test.tssrc/main/runtime/agent-session-resume-marker-store.tssrc/main/runtime/agent-session-store-serialization.tssrc/main/runtime/agent-session-store-transaction-queue.tssrc/main/runtime/rpc/methods/structured-agent-session-restart-resume.tssrc/main/runtime/rpc/methods/structured-agent-session-schemas.tssrc/main/runtime/rpc/methods/structured-agent-session.test.tssrc/main/runtime/rpc/methods/structured-agent-session.tssrc/main/runtime/structured-agent-session-runtime-teardown.tssrc/main/runtime/structured-agent-session-runtime.tssrc/main/startup/main-process-quit.tssrc/renderer/src/app-shell/AppRootSurfaces.tsxsrc/renderer/src/components/NativeChatResumeOnRestartAgentRow.tsxsrc/renderer/src/components/NativeChatResumeOnRestartGroups.tsxsrc/renderer/src/components/NativeChatResumeOnRestartModal.tsxsrc/renderer/src/components/native-chat-resume-on-restart-grouping.test.tssrc/renderer/src/components/native-chat-resume-on-restart-grouping.tssrc/renderer/src/components/settings/NativeChatExperimentalSetting.tsxsrc/renderer/src/components/sidebar/WorktreeHostContextBadge.tsxsrc/renderer/src/components/sidebar/worktree-card-meta-row.tsxsrc/renderer/src/components/ui/badge.tsxsrc/renderer/src/i18n/en-runtime-required.jsonsrc/renderer/src/i18n/locales/en.jsonsrc/shared/agent-session-restart-continuation.tssrc/shared/agent-session-resume-marker.tssrc/shared/default-global-settings.tssrc/shared/folder-workspace-worktree.test.tssrc/shared/folder-workspace-worktree.tssrc/shared/global-settings-types.tssrc/shared/rpc-contract/structured-agent-session-params.tssrc/shared/structured-agent-session-live-turn.tssrc/shared/structured-agent-session-projection.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.
Comment on lines
+87
to
+93
| const 'journal') | ||
| if (target !== null && typeof 'object') { | ||
| Reflect.set(target, 'appendItem', async (_envelope: unknown, body: unknown) => { | ||
| if (input.noteFails) { | ||
| throw new Error('journal refused the note') | ||
| } | ||
| noted.push({ sessionId, text: String(Reflect.get(body ?? {}, 'text')) }) |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Replace Reflect.get to clear the lint failure.
The static analysis gate reports anti-slop(no-reflect-get) at Line 87 and Line 93. Both sites read a known shape, so typed access works.
♻️ Proposed fix
for (const [sessionId, session] of sessions) {
- const 'journal')
+ const target: { appendItem?: unknown } | null = session.journal as {
+ appendItem?: unknown
+ } | null
if (target !== null && typeof 'object') {
Reflect.set(target, 'appendItem', async (_envelope: unknown, body: unknown) => {
if (input.noteFails) {
throw new Error('journal refused the note')
}
- noted.push({ sessionId, text: String(Reflect.get(body ?? {}, 'text')) })
+ const note = (body ?? {}) as { text?: unknown }
+ noted.push({ sessionId, text: String(note.text) })
})
}
}📝 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
| const target = Reflect.get(session, 'journal') | |
| if (target !== null && typeof target === 'object') { | |
| Reflect.set(target, 'appendItem', async (_envelope: unknown, body: unknown) => { | |
| if (input.noteFails) { | |
| throw new Error('journal refused the note') | |
| } | |
| noted.push({ sessionId, text: String(Reflect.get(body ?? {}, 'text')) }) | |
| const target: { appendItem?: unknown } | null = session.journal as { | |
| appendItem?: unknown | |
| } | null | |
| if (target !== null && typeof target === 'object') { | |
| Reflect.set(target, 'appendItem', async (_envelope: unknown, body: unknown) => { | |
| if (input.noteFails) { | |
| throw new Error('journal refused the note') | |
| } | |
| const note = (body ?? {}) as { text?: unknown } | |
| noted.push({ sessionId, text: String(note.text) }) |
🧰 Tools
🪛 GitHub Check: static analysis
[failure] 93-93: anti-slop(no-reflect-get)
src/main/native-chat/agent-session-wire/structured-agent-session-restart-claim.test.ts:93:46: Replace Reflect.get with typed property access. Parse dynamic input into a named domain type before reading it.
[failure] 87-87: anti-slop(no-reflect-get)
src/main/native-chat/agent-session-wire/structured-agent-session-restart-claim.test.ts:87:20: Replace Reflect.get with typed property access. Parse dynamic input into a named domain type before reading it.
Source: Linters/SAST tools
Comment on lines
+72
to
+90
| const kind = Reflect.get(value, 'kind') | ||
| return (kind === 'turn' || kind === 'submission') && isMarkerField(Reflect.get(value, 'id')) | ||
| } | ||
|
|
||
| export function isAgentSessionResumeMarker(value: unknown): value is AgentSessionResumeMarker { | ||
| if (typeof value !== 'object' || value === null) { | ||
| return false | ||
| } | ||
| const recordedAt = Reflect.get(value, 'recordedAt') | ||
| const trigger = Reflect.get(value, 'trigger') | ||
| return ( | ||
| isMarkerField(Reflect.get(value, 'sessionId')) && | ||
| isAgentSessionResumeWork(Reflect.get(value, 'work')) && | ||
| isMarkerField(Reflect.get(value, 'providerHandleRoot')) && | ||
| isMarkerField(Reflect.get(value, 'launchId')) && | ||
| typeof recordedAt === 'number' && | ||
| Number.isSafeInteger(recordedAt) && | ||
| recordedAt >= 0 && | ||
| (trigger === 'quit' || trigger === 'update') |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Replace Reflect.get reads so the static analysis check passes. The changed code reads dynamic fields with Reflect.get in three files. The repository's anti-slop(no-reflect-get) rule rejects that pattern, and the static analysis check reports a failure at each site. Narrow each dynamic value to a named record shape once, then read its fields by property access.
src/shared/agent-session-resume-marker.ts#L72-L90: narrowvalueto a record of the expected marker fields inisAgentSessionResumeWorkandisAgentSessionResumeMarker, then readkind,id,sessionId,work,providerHandleRoot,launchId,recordedAt, andtriggerfrom it.src/main/runtime/agent-session-launch-generation.ts#L47-L47: narrow theJSON.parseresult to{ current?: unknown }and destructurecurrent.src/main/native-chat/agent-session-wire/structured-agent-session-restart-resume-runner.ts#L99-L100: narrow the caught error to{ owner?: unknown }, and rename the local so it no longer shadows theownerparameter.
🧰 Tools
🪛 GitHub Check: static analysis
[failure] 84-84: anti-slop(no-reflect-get)
src/shared/agent-session-resume-marker.ts:84:30: Replace Reflect.get with typed property access. Parse dynamic input into a named domain type before reading it.
[failure] 83-83: anti-slop(no-reflect-get)
src/shared/agent-session-resume-marker.ts:83:19: Replace Reflect.get with typed property access. Parse dynamic input into a named domain type before reading it.
[failure] 81-81: anti-slop(no-reflect-get)
src/shared/agent-session-resume-marker.ts:81:19: Replace Reflect.get with typed property access. Parse dynamic input into a named domain type before reading it.
[failure] 80-80: anti-slop(no-reflect-get)
src/shared/agent-session-resume-marker.ts:80:22: Replace Reflect.get with typed property access. Parse dynamic input into a named domain type before reading it.
[failure] 73-73: anti-slop(no-reflect-get)
src/shared/agent-session-resume-marker.ts:73:70: Replace Reflect.get with typed property access. Parse dynamic input into a named domain type before reading it.
[failure] 72-72: anti-slop(no-reflect-get)
src/shared/agent-session-resume-marker.ts:72:16: Replace Reflect.get with typed property access. Parse dynamic input into a named domain type before reading it.
📍 Affects 3 files
src/shared/agent-session-resume-marker.ts#L72-L90(this comment)src/main/runtime/agent-session-launch-generation.ts#L47-L47src/main/native-chat/agent-session-wire/structured-agent-session-restart-resume-runner.ts#L99-L100
Source: Linters/SAST tools
Contributor
There was a problem hiding this comment.
ℹ️ No new issues in this delta — the new commits fix the two CI failures without introducing new concerns. The prior review's two open findings are in files these commits do not touch and remain outstanding, so this review is not an approval.
Reviewed changes
Since the prior pullfrog review (d4a182f7a4), the branch advanced only via 01476b52b3 and a merge from main.
- Marker and launch-stamp parsing —
agent-session-resume-marker.tsandagent-session-launch-generation.tsreplaceReflect.get-style field reads with a single zod parse; unknown keys still pass, and a malformed marker is dropped rather than thrown. - Resume-admission refusal —
structured-agent-session-restart-resume-runner.tsintroduces a named error carrying a typedowner, replacing a property bag assigned ontoError. - Test-harness typing —
HarnessJournal/HarnessSessionreplaceunknownandReflectin the claim suite's journal stubbing. - Cross-version surface — the four
agentSession.restart*methods are declared in the wire manifest (withinstallableHostreassembling the flat spy map into the host'srestartResumemember), and the manifest and params builders move tostructured-agent-session-surface-manifest.ts. The registeredagentSession.*count and the manifest length now agree at 26.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: 67143e6f-7fed-4b9b-94e3-2fb46c16766b
⛔ Files ignored due to path filters (1)
src/shared/rpc-contract/rpc-params-catalog.generated.tsis excluded by!**/*.generated.*
📒 Files selected for processing (60)
config/nul-byte-baseline.txtconfig/scripts/check-no-nul-bytes.mjspackage.jsonsrc/main/native-chat/agent-session-wire/structured-agent-session-host-teardown.tssrc/main/native-chat/agent-session-wire/structured-agent-session-host-types.tssrc/main/native-chat/agent-session-wire/structured-agent-session-host.tssrc/main/native-chat/agent-session-wire/structured-agent-session-restart-claim.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-restart-continuation.tssrc/main/native-chat/agent-session-wire/structured-agent-session-restart-resume-host.tssrc/main/native-chat/agent-session-wire/structured-agent-session-restart-resume-runner.tssrc/main/native-chat/agent-session-wire/structured-agent-session-restart-resume-set.tssrc/main/native-chat/agent-session-wire/structured-agent-session-restart-resume-test-harness.tssrc/main/native-chat/agent-session-wire/structured-agent-session-restart-resume-wiring.tssrc/main/native-chat/agent-session-wire/structured-agent-session-restart-resume.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-teardown-handoff-drain.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-working-at-teardown.tssrc/main/runtime/agent-session-launch-generation.test.tssrc/main/runtime/agent-session-launch-generation.tssrc/main/runtime/agent-session-operation-admission.tssrc/main/runtime/agent-session-record-store-file.tssrc/main/runtime/agent-session-record-store-write.tssrc/main/runtime/agent-session-record-store.tssrc/main/runtime/agent-session-recovery-publish-fault.test.tssrc/main/runtime/agent-session-reservation-admission.test.tssrc/main/runtime/agent-session-resume-marker-file.tssrc/main/runtime/agent-session-resume-marker-store.test.tssrc/main/runtime/agent-session-resume-marker-store.tssrc/main/runtime/agent-session-store-serialization.tssrc/main/runtime/agent-session-store-transaction-queue.tssrc/main/runtime/rpc/methods/structured-agent-session-restart-resume.tssrc/main/runtime/rpc/methods/structured-agent-session-schemas.tssrc/main/runtime/rpc/methods/structured-agent-session.test.tssrc/main/runtime/rpc/methods/structured-agent-session.tssrc/main/runtime/structured-agent-session-runtime-teardown.tssrc/main/runtime/structured-agent-session-runtime.tssrc/main/startup/main-process-quit.tssrc/renderer/src/app-shell/AppRootSurfaces.tsxsrc/renderer/src/components/NativeChatResumeOnRestartAgentRow.tsxsrc/renderer/src/components/NativeChatResumeOnRestartGroups.tsxsrc/renderer/src/components/NativeChatResumeOnRestartModal.tsxsrc/renderer/src/components/native-chat-resume-on-restart-grouping.test.tssrc/renderer/src/components/native-chat-resume-on-restart-grouping.tssrc/renderer/src/components/settings/NativeChatExperimentalSetting.tsxsrc/renderer/src/components/sidebar/WorktreeHostContextBadge.tsxsrc/renderer/src/components/sidebar/worktree-card-meta-row.tsxsrc/renderer/src/components/ui/badge.tsxsrc/renderer/src/i18n/en-runtime-required.jsonsrc/renderer/src/i18n/locales/en.jsonsrc/shared/agent-session-restart-continuation.tssrc/shared/agent-session-resume-marker.tssrc/shared/default-global-settings.tssrc/shared/folder-workspace-worktree.test.tssrc/shared/folder-workspace-worktree.tssrc/shared/global-settings-types.tssrc/shared/rpc-contract/structured-agent-session-params.tssrc/shared/structured-agent-session-live-turn.tssrc/shared/structured-agent-session-projection.tstests/e2e/cross-version-wire/cross-version-agent-session-wire.unit.test.tstests/e2e/cross-version-wire/structured-agent-session-host-fixture.tstests/e2e/cross-version-wire/structured-agent-session-surface-manifest.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/renderer/src/i18n/locales/en.json
Included review availability: Your plan provides up to 10 included reviews per hour; 6 remain after this review.
| } | ||
|
|
||
| const fixed = cleared.filter((file) => grandfathered.has(file)) | ||
| if (fixed.length > 0 && requested.length === 0) { |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Enforce baseline cleanup for staged files.
When lint-staged passes a cleaned grandfathered file, requested.length > 0 skips this check. The commit can retain a clean path in the baseline despite the stated shrink-only contract. Check fixed for requested files too.
Proposed fix
-if (fixed.length > 0 && requested.length === 0) {
+if (fixed.length > 0) {📝 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
| if (fixed.length > 0 && requested.length === 0) { | |
| if (fixed.length > 0) { |
| // predecessor and mistake it for its own, so this launch refuses adjacency in both directions. | ||
| it('claims no predecessor when the new stamp could not be written', async () => { | ||
| const first = await rotateAgentSessionLaunchGeneration(directory) | ||
| await chmod(directory, 0o500) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
sed -n '1,100p' src/main/runtime/agent-session-launch-generation.test.ts
cat package.json | sed -n '1,80p'
rg -n 'windows|win32|runs-on:.*windows|matrix.*os|chmod\(.*0o500' .github package.json config src/main/runtime --glob '!node_modules'Repository: stablyai/orca
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- test ---'
cat -n src/main/runtime/agent-session-launch-generation.test.ts | sed -n '1,130p'
printf '%s\n' '--- package scripts and platform declarations ---'
python3 - <<'PY'
import json
from pathlib import Path
p = Path('package.json')
data = json.loads(p.read_text())
print(json.dumps(data.get('scripts', {}), indent=2))
PY
rg -n --glob '!node_modules/**' --glob '!**/vendor/**' \
'runs-on:|matrix:|os:|windows-latest|ubuntu-latest|macos-latest|win32|platform\(\)|process\.platform|chmod\(.*0o500|agent-session-launch-generation' \
.github package.json config src/main/runtime
printf '%s\n' '--- test configuration ---'
rg -n --glob '!node_modules/**' \
'vitest|jest|mocha|test.*include|include.*test|platform|windows' \
package.json vite.config.* vitest.config.* jest.config.* .github 2>/dev/null || trueRepository: stablyai/orca
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- focused test ---'
sed -n '1,110p' src/main/runtime/agent-session-launch-generation.test.ts
printf '%s\n' '--- package scripts ---'
python3 - <<'PY'
import json
with open('package.json') as f:
data = json.load(f)
for key, value in data.get('scripts', {}).items():
print(f'{key}: {value}')
PY
printf '%s\n' '--- workflow files ---'
git ls-files '.github/workflows/*' '.github/actions/*' | sort
printf '%s\n' '--- workflow platform lines ---'
for f in $(git ls-files '.github/workflows/*' '.github/actions/*'); do
case "$f" in
*.yml|*.yaml) rg -n -e 'runs-on:' -e 'matrix:' -e 'os:' -e 'windows-latest' -e 'ubuntu-latest' -e 'macos-latest' "$f" || true ;;
esac
done
printf '%s\n' '--- focused platform guards and write injection ---'
rg -n -F 'process.platform' src/main/runtime/agent-session-launch-generation.test.ts src/main/runtime
rg -n -e 'chmod' -e 'writeFile' -e 'rename' -e 'mock.*write' -e 'spyOn.*write' src/main/runtime/agent-session-launch-generation.test.ts src/main/runtimeRepository: stablyai/orca
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- unit-test workflow ---'
sed -n '1,180p' .github/workflows/unit-tests.yml
printf '%s\n' '--- pull-request test workflow platform/test references ---'
rg -n -C 3 -e 'agent-session-launch-generation|vitest run|pnpm run test|matrix:|runs-on:' .github/workflows/unit-tests.yml .github/workflows/pr.yml .github/workflows/pr-test-loc.yml
printf '%s\n' '--- implementation ---'
sed -n '1,220p' src/main/runtime/agent-session-launch-generation.ts
printf '%s\n' '--- vitest config include/exclude ---'
rg -n -C 3 -e 'include|exclude|environment|test' config/vitest.config.tsRepository: stablyai/orca
Length of output: 22649
🌐 Web query:
Node.js fs promises chmod Windows only write permission official documentation
💡 Result:
<search_synthesis>
In Node.js, the fsPromises.chmod method behaves differently on Windows compared to POSIX-compliant systems [1][2]. While the method signature is identical, the following limitations apply on Windows: 1. Write Permission Only: On Windows, only the write permission (represented by the read-only file attribute) can be manipulated using chmod [1][2]. Other POSIX permission bits are ignored [1]. 2. No User/Group Distinction: There is no distinction among owner, group, or others when applying permissions; the change effectively acts on the file globally in terms of the read-only attribute [1][3]. 3. Official Documentation Caveats: While the official Node.js documentation for fsPromises.chmod often lists the mode argument as "Not supported on Windows" in newer versions [4][5][6], this refers to the full POSIX-style permission set. Historically and in practice, the function remains available and maps to the underlying Windows C runtime _wchmod function, which specifically impacts the read-only attribute [7][1]. In summary, you cannot use fsPromises.chmod on Windows to set fine-grained POSIX-style permissions. You can only use it to toggle whether a file is read-only [1][2]. If you require granular permission control on Windows (such as NTFS ACLs), you must use platform-specific tools or native modules, as Node.js's built-in fs module does not provide this functionality [2].
</search_synthesis>
<source_evidence>
Citations:
- 1: nodejs/node@a5c571424a
- 2: https://github.com/ehmicky/cross-platform-node-guide/blob/main/docs/5_security/permissions.md
- 3: https://www.geeksforgeeks.org/node-js/node-js-fspromises-chmod-method/
- 4: https://nodejs.org/docs/latest-v24.x/api/fs.html
- 5: https://beta.docs.nodejs.org/fs/promises-api
- 6: https://beta.docs.nodejs.org/fs.html
- 7: GitHub issue 30019 in nodejs/node (link omitted to avoid creating a cross-reference)
Use a deterministic write failure.
If this test runs on Windows, chmod(directory, 0o500) does not provide reliable POSIX directory-write protection. A privileged Unix process can also bypass the mode bits. The second rotation can therefore write successfully, so second.previous is not null and the test fails.
Inject or mock writeTempFileDurable so the test always exercises the intended failure path.
| ): Promise<void> { | ||
| // Drain an in-flight recovery before stopping children; recovery may still | ||
| // be writing lifecycle rows or acquiring a replacement child. | ||
| await installed.waitForRecovery() |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
sed -n '1,110p' src/main/runtime/structured-agent-session-runtime-teardown.ts
sed -n '270,330p' src/main/runtime/structured-agent-session-runtime.ts
rg -n 'tearDownRuntime|stopStructuredAgentSessionRuntime|closeAll\(' src/mainRepository: stablyai/orca
Length of output: 18793
🏁 Script executed:
#!/bin/bash
sed -n '130,180p' src/main/runtime/structured-agent-session-runtime.ts
sed -n '315,370p' src/main/runtime/structured-agent-session-runtime.ts
sed -n '1,95p' src/main/runtime/structured-agent-session-runtime-teardown.ts
sed -n '120,155p' src/main/startup/main-process-quit.tsRepository: stablyai/orca
Length of output: 8989
Continue cleanup if the initial recovery drain fails.
If installed.waitForRecovery() rejects, tearDownRuntime exits before installed.host.flushAllStreamedEvents() and installed.adapter.closeAll() run. stopStructuredAgentSessionRuntime() only records the failed runtime for a later stop; it does not provide equivalent cleanup. Provider children and session resources can remain active during shutdown.
Capture this failure with the other teardown failures, then continue the cleanup sequence.
Proposed fix
- await installed.waitForRecovery()
const failures: unknown[] = []
+ try {
+ await installed.waitForRecovery()
+ } catch (error) {
+ failures.push(error)
+ }| }) | ||
| // Rotated once per launch, before anything can act on a marker. Deliberately NOT inside the | ||
| // record store directory: see the module for why its rollback domain must stay separate. | ||
| const launchGeneration = await rotateAgentSessionLaunchGeneration(deps.stateDirectory) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline src/main/runtime/structured-agent-session-runtime.ts \
--match 'ensureStructuredAgentSessionHost|install' --view expanded
rg -n -C 5 \
'class StructuredAgentSessionHost|constructor\s*\(|function createStructuredClaudeRuntimeAdapter|class StructuredAgentSessionAdapterRouter|function setStructuredAgentSessionHost' \
src/main --glob '*.ts'Repository: stablyai/orca
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- runtime file outline ---'
ast-grep outline src/main/runtime/structured-agent-session-runtime.ts
printf '%s\n' '--- runtime file relevant sections ---'
cat -n src/main/runtime/structured-agent-session-runtime.ts | sed -n '1,280p'
printf '%s\n' '--- exact symbol references ---'
rg -n -C 8 \
'ensureStructuredAgentSessionHost|rotateAgentSessionLaunchGeneration|StructuredAgentSessionHost|createStructuredClaudeRuntimeAdapter|StructuredAgentSessionAdapterRouter|setStructuredAgentSessionHost' \
src/main/runtime --glob '*.ts' --glob '!*.test.ts' | head -n 1200Repository: stablyai/orca
Length of output: 50370
🏁 Script executed:
set -euo pipefail
cat -n src/main/runtime/structured-agent-session-runtime.ts | sed -n '1,280p'
rg -n -C 8 'ensureStructuredAgentSessionHost|rotateAgentSessionLaunchGeneration|StructuredAgentSessionHost|createStructuredClaudeRuntimeAdapter|StructuredAgentSessionAdapterRouter|setStructuredAgentSessionHost' src/main/runtime --glob '*.ts' --glob '!*.test.ts' | head -n 1200Repository: stablyai/orca
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- install remainder ---'
cat -n src/main/runtime/structured-agent-session-runtime.ts | sed -n '275,380p'
printf '%s\n' '--- exact constructor/function definitions ---'
rg -n -l \
'export class StructuredAgentSessionHost|class StructuredAgentSessionHost|export class StructuredAgentSessionAdapterRouter|class StructuredAgentSessionAdapterRouter|export function createStructuredClaudeRuntimeAdapter|function createStructuredClaudeRuntimeAdapter|export class CodexStructuredSessionAdapter|class CodexStructuredSessionAdapter|function rotateAgentSessionLaunchGeneration|export async function rotateAgentSessionLaunchGeneration' \
src/main --glob '*.ts' --glob '!*.test.ts' |
while IFS= read -r file; do
echo "--- $file ---"
rg -n -C 12 \
'export class StructuredAgentSessionHost|class StructuredAgentSessionHost|export class StructuredAgentSessionAdapterRouter|class StructuredAgentSessionAdapterRouter|export function createStructuredClaudeRuntimeAdapter|function createStructuredClaudeRuntimeAdapter|export class CodexStructuredSessionAdapter|class CodexStructuredSessionAdapter|function rotateAgentSessionLaunchGeneration|export async function rotateAgentSessionLaunchGeneration' \
"$file"
doneRepository: stablyai/orca
Length of output: 11630
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cat -n src/main/runtime/structured-agent-session-runtime.ts | sed -n '275,380p'
rg -n -l 'StructuredAgentSessionHost|StructuredAgentSessionAdapterRouter|createStructuredClaudeRuntimeAdapter|CodexStructuredSessionAdapter|rotateAgentSessionLaunchGeneration' src/main --glob '*.ts' --glob '!*.test.ts' | while IFS= read -r file; do
echo "--- $file ---"
rg -n -C 12 'class StructuredAgentSessionHost|class StructuredAgentSessionAdapterRouter|function createStructuredClaudeRuntimeAdapter|class CodexStructuredSessionAdapter|function rotateAgentSessionLaunchGeneration' "$file"
doneRepository: stablyai/orca
Length of output: 3583
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- host constructor ---'
rg -n -C 30 'constructor\s*\(' src/main/native-chat/agent-session-wire/structured-agent-session-host.ts | head -n 220
printf '%s\n' '--- codex constructor ---'
cat -n src/main/codex/codex-structured-session-adapter.ts | sed -n '45,115p'
printf '%s\n' '--- claude adapter factory and constructor ---'
cat -n src/main/runtime/structured-claude-runtime-adapter.ts | sed -n '1,100p'
rg -n -C 20 'constructor\s*\(' src/main/claude/claude-structured-session-adapter.ts | head -n 180
printf '%s\n' '--- launch generation helper ---'
cat -n src/main/runtime/agent-session-launch-generation.ts | sed -n '1,140p'Repository: stablyai/orca
Length of output: 18896
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 30 'constructor\s*\(' src/main/native-chat/agent-session-wire/structured-agent-session-host.ts | head -n 220
cat -n src/main/codex/codex-structured-session-adapter.ts | sed -n '45,115p'
cat -n src/main/runtime/structured-claude-runtime-adapter.ts | sed -n '1,100p'
rg -n -C 20 'constructor\s*\(' src/main/claude/claude-structured-session-adapter.ts | head -n 180
cat -n src/main/runtime/agent-session-launch-generation.ts | sed -n '1,140p'Repository: stablyai/orca
Length of output: 18765
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- host constructor continuation ---'
cat -n src/main/native-chat/agent-session-wire/structured-agent-session-host.ts | sed -n '85,180p'
printf '%s\n' '--- direct post-rotation helpers ---'
rg -n -C 8 \
'function createStructuredAgentSessionHostHandoff|createStructuredAgentSessionHostHandoff|class StructuredAgentSessionBackgroundTaskChannel|class StructuredAgentSessionHostRuntimeState|function createRestartReconciler|setStructuredAgentSessionHost' \
src/main/native-chat/agent-session-wire --glob '*.ts' --glob '!*.test.ts' | head -n 800
printf '%s\n' '--- explicit throws in direct construction files ---'
rg -n -C 3 'throw new|throw ' \
src/main/native-chat/agent-session-wire/structured-agent-session-host.ts \
src/main/native-chat/agent-session-wire/structured-agent-session-host-runtime-state.ts \
src/main/native-chat/agent-session-wire/structured-agent-session-background-task-channel.ts \
src/main/native-chat/agent-session-wire/structured-agent-session-handoff.ts \
src/main/native-chat/agent-session-wire/structured-agent-session-registry.ts \
src/main/codex/codex-structured-session-adapter.ts \
src/main/claude/claude-structured-session-adapter.ts \
src/main/runtime/structured-claude-runtime-adapter.ts || trueRepository: stablyai/orca
Length of output: 25144
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cat -n src/main/native-chat/agent-session-wire/structured-agent-session-host.ts | sed -n '85,180p'
rg -n -C 8 'createStructuredAgentSessionHostHandoff|class StructuredAgentSessionBackgroundTaskChannel|class StructuredAgentSessionHostRuntimeState|function createRestartReconciler|setStructuredAgentSessionHost' src/main/native-chat/agent-session-wire --glob '*.ts' --glob '!*.test.ts' | head -n 800
rg -n -C 3 'throw new|throw ' src/main/native-chat/agent-session-wire/structured-agent-session-host.ts src/main/native-chat/agent-session-wire/structured-agent-session-host-runtime-state.ts src/main/native-chat/agent-session-wire/structured-agent-session-background-task-channel.ts src/main/native-chat/agent-session-wire/structured-agent-session-handoff.ts src/main/native-chat/agent-session-wire/structured-agent-session-registry.ts src/main/codex/codex-structured-session-adapter.ts src/main/claude/claude-structured-session-adapter.ts src/main/runtime/structured-claude-runtime-adapter.ts || trueRepository: stablyai/orca
Length of output: 25016
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- host handoff factory ---'
cat -n src/main/native-chat/agent-session-wire/structured-agent-session-host-handoff.ts | sed -n '1,150p'
printf '%s\n' '--- host restore/restart wiring ---'
rg -n -l 'createStructuredAgentSessionHostRestore|structuredAgentSessionRestartResumeSurfaces|createStructuredAgentSessionRestartResume' src/main/native-chat/agent-session-wire --glob '*.ts' --glob '!*.test.ts' |
while IFS= read -r file; do
echo "--- $file ---"
rg -n -C 12 'createStructuredAgentSessionHostRestore|structuredAgentSessionRestartResumeSurfaces|createStructuredAgentSessionRestartResume' "$file"
done
printf '%s\n' '--- retry/rotation tests ---'
rg -n -C 8 'rotateAgentSessionLaunchGeneration|ensureStructuredAgentSessionHost|install.*reject|retry.*install|launchGeneration' src/main --glob '*.test.ts' | head -n 1000Repository: stablyai/orca
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cat -n src/main/native-chat/agent-session-wire/structured-agent-session-host-handoff.ts | sed -n '1,150p'
rg -n -l 'createStructuredAgentSessionHostRestore|structuredAgentSessionRestartResumeSurfaces|createStructuredAgentSessionRestartResume' src/main/native-chat/agent-session-wire --glob '*.ts' --glob '!*.test.ts' | while IFS= read -r file; do
echo "--- $file ---"
rg -n -C 12 'createStructuredAgentSessionHostRestore|structuredAgentSessionRestartResumeSurfaces|createStructuredAgentSessionRestartResume' "$file"
done
rg -n -C 8 'rotateAgentSessionLaunchGeneration|ensureStructuredAgentSessionHost|install.*reject|retry.*install|launchGeneration' src/main --glob '*.test.ts' | head -n 1000Repository: stablyai/orca
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- PTY gate attach ---'
rg -n -C 15 'attachRecordLookup|class .*Pty.*Gate' src/main/runtime/agent-session-pty-write-gate.ts
printf '%s\n' '--- lease renewal startup ---'
cat -n src/main/native-chat/agent-session-wire/structured-agent-session-host-runtime-state.ts | sed -n '1,130p'
rg -n -C 12 'startLeaseRenewal' src/main/native-chat/agent-session-wire --glob '*.ts' --glob '!*.test.ts'Repository: stablyai/orca
Length of output: 13892
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 15 'attachRecordLookup|class .*Pty.*Gate' src/main/runtime/agent-session-pty-write-gate.ts
cat -n src/main/native-chat/agent-session-wire/structured-agent-session-host-runtime-state.ts | sed -n '1,130p'
rg -n -C 12 'startLeaseRenewal' src/main/native-chat/agent-session-wire --glob '*.ts' --glob '!*.test.ts'Repository: stablyai/orca
Length of output: 13838
Rotate the launch generation once per process launch. ensureStructuredAgentSessionHost() clears installing when install() rejects, so a later call starts a new attempt. Each attempt runs rotateAgentSessionLaunchGeneration() at line 198. If the synchronous adapter or host setup after that line throws, the retry writes a second generation and refuses markers from the actual previous launch. Move rotation to process-launch scope and reuse that generation across installation retries.
Contributor
There was a problem hiding this comment.
ℹ️ No critical issues — one stale invariant comment inline, plus a description that still documents the removed launch-scoping.
Reviewed changes
Since the prior pullfrog review (52ffed7114), the branch advanced by one commit, b3e29dcd58, touching 30 files (+474/−621).
- Durable markers moved into an isolated advisory capsule —
AgentSessionRecoveryCapsulereplaces the store-residentresumeMarkers:recordpublishes the whole set,takereads, clears in the same locked step, and returns only fresh markers. The capsule is its own file with no.bak, so it no longer shares a rollback domain with the session store or its transaction queue. - Launch-generation adjacency removed —
agent-session-launch-generation.tsand its stamp file are deleted, and the marker'slaunchIdbecomesteardownId, used only to key the continuation's deterministicclientOperationId. A failed clear now leaves the capsule for a later launch instead of refusing it. - Teardown record is bounded and diagnosed —
record-resume-markersruns behindwithPhaseTimeout(…, 2s), turning a stall into a logged failure rather than a wait, and still runs before eviction settles turns. - Shared whole-file lock generalized —
agent-session-store-transaction-lock.tsbecomessrc/main/file-transaction-lock.tswith an optionalretriesoverride (0for the capsule); the store, its loader, and the capsule all use it. - Coverage — new
agent-session-recovery-capsule.test.ts(take-once across competing lock owners, failed-clear exposes nothing then allows a later take, corrupt capsule refuses without partial candidates, freshness window, legacy store markers ignored and preserved) plus claim/ownership/drain tests for the capsule. The four affected suites run green at this head (82 tests), ownership green (16).
ℹ️ The description still specifies launch-scoped markers
The PR body's "Why the marker is launch-scoped" and "Guarantee → Markers only survive one launch" sections still present launch adjacency as the mechanism that stops a stale marker being acted on. This commit deletes that module and its check, so the description no longer describes the shipped design — a reviewer reading only the body would believe a stronger invariant than the code enforces.
Technical details
# PR body describes the removed launch-adjacency guard
## Affected sites
- `b3e29dcd58` — deletes `src/main/runtime/agent-session-launch-generation.ts` and the `launchId === previous` filter in `structured-agent-session-restart-resume-host.ts`.
- `src/shared/agent-session-resume-marker.ts:54` — `teardownId` is documented as "not launch ancestry".
## Required outcome
- Update the safety sections to the capsule model (isolated file, no backup, consumed atomically on `take`), or state plainly that adjacency was dropped and why the remaining predicate still bounds a stale marker.
- Confirm the intent: a capsule that survives a failed clear, or a launch that never installs the host, can now be taken by a later launch within the 24h TTL, so "only the immediately preceding launch" is intentionally relaxed rather than enforced.
## Open questions for the human
Is dropping adjacency deliberate and permanent, or an intermediate step toward rotating a launch id inside the capsule?DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: 67daa053-6186-4e10-93f4-30305440e149
📒 Files selected for processing (28)
src/main/file-transaction-lock.tssrc/main/native-chat/agent-session-wire/structured-agent-session-host-teardown.tssrc/main/native-chat/agent-session-wire/structured-agent-session-host-test-harness.tssrc/main/native-chat/agent-session-wire/structured-agent-session-host-types.tssrc/main/native-chat/agent-session-wire/structured-agent-session-restart-claim.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-restart-continuation.tssrc/main/native-chat/agent-session-wire/structured-agent-session-restart-ownership.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-restart-resume-host.tssrc/main/native-chat/agent-session-wire/structured-agent-session-restart-resume-runner.tssrc/main/native-chat/agent-session-wire/structured-agent-session-restart-resume-set.tssrc/main/native-chat/agent-session-wire/structured-agent-session-restart-resume-test-harness.tssrc/main/native-chat/agent-session-wire/structured-agent-session-restart-resume.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-teardown-handoff-drain.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-working-at-teardown.tssrc/main/runtime/agent-session-record-store-file.tssrc/main/runtime/agent-session-record-store-security.tssrc/main/runtime/agent-session-record-store.tssrc/main/runtime/agent-session-recovery-capsule.test.tssrc/main/runtime/agent-session-recovery-capsule.tssrc/main/runtime/agent-session-store-transaction-queue.tssrc/main/runtime/structured-agent-session-runtime.tssrc/renderer/src/components/NativeChatResumeOnRestartModal.test.tsxsrc/renderer/src/components/NativeChatResumeOnRestartModal.tsxsrc/renderer/src/components/native-chat-restart-action-notifications.tssrc/renderer/src/i18n/en-runtime-required.jsonsrc/renderer/src/i18n/locales/en.jsonsrc/shared/agent-session-resume-marker.tssrc/shared/structured-agent-session-projection.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- src/renderer/src/i18n/en-runtime-required.json
- src/renderer/src/components/NativeChatResumeOnRestartModal.test.tsx
- src/main/native-chat/agent-session-wire/structured-agent-session-restart-resume-host.ts
- src/main/native-chat/agent-session-wire/structured-agent-session-restart-resume-runner.ts
- src/main/native-chat/agent-session-wire/structured-agent-session-teardown-handoff-drain.test.ts
- src/main/runtime/agent-session-record-store-file.ts
- src/renderer/src/i18n/locales/en.json
Included review availability: Your plan provides up to 10 included reviews per hour; 3 remain after this review.
| await this.publish([]) | ||
| return markers.filter((marker) => !isExpiredAgentSessionResumeMarker(marker, now)) | ||
| }, | ||
| { retries: 0 } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
sed -n '1,115p' src/main/runtime/agent-session-recovery-capsule.ts
sed -n '1,330p' src/main/native-chat/agent-session-wire/structured-agent-session-restart-resume-host.ts
sed -n '1,90p' src/main/file-transaction-lock.ts
rg -n "restartResume|\.list\(|recoveryCapsule\.take|waitForRecovery|flushAllStreamedEvents|recordResume" src/main --glob '*.ts'Repository: stablyai/orca
Length of output: 41943
🏁 Script executed:
#!/bin/bash
sed -n '1,125p' src/main/native-chat/agent-session-wire/structured-agent-session-host-teardown.ts
sed -n '1,105p' src/main/runtime/structured-agent-session-runtime-teardown.ts
sed -n '100,175p' src/main/runtime/structured-agent-session-runtime.ts
sed -n '300,340p' src/main/runtime/structured-agent-session-runtime.ts
sed -n '1,120p' src/main/native-chat/agent-session-wire/structured-agent-session-host.ts
sed -n '245,285p' src/main/native-chat/agent-session-wire/structured-agent-session-host.ts
sed -n '1,120p' src/main/native-chat/agent-session-wire/structured-agent-session-restart-claim.test.ts
sed -n '300,345p' src/main/native-chat/agent-session-wire/structured-agent-session-restart-claim.test.ts
sed -n '1,95p' src/main/runtime/structured-agent-session-runtime-teardown.test.ts 2>/dev/null || true
rg -n "createStructuredAgentSessionRuntime|structuredAgentSessionRuntimeTeardown|waitForRecovery|flushAllStreamedEvents|recordResumeMarkers|restartResume\.list" src/main --glob '*.ts'Repository: stablyai/orca
Length of output: 35906
🏁 Script executed:
#!/bin/bash
rg -n -C 5 "stopStructuredAgentSessionRuntime|ensureStructuredAgentSessionHost|waitForStructuredAgentSessionRecovery|structuredAgentSessionTeardown" . --glob '*.ts' --glob '*.tsx' --glob '*.js'
rg -n -C 8 "restartResume\.list\(\)|agentSession\.restartResume|restartResumable" src --glob '*.ts' --glob '*.tsx'
rg -n -C 6 "start.*runtime|stop.*runtime|teardown.*startup|startup.*teardown" src/main --glob '*test*.ts' --glob '*.ts'Repository: stablyai/orca
Length of output: 50370
🏁 Script executed:
#!/bin/bash
rg -n -l "stopStructuredAgentSessionRuntime|ensureStructuredAgentSessionHost|waitForStructuredAgentSessionRecovery" src/main/startup src/main/runtime src/main/orcad 2>/dev/null
sed -n '1,180p' src/main/startup/main-process-quit.ts
rg -n -C 12 "ensureStructuredAgentSessionHost|stopStructuredAgentSessionRuntime" src/main/startup/main-process-runtime-service.ts src/main/orcad/orcad-entry.ts 2>/dev/null
rg -n -C 12 "stopStructuredAgentSessionRuntime|ensureStructuredAgentSessionHost" src/main --glob '*.ts' --glob '!*.test.ts' --glob '!*.spec.ts' | head -n 240Repository: stablyai/orca
Length of output: 37670
🏁 Script executed:
#!/bin/bash
sed -n '180,285p' src/main/startup/main-process-quit.ts
sed -n '1,220p' src/main/startup/desktop-startup-ordering.test.ts
rg -n -C 8 "structuredAgentSessionShutdown|stopStructuredAgentSessionRuntime|app\.quit|quitTeardown" src/main/startup/main-process-quit.tsRepository: stablyai/orca
Length of output: 27263
Retry take() during capsule teardown.
record() and take() use the same file lock. Because take() passes { retries: 0 }, it can fail immediately while record() publishes markers. claimMarkers() catches that failure, caches claimed = [], and does not retry, so restartResume.list() returns no launch offer.
The shutdown path waits before app.quit(), but stopStructuredAgentSessionRuntime() clears installing before awaiting teardown. A new host can therefore start while the previous record() is still pending.
Keep record() bounded, but use the default bounded retry policy for take().
Proposed fix
- { retries: 0 }
+ // Startup can wait briefly for an in-progress teardown marker write.📝 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
| { retries: 0 } | |
| // Startup can wait briefly for an in-progress teardown marker write. |
Contributor
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Since the prior pullfrog review (b3e29dcd58), the branch advanced by one commit, 5761c9f368, touching 9 files (+286/−93).
- Tightened the journal-agreement predicate — the former
acceptedSubmissionWasCutOffbecomessubmissionWorkWasCutOff, and every non-rejecteddispatch (pending, unknown, accepted) is now judged by the journal's newest turn. Previously apending/unknownsend returned "cut off" unconditionally, so a turn the provider completed without a delivery acknowledgement could still be offered. - Simplified the submission branch —
rejectedis the only dispatch state that refuses outright; the completed/running turn ambiguity now resolves to no whether or not a turn can be linked back to the marked send. - Extracted renderer outcome reporting — the new
native-chat-restart-action-notifications.tsbuckets each requested chat into succeeded / unconfirmed (pending|unknown) / refused and toasts each bucket, keeping the existing success copy. - Surfaced failed actions instead of closing silently — both reconnect and reconnect-and-continue now catch, announce unconfirmed for the requested chats, and resolve; the automatic reconnect path announces unconfirmed rather than letting the rejection escape.
- Fixed the stale
teardownIdcomment —structured-agent-session-working-at-teardown.ts:75now describes a per-teardown identity for continuation dedup, retiring the prior review's open thread. - Coverage — ownership (
refuses a completed turn even when its original delivery acknowledgement is unknown), resume-set (pending/unknown + completed turn), and modal (refused/unconfirmed/lost-response) tests, plus the matching plural i18n keys.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
Contributor
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Since the prior pullfrog review (5761c9f368), the branch advanced one commit, c6ce0e1ad5, touching 4 files (+87/−3).
- Admission recheck now sees queued provider lifecycle —
runSettledAgentSessionMutationflushes the session's streamed event sink before invokingplan.beforeRun(), so the restart continuation's supersession guard judges the journal with a completion/message the host already accepted applied, rather than a stale snapshot. - Thrown sends are reported as unverifiable — the continuation distinguishes its own pre-dispatch
RestartContinuationSupersededError(rethrown → refused) from any other send rejection (logged, returned asunknown);performSendonly throws at its post-dispatch persistence step, where a lost reply cannot prove non-delivery. - Coverage — the ownership suite adds a
completion/messagetest proving a queued event flips the decision toagent_session_restart_work_superseded, plus a settlement-failure test pinningunknownwith no redispatch. I confirmed the two queued-event tests fail when the flush is removed, so they exercise the new guard.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
Contributor
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Since the prior pullfrog review, the branch advanced one commit, a6bd3a5549 ("fix(native-chat): rederive recovery evidence before continuing"), touching 17 files (+395/−112).
- Teardown re-derives each marker after the provider stops —
captureMarkerssnapshots candidate markers into memory before eviction, andevict-owned-sessionscallsconfirmStoppedMarkerafter the child is proven stopped and its tail drained (viaonStoppedWorkinsidesettleWork), so only confirmed markers reach the capsule. A terminal event (approval/question/completed) that lands duringcloseSessionnow suppresses the offer instead of being recorded as working. - A stopped provider makes a
runningturn count as cut off —turnWasCutOffgains a teardown-onlyproviderStoppedflag, and the resumable predicate gains awaitingOnUserclause that refuses work projecting asattention. The flag is only ever set fromconfirmStoppedMarker; launch-side derivation leaves it off. - Eviction is serialized per session —
evictOwnedStructuredAgentSessionsnow routes each close through the host's per-sessionserializequeue, so a settlement cannot race an in-flight mutation. - Teardown phase order extracted —
capture-resume-markersruns first,record-resume-markersmoves to afterevict-owned-sessions, and the wiring consolidates intoflushStructuredAgentSessionHost. - Admission barrier bounded —
runSettledAgentSessionMutationwraps the pre-beforeRunevent flush in a 2s timeout and throws the newAgentSessionPreDispatchErrorwhen evidence is unavailable;RestartContinuationSupersededErrornow extends it, and the continuation rethrows any pre-dispatch failure as a refusal. - A proven refusal survives failed bookkeeping — the settlement catch returns a refusal outcome rather than rethrowing when its own write fails.
- Capsule sweeps expired temp debris on publish.
- Coverage — new
structured-agent-session-operation-settlement.test.ts, thedoes not offer work superseded by a provider %s during closeteardown cases, approval/question parametrizations in the ownership and resume-set suites, and a capsule debris-reclaim test. I ran the 10 changed suites (181 tests) green at this head; no failing check runs ona6bd3a5549.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
Contributor
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Since the prior pullfrog review (a6bd3a5549), the branch advanced one commit, b12ea7a104 ("Validate restart continuation at provider dispatch"), touching 10 files (+181/-30).
- Admission barrier moved to provider dispatch — the pre-
beforeRunflush and its 2s timeout leftrunSettledAgentSessionMutationand now run insidedispatchSafely, immediately beforeadapter.dispatch; only the restart-continuation send setsbeforeRun, so no ordinary mutation path is affected. - Post-flush pending-event guard — the host exposes
hasPendingStreamedEvents(failed || queuedOperations > 0) through the mutation and turn contexts, anddispatchSafelyrefuses withagent_session_admission_evidence_unavailablewhen the barrier resolves while newer provider events are still queued, closing the window between "drained" and "dispatch". - Pre-dispatch refusals are journaled —
performSendcatchesAgentSessionPreDispatchError, records the submissionrejected, publishes, and rethrows; the operation ledger staysunknownand a replay reconstructs the rejected row without re-dispatching. - Continuation no longer supersedes itself —
derivefilters the continuation's own submission key out of the snapshot, so its pending user row, now written before the recheck, is not read as newer user work. - Coverage — ownership cases for provider evidence accepted during
appendSubmissionand for events accepted after the barrier was captured, plus thestalled-with-refusal-writesettlement case; the changed suites run green (41 tests) at this head.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
Contributor
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Since the prior pullfrog review (b12ea7a104), the branch advanced one commit, 358673de68 ("fix(native-chat): finish restart refusal and attribution delivery"), touching 9 files (+130/−13). All prior Pullfrog threads are resolved, and the four changed suites run green (67 tests) at this head.
- Continuation attribution is published —
StructuredAgentSessionRestartResumeSurfacesgains apublish(sessionId, journal)member (host bindsthis.subscribers.publish; the wiring returnsOmit<…, 'publish'>), and the resume host'snotecallback calls it after appending the status item. The "Orca sent this" note now reaches subscribed renderers without waiting on a subsequent provider event. - Rejected sends no longer render as sent —
projectStructuredAgentSessionMessagesfilters out journal items whose id matches adispatchState === 'rejected'submission, and derivesjournalledfrom the filtered set so a retained outbox draft still renders. A refused continuation, which has no outbox entry, stops appearing as a sent user message. - Pre-dispatch refusals skip the redundant uncertainty write — the settlement catch rethrows an
AgentSessionPreDispatchErrorimmediately whenmarkUnknownBeforeRunis set, since the pre-rununknownrow is already durable andperformSendhas journaled the submissionrejected. Only the continuation setsbeforeRun, so no ordinary send path changes; the ledger staysunknownand replay finds the rejected row without re-dispatching. - Capsule-take failure no longer logs the error —
catch {}replacescatch (error)on the recovery-capsule take warning, keeping capsule payloads and filesystem paths out of logs. - Coverage — settlement pins the stalled-write pre-dispatch refusal (exactly one uncertainty write, no dispatch), claim pins the log-privacy behavior, ownership pins attribution publication over a subscriber plus the corrected
settlementFailsexpectation, and the renderer projection covers the hidden rejected send and the retained refused draft.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
Contributor
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Since the prior pullfrog review (358673de68), the branch advanced one commit, 40a9b89a73 ("fix(native-chat): keep recovery teardown errors out of logs"), touching 4 files (+37/−9). The three changed suites run green (44 tests).
- Teardown recovery failures no longer log the error object —
structured-agent-session-host-teardown.tsdropserrorfrom thecapture-resume-markerscatch and from therecord-resume-markers.catch, so both warnings are a single fixed string. This extends the same privacy treatment358673de68gave the capsule take to the two teardown phases, keeping capsule payloads and filesystem paths off the console. - Coverage —
structured-agent-session-restart-teardown.test.tsadds a parametrizedcaptureMarkers/recordMarkersfailure test asserting the exact one-argument warning, that the private failure message does not appear, and that teardown still completes (lease released, session ownership unknown). The ownership capsule-failure test now pins the one-arg warning and a root-path-absent check, and the handoff-drain test addsexpect(vi.getTimerCount()).toBe(0).
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
Check continuation authority after the SDK input queue wait and arm replay correlation only after authorization. Preserve typed pre-dispatch refusal, ordinary send behavior, and cleanup when the provider exits or capacity fills during authorization.
Contributor
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Since the prior pullfrog review (40a9b89a73), the branch advanced one commit, b2f72902e8 ("fix(native-chat): validate restart continuation at provider dispatch"), touching 13 files (+141/−29).
- Pre-dispatch validation moved inside the adapter dispatch —
StructuredAgentSessionAdapter.dispatchgains an optionalbeforeDispatch, anddispatchClaudeTurn/CodexStructuredSessionAdapter.dispatchnow await it after provider preparation (Claude after content translation; Codex aftercaptureTurnProcesses) and before registering a Claude dispatch waiter or writingturn/start. The router forwards the whole input, so the contract holds for both providers. - The admission barrier is now that callback —
dispatchSafelybuilds an asyncbeforeDispatchthat flushes streamed events, refuses withagent_session_admission_evidence_unavailablewhen events are still queued, then runs the plan's continuation authority recheck. Because the refusal now surfaces from insideadapter.dispatch,dispatchSafelyrethrowsAgentSessionPreDispatchErrorinstead of converting it tounknown;performSendstill journals the submissionrejected. - Attribution-note failure reporting simplified — the restart-resume surface drops its
reportErrorwiring and always logs one fixed string, keeping session ids and error payloads off the console; the note stays best-effort and cannot fail the continuation. - Coverage — new Claude and Codex dispatch-boundary tests pin the ordering (refusal sends nothing / writes no
turn/start), the host test harness adapter invokesinput.beforeDispatch, and the ownership suite pins the fixed attribution warning. The seven touched suites run green at this head (86 tests).
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
Contributor
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Since the prior pullfrog review (b2f72902e8), the branch advanced one commit, 3d4bd63b55 ("Revalidate restart continuation when Claude dequeues input"), touching 6 files (+243/−86).
- The continuation authority recheck now runs when the SDK input pump dequeues the frame, not when the send is queued.
dispatchClaudeTurntakes an optionalbeforeDispatchand forwards it throughClaudeStreamJsonConnection.sendintocreateClaudeUserMessageQueue, whose pump awaits it immediately before yielding the frame;ClaudeStructuredSessionAdapter.dispatchpassesinput.beforeDispatch. A continuation waiting behind a running turn is therefore re-judged against the journal at the moment it would be written, closing the window the prior head left open. - Replay correlation is armed only after authorization. The new
arm()helper incrementsdispatchSequenceand registers the waiter; with abeforeDispatchpresent,pending.replaystarts undefined andauthorizearms it only after the callback resolves and the queue-full check passes. A refused continuation registers no waiter at all. - Capacity is re-checked inside authorization, throwing
claudeUnwrittenUserMessageError(DISPATCH_REJECTED_QUEUE_FULL)when the queue filled during the wait; the dispatch catch maps that torejected(the reason still classifies as internal through theprovider_write_failed:prefix). - Typed pre-dispatch refusals survive the queue hop. When authorization throws before arming,
dispatchClaudeTurnrethrows anAgentSessionPreDispatchErrorunchanged rather than folding it intounknown, soperformSendstill journals the submissionrejected. fail()no longer rejects a frame that is still awaiting authorization. It rejects the in-flight frame only whenhandedOff(already yielded); an authorizing frame is rejected as provably unwritten by the pump's post-callbackfailurecheck, so provider exit during the wait settlesrejectedrather thanunknown.- Extraction and coverage. The waiter helpers move to
claude-structured-dispatch-waiters.ts; the newclaude-structured-dispatch-queue-boundary.test.tsdrives the real queue to pin that the recheck does not run at send time and a superseded continuation is refused after the pump wait with no leaked waiter, and that provider exit and capacity-fill during authorization settlerejectedwithout leaked correlation. The fake connection now invokesbeforeDispatch, keeping the existing boundary test valid.
I traced the queue/dispatch interleavings independently and found no path where a beforeDispatch frame's promise hangs, a waiter is orphaned, or a write-unknown frame is reported as accepted/rejected.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
# Conflicts: # src/main/claude/claude-stream-json-connection.ts # src/main/claude/claude-structured-session-test-support.ts
Contributor
Author
|
Review status for head READY/CLEAN for the PR patch. A fresh same-model review and the final readiness pass found no proven in-scope functional, architectural, performance, security, lifecycle, cross-platform, compatibility, or scope issues. The branch was rebased by merging current Validation passed: 160 focused recovery/ownership/continuation/capsule/teardown tests, The current PR check run has one unrelated failure in |
Contributor
Author
|
Follow-up validation for the spacing polish at head
|
9 tasks done
ivanvykopal
pushed a commit
to ivanvykopal/orca
that referenced
this pull request
Sep 20, 2026
…stablyai#21096) * feat(native-chat): resume structured chats that were working at restart Teardown records a marker for every session this host was genuinely running a turn for, derived from the LIVE runtime rather than a persisted status row, so a stale `running` row left by an older crash can never trigger a resume. On the next launch a modal lists exactly which chats would resume and resumes them via native continuation (Claude resume/resumeSessionAt, Codex thread id) — never by re-sending the prompt, which is what makes an agent redo finished work. A session resumes only when all of these hold: a teardown marker exists and has not expired, the record's lease is released and reconciled, a provider resume cursor exists and still matches the marker, the journal's own turn record names the same turn, and the marker has not already been spent. Markers are consumed before the resume is submitted, so a crash mid-resume cannot double-fire, and an admission gate refuses a second concurrent resume for one session. Resumes are staggered three at a time rather than spawning every provider at once. The modal's "Don't ask again" checkbox writes the nativeChatResumeWorkOnRestart setting, which Settings can turn back off; automatic mode runs the identical predicate and staggering and reports what it did. Declining consumes the markers so the prompt cannot return every launch — nothing is lost, because opening a chat still re-acquires it at the same cursor. * fix(native-chat): compare handle ROOT and turn state when offering a resume Four defects QA found in the restart-resume offer, fixed together because the first two interact: shipping the root fix without the state fix would convert a silent no-op into actively offering finished chats. 1. Claude was never offered (0/4). The marker recorded agentSessionProviderHandleKey, which embeds Claude's leaf uuid — a branch cursor. The adapter's own close path appends a `resumed` link with an advanced leaf during the SAME teardown, so the marker went stale seconds after it was written and the drift guard refused every Claude session forever. Record and compare agentSessionProviderHandleRoot instead: the root is the part a resume must preserve, and changing it is a fork, which is exactly what this guard is for. Codex is unaffected (its thread id is the whole key) but uses the root too, so the rule is uniform. 2. The predicate compared turn IDENTITY but discarded turn STATE, so a `completed` turn satisfied it as readily as an interrupted one. Eviction rewrites `running` to `interrupted` and never to `completed`, so the state is what separates work that was cut off from work that finished. Require `interrupted` or `unverifiable`. 3. A chat blocked on a pending approval or question was marked as working, because the teardown reader accepted any `running` turn while the product's own projection calls that state `attention`. Teardown now defers to that projection: an agent waiting on the USER is not interrupted work. 4. "Resume all" could silently no-op. The modal fetched candidates at mount; by click time the chat's own pane may have bound and taken the hold, moving the lease to `live` so the predicate dropped it and the call returned no results, leaving the dialog open behind a dead button. Re-derive at click time and settle an already-live session as resumed — it is running, which is what the user asked for. Test fakes now model the Claude close path that advances the leaf, which is why no unit test could previously exhibit defect 1. Ablation covers all eleven guards. * fix(native-chat): gate the already-live settlement on the full resume predicate Two follow-ups from re-QA, both cases of a rule stated by intent rather than by discriminator. 1. The already-live path bypassed the predicate. "Resume all" sends no session ids, so the fallback's target set was every marker, and it was gated only on the session having a live provider child. A chat the predicate had refused -- a completed turn, say -- whose pane happened to own the lease was therefore settled as `already_live` and had its marker spent, inflating the "Resumed N" count with chats that were never eligible. No provider spawned and no tokens were spent, but a marker the predicate rejected must never be consumed. The resumable set now takes an explicit `leaseState`. The already-live path derives a second set with ONLY the released-lease clause relaxed, and settles a session just when it is in that set. Every other clause still applies. 2. The `attention` rule was one-sided. Teardown refuses to mint a marker for a chat blocked on the user, but the set predicate had no equivalent, so a marker arriving by any other route was offered once eviction rewrote its turn to `interrupted` -- the same asymmetry the completed-turn case had. Gated on projectStructuredAgentSessionStatus === 'attention'. That projection tests for a pending approval or question BEFORE it looks at turn state, so it still reports `attention` after the turn is settled, which makes it the durable signal and keeps one source of truth with teardown. Ablation now covers thirteen guards, including one for each of the above. * fix(native-chat): capture awaits-user on the marker instead of re-deriving it The awaits-user clause could never fire. It asked the live projection for `attention`, which needs a prompt whose resolution is still `pending` -- but teardown CANCELS that prompt a few phases after it writes the marker. By the next launch the evidence is gone, for precisely the sessions the clause was written for. QA measured the injection still being offered and then resumed. This is the same shape as the leaf-drift bug: state read after teardown is not the state that justified the marker. The discriminator, now applied across the whole predicate: - a fact teardown itself destroys or mutates must be CAPTURED on the marker while it is still true; - a fact that evolves on its own must be RE-DERIVED at read time, never snapshotted. So `awaitsUser` is now recorded at teardown and the predicate reads the recorded value. Teardown still declines to mint a marker for such a session, so the recorded flag is the second line rather than the only one. Audit of every other clause against the same test: - turn id (captured) -- teardown rewrites turn STATE but never the id. Correct. - provider handle root (captured) -- the close path appends a resumed link, and appendAgentSessionProviderHandleLink refuses one that changes the root, so the root is invariant under exactly the mutation that broke the key. Correct. - turn state (re-derived) -- DELIBERATE exception, stated here rather than left implicit: we are not reading the state that justified the marker, we are reading teardown's receipt that it settled the turn. A turn still `running` means eviction never finished, and we refuse. Correct, and intentionally so. - lease reconciled / released / handoff stage (re-derived) -- these answer a different, launch-time question: may this host take the lease NOW. The teardown-time value would be meaningless, and `unreconciled` is cleared by this launch's own reconciliation. Correct. - adapter support, marker TTL, marker consumption (re-derived) -- all evolve independently of teardown. Correct. Only awaitsUser was on the wrong side. * fix(native-chat): drop the unreachable awaits-user marker flag The captured flag was dead code. `awaitsUser` could only be true when the projected status was `attention`, and `attention` hits the `continue` above the push -- so every marker teardown can ever write carries `false` (QA measured 22 of 22 across two real teardowns). The predicate clause reading it was unreachable by any production path. A flag that is structurally always false is worse than no flag: it reads as a safeguard, so the next person to touch this trusts it. The asymmetry it was added to close was only ever reachable by fault injection, because teardown is the sole writer of markers and already refuses attention sessions. Removing it also drops an upgrade discontinuity: as a required field it made a marker written by the previous build fail validation and be silently discarded, costing a resume offer on precisely the upgrade where the user was mid-turn. Markers predating the providerHandleRoot rename still will not parse, but those carry a leaf-sensitive key the predicate would refuse anyway, so nothing usable is lost. In its place the teardown gate now states that `status !== 'working'` is the SINGLE gate for awaiting-user sessions, why a predicate-side mirror would be unreachable, and why it could not even re-derive the fact -- so the reasoning is inherited rather than rediscovered. Ablation is back to twelve guards; every other clause is unchanged. * fix(native-chat): say reconnect, not resume, and show each offer's age Two changes, both independent of the parked continuation decision. 1. The copy claimed something QA disproved. "Resuming continues each agent where it left off" is false: reconnection restores the session at the point it stopped, with full context and without re-sending the prompt, but the interrupted reply does not continue on its own. The toast's "Resumed N chats" implied work had restarted. Audited every user-facing string against the rule that none may claim work continues or that a reply resumes -- which caught more than the three strings the fix started from. The title, the row button, "Resume all", "Resuming...", the not-now hint ("picks it up where it left off"), the checkbox and its hint ("resume on their own"), the list's aria-label and the Settings row all made the same claim. The user-facing verb is now reconnect throughout; the body and update variant state outright that the interrupted reply will not continue. en.json synced, runtime boot catalog regenerated. If we later decide to send a continuation instruction, this is one commit to change back. Shipping text we know to be false was the worse option. 2. Rows now show each offer's age. The TTL is 24 hours and a stale offer looked identical to a fresh one. The marker already carried `recordedAt`, so this is a render change plus one field on the renderer's candidate type, formatted with the existing formatUiRelativeTime helper rather than a new one. The clock is stamped once when the list arrives rather than read during render: ages then stay stable across re-renders, and the render stays pure, which the react(purity) rule requires. Guards, predicate and RPC are untouched; ablation still covers twelve. * feat(native-chat): show the workspace name on each reconnect row A row read `codex · folder:8f3a1c22-… · 8 hours ago`. Recognising which chats would reconnect is the entire point of the list, and at twenty rows a UUID identifies nothing. No RPC or host change was needed: the renderer can already resolve this id. Resolved the way automation dispatch resolves the same id space (resolveAutomationDispatchWorkspace) -- a folder workspace by its full `folder:<uuid>` key via getKnownWorktreeById, a git worktree by its bare `repoId::path` id via allWorktrees. Both return a Worktree, whose displayName is a required field, and DetectedWorktree extends Worktree so either shape answers. Falls back to the id when nothing resolves, which is what the row showed before and also covers the window before the worktree store has hydrated. The lookup lives in a per-row subcomponent because a hook cannot run inside `map`, and its selector returns a primitive string so repeated selector runs cannot churn referential equality. * feat(native-chat): group the reconnect modal by worktree and add opt-in continuation Grouping. Rows are now grouped under a worktree heading with the repo glyph and an agent count, using the sidebar's own collapse mechanics. Only presentational pieces are reused -- RepoIconGlyph, CompactAgentExpansion, AgentIcon and formatShortTimeAgo. The sidebar's agent row cannot be: worktree-card-compact-agent-row imports DashboardAgentRow, the dashboard's own type, so both surfaces render one live-agent model requiring a pane, tab and status entry. Every chat offered here is by definition stopped, so supplying that would mean inventing live state. Two things I had assumed were reusable and were not: - DashboardHostBadge returns null unless hostKind is ssh or remote. Structured chat is local-only, so it would always render nothing. The host line is omitted rather than faked; the badge is the right element to add if and when structured chat gains remote support. - No state dot. Every AgentDotState misleads here: idle and unverifiable both presuppose a live pane, interrupted renders red like an error, done green, working a spinner. A missing dot beats one saying these agents are running. One worktree renders flat with no heading -- a name, count and chevron around a single group says nothing the dialog has not already said. The age column now uses formatShortTimeAgo for sidebar consistency. It takes (timestamp, now) and subtracts internally rather than taking a delta, so the call is (recordedAt, listedAt); passing the old delta would have rendered plausible nonsense. The clock is still stamped once into state, so ages stay stable and the render stays pure. Continuation. A secondary "Reconnect and continue" action sends one message, from a single shared constant, identical for both providers. Reconnect is unchanged and still sends nothing. An info popover quotes the literal message read from that same constant, so what is shown cannot drift from what is sent. Ablation now covers fourteen guards. Two are new: continuation only follows a reconnect that actually happened, and -- inversely -- a send injected into the reconnect path must turn the test red, since "don't ask again" rests on reconnect never sending. * feat(native-chat): say terminal sessions kept running, and clear the quality gate The modal lists stopped chats with no way to tell that CLI agents are fine, and the true state of the world is counterintuitive: the terminal sessions survived the restart and the chats did not. One line now says so, next to the heading where it frames the list rather than as a footnote at the bottom. Wording follows the app's own vocabulary rather than inventing a term: the catalog settles on "terminal sessions" (terminalSessionCount, "Terminal sessions are grouped by workspace", "No terminal sessions yet"), and UpdateCard already reassures with "Your terminal sessions won't be interrupted during the update" in the same text-xs text-muted-foreground treatment. "kept running" rather than "were restored" -- nothing reconnected them, they never stopped, and the line says nothing about why. Also clears check:code-quality:changed, which I had not been running -- oxlint alone covers neither the design-system nor the casting audit, so 18 findings had accumulated across the branch. - design system (4): Button spacing hand-rolled as gap-1/px-2 is just size="xs"; PopoverContent and DialogTitle own their typography and spacing, so the text-xs moved to the popover's own children and the title's icon gap moved to a plain wrapper. - casting (14): production code loses its assertions outright via Reflect.get, the idiom already used in managed-hook-detection-commands and worktree-name-retirement. The marker validator reads each field through Reflect.get and now checks recordedAt is a number rather than asserting it; the store-file parse uses the existing `file` shape instead of a second assertion; the runner narrows the admission error's owner with typeof. Test fixtures keep their assertions behind the line-specific SAFETY: rationale the repo mandates for exactly this case. One trap worth recording: the audit reports an assertion at the line its EXPRESSION OPENS, not where `as` appears, so a disable-next-line above the closing brace of a multi-line literal is inert and silently changes nothing. Guards unchanged; ablation re-proved 14/14 at this head. * fix(native-chat): give the reconnect row's provider icon an accessible name Every row rendered the provider as a bare AgentIcon, whose svg carries no aria-label, title or alt. With a Claude chat and a Codex chat in one worktree the two rows were identical to any non-visual consumer, and the dialog offered several identically-named "Reconnect" buttons with nothing to tell them apart. A regression from 233e37b, where the row read `${agent} · ${workspace} · …` as text. Moving the workspace name into the group heading was right; dropping the provider to an unlabelled glyph is what lost the information. AgentIcon takes no label prop, so the icon is wrapped the way NativeChatSupportedAgents already names it: a span with role="img" and an aria-label from formatAgentTypeLabel, the same labeller the sidebar and dashboard rows use. The per-row button also names its agent now ("Reconnect Claude chat"). The identical buttons were half the reported harm, and an accessible name that opens with the visible word keeps WCAG 2.5.3 satisfied. Say so if you would rather ship only the icon label -- it is one attribute and one catalog key to drop. Age code untouched, as asked: formatShortTimeAgo still takes (timestamp, now) and is still called with (recordedAt, listedAt). * fix(native-chat): scope resume markers to one launch and report the real dispatch Three defects in the restart-resume path, all of which could resume a session that was not genuinely working or claim one was continued when it was not. Launch scoping. A durable marker with a 24h TTL is a write-ahead latch: a teardown write that failed or timed out, or a store restored from its backup, left a previous generation's marker actionable, and automatic reconnect would have acted on it silently. Markers now carry the id of the launch that wrote them, and only the launch immediately after may claim them. The launch id lives in its own file with no backup mechanism, so it cannot roll back in step with the markers it is proving adjacency for. Startup claims the previous launch's markers into launch-scoped memory and deletes every durable copy in the same step, so the durable fact dies at claim time rather than at use time. Both halves fail closed: an unprovable predecessor and a clear that throws each claim nothing. Dispatch states. The send layer answers ok as soon as Orca owns the message; the provider's own answer lives in the submission. Continuation read only the envelope, so a rejected turn/start was reported as continued and stamped the journal saying the agent had been asked to carry on. All four states are now preserved, and only an accepted dispatch appends the attribution note. Claude pre-echo sends. Claude cannot write a running turn until the SDK echoes the user message back, which is seconds on a real journal, so a turn-id-only marker dropped exactly the sessions that were working hardest. A send that has not become a turn now carries its own identity, and the launch-side predicate asks the journal about that submission's dispatch state instead. * fix(native-chat): follow an accepted send to its turn, and settle before judging Two defects found in QA, both reproduced twice. Follow the submission forward. The launch-side predicate accepted a submission-shaped marker only while its dispatch was pending or unknown, but the window in which work is submission-shaped is precisely the window in which the dispatch is about to be accepted: the send settles during teardown and the turn it opened is then cut off as interrupted. Judgement was frozen at the moment the marker was written, so the predicate refused the very sessions this was built for and fired only when the send never reached the provider. An accepted submission is now followed to the turn it opened -- matched through the user item key a turn names and a submission is aliased by -- and that turn is judged by the existing turn rule. Accepted alone still proves nothing: without the link, or with a turn that completed, this refuses as before. Settle before judging. A send resolves as soon as Orca owns the message, while its dispatch is still pending; that is the ordinary successful path. Reading the dispatch off the send result therefore reported every delivered continuation as pending and never wrote the attribution note. The outcome is now decided on the settled submission, through the host's existing settlement waiter, with the send result as fallback when nothing settles in time. The failed-note path no longer swallows its error. It stays best effort -- a journal that refuses the note must not turn a delivered continuation into a failure -- but the failure is reported through the host's error sink instead of being discarded, so it cannot regress unseen again. The surface's send is typed against the wire result rather than a hand-written subset, which is what let a test assert a shape the host never returns. Binding the surface to the host moves into its own file: the host was one line under the line cap, and the bindings carry decisions that belong beside their consumer. * feat(native-chat): show the reconnect offer the way the worktree sidebar does The offer is a list of workspaces, so it should read like the one users already know. Rows are now three tiers -- repo or project, then workspace, then the agent sessions inside it -- and each agent carries a checkbox rather than its own button, checked by default, with the footer acting on whatever is ticked. Reused rather than rebuilt. The host chip is the sidebar's own: its markup lived inline in the card's meta row, so it moves to a shared component both surfaces render, and the label comes from getHostContextLabel, which is where "Local Mac" has always come from. The repo glyph is RepoIconGlyph; a group with no repo uses the FolderTree the sidebar's own project-group metadata uses. The agent row reuses AgentIcon, the agent-type label helpers, formatShortTimeAgo and the same model treatment. Two things could NOT be reused, and both are deliberate. The sidebar's CompactAgentRow needs a live pane, tab and status entry, and every chat here is stopped by definition. And the sidebar has no git-worktree-vs-folder glyph resolver at all -- both kinds render the same card, and the difference people read is its status lane choosing GitBranch when a workspace has branch identity; that single precedent is what the workspace glyph follows. The model, the execution host and the workspace kind now travel with each offered chat. All three are read off the durable record the predicate already holds -- the model through the same normalizer the status feed uses -- so the glyph is never inferred from a name and no new data source appears. They are optional on the wire, so an older host still renders a row. Selection changes which ELIGIBLE chats are acted on, never what is eligible. Ids are seeded from the host's own answer and intersected back against it before any call, and the host re-derives the predicate regardless of what it is sent. Continuing still requires an explicit click, and the automatic path still calls the reconnect method, which contains no send. The badge's treatment becomes a variant instead of a pile of overrides, which is what the design-system gate asks for once the markup is somewhere it can see it. * fix(native-chat): title a folder workspace group with its project name A folder workspace's synthetic worktree borrows the `repoId` slot to name the project group it belongs to, so that field is NEVER null. The reconnect offer read a non-null `repoId` as proof of a git repo, looked it up in the repos list, found nothing, and rendered the raw `folder-workspace:<uuid>` string as the group header. The project glyph written for the no-repo case was unreachable for the one workspace kind it was meant for, and the string fallback behind it was dead for the same reason. The project group name was available all along and the sidebar already titles these with it, which is what this list is meant to mirror. Recognising the id now lives beside the code that mints it, so the two cannot drift: there was no such helper, only forward constructions of the same prefix in five places. The header choice itself moved into a pure resolver, so the branch that was wrong is now the branch under test. The dead fallback string is gone, along with its catalog entries. * fix(native-chat): offer an accepted send the provider never opened a turn for QA: a chat that was genuinely working was silently dropped from the offer. The discriminator was how far the send had progressed -- it was the last chat prompted before quitting, reachable by quitting a second or two after sending. Mechanism, reproduced against the predicate. The marker was written while the send was still pending, so it is submission-shaped. During teardown the dispatch then settled to `accepted`, which took it out of the pending/unknown branch and into the follow-forward branch. But the provider died before writing a turn row for that send, so there was no turn to follow forward TO, and the branch demanded a proved link before it would answer. Both the no-turn-at-all case and the newest-turn-belongs-to-an-earlier-exchange case therefore refused. An accepted send that never became a turn cannot be finished work, because finishing writes a turn row. The marked send is also the newest work in the session, so any turn it opened would be the newest turn. That makes the link unnecessary to prove for a safe answer. When the newest turn is interrupted or unverifiable the two readings agree: if the row really is this send's under a key we failed to match, it was cut off; if it belongs to an earlier exchange, this send opened no turn at all. Either way the work was interrupted. A journal with no turn row at all is the same case with nothing to disagree about. The readings only diverge on a `completed` row, where an unmatched one might be this very send's finished turn under a key we did not recognise. That stays refused. Ambiguity resolves to no, because resuming finished work is the one outcome never worth risking. * fix: write the grouping separators as escapes so the files stay text Five separators in the reconnect-offer redesign were written as raw NUL bytes instead of the `\0` escape. The runtime strings were correct and the app behaved, but git classifies a file containing a NUL as binary -- so the two central files of that redesign rendered as "Binary file not shown" in review, and `rg` skipped them silently, returning no matches rather than an error. The escape produces the identical string, so the NUL separator is kept: the previous separator was a space, and a workspace id containing one would corrupt the join/split pair this grouping depends on. Nothing could have caught this. Typecheck, lint, the quality gate, the localization verifiers and the full suite all passed throughout, because none of them look at file encoding. So this adds a check that does, wired into the pre-commit hook where it costs nothing and catches the next one at the moment it is written. Two files already on main carry a raw NUL for the same reason -- one a template separator, one a deliberately tricky test alphabet whose neighbours are all written as escapes. They are grandfathered rather than fixed here, since they belong to their own change, and the gate fails if the list ever grows or goes stale. * fix: parse markers into a domain type, and declare the four restart methods Two CI failures, both ours. Static analysis. `Reflect.get` was adopted to clear the casting audit, and the anti-slop rule forbids it -- the two gates disagree, and the rule text says what both want: parse dynamic input into a named type once, then read typed fields off it. Markers re-enter from a file this process may not have written and decide whether an agent is handed a provider child, so they now go through a single zod parse. Unknown keys still pass, and a malformed marker is still dropped rather than thrown, so a bad entry cannot make a user's sessions unreadable. The launch stamp is parsed the same way, the resume-admission refusal becomes a named error carrying a typed `owner` instead of a bag assigned onto `new Error`, and the test harness gets a named journal type instead of reaching into `unknown`. Cross-version wire. The four restart methods are added to the manifest rather than the count being bumped, so the suite now exercises them in both skews. They are bare additions, not capability-negotiated: an unknown RPC method answers `method_not_found`, which is explicit and visible during negotiation, unlike a stream opcode that is dropped in silence. The whole `agentSession.*` surface already sits behind its runtime capability, so an old client is told it does not exist and never reaches a host method. The stub's spies stay a flat map because callers iterate it asserting each entry is a spy that did not run; a composer reassembles the member the host really exposes. The manifest and its params builders move to their own module, which is what keeps the suite under its line cap as the surface grows. * Prevent duplicate restart continuation and release reconnect holds * fix: recheck interrupted work when admitting restart continuation * fix(native-chat): invalidate restart offers after newer user work * Consume restart recovery offers from an isolated advisory capsule * Refuse completed restart work and report recovery outcomes * fix(native-chat): honor queued completion and uncertain restart delivery * fix(native-chat): preserve restart refusal and teardown evidence * fix(native-chat): rederive recovery evidence before continuing * Validate restart continuation at provider dispatch * fix(native-chat): finish restart refusal and attribution delivery * fix(native-chat): keep recovery teardown errors out of logs * fix(native-chat): validate restart continuation at provider dispatch * Revalidate restart continuation when Claude dequeues input Check continuation authority after the SDK input queue wait and arm replay correlation only after authorization. Preserve typed pre-dispatch refusal, ordinary send behavior, and cleanup when the provider exits or capacity fills during authorization. * Deduplicate settlement test import * Keep merge update scoped to restart recovery * Polish continuation popover spacing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ELI5
Before this change, restarting Orca while a native chat was answering left that chat interrupted with no guided recovery. After this change, the next launch can show the affected local chats and let the user reconnect them or explicitly ask them to continue. Reconnecting never sends a message.
What Changed
Why
The original failure is a lifecycle ownership problem: the desktop runtime owns the provider child, so an app restart can preserve conversation history while losing the in-progress reply.
Recovery evidence must have a shorter lifetime than conversation history. Storing it beside backed-up session data can bring stale permission back after a restore. The separate capsule gives the evidence one owner and one bounded lifecycle: shutdown publishes it, startup consumes it, and dismissal, expiry, superseding provider state, or successful take ends it.
This is deliberately fail-closed and advisory. If Orca exits after consuming the capsule but before reconnecting, the offer can be lost; the conversation remains available to open normally. That tradeoff avoids waking an agent from stale evidence. Reconnect and continue remain separate because sending work requires an explicit user action and delivery accounting.
Linked Issue
N/A — this PR has no linked issue.
Visual Proof
Captured from the exact reviewed head in a hidden macOS Electron renderer over CDP. A neutral RPC fixture supplied eligible chats and result states while the production modal, grouping, action handlers, and notification code ran unchanged.
Grouped recovery offer. Three interrupted chats are selected by default across a git workspace and a folder workspace. Reconnect is the primary action; continuing remains explicit.
Exact continuation message. The popover reads the same shared constant sent by the action.
Mixed delivery results. Accepted, refused, and unconfirmed outcomes are reported separately so the UI does not turn uncertainty into success.
Testing
Validated the recovery capsule, teardown ordering, queued provider lifecycle events, concurrent continuation requests, operation settlement failures, folder workspace grouping, renderer result feedback, current/older wire compatibility, and desktop/mobile at-most-once delivery contracts. Fresh final readiness runs passed 428 tests across 37 files, alongside node/web typechecks, changed-code quality, localization, and whitespace checks.
The Electron pass measured one resume call and zero continuation calls for reconnect, reconnect-all, and automatic reconnect. Explicitly continuing one selected chat issued exactly one continuation call. It also checked keyboard focus, accessible names, the exact message text, and mixed result feedback. Host-side eligibility and delivery semantics are covered by deterministic tests; this UI fixture does not prove real provider delivery or a full process restart.
Windows packaging and the cross-version checks run in CI. The feature remains limited to locally hosted structured chats; existing launch resolution excludes SSH, WSL, and paired remote execution. Native mobile UI is unaffected.
AI Disclosure
Review
The final implementation was reviewed repeatedly for functional correctness, lifecycle ownership, delivery semantics, crash boundaries, security, cross-platform behavior, and performance. The architecture uses the existing host journal, acquisition path, and operation ledger instead of adding a parallel provider lifecycle or delivery system.
Agent skill upstream boundary
Notes
Checklist