fix(atomic): close descriptor mode gaps

[steipete]

Summary

• keep the POSIX parent no-follow identity check active for synchronous adapters that omit fchmodSync
• apply fallback replacement modes before synchronizing the actual destination, and restore both bytes and mode when bounded restoration fails
• retain source compatibility for legacy adapter literals that still expose unused chmod or chmodSync methods

The audit also expands exact-mode coverage across umasks 000, 022, 077, and 777, verifies present and missing preserveExistingMode destinations for async and sync calls, counts descriptors after a forced fallback-mode failure, and documents the Windows staged-directory mode caveat.

Verification

• pnpm check
• pnpm test:security
• git diff --check
• strict TypeScript compile of the atomic adapter regression file
• packaged, source-blind behavior validation of mode, adapter, fallback durability, restoration, cleanup, and descriptor outcomes
• Codex autoreview: clean, no accepted/actionable findings

Escalated limitation

With a POSIX process umask of 0777, a newly created directory lands as mode 000 and cannot be reopened for descriptor-bound mode application. This affects new atomic parent directories and staged directory move fallback. This PR does not reintroduce pathname chmod or mutate the process-wide umask; resolving that case safely needs an explicit design decision or a descriptor-relative creation primitive.

[clawsweeper]

Codex review: needs real behavior proof before merge. Reviewed August 2, 2026, 7:37 PM ET / 23:37 UTC.

ClawSweeper review

What this changes

The PR keeps synchronous atomic parent validation active without fchmodSync, applies copy-fallback modes before durable sync, restores original bytes and modes after bounded fallback failures, and adds focused mode regressions.

Merge readiness

⛔ Blocked until real behavior proof is added - 4 items remain

This PR is still necessary: current main contains the earlier descriptor-mode work but not these fallback-order, restoration-mode, and missing-fchmodSync safeguards. The patch is coherent and has no discrete correctness finding from source review, but it needs inspectable after-fix runtime evidence and a maintainer security-boundary decision on the documented POSIX umask 0777 limitation before merge.

Priority: P2
Reviewed head: b1d54f27a07d4c4c3490e5f9a7e2a91e810bb27e
Owner decision: Required. See Decision needed.

Review scores

Measure	Result	What it means
Overall readiness	🧂 unranked krab (1/6)	The patch is focused and well-covered in source, but missing inspectable real-behavior proof remains a merge gate.
Proof confidence	🧂 unranked krab (1/6)	Needs real behavior proof before merge: The PR body lists checks and a packaged validation but provides no inspectable after-fix terminal output, logs, recording, or artifact; add redacted runtime evidence before merge. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Patch quality	🐚 platinum hermit (4/6)	No actionable review findings were identified.

Verification

Check	Result	Evidence
Real behavior	Needs proof	Needs real behavior proof before merge: The PR body lists checks and a packaged validation but provides no inspectable after-fix terminal output, logs, recording, or artifact; add redacted runtime evidence before merge. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed	6 items	Current main lacks this patch: The PR head is not an ancestor of current main, so its fallback ordering and adapter changes are not already implemented there. Descriptor-bound fallback change: The proposed copy fallback now applies the replacement mode through the destination descriptor before the requested final sync, and restoration writes both original bytes and mode before syncing. Fail-closed parent validation remains active: The synchronous directory helper no longer skips identity validation when fchmodSync is omitted; it opens and verifies the directory, then conditionally applies its mode.
Findings	None	None.
Security	None	None.

How this fits together

The atomic replacement subsystem stages a file, verifies the target parent, then publishes the replacement by rename or a guarded copy fallback. It is used by higher-level safe writes, so its descriptor identity checks, modes, and durability behavior affect filesystem confinement and persisted-file correctness.

flowchart LR
  Request[Atomic replacement request] --> Parent[Create and verify parent directory]
  Parent --> Stage[Write staged file by descriptor]
  Stage --> Publish[Rename or guarded copy fallback]
  Publish --> Mode[Apply destination mode by descriptor]
  Mode --> Durable[Sync file and parent when requested]
  Durable --> Result[Constrained published file]

Loading

Decision needed

Question	Recommendation
Should this PR merge while preserving the documented fail-closed behavior for newly created POSIX atomic directories under process umask 0777, or should that creation-path limitation be resolved first?	Merge the narrow hardening and track creation separately: Accept the documented fail-closed result under umask 0777 while landing the symlink, mode-ordering, and restoration safeguards.

Why: The safe fix requires either accepting an explicit availability limitation or choosing a new descriptor-relative creation design; source review cannot select that security and compatibility tradeoff.

Before merge

• Add real behavior proof - Needs real behavior proof before merge: The PR body lists checks and a packaged validation but provides no inspectable after-fix terminal output, logs, recording, or artifact; add redacted runtime evidence before merge. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
• Resolve merge risk (P1) - With a POSIX process umask of 0777, newly created atomic parent or staged directories can be mode 000 and cannot be reopened for descriptor-bound mode application; the PR documents and preserves a fail-closed outcome rather than solving that creation-path limitation.
• Resolve merge risk (P2) - This security-sensitive change alters the fallback publication path, so merge should retain the descriptor-only invariant and receive real packaged runtime evidence rather than relying only on test and CI claims.

