Skip to content

fix(write): tolerate raw stat modes with file-type bits in dirMode - #228

Merged
steipete merged 1 commit into
mainfrom
steipete/serene-roentgen-60767c
Sep 4, 2026
Merged

fix(write): tolerate raw stat modes with file-type bits in dirMode#228
steipete merged 1 commit into
mainfrom
steipete/serene-roentgen-60767c

Conversation

@steipete

@steipete steipete commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Problem

0.8.0 regressed input tolerance for replaceFileAtomic({ dirMode }): passing a raw fs.stat mode (e.g. 0o40755 from stat.mode, which includes the S_IFDIR type bits) now fails with path-mismatch: directory final mode could not be verified.

Root cause: ownDirectoryMode.apply() compares the requested mode unmasked against the pinned directory descriptor's mode, which inspect() reports masked to 0o7777. chmod itself ignores file-type bits, so pre-0.8.0 tolerated the raw value; the final-mode verification introduced in 0.8.0 did not.

This broke openclaw/openclaw's doctor-session-transcripts.test.ts in CI (openclaw/openclaw#138500).

Fix

Mask requested directory modes to 0o7777:

  • src/directory-mode-owner.ts — at the apply() comparison point, covering every owner-based entry point (applyDirectoryMode/replaceFileAtomic, sibling-temp, secret-file, archive-merge).
  • src/replace-file-descriptor.ts — before the synchronous fchmodSync in applyDirectoryModeSync for parity (that path has no verification compare, but the mask keeps requested-mode semantics consistent).

This widens input tolerance only; the mode actually applied and verified is unchanged, so no confinement or identity boundary is weakened.

Regression test

test/atomic-dirmode-regression.test.ts now passes a raw stat.mode (asserted to include S_IFDIR) as dirMode through both async replaceFileAtomic and replaceFileAtomicSync, verifying success and the final 0o755 mode.

Proof

  • New test fails on the base commit with exactly the reported error: FsSafeError: directory final mode could not be verified at directory-mode-owner.ts:76; passes with the fix.
  • pnpm vitest run test/atomic-dirmode-regression.test.ts test/sibling-temp-directory-mode.test.ts test/directory-mode-owner.test.ts → 21 passed.
  • pnpm lint:file-size && pnpm lint:fs-boundary && pnpm build && pnpm docs:check && node scripts/check-pack.mjs → all pass.
  • Full pnpm test: remaining failures (consumer-pnpm-lifecycle pnpm-CLI environment requirement, plus timing-sensitive stress/archive tests under parallel load) reproduce identically on clean main; every one passes when re-run serially with this change applied. archive-directory-publication and archive-publication-modes (closest to the change) pass 3/3 repeat runs.
  • Codex autoreview (gpt-5.6-sol, high): scoped-clean, no accepted/actionable findings.

0.8.0 regressed input tolerance for replaceFileAtomic({ dirMode }): a
raw fs.stat mode such as 0o40755 (S_IFDIR | 0o755) was compared
unmasked against the pinned directory's masked mode (0o7777), failing
with 'directory final mode could not be verified' even though chmod
ignores file-type bits.

Mask requested directory modes to 0o7777 at the ownDirectoryMode.apply
comparison point (covering applyDirectoryMode, sibling-temp,
secret-file, and archive-merge) and before the synchronous fchmod in
applyDirectoryModeSync. Add a regression test passing a raw stat mode
through async and sync replaceFileAtomic.
@steipete
steipete requested a review from a team as a code owner September 4, 2026 22:31
@clawsweeper

clawsweeper Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@clawsweeper clawsweeper Bot added P2 Normal priority bug or improvement with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Sep 4, 2026
@clawsweeper

clawsweeper Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed September 4, 2026, 6:34 PM ET / 22:34 UTC.

ClawSweeper review

What this changes

The PR masks raw fs.stat().mode directory values to permission and special-mode bits before atomic-write helpers apply and verify them.

Regression provenance

Possible regression — probable (reviewed change; failure trace). No predecessor PR is attributed.

Merge readiness

Ready for maintainer review

Keep open: this is a necessary, narrowly scoped fix for a released directory-mode regression, with no actionable defect found in the introduced patch.

