Conversation
|
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:
📝 WalkthroughWalkthroughAdds support for discovering, reading, parsing, and caching plain and zstd-compressed Codex rollouts, including paginated turn items. Adds filesystem markers, preservation, replacement, migration, refresh, rollback, and WSL bridging for Codex sessions. Updates native chat transcript resolution, decoding limits, caching, and watching for compressed and paginated files, deduplicates skill suggestions by source priority, and expands Codex slash-command suggestions. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
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: 1
🧹 Nitpick comments (1)
src/main/ai-vault/session-scanner-parse-cache.ts (1)
53-57: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
isCodexCompressedRolloutPathinstead of inline.endsWith.The inline
.endsWith('.jsonl.zst')duplicatesisCodexCompressedRolloutPathfromsession-scanner-codex-paths.ts, which is already used consistently insession-scanner-codex-rollout-read.tsandtranscript-reader.ts. Using the shared helper keeps the compressed-path detection logic in one place.♻️ Proposed refactor
case 'codex': // Why: byte offsets only describe plain append-only JSONL. Cold zstd // sessions must be decompressed from the beginning after each change. - return candidate.file.path.endsWith('.jsonl.zst') + return isCodexCompressedRolloutPath(candidate.file.path) ? null : () => createCodexSessionResumeState(candidate.file, candidate.codexHome)
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 82c4dfbe-81cb-4445-a9ac-a118675dfdec
📥 Commits
Reviewing files that changed from the base of the PR and between ff8192a and 22db9241c5ad5e827a13d46881dd2236e91c66a5.
📒 Files selected for processing (29)
src/main/ai-vault/session-scanner-codex-discovery.tssrc/main/ai-vault/session-scanner-codex-parser.test.tssrc/main/ai-vault/session-scanner-codex-parser.tssrc/main/ai-vault/session-scanner-codex-paths.tssrc/main/ai-vault/session-scanner-codex-record-consume.tssrc/main/ai-vault/session-scanner-codex-rollout-read.tssrc/main/ai-vault/session-scanner-codex-zstd.test.tssrc/main/ai-vault/session-scanner-discovery.tssrc/main/ai-vault/session-scanner-parse-cache.tssrc/main/ai-vault/session-scanner-source-discovery.tssrc/main/codex/codex-session-bridge-link.tssrc/main/codex/codex-session-bridge.test.tssrc/main/codex/codex-session-bridge.tssrc/main/codex/codex-session-copy-markers.tssrc/main/codex/codex-session-file-listing.tssrc/main/codex/codex-session-preserved-copies.tssrc/main/codex/codex-session-preserved-install.tssrc/main/codex/wsl-codex-session-bridge.test.tssrc/main/codex/wsl-codex-session-bridge.tssrc/main/native-chat/session-file-resolver.test.tssrc/main/native-chat/session-file-resolver.tssrc/main/native-chat/transcript-codex-turn-items.tssrc/main/native-chat/transcript-line-decoders-codex.tssrc/main/native-chat/transcript-reader.test.tssrc/main/native-chat/transcript-reader.tssrc/renderer/src/components/native-chat/native-chat-composer-state.test.tssrc/renderer/src/components/native-chat/native-chat-composer-state.tssrc/shared/native-chat-slash-commands.test.tssrc/shared/native-chat-slash-commands.ts
22db924 to
598ce95
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 1fb7c96b-1c75-48d8-ab0b-40450ddd132f
📥 Commits
Reviewing files that changed from the base of the PR and between 22db9241c5ad5e827a13d46881dd2236e91c66a5 and 598ce9545f99a66c5fe8eecff2542f6b89b58253.
📒 Files selected for processing (36)
src/main/ai-vault/session-scanner-codex-discovery.tssrc/main/ai-vault/session-scanner-codex-parser.test.tssrc/main/ai-vault/session-scanner-codex-parser.tssrc/main/ai-vault/session-scanner-codex-paths.tssrc/main/ai-vault/session-scanner-codex-record-consume.tssrc/main/ai-vault/session-scanner-codex-rollout-read.tssrc/main/ai-vault/session-scanner-codex-zstd.test.tssrc/main/ai-vault/session-scanner-discovery.tssrc/main/ai-vault/session-scanner-parse-cache.tssrc/main/ai-vault/session-scanner-source-discovery.tssrc/main/codex/codex-session-bridge-link.tssrc/main/codex/codex-session-bridge.test.tssrc/main/codex/codex-session-bridge.tssrc/main/codex/codex-session-copy-markers.tssrc/main/codex/codex-session-file-listing.tssrc/main/codex/codex-session-preserved-copies.tssrc/main/codex/codex-session-preserved-install.tssrc/main/codex/wsl-codex-session-bridge.test.tssrc/main/codex/wsl-codex-session-bridge.tssrc/main/native-chat/session-file-resolver.test.tssrc/main/native-chat/session-file-resolver.tssrc/main/native-chat/transcript-codex-turn-items.tssrc/main/native-chat/transcript-line-decoders-codex.tssrc/main/native-chat/transcript-read-cache.test.tssrc/main/native-chat/transcript-read-cache.tssrc/main/native-chat/transcript-reader.test.tssrc/main/native-chat/transcript-reader.tssrc/main/native-chat/transcript-stream-lines.tssrc/main/native-chat/transcript-watch.test.tssrc/main/native-chat/transcript-watch.tssrc/main/runtime/rpc/methods/native-chat.test.tssrc/main/runtime/rpc/methods/native-chat.tssrc/renderer/src/components/native-chat/native-chat-composer-state.test.tssrc/renderer/src/components/native-chat/native-chat-composer-state.tssrc/shared/native-chat-slash-commands.test.tssrc/shared/native-chat-slash-commands.ts
🚧 Files skipped from review as they are similar to previous changes (23)
- src/main/ai-vault/session-scanner-codex-discovery.ts
- src/main/ai-vault/session-scanner-discovery.ts
- src/shared/native-chat-slash-commands.test.ts
- src/main/ai-vault/session-scanner-codex-parser.test.ts
- src/main/native-chat/transcript-line-decoders-codex.ts
- src/renderer/src/components/native-chat/native-chat-composer-state.ts
- src/main/codex/wsl-codex-session-bridge.test.ts
- src/main/ai-vault/session-scanner-codex-rollout-read.ts
- src/main/ai-vault/session-scanner-parse-cache.ts
- src/main/codex/codex-session-preserved-copies.ts
- src/shared/native-chat-slash-commands.ts
- src/renderer/src/components/native-chat/native-chat-composer-state.test.ts
- src/main/ai-vault/session-scanner-codex-zstd.test.ts
- src/main/ai-vault/session-scanner-codex-paths.ts
- src/main/codex/codex-session-file-listing.ts
- src/main/codex/codex-session-preserved-install.ts
- src/main/codex/codex-session-bridge.ts
- src/main/codex/codex-session-copy-markers.ts
- src/main/ai-vault/session-scanner-source-discovery.ts
- src/main/codex/codex-session-bridge-link.ts
- src/main/ai-vault/session-scanner-codex-record-consume.ts
- src/main/native-chat/transcript-codex-turn-items.ts
- src/main/codex/codex-session-bridge.test.ts
598ce95 to
1c0082b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 703ebb5e-f963-4a93-980d-0b0ab4f8a791
📥 Commits
Reviewing files that changed from the base of the PR and between 598ce9545f99a66c5fe8eecff2542f6b89b58253 and 1c0082b.
📒 Files selected for processing (37)
src/main/ai-vault/session-scanner-codex-discovery.tssrc/main/ai-vault/session-scanner-codex-parser.test.tssrc/main/ai-vault/session-scanner-codex-parser.tssrc/main/ai-vault/session-scanner-codex-paths.tssrc/main/ai-vault/session-scanner-codex-record-consume.tssrc/main/ai-vault/session-scanner-codex-rollout-read.tssrc/main/ai-vault/session-scanner-codex-zstd.test.tssrc/main/ai-vault/session-scanner-discovery.tssrc/main/ai-vault/session-scanner-parse-cache.tssrc/main/ai-vault/session-scanner-source-discovery.tssrc/main/codex/codex-session-bridge-link.tssrc/main/codex/codex-session-bridge.test.tssrc/main/codex/codex-session-bridge.tssrc/main/codex/codex-session-copy-markers.tssrc/main/codex/codex-session-file-listing.tssrc/main/codex/codex-session-preserved-copies.tssrc/main/codex/codex-session-preserved-install.tssrc/main/codex/wsl-codex-session-bridge.test.tssrc/main/codex/wsl-codex-session-bridge.tssrc/main/native-chat/session-file-resolver.test.tssrc/main/native-chat/session-file-resolver.tssrc/main/native-chat/transcript-codex-turn-items.tssrc/main/native-chat/transcript-line-decoders-codex.tssrc/main/native-chat/transcript-read-cache.test.tssrc/main/native-chat/transcript-read-cache.tssrc/main/native-chat/transcript-reader.test.tssrc/main/native-chat/transcript-reader.tssrc/main/native-chat/transcript-stream-lines.test.tssrc/main/native-chat/transcript-stream-lines.tssrc/main/native-chat/transcript-watch.test.tssrc/main/native-chat/transcript-watch.tssrc/main/runtime/rpc/methods/native-chat.test.tssrc/main/runtime/rpc/methods/native-chat.tssrc/renderer/src/components/native-chat/native-chat-composer-state.test.tssrc/renderer/src/components/native-chat/native-chat-composer-state.tssrc/shared/native-chat-slash-commands.test.tssrc/shared/native-chat-slash-commands.ts
✅ Files skipped from review due to trivial changes (1)
- src/shared/native-chat-slash-commands.ts
🚧 Files skipped from review as they are similar to previous changes (32)
- src/shared/native-chat-slash-commands.test.ts
- src/main/codex/codex-session-file-listing.ts
- src/main/ai-vault/session-scanner-discovery.ts
- src/main/ai-vault/session-scanner-codex-parser.test.ts
- src/main/ai-vault/session-scanner-codex-zstd.test.ts
- src/main/ai-vault/session-scanner-parse-cache.ts
- src/renderer/src/components/native-chat/native-chat-composer-state.ts
- src/renderer/src/components/native-chat/native-chat-composer-state.test.ts
- src/main/native-chat/transcript-line-decoders-codex.ts
- src/main/ai-vault/session-scanner-codex-paths.ts
- src/main/native-chat/transcript-reader.ts
- src/main/native-chat/transcript-read-cache.test.ts
- src/main/ai-vault/session-scanner-source-discovery.ts
- src/main/ai-vault/session-scanner-codex-rollout-read.ts
- src/main/native-chat/transcript-codex-turn-items.ts
- src/main/codex/codex-session-copy-markers.ts
- src/main/codex/codex-session-preserved-copies.ts
- src/main/runtime/rpc/methods/native-chat.test.ts
- src/main/ai-vault/session-scanner-codex-discovery.ts
- src/main/codex/wsl-codex-session-bridge.test.ts
- src/main/native-chat/session-file-resolver.test.ts
- src/main/native-chat/transcript-watch.ts
- src/main/ai-vault/session-scanner-codex-record-consume.ts
- src/main/native-chat/transcript-reader.test.ts
- src/main/runtime/rpc/methods/native-chat.ts
- src/main/native-chat/transcript-stream-lines.ts
- src/main/codex/codex-session-preserved-install.ts
- src/main/native-chat/transcript-read-cache.ts
- src/main/codex/codex-session-bridge-link.ts
- src/main/native-chat/session-file-resolver.ts
- src/main/codex/codex-session-bridge.test.ts
- src/main/ai-vault/session-scanner-codex-parser.ts
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/main/codex/wsl-codex-session-bridge.ts (2)
108-138: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the sourceSize-clamping semantics.
write_copy_markerdeliberately overwritessource_sizewithtarget_size(line 116) before recording it as"sourceSize"in the marker, andcopy_marker_matchesmirrors this by buildingmarker_prefixwith the currenttarget_sizein place of the real source size (line 132). This means the marker's"sourceSize"field is not the literal size of the source file but the amount actually verified-copied — a non-obvious design choice (accounting for sources that may still be growing) that isn't explained anywhere nearby.As per coding guidelines, "When code is driven by a design document or non-obvious constraint, add a brief one- or two-line comment explaining why it behaves that way, without restating the implementation."
📝 Suggested addition
+ // Why: "sourceSize" records the verified-copied byte count, not the + // literal source size, since the source may still be growing. 'write_copy_marker() {',Source: Coding guidelines
92-208: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffConsider deduplicating repeated full-file SHA-256 hashing.
A single
refresh_copycall for an existing target can hash the same file content up to ~4 times: once insidecopy_marker_matches, thentarget_fingerprint_before,target_fingerprint_after, andpreserved_fingerprint. For larger rollout files and a periodic sync, this is avoidable I/O — e.g.copy_marker_matchescould surface its computed fingerprint for reuse astarget_fingerprint_beforeinstead of recomputing.Given the safety-critical, already heavily-reviewed nature of this script and that rollout files are typically modest-sized JSONL logs, this is a nice-to-have rather than urgent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5b123b28-9eff-433f-8168-e72b1465d2da
📥 Commits
Reviewing files that changed from the base of the PR and between 1c0082b and a33d90cc9d265fcd0cad8d8ac0f0dda88c069297.
📒 Files selected for processing (1)
src/main/codex/wsl-codex-session-bridge.ts
a33d90c to
bd57186
Compare
|
Followed up on the current-head review in amended commit I did not deduplicate the repeated SHA-256 calls. That suggestion is explicitly a poor-tradeoff nitpick: these rollout files are normally modest, while threading a cached fingerprint through Focused bridge tests: 9/9; targeted lint/format, max-lines, and diff checks pass under Node 24. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
reviewing |
16df9fa to
d529b25
Compare
AmethystLiang
left a comment
AmethystLiang
left a comment
There was a problem hiding this comment.
The writable per-file session bridge needs an architectural redesign before this can merge.
The bridge assumes that hardlinking an individual rollout makes the system and Orca-managed CODEX_HOME share one logical Codex session. That invariant does not hold for current Codex. A rollout is managed as a path-level .jsonl/.jsonl.zst pair: resume materializes a compressed rollout by creating a new .jsonl and removing the .jsonl.zst in that home, while background compression creates the sibling .jsonl.zst and removes the plain path. Hardlinks share an existing inode, but they do not mirror those namespace transitions. After either transition, the two homes can silently retain different histories. The cross-volume copy fallback forks even earlier. Marker, fingerprint, preservation, and race machinery cannot make two independently mutated namespaces coherent.
Please replace the writable bridge with an explicit ownership model: use one authoritative Codex persistence namespace, make import/continuation a clearly one-way fork into the managed home, or integrate through Codex thread/resume APIs and let Codex own the migration. The compressed-rollout discovery and reader work is still useful and can be retained independently.
There are also two correctness issues to address:
-
Paginated Codex rollouts currently decode both persisted response_item messages and event_msg.item_completed TurnItems. Current Codex persists the raw response and then emits ItemCompleted, so realistic paginated transcripts duplicate both user and assistant messages. Parsing needs to be history_mode-aware or deduplicate canonical items.
-
The remote transcript seed is described as a tail, but reading always begins at byte zero under a 64 MiB total decoded-content ceiling. Once a transcript exceeds that ceiling, the seed fails and the subscription advances to EOF, yielding no initial history. Plain JSONL needs a newline-aligned suffix read; compressed rollouts need an explicit bounded strategy.
Finally, the branch currently conflicts with main in transcript read/watch code. Conflict resolution must preserve the newer retryable notFound / first-flush behavior from #8401.
d529b25 to
9e6e9b3
Compare
9e6e9b3 to
f07fd6f
Compare
bbingz
changed the title
|
Addressed the requested architecture change in The review is correct that a hardlink/copy cannot preserve Codex's The replacement model is authority-based:
Regression coverage includes the Verification on Node 24:
Windows/WSL live smoke remains for CI or a Windows host. Ready for re-review. |
|
@Jinwoo-H The requested bridge-architecture redesign is ready for re-review at exact head 57318f8. The writable local and WSL session bridges were removed. AI Vault now discovers each authoritative Codex home directly, sessions carry their owning codexHome, and resume runs against that exact CODEX_HOME. Legacy copy markers are read-only compatibility only. Verified at this head: focused authority/resume tests 90/90, full Vitest 2,826 files / 29,757 tests, pnpm lint, pnpm build:desktop, and git diff --check origin/main...HEAD. Windows/WSL live smoke remains for CI or a Windows host. |
|
Thanks for the work here. Since @brennanb2025 is already migrating CODEX_HOME back to its standard location, I think the cleanest path is to hand this area over to that effort rather than continue developing this PR independently. The bridge-removal and single-authoritative-home direction is still valuable, particularly avoiding writable aliases and preserving exact session ownership. The migration should also account for existing bridges, WSL ownership, compressed/plain rollout transitions, and Native Chat/usage discovery. I’m going to leave this PR unmerged so @brennanb2025 can reuse any relevant pieces while implementing the consolidated approach. Thanks again for pushing the investigation and implementation forward. |
|
Closing following the maintainer handoff decision in this thread. The bridge-removal, authoritative-home, compressed-rollout, and Native Chat findings remain useful reference material, but this PR is no longer the intended merge vehicle. The consolidated CODEX_HOME work is proceeding through #8802, #8828, #8921, and #9179; closing this PR records that ownership transfer rather than discarding its investigation or credit. |
3 participants
Summary
Replacement slice for #7950, limited to Codex session discovery/resume and Native Chat decoding.
.jsonl.zstrollouts; prefer a plain sibling before applying result limitsitem_completedTurnItems for AI Vault and Native ChatCODEX_HOMEthat owns the selected rolloutWhy this is separate
#7950 mixed startup, hooks, auth/rate limits, and session behavior. This PR contains only the session lifecycle/read path so its data-safety policy can be reviewed independently.
Session authority model
Codex owns a rollout as a namespace pair (
.jsonl/.jsonl.zst), not as one stable inode. Compression and resume may create, delete, or replace either directory entry. A hardlink or copy in a secondCODEX_HOMEtherefore cannot remain a correct writable alias.This revision removes the bridge entirely:
~/.codexhome.CODEX_HOME, so.jsonlto.jsonl.zsttransitions stay in one authoritative namespace.Existing aliases from earlier builds are intentionally not deleted automatically because Orca cannot prove they have not diverged.
Transcript byte-safety
StringDecodercarries incomplete UTF-8 across Buffer chunks and flushes once at EOF. Raw0x0Aboundaries drive JSONL line settlement, so malformed bytes that decode toU+FFFDcannot inflate a persisted watch offset or weaken byte limits. Plain file readers and incremental watches keep data as Buffers until this boundary; the string-stream compatibility path carries a trailing high surrogate across string chunks.Testing
origin/main@1b3f281ffpnpm lint(all repository gates passed; only pre-existing warnings)pnpm build:desktop(Node/CLI/web typecheck, relay, CLI, Electron/Vite, and web builds)oxfmt --checkon touched TypeScript filesgit diff --check origin/main...HEAD.jsonlto.jsonl.zstnamespace transitions, default/custom host authorities, and default/custom WSL authoritiesVerification used the repository-declared Node 24 toolchain. CI remains the authoritative full-platform verifier.