diff --git a/tests/helpers/hooks-dist.cjs b/tests/helpers/hooks-dist.cjs index 29f37fd52..d31d69a03 100644 --- a/tests/helpers/hooks-dist.cjs +++ b/tests/helpers/hooks-dist.cjs @@ -7,29 +7,108 @@ * tests, so the first test that needs hooks/dist would fail. This mirrors * the pattern used in bug-3357-codex-legacy-hooks-json-migration.test.cjs. * - * Idempotent: only rebuilds when the directory is absent or empty of .js - * files. Extracted from six behaviorally-identical copies that had - * accumulated across tests/install.test.cjs (x2) and - * tests/install-minimal-hooks.test.cjs (x4) — see #2704's Failure B, where a - * seventh suite (tests/mcp-catalog-parity.install.test.cjs) needed the same - * guard but had no copy of its own, and so failed only on lanes where no - * other suite happened to build hooks/dist first. + * Idempotent: `isHooksDistStale` rebuilds only when the directory is absent + * or missing an entry from the EXPECTED SET — `scripts/build-hooks.js`'s + * exported `HOOKS_TO_COPY` list. This replaces a former `.js`-extension-count + * heuristic ("populated" if at least one `.js` file exists), which could not + * see a dist missing exactly the file class that caused #3108: build-hooks + * also ships `.sh` files (e.g. `gsd-session-state.sh`), so a dist with every + * `.js` present and every `.sh` absent read as fully populated and the + * rebuild was silently skipped. The set-membership check has no such blind + * spot: any expected entry missing, of any extension, is stale. It does NOT + * flag extra/unexpected files as stale — hooks/dist legitimately accumulates + * output the list does not name (subdirectory output, hooks/lib) — and it + * stays cheap (one `readdirSync` into a `Set`, no per-entry `existsSync`, no + * `statSync`) since it runs once per suite. + * + * Extracted from six behaviorally-identical copies that had accumulated + * across tests/install.test.cjs (x2) and tests/install-minimal-hooks.test.cjs + * (x4) — see #2704's Failure B, where a seventh suite + * (tests/mcp-catalog-parity.install.test.cjs) needed the same guard but had + * no copy of its own, and so failed only on lanes where no other suite + * happened to build hooks/dist first. */ const fs = require('node:fs'); const path = require('node:path'); -const { runNode } = require('./process-seam.cjs'); +// Required as a module object (not destructured) so tests can monkeypatch +// `processSeam.runNode` in place and have `ensureHooksDist` observe the +// replacement — a destructured `const { runNode } = require(...)` would +// bind a local reference at require-time that a later patch to the +// process-seam module's exports could never reach. +const processSeam = require('./process-seam.cjs'); const { throwIfFailed } = require('./git-fixture.cjs'); const { BUILD_TIMEOUT_MS } = require('./timeouts.cjs'); +const { HOOKS_TO_COPY, HOOKS_SUBDIRS_TO_COPY } = require('../../scripts/build-hooks.js'); const REPO_ROOT = path.resolve(__dirname, '..', '..'); const HOOKS_DIST_DIR = path.join(REPO_ROOT, 'hooks', 'dist'); const BUILD_HOOKS_SCRIPT = path.join(REPO_ROOT, 'scripts', 'build-hooks.js'); +/** + * True when `dir` is missing, or missing any entry `scripts/build-hooks.js` + * expects to have copied there (`HOOKS_TO_COPY`, bare top-level filenames, + * and `HOOKS_SUBDIRS_TO_COPY`, bare subdirectory names such as `lib`). A dist + * with every top-level file present but no `lib/` (e.g. missing + * `gsd-graphify-rebuild.sh`) is exactly the same blindness the old + * `.js`-count heuristic had, one level down — so each subdir entry is first + * checked against the same top-level readdir Set, THEN additionally required + * to be a readable, non-empty directory (one extra `readdirSync` per subdir + * entry — there is exactly one, `lib` — never a per-expected-file + * `existsSync`/`statSync`). Presence alone is not enough: a stray regular + * file named `lib`, or an empty `lib/`, would otherwise read as fresh, which + * is the same blind spot one level down again. Extra/unexpected entries never + * count as stale — this is a "is everything expected present and populated" + * check, not an exact-set check. + * + * @param {string} dir + * @returns {boolean} + */ +function isHooksDistStale(dir) { + if (!fs.existsSync(dir)) return true; + let present; + try { + present = new Set(fs.readdirSync(dir)); + } catch (e) { + // dir exists (existsSync passed above) but readdirSync still threw — + // e.g. dir is actually a regular file (ENOTDIR), unreadable (EACCES), + // or was removed in the race between the two calls. Unreadable is + // indistinguishable from unusable here, and treating it as stale is + // both safe (rebuilding is idempotent) and the likely remedy — whereas + // throwing would fail every suite's before() on a diagnosis it cannot + // act on. + return true; + } + if (HOOKS_TO_COPY.some((name) => !present.has(name))) return true; + for (const name of HOOKS_SUBDIRS_TO_COPY) { + if (!present.has(name)) return true; + // Presence in the top-level Set only proves an entry named `lib` + // exists — not that it is a directory, nor that it is populated. A + // stray regular file named `lib`, or an empty `lib/` missing e.g. + // `gsd-graphify-rebuild.sh`, would otherwise read as fresh: exactly + // the blind spot the subdir check exists to close. One `readdirSync` + // per subdir entry (there is exactly one, `lib`) — no per-expected-file + // `existsSync`/`statSync`, keeping the added cost to one syscall total. + try { + if (fs.readdirSync(path.join(dir, name)).length === 0) return true; + } catch (e) { + // ENOTDIR (regular file), EACCES, or a race where it vanished — + // unreadable/unusable is indistinguishable from absent here, and + // treating it as stale is safe (idempotent rebuild) per the same + // posture as the top-level readdirSync catch above. + return true; + } + } + return false; +} + function ensureHooksDist() { - if (!fs.existsSync(HOOKS_DIST_DIR) || fs.readdirSync(HOOKS_DIST_DIR).filter((f) => f.endsWith('.js')).length === 0) { - throwIfFailed(runNode([BUILD_HOOKS_SCRIPT], { timeoutMs: BUILD_TIMEOUT_MS }), `node ${BUILD_HOOKS_SCRIPT}`); + if (isHooksDistStale(HOOKS_DIST_DIR)) { + throwIfFailed( + processSeam.runNode([BUILD_HOOKS_SCRIPT], { timeoutMs: BUILD_TIMEOUT_MS }), + `node ${BUILD_HOOKS_SCRIPT}`, + ); } } -module.exports = { ensureHooksDist, HOOKS_DIST_DIR, BUILD_HOOKS_SCRIPT }; +module.exports = { ensureHooksDist, isHooksDistStale, HOOKS_DIST_DIR, BUILD_HOOKS_SCRIPT }; diff --git a/tests/helpers/overlay-repo.cjs b/tests/helpers/overlay-repo.cjs index 436be73eb..9fccec835 100644 --- a/tests/helpers/overlay-repo.cjs +++ b/tests/helpers/overlay-repo.cjs @@ -85,23 +85,85 @@ function isMissingPath(err) { * the tree (nothing to mirror, so the leaf is skipped). No sleep, no spin — a * timing-based wait here would be the flake this is fixing, not a fix for it. * - * Returns false only when the path left the source tree entirely; the overlay - * mirrors the tree, and a file that is no longer in it is not part of the - * snapshot. Every other error propagates untouched. + * The retry itself can also lose the race — a second atomic replace landing in + * the same window makes the retry throw ENOENT too (e.g. two concurrent + * `build:hooks` runs). That is still just "the path is vanishing": the same + * conclusion the single-vanish case reaches, so it is likewise treated as + * skipped rather than left to escape as a bare uncaught ENOENT (#3108). There + * is still only ONE retry — a second retry would turn this into the sleep/spin + * loop the comment above already rejects. + * + * Returns false only when the path left the source tree entirely (on the + * first attempt OR the retry); the overlay mirrors the tree, and a file that + * is no longer in it is not part of the snapshot. Every other error — from + * EITHER attempt, non-ENOENT — propagates untouched; that invariant must hold + * for any future widening of this tolerance. + * + * When no `destPath` is supplied, an ENOENT is tolerated as a vanished + * source: on the FIRST attempt's ENOENT, `srcPath` is re-checked with + * `fs.existsSync` to decide whether the retry is even worth attempting (gone + * already -> skip, no retry); if the retry's own attempt ALSO throws ENOENT, + * that is tolerated UNCONDITIONALLY — by the time a second atomic replace has + * landed in the same window there is nothing left to meaningfully re-check, + * and this is deliberately optimistic rather than throwing on the race this + * function exists to tolerate. + * + * Two discriminators that look like they should tell a vanished-source ENOENT + * apart from a dest-side one (a missing DEST parent directory, e.g. a Windows + * MAX_PATH failure or a concurrently-removed dest subtree) both fail, and + * must not be reached for again here: + * - `fs.existsSync(srcPath)` re-checked at catch time: in the genuine + * double-vanish race the source is being atomically REPLACED (e.g. + * `hooks/dist`'s unlink+rename), so it can be present again by the time + * the ENOENT is handled even though the ENOENT was genuinely + * source-side. Gating the RETRY's ENOENT on it throws on exactly the + * race this function exists to tolerate (#3108 regression). + * - `err.path`: empirically, Node's `fs.linkSync` reports the SOURCE path + * in `err.path` for BOTH a missing source and a missing dest parent + * directory — it does not distinguish them either. + * + * The only discriminator that actually works is the DEST PARENT DIRECTORY, + * because `buildOverlayRepo` builds its own dest tree (`fs.mkdirSync(destDir, + * {recursive:true})` before every walk, into a private `mkdtempSync` root no + * other process touches) — so a missing dest parent is always a bug, never + * the atomic-replace race. Callers that know the dest path (`linkOrCopyFile`, + * the `copy`-mode branch in `place()`) pass it as `destPath`; when supplied, + * it REPLACES the source-existence check entirely (on both the first attempt + * and the retry): an ENOENT is tolerated as a vanished source only if the + * dest parent is confirmed present, and rethrown untouched if the dest + * parent is missing. Callers with no dest to check keep the source-only + * logic above, unchanged. * * @param {string} srcPath * @param {() => void} attempt + * @param {string} [destPath] - when supplied, an ENOENT (on either attempt) + * is tolerated as a vanished source only if + * `fs.existsSync(path.dirname(destPath))`; a missing dest parent rethrows + * instead (see discriminator discussion above). * @returns {boolean} whether the leaf was placed */ -function placeVanishableLeaf(srcPath, attempt) { +function placeVanishableLeaf(srcPath, attempt, destPath) { + function destParentPresent() { + return fs.existsSync(path.dirname(destPath)); + } try { attempt(); return true; } catch (err) { if (!isMissingPath(err)) throw err; - if (!fs.existsSync(srcPath)) return false; - attempt(); - return true; + if (destPath !== undefined) { + if (!destParentPresent()) throw err; + } else if (!fs.existsSync(srcPath)) { + return false; + } + try { + attempt(); + return true; + } catch (retryErr) { + if (!isMissingPath(retryErr)) throw retryErr; + if (destPath !== undefined && !destParentPresent()) throw retryErr; + return false; + } } } @@ -111,17 +173,21 @@ function placeVanishableLeaf(srcPath, attempt) { * Returns whether the leaf was placed; false means the source vanished * mid-walk (see `placeVanishableLeaf`). */ function linkOrCopyFile(src, dest) { - return placeVanishableLeaf(src, () => { - try { - fs.linkSync(src, dest); - } catch (err) { - if (err.code === 'EXDEV' || err.code === 'EPERM') { - fs.copyFileSync(src, dest); - } else { - throw err; + return placeVanishableLeaf( + src, + () => { + try { + fs.linkSync(src, dest); + } catch (err) { + if (err.code === 'EXDEV' || err.code === 'EPERM') { + fs.copyFileSync(src, dest); + } else { + throw err; + } } - } - }); + }, + dest, + ); } /** @@ -133,14 +199,18 @@ function linkOrCopyFile(src, dest) { * `fs.rmSync(..., {recursive:true, force:true})` it away. * * @param {{[relPath: string]: string}} fileOverrides - * @param {{mode?: 'link'|'copy'}} [opts] - `mode` defaults to `'link'` so - * every pre-existing caller is unchanged. Pass `{mode: 'copy'}` when the - * overlay must survive a real `--write` generator run (see the module doc - * above) — every leaf file becomes a real independent inode, so no write - * inside the overlay can ever reach `REPO_ROOT`. + * @param {{mode?: 'link'|'copy', warn?: (msg: string) => void}} [opts] - + * `mode` defaults to `'link'` so every pre-existing caller is unchanged. + * Pass `{mode: 'copy'}` when the overlay must survive a real `--write` + * generator run (see the module doc above) — every leaf file becomes a + * real independent inode, so no write inside the overlay can ever reach + * `REPO_ROOT`. `warn` defaults to `console.warn` (byte-identical to every + * existing caller) and exists so a test can inject a spy to assert on the + * skipped-leaf warning without capturing real console output. */ function buildOverlayRepo(fileOverrides, opts = {}) { const mode = opts.mode || 'link'; + const warn = opts.warn || console.warn; const tmpRepo = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-2930-overlay-')); const entries = Object.entries(fileOverrides).map(([relPath, content]) => ({ parts: relPath.split('/'), @@ -189,7 +259,11 @@ function buildOverlayRepo(fileOverrides, opts = {}) { // Real independent inode — a write through this path in the overlay // can never alias back to REPO_ROOT's own tracked file (see // opts.mode doc above). - const placed = placeVanishableLeaf(srcPath, () => fs.copyFileSync(srcPath, destPath)); + const placed = placeVanishableLeaf( + srcPath, + () => fs.copyFileSync(srcPath, destPath), + destPath, + ); if (!placed) skipped.push(srcPath); } else { const placed = linkOrCopyFile(srcPath, destPath); @@ -206,9 +280,10 @@ function buildOverlayRepo(fileOverrides, opts = {}) { // exists to remove. But it must not be SILENT either — a dropped leaf can // surface later as a confusing "file missing" in an unrelated assertion, or // as nothing at all for a test that never touches it. - console.warn( + warn( `buildOverlayRepo: ${skipped.length} source file(s) vanished mid-walk and were ` + - `omitted from the overlay (likely a concurrent atomic replace, e.g. hooks/dist):\n ` + + `omitted from the overlay (likely a concurrent atomic replace, e.g. hooks/dist — ` + + `run \`npm run build:hooks\` to regenerate it):\n ` + skipped.join('\n '), ); } diff --git a/tests/overlay-repo-helpers.test.cjs b/tests/overlay-repo-helpers.test.cjs new file mode 100644 index 000000000..c8eebc7bc --- /dev/null +++ b/tests/overlay-repo-helpers.test.cjs @@ -0,0 +1,694 @@ +'use strict'; + +/** + * Failing-first (RED) coverage for issue #3108. + * + * Two independent gaps around `hooks/dist` tolerance: + * + * A. `placeVanishableLeaf` (tests/helpers/overlay-repo.cjs) tolerates a + * SINGLE ENOENT on the first attempt via one unguarded retry — but if the + * retry ALSO throws ENOENT (the leaf vanished a second time, e.g. two + * concurrent `build:hooks` runs racing an atomic replace), the bare retry + * escapes as an uncaught exception instead of being treated as "left the + * tree" like the first-attempt case. + * + * B. `ensureHooksDist` (tests/helpers/hooks-dist.cjs) only rebuilds when + * hooks/dist is absent or has zero `.js` files. `scripts/build-hooks.js` + * also ships `.sh` files (HOOKS_TO_COPY), so a dist dir missing every + * `.sh` entry is extension-blind-accepted as "populated" and never + * rebuilt. The fix is a pure, exported `isHooksDistStale(dir)` predicate + * driven off the expected set (HOOKS_TO_COPY), not one extension's count. + * That export does not exist yet — requiring it is itself part of the RED + * state. + * + * Group 3 (the skipped-leaf warning naming `npm run build:hooks`) asserts on + * the warning text directly, via `buildOverlayRepo`'s injectable `opts.warn` + * (defaulting to `console.warn`, byte-identical for every existing caller) — + * no TOCTOU race or source-grep needed. + */ + +const { describe, test } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); +const os = require('node:os'); +const fc = require('fast-check'); + +const { + placeVanishableLeaf, + linkOrCopyFile, + buildOverlayRepo, +} = require('./helpers/overlay-repo.cjs'); +const { HOOKS_TO_COPY, HOOKS_SUBDIRS_TO_COPY } = require('../scripts/build-hooks.js'); +const { createTempDir, cleanup } = require('./helpers.cjs'); + +// ── Group 1: placeVanishableLeaf ──────────────────────────────────────────── + +describe('placeVanishableLeaf: vanish tolerance', () => { + test('a leaf that never vanishes is placed on the first attempt', () => { + let calls = 0; + const attempt = () => { calls += 1; }; + const placed = placeVanishableLeaf('/does/not/matter', attempt); + assert.strictEqual(placed, true); + assert.strictEqual(calls, 1); + }); + + test('a leaf replaced once mid-walk is placed by the single retry', () => { + const srcPath = path.join(os.tmpdir(), 'gsd-3108-fixture-does-not-exist'); + let calls = 0; + const attempt = () => { + calls += 1; + if (calls === 1) { + const err = new Error('ENOENT: no such file or directory'); + err.code = 'ENOENT'; + throw err; + } + }; + const originalExistsSync = fs.existsSync; + fs.existsSync = (p) => (p === srcPath ? true : originalExistsSync(p)); + try { + const placed = placeVanishableLeaf(srcPath, attempt); + assert.strictEqual(placed, true); + assert.strictEqual(calls, 2); + } finally { + fs.existsSync = originalExistsSync; + } + }); + + test('a leaf that leaves the tree is skipped, not fatal', () => { + const srcPath = path.join(os.tmpdir(), 'gsd-3108-fixture-gone'); + let calls = 0; + const attempt = () => { + calls += 1; + const err = new Error('ENOENT: no such file or directory'); + err.code = 'ENOENT'; + throw err; + }; + const originalExistsSync = fs.existsSync; + fs.existsSync = (p) => (p === srcPath ? false : originalExistsSync(p)); + try { + const placed = placeVanishableLeaf(srcPath, attempt); + assert.strictEqual(placed, false); + assert.strictEqual(calls, 1); + } finally { + fs.existsSync = originalExistsSync; + } + }); + + // Deliverable — fails today: the bare retry call in placeVanishableLeaf + // throws an uncaught ENOENT instead of returning false when the leaf + // vanishes AGAIN during the retry. + test('a leaf that vanishes again during the retry is skipped, not a bare ENOENT', () => { + const srcPath = path.join(os.tmpdir(), 'gsd-3108-fixture-double-vanish'); + let calls = 0; + const attempt = () => { + calls += 1; + const err = new Error('ENOENT: no such file or directory'); + err.code = 'ENOENT'; + throw err; + }; + const originalExistsSync = fs.existsSync; + fs.existsSync = (p) => (p === srcPath ? true : originalExistsSync(p)); + try { + let placed; + assert.doesNotThrow(() => { + placed = placeVanishableLeaf(srcPath, attempt); + }); + assert.strictEqual(placed, false); + assert.strictEqual(calls, 2); + } finally { + fs.existsSync = originalExistsSync; + } + }); + + test('a non-ENOENT error is never swallowed by the vanish tolerance', () => { + const attempt = () => { + const err = new Error('EACCES: permission denied'); + err.code = 'EACCES'; + throw err; + }; + assert.throws(() => placeVanishableLeaf('/irrelevant', attempt), /EACCES/); + }); + + test('a non-ENOENT error on the retry still propagates', () => { + const srcPath = path.join(os.tmpdir(), 'gsd-3108-fixture-retry-eacces'); + let calls = 0; + const attempt = () => { + calls += 1; + if (calls === 1) { + const err = new Error('ENOENT: no such file or directory'); + err.code = 'ENOENT'; + throw err; + } + const err = new Error('EACCES: permission denied'); + err.code = 'EACCES'; + throw err; + }; + const originalExistsSync = fs.existsSync; + fs.existsSync = (p) => (p === srcPath ? true : originalExistsSync(p)); + try { + assert.throws(() => placeVanishableLeaf(srcPath, attempt), /EACCES/); + assert.strictEqual(calls, 2); + } finally { + fs.existsSync = originalExistsSync; + } + }); + + // Regression pin for #3108's review finding: linkOrCopyFile's attempt can + // throw ENOENT because the DEST parent is missing (Windows MAX_PATH, a + // concurrently-removed dest subtree), which has nothing to do with the + // source tree. `fs.existsSync(srcPath)` and `err.path` both fail to + // discriminate this from a genuine vanished-source ENOENT (see the doc + // comment on `placeVanishableLeaf` in tests/helpers/overlay-repo.cjs), so + // this is driven through `linkOrCopyFile` with a REAL source file and a + // dest whose parent directory does not exist — the actual mechanism, + // rather than a source-existence mock that cannot tell the two apart. + test('a dest-side ENOENT is not mistaken for a vanished source', () => { + const tmpBase = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-3108-dest-side-')); + const src = path.join(tmpBase, 'real-source-leaf.txt'); + const dest = path.join(tmpBase, 'missing-parent-dir', 'leaf.txt'); + fs.writeFileSync(src, 'content\n'); + try { + assert.throws(() => linkOrCopyFile(src, dest), /ENOENT/); + } finally { + cleanup(tmpBase); + } + }); + + // Pins that the guard keys ONLY on the dest parent, not on source + // existence: the dest parent is present, so an ENOENT on both the attempt + // and the retry is still treated as a vanished source (returns false), + // never a throw. + test('an ENOENT with the dest parent present is treated as a vanished source', () => { + const tmpBase = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-3108-dest-present-')); + const srcPath = path.join(os.tmpdir(), 'gsd-3108-fixture-dest-present-src'); + const destPath = path.join(tmpBase, 'leaf.txt'); + let calls = 0; + const attempt = () => { + calls += 1; + const err = new Error('ENOENT: no such file or directory'); + err.code = 'ENOENT'; + throw err; + }; + try { + let placed; + assert.doesNotThrow(() => { + placed = placeVanishableLeaf(srcPath, attempt, destPath); + }); + assert.strictEqual(placed, false); + assert.strictEqual(calls, 2); + } finally { + cleanup(tmpBase); + } + }); + + test('a real vanished source is still tolerated after the fix', () => { + const srcPath = path.join(os.tmpdir(), 'gsd-3108-fixture-real-double-vanish'); + let calls = 0; + const attempt = () => { + calls += 1; + const err = new Error('ENOENT: no such file or directory'); + err.code = 'ENOENT'; + throw err; + }; + const originalExistsSync = fs.existsSync; + // Source is genuinely gone at the re-check, both times. + fs.existsSync = (p) => (p === srcPath ? false : originalExistsSync(p)); + try { + let placed; + assert.doesNotThrow(() => { + placed = placeVanishableLeaf(srcPath, attempt); + }); + assert.strictEqual(placed, false); + assert.strictEqual(calls, 1); + } finally { + fs.existsSync = originalExistsSync; + } + }); + + test('linkOrCopyFile throws (not skips) when the source is real but the dest parent is missing', () => { + const tmpBase = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-3108-linkorcopy-')); + const src = path.join(tmpBase, 'real-source-leaf.txt'); + const dest = path.join(tmpBase, 'missing-parent-dir', 'leaf.txt'); + fs.writeFileSync(src, 'content\n'); + try { + assert.throws(() => linkOrCopyFile(src, dest), /ENOENT/); + } finally { + cleanup(tmpBase); + } + }); + + // EACCES is exercised standalone above; the sweep below only adds codes + // not already covered, so it does not re-test EACCES a second time. + for (const code of ['EMFILE', 'EPERM']) { + test(`a non-ENOENT error (${code}) is never swallowed by the vanish tolerance`, () => { + const attempt = () => { + const err = new Error(`${code}: synthetic failure`); + err.code = code; + throw err; + }; + assert.throws(() => placeVanishableLeaf('/irrelevant-not-a-link-path', attempt), new RegExp(code)); + }); + } +}); + +// ── Group 2: ensureHooksDist staleness predicate ──────────────────────────── + +describe('isHooksDistStale: extension-agnostic staleness predicate', () => { + // Does not exist yet — this require is expected to fail until the + // implementation step adds the named export. Deferred inside each test + // (rather than at module scope) so the other groups in this file can still + // run and report their own pass/fail independently. + function loadPredicate() { + const mod = require('./helpers/hooks-dist.cjs'); + return mod.isHooksDistStale; + } + + function mkTmpDir() { + return createTempDir('gsd-3108-hooks-dist-'); + } + + function rmTmpDir(dir) { + cleanup(dir); + } + + function writeExpectedSet(dir, { includeJs = true, includeSh = true, extra = false, includeSubdirs = true } = {}) { + for (const name of HOOKS_TO_COPY) { + if (name.endsWith('.js') && !includeJs) continue; + if (name.endsWith('.sh') && !includeSh) continue; + fs.writeFileSync(path.join(dir, name), '// fixture\n'); + } + if (includeSubdirs) { + for (const name of HOOKS_SUBDIRS_TO_COPY) { + const subdir = path.join(dir, name); + fs.mkdirSync(subdir, { recursive: true }); + // A non-empty subdir — an empty dir does not count as "populated" + // (see the dedicated empty-subdir test below). + fs.writeFileSync(path.join(subdir, 'fixture-leaf.txt'), '// fixture\n'); + } + } + if (extra) { + fs.writeFileSync(path.join(dir, 'zz-unexpected.txt'), 'not a hook\n'); + } + } + + test('an absent hooks/dist triggers the build', () => { + const isHooksDistStale = loadPredicate(); + const dir = path.join(os.tmpdir(), 'gsd-3108-hooks-dist-absent-does-not-exist'); + assert.strictEqual(isHooksDistStale(dir), true); + }); + + test('an empty hooks/dist triggers the build', () => { + const isHooksDistStale = loadPredicate(); + const dir = mkTmpDir(); + try { + assert.strictEqual(isHooksDistStale(dir), true); + } finally { + rmTmpDir(dir); + } + }); + + // Deliverable — fails today: no isHooksDistStale export exists, and the + // current inline predicate in ensureHooksDist only counts .js files, so a + // dist missing every .sh entry would be (wrongly) accepted as populated. + test('a hooks/dist missing every .sh is not considered populated', () => { + const isHooksDistStale = loadPredicate(); + const dir = mkTmpDir(); + try { + writeExpectedSet(dir, { includeJs: true, includeSh: false }); + assert.strictEqual(isHooksDistStale(dir), true); + } finally { + rmTmpDir(dir); + } + }); + + test('a hooks/dist missing every .js is not considered populated', () => { + const isHooksDistStale = loadPredicate(); + const dir = mkTmpDir(); + try { + writeExpectedSet(dir, { includeJs: false, includeSh: true }); + assert.strictEqual(isHooksDistStale(dir), true); + } finally { + rmTmpDir(dir); + } + }); + + test('a complete hooks/dist does not trigger a rebuild', () => { + const isHooksDistStale = loadPredicate(); + const dir = mkTmpDir(); + try { + writeExpectedSet(dir); + assert.strictEqual(isHooksDistStale(dir), false); + } finally { + rmTmpDir(dir); + } + }); + + test('an unexpected extra file does not force a rebuild', () => { + const isHooksDistStale = loadPredicate(); + const dir = mkTmpDir(); + try { + writeExpectedSet(dir, { extra: true }); + assert.strictEqual(isHooksDistStale(dir), false); + } finally { + rmTmpDir(dir); + } + }); + + // Deliverable — fails today: HOOKS_SUBDIRS_TO_COPY entries (e.g. `lib`, + // which carries gsd-graphify-rebuild.sh, #3579) are not checked at all, so + // a dist with every top-level file but no `lib/` reads as fully populated — + // the exact blindness the old `.js`-count predicate had, one level down. + test('a hooks/dist missing the lib subdirectory is not considered populated', () => { + const isHooksDistStale = loadPredicate(); + const dir = mkTmpDir(); + try { + writeExpectedSet(dir, { includeSubdirs: false }); + assert.strictEqual(isHooksDistStale(dir), true); + } finally { + rmTmpDir(dir); + } + }); + + test('a complete hooks/dist including lib is not stale', () => { + const isHooksDistStale = loadPredicate(); + const dir = mkTmpDir(); + try { + writeExpectedSet(dir, { includeSubdirs: true }); + assert.strictEqual(isHooksDistStale(dir), false); + } finally { + rmTmpDir(dir); + } + }); + + // Deliverable — presence in the top-level readdir Set alone is not + // enough: an empty `lib/` (missing e.g. gsd-graphify-rebuild.sh) is the + // exact missing-file-class case the subdir check exists to catch, and a + // bare membership check cannot see it. + test('an empty lib subdirectory is not considered populated', () => { + const isHooksDistStale = loadPredicate(); + const dir = mkTmpDir(); + try { + writeExpectedSet(dir, { includeSubdirs: false }); + for (const name of HOOKS_SUBDIRS_TO_COPY) { + fs.mkdirSync(path.join(dir, name), { recursive: true }); + } + assert.strictEqual(isHooksDistStale(dir), true); + } finally { + rmTmpDir(dir); + } + }); + + // Deliverable — a stray regular FILE named `lib` also satisfies bare + // top-level Set membership; readdirSync on it must throw ENOTDIR, which + // is caught and treated as stale, not left to escape uncaught. + test('a regular file named lib is not considered populated', () => { + const isHooksDistStale = loadPredicate(); + const dir = mkTmpDir(); + try { + writeExpectedSet(dir, { includeSubdirs: false }); + for (const name of HOOKS_SUBDIRS_TO_COPY) { + fs.writeFileSync(path.join(dir, name), 'i am a file, not a dir\n'); + } + let stale; + assert.doesNotThrow(() => { + stale = isHooksDistStale(dir); + }); + assert.strictEqual(stale, true); + } finally { + rmTmpDir(dir); + } + }); + + // Deliverable — fails today: readdirSync throws ENOTDIR when `dir` is a + // regular file, and isHooksDistStale lets that escape uncaught, failing + // every suite's before() on a condition it cannot act on. Unreadable is + // indistinguishable from unusable here; treating it as stale (safe, + // rebuild-triggering) is the correct outcome, not a throw. + test('a hooks/dist path that is a regular file is treated as stale, not thrown', () => { + const isHooksDistStale = loadPredicate(); + const dir = mkTmpDir(); + const filePath = path.join(dir, 'not-a-directory'); + fs.writeFileSync(filePath, 'i am a file, not a dir\n'); + try { + let stale; + assert.doesNotThrow(() => { + stale = isHooksDistStale(filePath); + }); + assert.strictEqual(stale, true); + } finally { + rmTmpDir(dir); + } + }); +}); + +// ── Group 2b: ensureHooksDist build-seam wiring ───────────────────────────── +// +// Group 2 above only exercises the pure predicate `isHooksDistStale` — none +// of it calls `ensureHooksDist`, so restoring the OLD inline `.js`-count +// check inside `ensureHooksDist` (while leaving `isHooksDistStale` exported +// and correct) would keep every Group-2 test green. These two tests drive +// `ensureHooksDist` itself and assert on the build seam, without running a +// real `build-hooks.js` subprocess: `fs.existsSync`/`fs.readdirSync` are +// monkeypatched (scoped to `HOOKS_DIST_DIR` only, restored in `finally`) to +// fake staleness/freshness with ZERO mutation of the worktree's real +// `hooks/dist`, and `processSeam.runNode` — the module OBJECT +// `hooks-dist.cjs` now calls through (`processSeam.runNode(...)`) rather +// than a destructured reference — is monkeypatched to observe whether the +// build seam was invoked, without ever spawning `build-hooks.js`. + +describe('ensureHooksDist: build-seam wiring', () => { + const { ensureHooksDist, HOOKS_DIST_DIR } = require('./helpers/hooks-dist.cjs'); + const processSeam = require('./helpers/process-seam.cjs'); + + // Replaces fs.existsSync/fs.readdirSync process-wide for the duration of + // `run`. Safe ONLY because this file's tests execute sequentially — adding + // `{ concurrency: true }` to this file (or running it alongside another + // suite in the same process) would let a concurrently-running test observe + // these faked responses and cross-contaminate unrelated assertions. + function withFakeDistState({ stale }, run) { + const originalExistsSync = fs.existsSync; + const originalReaddirSync = fs.readdirSync; + if (stale) { + // Absent is the simplest, unambiguous stale signal — short-circuits + // isHooksDistStale before it ever calls readdirSync. + fs.existsSync = (p) => (p === HOOKS_DIST_DIR ? false : originalExistsSync(p)); + } else { + fs.existsSync = (p) => (p === HOOKS_DIST_DIR ? true : originalExistsSync(p)); + fs.readdirSync = (p, ...rest) => { + if (p === HOOKS_DIST_DIR) return [...HOOKS_TO_COPY, ...HOOKS_SUBDIRS_TO_COPY]; + for (const name of HOOKS_SUBDIRS_TO_COPY) { + if (p === path.join(HOOKS_DIST_DIR, name)) return ['fixture-leaf.txt']; + } + return originalReaddirSync(p, ...rest); + }; + } + try { + run(); + } finally { + fs.existsSync = originalExistsSync; + fs.readdirSync = originalReaddirSync; + } + } + + function fakeSuccessfulRun() { + return { + outcome: 'exited', + exitCode: 0, + stdout: '', + stderr: '', + timedOut: false, + signal: null, + killed: false, + code: null, + }; + } + + test('ensureHooksDist runs the build when the dist is stale', () => { + const originalRunNode = processSeam.runNode; + let calls = 0; + processSeam.runNode = () => { + calls += 1; + return fakeSuccessfulRun(); + }; + try { + withFakeDistState({ stale: true }, () => { + ensureHooksDist(); + }); + assert.strictEqual(calls, 1); + } finally { + processSeam.runNode = originalRunNode; + } + }); + + // Deliverable — the discriminating case: a rewiring regression that drops + // (or bypasses) the `isHooksDistStale` guard and unconditionally invokes + // the build seam would pass the STALE test above but FAIL this one, since + // it would call the seam even against a fresh, fully-populated dist. The + // STALE test alone cannot catch that shape of revert; this one does. + test('ensureHooksDist does not run the build when the dist is complete', () => { + const originalRunNode = processSeam.runNode; + let calls = 0; + processSeam.runNode = () => { + calls += 1; + return fakeSuccessfulRun(); + }; + try { + withFakeDistState({ stale: false }, () => { + ensureHooksDist(); + }); + assert.strictEqual(calls, 0); + } finally { + processSeam.runNode = originalRunNode; + } + }); +}); + +// ── Group 3: skipped-leaf warning text ────────────────────────────────────── + +describe('buildOverlayRepo: skipped-leaf warning', () => { + test('fires the build:hooks warning when a leaf vanishes mid-walk', () => { + // `opts.warn` (defaulting to console.warn) is a trivially injectable seam + // for this — no TOCTOU race or source-grep needed. Force a single real + // top-level source file (package.json) to appear to have genuinely + // vanished from the source tree: `fs.linkSync` ENOENTs for it on both the + // attempt and the retry, and `fs.existsSync` reports it as gone at the + // re-check, so `placeVanishableLeaf` reaches the tolerated "vanished mid- + // walk" branch and `buildOverlayRepo` records it in `skipped`. + // + // Monkeypatching `fs.linkSync`/`fs.existsSync` process-wide here is safe + // only because this file's tests run sequentially (node:test's default + // for this repo, never `{ concurrency: true }`) — see the same caveat on + // `withFakeDistState` above. Concurrency would let another in-flight test + // in this process observe the faked path. + const REPO_ROOT_PKG = path.join(__dirname, '..', 'package.json'); + const originalExistsSync = fs.existsSync; + const originalLinkSync = fs.linkSync; + fs.existsSync = (p) => (p === REPO_ROOT_PKG ? false : originalExistsSync(p)); + fs.linkSync = (src, dest) => { + if (src === REPO_ROOT_PKG) { + const err = new Error('ENOENT: no such file or directory'); + err.code = 'ENOENT'; + throw err; + } + return originalLinkSync(src, dest); + }; + + const warnCalls = []; + let overlayPath; + try { + overlayPath = buildOverlayRepo({}, { warn: (msg) => warnCalls.push(msg) }); + } finally { + fs.existsSync = originalExistsSync; + fs.linkSync = originalLinkSync; + if (overlayPath) cleanup(overlayPath); + } + assert.strictEqual(warnCalls.length, 1); + assert.match(warnCalls[0], /npm run build:hooks/); + assert.match(warnCalls[0], /package\.json/); + }); + + test('does not warn on a clean walk', () => { + const warnCalls = []; + const overlayPath = buildOverlayRepo( + { 'CONTRIBUTING.md': 'fixture override content\n' }, + { warn: (msg) => warnCalls.push(msg) }, + ); + try { + assert.strictEqual(warnCalls.length, 0); + } finally { + cleanup(overlayPath); + } + }); +}); + +// ── Group 4: property coverage ────────────────────────────────────────────── + +describe('placeVanishableLeaf: property coverage', () => { + // Monkeypatches fs.existsSync process-wide for the duration of each fc run + // below — safe only because this file's tests execute sequentially; see + // the same caveat on withFakeDistState in Group 2b above. + test('property: an ENOENT is tolerated only when the source is confirmed gone', () => { + fc.assert( + fc.property( + // vanishCount === 0 never consults existsSync at all (the first + // attempt succeeds outright), so presentAtFinalAttempt can never + // change that case's outcome — the filter below drops the redundant + // half of those samples (both flag values reduce to one behavior) + // instead of letting them silently pad the run count. For every + // vanishCount >= 1, existsSync IS consulted and the flag is wired + // to change an observable outcome below (either `placed`, or — + // for vanishCount >= 2, where `placed` is false either way — the + // number of `attempt` calls), so no other combination is dropped. + fc.tuple(fc.integer({ min: 0, max: 3 }), fc.boolean()).filter( + ([vanishCount, presentAtFinalAttempt]) => vanishCount !== 0 || presentAtFinalAttempt === true, + ), + ([vanishCount, presentAtFinalAttempt]) => { + const srcPath = path.join(os.tmpdir(), `gsd-3108-fc-fixture-${vanishCount}-${presentAtFinalAttempt}`); + let calls = 0; + // Throws ENOENT on every call up to and including `vanishCount`, + // succeeds thereafter. placeVanishableLeaf only ever calls + // `attempt` twice (first try + one retry), so only calls 1 and 2 + // are ever observed. + const attempt = () => { + calls += 1; + if (calls <= vanishCount) { + const err = new Error('ENOENT: no such file or directory'); + err.code = 'ENOENT'; + throw err; + } + }; + // existsSync is consulted only when the first attempt threw + // (vanishCount >= 1), gating whether the retry is even attempted. + // For vanishCount === 1 it decides whether the retry succeeds. + // For vanishCount >= 2 the retry ALSO throws ENOENT, and existsSync + // is consulted a SECOND time (post-fix) to decide whether that is a + // genuinely vanished source (tolerated, placed=false) or a dest-side + // ENOENT masquerading behind a source that is actually still there + // (must propagate, since only a confirmed-absent source is ever + // tolerated — see #3108's dest-side-ENOENT fix). + const existsAnswer = presentAtFinalAttempt; + const originalExistsSync = fs.existsSync; + fs.existsSync = (p) => (p === srcPath ? existsAnswer : originalExistsSync(p)); + try { + if (vanishCount === 0) { + const placed = placeVanishableLeaf(srcPath, attempt); + assert.strictEqual(placed, true); + assert.strictEqual(calls, 1); + } else if (vanishCount === 1 && presentAtFinalAttempt) { + const placed = placeVanishableLeaf(srcPath, attempt); + assert.strictEqual(placed, true); + assert.strictEqual(calls, 2); + } else if (vanishCount === 1 && !presentAtFinalAttempt) { + // Gone for good before the retry is ever attempted. + const placed = placeVanishableLeaf(srcPath, attempt); + assert.strictEqual(placed, false); + assert.strictEqual(calls, 1); + } else if (presentAtFinalAttempt) { + // vanishCount >= 2: the retry is attempted (existsSync said + // present at the first check) and throws ENOENT again. With no + // destPath supplied, the retry's ENOENT is tolerated + // unconditionally (source existence is not re-consulted for + // the retry outcome — see the doc comment on + // placeVanishableLeaf): this is the double-vanish race the + // function exists to tolerate, so it is skipped, not thrown. + const placed = placeVanishableLeaf(srcPath, attempt); + assert.strictEqual(placed, false); + assert.strictEqual(calls, 2); + } else { + // vanishCount >= 2 && !presentAtFinalAttempt: existsSync + // already reports the source gone on the FIRST check, so the + // retry is never even attempted. + const placed = placeVanishableLeaf(srcPath, attempt); + assert.strictEqual(placed, false); + assert.strictEqual(calls, 1); + } + } finally { + fs.existsSync = originalExistsSync; + } + }, + ), + { seed: 3108, numRuns: 200 }, + ); + }); +});