Repository navigation
fix(native-chat): resume a structured chat from Agent Session History - #18933
Merged
Merged
Conversation
Clicking Resume on a chat-UI row could only reveal an already-open tab. If the chat had been closed, or this process had never published it, the click re-read an inventory that did not contain it and toasted "Retry in a moment" — advice that could never come true, because nothing republishes an unpublished tab. The legacy `claude --resume` fallback is deliberately refused for structured-owned rows, so the row had no way back at all. `close` already keeps the record and the journal on disk so a session can be attached again, and the hold path already resurrects one in full. What was missing was the tab: `restoreReadableSessions` is latched to run once, at startup, so nothing could ask for a single session later. Adds `agentSession.reveal`. The host looks up its own record, restores the session readable, and republishes the tab through the same call `agentSession.create` uses. Deliberately narrow: - It takes no hold. A provider child exists because a surface asked, and the chat pane asks when it binds. - A journal it cannot read is not a refusal. A chat whose journal predates the SQLite store restores to nothing here, but attach still recovers it, so the tab is published and the pane's hold finishes the job. - Workspace and provider come from the record, never the client, so a session id alone cannot aim the publication at another workspace. Claude and Codex both, by construction: eligibility is `adapterSupportsRecord`, which the router answers from the record's own provider. Gated on a new advertised capability rather than probing for method_not_found, matching agent-session.structured.hold.v1 — absence is visible during negotiation instead of by calling.
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughThe change adds on-demand reveal for persisted structured agent sessions. Host restoration now supports single-session reads and returns session metadata with readability status. A new Merge Risk: ⚪ Minimal · up to Structured sessions can now be reopened from history when their tabs are absent, with clear handling for unavailable, unsupported, and missing sessions. The restored-session flow and its recovery paths are covered, with no remaining merge-blocking risk identified. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description gives detailed defect, implementation, compatibility, scope, and verification context. However, it omits most required template sections, including ELI5, Linked Issue with a Resolution Restructure the description using the repository template. Add the missing required sections, provide the linked issue in ✨ 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: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Team
Run ID: b650b528-4ba2-4507-a5a8-0c3398ac8733
📒 Files selected for processing (16)
src/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-readable-restorer.tssrc/main/native-chat/agent-session-wire/structured-agent-session-restart-restore.tssrc/main/native-chat/agent-session-wire/structured-agent-session-reveal.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-reveal.tssrc/main/runtime/mobile-rpc-allowlist.test.tssrc/main/runtime/rpc/methods/structured-agent-session-reveal.tssrc/main/runtime/rpc/methods/structured-agent-session.test.tssrc/main/runtime/rpc/methods/structured-agent-session.tssrc/main/runtime/runtime-rpc/runtime-rpc-mobile-method-allowlist.tssrc/renderer/src/lib/activate-ai-vault-structured-session.test.tssrc/renderer/src/lib/activate-ai-vault-structured-session.tssrc/shared/agent-session-wire.tssrc/shared/protocol-version.tstests/e2e/cross-version-wire/cross-version-agent-session-wire.unit.test.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Reviewed the new agentSession.reveal RPC and the renderer fallback that lets Agent Session History resume a closed or never-published structured chat, plus the readable-restore refactor it rides on.
agentSession.revealhost method — capability-gated RPC (agent-session.structured.reveal.v1) that restores one persisted session readable on demand and republishes its tab from the host's own record, deliberately taking no hold and treating an unreadable journal as non-refusal.- Readable-restore refactor — the startup sweep is split into a per-session
restoreOneStructuredAgentSessionRead, behavior-preserving, with a new unlatchedrestoreOnepath in the restorer for on-demand reveals. - Renderer fallback —
activateAiVaultStructuredSessionnow reveals a missing tab before falling back to a terminalgonetoast, replacing advice that could never come true. - Wire & tests — mobile allowlist, cross-version manifest, and capability ratchets updated; 52 tests cover reveal for both providers, live-first, adapter-unsupported, absent record, unreadable journal, refusal-vs-throw, capability gating, and the renderer paths.
The restoreHandoff ordering in the fresh-restore branch is unchanged from base 062db771, so the startup sweep's behavior is preserved through the refactor.
ℹ️ Nitpicks
revealStructuredSessiongates onreadLocalRuntimeCapabilities()— the local build's advertised capabilities — but dispatchesagentSession.revealtogetActiveRuntimeTarget({ activeRuntimeEnvironmentId: environmentId }), which for a remote-owned worktree is a different Orca process whose build may differ. The gate therefore reflects the client's own build rather than the host's negotiated capabilities; themethod_not_foundcatch incallRuntimeRpcis what actually absorbs an older remote host. No user-visible divergence today (both paths terminate atgone()), just noting the "negotiated rather than discovered" intent isn't fully realized for remote targets.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
added 2 commits
September 5, 2026 18:43
Both sides add to the same additive lists: the runtime capability list, the structured method list, and the cross-version manifest. Kept both, and bumped the structured method count ratchet to 19 for reveal plus the status feed.
…kspace The capability gate read the LOCAL runtime's advertised capabilities while the call went to the host that owns the workspace, which for a paired workspace is a different build. On desktop the renderer and its local host are always the same build, so the gate passed unconditionally and proved nothing about the host being called: an older paired host still received the unknown method and its method_not_found was reported to the user as 'this chat is no longer on this host'. The cache it read also starts empty and resets to empty when status.get fails, so 'not fetched yet' and 'unsupported' were the same value. Gate on the environment that will answer, the way agentSession.close already does, and skip the round trip entirely for a local host. Reveal now reports four outcomes instead of a boolean, so a host that is merely too old is not reported as a chat that is gone, and a host we could not reach keeps the retryable message. Also syncs the localization catalog: the 'gone' key shipped without an en.json entry, which reddens static analysis and verify while typecheck stays green.
Contributor
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
The host raises two refusals here and they mean opposite things to a user: it holds no such record, or it holds one no adapter of its own can open. The client collapsed both into 'this chat is no longer on this host', which is a eulogy for a chat still sitting on disk. Read the refusal code, and fold the host-side case in with the too-old host under one honest message, since the remedy for both is the same. Adds the coverage the readiness pass found missing: the host's reveal answer itself (workspace and provider from the record, both refusals, an unreadable journal, a live session), and the activation branches for a host that cannot open the chat and for one that never answered.
Contributor
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Reviewed the incremental delta that negotiates agentSession.reveal against the host that owns the workspace, resolving the prior capability-gate nitpick.
- Host-scoped capability negotiation —
revealStructuredSessionnow resolves the runtime target from the worktree's owning environment and probesruntimeEnvironmentSupportsCapabilityagainst that host, skipping the round trip for a local host. A paired host's build — not the local build — now decides whetheragentSession.revealexists, so an older host gets a truthful "update the host" instead of amethod_not_foundlaundered into "chat gone". - Four-way reveal outcome — the boolean
revealcontract becamerevealed | gone | host-too-old | unreachable, each mapped to a distinct toast inactivateAiVaultStructuredSession, with a newhostTooOldmessage that no longer tells a user their work is lost when an update would bring it back. - New reveal-focused renderer test —
activate-ai-vault-structured-session-reveal.test.tsexercises local-no-probe, remote negotiation, host-too-old, unreachable, gone, and call-failure against the realgetActiveRuntimeTargetresolution, plus the four-way outcome branching inactivate-ai-vault-structured-session.test.ts.
The mock accurately reflects production transport: callRuntimeRpc resolves with the result payload { ok: false, refusal } on a host refusal (the envelope ok is true), so the gone vs unreachable split is genuinely exercised rather than absorbing refusals into the timeout catch.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
…lost link The capability probe reaches assertRuntimeStatusCompatible, which throws a runtime_compat_block error. Treating that as unreachable told a user with an out-of-date host to retry, which is the one thing that cannot help. Branch on isRuntimeCompatBlockError the way remote-agent-session-launch already does for the same probe. Also adds the refusal-code case a previous commit claimed and did not deliver: nothing drove a structured_agent_session_unsupported reply through the reveal client, which is the branch that commit existed to add. Corrects a doc comment that reveal made wrong: attach is no longer the only call that builds the host.
Contributor
There was a problem hiding this comment.
ℹ️ No critical issues — one minor test-coverage suggestion inline.
Reviewed changes
Reviewed the new commit that tells a refused reveal apart from a missing chat.
- Refusal discrimination —
revealStructuredSessionnow returnsgoneonly foragent_session_identity_requiredandhost-cannot-openfor every other host refusal, and thehost-too-oldoutcome was renamedhost-cannot-openwith a broader "This host can't reopen that chat" message that covers both an outdated host and an adapter that can't handle the record's provider. - Host-level reveal tests —
structured-agent-session-reveal.test.tsgains direct tests ofrevealStructuredAgentSessionasserting the record-owned workspace/provider (not the caller's), no-record refusal, unsupported-adapter refusal, not-readable-without-refusing, and live-session skip. - Renderer toast wiring —
activateAiVaultStructuredSessionmaps the renamed outcome tohostCannotOpen(); the activation and reveal suites were updated for the "update rather than eulogy" and "retryable when unanswered" paths.
The rename and discrimination are correct against the host contract (the host raises exactly agent_session_identity_required and structured_agent_session_unsupported), and the four-way outcome's terminal mapping is coherent.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
…a click Dropping an Agent Session History row onto a pane activated the tab by id and, on a miss, raised the very toast this PR exists to remove — so the same row answered a click and a drop differently, and the drop kept the advice that can never come true. The structured branch never used the drop pane, so routing it through the shared activation loses nothing and gains the reveal. The helper only ever read one field, so its parameter narrows to that field and the drag payload satisfies it directly. A source ratchet holds both entry points to the reveal-capable path, since a mounted drag harness does not exist for this layer and what regresses is a call site, not a rendering.
Contributor
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Reviewed the new commit that reads a host version block as the host's age rather than a lost connection, and the test that closes the prior coverage gap.
- Version-block vs lost-connection split — the capability probe's
catchnow distinguishes a protocol-compat block viaisRuntimeCompatBlockError, mapping a too-old host tohost-cannot-openand a transient/unreachable host tounreachable, following the same splitremote-agent-session-launchuses on the same probe. - Discrimination test coverage —
activate-ai-vault-structured-session-reveal.test.tsgaineddoes not call a host-side refusal a missing chat(resolving the prior nitpick) andreads a version block as a host too old, not as a lost connection. - Gate doc clarification —
ensureStructuredHostInstalled's JSDoc now names reveal alongside attach and hold as a caller that addresses a session by durable record.
The version-block split is correct against the transport: getRuntimeEnvironmentStatus surfaces assertRuntimeStatusCompatible's block as an error tagged runtime_compat_block, while genuine transport/timeout failures carry no such tag, so the two terminal remedies stay honest for the user.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
Contributor
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Reviewed the new commit that lets a dragged history row reach the same reveal as a click.
- Drop path routed through reveal —
AiVaultSessionDropLayernow callsactivateAiVaultStructuredSession(payload)for a structured drop instead of bareactivateStructuredAgentSessionById, so a dropped row can republish a closed tab exactly like clicking Resume (and itsvoid-fire-and-forget matches the click path inai-vault-session-launch-actions.ts). - Parameter narrowing —
activateAiVaultStructuredSessionnow acceptsPick<AiVaultSession, 'structuredSession'>rather than a fullAiVaultSession, since onlystructuredSessionis read, letting the drag payload be passed directly. Structurally compatible with the payload's{ sessionId, workspaceId }shape. - Source ratchet test — a new
describeblock reads both entry-point files and asserts each routes throughactivateAiVaultStructuredSession(and no longer containsactivateStructuredAgentSessionById, guarding against the two surfaces diverging again.
The drop path is consistent with the click path: it uses the payload's own workspace, and the terminal toasts (gone / host-cannot-open / unavailable) are shared.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
…click per row Manual QA found the reveal never ran: the inventory refresh that precedes it is an optimization, but its failure returned early with 'not available yet, retry in a moment' — reinstating the dead end this PR removes, one step earlier. A failed refresh now falls through to the reveal, which is the repair and does not need the refresh to have worked. The click can chain a refresh, a capability probe, a reveal and a second refresh, each with its own timeout, while nothing on the row says it is working. A per-session in-flight guard keeps an impatient second click from running the whole sequence again and landing its own toast. Also drops an unreachable owner scope: the snapshot apply discards any worktree whose execution host is not local before it reads one, so naming a remote scope there described a synchronisation that cannot happen.
Contributor
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Reviewed the new commit that stops a failed inventory refresh from ending the click and dedupes concurrent clicks on the same row.
- Advisory refresh fallthrough — a failed
session.tabs.listrefresh now falls through torevealStructuredSession(viarefreshedWithoutThrowing) instead of returning with a "retry" toast that cannot come true, so a closed chat's tab can still be republished when the refresh is what failed. - Per-session in-flight guard —
activationsInFlightkeyed bysessionIdreturns the already-running activation for an impatient second click, so one row click runs one sequence and lands one toast. - Dead owner-scope removal —
refreshStructuredSessionTabsno longer passes anenvironmentId-derived owner scope toapplyStructuredSessionTabSnapshots; local worktrees resolve to a null environment ID and remote ones are discarded before the scope is read, so the custom scope was never observable.
Two new tests lock both behaviors: one asserts the reveal still runs when the initial refresh rejects, the other asserts three concurrent clicks call reveal and gone exactly once. The owner-scope removal is consistent with every other applyStructuredSessionTabSnapshots call site, which all use the default LOCAL_STRUCTURED_SESSION_OWNER.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
added 2 commits
September 5, 2026 19:23
…g machine The in-flight guard releases when the activation settles, so an await that never settles holds the row for the life of the process. The capability probe was the one call in the chain not raced against a deadline: on a cache hit it awaits a promise an earlier probe created, which may carry no deadline of its own. Race it like the two calls around it. A version block can name either side — evaluateRuntimeCompat reports client-too-old as well as host-too-old — so a message that blamed the host pointed half of those at the wrong machine. Name the remedy instead of the machine, which is true for every case that reaches it.
It was swept into the previous commit by a broad `git add` while a diagnostic ran in this worktree. It asserts the current renderer-sync defect as expected behaviour, so it would fail the moment that defect is fixed.
Contributor
There was a problem hiding this comment.
Important
The reveal path still has a hole: a republished tab can be silently dropped by the renderer's stale-version cursor, and this commit ships a __scratch- test that documents the bug without fixing it (see inline).
Reviewed changes
Reviewed the incremental delta that bounds the reveal's capability probe and stops the refusal toast from naming a machine the version block cannot identify.
- Raced the capability probe against a deadline —
revealStructuredSessionnow wrapsruntimeEnvironmentSupportsCapability(and the reveal RPC) inwithStructuredSessionRestoreTimeout, so a cache-hit probe that never settles releases the per-session in-flight entry after 5s instead of hanging the row for the process lifetime. - De-machined the refusal toast — the
hostCannotOpenmessage no longer names "this host", since aruntime_compat_blockcan name either the client or the host; it names the remedy ("until Orca is updated") instead. - Added a scratch repro for a dropped republish —
__scratch-absorption.test.tscharacterizes the case where a republished chat tab is dropped after the host's storedsnapshotVersionregresses under the same epoch.
ℹ️ Nitpicks
activate-ai-vault-structured-session.ts:74— "This chat can't be reopened until Orca is updated." also covers thestructured_agent_session_unsupportedrefusal, where no update will help (the host simply lacks an adapter for the record's provider); the prior "may need an Orca update" hedge at least admitted that.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
added 2 commits
September 5, 2026 19:34
…s republished tab Manual QA: the host answered reveal with ok:true and republished the tab, and the chat still did not reopen — only a renderer reload brought it back. The renderer publishes under one epoch string for its whole lifetime, and a frame recorded under a different lineage retires that epoch permanently with nothing to un-retire it. The Resume click asks for an inventory first, and a worktree the host holds no entry for answers with the none/v0 sentinel; the structured path recorded it, retiring the renderer's own epoch, so the tab the reveal published a moment later was dropped. A reload minted a new epoch, which is why reloading appeared to fix it. A frame that carries no publication is not a later publication to fence against. Treat the sentinel and a removal frame as a cursor reset, the way the mainstream session-tabs path already clears its tracking — its comment names this exact hazard: recording that sentinel would retire the host epoch and reject the next live frame. Pre-existing, and it swallows an ordinary new-tab launch on an empty worktree too; the reveal is what turned a silent invisibility into a visible failure.
…e epoch Correcting the previous commit. Skipping a retraction frame outright stopped it pruning the mirrored rows, so a worktree the host no longer publishes would have kept a chat on screen with nothing behind it. Apply the frame as before and clear its cursors instead of recording them, which is what the mainstream session-tabs path does. The unpublished sentinel keeps its cursor now too: it is skipped rather than cleared, so a stale frame arriving late is still fenced. Adds the case the earlier version would have broken.
Contributor
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Reviewed the incremental delta that closes the republish-drop defect the prior review flagged in applyLocalStructuredSessionTabSnapshots.
- Removed the scratch repro —
__scratch-absorption.test.ts, which documented the drop as passing behavior, is deleted. - Fixed the stale-version cursor drop — the structured snapshot path now skips the unpublished-worktree sentinel (
none/v0) instead of recording it, and clears its cursors on aremovedretraction frame rather than retiring the epoch, so a reveal's republished tab reaches the store. - Replaced the repro with a regression suite —
local-structured-session-reveal-visibility.test.tsasserts sentinel survival, rebuilt-host-entry survival, retraction pruning, and that a genuinely superseded republication is still fenced.
The fix reuses the shared hostSnapshotAffirmsWorktreeContents authority and mirrors the mainstream session-tabs path's cursor handling. The four tests assert exact outcomes (not loose truthiness), so they fail if either fence or the pruning regresses.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
added 4 commits
September 5, 2026 19:54
…s refresh Correcting the retraction handling again. Clearing its cursors was more than the bug needed and cost a guard: the host mints a fresh epoch when it rebuilds a pruned entry, so a republication is never gated by the retained cursor, while dropping it left an inventory response issued before the close free to land afterwards and strand a chat row for a worktree the host no longer publishes. Skip only the recording. The mainstream path keeps its epoch history for the same reason, as a tombstone fence. The test that justified the stronger clearing asserted a host behaviour that does not exist — a rebuilt entry republishing under the renderer's epoch with a restarted counter. It now uses what publishStructuredAgentSessionTab actually mints for a pruned entry, and a new case covers the frame that would strand. Also fences the reveal's inventory refresh on the sync generation, which every other caller that applies an inventory already does: structured chat can be switched off mid-flight, and the answer would otherwise re-seed a row into a renderer that just discarded them.
…on cursor Third and final shape for this branch, and the only one of the three that holds. Keeping both maps re-poisons the epoch one cycle later: the consumer here is also the publisher, so the history's current is the renderer's own lifetime epoch, and recording the reveal's fresh epoch retires it. The next chat the renderer publishes is then dropped — this bug again, one close later. Deleting both loses the guard that stops a frame issued before the close landing after it and stranding a row nothing republishes. So: clear the history, keep the cursor. The mainstream path keeps its history as a tombstone because there the epochs belong to a remote publisher; that reasoning does not carry to a path that publishes under its own. Each of the three variants now fails a different test.
…tones The delete lost a fence the cursor cannot replace: the version cursor only compares within a lineage, so a delayed frame from an already-superseded epoch had nothing left to stop it putting a chat row back for a worktree the host no longer publishes. Keeping the record intact had the opposite fault — the renderer's own epoch is the history's current, so the next frame under any other epoch retired it. Clearing only current does neither: noteRetiredValue retires nothing when there is nothing current, and the tombstones stay. Each of the four shapes now fails a different test.
Typecheck caught what the tests could not: `removed` is not on RuntimeMobileSessionTabsResult. The repo already names the shape — RuntimeMobileSessionTabsRemovedResult — so this reads it through a guard rather than the inline cast the mainstream path uses.
Contributor
There was a problem hiding this comment.
Important
Retaining the epoch history across a retraction can fence the renderer's own publications after a reveal republishes under a fresh epoch (see inline). The cursor should be kept, but the epoch history likely should not.
Reviewed changes
Reviewed the incremental delta that keeps the retraction's fences and fences the reveal's own inventory refresh.
- Retraction now retains its cursor —
applyLocalStructuredSessionTabSnapshotsno longer deletes the worktree's publication cursor on aremoved === trueframe. This closes the in-flight-stranding hole (asession.tabs.listframe issued before the close, same lineage,snapshotVersion <= prior, is still fenced), and is safe for rebuilt entries because the host deletes itsmobileSessionTabsByWorktreeentry before emitting the retraction, sopublishStructuredAgentSessionTabmints a freshstructured:<ts>epoch on rebuild. - Retraction now also retains its epoch history — the retraction
continues without deletinglocalStructuredSessionEpochHistoryByWorktree. The cursor half of this is justified; the history half is the concern flagged inline. - Reveal refresh fenced on sync generation —
refreshStructuredSessionTabscaptureslocalStructuredSessionGeneration()before the RPC and drops the answer if the generation advanced, so a structured-chat toggle-off mid-flight can't re-seed a discarded row. Mirrorsinventory-refresh.tsandsubscription.ts. - Test model corrected and extended — the rebuild case now uses a fresh epoch (
structured:mf3k1) and a newdoes not let a frame from before the retraction strand a rowtest locks the cursor fix.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
Contributor
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Reviewed the incremental delta that resolves the prior finding by dropping the epoch history's current on a retraction while keeping its tombstones.
- Retraction forgets
current, keeps tombstones —applyLocalStructuredSessionTabSnapshotsnow sets the epoch history'scurrenttonullon aremoved === trueframe, preserving theretiredlist. This means a reveal's fresh-epoch republish no longer retires the renderer's own lifetime epoch (so a laterrenderer:<uuid>publication still applies), while delayed frames from an already-superseded epoch remain fenced. This closes the fence-the-renderer's-own-epochs concern from the prior review. - Retraction narrowed through a type guard —
isWorktreeRetractionnarrowsremoved === truetoRuntimeMobileSessionTabsRemovedResult, making the retraction branch's intent explicit. - Two regression tests lock both halves —
does not retire the renderer's own epoch when a reveal republishes under a new oneandstill fences a delayed frame from an epoch that was already supersededassert exact outcomes for both the cleared-currentbehavior and the preserved-tombstone behavior.
The current: null transition is correct against noteRetiredValue (which retires nothing when there is nothing current), and the tombstone retention keeps the cross-lineage fence that the version cursor alone cannot provide.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
Contributor
Status: reviewed, fixed, and verified in the app — holding on CIHead is What manual QA found, and what it looks like nowThe first run reproduced the exact bug this PR set out to remove — the row toasted "not available yet. Retry in a moment" and the chat never came back, even though
The tab now reopens with its transcript and no toast:
And it survives a second close/Resume cycle, which is where a regression introduced mid-review was caught:
Every run was driven over CDP against source proven served by the dev server, on a clean isolated profile. Why the original change could not workThe renderer publishes under a single epoch string for its whole process lifetime. A frame recorded under a different lineage retires that epoch permanently, and nothing un-retires it. Two frames did that here, and the first is the unkind one: the Resume click poisoned its own reveal. It asks the host for an inventory first; a worktree the host holds no entry for answers with the This is pre-existing and not caused by this PR — it also swallows an ordinary New tab → Codex launch on an empty worktree. The reveal only made it visible, by turning a silent invisibility into Changes made on top of the original PRMaking the feature work
Correctness of the reveal itself
Housekeeping: missing Scope, flagged explicitly
Known and deliberately not fixed
Verification
|
Main landed #18906, which repairs the same epoch-fencing failure from the other end: when the fence drops a frame it now schedules one bounded authoritative census, and only that census may revive an epoch. This branch prevents the wrongful retirement instead — a sentinel or a retraction is no longer recorded as the current publication. They compose. Prevention keeps the common case from ever reaching the fence, so no census round trip and no window where the chat is invisible; the repair lane still covers a retirement this branch does not prevent, such as a publisher returning under an epoch it inherited. Kept both sides of the one textual conflict: the retraction type guard and the census options.
Contributor
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Reviewed the incremental delta since the prior review — a single merge main commit (64e040d0a) with no new feature work.
- Merged
maininto the branch — reconciledsnapshot-apply.tswith main's newly-landed retired-epoch repair lane:applyLocalStructuredSessionTabSnapshots/applyStructuredSessionTabSnapshotsgain anoptionsparam (authoritative,onRetiredEpochDrop), the retired-epoch fence now schedules an authoritativesession.tabs.listAllrepair instead of dropping silently, and dead-worktree repair state is swept viaforgetRetiredEpochRepairsOutside. - Reveal-visibility fix intact — the PR's sentinel-skip (
hostSnapshotAffirmsWorktreeContents) and retraction handling (current: null+ kept tombstones) sit unchanged on top of that lane.hasRetiredValueis semantically identical to the previous inlineepochHistory?.retired.includes(...), so fence behavior is unchanged for the reveal path. - Formatting-only drift in four unrelated test files —
relay-asset-line-ending-pin.test.mjs,artifact-create-intent-store.test.ts,ssh-remote-powershell.test.ts, andwindows-process-table.test.tspicked up prettier reformatting via the merge; no behavioral change.
Verified the reconciliation is behavior-preserving and ran the two touched renderer suites (local-structured-session-reveal-visibility.test.ts, retired-epoch-repair.test.ts) — 12/12 pass.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
8 of 9 tasks
enrick-dev
pushed a commit
to enrick-dev/orca
that referenced
this pull request
Oct 15, 2026
…stablyai#18933) * fix(native-chat): resume a structured chat from Agent Session History Clicking Resume on a chat-UI row could only reveal an already-open tab. If the chat had been closed, or this process had never published it, the click re-read an inventory that did not contain it and toasted "Retry in a moment" — advice that could never come true, because nothing republishes an unpublished tab. The legacy `claude --resume` fallback is deliberately refused for structured-owned rows, so the row had no way back at all. `close` already keeps the record and the journal on disk so a session can be attached again, and the hold path already resurrects one in full. What was missing was the tab: `restoreReadableSessions` is latched to run once, at startup, so nothing could ask for a single session later. Adds `agentSession.reveal`. The host looks up its own record, restores the session readable, and republishes the tab through the same call `agentSession.create` uses. Deliberately narrow: - It takes no hold. A provider child exists because a surface asked, and the chat pane asks when it binds. - A journal it cannot read is not a refusal. A chat whose journal predates the SQLite store restores to nothing here, but attach still recovers it, so the tab is published and the pane's hold finishes the job. - Workspace and provider come from the record, never the client, so a session id alone cannot aim the publication at another workspace. Claude and Codex both, by construction: eligibility is `adapterSupportsRecord`, which the router answers from the record's own provider. Gated on a new advertised capability rather than probing for method_not_found, matching agent-session.structured.hold.v1 — absence is visible during negotiation instead of by calling. * fix(native-chat): negotiate reveal against the host that owns the workspace The capability gate read the LOCAL runtime's advertised capabilities while the call went to the host that owns the workspace, which for a paired workspace is a different build. On desktop the renderer and its local host are always the same build, so the gate passed unconditionally and proved nothing about the host being called: an older paired host still received the unknown method and its method_not_found was reported to the user as 'this chat is no longer on this host'. The cache it read also starts empty and resets to empty when status.get fails, so 'not fetched yet' and 'unsupported' were the same value. Gate on the environment that will answer, the way agentSession.close already does, and skip the round trip entirely for a local host. Reveal now reports four outcomes instead of a boolean, so a host that is merely too old is not reported as a chat that is gone, and a host we could not reach keeps the retryable message. Also syncs the localization catalog: the 'gone' key shipped without an en.json entry, which reddens static analysis and verify while typecheck stays green. * fix(native-chat): tell a refused reveal apart from a missing chat The host raises two refusals here and they mean opposite things to a user: it holds no such record, or it holds one no adapter of its own can open. The client collapsed both into 'this chat is no longer on this host', which is a eulogy for a chat still sitting on disk. Read the refusal code, and fold the host-side case in with the too-old host under one honest message, since the remedy for both is the same. Adds the coverage the readiness pass found missing: the host's reveal answer itself (workspace and provider from the record, both refusals, an unreadable journal, a live session), and the activation branches for a host that cannot open the chat and for one that never answered. * fix(native-chat): read a host version block as the host's age, not a lost link The capability probe reaches assertRuntimeStatusCompatible, which throws a runtime_compat_block error. Treating that as unreachable told a user with an out-of-date host to retry, which is the one thing that cannot help. Branch on isRuntimeCompatBlockError the way remote-agent-session-launch already does for the same probe. Also adds the refusal-code case a previous commit claimed and did not deliver: nothing drove a structured_agent_session_unsupported reply through the reveal client, which is the branch that commit existed to add. Corrects a doc comment that reveal made wrong: attach is no longer the only call that builds the host. * fix(native-chat): let a dragged history row reach the same reveal as a click Dropping an Agent Session History row onto a pane activated the tab by id and, on a miss, raised the very toast this PR exists to remove — so the same row answered a click and a drop differently, and the drop kept the advice that can never come true. The structured branch never used the drop pane, so routing it through the shared activation loses nothing and gains the reveal. The helper only ever read one field, so its parameter narrows to that field and the drag payload satisfies it directly. A source ratchet holds both entry points to the reveal-capable path, since a mounted drag harness does not exist for this layer and what regresses is a call site, not a rendering. * fix(native-chat): stop an advisory refresh ending the click, and one click per row Manual QA found the reveal never ran: the inventory refresh that precedes it is an optimization, but its failure returned early with 'not available yet, retry in a moment' — reinstating the dead end this PR removes, one step earlier. A failed refresh now falls through to the reveal, which is the repair and does not need the refresh to have worked. The click can chain a refresh, a capability probe, a reveal and a second refresh, each with its own timeout, while nothing on the row says it is working. A per-session in-flight guard keeps an impatient second click from running the whole sequence again and landing its own toast. Also drops an unreachable owner scope: the snapshot apply discards any worktree whose execution host is not local before it reads one, so naming a remote scope there described a synchronisation that cannot happen. * fix(native-chat): bound the capability probe and stop naming the wrong machine The in-flight guard releases when the activation settles, so an await that never settles holds the row for the life of the process. The capability probe was the one call in the chain not raced against a deadline: on a cache hit it awaits a promise an earlier probe created, which may carry no deadline of its own. Race it like the two calls around it. A version block can name either side — evaluateRuntimeCompat reports client-too-old as well as host-too-old — so a message that blamed the host pointed half of those at the wrong machine. Name the remedy instead of the machine, which is true for every case that reaches it. * chore: remove a scratch repro file committed by mistake It was swept into the previous commit by a broad `git add` while a diagnostic ran in this worktree. It asserts the current renderer-sync defect as expected behaviour, so it would fail the moment that defect is fixed. * fix(native-chat): stop a reveal's own inventory refresh discarding its republished tab Manual QA: the host answered reveal with ok:true and republished the tab, and the chat still did not reopen — only a renderer reload brought it back. The renderer publishes under one epoch string for its whole lifetime, and a frame recorded under a different lineage retires that epoch permanently with nothing to un-retire it. The Resume click asks for an inventory first, and a worktree the host holds no entry for answers with the none/v0 sentinel; the structured path recorded it, retiring the renderer's own epoch, so the tab the reveal published a moment later was dropped. A reload minted a new epoch, which is why reloading appeared to fix it. A frame that carries no publication is not a later publication to fence against. Treat the sentinel and a removal frame as a cursor reset, the way the mainstream session-tabs path already clears its tracking — its comment names this exact hazard: recording that sentinel would retire the host epoch and reject the next live frame. Pre-existing, and it swallows an ordinary new-tab launch on an empty worktree too; the reveal is what turned a silent invisibility into a visible failure. * fix(native-chat): let a retraction prune its rows without retiring the epoch Correcting the previous commit. Skipping a retraction frame outright stopped it pruning the mirrored rows, so a worktree the host no longer publishes would have kept a chat on screen with nothing behind it. Apply the frame as before and clear its cursors instead of recording them, which is what the mainstream session-tabs path does. The unpublished sentinel keeps its cursor now too: it is skipped rather than cleared, so a stale frame arriving late is still fenced. Adds the case the earlier version would have broken. * fix(native-chat): keep the retraction's fences, and fence the reveal's refresh Correcting the retraction handling again. Clearing its cursors was more than the bug needed and cost a guard: the host mints a fresh epoch when it rebuilds a pruned entry, so a republication is never gated by the retained cursor, while dropping it left an inventory response issued before the close free to land afterwards and strand a chat row for a worktree the host no longer publishes. Skip only the recording. The mainstream path keeps its epoch history for the same reason, as a tombstone fence. The test that justified the stronger clearing asserted a host behaviour that does not exist — a rebuilt entry republishing under the renderer's epoch with a restarted counter. It now uses what publishStructuredAgentSessionTab actually mints for a pruned entry, and a new case covers the frame that would strand. Also fences the reveal's inventory refresh on the sync generation, which every other caller that applies an inventory already does: structured chat can be switched off mid-flight, and the answer would otherwise re-seed a row into a renderer that just discarded them. * fix(native-chat): drop the retraction's epoch history, keep its version cursor Third and final shape for this branch, and the only one of the three that holds. Keeping both maps re-poisons the epoch one cycle later: the consumer here is also the publisher, so the history's current is the renderer's own lifetime epoch, and recording the reveal's fresh epoch retires it. The next chat the renderer publishes is then dropped — this bug again, one close later. Deleting both loses the guard that stops a frame issued before the close landing after it and stranding a row nothing republishes. So: clear the history, keep the cursor. The mainstream path keeps its history as a tombstone because there the epochs belong to a remote publisher; that reasoning does not carry to a path that publishes under its own. Each of the three variants now fails a different test. * fix(native-chat): a retraction forgets what is current, not the tombstones The delete lost a fence the cursor cannot replace: the version cursor only compares within a lineage, so a delayed frame from an already-superseded epoch had nothing left to stop it putting a chat row back for a worktree the host no longer publishes. Keeping the record intact had the opposite fault — the renderer's own epoch is the history's current, so the next frame under any other epoch retired it. Clearing only current does neither: noteRetiredValue retires nothing when there is nothing current, and the tombstones stay. Each of the four shapes now fails a different test. * fix(native-chat): narrow the retraction frame through its own type Typecheck caught what the tests could not: `removed` is not on RuntimeMobileSessionTabsResult. The repo already names the shape — RuntimeMobileSessionTabsRemovedResult — so this reads it through a guard rather than the inline cast the mainstream path uses. --------- Co-authored-by: Orca Worker <orca-worker@localhost> Co-authored-by: Merge Sim <sim@local>
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.
The defect
Clicking Resume on a chat-UI row in Agent Session History could only reveal an already-open tab. If the chat had been closed, or this process had never published it, the click re-read an inventory that did not contain it and toasted "The structured agent session is not available yet. Retry in a moment." — advice that can never come true, because nothing republishes an unpublished tab.
ai-vault-session-launch-actions.ts:88routes structured rows toactivateAiVaultStructuredSession, which activates a tab, refreshessession.tabs.liston a miss, tries once more, then gives up. There was no third step.The fallback is deliberately closed for these rows too:
assertLegacyAiVaultResumeAllowedrefusesclaude --resume/codex resumefor any structured-owned session (#15161). So the row had no way back at all.What was actually missing
Less than it looks.
agentSession.closealready keeps the record and journal on disk "so the same session can be attached again", and the hold path already resurrects a closed session in full —attachStructuredAgentSessionnever requires a live map entry andresumeHeldStructuredAgentSessionreads the durable record.The missing piece was the tab.
restoreReadableSessionsis latched (restorePromise ??=, wrapped again inStructuredAgentSessionRestartRestoreGate) so it runs once per process at startup, andapply-preparation-browser.ts:172drops everyagent-sessiontab on each sync, re-adding only what the host published. Nothing could ask for one session later.The change
agentSession.reveal— the host looks up its own record, restores the session readable, and republishes the tab through the samepublishStructuredAgentSessionTabcallagentSession.createalready uses.Three deliberate narrowings, each of which could plausibly have gone the other way:
structured-agent-session-holds.ts:1-11). "Visible tab, readable, no child" is already the normal state for every startup-restored tab. Holding from the RPC would leave a child running with no surface until the release grace — the regression the holds design removed.openAgentSessionJournalWithRecovery, so the tab is published and the pane's hold finishes the job. Refusing would strand exactly those chats forever.Claude and Codex both, by construction: eligibility is
adapterSupportsRecord, which the router answers fromrecord.provider; the reveal result carries that provider through to the publish. Tested for both.Wire compatibility
Gated on a new advertised capability,
agent-session.structured.reveal.v1, matching the precedent set byagent-session.structured.hold.v1— absence is visible during negotiation rather than discovered by calling, which is whatdocs/reference/remote-wire-compatibility.mdasks for.method_not_foundremains a defensive backstop in the client. Additive method, no protocol bump. Added to the mobile allowlist; mobile has the identical gap today (MobileAgentSessionHistoryPaneldrives the legacy prepare that the host then refuses) and can adopt the same RPC.Three existing count ratchets were updated deliberately: the cross-version manifest, the mobile allowlist exact-list, and
STRUCTURED_AGENT_SESSION_METHODS.toHaveLength.Two files were split rather than suppressed
structured-agent-session-host.tssat exactly at the 300-line ceiling, so its readable-restore surface moved intostructured-agent-session-reveal.ts(mirroring-host-handoff/-host-lifetime), and the RPC method moved into its own file (mirroringstructured-agent-session-hold.ts). Nomax-linesdisable.Not fixed here
Verification
tscclean on both the node and web projects;oxlintclean.EBUSY … journal.dbon temp cleanup, from refactor(agent-session-journal): move the session journal onto SQLite #18652's SQLite move). Verified identical on the base commit062db771— same 4 tests, 9 passed either side.