fix(root): report documented remove() failure codes - #84
Conversation
Root.remove() routed every failure through normalizePinnedPathError, which rewrites any non-FsSafeError into path-alias "path is not under root". A missing target and a non-empty directory were both reported as boundary violations, and the documented not-empty and not-removable codes were never constructed anywhere in the package. Map the syscall failures at the remove call site instead: ENOENT/ENOTDIR to not-found, ENOTEMPTY/EEXIST to not-empty, and any other errno to not-removable. Directory guard failures are already FsSafeError instances, so path-mismatch and not-file pass through untouched.
clawsweeper
Bot
added
proof: sufficient
|
Codex review: needs maintainer review before merge. Reviewed August 2, 2026, 2:20 PM ET / 18:20 UTC. ClawSweeper reviewWhat this changesThe branch maps Merge readinessThis PR is a focused correction to the documented Priority: P2 Review scores
Verification
How this fits together
flowchart TD
A[Caller requests removal] --> B[Resolve target under root]
B --> C[Pin parent directory identity]
C --> D{Parent guard succeeds?}
D -->|No| E[Fail-closed boundary error]
D -->|Yes| F[Inspect and remove target]
F --> G[Map target syscall failure]
G --> H[Public remove result]
Decision needed
Why: The patch is mechanically narrow and preserves the guard fail-closed boundary, but selecting which externally observable error contract takes precedence is a maintainer compatibility decision. Before merge
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Adopt the documented removal-error contract with the guard/deletion separation in this branch, and call out the legacy Do we have a high-confidence way to reproduce the issue? Yes. Current main routes raw fallback ENOENT and ENOTEMPTY errors through Is this the best way to solve the issue? Yes, subject to the compatibility decision. Separating parent-directory guard errors from target deletion syscall errors is the narrowest maintainable repair and retains fail-closed behavior for ambiguous guard failures. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 2477f5681f68. LabelsLabel changes:
Label justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (8 earlier review cycles)
|
|
The two Windows What failed here: That exact test also fails on
A different test fails almost every run, across both Windows and ubuntu, and all of them are concurrency or file-handle timing cases. On my Windows machine This change cannot reach that code: I cannot re-run the job myself — that needs repository admin rights — so if you would like a clean run before reviewing, a maintainer re-run or a rebase once If the flakes are worth chasing separately, I am glad to open an issue with the run links collected above rather than expanding this PR. |
clawsweeper
Bot
added
rating: 🦞 diamond lobster
removePathFallback() creates and asserts the parent-directory guard before it touches the target, so wrapping the whole call in the remove normalizer turned a raw guard error such as ELOOP into not-removable even though no deletion was attempted. Scope the errno mapping to the deletion syscalls and give the guard stage its own normalizer: FsSafeError passes through, ENOENT/ENOTDIR still report not-found because the target is definitionally absent and nothing mutated, and every other raw error keeps the fail-closed path-alias contract. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ah1kwRo32A6sGU1tbMFEFk
|
Pushed What was wrong
What changedThe guard stage moved into
The EvidenceThe new end-to-end regression throws an errno-shaped That is the finding reproduced: a guard-originated raw error surfacing as Coverage added to
Validation on Windows 11, Node v22.20.0: The pre-existing Windows lock flakes noted earlier did not reproduce in this run. |
clawsweeper
Bot
added
rating: 🐚 platinum hermit
|
The red Windows leg here isn't from this diff — it's the flake family that's been on I closed/reopened this PR to retrigger CI (I can't
Each time the other Windows Node version passed. A real defect fails the same leg every run; this one rotates. It's also outside this PR's call graph. The diff touches For context on the first failure specifically — I did chase it before the re-run, in case it was a genuine identity bug. It isn't: with The Happy to rebase once |
|
Correction to my note above: I called the lock failures timing/environment. I chased them properly afterwards and they are a real, fixable defect — #87 has the root cause and the fix. Windows denies access to a lock file while a just-unlinked directory entry is still being torn down, so a contended
Two things I had wrong:
Measured on Windows 11 / Node 22 over That covers the This PR's own diff is unrelated to any of it, so #87 is independent of #84. |
removePathInRoot() routed every non-FsSafeError through normalizePinnedPathError(), so an ordinary ENOENT or ENOTEMPTY surfaced as the boundary-violation code path-alias. The documented codes not-found, not-empty and not-removable were declared in the exported union and documented in six places but constructed nowhere. The guard stage is separated from the errno mapping so a raw guard failure such as ELOOP is not reported as a removal outcome, and the three codes are now classified as operational rather than policy, so FsSafeError.category stops describing routine filesystem outcomes as safety-policy rejections. Supersedes #84. Co-authored-by: Yigtwxx <yigiterdogan023@gmail.com>
|
Landed as 6af7f93 via #93 — thank you, and your commits are credited on the squash. Your diagnosis and your second push were both right. I added one thing before landing. Worth knowing for the downstream bump: this breaks The red Windows legs on this PR were never yours — that was the sidecar-lock teardown race, fixed separately in #92. |
2 participants
What Problem This Solves
Fixes an issue where consumers calling
Root.remove()would receive the boundary-violation codepath-alias("path is not under root") when the target was simply missing or the directory was simply not empty.The affected surface is root confinement error reporting.
removePathInRoot()routes every failure throughnormalizePinnedPathError(), which rewrites any non-FsSafeErrorintopath-alias. BecauseremovePathFallback()reaches the filesystem through plainfs.lstat/fs.rmdir/fs.rm, anENOENTand anENOTEMPTYboth surface as boundary violations.The documentation already specifies the intended behavior in six places:
docs/writing.md:130,135anddocs/writing.md:193,194— non-empty directories thrownot-empty,not-removablecovers other unlink/rmdir failuresdocs/errors.md:96,100— same two codes, withnot-foundatdocs/errors.md:98docs/file-store.md:135— "non-empty dirs thrownot-empty"docs/root.md:156—not-foundfor a missing targetnot-emptyandnot-removableare declared in the exportedFsSafeErrorCodeunion (src/errors.ts:10,14) and listed indocs/types.md:156-157, but before this change they were constructed nowhere insrc/and asserted nowhere intest/. The documentation was already correct; the implementation was not.Why This Change Was Made
A new
normalizeRemovePathError()insrc/root-errors.tsclassifies the failure at theremove()call site:ENOENT/ENOTDIRtonot-found,ENOTEMPTY/EEXISTtonot-empty, and any other errno tonot-removable. Anything that is not errno-shaped still falls back tonormalizePinnedPathError(), so today'spath-aliasbehavior is preserved for genuinely unclassifiable throws.Three boundaries were deliberate:
createAsyncDirectoryGuard()andassertAsyncDirectoryGuard()already throwFsSafeError(not-fileandpath-mismatch), so the leadinginstanceof FsSafeErrorcheck returns them unchanged. TOCTOU identity drift still reportspath-mismatch— that is covered by a unit assertion.normalizePinnedPathError()itself is unchanged.mkdirPathInRoot()shares it and is out of scope for this PR, andtest/edge-coverage.test.tspins its current contract.EEXISTmaps tonot-emptybecause POSIX permitsrmdirto reportEEXISTfor a non-empty directory. The package already treats the pair as equivalent insrc/move-path.ts:346andsrc/trash.ts:12.Compatibility: error codes are a public compatibility surface, and this is a deliberate behavior change. A consumer branching on
path-aliasaroundremove()will observe the new codes. Consumers written against the documented contract are fixed by this change; consumers written against the previous behavior were treating a routineENOTEMPTYas a containment failure, which is the more dangerous of the two readings. No internal caller depends on the old code —src/file-store-prune.ts:89,95is the only in-package consumer and it discards remove errors.Non-goal:
mkdirPathInRoot()shares the same normalizer and has its own error-shape questions. Left alone to keep this to one concern; happy to open a follow-up.User Impact
Root.remove()andFileStore.remove()now report the codes their documentation already promised:Directory identity drift during the remove still reports
path-mismatch, unchanged. No API, option, default, or export changed; no migration is needed beyond widening apath-aliascatch if one was written against the previous behavior.Evidence
Reproduction on
mainbefore the fix, using the new regression test:Regression coverage added:
test/fs-safe.test.ts— real-diskmkdtemproot exercisingnot-foundfor a missing target,not-foundfor a missing parent, andnot-emptyfor a populated directory. Not platform-skipped:removePathInRoot()has no platform branch, so it runs on all six CI matrix legs.test/edge-coverage.test.ts— unit assertions over the six errno mappings, theFsSafeErrorpass-through forpath-mismatch, and the non-errno fallback topath-alias.not-removableis covered at the unit level rather than end to end on purpose: forcingEACCESneeds achmodthat is a no-op for root in container CI and is ignored on Windows, and forcingEBUSYonly works on Windows. A platform-skipped end-to-end case would assert less than the unit test does on every platform.Validation on Windows 11, Node v22.20.0, pnpm 10.34.5:
CHANGELOG.mdupdated when release-relevantUpdate —
e96eb9cThe errno mapping wrapped the whole fallback, and the parent-directory guard runs inside it, so a raw guard error such as
ELOOPbecamenot-removablealthough no deletion was attempted.The guard stage now sits in
prepareRemoveGuard()with its own normalizer, and the errno mapping wraps only thelstat/rmdir/rmcalls on the target.removePathInRoot()is back tonormalizePinnedPathError(), unchanged frommain.normalizeRemoveGuardError()keepsFsSafeErrorunchanged, mapsENOENT/ENOTDIRtonot-found— the parent does not exist, so the target is definitionally absent and nothing mutated — and fails closed withpath-aliasfor every other raw error.Pre-fix, on head
44cc206with the new regression:Coverage added: an end-to-end case driving an errno-shaped guard failure through the existing
beforeRootFallbackMutationhook, so it is deterministic on all six CI legs, plus unit assertions over every branch of the new normalizer.Validation on Windows 11, Node v22.20.0: