Skip to content

fix(atomic): tolerate FAT-family rename identity drift on exFAT/FAT32 - #231

Closed
dongsheng123132 wants to merge 1 commit into
openclaw:mainfrom
dongsheng123132:fix/exfat-long-tmp-rename-identity
Closed

fix(atomic): tolerate FAT-family rename identity drift on exFAT/FAT32#231
dongsheng123132 wants to merge 1 commit into
openclaw:mainfrom
dongsheng123132:fix/exfat-long-tmp-rename-identity

Conversation

@dongsheng123132

Copy link
Copy Markdown

Summary

Windows mints a fresh file identity (new ino) on rename when names exceed the 8.3 short-name budget. Observed on real hardware: destination basenames >=16 chars get a fresh ino after rename on exFAT/FAT32 USB sticks, <=15 chars keep it. replaceFileAtomic then fails with path-mismatch even though the bytes landed correctly -- every atomic write to a USB stick with a normal-length filename fails (e.g. OpenClaw gateway config writes, portable USB apps).

Changes

  • Temp/staging basenames stay within 8.3 (<=12 chars): buildReplaceTempPath, sibling-temp, output-sibling, json temp names, all now xxxxxxxx.tmp.
  • Post-rename verification re-anchors to the published file's live identity after structural checks (symlink/hardlink/not-file rejection, fresh descriptor-vs-pathname agreement) PLUS a staged-bytes equality gate. Swapped bytes still fail closed with path-mismatch; never deletes or rolls back the published file.
  • mkdir -p skipped when the directory exists (FAT USB roots reject mkdir-with-mode with EPERM even for existing dirs).
  • Hostile tempPrefix (path separators) still throws instead of silently collapsing.

Test plan

  • pnpm build clean
  • pnpm test: 185 files passed, 4307 tests passed (38 files / 3187 tests skipped, same as main)
  • Real-hardware repro: exFAT + FAT32 + NTFS USB sticks, 5/5 pass (async/sync replace, dest basenames 12-18 chars 7/7 pass)
  • New regression tests: content-identical copy+unlink publication resolves; swapped-bytes-after-rename still rejects (async + sync)
  • pnpm check file-size budget: 3 files exceed 500 lines (temp-owner 538, replace-file 512, callback-substitution 526) -- left for maintainer to decide on budget vs split; all other check steps pass

Repro (minimal, Windows + exFAT USB stick)

replaceFileAtomic({ filePath: 'E:/state/xxxxxxxxxxxxxxxx.json', content: 'new' })
before: rejects path-mismatch (bytes correct on disk). after: resolves rename.

Environment: @openclaw/fs-safe 0.8.1, openclaw 2026.9.1, Node v22.22.3, Windows 11, exFAT + FAT32 USB sticks.

Windows mints a fresh file identity on rename when names exceed the
8.3 short-name budget (dest basenames >=16 chars observed), so
replaceFileAtomic failed with path-mismatch on exFAT/FAT32 USB sticks
even though the bytes landed correctly.

- temp/staging basenames stay within 8.3 (<=12 chars)
- post-rename verification re-anchors to the published file's live
  identity after structural checks plus a staged-bytes equality gate
  (swapped bytes still fail closed, never rollback)
- mkdir -p skipped when the directory exists (FAT USB roots reject
  mkdir-with-mode with EPERM)
- hostile tempPrefix (path separators) still throws
@dongsheng123132
dongsheng123132 requested a review from a team as a code owner September 5, 2026 07:12
@clawsweeper

clawsweeper Bot commented Sep 5, 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. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Sep 5, 2026
@clawsweeper

clawsweeper Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed September 5, 2026, 3:17 AM ET / 07:17 UTC.

ClawSweeper review

What this changes

The PR shortens staging filenames, accepts matching-content files after rename identity changes, and adjusts directory creation to accommodate FAT-family filesystems.

Regression provenance

