fix(pregate): pre-landing review fixes for removed paths

A removal owes what an edit owes: manifests, lockfiles, config, fixtures,
goldens, workflows and files a config still names keep PREGATE_NO_LANE. A test
reading a fully deleted directory is selected again, a deleted Rust module
selects its crate's tests, a test renamed to a non-test path is accounted as
deleted, and the owning suite is the tests that directly exercise the same files.

Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>

docs/pregate.md

@@ -196,18 +196,25 @@ Two stages with separate clocks, both printed:
   transitively through
   non-test files; deleted and renamed files select the tests that named them.
   A removed path (a deleted file, or a rename's old name) that no remaining
-  test selects owes no lane: it prints `removed (no lane owed): <path> — <why>`
-  and the `lanes` detail adds `N removed path(s)`. A rename's old name defers to
-  the new path (`old name of <new>`); a deleted test runs its owning suite, the
-  remaining tests of the files its merge-base text exercised (`owning suite
-  runs: …`; test helpers and fixtures are not subjects), and names each file
-  left with no test (`coverage removed: <file> has no remaining test`, and
-  `M subject(s) lost their last test` in the detail); any other deleted file
-  has nothing left to test (`no remaining test references it`). A deleted test
-  whose merge-base text cannot be read (a symlink, invalid UTF-8, over 1 MiB)
-  fails closed with `PREGATE_NO_LANE`, and a modified file whose only test the
-  branch deleted still has no lane. /review lists the deletion as a gate edit
-  (RH-1) either way.
+  test selects owes no lane when one of these holds, and prints
+  `removed (no lane owed): <path> — <why>` (the `lanes` detail adds
+  `N removed path(s)`): a rename's old name defers to the new path (`old name
+  of <new>`); a deleted test, or a test renamed to a non-test path, runs its
+  owning suite, the remaining tests that directly exercise the files its
+  merge-base text exercised (`owning suite runs: …`; test helpers and fixtures
+  are not subjects), and names each file left with no test at all (`coverage
+  removed: <file> has no remaining test`, and `M subject(s) lost their last
+  test` in the detail); a deleted code file outside fixture, golden and CI
+  trees that no remaining non-Markdown file names reads `nothing in the tree
+  references it`. Every other removal owes what an edit owes, so it is still
+  `PREGATE_NO_LANE` until a test or a `dependencies` entry covers it: a
+  deleted manifest, lockfile, config, conftest.py, fixture, golden or workflow,
+  a deleted file a config or script still names, and a deleted test whose
+  merge-base text cannot be read (a symlink, invalid UTF-8, over 1 MiB). A
+  modified file whose only test the branch deleted still has no lane, a
+  remaining test that reads a deleted file or directory is selected, and a
+  deleted Rust module selects its crate's tests. /review lists a deleted test
+  as a gate edit (RH-1) either way.
   An extensionless JS/TS import (relative, a tsconfig/jsconfig `paths` alias or
   `baseUrl`) tries the exact path, then `.ts`, `.tsx`, `.js`, `.jsx`, `.mjs`,
   `.cjs`, `.mts`, `.cts`, then `index.ts`, `index.tsx`, `index.js`, `index.jsx`

docs/troubleshooting.md

@@ -3211,12 +3211,15 @@ versions, line endings or any non-version byte changed, one under a test,
 fixture, golden, `.gstack/` or workflow tree, or one added, deleted, renamed
 or re-moded, is still this error; `--explain` prints `not version-stamp-only (<reason>)`
 for it.
-A path the branch removed is not this error either: a deleted file, or a
-rename's old name, that no remaining test selects prints `removed (no lane
-owed): <path> — <why>` instead (a deleted test runs its owning suite or says
-`coverage removed: <file> has no remaining test`). A modified file whose only
-test the branch deleted is still this error, and so is a deleted test whose
-merge-base text cannot be read (a symlink, invalid UTF-8, over 1 MiB).
+A path the branch removed is usually not this error either: a rename's old
+name, a deleted test, and a deleted code file nothing in the tree still names
+print `removed (no lane owed): <path> — <why>` instead (a deleted test runs its
+owning suite or says `coverage removed: <file> has no remaining test`). A
+removal owes what an edit owes, so these are still this error: a deleted
+manifest, lockfile, config, conftest.py, fixture, golden or workflow, a deleted
+file a config or script still names, a deleted test whose merge-base text
+cannot be read (a symlink, invalid UTF-8, over 1 MiB), and a modified file whose
+only test the branch deleted.
 
 **Fix.** Add a test that names the file, or declare its lane (an explicit `[]`
 for files with no behaviour) under `dependencies` in `.gstack/pregate.json` (or the user-owned file the message names).

lib/pregate/docs-lane.ts

@@ -41,6 +41,9 @@ function isMarkdown(rel: string): boolean {
   return MARKDOWN.test(rel);
 }
 
+/** Fixture, snapshot and golden trees, CI workflows and actions, and `.gstack/`: files a test or the CI may read by a path the import graph cannot see. */
+export function isFixtureOrCiTree(rel: string): boolean { return PROTECTED_TREE.test(rel.replace(/\\/g, '/').replace(/^\.\//, '')); }
+
 function isTestTree(rel: string): boolean {
   return TEST_TREE.test(rel);
 }

lib/pregate/import-graph.ts

@@ -478,7 +478,7 @@ export function buildGraph(repoRoot: string, opts: BuildOptions = {}): Graph {
   const all = listRepoFiles(repoRoot).filter(f => !gone.has(f));
   const files = new Set([...all, ...gone]);
   const dirs = new Set<string>();
-  for (const f of all) { let d = path.posix.dirname(f); while (d && d !== '.') { dirs.add(d); d = path.posix.dirname(d); } }
+  for (const f of files) { let d = path.posix.dirname(f); while (d && d !== '.') { dirs.add(d); d = path.posix.dirname(d); } }
   const config = readPathAliases(repoRoot);
   const graph: Graph = { files, tests: new Set(all.filter(isTestFile)), reverse: new Map(), unresolved: new Map(), aliasNotes: config.notes };
   const maxBytes = opts.maxBytes ?? 2 * 1024 * 1024;
@@ -493,8 +493,8 @@ export function buildGraph(repoRoot: string, opts: BuildOptions = {}): Graph {
     for (const { target, via } of referencesIn(files, dirs, rel, source, config, graph.tests.has(rel) ? unresolved : undefined, packages)) addEdge(graph, target, rel, via);
     if (unresolved.length) graph.unresolved!.set(rel, unresolved);
   }
-  const rust = rustEdges(all, rel => readRepoFile(repoRoot, rel, maxBytes));
-  for (const t of rust.tests) graph.tests.add(t);
+  const rust = rustEdges([...files], rel => readRepoFile(repoRoot, rel, maxBytes));
+  for (const t of rust.tests) if (!gone.has(t)) graph.tests.add(t);
   for (const e of rust.edges) addEdge(graph, e.dependency, e.dependent, e.via);
   const django = djangoEdges({ files: [...files], tests: graph.tests, read: rel => pySources.get(rel) ?? readRepoFile(repoRoot, rel, maxBytes), code: pythonCode, resolveDotted: dotted => pythonPatchTarget(files, dotted) });
   for (const e of django) addEdge(graph, e.dependency, e.dependent, 'framework');

lib/pregate/lane-selection.ts

@@ -12,10 +12,10 @@ import { GENERATED_PATH } from '../regen';
 import { runBashSync } from '../script-invocation';
 import { matchesAny } from '../ship-policy';
 import { PREGATE_CONFIG_PATH } from './config';
-import { isDocsEmptyLane, isInertNoTests, isProtectedTree } from './docs-lane';
+import { isDocsEmptyLane, isFixtureOrCiTree, isInertNoTests, isProtectedTree } from './docs-lane';
 import * as fs from 'node:fs';
 import * as path from 'node:path';
-import { buildGraph, isTestFile, listRepoFiles, testsFor, type Graph } from './import-graph';
+import { buildGraph, isTestFile, listRepoFiles, SOURCE_EXT, testsFor, type Graph } from './import-graph';
 import { stampFileKind, versionStampOnly } from './stamp-only';
 import type { PregateContext, RemoteObligation } from './types';
 import type { Lane } from './workflows';
@@ -105,30 +105,47 @@ function readWorkingTree(repoRoot: string, rel: string): { text: string | null;
 /** Tracked files matching a registry entry's output globs. */
 function outputsOf(all: readonly string[], globs: readonly string[]): string[] { return all.filter(f => matchesAny(globs, f)); }
 
+/** Tracked files outside Markdown that name `file` by its repo path (a config, a script or a workflow may load it where the import graph cannot see). */
+function namedBy(ctx: PregateContext, file: string): string[] {
+  const r = spawnSync('git', ['grep', '--untracked', '-l', '-F', '-e', file, '--', '.', ':(exclude)*.md', ':(exclude)*.mdx'], { cwd: ctx.repoRoot, encoding: 'utf8', timeout: 60_000, maxBuffer: 16 * 1024 * 1024, env: ctx.env });
+  if (r.status === 1) return [];
+  return r.status === 0 ? (r.stdout ?? '').split('\n').filter(Boolean) : ['(git grep failed)'];
+}
+
+const shown = (files: readonly string[]) => `${files.slice(0, 8).join(' ')}${files.length > 8 ? ` … +${files.length - 8} more` : ''}`;
+
 /**
- * Why a removed path no remaining test selects owes no lane, and the owning-suite tests it selects: a rename's old name defers
- * to the new path; a deleted test selects the remaining tests of the files it exercised and names those that lost their last
- * test; any other removed file has nothing left to test. Null when a deleted test's merge-base text could not be read: it fails closed.
+ * Why a removed path no remaining test selects owes no lane, and the owning-suite tests it selects, or `refused` when it must
+ * fail closed as no lane. A rename's old name defers to the new path, unless a test was renamed to a non-test path. A deleted
+ * test runs the tests that directly exercise the files it exercised and names those left with no test at all. Any other removed
+ * file owes nothing only when it is code the import graph reads (outside fixture, golden and CI trees) and no remaining
+ * non-Markdown file names it: deleting a manifest, a lockfile, a config or a workflow owes what editing it owes.
  */
-function removedPath(ctx: PregateContext, graph: Graph, file: string): { removed: Removed; suite: string[] } | null {
+function removedPath(ctx: PregateContext, graph: Graph, file: string): { removed: Removed; suite: string[] } | { refused: string } {
   const renamedTo = ctx.touched.find(t => t.renamedFrom === file)?.path;
-  if (renamedTo) return { removed: { file, note: `old name of ${renamedTo}`, lostCoverage: [] }, suite: [] };
-  if (!isTestFile(file)) return { removed: { file, note: 'no remaining test references it', lostCoverage: [] }, suite: [] };
+  if (renamedTo && !(isTestFile(file) && !isTestFile(renamedTo))) return { removed: { file, note: `old name of ${renamedTo}`, lostCoverage: [] }, suite: [] };
+  if (!isTestFile(file)) {
+    const code = (SOURCE_EXT.test(file) || file.startsWith('bin/')) && !isFixtureOrCiTree(file) && path.posix.basename(file) !== 'conftest.py';
+    if (!code) return { refused: 'removed, but not code the import graph reads (a manifest, lockfile, config, fixture or workflow owes what an edit owes)' };
+    const names = namedBy(ctx, file);
+    if (names.length) return { refused: `removed, but still named by ${shown(names)}` };
+    return { removed: { file, note: 'nothing in the tree references it', lostCoverage: [] }, suite: [] };
+  }
   const subjects = graph.goneSubjects?.get(file);
-  if (!subjects) return null;
+  if (!subjects) return { refused: 'removed test whose merge-base text could not be read (not a regular file there, invalid UTF-8 or over 1 MiB)' };
   if (!subjects.length) return { removed: { file, note: 'it exercised no file still in the tree', lostCoverage: [] }, suite: [] };
   const suite = new Set<string>();
   const lostCoverage: string[] = [];
   for (const subject of subjects) {
     const m = testsFor(graph, subject, ctx.config.dependencies);
-    for (const t of m.tests) suite.add(t);
+    for (const e of graph.reverse.get(subject) ?? []) if (graph.tests.has(e.from)) suite.add(e.from);
     if (!m.tests.length && !m.declaredNone) lostCoverage.push(subject);
   }
   const notes = [
-    suite.size ? `owning suite runs: ${[...suite].sort().join(' ')}` : '',
+    suite.size ? `owning suite runs: ${shown([...suite].sort())}` : '',
     lostCoverage.length ? `coverage removed: ${lostCoverage.join(' ')} ${lostCoverage.length === 1 ? 'has' : 'have'} no remaining test` : '',
   ].filter(Boolean);
-  return { removed: { file, note: notes.join('; ') || 'it exercised only files declared no lane needed', lostCoverage }, suite: [...suite] };
+  return { removed: { file, note: notes.join('; ') || 'its files keep indirect or declared coverage', lostCoverage }, suite: [...suite] };
 }
 
 export function selectLanes(ctx: PregateContext): Selection {
@@ -136,9 +153,10 @@ export function selectLanes(ctx: PregateContext): Selection {
   const gone = touched.flatMap(t => [t.status === 'D' ? t.path : null, t.renamedFrom ?? null]).filter((p): p is string => !!p);
   const goneSources = new Map<string, string>();
   for (const t of touched) {
-    if (t.status !== 'D' || !isTestFile(t.path)) continue;
-    const text = readAtMergeBase(ctx, t.path, ['100644', '100755']);
-    if (text !== null) goneSources.set(t.path, text);
+    const removedTest = t.status === 'D' && isTestFile(t.path) ? t.path : t.renamedFrom && isTestFile(t.renamedFrom) && !isTestFile(t.path) ? t.renamedFrom : null;
+    if (!removedTest) continue;
+    const text = readAtMergeBase(ctx, removedTest, ['100644', '100755']);
+    if (text !== null) goneSources.set(removedTest, text);
   }
   const graph: Graph = buildGraph(ctx.repoRoot, { declarations: ctx.config.dependencies, extraFiles: gone, goneSources });
   const all = listRepoFiles(ctx.repoRoot);
@@ -199,13 +217,13 @@ export function selectLanes(ctx: PregateContext): Selection {
     }
     if (gone.includes(file)) {
       const r = removedPath(ctx, graph, file);
-      if (r) {
+      if ('removed' in r) {
         removed.push(r.removed);
         for (const t of r.suite) tests.add(t);
         provenance.push(`${file}: removed (no lane owed) — ${r.removed.note}`);
         continue;
       }
-      provenance.push(`${file}: removed test whose merge-base text could not be read (not a regular file there, invalid UTF-8 or over 1 MiB); no lane until declared`);
+      provenance.push(`${file}: ${r.refused}; no lane until declared`);
     }
     noLane.push(file);
   }

test/pregate-removed-paths.test.ts

@@ -9,7 +9,8 @@
 import { afterEach, describe, expect, test } from 'bun:test';
 import * as fs from 'node:fs';
 import * as path from 'node:path';
-import { BASE_PREGATE, cleanup, commit, makeFixtureRepo, runBin, write, type FixtureRepo } from './helpers/pregate-fixture';
+import { buildGraph, testsFor } from '../lib/pregate/import-graph';
+import { BASE_PREGATE, cleanup, commit, makeFixtureRepo, runBin, type FixtureRepo } from './helpers/pregate-fixture';
 
 const repos: FixtureRepo[] = [];
 const fixture = (opts?: Parameters<typeof makeFixtureRepo>[0]) => { const r = makeFixtureRepo(opts); repos.push(r); return r; };
@@ -60,7 +61,7 @@ describe('removed paths in lanes (#3529)', () => {
     const r = lanes(repo);
     expect(r.check.status).toBe('pass');
     expect(r.removed).toEqual([
-      'removed (no lane owed): lib/util.js — no remaining test references it',
+      'removed (no lane owed): lib/util.js — nothing in the tree references it',
       'removed (no lane owed): test/util.test.js — it exercised no file still in the tree',
     ]);
     expect(r.check.detail).not.toContain('lost their last test');
@@ -89,7 +90,7 @@ describe('removed paths in lanes (#3529)', () => {
     commit(repo, { 'lib/orphan.js': null });
     const r = lanes(repo);
     expect(r.check.status).toBe('pass');
-    expect(r.removed).toEqual(['removed (no lane owed): lib/orphan.js — no remaining test references it']);
+    expect(r.removed).toEqual(['removed (no lane owed): lib/orphan.js — nothing in the tree references it']);
   });
 
   test('an unstaged deletion behaves like a committed one: the missing test is never selected', () => {
@@ -122,7 +123,71 @@ describe('removed paths in lanes (#3529)', () => {
     expect(r.ran).not.toContain('helper-user');
   });
 
+  test('a deleted test named after a bin/ entrypoint reports that bin when it loses its last test', () => {
+    const repo = fixture({ baseFiles: { 'bin/gstack-foo': '#!/bin/sh\necho foo\n', 'test/gstack-foo.test.js': "const test = require('node:test');\ntest('foo', () => {});\n" } });
+    commit(repo, { 'test/gstack-foo.test.js': null });
+    expect(lanes(repo).removed).toEqual(['removed (no lane owed): test/gstack-foo.test.js — coverage removed: bin/gstack-foo has no remaining test']);
+  });
+
+  test('a file declared no lane needed never reads as lost coverage', () => {
+    const repo = fixture({ pregate: { ...BASE_PREGATE, dependencies: { ...BASE_PREGATE.dependencies, 'lib/util.js': [] } } });
+    commit(repo, { 'test/util.test.js': null });
+    const r = lanes(repo);
+    expect(r.check.status).toBe('pass');
+    expect(r.removed).toEqual(['removed (no lane owed): test/util.test.js — its files keep indirect or declared coverage']);
+    expect(r.check.detail).not.toContain('lost their last test');
+  });
+
+  test('the owning suite is the tests that exercise the same files directly, not every test that reaches them', () => {
+    const wrap = "exports.addTwice = (a) => require('./util.js').add(a, a);\n";
+    const wrapTest = "const test = require('node:test');\nconst assert = require('node:assert');\nconst { addTwice } = require('../lib/wrap.js');\ntest('twice', () => assert.strictEqual(addTwice(2), 4));\n";
+    const repo = fixture({ baseFiles: { 'lib/wrap.js': wrap, 'test/wrap.test.js': wrapTest, 'test/util-v2.test.js': UTIL_V2 } });
+    commit(repo, { 'test/util.test.js': null });
+    const r = lanes(repo);
+    expect(r.removed).toEqual(['removed (no lane owed): test/util.test.js — owning suite runs: test/util-v2.test.js']);
+    expect(r.ran).not.toContain('test/wrap.test.js');
+  });
+
+  test('a test renamed to a non-test path is accounted for like a deleted test', () => {
+    const repo = fixture();
+    move(repo, 'test/util.test.js', 'docs/util-notes.md');
+    expect(lanes(repo).removed).toEqual(['removed (no lane owed): test/util.test.js — coverage removed: lib/util.js has no remaining test']);
+  });
+
+  test('a deleted Rust module selects its crate\'s tests', () => {
+    const repo = fixture({ baseFiles: { 'crates/demo/Cargo.toml': '[package]\nname = "demo"\nversion = "0.1.0"\n', 'crates/demo/src/lib.rs': 'pub mod gone;\n#[cfg(test)]\nmod tests {\n  #[test]\n  fn t() {}\n}\n', 'crates/demo/src/gone.rs': 'pub fn f() {}\n' } });
+    commit(repo, { 'crates/demo/src/gone.rs': null });
+    const g = buildGraph(repo.work, { extraFiles: ['crates/demo/src/gone.rs'] });
+    expect(testsFor(g, 'crates/demo/src/gone.rs').tests).toEqual(['crates/demo/src/lib.rs']);
+  });
+
   describe('negative controls', () => {
+    test('deleting every file of a fixture directory a remaining test reads selects that test', () => {
+      const reader = "const test = require('node:test');\nconst fs = require('node:fs');\ntest('reads the fixture dir', () => { fs.readdirSync('test/fixtures/data'); });\n";
+      const repo = fixture({ baseFiles: { 'test/fixtures/data/a.json': '{}\n', 'test/fixtures/data/b.json': '{}\n', 'test/fixture-dir.test.js': reader } });
+      commit(repo, { 'test/fixtures/data/a.json': null, 'test/fixtures/data/b.json': null });
+      const r = lanes(repo);
+      expect(r.removed).toEqual([]);
+      expect(r.ran).toContain('test/fixture-dir.test.js');
+    });
+
+    test.each(['package.json', '.github/workflows/windows.yml', 'test/golden/digest.txt'])('deleting %s owes what editing it owes: PREGATE_NO_LANE', file => {
+      const repo = fixture();
+      commit(repo, { [file]: null });
+      const r = lanes(repo);
+      expect(r.check.code).toBe('PREGATE_NO_LANE');
+      expect(r.check.detail).toContain(file);
+      expect(r.removed).toEqual([]);
+    });
+
+    test('a deleted source file a remaining config still names fails closed', () => {
+      const repo = fixture({ baseFiles: { 'scripts/setup.js': "console.log('setup');\n", 'bunfig.toml': '[test]\npreload = ["./scripts/setup.js"]\n' } });
+      commit(repo, { 'scripts/setup.js': null });
+      const r = lanes(repo);
+      expect(r.check.code).toBe('PREGATE_NO_LANE');
+      expect(r.check.detail).toContain('scripts/setup.js');
+    });
+
     test('a modified file whose only test was deleted still fails PREGATE_NO_LANE', () => {
       const repo = fixture();
       commit(repo, { 'test/util.test.js': null, 'lib/util.js': "exports.add = (a, b) => b + a;\n" });
@@ -131,6 +196,7 @@ describe('removed paths in lanes (#3529)', () => {
       expect(r.check.code).toBe('PREGATE_NO_LANE');
       expect(r.check.detail).toContain('lib/util.js');
       expect(r.check.detail).not.toContain('test/util.test.js');
+      expect(r.check.detail).toContain('1 removed path(s); 1 subject(s) lost their last test');
       expect(r.removed).toEqual(['removed (no lane owed): test/util.test.js — coverage removed: lib/util.js has no remaining test']);
     });