fix: support relocated Claude config transfers - #729
Conversation
sylvesterkaczmarek
left a comment
sylvesterkaczmarek
left a comment
There was a problem hiding this comment.
One containment edge remains in the staging path. relative is derived from the canonical source root, but the destination is built under the lexical default projects path and only realpathed after the copy; an existing symlink in an intermediate staging directory can make canonicalImportPath resolve outside canonical ~/.claude/projects, and that path is then handed to native import without another containment check. Please revalidate the canonical staged path against realpath(defaultProjects) before import, or create it through an equivalent no-follow rooted path.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a958132657
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Follow-up on the staging-containment review: the first fix ( |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d1ac612b71
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 638f727146
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const candidate = attempt === 0 | ||
| ? preferredPath | ||
| : path.join(parsed.dir, `${parsed.name}.codex-import-${randomUUID()}${parsed.ext}`); |
There was a problem hiding this comment.
When the mirrored destination exists—for example, after an import completed but the process died before cleanup, or when the config was copied—this UUID fallback changes the canonical path passed to Codex on every retry. importedThreadIdForSource and the native-import ledger match on both source_path and content hash, so even an identical transcript no longer matches the previous record and each retry creates another Codex thread; use an existing identical staged file or another stable staging identity instead.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 866d3aa. I made collision fallback deterministic by source SHA-256 instead of UUID. If a candidate already exists, it is reused only when its content hash matches the source; otherwise the deterministic hash-derived sibling is used with COPYFILE_EXCL. Cleanup now deletes only a staging file created by the current transfer, so pre-existing identical files are never removed. Regression was RED on 638f727: two identical retries produced different staged source_path values; GREEN on this commit: both retries present the exact same path to native import. Full transfer family 7/7 PASS; node --check for transfer/companion and git diff --check clean. @codex review
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 866d3aa330
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (filesHaveSameContent(source, candidate, sourceSha256)) { | ||
| return { path: candidate, created: false }; |
There was a problem hiding this comment.
When two transfer commands for the same relocated transcript overlap, the second command can take this identical-file branch and reuse the path created by the first command. The first command still owns that path and unlinks it in its finally cleanup, so if it finishes while the second command is starting its app server or before that server reads the session, the second import fails because its importPath has disappeared. Reused files need ownership/reference coordination, or each concurrent transfer needs a staging file that another transfer cannot delete.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 21b617c. Shared deterministic staging paths now use a managed marker plus per-transfer leases coordinated by an atomic cross-process lock. Each transfer acquires its lease synchronously before native import; cleanup removes only that lease and deletes the staged file only when no other lease remains. Pre-existing/unmanaged files are never deleted. Added a deterministic regression with two overlapping prepareClaudeSessionImport() consumers: RED on 866d3aa because the first cleanup removed the second consumer's path; GREEN on 21b617c, where the path remains until the final cleanup. Transfer-focused runtime + lease tests: 8/8 PASS; node --check and git diff --check clean. @codex review
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 866d3aa330
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (filesHaveSameContent(source, candidate, sourceSha256)) { | ||
| return { path: candidate, created: false }; |
There was a problem hiding this comment.
When two transfers of the same relocated transcript overlap, the second process can reuse the first process's staged file here with created: false, while the first process still considers itself the owner and unlinks that path during its finally cleanup. If the first import completes before the second importer opens the file, the second transfer receives a now-missing importPath and fails. Coordinate ownership of reused paths or otherwise prevent cleanup while another transfer is using the staging file.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Also addressed in 21b617c. Reused managed staging files are now protected by per-transfer leases under a cross-process lock, so one import cannot unlink the shared path while another still holds a lease. The final cleanup removes the managed file only after the last lease is released; unmanaged/pre-existing files remain untouched. Regression RED on 866d3aa, GREEN on 21b617c; transfer/lease tests 8/8 PASS, syntax and diff checks clean. @codex review
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 21b617c0ea
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (staged.created) fs.unlinkSync(canonicalImportPath); | ||
| throw new Error(`Cannot stage Claude session outside the default Claude projects root: ${canonicalImportPath}`); | ||
| } | ||
| const lease = acquireStagingLease(canonicalImportPath, staged); |
There was a problem hiding this comment.
When a transfer reuses a managed staging file while its final existing lease is being released, copyToExclusiveStagingPath and realpathSync run before this lock-protected lease acquisition. The current holder can therefore delete the file and marker in that interval, causing the newcomer either to fail during realpathSync or to acquire no lease and pass a missing path to Codex. Fresh evidence in 21b617c is that only lease creation is locked, while staging-path selection remains outside the lock; perform reuse validation and lease acquisition in one critical section.
Useful? React with 👍 / 👎.
| const activeLeases = fs.readdirSync(directory).filter((name) => name.startsWith(leasePrefix)); | ||
| if (activeLeases.length > 0 || !managedMarkerMatches(markerPath)) return; |
There was a problem hiding this comment.
If a transfer is killed after creating its lease, its cleanup never removes the lease file. Every later transfer then treats that orphan as active here, so even after all live imports finish the managed transcript and marker remain permanently; this defeats the intended final cleanup and leaves copied session data behind. Use the PID encoded in each lease name to discard leases whose owning process is no longer alive before deciding that active leases remain.
Useful? React with 👍 / 👎.
2 participants
Summary
Fixes #721.
CLAUDE_CONFIG_DIR/projectsas well as the default~/.claude/projectsUSERPROFILEalongside the syntheticHOMEValidation
On Windows 11 / Node 26.3.1:
mainwithClaude session file not foundeven though the transcript existed underCLAUDE_CONFIG_DIR/projectsnode --test --test-name-pattern "transfer" tests/runtime.test.mjs-> 5 passed, 0 failednode --check plugins/codex/scripts/lib/claude-session-transfer.mjsnode --check plugins/codex/scripts/codex-companion.mjsgit diff --cached --checkThe existing Node DEP0190 warning on this Windows base is tracked separately by #717 / PR #725.