Possible regression — suspected (reviewed change). No predecessor PR is attributed.

Merge readiness

Blocked before merge - 15 items remain

The reported hardware failure merits follow-up, but this branch introduces blocking ownership and temporary-file regressions. Current main provides an explicit compatibility policy; the complete FAT scenario is not demonstrably resolved there.

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

Review scores

Measure Result What it means
Overall readiness 🧂 unranked krab (1/6) The hardware report is useful, but ownership regressions and deterministic staging collisions make the current patch unsafe to land.
Proof confidence 🦐 gold shrimp (3/6) Needs stronger real behavior proof before merge: Authority-chain proof required: the captured body reports real Windows FAT/NTFS async/sync successes, but does not show strict rejection of a same-byte substituted inode or preservation of pre-existing staging files and symlink referents before callback I/O. The hardware summary is useful positive evidence, but does not cover these changed authority boundaries. 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 🧂 unranked krab (1/6) Security review found an item that needs attention.

Verification

Check Result Evidence
Real behavior Needs proof Needs stronger real behavior proof before merge: Authority-chain proof required: the captured body reports real Windows FAT/NTFS async/sync successes, but does not show strict rejection of a same-byte substituted inode or preservation of pre-existing staging files and symlink referents before callback I/O. The hardware summary is useful positive evidence, but does not cover these changed authority boundaries. 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 10 items Repository safety policy: The full root AGENTS.md was read. It requires preserving post-operation identity checks, explicit bounded fallbacks, shared guarded helpers, and public error/default contracts. No nested AGENTS.md or maintainer notes apply.
Verified introduced changes: The complete local diff covers 11 files. Raw commit records confirm the head directly parents the pinned main commit; the verified test merge has main then head as parents and the same tree as the head.
Existing compatibility contract: Current main documents strict identity as the default and reserves matching-content adoption for verify-content-with-lock. It explicitly warns that identical bytes cannot distinguish a same-authority substitute.
Findings 4 actionable findings [P1] Preserve strict identity rejection after publication
[P1] Retain randomness when callers supply a short prefix
[P2] Preserve shared prefix validation and typed errors
Security Needs attention Strict publication accepts a substituted inode: Matching bytes authorize adoption without the existing explicit policy or cooperative lock, allowing a same-content replacement with different ownership or permissions to satisfy strict publication checks.
Predictable staging is consumed before admission: A short prefix selects a fixed pathname passed directly to the producer; a pre-existing file can be overwritten, or a planted symlink followed by the producer before the library's later checks reject it.

How this fits together

fs-safe stages application data in temporary files, then publishes it by rename. Identity checks determine whether the published file is still the one the operation created.

flowchart TD
  A[Application data and destination] --> B[Choose temporary filename]
  B --> C[Write staged file]
  C --> D[Rename to destination]
  D --> E{Published identity matches?}
  E -->|Yes| G[Report success]
  E -->|No| F[Compare content and adopt identity]
  F -->|Matching bytes| G
  F -->|Different bytes| H[Report mismatch]
Loading

Decision needed

Question Recommendation
Should FAT compatibility be confined to the existing explicit locked policy, or should transparent support wait for an identity-preserving implementation? Keep compatibility explicit: Narrow this work to FAT support under verify-content-with-lock while preserving strict rejection and repairing temporary-name handling.

Why: The proposed automatic adoption changes a deliberately documented security contract; successful hardware writes alone cannot authorize that policy change.

