From 89736a4d2cbe5cc624d51e2b23f2646645737fd2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mat=C3=ADas=20D=2E?= <9351115+decode2@users.noreply.github.com> Date: Sun, 4 Oct 2026 00:19:20 +0000 Subject: [PATCH] fix(review): preserve exact candidate owner filesystem identity --- lib/review-candidate-view-owner.ts | 14 ++-- odd/tasks/candidate-owner-file-identity.md | 30 ++++++++ tests/review-candidate-view.test.ts | 88 ++++++++++++++++++++++ 3 files changed, 126 insertions(+), 6 deletions(-) create mode 100644 odd/tasks/candidate-owner-file-identity.md diff --git a/lib/review-candidate-view-owner.ts b/lib/review-candidate-view-owner.ts index ee234bc62..2052ce323 100644 --- a/lib/review-candidate-view-owner.ts +++ b/lib/review-candidate-view-owner.ts @@ -286,14 +286,15 @@ function posixPrivateModeProbe(path: string): boolean { } function directory(path: string, privateMode = false, platform: NodeJS.Platform = process.platform, classifyParentPrivacy = false): string { - const stat = lstatSync(path); + // 64-bit dev/ino may collide as Numbers; validate metadata and identity from one exact observation. + const stat = lstatSync(path, { bigint: true }); if (!stat.isDirectory() || stat.isSymbolicLink() || !samePath(realpathSync(path), path, platform)) throw new Error("Unsafe candidate owner directory"); if (privateMode && platform !== "win32") { const uid = process.getuid?.(); if (uid === undefined) throw new Error("Unsafe candidate owner directory"); - if (stat.uid !== uid || (stat.mode & 0o077) !== 0) { + if (stat.uid !== BigInt(uid) || (stat.mode & 0o077n) !== 0n) { if (classifyParentPrivacy) { - if (stat.uid === uid && (stat.mode & 0o077) !== 0 && !posixPrivateModeProbe(path)) throw new PosixCandidateOwnerParentChmodIneffectiveError(); + if (stat.uid === BigInt(uid) && (stat.mode & 0o077n) !== 0n && !posixPrivateModeProbe(path)) throw new PosixCandidateOwnerParentChmodIneffectiveError(); throw new PosixCandidateOwnerParentPrivacyError(); } throw new Error("Unsafe candidate owner directory"); @@ -329,10 +330,11 @@ export function prepareCandidateOwnerParent(commonDir: string, platform: NodeJS. } function regular(path: string, privateMode = false, platform: NodeJS.Platform = process.platform): string { - const stat = lstatSync(path); + // Keep 64-bit identity exact without a second stat lookup racing the privacy checks. + const stat = lstatSync(path, { bigint: true }); const uid = process.getuid?.(); - if (!stat.isFile() || stat.isSymbolicLink() || stat.nlink !== 1 || !samePath(realpathSync(path), path, platform) || stat.size > 16384 || - (privateMode && platform !== "win32" && (uid === undefined || stat.uid !== uid || (stat.mode & 0o777) !== 0o600))) throw new Error("Unsafe candidate owner file"); + if (!stat.isFile() || stat.isSymbolicLink() || stat.nlink !== 1n || !samePath(realpathSync(path), path, platform) || stat.size > 16384n || + (privateMode && platform !== "win32" && (uid === undefined || stat.uid !== BigInt(uid) || (stat.mode & 0o777n) !== 0o600n))) throw new Error("Unsafe candidate owner file"); if (privateMode && platform === "win32") privateWindowsDacl(path, "file", false); return `${stat.dev}:${stat.ino}`; } diff --git a/odd/tasks/candidate-owner-file-identity.md b/odd/tasks/candidate-owner-file-identity.md new file mode 100644 index 000000000..5c24f4fe7 --- /dev/null +++ b/odd/tasks/candidate-owner-file-identity.md @@ -0,0 +1,30 @@ +# Exact candidate-owner filesystem identity + +Refs #343 (approved cleanup tracker; remains closed). Independent follow-up to merged #672; blocks Windows check on coordination PR #1736. No main merge, auto-merge or runtime reload authorized. + +## Invariant and scope +Rollback and reclamation must preserve a replaced marker/root even when two filesystem identifiers exceed Number's safe range. Use exact dev/ino from the same filesystem observation; no new authority representation, retry or gate. Limit changes to the owner module, focused regression and this tracker. Forecast <150 changed lines; dedicated `fix/candidate-owner-file-identity` worktree, current main `a87192ee`. + +## Evidence before correction +Observed Windows CI run `37159261739`, job `111309064512`: original replacement-preservation test failed because marker was absent. Actual NTFS identifiers were not logged, so that exact CI cause is inferred, not measured. + +On clean current main, a deterministic actual-registry/real-filesystem fixture assigns two distinct 64-bit marker identities (`2^53`, `2^53+1`) which collide when represented as Number. It observes one controlled fsync replacement, no worktree addition, preserved original `.saved` and no lock, but rollback deletes the replacement. Focused command: `node --experimental-strip-types --test --test-name-pattern='candidate owner publication.*replaced|64-bit|precision' tests/review-candidate-view.test.ts`: one passed, one failed. This is modeled precision loss, not native NTFS measurement. + +## Plan +- [x] Current-main behavior RED and approved-root/duplicate audit. +- [x] Exact bigint identity, same-stat privacy/type/size validation and GREEN. +- [x] Relevant candidate/repository suite and types/runtime validation. +- [ ] Independent read-only verification by parent (high-risk deletion authority); no semantic approval or native verdict claimed. +- [ ] Separate PR and actual Windows CI; propagate fix without main merge and verify stacked checks. + +## Correction verification +Both `regular()` and `directory()` now request `{ bigint: true }` once; their existing template-string dev/ino identity API stays unchanged. UID/mode/nlink/size validation uses that same observation with bigint comparisons; the unrelated numeric chmod probe remains unchanged. No marker JSON, ACL, path, registration or PID-death changes. + +Additional controlled root-replacement regression retains the Git pointer and intercepts removal without executing it, protecting fixture replacement data even on RED. Before correction, the focused command above observed 3 tests: 1 pass, 2 fail (marker replacement deleted; directory replacement reached Git removal). After correction: 3 pass, 0 fail. + +- `node --experimental-strip-types --test tests/review-candidate-view.test.ts tests/review-repository.test.ts`: 173 tests, 165 pass, 8 Windows-only skips, 0 fail; existing privacy, symlink, replacement, failed-fsync/removal and death-proof controls pass. +- `node scripts/check-types.mjs`: 186 recorded diagnostics, no regressions; 12 file/code pairs improved (no baseline update). +- `node scripts/build-runtime-modules.mjs --check`: 8 generated modules match; one-shot metrics sources validated. +- `git diff --check`: passed. Native Windows CI is unavailable locally; modeled precision evidence does not measure the original NTFS identifiers. Review-ready uncommitted diff only; delivery and independent verification remain parent-owned. + +Rollback boundary: owner-module precision changes and their regression. Marker JSON, public APIs, privacy/ACL/registration/dead-owner gates must remain unchanged. CodeGraph at parent cwd indexes gentle-ai, not this repository; child CodeGraph unavailable/uninitialized, scoped known-path fallback used. diff --git a/tests/review-candidate-view.test.ts b/tests/review-candidate-view.test.ts index 296481fa5..dd2f4518c 100644 --- a/tests/review-candidate-view.test.ts +++ b/tests/review-candidate-view.test.ts @@ -364,6 +364,53 @@ test("candidate owner publication never removes a replaced marker", (t) => { assert.equal(readdirSync(parent).some((name) => name.endsWith(".reaper-lock")), false); }); +test("candidate owner publication preserves a replaced marker with 64-bit identity precision loss", (t) => { + mockWindowsAcl(t); + const cwd = repository(t), parent = join(cwd, ".git", "gentle-ai", "candidate-views"); + const originalLstat = fs.lstatSync, fsync = fs.fsyncSync, write = fs.writeFileSync; + // Modeled identities, not measured NTFS values: distinct 64-bit indices round to one Number. + const high = 2n ** 53n; + assert.notEqual(high, high + 1n); + assert.equal(Number(high), Number(high + 1n)); + let marker: string | undefined, replaced = false, faults = 0, adds = 0; + let savedContent: string | undefined; + t.mock.method(fs, "lstatSync", (path: Parameters[0], ...args: unknown[]) => { + const stat = (originalLstat as (...arguments_: unknown[]) => ReturnType)(path, ...args); + if (marker === undefined && typeof path === "string" && path.endsWith(".owner.json") && path === join(parent, readdirSync(parent).find((name) => name.endsWith(".owner.json")) ?? "")) marker = path; + if (path !== marker) return stat; + const bigint = (args[0] as { bigint?: boolean } | undefined)?.bigint === true; + const ino = replaced ? high + 1n : high; + // Retain real metadata and Stats methods; only the identity pair is synthetic. + return Object.assign(Object.create(Object.getPrototypeOf(stat)), stat, { + dev: bigint ? high : Number(high), ino: bigint ? ino : Number(ino), + }); + }); + t.mock.method(fs, "fsyncSync", (fd: number) => { + if (replaced) return fsync(fd); + assert.ok(marker, "owned marker identity must be captured before the fault"); + faults++; + savedContent = readFileSync(marker, "utf8"); + renameSync(marker, `${marker}.saved`); + write(marker, "replacement", { mode: 0o600 }); + replaced = true; + throw new Error("fixture marker fsync failure after 64-bit identity replacement"); + }); + syncBuiltinESMExports(); + t.after(() => { t.mock.restoreAll(); syncBuiltinESMExports(); }); + const registry = new CandidateViewRegistry((file, args, options) => { + if (args[0] === "worktree" && args[1] === "add") adds++; + return execFileSync(file, args, options); + }, "win32"); + assert.throws(() => registry.create({ contributorRoot: cwd }), (error: unknown) => error instanceof CandidateViewError && error.reason === "candidate-owner-preparation-failed"); + assert.equal(faults, 1, "replacement fault must be reached exactly once"); + assert.equal(adds, 0); + assert.ok(marker); + assert.equal(readFileSync(`${marker}.saved`, "utf8"), savedContent); + assert.equal(readdirSync(parent).some((name) => name.endsWith(".reaper-lock")), false); + assert.equal(existsSync(marker), true, "publication rollback deleted the replacement after distinct 64-bit identities rounded to the same Number"); + assert.equal(readFileSync(marker, "utf8"), "replacement"); +}); + test("public POSIX candidate-views parent reports bounded privacy guidance without changing permissions or adding a worktree", { skip: process.platform === "win32" }, (t) => { const cwd = repository(t); const parent = join(cwd, ".git", "gentle-ai", "candidate-views"); @@ -721,6 +768,47 @@ for (const race of ["root", "registration", "lock"] as const) { }); } +test("ordinary cleanup preserves a replaced root with 64-bit identity precision loss", (t) => { + mockWindowsAcl(t); + const cwd = repository(t), high = 2n ** 53n; + const originalLstat = fs.lstatSync; + let root = "", replaced = false, lists = 0, removes = 0; + const registry = new CandidateViewRegistry((file, args, options) => { + // Never allow Git to delete the replacement, even on the failing implementation. + if (args[0] === "worktree" && args[1] === "remove") { removes++; return ""; } + const result = execFileSync(file, args, options); + if (root && args[0] === "worktree" && args[1] === "list" && ++lists === 2) { + const pointer = readFileSync(join(root, ".git")); + renameSync(root, `${root}.saved`); + mkdirSync(root); + writeFileSync(join(root, ".git"), pointer); + writeFileSync(join(root, "replacement.txt"), "replacement"); + replaced = true; + } + return result; + }); + const view = registry.create({ contributorRoot: cwd }); + root = view.root; + t.mock.method(fs, "lstatSync", (path: Parameters[0], ...args: unknown[]) => { + const stat = (originalLstat as (...arguments_: unknown[]) => ReturnType)(path, ...args); + if (path !== root) return stat; + const bigint = (args[0] as { bigint?: boolean } | undefined)?.bigint === true; + const ino = replaced ? high + 1n : high; + return Object.assign(Object.create(Object.getPrototypeOf(stat)), stat, { + dev: bigint ? high : Number(high), ino: bigint ? ino : Number(ino), + }); + }); + syncBuiltinESMExports(); + t.after(() => { t.mock.restoreAll(); syncBuiltinESMExports(); }); + assert.throws(() => view.cleanup()); + assert.equal(replaced, true); + assert.equal(readFileSync(join(root, "replacement.txt"), "utf8"), "replacement"); + assert.equal(existsSync(`${root}.saved`), true); + assert.equal(existsSync(ownerMarker(root)), true); + assert.equal(existsSync(`${root}.reaper-lock`), false); + assert.equal(removes, 0, "distinct 64-bit directory identities must stop cleanup before Git removal"); +}); + test("ordinary cleanup never unlinks a replaced sidecar after Git removal", (t) => { const cwd = repository(t); let marker = "";