[Mnigos]

Atomic replacement previously discarded the destination file’s permissions. Saving a private state file could make it readable according to the process umask, and an explicitly group-writable file could lose that setting.

Read the resolved destination’s permission bits and apply them to the temporary file before renaming it. New files use 0600. The temporary directory stays private while the replacement is prepared, and existing symlink behavior is preserved.

Validation after resolving conflicts with current main: all seven current filesystem tests pass on Linux under umasks 022, 002, and 077; the three permission assertions remain folded into existing cases. Main’s removal of redundant tests is preserved, with the new-file 0600 assertion moved to the surviving dangling-symlink case. Shared-package typecheck, scoped lint, and format pass. Lint reports main’s existing polling-helper warning; no suppression was added. Native Windows and macOS execution was not performed.

The merge preserves the PR’s production atomicWrite.ts byte for byte, including resolved-target stat, chmod before rename, and 0600 for missing targets; main’s Windows rename retry behavior is retained. This conflict resolution adds no production behavior, and the earlier independent reviews are historical, not fresh reviews of this merge.

The shared permission change remains subject to the configured human-review gate. No merge or auto-merge is requested by this takeover.

Original contribution by @Mnigos. Maintainer takeover: Claude Opus 5.5 for tests/review, Claude Fable 5.1 for implementation, GPT-6.1 for independent review; Claude Code and Codex harnesses.

Conflict resolution: GPT-6.1 Sol through the Codex harness.

[macroscopeapp]

Approvability

Verdict: Needs Human Approval

Macroscope's review found this PR requires human approval — This focused fix changes a shared production atomic-write path used for settings, state, caches, ledgers, and potentially secret-bearing files by altering filesystem permissions. Because the change affects confidentiality and permission semantics across multiple callers, it warrants human review despite its small size and targeted tests.

^{You can add or adjust custom eligibility rules. Learn more.}

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

• Last reviewed commit: 71efd5c
• Latest commit, not reviewed: 27860a8

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: Path: .coderabbit.config.ts
• Review profile: CHILL
• Plan: Advanced
• Run ID: d343e891-0a32-4dc7-b598-5e9deb5fa74a

📥 Commits

Reviewing files that changed from the base of the PR and between 68d4107 and 71efd5c.

📒 Files selected for processing: 2

• packages/shared/src/atomicWrite.test.ts
• packages/shared/src/atomicWrite.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 4 remain after this review.

📝 Walkthrough

writeFileStringAtomically now preserves existing target permission bits and uses mode 0600 for new files. Tests cover permissions, exact UTF-8 contents, temporary-directory cleanup, and write failure behavior.

Changes: Atomic write permissions

Layer / File(s)	Summary
Select and apply file permissions packages/shared/src/atomicWrite.ts	The function reads the target’s permission bits, uses 0600 when the target is absent, and applies the selected mode to the temporary file before renaming it.
Verify permissions and write outcomes packages/shared/src/atomicWrite.test.ts	Tests check preservation of existing modes, 0600 for new files, exact UTF-8 contents, cleanup after success, and failure behavior. Permission checks are skipped on Windows; the read-only-parent test also skips when run as root.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~15 minutes

Change: Bug fix · Severity of issue fixed: Low

Suggested reviewers: juliusmarminge

Merge Risk: ⚪ Minimal · up to 71efd

The permission-preserving change has no established merge-blocking issue; proceed with normal checks.

Security architecture risk: 🔵 Low · up to 71efd

The change improves file-permission handling without a demonstrated new attack path. Directory access controls, temporary-file privacy, and platform-specific filesystem behavior remain incompletely validated.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• inferred — The shared helper's permission change is inherited by its callers. For the examined ACP consumer, the affected asset is per-instance cached sign-in state under the server-configured cache directory. Source does not establish which local actors can traverse, replace, or modify that directory at runtime.

Security Findings and Attack Paths

• inferred — No introduced or worsened authentication attack path was demonstrated. The comparison leaves the ACP authentication-state implementation unchanged and adds permission controls in its existing filesystem sink. This does not resolve the unknown upstream authority to alter provider configuration or cache files.

Trust Boundaries and Controls

• observed — Contents are written before chmod. Pre-chmod confidentiality therefore depends on the temporary directory's access restrictions. The code asserts that directory is 0700, but the version-applicable adapter implementation was unavailable to verify the assertion. The same temporary-write location and ordering existed before this PR.

Resilience and Maintainability Implications

• observed — Permission selection is a snapshot taken before the write, without cross-process serialization against concurrent chmod or replacement. Tests assert existing and new-file modes and selected failure outcomes, but skip Windows mode assertions. No demonstrated concurrency-related security regression or supported-platform runtime failure was established.

Hardening Proposals

• proposed — Validate private temporary-directory creation using the pinned adapter and validate chmod behavior on supported Windows filesystems. If private staging is not guaranteed, establish restrictive access before writing contents. Separately verify runtime cache-directory ownership and ACLs; file modes alone do not establish that boundary.

Pre-merge checks: 3 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Linked Issues check		Issue #14967 requires permission preservation or a restrictive default for new files. The PR implements stat.mode & 0o777, 0o600 for new files, and applies the mode before rename. Issue #14967 als…	Add an fsync for the temporary file before rename and an fsync for the parent directory after rename. Add automated coverage for the durability sequence or its observable failure behavior.

Passed checks: 3 passed

Check name	Status	Explanation
Out of Scope Changes check		The changes are limited to writeFileStringAtomically and its tests. Mode preservation, restrictive defaults, symlink referent behavior, content checks, and cleanup checks support the requirements in…
Title check		The title clearly identifies the main change: atomic writes preserve destination file permission bits. It uses a concise conventional-commit format.
Description check		The description clearly explains the problem, implementation, verification results, scope limits, platform limitations, and agent involvement. It does not include a direct link to the triaged issue or…

Full details: Out of Scope Changes check

Explanation

The changes are limited to writeFileStringAtomically and its tests. Mode preservation, restrictive defaults, symlink referent behavior, content checks, and cleanup checks support the requirements in issue #14967.

Full details: Description check

Explanation

The description clearly explains the problem, implementation, verification results, scope limits, platform limitations, and agent involvement. It does not include a direct link to the triaged issue or maintainer approval required by the Scope and approval section, but the remaining required information is substantially complete.

Full details: Linked Issues check

Explanation

Issue #14967 requires permission preservation or a restrictive default for new files. The PR implements stat.mode & 0o777, 0o600 for new files, and applies the mode before rename. Issue #14967 also requires crash durability: fsync the temporary file, rename it, and fsync the parent directory. The reported implementation adds neither fsync operation.

Resolution

Add an fsync for the temporary file before rename and an fsync for the parent directory after rename. Add automated coverage for the durability sequence or its observable failure behavior.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

_{Comment @coderabbitai help to get the list of available commands.}

[juliusmarminge]

Taking over this one. I’ll reduce the tests to the useful coverage, update the implementation against current main, and verify it before publishing the changes.

[juliusmarminge]

Thank you, @Mnigos, for the earlier work on preserving atomic-write permissions. This PR has had no author push or reply for over a week, and main has changed the same files since its base. #24310 by @juliusmarminge covers the same permission issue and includes broader regression and runtime evidence, so we are keeping it as the duplicate.