Before merge

  • Add real behavior proof - Needs stronger real behavior proof before merge: Authority-chain proof required: the captured body reports real Windows FAT/NTFS async/sync successes, but does not show strict rejection of a same-byte substituted inode or preservation of pre-existing staging files and symlink referents before callback I/O. The hardware summary is useful positive evidence, but does not cover these changed authority boundaries. 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.
  • Preserve strict identity rejection after publication (P1) - With renameIdentity omitted or explicitly strict, expectedHash is undefined, so this branch accepts a different inode on every platform. A writer can replace the published file with another single-link regular file containing identical bytes, potentially with different permissions, and the operation now succeeds. docs/atomic.md reserves this weaker contract for verify-content-with-lock. Preserve strict rejection and reuse the existing explicit compatibility path in both async and sync implementations.
  • Retain randomness when callers supply a short prefix (P1) - tempPrefix: "save" now always selects save.tmp. writeCallbackSibling calls the producer before inspecting that path, so a normal fs.writeFile callback can truncate an existing save.tmp and then move it to the destination. Concurrent callbacks can also share staged bytes. The matching builder in replace-file.ts causes exclusive-create collisions, including concurrent queue entries using tempPrefix: "queue". Keep every operation's staging name unique, including with custom prefixes.
  • Preserve shared prefix validation and typed errors (P2) - Replacing assertSafePathPrefix changes observable input handling: NUL-containing, empty, and dot-only prefixes now reach staging instead of failing, while separator-containing prefixes throw plain Error instead of FsSafeError("invalid-path"). These are public validation/error contracts. Validate through the existing shared helper before constructing the shorter name, and apply the same repair to the sibling builders.
  • Restore compliance with the enforced file-size check (P2) - The additions take this file, src/replace-file.ts, and test/atomic-callback-substitution.test.ts beyond the enforced 500-line budget. None has an exception, so pnpm check stops before its build and tests; the body acknowledges this failure. Remove the duplicated verification implementation or split focused helpers/tests so the required check can complete.
  • Resolve security concern: Strict publication accepts a substituted inode - Matching bytes authorize adoption without the existing explicit policy or cooperative lock, allowing a same-content replacement with different ownership or permissions to satisfy strict publication checks.
  • Resolve security concern: Predictable staging is consumed before admission - A short prefix selects a fixed pathname passed directly to the producer; a pre-existing file can be overwritten, or a planted symlink followed by the producer before the library's later checks reject it.
  • Resolve merge risk (P1) - Existing strict-policy callers would silently receive weaker identity guarantees after upgrading.
  • Resolve merge risk (P1) - Existing short-prefix workflows can encounter staging collisions; callback producers can overwrite an unrelated existing file before admission checks.
  • Resolve merge risk (P1) - The hardware summary does not establish safety under identical-content substitution, concurrent writers, or pre-existing staging paths.
  • Complete next step (P2) - Resolve the compatibility-policy decision, repair the findings, and attach after-fix safety evidence. Terminal screenshots or recordings are preferred when they show the behavior; copied output and logs also count. Redact private paths, hosts, credentials, and other sensitive details. Updating the PR body should trigger review; otherwise ask a maintainer to comment @clawsweeper re-review.
  • Improve patch quality - Preserve strict identity semantics and consolidate compatibility verification under the existing explicit policy.
  • Improve patch quality - Restore unique, validated staging names and bring the changed files within their enforced budgets.
  • Improve patch quality - Provide after-fix hardware output and final-effect evidence for identical-content substitution and pre-existing staging paths.
  • Resolve maintainer decision - Resolve the maintainer decision shown above before merge.

Findings

  • [P1] Preserve strict identity rejection after publication — src/replace-file-temp-owner.ts:210-212
  • [P1] Retain randomness when callers supply a short prefix — src/sibling-temp.ts:42-44
  • [P2] Preserve shared prefix validation and typed errors — src/replace-file.ts:239-243
  • [high] Strict publication accepts a substituted inode — src/replace-file-temp-owner.ts:210
  • [high] Predictable staging is consumed before admission — src/sibling-temp.ts:43
Agent review details

Security

Needs attention: The patch weakens filesystem ownership guarantees through unconditional identity adoption and predictable callback staging; no supply-chain changes were introduced.

Review metrics

