Repository navigation
fix(transfer): report already-imported sessions and add --refresh - #835
ehsan-parloa wants to merge 5 commits into
Conversation
Codex's externalAgentConfig/import skips a Claude transcript whose path is already in its import ledger, even after the transcript has grown, and completes with no successes and no failures. The plugin then looked for a ledger record matching the transcript's current content hash, found none, and failed with "did not record an imported thread". - Take the imported thread id from the import/completed notification (successes[].target) and surface failures[] messages. - When Codex skips a path it already imported, print the existing thread and say whether later turns are missing from it, instead of failing. - Add --refresh: back up the ledger, drop this transcript's records and import the current content into a new thread. - For Codex versions that complete without structured results, accept the ledger record added by this import by path, so a transcript that grows during the import no longer breaks the lookup. - Make the fake Codex skip already-imported paths and report structured results, as Codex 0.166 does. Fixes openai#750 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: 6a2ce8db7f
ℹ️ 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".
When an already-imported transcript was selected with --source, the printed hint dropped the override, so following it refreshed the session's own transcript instead. The hint now includes --source with the resolved path, and --json output exposes it as refreshCommand. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Codex writes external_agent_session_imports.json without a lock the plugin could share, so a refresh could overwrite a record that another import added between reading and replacing the ledger. Re-read the ledger right before the rename and start over when it changed, and write the backup from the exact content being replaced. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7e4a12db3a
ℹ️ 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".
|
@codex review |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
--refresh removed the transcript's ledger records before asking Codex to import it. When the import was then rejected (an RPC error, a reported failure, or no record written), the records stayed removed and the next transfer lost the mapping to the original thread. Put the removed records back on any failed refresh, unless Codex recorded the transcript again, keep other ledger updates, and name the ledger backup in the error. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
mangeshraut712
left a comment
mangeshraut712
left a comment
There was a problem hiding this comment.
Ledger lookups use fs.realpathSync(options.sourcePath), but the import request still sends the unresolved path.
In plugins/codex/scripts/lib/codex.mjs (importExternalAgentSession), canonicalSource is realpath’d for importRecordsForSource / --refresh forget-restore, while externalAgentSessionMigration(options.sourcePath, cwd) passes the original options.sourcePath through to Codex. If the caller passes a symlink or non-canonical path, Codex records a different source_path than the plugin later queries, so the “already imported” / post-import ledger match can miss the row this import just wrote (and --refresh may remove the wrong key).
Pass canonicalSource into the migration/import request as well, so the ledger key and the lookup always agree.
… intact From the adversarial review of the openai#835 merge. - Codex records source_path through std::fs::canonicalize, which on Windows gives an extended-length path (\\?\C:\..., \\?\UNC\...), while Node's realpath gives C:\.... Compared as raw strings, an already-imported transcript never matched its own record there, so a second transfer still failed and --refresh removed nothing. ledgerPathKey() strips the prefix and compares case-insensitively on win32, and every ledger match goes through it. This is from Codex's source; it is not verified on a Windows machine. - A symlinked ledger was replaced by a regular file, leaving its target stale. The rewrite now replaces the file the ledger resolves to. - The replacement and the backup lost the ledger's mode (a 0600 ledger came back 0644). Both now get the ledger's own mode, also under a restrictive umask. - Temp files were named by pid only and backups by millisecond, without wx, so either could overwrite another. Both now carry a random suffix and are created exclusively; a temp file is always removed. - Codex writes the ledger by truncating and writing it, so a read could land on half a file and fail the transfer. Reads now retry a parse failure a few times before giving up with the path in the error. Each of these has a test in tests/transfer-ledger.test.mjs that fails with its mechanism removed.
…LAUDE_CONFIG_DIR The openai#835 tests all use the default ~/.claude/projects, so nothing exercised the hand-resolved merge in executeTransfer, where a relocated transcript is imported from a copy staged under the default root. This walks that case: first transfer, unchanged, grown, --refresh, and a rejected --refresh. It checks that the ledger is keyed on the staged path, that the staged copy is gone each time, and that the earlier record comes back. Passing the original path instead of the staged one fails it.
…rdening of both Also stops saying Codex imports a transcript only once: newer Codex can import a grown transcript into the existing thread. The README now says which versions skip, and the already-transferred message says what happened (Codex skipped it this time) rather than why in general.
The ledger lookups and --refresh use the realpath of the transcript, so send that same path in the import request instead of relying on Codex to resolve it the same way. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@mangeshraut712 done in deae1e6, the import request now sends |
Import upstream openai#834 and openai#835, with hardening from an adversarial review
Fixes #750
Problem
Codex skips a Claude transcript it has already imported (it dedupes by path), even when the transcript has grown since. it still sends
import/completed, just with emptysuccessesandfailures. the plugin then can't find a ledger record matching the current file hash, so every/codex:transferafter the first one fails with "did not record an imported thread".Change
import/completednotification (successes[].target) and showfailures[]messages when there are any.--refreshwhen the transcript has changed since.--refreshflag. it backs up Codex's import ledger, removes this transcript's entry and imports the current state into a new thread.--refreshedits Codex's internal ledger, which is why it's opt-in and makes a backup. a re-import option in Codex itself would be cleaner.Notes
#701 and #469 also touch this lookup. this PR doesn't change path handling, so on Windows it still needs their fix for
\\?\paths. happy to rebase on whichever lands first.Tests
New tests cover an already-imported session,
--refresh, a failure reported by Codex, and a transcript that grows during the import. the fake Codex now skips already-imported paths like Codex 0.166 does.npm run buildpasses, andnpm testpasses except 3 stop-hook tests that fail the same way onmainlocally. I also checked it by hand against Codex CLI 0.166.0.