Repository navigation
Import upstream #834 and #835, with hardening from an adversarial review - #27
Merged
Merged
Conversation
formatUntrackedFile followed symlinks, so an untracked link to a file outside the repository put that file's contents into the review prompt. Use lstat and skip symlinks and other non-regular files. Fixes openai#828
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>
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>
--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>
… symlink in review context
openai#834 checks an untracked file with lstat and skips symlinks, then reads it with readFileSync, which follows them. Between the two calls the file can be replaced by a symlink to something outside the repository, and the target then lands in the review prompt after all. readRegularFileNoFollow() opens with O_NOFOLLOW, so a symlink is refused at open (ELOOP, reported as "symlink"), and checks and reads that same descriptor: what is read is what was checked, including the size limit. Windows has no O_NOFOLLOW; the lstat check stays and is all there is there. The new test points the path at a symlink to a file outside the directory and expects a refusal. Without O_NOFOLLOW it fails.
…dex:transfer and add --refresh Conflict in plugins/codex/scripts/codex-companion.mjs, two hunks: - printUsage: the fork's usage lines are kept (aliases, --ephemeral, --sandbox, --read-root, --resume-thread) and transfer gains [--refresh]. - executeTransfer: the fork imports the transcript through prepareClaudeSessionImport(), which stages a copy under the default Claude projects root when CLAUDE_CONFIG_DIR is relocated (openai#729). The staging and its cleanup are kept, and upstream's `refresh` option is passed to importExternalAgentSession() alongside the prepared import path, so the ledger lookups and --refresh act on the path Codex actually imported. The --refresh hint still names the user's own --source path. The rest merged cleanly, including lib/codex.mjs. Before this, a second transfer of the same session failed in the fork exactly as upstream's openai#750 describes: "did not record an imported thread".
openai#835 swaps in the rewritten external_agent_session_imports.json with a bare fs.renameSync(). On Windows that rename over an existing file can be refused with a sharing violation while a scanner or indexer holds the ledger open, which would fail a --refresh for no reason of its own. The fork routes every file replacement through renameReplacing(), which retries those codes on Windows and is a plain rename elsewhere. Caught by the fork's guard "plugin scripts replace files through the Windows-aware rename", which failed on both full-suite runs after the merge and passes now.
… 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.
…bounded From the adversarial review of the openai#834 follow-up. - O_NOFOLLOW only covers the last component. If git listed realdir/f.txt and realdir was then replaced by a symlink to a directory outside the repository, the read followed it (reproduced: PARENT_SECRET landed in the review context). The file actually opened must now lie inside the repository root: read from the descriptor itself on Linux (/proc/self/fd), from the path's realpath elsewhere, which narrows that window there without closing it. - A FIFO swapped in between the lstat and the open blocked the open, and the whole review with it. The open now carries O_NONBLOCK. - readFileSync(fd) read to EOF, so a file that grew after the size check was read past the limit. The read now stops at maxBytes + 1 bytes and skips the file when it got there. Tests: the follow-up's test claimed to simulate a swap and did not; it is renamed to what it checks. New tests swap the directory between git's listing and the read (through collectReviewContext), open a real FIFO in a child process with a timeout (a blocking open would freeze an in-process test for good), and grow a file past the limit after the size check. Each fails with its mechanism removed.
…er reads it The hint quoted a path with JSON.stringify, but splitRawArgumentString takes a quoted string literally, with no escapes inside. A path with a double quote came back wrong: /home/a"b/x.jsonl was read as /home/a\b/x.jsonl" (checked against the splitter before the change), and a Windows path printed doubled backslashes. quoteRawArgument() is the splitter's inverse: a value goes in the quote it does not contain, or is escaped character by character when it holds both. A test runs paths with spaces, both quotes, backslashes, tabs and the empty string through both and gets each back unchanged.
…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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This imports two upstream PRs, each as its own merge commit with upstream authorship kept. The fork's own fixes land on top as separate commits, so each one can be reviewed or reverted alone.
openai/codex-plugin-cc#834: untracked symlinks in the review prompt
The fork had the same bug as upstream.
formatUntrackedFileusedstatSync, so an untracked symlink to a file outside the repository (~/.ssh/id_rsa, say) put that file's contents in the prompt sent to Codex. Upstream's fix useslstatand skips symlinks and non-regular files. It merged cleanly, and reverting it fails its two tests.The check and the read are still separate steps, so on top of it the fork's
readRegularFileNoFollow()(ca34900,c50692f) does the following:O_NOFOLLOW, so a file swapped for a symlink after thelstatis refused;/proc/self/fd; elsewhere it uses the path's realpath, which narrows the window without closing it. The review reproduced the leak this catches: oncerealdir/f.txtis listed by git, replacingrealdirwith a link to an outside directory put that directory's file in the prompt;O_NONBLOCK, so a FIFO swapped in cannot block the review forever;openai/codex-plugin-cc#835:
/codex:transferof an already-imported sessionThe fork had the same bug as upstream: a second transfer of the same session failed with "did not record an imported thread". After this change, transfer prints the existing thread and says whether later turns are missing from it, and
--refreshre-imports the current transcript into a new thread.executeTransfer/printUsage. The fork imports throughprepareClaudeSessionImport(), which stages a copy under~/.claude/projectswhenCLAUDE_CONFIG_DIRis relocated. That staging is kept, andrefreshis passed along with the prepared import path, so the ledger lookups act on the path Codex actually recorded.lib/codex.mjsfails six upstream tests plus the ledger test file. A new test covers the relocated-config case end to end, and it fails if the original path is passed instead of the staged one.Fork fixes on top:
75908b1: the ledger was replaced with a barefs.renameSync. The fork's guard test caught this. It now goes throughrenameReplacing(), which retries the Windows sharing violations a plain rename can hit.9129b90:source_pathas an extended-length path (\\?\C:\...), so on Windows neither detection nor--refreshever matched a record. Paths are now normalised and compared case-insensitively. This is taken from Codex's source and is not verified on a Windows machine.wx, so one refresh can no longer overwrite another's backup.eb96913: the printed--sourcein the refresh hint is quoted the waysplitRawArgumentStringreads it. Before, a path containing"came back wrong.Verification
node --test tests/*.test.mjs: 447/447 on two full runs, with none of this run's brokers left running.npx tsc -p tsconfig.app-server.json: clean.Known and not fixed here
--refreshis killed after removing the records, nothing restores them. The backup remains, so they can be recovered by hand.Generated by Claude Code