Metric Value Why it matters
Production and test growth Production +144 net lines; tests +52 net lines FAT compatibility motivates the growth, but the new verifier duplicates an existing opt-in implementation.
File-size budget 3 changed files exceed 500 lines These files deterministically fail the first stage of the required pnpm check.

Merge-risk options

Maintainer options:

  1. Preserve the established contract (recommended)
    Restore strict identity rejection, reuse locked compatibility verification, and repair temporary-name uniqueness and validation before landing.
  2. Pause automatic identity adoption
    Hold the PR while maintainers establish an acceptable FAT ownership model and the contributor supplies matching safety evidence.

Technical review

Best possible solution:

Preserve strict defaults and implement FAT compatibility through the existing locked policy, with validated, collision-safe temporary names and demonstrated upgrade safety.

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

Yes for the introduced defects: source establishes deterministic short-prefix collisions and acceptance of identical-content replacement files in strict mode. The reported physical FAT failure was not independently executed.

Is this the best way to solve the issue?

No. Main and v0.8.1 already provide explicit locked content verification; FAT-specific adjustments should extend that path without replacing strict semantics or duplicating its verifier.

Full review comments:

  • [P1] Preserve strict identity rejection after publication — src/replace-file-temp-owner.ts:210-212
    With renameIdentity omitted or explicitly strict, expectedHash is undefined, so this branch accepts a different inode on every platform. A writer can replace the published file with another single-link regular file containing identical bytes, potentially with different permissions, and the operation now succeeds. docs/atomic.md reserves this weaker contract for verify-content-with-lock. Preserve strict rejection and reuse the existing explicit compatibility path in both async and sync implementations.
    Confidence: 0.99
  • [P1] Retain randomness when callers supply a short prefix — src/sibling-temp.ts:42-44
    tempPrefix: "save" now always selects save.tmp. writeCallbackSibling calls the producer before inspecting that path, so a normal fs.writeFile callback can truncate an existing save.tmp and then move it to the destination. Concurrent callbacks can also share staged bytes. The matching builder in replace-file.ts causes exclusive-create collisions, including concurrent queue entries using tempPrefix: "queue". Keep every operation's staging name unique, including with custom prefixes.
    Confidence: 0.99
  • [P2] Preserve shared prefix validation and typed errors — src/replace-file.ts:239-243
    Replacing assertSafePathPrefix changes observable input handling: NUL-containing, empty, and dot-only prefixes now reach staging instead of failing, while separator-containing prefixes throw plain Error instead of FsSafeError("invalid-path"). These are public validation/error contracts. Validate through the existing shared helper before constructing the shorter name, and apply the same repair to the sibling builders.
    Confidence: 0.98
  • [P2] Restore compliance with the enforced file-size check — src/replace-file-temp-owner.ts:264-268
    The additions take this file, src/replace-file.ts, and test/atomic-callback-substitution.test.ts beyond the enforced 500-line budget. None has an exception, so pnpm check stops before its build and tests; the body acknowledges this failure. Remove the duplicated verification implementation or split focused helpers/tests so the required check can complete.
    Confidence: 1

Overall correctness: patch is incorrect
Overall confidence: 0.98

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 899f91b97d26.

Labels

Label changes:

  • add P2: This is a bounded filesystem compatibility improvement with substantive pre-merge defects, rather than an established emergency in current main.
  • add merge-risk: 🚨 compatibility: The patch changes strict-mode guarantees, temporary-prefix behavior, and invalid-path error handling for existing callers.
  • add merge-risk: 🚨 security-boundary: Automatic adoption of another inode weakens strict ownership verification, and deterministic callback paths can redirect producer writes.
  • add rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🦐 gold shrimp and patch quality is 🧂 unranked krab.
  • add status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: Authority-chain proof required: the captured body reports real Windows FAT/NTFS async/sync successes, but does not show strict rejection of a same-byte substituted inode or preservation of pre-existing staging files and symlink referents before callback I/O. The hardware summary is useful positive evidence, but does not cover these changed authority boundaries. 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.

