Repository navigation
fix(codex): pin Windows shim refresh identities on Linux and record the bypass-token cold-spawn disposition - #7408
Conversation
On Linux (ext4/tmpfs) a freed inode number is handed straight to the next file or symlink, and link(2) of a symlink source publishes the symlink itself. The refresh compared bare dev/ino, so a stage or aside swapped for a symlink or new file could carry the recorded numbers and be published or restored as the launcher. The stage and the moved wrapper are now held open for the whole transaction (an open inode cannot be reused) and identities include whether the entry is a symlink, so the guarantee no longer depends on platform link(2) or inode allocation. Fixes #7401
holdExisting now closes the descriptor on every unsuccessful path, including a throwing fstat, so the refresh hands the moved wrapper back instead of leaking the descriptor and leaving the launcher missing. Refs #7401
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. |
|
✅ Deterministic PR hygiene checks passed. |
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configuration
📒 Files selected for processing: 3
✨ Finishing Touches
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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5f66c1aefc
ℹ️ 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 movedFingerprint = shimPathFingerprint(aside); | ||
| moved = holdExisting(aside); |
There was a problem hiding this comment.
Bind the fingerprint to the held aside file
If a concurrent installer replaces aside after shimPathFingerprint() finishes but before holdExisting() performs its first lookup, movedFingerprint still describes the observed wrapper while moved pins the replacement. Because these results are validated independently, the transaction proceeds, publishes the generated shim, and then discard(aside, moved) deletes the concurrent install. Derive the fingerprint from the held descriptor, or explicitly verify that the fingerprint and held identity describe the same entry before proceeding.
Useful? React with 👍 / 👎.
Summary
Follow-up to #7397. It fixes the two real failures from the post-merge Ubuntu CI run (39331404182).
#7401: the Windows shim refresh identity check depended on Linux filesystem behavior.
src/codex/shim-windows-refresh.tsreplaces an obsolete Windows Codex wrapper by moving it aside, creating an exclusive stage, and hard-linking that stage into place. Each step checked that a file was still "ours" by comparing only device and inode numbers. Two Linux behaviors break that check, both verified on an ext4 host:link(2)on a symlink source creates a symlink entry. macOS follows the symlink instead.So on Linux, a stage swapped for a symlink was reported as published, and a substituted aside was restored as the launcher. The two round-4 tests caught this on Ubuntu but passed on macOS. It's a real defect in the code, not a test assumption, so the tests stay strict and are not skipped on Linux.
The fix makes the guarantee independent of platform behavior:
fstat, closes its descriptors and puts the wrapper back. That gap was found by the security re-review.#7400: missing cold-spawn disposition.
tests/codex-integration/codex-shim-bypass-token.test.ts(added in #7397) times spawned children against the deadline but had no entry intests/ci-workflows/cold-spawn-warmup.test.ts. It is now recorded as unwarmed, with the same reasoning as itscodex-shim-ensure-failureneighbour: the children are/bin/shstand-ins, so no repository module graph exists to warm.Fixes #7400
Fixes #7401
Verification
bun run typecheck,bun run structure:check,bun run privacy:scan: pass.bun test --isolateover the file-size and import-cycle ratchets, the test-layout guards, the core/Lab boundary,cold-spawn-warmup,codex-shim-bypass-tokenandcodex-shim-windows-revision: 115 pass, 0 fail (before the descriptor-hygiene test was added).codex-shim-windows-revisionalone is now 15/15 on macOS.codex-shim.test.tsandcodex-shim-readiness.test.ts: 14 pass, 23 platform skips, 0 fail.devon a separate host:devreproduces the CI failures (12 pass, 2 fail, the same two round-4 cases).codex-shim-windows-revision+cold-spawn-warmuppass 35/35 on each of 3 runs. The clone was deleted afterwards.Security review
SECURITY REVIEW: PASS
Dedicated sol (gpt-6.1-sol) review of the refresh change:
fstatleaked the aside descriptor and left the launcher missing. Fixed, with a regression test.Checklist
Summary by CodeRabbit