Priority: P2
Reviewed head: 7d2923979941fa17d9bf12a81f911fe59c820bf7

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) A small, security-conscious compatibility repair with focused real-path proof and no identified correctness finding.
Proof confidence 🐚 platinum hermit (4/6) Sufficient (terminal): The changed production owners are the descriptor-based async and sync directory-mode paths; the PR body records a POSIX base failure with the exact verification error, followed by successful real atomic writes and preserved 0o755 modes after the fix. The focused test command is reported as 21 passing tests.
Patch quality 🦞 diamond lobster (5/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Verified Sufficient (terminal): The changed production owners are the descriptor-based async and sync directory-mode paths; the PR body records a POSIX base failure with the exact verification error, followed by successful real atomic writes and preserved 0o755 modes after the fix. The focused test command is reported as 21 passing tests.
Evidence reviewed 6 items Current-main regression path: Current main masks the descriptor's observed mode to 0o7777, but the shared owner compares the final observed mode against the caller value without masking; a raw stat mode therefore cannot verify equal.
Descriptor semantics: The pinned descriptor inspection intentionally returns only 0o7777 mode bits, establishing why stripping file-type bits before comparison is the narrow matching normalization.
Introduced fix and regression coverage: The branch masks at the shared async owner and synchronous descriptor chmod sites; its POSIX regression test passes raw stat.mode through both async and sync public atomic replacement APIs and verifies the resulting directory permissions.
Findings None None.
Security None None.

How this fits together

Atomic write helpers pin and verify a parent directory before changing its permissions and publishing a replacement file. A caller-provided directory mode flows through descriptor-based chmod and verification while confinement and directory identity checks remain in place.

flowchart LR
  A[Caller directory mode] --> B[Atomic write helper]
  B --> C[Pinned directory descriptor]
  C --> D[Mask permission bits]
  D --> E[Apply and verify mode]
  E --> F[Replacement file]
  C --> G[Directory identity check]
Loading

Before merge

None.

Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Focused implementation and coverage production +5/-1, tests +24, changelog +1 The small shared normalization is exercised through both public async and synchronous atomic-write paths.

Technical review

Best possible solution:

Land the shared boundary normalization and its async/sync regression coverage so raw directory stat modes retain their documented chmod-equivalent behavior without changing identity or confinement safeguards.

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

Yes—source-reproducible with high confidence: current main compares a permission-only inspected mode to an unmasked caller value, and the supplied POSIX regression run reports the exact base failure and fixed result.

Is this the best way to solve the issue?

Yes. Normalizing at descriptor application and verification preserves all 0o7777 permission and special bits while discarding only raw stat file-type bits, matching chmod semantics without weakening filesystem checks.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 5872f54f62a7.

Labels

Label changes:

  • add P2: This fixes a released public API compatibility regression with a bounded blast radius and a focused repair.
  • add proof: sufficient: Contributor real behavior proof is sufficient. The changed production owners are the descriptor-based async and sync directory-mode paths; the PR body records a POSIX base failure with the exact verification error, followed by successful real atomic writes and preserved 0o755 modes after the fix. The focused test command is reported as 21 passing tests.
  • add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🦞 diamond lobster.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The changed production owners are the descriptor-based async and sync directory-mode paths; the PR body records a POSIX base failure with the exact verification error, followed by successful real atomic writes and preserved 0o755 modes after the fix. The focused test command is reported as 21 passing tests.

Label justifications:

  • P2: This fixes a released public API compatibility regression with a bounded blast radius and a focused repair.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🦞 diamond lobster.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The changed production owners are the descriptor-based async and sync directory-mode paths; the PR body records a POSIX base failure with the exact verification error, followed by successful real atomic writes and preserved 0o755 modes after the fix. The focused test command is reported as 21 passing tests.
  • proof: sufficient: Contributor real behavior proof is sufficient. The changed production owners are the descriptor-based async and sync directory-mode paths; the PR body records a POSIX base failure with the exact verification error, followed by successful real atomic writes and preserved 0o755 modes after the fix. The focused test command is reported as 21 passing tests.

Evidence

What I checked:

  • Current-main regression path: Current main masks the descriptor's observed mode to 0o7777, but the shared owner compares the final observed mode against the caller value without masking; a raw stat mode therefore cannot verify equal. (src/directory-mode-owner.ts:78, 5872f54f62a7)
  • Descriptor semantics: The pinned descriptor inspection intentionally returns only 0o7777 mode bits, establishing why stripping file-type bits before comparison is the narrow matching normalization. (src/replace-file-descriptor.ts:98, 5872f54f62a7)
  • Introduced fix and regression coverage: The branch masks at the shared async owner and synchronous descriptor chmod sites; its POSIX regression test passes raw stat.mode through both async and sync public atomic replacement APIs and verifies the resulting directory permissions. (test/atomic-dirmode-regression.test.ts:146, 7d2923979941)
  • Contributor real-behavior proof: The PR body records the focused POSIX regression failing on the base with directory final mode could not be verified, then passing after the patch through the real async and synchronous atomic-write entry points; it also reports 21 focused tests passing. (test/atomic-dirmode-regression.test.ts:146, 7d2923979941)
  • Feature history: Blame ties the existing unmasked final-mode comparison to the merged directory-mode publication work, identifying the current area owner and the source of the regression mechanism. (src/directory-mode-owner.ts:78, a48403ab44c7)
  • Release context: The current main commit containing the unmasked comparison is included in v0.8.1, so the proposed correction is not already present in the current release. (CHANGELOG.md:3, 5872f54f62a7)

Likely related people:

  • Peter Steinberger: Raw commit a48403a adds src/directory-mode-owner.ts:75 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: high; commits: a48403ab44c7; files: src/directory-mode-owner.ts)

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.

@steipete
steipete merged commit 39c9807 into main Sep 4, 2026
28 checks passed
@steipete
steipete deleted the steipete/serene-roentgen-60767c branch September 4, 2026 22:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P2 Normal priority bug or improvement with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant