fix: preserve symlinked destinations in atomic config writes - #869
fix: preserve symlinked destinations in atomic config writes#869nicosuave wants to merge 3 commits into
Conversation
|
✅ PR quality gates passed This pull request now targets The |
github-actions
Bot
changed the title
|
Warning Review limit reached
Next review available in: 20 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughAtomic writes resolve symlinked destinations before temporary-file creation and renaming. Response-state recovery scans both configured and resolved target directories. Tests cover write behavior, stale temporary-file cleanup, and the real-home guard. ChangesSymlink-safe filesystem operations
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
github-actions
Bot
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/config.test.ts`:
- Around line 1673-1731: Add regression tests for atomicWriteFileAsync alongside
the existing atomicWriteFile coverage: verify an existing symlink remains intact
while its target receives the rewritten content, and verify a dangling symlink
is replaced with the new file. Await each asynchronous write and assert the same
symlink-preservation and recovery behavior covered by the synchronous tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ca45f95a-945b-4a2d-9303-c8971eaf076e
📒 Files selected for processing (2)
src/config.tstests/config.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/config.test.ts`:
- Around line 1773-1780: Update the test around atomicWriteFileAsync to
explicitly verify that link is no longer a symbolic link after the write, using
the appropriate filesystem link-type assertion while retaining the existing
content check.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c4c155ba-bd7f-4c88-887c-2ad7039d336b
📒 Files selected for processing (1)
tests/config.test.ts
![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: dfd341281a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| { | ||
| "name": "@bitkyc08/opencodex", | ||
| "version": "2.8.0", | ||
| "version": "2.9.0", |
There was a problem hiding this comment.
This fix commit changes the package identity from 2.8.0 to 2.9.0 even though the repository's release flow makes scripts/release.ts the release authority; if this lands as an ordinary symlink-preservation fix before the actual release commit/tag/workflow, source builds and health/update surfaces will report 2.9.0 while the npm/tag/GitHub release artifacts for that version do not exist yet. Please leave the package version unchanged here and let the release script create the version-bump commit.
AGENTS.md reference: AGENTS.md:L23-L24
Useful? React with 👍 / 👎.
| }; | ||
| const tmp = `${path}.ocx.${process.pid}.${++_atomicSeq}.tmp`; | ||
| const target = resolveWriteTarget(path); | ||
| const tmp = `${target}.ocx.${process.pid}.${++_atomicSeq}.tmp`; |
There was a problem hiding this comment.
When responses-state.json is symlinked, this creates the async snapshot temp beside the resolved target (and under the target basename), but startup recovery still calls recoverStaleResponseStateTemps(dirname(path)) and only matches responses-state.json.ocx... in the literal config directory. If the process dies between the temp write and rename, the private previous-response snapshot temp is left permanently in the dotfiles/target directory; either keep this temp name/location discoverable by the existing recovery pass or teach recovery to scan the resolved target location too.
Useful? React with 👍 / 👎.
| }): void { | ||
| recordOwnedConfigPath(resolveConfigDir(), path); | ||
| const tmp = `${path}.ocx.${process.pid}.${++_atomicSeq}.tmp`; | ||
| const target = resolveWriteTarget(path); |
There was a problem hiding this comment.
In an armed test process, callers such as saveConfig validate only getConfigDir() before reaching this helper; after this change a temp OPENCODEX_HOME containing config.json -> <real home>/.opencodex/config.json now resolves through the symlink and writes the protected real file, whereas the old rename would have replaced only the symlink in the temp directory. Please run the real-home guard against the resolved target or otherwise refuse symlink targets that escape the allowed test home before writing.
Useful? React with 👍 / 👎.
atomicWriteFile and atomicWriteFileAsync wrote a temp file beside the literal destination path and renamed over it. rename(2) replaces a directory entry, so when the destination was itself a symlink the rename destroyed the link and left a plain file in its place. This breaks dotfiles-managed setups, where ~/.codex/config.toml is a symlink into a tracked repo. The first injected write silently converts it to a real file and the repo stops receiving updates. Nothing surfaces the divergence: the live config keeps working, so the stale repo copy looks current until someone diffs it. Resolve the destination through realpath before choosing the temp path. Both the temp file and the rename target then live inside the link's real directory, so the entry replaced is the real file and the link survives. Same-filesystem atomicity is preserved because the temp stays beside its resolved target, and an unresolvable path falls back to the literal path, which is correct for a first write and for a dangling link.
The symlink regression tests only exercised the synchronous atomicWriteFile, so an async-only regression could have slipped through while the sync suite stayed green. Mirror all four cases against atomicWriteFileAsync: an existing symlink survives and its target receives the write, no temp file is left beside either the link or its target, a plain destination is unaffected, and a dangling symlink is replaced rather than followed into nothing. Verified by reverting the async resolveWriteTarget call in isolation -- the symlink case fails, and passes again once restored.
Two follow-ups from review of the symlink-preservation change. Re-check the real-home guard against the resolved target. Callers such as saveConfig validate only their logical config dir, which passes when OPENCODEX_HOME points at a temp fixture. Resolving a symlink out of that fixture could then land on the protected home the caller had just cleared, so the guard now runs again on wherever the write actually terminates. Inert in production, where the guard is disarmed. Sweep stale response-state temps in the resolved directory too. Temps are created beside the resolved target, so a symlinked responses-state.json stranded them in the link's real directory while startup recovery only scanned the literal config dir. A crash between write and rename would have left a private snapshot temp there permanently. Both directories are now swept, collapsing to one when nothing is symlinked. Also assert the dangling-symlink cases replace the link rather than following it: reading through the link passed either way.
2f2c84c to
5bb6aa0
Compare
1 participant
Summary
atomicWriteFile and atomicWriteFileAsync wrote a temp file beside the literal destination path and renamed over it. rename(2) replaces a directory entry, so when the destination was itself a symlink the rename destroyed the link and left a plain file in its place.
This breaks dotfiles-managed setups, where ~/.codex/config.toml is a symlink into a tracked repo. The first injected write silently converts it to a real file and the repo stops receiving updates. Nothing surfaces the divergence: the live config keeps working, so the stale repo copy looks current until someone diffs it.
Resolve the destination through realpath before choosing the temp path. Both the temp file and the rename target then live inside the link's real directory, so the entry replaced is the real file and the link survives. Same-filesystem atomicity is preserved because the temp stays beside its resolved target, and an unresolvable path falls back to the literal path, which is correct for a first write and for a dangling link.
Verification
Ran tests
Summary by CodeRabbit