Agent review details

Security

None.

Review metrics

Metric	Value	Why it matters
Changed surface	8 files: 3 source, 3 regression tests, 1 doc, 1 changelog	The production changes stay within atomic replacement and are paired with focused regression coverage and contract documentation.
Regression coverage	316 test lines added	The new tests exercise restrictive umasks, copy-fallback ordering, restoration, adapter compatibility, and descriptor cleanup.

Merge-risk options

Maintainer options:

1. Require runtime proof and boundary sign-off (recommended)
   Before merge, attach redacted packaged or terminal evidence for the fallback paths and record maintainer acceptance of the documented umask 0777 fail-closed behavior.
2. Pause for descriptor-relative creation
   Keep the PR open without merging if the project requires new atomic directories to work under umask 0777 before accepting the hardening.

Technical review

Best possible solution:

Retain descriptor-bound mode and identity handling, land this narrow hardening only after redacted runtime proof is attached and maintainers explicitly accept the fail-closed umask 0777 behavior while tracking any descriptor-relative creation design separately.

Do we have a high-confidence way to reproduce the issue?

Yes for the addressed paths: the added tests define current source-level reproductions for missing synchronous parent validation, fallback mode ordering, and mode restoration. The umask 0777 directory limitation is also explicitly documented, but it requires a design decision rather than a narrow regression assertion.

Is this the best way to solve the issue?

Unclear: descriptor-bound validation and mode application are the safest repair direction, but maintainers must decide whether the documented fail-closed umask 0777 limitation is acceptable before merge.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 9bb846538fa5.

Labels

Label justifications:

• P2: This is a bounded atomic-write correctness and security hardening change with limited but important filesystem impact.
• merge-risk: 🚨 security-boundary: The patch changes descriptor identity validation, mode application, and fallback publication behavior at a filesystem confinement boundary.
• rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🐚 platinum hermit.
• status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body lists checks and a packaged validation but provides no inspectable after-fix terminal output, logs, recording, or artifact; add redacted runtime evidence before merge. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Evidence

What I checked:

• Current main lacks this patch: The PR head is not an ancestor of current main, so its fallback ordering and adapter changes are not already implemented there. (src/replace-file.ts:128, b1d54f27a07d)
• Descriptor-bound fallback change: The proposed copy fallback now applies the replacement mode through the destination descriptor before the requested final sync, and restoration writes both original bytes and mode before syncing. (src/replace-file-copy-fallback.ts:255, b1d54f27a07d)
• Fail-closed parent validation remains active: The synchronous directory helper no longer skips identity validation when fchmodSync is omitted; it opens and verifies the directory, then conditionally applies its mode. (src/replace-file-descriptor.ts:65, b1d54f27a07d)
• Focused regression coverage: The branch adds coverage for symlinked synchronous parents without fchmodSync, exact modes across restrictive umasks, fallback sync ordering, restoration, and descriptor cleanup. (test/atomic-fchmod-regression.test.ts:102, b1d54f27a07d)
• Feature provenance: Current-main atomic descriptor-mode work appears to date to the mode-binding and directory-mode commits; the PR follows that same surface with an additional gap closure. (src/replace-file-copy-fallback.ts:249, da387a72a840)
• Release boundary: The latest release tag is v0.5.1 at commit 16e1bd489ae8a1c0e752797ca1e8501a36790d98; both the current-main descriptor fixes and this PR are newer, so the proposed behavior is not released. (CHANGELOG.md:4, 16e1bd489ae8)

Likely related people:

• steipete: The current-main commits that introduced descriptor-bound replacement and directory modes, as well as this follow-up patch, are authored by Peter Steinberger under this GitHub handle. (role: recent atomic-mode contributor; confidence: high; commits: da387a72a840, 14448ba3bb34, b1d54f27a07d; files: src/replace-file.ts, src/replace-file-descriptor.ts, src/replace-file-copy-fallback.ts)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

• Attach redacted packaged or terminal evidence showing the actual fallback, restoration, and descriptor outcomes after the fix.
• Obtain maintainer confirmation that the documented POSIX umask 0777 fail-closed limitation is acceptable for this merge.

Rating scale

Score	Internal tier	Crab rank	Meaning
6/6	S	🦀 challenger crab	Exceptional readiness
5/6	A	🦞 diamond lobster	Very strong readiness
4/6	B	🐚 platinum hermit	Good normal PR; ordinary maintainer review
3/6	C	🦐 gold shrimp	Useful, but confidence is limited
2/6	D	🦪 silver shellfish	Proof or implementation needs work
1/6	F	🧂 unranked krab	Not merge-ready
N/A	NA	🌊 off-meta tidepool	Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

• ClawSweeper keeps one durable marker-backed review comment per issue or PR.
• Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
• A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
• PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
• Maintainers can also comment @clawsweeper review to request a fresh review only.
• Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
• Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
• Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (1 earlier review cycle)

• reviewed 2026-08-02T23:01:44.510Z sha 4b13d0e :: needs real behavior proof before merge. :: none