From 1178c5f9950d85ca6f2df7f94f7354492469cd2a Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Sat, 22 Aug 2026 23:04:44 -0400 Subject: [PATCH] test(#3108): make the overlay ENOENT tolerance and hooks/dist readiness check honest (#3772) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * test(#3108): failing-first suite for the overlay vanish-retry and hooks/dist staleness RED by construction, and deliberately narrower than the issue. #3108 reports a bare `ENOENT ... link '/work/hooks/dist/gsd-session-state.sh'` and attributes it to hooks/dist never having been built. That mechanism cannot produce that error: buildOverlayRepo enumerates with readdirSync and links what it enumerated, so a directory that never existed yields no names and no link is ever attempted. The error requires the file to have existed at readdir and vanished before the link -- which is the atomic-replace race placeVanishableLeaf was written for in #3285, three days AFTER this issue was filed. What is still genuinely broken, and what these tests bind to: placeVanishableLeaf's retry is unguarded. On ENOENT it re-checks existsSync and calls attempt() once more, bare. A second atomic replace inside that window throws an unhandled ENOENT of exactly the reported shape. Rows 1/2/3/5/6/7 fence the surrounding contract -- most already pass, which is the point: they are what stops the fix from widening into "swallow every error". Row 6 in particular covers a non-ENOENT on the RETRY, the exact path the new code will live on. ensureHooksDist's staleness predicate is extension-blind. It rebuilds only when hooks/dist is absent or holds zero .js files, while build-hooks.js also ships .sh -- including gsd-session-state.sh, the very file in the report. A dist with .js present and every .sh missing reads as populated and the rebuild is skipped. Rows 10 and 11 are mirrored on purpose: asserting only the .sh direction would permit swapping one extension heuristic for another, so both directions force the predicate to be about the expected set (HOOKS_TO_COPY, which build-hooks.js already exports) rather than about counting an extension. Rows 12 and 13 are cost guards. ensureHooksDist runs per suite and its rebuild is a real subprocess, so a predicate that over-triggers turns a correctness fix into a throughput regression nobody attributes to it; and hooks/dist legitimately carries files the expected list does not name, so an exact-set match would rebuild forever. The warning-text row is t.skip()'d rather than faked: the only ways to assert it were a source-grep (banned by local/no-source-grep) or a full overlay build, and a test that cannot be written honestly is better skipped visibly than written vacuously. Filename note: this started as fix-3108-*.test.cjs and tripped lint-regression-test-names, which bans new fix/bug/issue-NNNN files, then as install-overlay-helpers.test.cjs and tripped lint-test-file-count, whose `install` bucket is already at its limit. Module-named under the overlay bucket satisfies both. The allowlists were left untouched -- both are empty, so nothing here is grandfathered and adding an entry would have been the wrong instinct. * fix(#3108): guard the overlay retry and make the hooks/dist check see .sh Two holes, both reachable from the failure #3108 reports, neither of them the cause it names. placeVanishableLeaf's retry was bare. On ENOENT it re-checked existsSync and called attempt() once more with no catch, so a second atomic replace landing inside that window threw an unhandled ENOENT -- exactly the reported `ENOENT ... link '/work/hooks/dist/gsd-session-state.sh'`. Two vanishes inside the window means the same thing one does: the path is going away and is not part of the snapshot. It now returns false and skips the leaf, reaching the conclusion the single-vanish case already reached. Still ONE retry. No loop, no backoff, no sleep -- the existing comment argues that a timing-based wait here would be the flake rather than the fix, and that reasoning did not change. Non-ENOENT still propagates from either attempt, which is the invariant a careless widening would eat; the suite pins it on the retry path specifically, because a fix that guarded only the first attempt would look right and be wrong. ensureHooksDist could not see the file class that caused the report. It rebuilt only when hooks/dist was absent or held zero .js files, while build-hooks.js also ships .sh -- including gsd-session-state.sh itself. A dist with .js present and every .sh missing read as populated and the rebuild was skipped. The predicate is now membership against build-hooks.js's own exported HOOKS_TO_COPY, so it asks "is everything expected present" instead of counting an extension, and it cannot be blind to a file class again. It is extracted as isHooksDistStale(dir) with ensureHooksDist calling it, so there is one predicate rather than two that can drift. Extra unexpected entries are explicitly not stale -- hooks/dist legitimately accumulates subdirectory and hooks/lib output, and an exact-set match would rebuild forever. One readdirSync into a Set, no per-entry existsSync, no stat: it runs per suite and its rebuild is a real subprocess, so an over-triggering predicate would turn this into a throughput regression nobody would attribute to it. The skipped-leaf warning now names `npm run build:hooks`. It already named the cause; a reader still had to know what produces that directory. Deliberately NOT done: nothing here makes an absent hooks/dist fail. Absence is a legitimate package shape that bin/install.js:11191 treats as "nothing to verify", and six of the seven install suites never read the directory at all. * fix(#3108): count hooks/dist subdirectories, and never throw out of the predicate Two gaps found reviewing the predicate I had just written. It ignored HOOKS_SUBDIRS_TO_COPY. That is ["lib"], and hooks/dist/lib carries gsd-graphify-rebuild.sh, so a dist with all 27 top-level files but no lib/ read as populated. That is precisely the blindness the .js-count heuristic had, one level down: a whole file class invisible to the check. Fixing the extension case and leaving the subdirectory case would have been half a fix, and the half left behind is the one nobody would look at again. Subdir names are bare (no slashes), so they slot into the same top-level readdir Set — no second readdirSync, no stat. Whether lib is really a directory is not checked; that would cost a stat per entry and buys nothing, since the build owns that. It could also throw. existsSync passing does not make readdirSync safe: the path may be a regular file (ENOTDIR), unreadable (EACCES), or retired in the race between the two calls. This helper runs in every install suite's before(), so an unhandled throw there fails a suite on a condition it cannot act on. Unreadable is indistinguishable from unusable for this question, and rebuilding is idempotent, so it now reports stale instead. Both are the same shape as the original bug and the same shape as each other: a guard that answers "is this ready" must not have a blind spot or a hard edge, because every caller treats a false negative as "carry on". Two existing tests asserted a "complete" dist without lib and had to be corrected to keep meaning what their names claim, rather than being left passing against a definition of complete that no longer holds. * test(#3108): close the review findings, including a half-closed subdir check An isolated correctness reviewer found no blockers and four real gaps. The wiring was untested. Every Group-2 test exercised the pure predicate; none called ensureHooksDist. So restoring the old inline .js-count check INSIDE ensureHooksDist -- keeping isHooksDistStale exported and correct -- left the whole suite green, and that wiring is the actual #3108 defect. Two tests now drive ensureHooksDist itself through the process seam: build invoked exactly once when stale, never when fresh. The second is the one a permissive revert fails. Reaching that seam meant requiring process-seam as a module object rather than destructuring runNode, so a test can replace it in place. That is a testability affordance in a test helper, not a production change, and it is commented as such so it does not read as an accident later. The subdir check was only half closed, and the half left open was the important one. It required `lib` to be PRESENT in the top-level readdir, never looked inside -- so an EMPTY dist/lib, missing gsd-graphify-rebuild.sh, still read as populated. That is precisely the missing-file-class case the subdir check was added to catch, which made the fix a gesture at the problem rather than a fix. Each subdir entry must now be a readable, NON-EMPTY directory. A stray regular file named `lib` throws ENOTDIR into the same try/catch and reads stale too. Cost stayed honest: one extra readdirSync total (there is exactly one subdir entry), no stat, no per-expected-file syscall. Probed against the real hooks/dist -- still reports fresh, so no suite gains a rebuild. Two nits, both real: the error-code sweep re-tested EACCES already covered standalone, and the property ignored presentAtFinalAttempt whenever vanishCount was not 1, making roughly half the 200 runs duplicates. The flag now varies meaningfully across the whole range and the assertions depend on it. One reviewer finding was already stale: the subdir and ENOTDIR work was uncommitted when the reviewer snapshotted the tree, and had landed in a67aefb9c before the report arrived. Verified rather than assumed. * fix(#3108): stop the vanish tolerance from swallowing a dest-side ENOENT A defect this PR introduced, caught by an isolated security reviewer. linkSync(src, dest) throws ENOENT for the DESTINATION path too, not only for a vanished source. The widened retry caught that, saw the source still present, retried, got the same dest-side ENOENT, and returned false -- recording the leaf as "vanished mid-walk" and printing a warning that tells the reader to run `npm run build:hooks`. A remedy with nothing to do with the actual cause, an overlay quietly short a file, and the install under test proceeding against an incomplete tree. It also falsified the function's own documented invariant, which says in as many words: "Returns false only when the path left the source tree entirely." Widening the tolerance without re-reading the sentence above it is how that happens. The retry now re-checks existsSync(srcPath) before tolerating: source still present means the ENOENT was about something else and it propagates untouched. Chose the existsSync re-check over comparing retryErr.path to srcPath -- err.path normalization is not guaranteed across platforms, and a path-equality test is a subtler thing to get wrong later. The FIRST catch was probed and is already correct: for a dest-side ENOENT the source is present, so it falls through to the retry rather than returning false. Left unchanged rather than "fixed" symmetrically. Two regression pins, deliberately opposed: a dest-side ENOENT with the source present must THROW, and a genuinely absent source must still return false. The second exists because the obvious over-correction -- always rethrow on the retry -- passes the first and silently undoes what this PR set out to fix. Also closed the skipped placeholder. It claimed no non-flaky seam existed for asserting the warning text; the reviewer pointed out an injectable `warn` param is trivial, and they were right. buildOverlayRepo now takes opts.warn defaulting to console.warn (byte-identical for every existing caller) and the skip is replaced by real tests: fires with the remedy named on a skipped leaf, silent on a clean walk. "No seam exists" was a design choice presented as a constraint. Recorded the sequential-only constraint at the two sites that monkeypatch fs process-wide: adding { concurrency: true } to this file would cross-contaminate every other suite in the process. Better written down than rediscovered. Known limit, disclosed rather than fixed here: a legitimately dropped leaf can still pass vacuously downstream -- agent-fragments-emission asserts a negative over filesContaining, and mcp-catalog-parity has only an anti-vacuity floor of one. That is a pre-existing property of those suites and the tolerance #3285 already chose; this change narrows which drops are possible rather than adding the completeness assertion those suites lack. * fix(#3108): discriminate ENOENT by the dest parent, not by re-checking the source The previous commit's dest-side guard was wrong, and the remote run said so: "a leaf that vanishes again during the retry is skipped, not a bare ENOENT" Got unwanted exception. Actual message: "ENOENT: no such file or directory" That test was right and the guard was wrong. It rethrew when existsSync(srcPath) was still true, on the theory that a present source means the ENOENT was about the destination. But in the genuine race the source is being atomically REPLACED, so it is legitimately present again at the re-check while the ENOENT was entirely source-side. The gate therefore threw on precisely the race #3285 exists to tolerate -- trading one misclassification for a worse one, since the old bug was a bare crash and the new one broke the working tolerance. The security reviewer's alternative discriminator does not work either, and a probe settles it. Node populates BOTH `path` and `dest` on a link ENOENT, and `err.path` is the SOURCE in both directions: linkSync(existingSrc, missingDir/a.txt) -> ENOENT path= dest= linkSync(missingSrc, validDest) -> ENOENT path= dest= So the error object cannot tell you which side failed. What CAN: the dest parent. buildOverlayRepo builds its own dest tree -- place() mkdirSync's recursively into a private mkdtempSync root no other process touches -- so a missing dest parent is always a bug (Windows MAX_PATH, a concurrent cleanup, a bad dest), never the replace race. A present dest parent means the ENOENT was about the source, which is the case we tolerate. placeVanishableLeaf therefore takes an optional destPath and uses the dest parent as the sole discriminator when it has one; with no destPath it behaves exactly as before. linkOrCopyFile and the copy-mode call site both pass it, because those are the two places that actually know the destination. The doc comment now records BOTH failed discriminators and why each fails -- existsSync because the source is legitimately replaced mid-race, err.path because it names the source either way. Those are the two things a future reader reaches for first, and both look correct until they are not. The dest-side regression pin was rewritten to drive the real mechanism: a real temp source and a dest whose parent does not exist, through linkOrCopyFile. Previously it forced a throw through a present source, which is what encoded the wrong theory into a test and made it look verified. --------- Co-authored-by: sim --- tests/helpers/hooks-dist.cjs | 101 +++- tests/helpers/overlay-repo.cjs | 125 ++++- tests/overlay-repo-helpers.test.cjs | 694 ++++++++++++++++++++++++++++ 3 files changed, 884 insertions(+), 36 deletions(-) create mode 100644 tests/overlay-repo-helpers.test.cjs 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 }, + ); + }); +});