infra: atomically replace sync JSON writes#60589
Conversation
Greptile SummaryThis PR replaces synchronous direct writes in The implementation is correct. The only noteworthy finding is a redundant Confidence Score: 5/5Safe to merge; the atomic write semantics are correct and the only finding is a redundant chmod that has no runtime impact. All findings are P2 (style/cleanup). The core correctness property — atomic replace via same-directory renameSync — is implemented correctly. No data-integrity or reliability risk introduced. No files require special attention. Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/infra/json-file.ts
Line: 29-33
Comment:
**Redundant `chmodSync` after rename**
On POSIX, `renameSync` atomically replaces the destination and carries over the source file's permissions — `tmpPath` already has `0o600` set both via the `mode` option in `writeFileSync` and the preceding `chmodSync(tmpPath, 0o600)`. The post-rename `chmodSync(pathname, 0o600)` is therefore a no-op on every platform that supports `chmod`, and a no-op on Windows too (where chmod is ignored). Consider removing it to simplify the flow.
```suggestion
fs.renameSync(tmpPath, pathname);
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "infra: atomically replace sync JSON writ..." | Re-trigger Greptile |
There was a problem hiding this comment.
Pull request overview
Updates the infra JSON file helpers to reduce TOCTOU risk and prevent readers from observing partially-written JSON by switching saves to a temp-file + rename flow, and simplifying loads by removing pre-read existence checks.
Changes:
- Replace direct JSON writes with sibling temp-file writes followed by
renameSyncreplacement. - Remove
existsSyncpre-check from JSON reads and rely onreadFileSync/JSON.parsefailure fallback. - Add a regression test asserting the temp-path → destination rename behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/infra/json-file.ts | Implements atomic(ish) JSON saves via temp file + rename and simplifies loadJsonFile by removing existsSync pre-check. |
| src/infra/json-file.test.ts | Adds coverage for the temp-file replacement flow using a renameSync spy. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 507e40ad90
ℹ️ 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: b7a5995c8b
ℹ️ 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: dd3072f322
ℹ️ 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".
60ee5ec to
cb8ed77
Compare
|
Merged via squash.
Thanks @gumadeiras! |
Merged via squash. Prepared head SHA: cb8ed77 Co-authored-by: gumadeiras <5599352+gumadeiras@users.noreply.github.com> Co-authored-by: gumadeiras <5599352+gumadeiras@users.noreply.github.com> Reviewed-by: @gumadeiras
Merged via squash. Prepared head SHA: cb8ed77 Co-authored-by: gumadeiras <5599352+gumadeiras@users.noreply.github.com> Co-authored-by: gumadeiras <5599352+gumadeiras@users.noreply.github.com> Reviewed-by: @gumadeiras
Merged via squash. Prepared head SHA: cb8ed77 Co-authored-by: gumadeiras <5599352+gumadeiras@users.noreply.github.com> Co-authored-by: gumadeiras <5599352+gumadeiras@users.noreply.github.com> Reviewed-by: @gumadeiras
Merged via squash. Prepared head SHA: cb8ed77 Co-authored-by: gumadeiras <5599352+gumadeiras@users.noreply.github.com> Co-authored-by: gumadeiras <5599352+gumadeiras@users.noreply.github.com> Reviewed-by: @gumadeiras
Summary
existsSynccheck so reads rely on direct read/parse fallbackVerification
pnpm format src/infra/json-file.ts src/infra/json-file.test.tssrc/infra/json-file.ts(parseFailures: 0across 835 reads)pnpm test -- src/infra/json-file.test.tshung in this environment, so I did not count it as passingPart of #54295.