Label justifications:

  • P2: This is a bounded filesystem compatibility improvement with substantive pre-merge defects, rather than an established emergency in current main.
  • merge-risk: 🚨 security-boundary: Automatic adoption of another inode weakens strict ownership verification, and deterministic callback paths can redirect producer writes.
  • merge-risk: 🚨 compatibility: The patch changes strict-mode guarantees, temporary-prefix behavior, and invalid-path error handling for existing callers.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🦐 gold shrimp and patch quality is 🧂 unranked krab.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: Authority-chain proof required: the captured body reports real Windows FAT/NTFS async/sync successes, but does not show strict rejection of a same-byte substituted inode or preservation of pre-existing staging files and symlink referents before callback I/O. The hardware summary is useful positive evidence, but does not cover these changed authority boundaries. 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

Security concerns:

  • [high] Strict publication accepts a substituted inode — src/replace-file-temp-owner.ts:210
    Matching bytes authorize adoption without the existing explicit policy or cooperative lock, allowing a same-content replacement with different ownership or permissions to satisfy strict publication checks.
    Confidence: 0.99
  • [high] Predictable staging is consumed before admission — src/sibling-temp.ts:43
    A short prefix selects a fixed pathname passed directly to the producer; a pre-existing file can be overwritten, or a planted symlink followed by the producer before the library's later checks reject it.
    Confidence: 0.98

What I checked:

  • Repository safety policy: The full root AGENTS.md was read. It requires preserving post-operation identity checks, explicit bounded fallbacks, shared guarded helpers, and public error/default contracts. No nested AGENTS.md or maintainer notes apply. (AGENTS.md:25, 5751f12a972e)
  • Verified introduced changes: The complete local diff covers 11 files. Raw commit records confirm the head directly parents the pinned main commit; the verified test merge has main then head as parents and the same tree as the head. (5751f12a972e)
  • Existing compatibility contract: Current main documents strict identity as the default and reserves matching-content adoption for verify-content-with-lock. It explicitly warns that identical bytes cannot distinguish a same-authority substitute. (docs/atomic.md:80, 899f91b97d26)
  • Released behavior: The supplied latest release, v0.8.1, contains the strict rejection guard and the explicit locked compatibility implementation. This does not prove the reported FAT directory-creation failure is fixed. (src/replace-file-temp-owner.ts:203, 5872f54f62a7)
  • Strict policy bypass: The added branch adopts a different inode whenever expectedHash is absent, including explicit strict mode on non-FAT filesystems. The new verifier checks bytes but does not establish that the replacement retains the staged ownership or permissions. (src/replace-file-temp-owner.ts:210, 5751f12a972e)
  • Deterministic staging reaches real writers: Short prefixes become fixed filenames. writeCallbackSibling invokes the producer before admitting the resulting file, so an existing save.tmp can be overwritten and published. The matching atomic builder also makes concurrent durable-queue entries with prefix queue contend for queue.tmp. (src/sibling-temp.ts:43, 5751f12a972e)

Likely related people:

  • Peter Steinberger: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

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 commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Closing this implementation after independent review with real filesystem probes.

The exFAT/FAT32 rename identity drift you report is a real problem, and thank you for the hardware evidence. The proposed fix, however, changes the security contract for every caller: with the branch's changed modules, explicit strict mode accepted a substituted inode containing identical bytes with mode 0777 (main rejects it with path-mismatch), and tempPrefix "save" consumed an unrelated existing save.tmp (main preserves it). The branch also replaces the shared prefix validation and exceeds the enforced file-size limits.

Main already offers an explicit opt-in for this situation via renameIdentity: "verify-content-with-lock", with documented limitations. Transparent acceptance would weaken existing callers without their opting in, so we will not land it in this form. Your report remains useful evidence for future work under that explicit compatibility policy.

@steipete steipete closed this Sep 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. P2 Normal priority bug or improvement with limited blast radius. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants