fix(test-cache): close the false skips the second review found

Re-review of fbe832cbdc confirmed three more false skips and one breakage:

- A dependency deleted or renamed mid-run was recorded as `missing`, which
  the next run matched. changedSince now asks the parent directory, whose
  mtime moves on a removal, and a module that was in the graph but is gone
  at the end refuses the record outright.
- `createRequire(...)('child_process')`, `process.getBuiltinModule(...)` and
  `from"node:child_process"` walked past the import-syntax patterns. The
  module name is now matched as a string anywhere, plus the common spawn
  wrappers. The graph-level builtin check is deleted: builtins never enter
  vite's graph, so it could not fire.
- The tracker's wrappers dropped properties living on the function, so
  `fs.realpathSync.native` (called by next off-Windows) vanished with the
  cache on. Own properties are copied and `.native` is wrapped.
- An unhandled error now blocks only the file vitest attributes it to
  (VITEST_TEST_PATH, verified present on a leaked rejection); an
  unattributed one still blocks the run. The key is taken before the change
  check, closing the window between them. A TRIPPED rename that EPERMs
  writes in place instead of aborting before records are forgotten.

The reporter and sequencer now have their own tests, driven with fake
vitest objects (scripts/__tests__/test-cache-reporter.test.ts), led by a
positive control that an ordinary file IS recorded. 13 revert controls,
each red on its own named test. The changedSince test now actually moves
one input past the run start per case, and covers deletion.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

scripts/__tests__/test-cache-core.test.ts

@@ -1,4 +1,4 @@
-import { mkdirSync, mkdtempSync, writeFileSync } from 'fs';
+import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'fs';
 import { tmpdir } from 'os';
 import { join } from 'path';
 import { describe, expect, it } from 'vitest';
@@ -189,15 +189,50 @@ describe('what a key must see besides content', () => {
 
   // The key is computed at the END of a run; an input written after the run started is not what
   // ran. Measured by review: without this an edit made during a queued suite was recorded as passing.
-  it('sees a file written after a given instant, and a directory whose subtree gained a file', () => {
-    const root = mkdtempSync(join(tmpdir(), 'test-cache-mtime-'));
-    mkdirSync(join(root, 'd/deep'), { recursive: true });
-    writeFileSync(join(root, 'a.ts'), 'a');
-    const since = Date.now() + 60_000;
-    expect(changedSince(root, 'a.ts', since)).toBe(false);
-    expect(changedSince(root, 'a.ts', 0)).toBe(true);
-    expect(changedSince(root, 'd', since)).toBe(false);
-    expect(changedSince(root, 'missing.ts', 0)).toBe(false);
+  describe('what changed during a run', () => {
+    // The tree is built, THEN the "run" starts, so each assertion below moves exactly one thing
+    // past `since`. Not backdated with utimes: that bumps ctime to now, and ctime is checked on
+    // purpose — it is what catches an edit made with a backdated mtime.
+    const pause = (ms: number) => Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, ms);
+    const setup = () => {
+      const root = mkdtempSync(join(tmpdir(), 'test-cache-mtime-'));
+      mkdirSync(join(root, 'd/deep'), { recursive: true });
+      mkdirSync(join(root, 'h'), { recursive: true });
+      writeFileSync(join(root, 'a.ts'), 'a');
+      writeFileSync(join(root, 'h/gone.ts'), 'g');
+      pause(50);
+      const since = Date.now();
+      pause(50);
+      return { root, since };
+    };
+
+    it('reports nothing for inputs untouched since the run started', () => {
+      const { root, since } = setup();
+      expect(changedSince(root, 'a.ts', since)).toBe(false);
+      expect(changedSince(root, 'd', since)).toBe(false);
+      expect(changedSince(root, 'h/gone.ts', since)).toBe(false);
+    });
+
+    it('sees a file edited after the run started', () => {
+      const { root, since } = setup();
+      writeFileSync(join(root, 'a.ts'), 'a2');
+      expect(changedSince(root, 'a.ts', since)).toBe(true);
+    });
+
+    // A convention guard lists a directory recursively; a file added three levels down counts.
+    it('sees a directory whose subtree gained a file', () => {
+      const { root, since } = setup();
+      writeFileSync(join(root, 'd/deep/new.ts'), 'n');
+      expect(changedSince(root, 'd', since)).toBe(true);
+    });
+
+    // The test ran WITH the file. Recording its absence would skip it green next time — confirmed
+    // by review as a false skip before this existed.
+    it('sees a file deleted after the run started', () => {
+      const { root, since } = setup();
+      rmSync(join(root, 'h/gone.ts'));
+      expect(changedSince(root, 'h/gone.ts', since)).toBe(true);
+    });
   });
 
   // A half-written marker must read as tripped, never as "safe to skip".

scripts/__tests__/test-cache-reporter.test.ts

@@ -0,0 +1,256 @@
+import { existsSync, mkdirSync, mkdtempSync, readdirSync, readFileSync, writeFileSync } from 'fs';
+import { tmpdir } from 'os';
+import { join } from 'path';
+import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
+
+import TestCacheReporter from '../test-cache/reporter.mjs';
+import TestCacheSequencer from '../test-cache/sequencer.mjs';
+
+/**
+ * The reporter and sequencer driven with fake vitest objects, so each guard can be reverted and
+ * seen to fail. Every case below was a false skip, or a hole in the check that catches one, found
+ * by adversarial review of this cache and reproduced on a fixture before it was fixed.
+ */
+
+type Node = { id: string; importedModules: Set<Node> };
+type State = {
+  hits: Set<string>;
+  sampled: Set<string>;
+  skipped: string[];
+  startedAt: number;
+  bailed?: string | null;
+};
+
+const fwd = (p: string) => p.replace(/\\/g, '/');
+let root: string;
+let cacheDir: string;
+
+function graphOf(edges: Record<string, string[]>) {
+  const nodes = new Map<string, Node>();
+  const node = (id: string) => {
+    if (!nodes.has(id)) nodes.set(id, { id, importedModules: new Set() });
+    return nodes.get(id)!;
+  };
+  for (const [from, tos] of Object.entries(edges)) {
+    node(from);
+    for (const to of tos) node(from).importedModules.add(node(to));
+  }
+  return { getModuleById: (id: string) => nodes.get(id) };
+}
+
+function write(rel: string, content = rel) {
+  mkdirSync(join(root, rel, '..'), { recursive: true });
+  writeFileSync(join(root, rel), content);
+}
+
+function testModule(
+  rel: string,
+  { passed = true, deps = [] as string[], reads = [] as string[], setupFiles = [] as string[] } = {}
+) {
+  const abs = fwd(join(root, rel));
+  const graph = graphOf({ [abs]: deps.map((d) => fwd(join(root, d))) });
+  return {
+    moduleId: abs,
+    project: {
+      name: 'unit',
+      config: { setupFiles: setupFiles.map((s) => fwd(join(root, s))) },
+      vite: { environments: { ssr: { moduleGraph: graph } } },
+    },
+    meta: () => ({ testCacheReads: reads.map((r) => join(root, r)) }),
+    diagnostic: () => ({}),
+    state: () => (passed ? 'passed' : 'failed'),
+    children: {
+      *allTests() {
+        yield { result: () => ({ state: passed ? 'passed' : 'failed' }) };
+      },
+    },
+  };
+}
+
+function run(
+  modules: ReturnType<typeof testModule>[],
+  { errors = [] as unknown[], state = {} as Partial<State> } = {}
+) {
+  (globalThis as { __civitaiTestCache?: State }).__civitaiTestCache = {
+    hits: new Set(),
+    sampled: new Set(),
+    skipped: [],
+    // Well in the future, so nothing counts as changed during the run unless a case says so.
+    startedAt: Date.now() + 60_000,
+    ...state,
+  };
+  const reporter = new TestCacheReporter();
+  reporter.onInit({ config: { root }, version: 'test' } as never);
+  reporter.onTestRunEnd(modules as never, errors as never, 'passed' as never);
+}
+
+/** How many stored records name `rel` as their test file's own entry. */
+function records(rel: string) {
+  const recDir = join(cacheDir, 'rec');
+  if (!existsSync(recDir)) return 0;
+  let n = 0;
+  for (const d of readdirSync(recDir)) {
+    for (const f of readdirSync(join(recDir, d))) {
+      const body = JSON.parse(readFileSync(join(recDir, d, f), 'utf8'));
+      if (body.entries.includes(rel)) n += 1;
+    }
+  }
+  return n;
+}
+
+beforeEach(() => {
+  root = mkdtempSync(join(tmpdir(), 'test-cache-root-'));
+  cacheDir = mkdtempSync(join(tmpdir(), 'test-cache-dir-'));
+  vi.stubEnv('CIVITAI_TEST_CACHE', 'on');
+  vi.stubEnv('CIVITAI_TEST_CACHE_DIR', cacheDir);
+  vi.stubEnv('CI', '');
+  vi.spyOn(console, 'error').mockImplementation(() => undefined);
+  vi.spyOn(console, 'log').mockImplementation(() => undefined);
+});
+
+afterEach(() => {
+  vi.unstubAllEnvs();
+  vi.restoreAllMocks();
+  delete (globalThis as { __civitaiTestCache?: State }).__civitaiTestCache;
+});
+
+describe('recording a pass', () => {
+  // The positive control every case below leans on: without it, "not recorded" passes vacuously —
+  // which is exactly how an earlier round of these fixes certified itself while recording nothing.
+  it('records an ordinary passing file', () => {
+    write('a.test.ts');
+    write('dep.ts');
+    run([testModule('a.test.ts', { deps: ['dep.ts'] })]);
+    expect(records('a.test.ts')).toBe(1);
+  });
+
+  it('refuses a file whose input changed after the run started', () => {
+    write('a.test.ts');
+    write('dep.ts');
+    run([testModule('a.test.ts', { deps: ['dep.ts'] })], { state: { startedAt: 0 } });
+    expect(records('a.test.ts')).toBe(0);
+  });
+
+  // The test ran WITH the module; recording its absence would skip it green next time.
+  it('refuses a file whose imported module is gone by the end of the run', () => {
+    write('a.test.ts');
+    run([testModule('a.test.ts', { deps: ['deleted.ts'] })]);
+    expect(records('a.test.ts')).toBe(0);
+  });
+
+  it('refuses a file whose HELPER makes a computed import', () => {
+    write('a.test.ts');
+    write('helper.ts', 'export const load = (f: string) => import(/* @vite-ignore */ f);');
+    run([testModule('a.test.ts', { deps: ['helper.ts'] })]);
+    expect(records('a.test.ts')).toBe(0);
+  });
+
+  it('refuses a file whose helper reaches child_process by any syntax', () => {
+    write('a.test.ts');
+    write('helper.ts', "const cp = process.getBuiltinModule('node:child_process');");
+    run([testModule('a.test.ts', { deps: ['helper.ts'] })]);
+    expect(records('a.test.ts')).toBe(0);
+  });
+});
+
+describe('unhandled errors', () => {
+  it('blocks only the file the error is attributed to', () => {
+    write('a.test.ts');
+    write('b.test.ts');
+    run([testModule('a.test.ts'), testModule('b.test.ts')], {
+      errors: [{ VITEST_TEST_PATH: fwd(join(root, 'a.test.ts')) }],
+    });
+    expect(records('a.test.ts')).toBe(0);
+    expect(records('b.test.ts')).toBe(1);
+  });
+
+  it('blocks every file when an error cannot be attributed', () => {
+    write('a.test.ts');
+    write('b.test.ts');
+    run([testModule('a.test.ts'), testModule('b.test.ts')], { errors: [{ message: 'somewhere' }] });
+    expect(records('a.test.ts')).toBe(0);
+    expect(records('b.test.ts')).toBe(0);
+  });
+});
+
+describe('setup files', () => {
+  // The fs tracker is instrumentation whose own code imports child_process. Walking it marked every
+  // test uncacheable; it is covered by the salt instead.
+  it('does not let the cache tracker itself make a file uncacheable', () => {
+    write('a.test.ts');
+    run([testModule('a.test.ts', { setupFiles: ['scripts/test-cache/fs-tracker.mjs'] })]);
+    expect(records('a.test.ts')).toBe(1);
+  });
+
+  it('refuses a file whose ordinary setup file is missing from the graph', () => {
+    write('a.test.ts');
+    run([testModule('a.test.ts', { setupFiles: ['src/__tests__/setup.ts'] })]);
+    expect(records('a.test.ts')).toBe(0);
+  });
+});
+
+describe('a false skip', () => {
+  it('trips the cache and forgets the file, so clearing the trip cannot revive the skip', () => {
+    write('a.test.ts');
+    run([testModule('a.test.ts')]);
+    expect(records('a.test.ts')).toBe(1);
+
+    const hit = `unit\0${fwd(join(root, 'a.test.ts'))}`;
+    run([testModule('a.test.ts', { passed: false })], { state: { hits: new Set([hit]) } });
+
+    expect(existsSync(join(cacheDir, 'TRIPPED.json'))).toBe(true);
+    expect(records('a.test.ts')).toBe(0);
+  });
+
+  // stdout is reserved for a caller's own `--reporter=json`.
+  it('says everything on stderr, nothing on stdout', () => {
+    write('a.test.ts');
+    run([testModule('a.test.ts')]);
+    expect(console.error).toHaveBeenCalled();
+    expect(console.log).not.toHaveBeenCalled();
+  });
+});
+
+describe('when the sequencer declines to skip', () => {
+  const bailFor = async (ctx: Record<string, unknown>) => {
+    const seq = new TestCacheSequencer({ config: {}, reporters: [], ...ctx } as never);
+    await seq.sort([]);
+    return (globalThis as { __civitaiTestCache?: State }).__civitaiTestCache?.bailed;
+  };
+  const withReporter = [new TestCacheReporter()];
+
+  it('declines on a name filter', async () => {
+    expect(await bailFor({ config: { testNamePattern: /x/ }, reporters: withReporter })).toBe(
+      'name filter'
+    );
+  });
+
+  it('declines when the run names files', async () => {
+    expect(await bailFor({ filenamePattern: ['a.test.ts'], reporters: withReporter })).toBe(
+      'file filter'
+    );
+  });
+
+  // Nothing would record or check the sample, so a skip would be unobserved.
+  it('declines when the cache reporter is not loaded', async () => {
+    expect(await bailFor({ reporters: [] })).toBe('cache reporter not loaded');
+  });
+
+  it('does not decline a whole-suite run with the reporter loaded', async () => {
+    expect(
+      await bailFor({ config: { root }, reporters: withReporter, version: 'test' })
+    ).toBeNull();
+  });
+});
+
+describe('the fs tracker', () => {
+  // Wrapping a function drops properties that live ON it. `fs.realpathSync.native` is one, and
+  // next's lib/realpath.js calls it off-Windows — review confirmed the tracker stripped it.
+  it('keeps realpathSync.native callable after wrapping fs', async () => {
+    vi.stubEnv('CIVITAI_TEST_CACHE', 'on');
+    await import('../test-cache/fs-tracker.mjs');
+    const fs = await import('node:fs');
+    expect(typeof fs.realpathSync.native).toBe('function');
+    expect(fs.realpathSync.native(root)).toBeTruthy();
+  });
+});

scripts/test-cache/core.mjs

@@ -55,8 +55,13 @@ const ALWAYS_RUN_SOURCE = [
   // IMPORTING a process or thread module, not calling one by name: a call pattern cannot see
   // `cp.execSync(` or a destructured alias, and `exec(` matched every `regex.exec(` in the repo —
   // measured: src/__tests__/setup.ts tripped it and nothing was cacheable at all.
-  /\bfrom\s+['"](?:node:)?(?:child_process|worker_threads|cluster)['"]/,
-  /\b(?:require|import)\s*\(\s*['"](?:node:)?(?:child_process|worker_threads|cluster)['"]\s*\)/,
+  // The module NAME anywhere as a string, not a particular import syntax: review found
+  // `createRequire(...)('child_process')`, `process.getBuiltinModule('node:child_process')` and
+  // `from"node:child_process"` (no space) all walking past syntax-specific patterns.
+  /['"`](?:node:)?(?:child_process|worker_threads|cluster)['"`]/,
+  // Wrappers that spawn for you. None is a direct dependency today; this keeps one from arriving
+  // unnoticed, since node_modules is never scanned.
+  /['"`](?:execa|cross-spawn|tinyexec|nano-spawn|zx)['"`]/,
   // Comments allowed between `import(` and the specifier: `import(/* @vite-ignore */ file)` is the
   // form this repo actually uses, and the first version of this pattern let it through.
   /import\(\s*(?:\/\*[\s\S]*?\*\/\s*)*(?:`[^`]*\$\{|[A-Za-z_$])/,
@@ -98,18 +103,6 @@ export function alwaysRuns(source) {
   return ALWAYS_RUN_SOURCE.some((re) => re.test(source));
 }
 
-// Reaching one of these anywhere in the graph means a process or thread the key cannot see into.
-// Checked on the graph rather than by pattern, so an aliased or destructured call cannot hide it.
-const OPAQUE_BUILTINS = new Set(['child_process', 'worker_threads', 'cluster']);
-
-export function reachesOpaqueBuiltin(ids) {
-  for (const id of ids) {
-    const bare = String(id).replace(/^node:/, '');
-    if (OPAQUE_BUILTINS.has(bare)) return true;
-  }
-  return false;
-}
-
 const RESOLVABLE_EXTS = ['ts', 'tsx', 'mts', 'cts', 'js', 'jsx', 'mjs', 'cjs', 'json'];
 
 /**
@@ -249,7 +242,16 @@ export function changedSince(root, rel, sinceMs) {
   try {
     st = statSync(abs);
   } catch {
-    return false; // absent now; its absence is what the key records
+    // Absent now. If it was deleted or renamed DURING the run, the test ran with it and the key
+    // would record its absence — review confirmed that as a false skip. Removing a file moves its
+    // directory's mtime, so ask the directory.
+    const parent = rel.includes('/') ? rel.slice(0, rel.lastIndexOf('/')) : '.';
+    try {
+      const p = statSync(join(root, parent));
+      return p.mtimeMs >= sinceMs || p.ctimeMs >= sinceMs;
+    } catch {
+      return true;
+    }
   }
   if (st.mtimeMs >= sinceMs || st.ctimeMs >= sinceMs) return true;
   if (!st.isDirectory()) return false;
@@ -326,7 +328,14 @@ export function writeTripped(dir, body) {
   const p = trippedPath(dir);
   const tmp = `${p}.${process.pid}.tmp`;
   writeFileSync(tmp, JSON.stringify(body, null, 2));
-  renameSync(tmp, p);
+  try {
+    renameSync(tmp, p);
+  } catch {
+    // Renaming over an existing marker can EPERM on Windows. Already tripped is still tripped;
+    // write in place rather than abort before the offending records are forgotten.
+    writeFileSync(p, JSON.stringify(body, null, 2));
+    rmSync(tmp, { force: true });
+  }
 }
 
 /** Every record of a file — used on a false skip, so clearing TRIPPED cannot revive the same skip. */

scripts/test-cache/fs-tracker.mjs

@@ -32,10 +32,21 @@ if (mode() !== 'off') {
   const wrap = (obj, name) => {
     const orig = obj[name];
     if (typeof orig !== 'function') return;
-    obj[name] = function (p, ...rest) {
+    const wrapper = function (p, ...rest) {
       note(p);
       return orig.call(this, p, ...rest);
     };
+    // Carry the original's own properties: `fs.realpathSync.native` lives on the function, and
+    // next's lib/realpath.js calls it off-Windows. Dropping it broke that path with the cache on.
+    Object.assign(wrapper, orig);
+    if (typeof orig.native === 'function') {
+      const native = orig.native;
+      wrapper.native = function (p, ...rest) {
+        note(p);
+        return native.call(this, p, ...rest);
+      };
+    }
+    obj[name] = wrapper;
   };
 
   for (const name of [

scripts/test-cache/reporter.mjs

@@ -102,14 +102,22 @@ export default class TestCacheReporter {
     // An unhandled error fails the run without failing any module, so no module's "passed" can be
     // trusted — measured by review: a leaked rejection left its file `passed`, got recorded, and the
     // next run skipped it green. Same for an interrupted run.
-    const runBlock =
-      unhandledErrors?.length > 0
-        ? 'unhandled errors in the run'
-        : reason === 'interrupted'
-          ? 'run interrupted'
-          : this.vitest.config.testNamePattern
-            ? 'name-filtered run'
-            : null;
+    // Narrowed to the files vitest attributes the errors to (VITEST_TEST_PATH, verified present on
+    // a leaked rejection), so one flaky leak does not stop the whole suite recording. An error with
+    // no attribution still blocks everything.
+    const errorFiles = new Set();
+    let unattributed = false;
+    for (const e of unhandledErrors ?? []) {
+      if (e?.VITEST_TEST_PATH) errorFiles.add(core.toRel(e.VITEST_TEST_PATH, root));
+      else unattributed = true;
+    }
+    const runBlock = unattributed
+      ? 'unattributed unhandled error in the run'
+      : reason === 'interrupted'
+        ? 'run interrupted'
+        : this.vitest.config.testNamePattern
+          ? 'name-filtered run'
+          : null;
 
     const fingerprint = core.makeFingerprinter(root);
     const salt = core.globalSalt(root, this.vitest.version, fingerprint);
@@ -129,7 +137,9 @@ export default class TestCacheReporter {
 
     for (const row of rows) {
       try {
-        this.recordOne(row, { root, dir, fingerprint, salt, runBlock, since: state.startedAt, opaqueSource });
+        const block =
+          runBlock ?? (errorFiles.has(row.file) ? 'unhandled error attributed to this file' : null);
+        this.recordOne(row, { root, dir, fingerprint, salt, runBlock: block, since: state.startedAt, opaqueSource });
       } catch (err) {
         row.why = `record failed: ${err?.code ?? err?.message ?? err}`;
       }
@@ -194,31 +204,32 @@ export default class TestCacheReporter {
       ids.add(setup);
       for (const id of s) ids.add(id);
     }
-    // A spawned process or worker reads what it likes, and a computed import is invisible to the
-    // graph — anywhere in the closure, not only in the test file. Measured by review: a helper's
-    // `import(/* @vite-ignore */ file)` loaded a module the key never saw.
-    if (core.reachesOpaqueBuiltin(ids)) return void (row.why = 'reaches child_process / worker_threads');
-
     const entries = new Set([testRel]);
     for (const id of ids) entries.add(core.toRel(id, root));
     for (const p of reads) entries.add(core.toRel(p, root));
     const kept = [...entries].filter((rel) => !core.isCoveredElsewhere(rel));
 
+    // A module in the graph that is gone now was deleted during the run: the test ran WITH it.
+    const graphRels = new Set([...ids].map((id) => core.toRel(id, root)));
+    const vanished = kept.find((rel) => graphRels.has(rel) && fingerprint(rel) === 'missing');
+    if (vanished) return void (row.why = `imported module gone: ${vanished}`);
+
+    // A spawned process or worker reads what it likes, and a computed import is invisible to the
+    // graph — anywhere in the closure, not only in the test file. Measured by review: a helper's
+    // `import(/* @vite-ignore */ file)` loaded a module the key never saw.
     for (const rel of kept) {
       if (/\.[cm]?[jt]sx?$/.test(rel) && opaqueSource(rel)) {
         return void (row.why = `spawns / computed import / glob in ${rel}`);
       }
     }
-    // The key is computed from disk NOW, at the end of the run. An input modified after the run
+    // Key FIRST, then the change check, so an edit landing between the two is caught by the check
+    // instead of being fingerprinted after a check that already passed.
+    const key = core.keyFor({ salt, project, testRel, entries: kept, fingerprint });
+    // The key is computed from disk at the END of the run. An input modified after the run
     // started is not what ran — recording it would certify the edited version as passing.
-    this.movedMemo ??= new Map();
-    const moved = kept.find((rel) => {
-      if (!this.movedMemo.has(rel)) this.movedMemo.set(rel, core.changedSince(root, rel, since - 2000));
-      return this.movedMemo.get(rel);
-    });
+    const moved = kept.find((rel) => core.changedSince(root, rel, since - 2000));
     if (moved) return void (row.why = `changed during the run: ${moved}`);
 
-    const key = core.keyFor({ salt, project, testRel, entries: kept, fingerprint });
     core.writeRecord(dir, project, testRel, { key, entries: kept, at: new Date().toISOString(), ms: row.ms });
     row.recorded = true;
   }