diff --git a/CHANGELOG.md b/CHANGELOG.md index a308d72..94dc79a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,11 +7,13 @@ - Retry a contended file-lock acquisition when Windows denies access to a lock file whose directory entry is still being torn down, including when the native binding performs the exclusive create. Both the exclusive create and the holder's snapshot read reported that transient `EPERM` as a hard failure, so concurrent `acquireFileLock()` calls failed intermittently on Windows even though the very next attempt would have succeeded. Retries stay bounded, so a genuine permission denial still surfaces as `EPERM` rather than a lock timeout; thanks @Yigtwxx for the fix. - Apply `replaceFileAtomic()` and `replaceFileAtomicSync()` modes through pinned temp-file descriptors before rename, and through pinned copy-fallback descriptors, so a post-rename symlink swap cannot redirect `chmod` to an unrelated file while exact modes remain independent of umask; thanks @yetval for reporting this (#86). - Bound fallback Windows owner and ACL command execution to 10 seconds per process and report owner-query failures as unverified instead of returning a partial permission classification; thanks @Yigtwxx for the diagnosis. +- Report the documented `remove()` failure codes instead of collapsing every failure to `path-alias`. A missing target now throws `not-found`, a non-empty directory throws `not-empty`, and any other filesystem failure throws `not-removable`, while directory identity drift keeps reporting `path-mismatch`; thanks @Yigtwxx for the fix. ### Compatibility - Report access-denied failures from native Windows operations as `EPERM` rather than `EACCES`, matching Node/libuv and the JavaScript fallback. Consumers that matched the previous native-only `EACCES` code should accept `EPERM` as well when supporting older package versions. - Add an optional `fchmodSync` operation to the injectable synchronous atomic-replacement filesystem type. Async adapters need no new member because their required `open()` returns a mode-capable `FileHandle`; custom sync adapters that pass `mode` or `preserveExistingMode` now fail before mutation when `fchmodSync` is absent, while adapters that request neither option remain compatible. +- Classify `not-found`, `not-empty` and `not-removable` as operational rather than policy errors, so `FsSafeError.category` no longer reports routine filesystem outcomes as safety-policy rejections. ### Features diff --git a/README.md b/README.md index d7b9700..c497aa3 100644 --- a/README.md +++ b/README.md @@ -506,13 +506,17 @@ Codes are grouped by category: ```ts if (err instanceof FsSafeError) { if (err.category === "policy") { - // Unsafe caller input or filesystem state. + // Unsafe caller input or filesystem state rejected by a safety policy. } else { - // Operational problem such as helper startup, timeout, or unverifiable permissions. + // Routine filesystem outcome or runtime/environment problem. } } ``` +Routine filesystem outcomes such as `not-found`, `not-empty`, and +`not-removable` are operational; they do not indicate that a filesystem +boundary policy was violated. + Current `FsSafeErrorCode` values are `already-exists`, `denied-path`, `device-path`, `hardlink`, `helper-failed`, `helper-unavailable`, `invalid-path`, `insecure-permissions`, `not-empty`, `not-file`, `not-found`, `not-owned`, `not-removable`, `outside-workspace`, `path-alias`, `path-mismatch`, `permission-unverified`, `secret-exists`, `symlink`, `timeout`, `too-large`, and `unsupported-platform`. ## Safety model diff --git a/docs/errors.md b/docs/errors.md index d993a20..a21c47d 100644 --- a/docs/errors.md +++ b/docs/errors.md @@ -50,8 +50,12 @@ destination. `category` separates caller-policy failures from operational failures: -- `"policy"` — unsafe input or target state, such as `outside-workspace`, `symlink`, `hardlink`, or `too-large`. -- `"operational"` — environment/runtime failures, such as helper startup, platform support, timeout, or unverifiable permissions. +- `"policy"` — unsafe input or target state rejected by a safety policy, such as `outside-workspace`, `symlink`, `hardlink`, or `too-large`. +- `"operational"` — routine filesystem outcomes or environment/runtime failures, such as `not-found`, `not-empty`, `not-removable`, helper startup, platform support, timeout, or unverifiable permissions. + +Routine absence or inability to remove a path does not by itself indicate a +filesystem boundary violation. Branch on the specific code when the distinction +between those operational outcomes matters. ## Code union diff --git a/docs/types.md b/docs/types.md index 5297c88..0e86b88 100644 --- a/docs/types.md +++ b/docs/types.md @@ -163,7 +163,7 @@ type FsSafeErrorCode = Closed union you switch on. See the [Errors](errors.md) reference for what each one means. -`FsSafeError.category` is `"policy"` for unsafe input/target-state failures and `"operational"` for environment/runtime failures. +`FsSafeError.category` is `"policy"` for unsafe input or target state rejected by a safety policy and `"operational"` for routine filesystem outcomes or environment/runtime failures. `not-found`, `not-empty`, and `not-removable` are operational. ## See also diff --git a/src/errors.ts b/src/errors.ts index b5f5f22..ded7b94 100644 --- a/src/errors.ts +++ b/src/errors.ts @@ -29,6 +29,9 @@ export type FsSafeErrorDetails = Readonly>; const OPERATIONAL_CODES: ReadonlySet = new Set([ "helper-failed", "helper-unavailable", + "not-empty", + "not-found", + "not-removable", "permission-unverified", "timeout", "unsupported-platform", diff --git a/src/root-errors.ts b/src/root-errors.ts index 355efb6..e352a13 100644 --- a/src/root-errors.ts +++ b/src/root-errors.ts @@ -1,5 +1,7 @@ import { FsSafeError } from "./errors.js"; -import { hasNodeErrorCode } from "./path.js"; +import { hasNodeErrorCode, isNodeError, isNotFoundPathError } from "./path.js"; + +const REMOVE_NOT_EMPTY_CODES = new Set(["ENOTEMPTY", "EEXIST"]); export function isAlreadyExistsError(error: unknown): boolean { return hasNodeErrorCode(error, "EEXIST") || /File exists|EEXIST/i.test(String(error)); @@ -22,3 +24,32 @@ export function normalizePinnedPathError(error: unknown): Error { cause: error instanceof Error ? error : undefined, }); } + +export function normalizeRemoveGuardError(error: unknown): Error { + if (error instanceof FsSafeError) { + return error; + } + if (isNotFoundPathError(error)) { + return new FsSafeError("not-found", "file not found", { + cause: error instanceof Error ? error : undefined, + }); + } + return normalizePinnedPathError(error); +} + +export function normalizeRemovePathError(error: unknown): Error { + if (error instanceof FsSafeError) { + return error; + } + if (!isNodeError(error) || typeof error.code !== "string") { + return normalizePinnedPathError(error); + } + const cause = error instanceof Error ? error : undefined; + if (isNotFoundPathError(error)) { + return new FsSafeError("not-found", "file not found", { cause }); + } + if (REMOVE_NOT_EMPTY_CODES.has(error.code)) { + return new FsSafeError("not-empty", "directory is not empty", { cause }); + } + return new FsSafeError("not-removable", "path could not be removed", { cause }); +} diff --git a/src/root-impl.ts b/src/root-impl.ts index 87665fc..5aa2b97 100644 --- a/src/root-impl.ts +++ b/src/root-impl.ts @@ -47,6 +47,8 @@ import { isAlreadyExistsError, normalizePinnedPathError, normalizePinnedWriteError, + normalizeRemoveGuardError, + normalizeRemovePathError, } from "./root-errors.js"; import { getFsSafeTestHooks } from "./test-hooks.js"; import { stringifyJsonDocument } from "./json-stringify.js"; @@ -1324,11 +1326,24 @@ async function resolvePinnedRootPathInRoot( }; } +async function prepareRemoveGuard(targetPath: string) { + try { + const guard = await createAsyncDirectoryGuard(path.dirname(targetPath)); + await getFsSafeTestHooks()?.beforeRootFallbackMutation?.("remove", targetPath); + await assertAsyncDirectoryGuard(guard); + return guard; + } catch (error) { + throw normalizeRemoveGuardError(error); + } +} + async function removePathFallback(resolved: { resolved: string }): Promise { - const guard = await createAsyncDirectoryGuard(path.dirname(resolved.resolved)); - await getFsSafeTestHooks()?.beforeRootFallbackMutation?.("remove", resolved.resolved); - await assertAsyncDirectoryGuard(guard); - await ((await fs.lstat(resolved.resolved)).isDirectory() ? fs.rmdir(resolved.resolved) : fs.rm(resolved.resolved)); + const guard = await prepareRemoveGuard(resolved.resolved); + try { + await ((await fs.lstat(resolved.resolved)).isDirectory() ? fs.rmdir(resolved.resolved) : fs.rm(resolved.resolved)); + } catch (error) { + throw normalizeRemovePathError(error); + } await assertAsyncDirectoryGuard(guard).catch(() => undefined); } diff --git a/test/edge-coverage.test.ts b/test/edge-coverage.test.ts index a411563..a6c455b 100644 --- a/test/edge-coverage.test.ts +++ b/test/edge-coverage.test.ts @@ -16,11 +16,15 @@ import { safePathSegmentHashed, } from "../src/install-path.js"; import { replaceDirectoryAtomic } from "../src/replace-directory.js"; +import { root as openRoot } from "../src/root.js"; import { isAlreadyExistsError, normalizePinnedPathError, normalizePinnedWriteError, + normalizeRemoveGuardError, + normalizeRemovePathError, } from "../src/root-errors.js"; +import { __setFsSafeTestHooksForTest } from "../src/test-hooks.js"; import { movePathToTrash } from "../src/trash.js"; const tempDirs = new Set(); @@ -60,6 +64,60 @@ describe("root error helpers", () => { }); expect(normalizePinnedPathError("raw string")).toMatchObject({ code: "path-alias" }); }); + + it("maps remove syscall failures without swallowing boundary errors", () => { + const drift = new FsSafeError("path-mismatch", "directory changed during operation"); + expect(normalizeRemovePathError(drift)).toBe(drift); + + const mappings = [ + ["ENOENT", "not-found"], + ["ENOTDIR", "not-found"], + ["ENOTEMPTY", "not-empty"], + ["EEXIST", "not-empty"], + ["EACCES", "not-removable"], + ["EBUSY", "not-removable"], + ] as const; + for (const [errno, code] of mappings) { + const error = Object.assign(new Error(errno), { code: errno }); + expect(normalizeRemovePathError(error)).toMatchObject({ code }); + } + + expect(normalizeRemovePathError(new Error("raw"))).toMatchObject({ code: "path-alias" }); + expect(normalizeRemovePathError("raw string")).toMatchObject({ code: "path-alias" }); + }); + + it("keeps guard-stage failures fail-closed instead of reporting a removal outcome", () => { + const drift = new FsSafeError("path-mismatch", "directory changed during operation"); + expect(normalizeRemoveGuardError(drift)).toBe(drift); + + for (const errno of ["ENOENT", "ENOTDIR"]) { + const error = Object.assign(new Error(errno), { code: errno }); + expect(normalizeRemoveGuardError(error), errno).toMatchObject({ code: "not-found" }); + } + for (const errno of ["ELOOP", "EACCES", "EBUSY", "ENOTEMPTY", "EEXIST"]) { + const error = Object.assign(new Error(errno), { code: errno }); + expect(normalizeRemoveGuardError(error), errno).toMatchObject({ code: "path-alias" }); + } + expect(normalizeRemoveGuardError(new Error("raw"))).toMatchObject({ code: "path-alias" }); + }); + + it("does not map a guard-stage filesystem failure to a removal code", async () => { + const rootPath = await tempRoot("fs-safe-remove-guard-"); + const scoped = await openRoot(rootPath); + await scoped.write("target.txt", "keep"); + + __setFsSafeTestHooksForTest({ + beforeRootFallbackMutation() { + throw Object.assign(new Error("ELOOP"), { code: "ELOOP" }); + }, + }); + try { + await expect(scoped.remove("target.txt")).rejects.toMatchObject({ code: "path-alias" }); + } finally { + __setFsSafeTestHooksForTest(undefined); + } + await expect(fs.readFile(path.join(rootPath, "target.txt"), "utf8")).resolves.toBe("keep"); + }); }); describe("directory replacement and file store boundary helpers", () => { diff --git a/test/fs-safe.test.ts b/test/fs-safe.test.ts index ce39515..bc6e62f 100644 --- a/test/fs-safe.test.ts +++ b/test/fs-safe.test.ts @@ -3,7 +3,12 @@ import { chmod, mkdtemp, readdir, readFile, rename, rm, stat, symlink, writeFile import os from "node:os"; import path from "node:path"; import { afterEach, describe, expect, it } from "vitest"; -import { configureFsSafeNative, FsSafeError, root as openRoot } from "../src/index.js"; +import { + categorizeFsSafeError, + configureFsSafeNative, + FsSafeError, + root as openRoot, +} from "../src/index.js"; import { openLocalFileSafely, readLocalFileSafely } from "../src/root.js"; import { __setFsSafeTestHooksForTest } from "../src/test-hooks.js"; import { expectedFsSafeCode } from "./helpers/security.js"; @@ -26,6 +31,20 @@ afterEach(async () => { }); describe("@openclaw/fs-safe", () => { + it("classifies routine removal outcomes as operational", () => { + for (const code of ["not-found", "not-empty", "not-removable"] as const) { + expect(categorizeFsSafeError(code)).toBe("operational"); + expect(new FsSafeError(code, "remove failed")).toMatchObject({ + category: "operational", + code, + }); + } + + for (const code of ["path-alias", "path-mismatch"] as const) { + expect(categorizeFsSafeError(code)).toBe("policy"); + } + }); + it.skipIf(skipOnWindows)("reuses a root capability across filesystem operations", async () => { const rootPath = await tempRoot("fs-root-object-"); const root = await openRoot(rootPath); @@ -396,6 +415,22 @@ describe("@openclaw/fs-safe", () => { }); }); + it("reports documented failure codes when remove cannot unlink the target", async () => { + const rootPath = await tempRoot("fs-safe-remove-codes-"); + const root = await openRoot(rootPath); + await root.mkdir("full/child"); + + await expect(root.remove("missing.txt")).rejects.toMatchObject({ + code: expectedFsSafeCode("not-found"), + }); + await expect(root.remove("missing-dir/missing.txt")).rejects.toMatchObject({ + code: expectedFsSafeCode("not-found"), + }); + await expect(root.remove("full")).rejects.toMatchObject({ + code: expectedFsSafeCode("not-empty"), + }); + }); + it("opens a file handle for fast reads when kernel fd path validation is available", async () => { const root = await openRoot(await tempRoot("fs-safe-open-")); await root.write("file.txt", "fast");