Repository navigation
Conversation
📝 WalkthroughWalkthroughAdds safe hardlink export of managed Codex transcripts into system history, with collision, symlink, traversal, failure, and concurrency handling. Hook events now schedule eligible exports with duplicate suppression and retry behavior. Native reconciliation exports managed sessions before bridging system sessions, while legacy migration logic moves to a dedicated module. WSL session bridging becomes bidirectional, and a reliability gate documents the new contract and validation coverage. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 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: 2
🧹 Nitpick comments (1)
src/main/codex/codex-session-history-hook.ts (1)
36-41: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winWrap the
setImmediatecallback in a try/catch.
exportManagedCodexSessionToSystemHistoryis designed not to throw, but an unexpected error would surface as an unhandled exception insetImmediate, crashing the Electron main process. A defensive catch prevents a history-export bug from taking down the app.🛡️ Proposed fix: guard against unhandled exceptions in deferred export
const task = setImmediate(() => { - const result = exportManagedCodexSessionToSystemHistory(transcriptPath, systemCodexHomePath) - if (result === 'linked' || result === 'already-linked') { - rememberExportedManagedSessionPath(exportKey) + try { + const result = exportManagedCodexSessionToSystemHistory(transcriptPath, systemCodexHomePath) + if (result === 'linked' || result === 'already-linked') { + rememberExportedManagedSessionPath(exportKey) + } + } catch (error) { + console.warn('[codex-session-bridge] Unexpected error during managed Codex session export:', { + transcriptPath, + error + }) } })
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 732eeda8-bbd9-4133-8fa9-29c28ae30941
📒 Files selected for processing (12)
config/reliability-gates.jsoncsrc/main/codex/codex-session-bridge.test.tssrc/main/codex/codex-session-bridge.tssrc/main/codex/codex-session-history-hook.test.tssrc/main/codex/codex-session-history-hook.tssrc/main/codex/codex-session-legacy-copy.tssrc/main/codex/codex-session-managed-export-failure.test.tssrc/main/codex/codex-session-managed-export.test.tssrc/main/codex/codex-session-managed-export.tssrc/main/codex/wsl-codex-session-bridge.test.tssrc/main/codex/wsl-codex-session-bridge.tssrc/main/index.ts
| replacementPath = `${targetPath}.orca-link-${process.pid}-${Date.now()}` | ||
| if (!tryLink(sourcePath, replacementPath)) { | ||
| return | ||
| } | ||
| rmSync(targetPath, { force: true }) | ||
| renameSync(replacementPath, targetPath) | ||
| clearLegacyCopiedSessionMarker(relativePath) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant file and nearby call sites.
git ls-files src/main/codex/codex-session-legacy-copy.ts
wc -l src/main/codex/codex-session-legacy-copy.ts
cat -n src/main/codex/codex-session-legacy-copy.ts | sed -n '1,180p'
# Check for any surrounding logic that could affect the rename/remove sequence.
rg -n "rmSync\\(|renameSync\\(|tryLink\\(" src/main/codex -SRepository: stablyai/orca
Length of output: 1949
🌐 Web query:
Node.js fs.renameSync overwrite existing destination file Windows libuv MOVEFILE_REPLACE_EXISTING documentation
💡 Result:
In Node.js, the fs.rename and fs.renameSync methods are intended to provide a cross-platform way to rename files, and they generally overwrite the destination file if it already exists [1][2]. On Windows, this operation relies on the underlying libuv library, which uses the Windows MoveFileEx API [3]. Technical details regarding this behavior include: 1. Underlying Windows API: libuv uses MoveFileEx with the MOVEFILE_REPLACE_EXISTING flag to implement the rename operation [3]. This flag explicitly instructs the Windows operating system to replace the destination file if it already exists, provided that security and access control requirements are met [4]. 2. Platform Inconsistencies and Reliability: While the intention is to overwrite the existing file, renaming operations on Windows can be notoriously unreliable due to factors such as antivirus software, indexing services, or other processes holding a lock on the file [5][6]. When a conflict occurs, these operations often fail with errors like EPERM, EBUSY, or EACCES, rather than successfully overwriting the file [5][6]. 3. Documentation and Best Practices: Because of these platform-specific reliability issues, the behavior of fs.rename on Windows has been a subject of ongoing discussion within the Node.js and libuv communities [7][1][8][5]. It is widely recommended to avoid relying on fs.rename for atomic replacements or mission-critical operations on Windows [5]. Developers frequently use user-land modules like graceful-fs, which implement retry logic to handle transient errors that occur when the OS-level move operation fails [5][6]. In summary, while fs.renameSync is designed to overwrite an existing destination file on Windows via the MOVEFILE_REPLACE_EXISTING flag, you may encounter EPERM or similar errors if the file is locked or otherwise inaccessible at the moment of the operation [5][3][4].
Citations:
- 1: doc: Change doc on fs.rename api w.r.t to windows behaviour nodejs/node#22014
- 2: https://stackoverflow.com/questions/21219018/node-js-does-fs-rename-overwrite-file-if-already-exists
- 3: Error renaming file in Win32, if the file already exists joyent/libuv#283
- 4: https://github.com/MicrosoftDocs/sdk-api/blob/docs/sdk-api-src/content/winbase/nf-winbase-movefileexa.md
- 5: EPERM when renaming files on Windows nodejs/node#29481
- 6: win,fs: retry if uv_fs_rename fails libuv/libuv#1981
- 7: win,fs: fix inconsistencies between platforms for rename libuv/libuv#1941
- 8: fs.rename doesn't work as documented nodejs/node#21957
Remove the pre-renameSync delete fs.renameSync already overwrites the destination, so deleting targetPath first only creates a gap where a failed rename can leave the session file missing.
| function prepareSafeSystemTargetDirectory( | ||
| systemSessionsRoot: string, | ||
| targetDirectory: string | ||
| ): boolean { | ||
| try { | ||
| mkdirSync(systemSessionsRoot, { recursive: true }) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Check systemSessionsRoot for symlinks after creation.
mkdirSync(systemSessionsRoot, { recursive: true }) follows a pre-existing symlink and succeeds, but the segment walk only checks intermediate directories — not the root itself. If systemSessionsRoot is a symlink to an arbitrary directory, the final canonical check passes because both canonicalRoot and canonicalTargetDirectory resolve through the same symlink, so relative() stays within the resolved root. This allows the hardlink to be created in the symlink target, bypassing the symlink-escape validation the PR intends to enforce.
🔒 Proposed fix: reject symlinked system history root
try {
mkdirSync(systemSessionsRoot, { recursive: true })
+ const rootStat = tryLstat(systemSessionsRoot)
+ if (!rootStat?.isDirectory() || rootStat.isSymbolicLink()) {
+ console.warn('[codex-session-bridge] Refusing symlinked system history root:', {
+ systemSessionsRoot
+ })
+ return false
+ }
const relativeTargetDirectory = relative(systemSessionsRoot, targetDirectory)📝 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.
| function prepareSafeSystemTargetDirectory( | |
| systemSessionsRoot: string, | |
| targetDirectory: string | |
| ): boolean { | |
| try { | |
| mkdirSync(systemSessionsRoot, { recursive: true }) | |
| function prepareSafeSystemTargetDirectory( | |
| systemSessionsRoot: string, | |
| targetDirectory: string | |
| ): boolean { | |
| try { | |
| mkdirSync(systemSessionsRoot, { recursive: true }) | |
| const rootStat = tryLstat(systemSessionsRoot) | |
| if (!rootStat?.isDirectory() || rootStat.isSymbolicLink()) { | |
| console.warn('[codex-session-bridge] Refusing symlinked system history root:', { | |
| systemSessionsRoot | |
| }) | |
| return false | |
| } |
|
Blocking follow-up from the bug-bash audit: this PR is intentionally draft until both CodeRabbit findings are fixed and revalidated.
The implementation worker was interrupted by the temporarily overloaded agent gateway before it could apply these changes. Existing green CI predates the findings and is not treated as sufficient. |
This branch has not been deployed
1 participant
Description
Closes #4444.
Orca redirects Codex to a managed
CODEX_HOME, but its session bridge only linked external system history into that managed home. Sessions created by Orca therefore stayed private to the managed history tree. The bridge also ran before a new rollout file was created, so merely reversing the startup scan would not make the current chat visible.This PR:
SessionStart, withUserPromptSubmitas an event-driven retry if creation was not yet observable;EXDEV/EPERMfailures fail-safe: the managed original remains untouched and no misleading partial copy is created;agent-session.codex-external-history-bridgereliability gate.Reliability proof
CODEX_HOMEbecomes discoverable from the configured external history root without overwriting either corpus or creating divergent mutable copies. Motivated by [Bug]: Codex chats started from Orca workspace do not appear in Codex Mac/CLI project history #4444.EXDEV/EPERMsafety, hook timing, and WSL command construction.[codex-session-bridge]warnings containing paths and errors, never session contents.Evidence of fix (before + after)
All fixtures are synthetic. No real auth, user history, reporter attachment, or user-provided code was read or copied.
Before
The exact production-boundary regression was run twice before implementation:
After
Full
pnpm run lintpassed oxlint, switch exhaustiveness, styled scrollbar, reliability, max-lines, and localization catalog reference checks, then stopped on an unrelated existing Japanese catalog interpolation mismatch forcomponents.native-chat.composer.uploadingAttachments. No localization files are changed here.ELI5
Orca and normal Codex were keeping chat notebooks in two different filing cabinets. Orca knew how to put normal Codex notebooks into its cabinet, but not how to put Orca-created notebooks back into the normal cabinet. This makes both cabinets point at the same notebook page, so new writing stays visible from either side without copying or overwriting anyone else’s notebook.
User-facing trade-offs
cwd. Orca keeps the truthful worktree path so resuming cannot silently edit the wrong checkout; users opening external Codex at the original repo path may need the worktree path orcodex resume --all.session_index.jsonlrename metadata is not synchronized. The transcript becomes externally discoverable, but an Orca-only renamed title may not appear externally.