Repository navigation
fix(native-chat): preserve structured chat across rollback - #17439
Conversation
Greptile SummaryThe PR adds a durable visible-session identity index so structured Codex chats can survive profile rewrites by older releases.
Confidence Score: 4/5The PR is not yet safe to merge because an interrupted startup can leave a partial durable index that hides otherwise recoverable structured chats on subsequent launches. The startup path still treats any present durable visibility index as authoritative, while visibility entries are committed one session at a time during sequential publication; a failure after an earlier commit leaves later profile-backed sessions outside the index and therefore excluded from the next restoration. Files Needing Attention: src/main/runtime/orca-runtime.ts
|
| Filename | Overview |
|---|---|
| src/main/runtime/orca-runtime.ts | Selects the durable visibility index during startup and persists visibility around tab publication and closure; the previously reported partial-index restoration defect remains. |
| src/main/runtime/agent-session-record-store.ts | Adds transactional APIs for reading and updating durable visible-session identities. |
| src/main/runtime/agent-session-record-store-file.ts | Loads and saves the new optional visibility index within the durable session store. |
| src/main/runtime/agent-session-store-transaction-queue.ts | Extends transaction change detection and rollback snapshots to include visibility state. |
| src/main/runtime/rpc/methods/structured-agent-session.ts | Awaits visibility persistence during session publication and clears visibility after close. |
| src/main/native-chat/agent-session-wire/structured-agent-session-handoff-reverse.ts | Awaits native-session reveal so persistence failures propagate through handoff. |
| src/main/runtime/structured-agent-session-rollback-compatibility.test.ts | Exercises durable identity survival across target-to-base-to-target profile compatibility transitions. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
P[Workspace profile session IDs] --> R[Startup restoration]
I[Durable visible-session index] --> R
R --> S[Restore readable sessions]
S --> T[Publish structured tabs]
T --> I
C[Close structured chat] --> D[Remove durable visibility reference]
D --> I
Reviews (2): Last reviewed commit: "fix(native-chat): preserve rollback visi..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| : [] | ||
| const profileIds = collectSavedStructuredAgentSessionIds( | ||
| this.store?.getWorkspaceSession?.(LOCAL_EXECUTION_HOST_ID) ?? null | ||
| ) |
There was a problem hiding this comment.
When startup restores legacy profile-backed sessions and publication fails after persisting only some visibility entries, the next startup selects that incomplete durable index instead of merging it with profileIds, causing the omitted structured chats to disappear even though their records and profile tabs remain durable.
Knowledge Base Used:
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (12)
🚧 Files skipped from review as they are similar to previous changes (6)
Included review availability: Your plan provides up to 10 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughThe agent-session store now persists visible session IDs with schema-aware parsing, conditional serialization, change detection, and transactional rollback. The session host exposes the visibility index and updates. Runtime publication and closure await visibility changes. Native-session handoff and RPC publication await asynchronous operations. Restoration uses persisted visible session IDs when the index is present. Tests cover empty indexes, RPC closure, and rollback round trips. Merge Risk: 🟡 Moderate · up to Closing a structured chat can currently hide a session using a caller-supplied ID without an established ownership check, and one failed visibility write during startup can prevent the remaining chats from being restored for that launch. The underlying records remain retained, but these bounded security and availability risks should be addressed or explicitly accepted before merging. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description explains the root cause, implementation, regression coverage, and validation. However, it omits required template sections, including the linked issue, visual proof or an N/A explanation, testing checkboxes, AI disclosure, review, boundary, notes, and checklist. Resolution Add the missing template sections. Include a valid linked issue after “Fixes #”. For this behavior change, attach before-and-after visual proof or write “N/A” with a reason. Complete the testing, review, boundary, notes, and checklist sections. Full details: Docstring CoverageExplanation Docstring coverage is 13.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 15 functions across 13 files. (1 skipped: 1 too large.)
✨ 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}
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 81078d31-bbab-4359-94ee-f51355ad0ed4
📒 Files selected for processing (13)
src/main/native-chat/agent-session-wire/structured-agent-session-handoff-reverse.tssrc/main/native-chat/agent-session-wire/structured-agent-session-handoff-types.tssrc/main/native-chat/agent-session-wire/structured-agent-session-host.tssrc/main/runtime/agent-session-record-store-file.tssrc/main/runtime/agent-session-record-store.tssrc/main/runtime/agent-session-recovery-publish-fault.test.tssrc/main/runtime/agent-session-store-serialization.tssrc/main/runtime/agent-session-store-transaction-queue.tssrc/main/runtime/agent-session-visible-tab-index.tssrc/main/runtime/orca-runtime-structured-session-restore.test.tssrc/main/runtime/orca-runtime.tssrc/main/runtime/rpc/methods/structured-agent-session.tssrc/main/runtime/structured-agent-session-rollback-compatibility.test.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 6 remain after this review.
| private async closeStructuredAgentSessionTab( | ||
| worktreeId: string, | ||
| snapshot: RuntimeMobileSessionTabsSnapshot, | ||
| tab: RuntimeMobileSessionAgentTab | ||
| ): void { | ||
| ): Promise<void> { | ||
| const host = getStructuredAgentSessionHost() | ||
| if (typeof host?.setSessionTabVisibility === 'function') { | ||
| await host.setSessionTabVisibility(tab.sessionId, false) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- repository conventions and learnings ---'
find /tmp/coderabbit-repo-knowledge/stablyai-orca-89dc44e4 -maxdepth 2 -type f -name '*.md' -print
printf '%s\n' '--- relevant source symbols ---'
rg -n -C 8 \
'closeMobileSessionTab|closeStructuredAgentSessionTab|setSessionTabVisibility|restoreStructuredAgentSessionTabsOnce|listPersistedVisibleSessionIds|collectSavedStructuredAgentSessionIds|publishStructuredAgentSessionTab' \
src/main/runtime/orca-runtime.ts srcRepository: stablyai/orca
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- applicable conventions ---'
cat /tmp/coderabbit-repo-knowledge/stablyai-orca-89dc44e4/conventions/repo-wide.md
cat /tmp/coderabbit-repo-knowledge/stablyai-orca-89dc44e4/conventions/src.md
cat /tmp/coderabbit-repo-knowledge/stablyai-orca-89dc44e4/conventions/mobile.md
printf '%s\n' '--- runtime learnings ---'
cat /tmp/coderabbit-repo-knowledge/stablyai-orca-89dc44e4/learnings/src-main-runtime.md
printf '%s\n' '--- close caller and helper ---'
sed -n '9883,10145p' src/main/runtime/orca-runtime.ts
printf '%s\n' '--- visibility store implementation ---'
sed -n '1,180p' src/main/runtime/agent-session-record-store.ts
printf '%s\n' '--- host binding and type ---'
sed -n '1,220p' src/main/native-chat/agent-session-wire/structured-agent-session-host.ts
rg -n -C 5 'getStructuredAgentSessionHost|StructuredAgentSessionHost' src/mainRepository: stablyai/orca
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- transaction failure semantics ---'
rg -n -C 12 'class AgentSessionStoreTransactionQueue|transact\(|persist|readOnly|setSessionTabVisibility' \
src/main/runtime/agent-session-store-transaction-queue.ts \
src/main/runtime/agent-session-record-store.ts \
src/main/runtime/*test.ts
printf '%s\n' '--- structured-tab close tests and failure cases ---'
rg -n -C 12 'agent-session|structured.*close|visibility|setSessionTabVisibility|closeMobileSessionTab' \
src/main/runtime/orca-runtime.test.ts \
src/main/runtime/*test.ts | head -n 1200
printf '%s\n' '--- host accessor and notifier contract ---'
rg -n -C 8 'function getStructuredAgentSessionHost|const getStructuredAgentSessionHost|export .*getStructuredAgentSessionHost|closeSessionTab:' \
src/mainRepository: stablyai/orca
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- transaction queue files ---'
fd -i 'transaction.*queue|agent-session.*store.*test|orca-runtime.test' src/main/runtime
printf '%s\n' '--- transaction queue implementation ---'
sed -n '1,260p' src/main/runtime/agent-session-store-transaction-queue.ts
printf '%s\n' '--- exact structured-session close test matches ---'
rg -n -i -C 6 'structured agent session|agent session tab|session tab visibility|setSessionTabVisibility' \
src/main/runtime/orca-runtime.test.ts \
src/main/native-chat/agent-session-wire \
src/main/runtime/agent-session-record-store*.test.ts
printf '%s\n' '--- host accessor definitions ---'
rg -n -C 10 'getStructuredAgentSessionHost' src/main/runtime/orca-runtime.ts src/main/native-chatRepository: stablyai/orca
Length of output: 20101
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- close-session notifier contract and implementations ---'
rg -n -C 12 'closeSessionTab|setNotifier\(' src/main/runtime/orca-runtime.ts src/main
printf '%s\n' '--- compatibility and failure-policy comments ---'
rg -n -i -C 8 'compatibility-write|compatibility.*fail|write.*fail|durable.*fail|retry.*close|close.*retry|visibility' \
src/main/runtime src/main/native-chat/agent-session-wire \
--glob '*.ts' --glob '*.md' | head -n 1600
printf '%s\n' '--- focused runtime tests near structured-session support ---'
rg -n 'setStructuredAgentSessionHost|StructuredAgentSessionHost|publishStructuredAgentSessionTab|structuredAgentSessionTab' \
src/main/runtime/orca-runtime.test.ts src/main/runtime/*.test.tsRepository: stablyai/orca
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- focused structured-session restore tests ---'
sed -n '1,330p' src/main/runtime/orca-runtime-structured-session-restore.test.ts
printf '%s\n' '--- relevant runtime comments and persistence calls ---'
rg -n -C 10 'compatibility|durable|visibility|publishStructuredAgentSessionTab|closeStructuredAgentSessionTab' \
src/main/runtime/orca-runtime.ts
printf '%s\n' '--- persistence failure tests ---'
rg -n -C 8 'saveAgentSessionStore|mockRejected|reject|readOnly|transaction' \
src/main/runtime/agent-session-record-store.test.ts \
src/main/runtime/agent-session-record-store-security.test.ts \
src/main/runtime/agent-session-store-transaction-queue.tsRepository: stablyai/orca
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- non-test closeSessionTab implementations ---'
rg -n -C 10 'closeSessionTab' src --glob '*.ts' --glob '*.tsx' --glob '!*.test.ts'
printf '%s\n' '--- durable-commit ordering precedent ---'
sed -n '32455,32478p' src/main/runtime/orca-runtime.ts
printf '%s\n' '--- exact transaction rollback lines ---'
sed -n '72,145p' src/main/runtime/agent-session-store-transaction-queue.tsRepository: stablyai/orca
Length of output: 18528
Commit durable visibility before closing the renderer tab
When setSessionTabVisibility fails, closeMobileSessionTab has already closed the renderer tab, but closeStructuredAgentSessionTab has not removed the runtime tab. AgentSessionStoreTransactionQueue also restores visibleSessionIds when the transaction fails. The next restore can therefore republish the closed tab. Commit the durable hide before the renderer close, or reconcile the close after a failed write.
| sessionId = sessionId.slice('agent-session:'.length) | ||
| } | ||
| this.publishStructuredAgentSessionTab({ | ||
| await this.publishStructuredAgentSessionTab({ |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Isolate per-session failures in the startup restore loop.
This call now awaits publishStructuredAgentSessionTab, which itself awaits a durable write (host.setSessionTabVisibility). The enclosing for loop has no try/catch. If the write rejects for one session, the rejection propagates out of restoreStructuredAgentSessionTabsOnce, and every subsequent session in host.listSessionTabs() is skipped for the rest of this startup.
A single failing write during startup restoration should not block restoring every other structured chat session. Wrap this call in a try/catch so one failure logs a warning and lets the loop continue.
🛡️ Proposed fix to isolate per-session restore failures
for (const session of host?.listSessionTabs() ?? []) {
if (session.agent !== 'codex') {
continue
}
let sessionId = session.sessionId
while (sessionId.startsWith('agent-session:')) {
sessionId = sessionId.slice('agent-session:'.length)
}
- await this.publishStructuredAgentSessionTab({
- ...session,
- agent: 'codex',
- sessionId,
- activate: false,
- notify: false
- })
+ try {
+ await this.publishStructuredAgentSessionTab({
+ ...session,
+ agent: 'codex',
+ sessionId,
+ activate: false,
+ notify: false
+ })
+ } catch (error) {
+ console.warn('[structured-agent-session] failed to restore session tab', {
+ sessionId,
+ error
+ })
+ }
}📝 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.
| await this.publishStructuredAgentSessionTab({ | |
| for (const session of host?.listSessionTabs() ?? []) { | |
| if (session.agent !== 'codex') { | |
| continue | |
| } | |
| let sessionId = session.sessionId | |
| while (sessionId.startsWith('agent-session:')) { | |
| sessionId = sessionId.slice('agent-session:'.length) | |
| } | |
| try { | |
| await this.publishStructuredAgentSessionTab({ | |
| ...session, | |
| agent: 'codex', | |
| sessionId, | |
| activate: false, | |
| notify: false | |
| }) | |
| } catch (error) { | |
| console.warn('[structured-agent-session] failed to restore session tab', { | |
| sessionId, | |
| error | |
| }) | |
| } | |
| } |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
The fix is sound for the happy path, but its rollback guarantee rests on an assumption that is not enforced anywhere: that the pinned base release never rewrites agent-sessions.json. One section below explains, plus two smaller error-ordering points inline.
Reviewed changes
A single commit that moves the "which structured chat sessions are visible" identity out of the rollback-rewritten profile and into the durable agent-session store, so a pinned-base → target round trip no longer loses the visible-tab index.
- Durable visible-session index —
AgentSessionStoreStategains avisibleSessionIdsset, parsed (parseVisibleSessionIds) and serialized (agent-session-store-serialization.ts) with schema-tolerant validation. - Store API —
listVisibleSessionIds(filtered to surviving records) andsetSessionTabVisibility(throwsagent_session_identity_requiredwhen marking a missing record visible). - Runtime wiring — restore prefers the durable index when non-empty, close clears the visibility reference, publish and handoff-reveal now
awaitthe persistence. - Regression coverage — a target → pinned-base → target fixture plus a restore-preference unit test.
⚠️ visibleSessionIds rides on schema v2, so the read-only protection never engages
The new field is added to AGENT_SESSION_STORE_SCHEMA_VERSION = 2 without a bump. The store's existing forward-compat safety net (readOnly = schemaVersion > AGENT_SESSION_STORE_SCHEMA_VERSION, which makes an older build "read but never write") is therefore not triggered: any build that shares schema v2 but predates this PR will parse the file, ignore the unknown visibleSessionIds key, and — on its next lease renewal or journal write — serialize the store without it, silently erasing the index this PR depends on.
The entire fix hinges on the pinned base release having no agent-session-record-store code at all (so it never touches the file). That may well be true given the reland history, but nothing in this PR enforces or tests it: pinnedBaseRoundTrip in the compatibility test only rewrites the profile (unifiedTabs) and never simulates an older build rewriting the store file, so the load-bearing assumption is unverified.
Technical details
# visibleSessionIds durability across rollback
## Affected sites
- src/main/runtime/agent-session-record-store-file.ts:32 — schema version stays 2 while a new durable field is introduced
- src/main/runtime/agent-session-store-serialization.ts:15 — writes the new field under schema v2
- src/main/runtime/structured-agent-session-rollback-compatibility.test.ts:102 — simulates base only via profile rewrite, never a store rewrite
## Required outcome
- Confirm the pinned base release contains no agent-session-record-store code (it can never rewrite agent-sessions.json), and document that assumption, or
- Bump AGENT_SESSION_STORE_SCHEMA_VERSION and add a v2 → v3 migration, so every older build treats the file as read-only and cannot drop the field.
## Suggested approach (optional)
- Note parseState rejects schemaVersion < current (returns null → agent_session_store_corrupt), so a bump requires a migration that admits v2 files and rewrites them as v3.
## Open questions for the human
- Between the pinned base and this PR, is there any released build that has schema-v2 store code but not visibleSessionIds? If so, a user rolling back through it loses the index today.DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
| } | ||
| context.releaseOwner(sessionId) | ||
| deps.transport?.revealNativeSession?.({ | ||
| await deps.transport?.revealNativeSession?.({ |
There was a problem hiding this comment.
revealNativeSession is now awaited after the durable handoff has already committed (reserveStoredAgentSessionHandoffOwner + acquireNative succeeded and releaseOwner ran). If the visibility write inside it fails, this throws and reports a failed handoff even though the record is already native-owned — and the retry path then re-reads a record whose handoffStage no longer matches a retryable state, which can wedge the session. Since the durable handoff is already complete here, consider logging the reveal failure instead of propagating it, or move the visibility persistence before the final commit.
| }): Promise<void> { | ||
| const host = getStructuredAgentSessionHost() | ||
| if (typeof host?.setSessionTabVisibility === 'function') { | ||
| await host.setSessionTabVisibility(input.sessionId, true) |
There was a problem hiding this comment.
setSessionTabVisibility(true) is awaited before the duplicate-tab early return and can now throw (agent_session_identity_required, or a store write failure) where publishStructuredAgentSessionTab was previously a fire-and-forget void. That propagates into two places that used to be infallible: the agentSession.create RPC (session already attached, so the client sees an error for a successful create) and the restoreStructuredAgentSessionTabsOnce loop (one failure aborts restoration of all remaining tabs). Confirm that's the intended failure surface — for the RPC path especially, the create has already durably succeeded, so failing it over the secondary visibility write is a surprising outcome.
|
Review completed on commit Findings addressed:
Validation:
Recommendation: land after acknowledging the Electron validation gap; CI checks are still completing. |
There was a problem hiding this comment.
ℹ️ Increment reviewed — the
presentflag cleanly fixes the empty-index fallback and the close path now clears visibility. No new issues in these commits; the three previously-flagged concerns (schema-v2 durability, and the two error-ordering points) are unchanged by this delta and remain open.
Reviewed changes
- Absent-vs-empty durable index —
AgentSessionStoreState.visibleSessionIdsIndexPresentrecords whether thevisibleSessionIdsfield was ever committed, and serialization emits the field only once set, so startup can distinguish a never-written index (migrate from profile tabs) from an intentionally empty one (closed sessions stay closed). - Close clears the durable visibility reference —
agentSession.closeadditionally callssetSessionTabVisibility(sessionId, false), covering terminal-disposal closes that skip the session-tabs retirement RPC. - Regression coverage — the target → pinned-base → target fixture now asserts the
presenttransitions, alongside an empty-index-authoritative restore test and an RPC-close-clears-visibility test.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
|
CI update for commit All native-chat-focused tests, typecheck, static analysis, wire compatibility, package, and Linux test shards are passing. This PR does not touch the Cursor hook path, so this currently looks like a Windows CI/environment flake rather than a regression. I have not changed code or expanded scope. |
|
Readiness update: the failed Windows check was rerun successfully. The PR is attached to Linear ticket STA-5930. |
Electron validation (landed commit
|
…17439) * fix(native-chat): preserve structured tabs across rollback * fix(native-chat): preserve rollback visibility state --------- Co-authored-by: Merge Sim <sim@local>
Summary
contentType: agent-sessiontabs.Root cause
The workspace profile tab list was treated as the sole restoration identity for structured sessions. The pinned base release rejects the newer
agent-sessiondiscriminator and salvages those entries away; after the base release rewrites the profile, re-upgrade can no longer discover the surviving records/journals.This is a persistence ownership/lifecycle issue, not a reason to revert the structured-chat feature. The visible-tab reference now lives with the structured-session store, which older builds do not rewrite. Closed historical journals remain lazy and are not resurrected on startup.
Regression coverage
Validation
pnpm tcELI5
An older Orca build can erase structured-chat tabs it does not understand when it rewrites the profile. The visible chat IDs now live in the durable agent-session store, which the older build leaves alone, and that list is used when the app returns. Closing removes only the visible-tab pointer; the record and journal stay available, and open/close waits for the durable write.
Performance Measurement
This is a persistence-compatibility fix, not a throughput optimization, so no speedup is claimed. In the deterministic target-to-pinned-base-to-target fixture, profile-only restoration returns 0 session IDs after the base round-trip, while the durable index restores 1 ID and preserves the journal; closing leaves the durable record and an empty visible index. No timing claim is made.
User-Facing Trade-offs
There is no visible data-loss trade-off. Each visibility change now waits for one small durable whole-store transaction, which can add a little open/close latency; closed chats remain in history but are not resurrected. Stores without the new field retain the legacy profile fallback.