review-collect: narrow the consumer scan and give it one 300 s deadline (GST-544)

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

changelog.d/gst-544-consumer-scan-timeout.md

@@ -0,0 +1,8 @@
+**The review planner no longer fails closed on repositories with big data trees just because its consumer search ran past 60 seconds (GST-544).** `gstack-review-plan` looks for code that names each changed data file. It used to run one `git grep` over the whole tree with every data file name, and each call had a hard 60-second limit. In garrytan/harness-evals, with 8,000+ result files, that search took about 150 seconds, so the plan fell back to a full review every time.
+
+### Changed
+- **The consumer scan searches only files that can match** (`lib/review-collect.ts`). A cheap first pass lists the files that contain a changed file's extension, such as `.json`; the full search then reads only those files, a batch of paths per `git grep` call, with docs files left out. The result is the same as before.
+- **One deadline for the whole scan:** 300 seconds by default, set in milliseconds with `GSTACK_CONSUMER_SCAN_TIMEOUT_MS` (docs/review-policy.md#review-depth). Past it the depth still fails closed with `consumer_scan_failed`, and the detail now says whether git timed out or exited with an error, and names the deadline. Other git reads keep their 60-second limit.
+
+### Tests
+- `test/review-collect.test.ts`: the narrowed search prints the same lines as one whole-tree `git grep` at the head, the merge base and the working tree; the same holds when every file is its own call; consumed sets on a mixed fixture (a chain through a data manifest, an untracked consumer, docs mentions ignored); a tiny deadline fails closed and says it timed out. The three new behavior tests failed before the fix.

docs/review-policy.md

@@ -178,7 +178,11 @@ directory, dotfiles, `*.config.*`, `package.json`, lockfiles, `tsconfig*.json`,
 `manifest.json` and the other tool manifests `lib/review-depth.ts` lists) and a named
 consumer each keep the path code, and YAML, TOML and SVG are always code. More than
 2,000 data paths keep every one code; a `git grep` failure is `consumer_scan_failed`
-and fails the depth closed. Only `classifyDiff` applies the rule (`lib/review-collect.ts`
+and fails the depth closed. The scan first lists the files that contain a candidate's
+extension and then searches only those, docs files left out, so a large data tree
+costs one cheap pass. The whole scan gets one 300-second deadline, set in milliseconds
+with `GSTACK_CONSUMER_SCAN_TIMEOUT_MS`; a scan that runs past it is also
+`consumer_scan_failed`, and its detail says it timed out and names the deadline. Only `classifyDiff` applies the rule (`lib/review-collect.ts`
 finds the consumers, `gstack-ship-preflight`'s `RUNTIME_LINES` reads the same
 function); every path-only caller (the receipt's gate-scan decision, the Step 16
 post-review route, /autoplan's docs-only scope) keeps data files code. A numeric claim

lib/review-collect.ts

@@ -34,6 +34,13 @@ function git(root: string, args: string[]): { ok: boolean; status: number | null
   const r = spawnSync('git', ['-c', 'core.quotePath=false', ...args], { cwd: root, encoding: 'utf8', timeout: 60_000, maxBuffer: 256 * 1024 * 1024 });
   return { ok: r.status === 0, status: r.status, out: r.stdout ?? '', err: (r.stderr ?? '').trim() };
 }
+/** git with what is left of an overall deadline (epoch ms) as its timeout, for a read that may take minutes. */
+function gitUntil(root: string, args: string[], deadline: number): { status: number | null; out: string; err: string; timedOut: boolean } {
+  const left = deadline - Date.now();
+  if (left <= 0) return { status: null, out: '', err: '', timedOut: true };
+  const r = spawnSync('git', ['-c', 'core.quotePath=false', ...args], { cwd: root, encoding: 'utf8', timeout: left, maxBuffer: 256 * 1024 * 1024 });
+  return { status: r.status, out: r.stdout ?? '', err: (r.stderr ?? '').trim(), timedOut: r.status === null && Date.now() >= deadline };
+}
 const scratch = (p: string) => p.startsWith('.gstack/tmp/');
 const oneLine = (s: string) => s.replace(/[\0-\x1f]+/g, ' ').trim().slice(0, 200);
 const unavailable = (input: string, detail: string): RegistryInfo => ({ sha: null, outputs: [], error: { code: 'registry_unavailable', detail: oneLine(detail), input, repair: 'fetch the base branch (git fetch origin <base>) so its commit and registry resolve' } });
@@ -97,36 +104,81 @@ function readDiff(root: string, range: string[], errors: DepthError[]): Pick<Col
 export const CONSUMER_SCAN_CAP = 2000;
 /** How many data files deep a chain of data naming data is followed (a manifest naming a data file) before the rest count as consumed. */
 export const CONSUMER_SCAN_DEPTH = 4;
+/** One consumer scan's whole budget, every git grep in it (GSTACK_CONSUMER_SCAN_TIMEOUT_MS overrides it); past it the scan fails closed. */
+export const CONSUMER_SCAN_TIMEOUT_MS = 300_000;
+/** Pathspec bytes per git grep call, so a long list of files to search stays under Windows' 32 KB command line. */
+export const CONSUMER_SCAN_PATHSPEC_BYTES = 16_000;
+
+export function consumerScanTimeoutMs(): number {
+  const v = Number(process.env.GSTACK_CONSUMER_SCAN_TIMEOUT_MS);
+  return Number.isFinite(v) && v > 0 ? v : CONSUMER_SCAN_TIMEOUT_MS;
+}
+
+export type GrepFailure = { timedOut: boolean; status: number | null; err: string };
+
+/**
+ * The lines `git grep -I -F -o --null` prints for `patterns` at `rev` ('' is the working tree with untracked files), for
+ * the files `keep` accepts, in some order. Every pattern holds its own extension, so a `-l` pass for the extensions alone
+ * lists every file that can match; the full pattern list then reads only those files, CONSUMER_SCAN_PATHSPEC_BYTES of
+ * literal pathspecs per call. Each call gets what is left of `deadline` (epoch ms).
+ */
+export function grepConsumers(root: string, patterns: readonly string[], rev: string, keep: (file: string) => boolean, deadline: number, chunkBytes = CONSUMER_SCAN_PATHSPEC_BYTES): string[] | GrepFailure {
+  const where = rev ? [rev, '--'] : ['--untracked', '--'];
+  const exts = [...new Set(patterns.map(b => b.slice(b.lastIndexOf('.'))))].flatMap(e => ['-e', e]);
+  const listed = gitUntil(root, ['grep', '-I', '-F', '-l', '-z', '--no-color', ...exts, ...where], deadline);
+  if (listed.status !== 0 && listed.status !== 1) return listed;
+  const files = listed.out.split('\0').filter(Boolean).map(f => (rev ? f.slice(rev.length + 1) : f)).filter(keep);
+  const flags = ['grep', '-I', '-F', '-o', '--null', '--no-line-number', '--no-column', '--no-color', ...patterns.flatMap(b => ['-e', b]), ...where];
+  const lines: string[] = [];
+  for (let i = 0; i < files.length;) {
+    const chunk: string[] = [];
+    for (let bytes = 0; i < files.length; i++) {
+      const spec = `:(literal)${files[i]}`;
+      if (chunk.length && bytes + spec.length > chunkBytes) break;
+      chunk.push(spec);
+      bytes += spec.length;
+    }
+    const r = gitUntil(root, [...flags, ...chunk], deadline);
+    if (r.status !== 0 && r.status !== 1) return r;
+    lines.push(...r.out.split('\n'));
+  }
+  return lines;
+}
 
 /**
  * The data candidates (lib/review-depth.ts dataCandidate) among the changed and deleted paths that a non-docs file names
  * by basename, at each rev and, with `live`, in the working tree with untracked files. The merge base is one of the revs,
  * so a consumer the change removes still counts. A data file named only by another data file is consumed when that one
- * is, followed CONSUMER_SCAN_DEPTH files deep; anything still open after that counts as consumed. A git grep failure is
- * a collector error, which fails the depth closed.
+ * is, followed CONSUMER_SCAN_DEPTH files deep; anything still open after that counts as consumed. A git grep failure, or
+ * a scan that outlives `timeoutMs`, is a collector error, which fails the depth closed.
  */
-export function scanConsumers(root: string, paths: readonly string[], revs: readonly string[], live: boolean, classRoot: string | readonly string[], errors: DepthError[]): string[] | 'all' {
+export function scanConsumers(root: string, paths: readonly string[], revs: readonly string[], live: boolean, classRoot: string | readonly string[], errors: DepthError[], timeoutMs = consumerScanTimeoutMs()): string[] | 'all' {
   const candidates = [...new Set(paths)].filter(p => dataCandidate(p, classRoot)).sort();
   if (candidates.length > CONSUMER_SCAN_CAP) return 'all';
+  const deadline = Date.now() + timeoutMs;
   const base = (p: string) => p.slice(p.lastIndexOf('/') + 1);
+  const read = (file: string) => !scratch(file) && classifyPath(file, classRoot) !== 'docs';
   const named = new Map<string, Set<string>>();
   const direct = new Set<string>();
   const probed = new Set<string>();
   let probe = candidates;
   for (let depth = 0; probe.length && depth < CONSUMER_SCAN_DEPTH; depth++) {
-    const patterns = [...new Set(probe.map(base))].flatMap(b => ['-e', b]);
-    const runs = [...revs.map(rev => ({ at: rev, args: [rev, '--'] })), ...(live ? [{ at: '', args: ['--untracked', '--'] }] : [])];
+    const patterns = [...new Set(probe.map(base))];
     const next = new Set<string>();
-    for (const run of runs) {
-      const r = git(root, ['grep', '-I', '-F', '-o', '--null', '--no-line-number', '--no-column', '--no-color', ...patterns, ...run.args]);
-      if (r.status !== 0 && r.status !== 1) { errors.push({ code: 'consumer_scan_failed', detail: oneLine(r.err || `git grep exited ${r.status}`), input: `git grep -F <data basenames> ${run.at || '--untracked'}`, repair: 'run the git grep by hand and fix what git reports' }); return 'all'; }
-      for (const line of r.out.split('\n')) {
+    for (const at of [...revs, ...(live ? [''] : [])]) {
+      const r = grepConsumers(root, patterns, at, read, deadline);
+      if (!Array.isArray(r)) {
+        const detail = r.timedOut ? `timed out: the scan's ${timeoutMs / 1000} s deadline passed (GSTACK_CONSUMER_SCAN_TIMEOUT_MS)` : `git grep exited ${r.status}${r.err ? `: ${r.err}` : ''} (deadline ${timeoutMs / 1000} s)`;
+        errors.push({ code: 'consumer_scan_failed', detail: oneLine(detail), input: `git grep -F <data basenames> ${at || '--untracked'}`, repair: r.timedOut ? 'raise GSTACK_CONSUMER_SCAN_TIMEOUT_MS (milliseconds) or run the git grep by hand to see why it is slow' : 'run the git grep by hand and fix what git reports' });
+        return 'all';
+      }
+      for (const line of r) {
         const parts = line.split('\0');
         const where = parts[0];
         const match = parts.length > 1 ? parts[parts.length - 1] : undefined;
         if (!where || !match) continue;
-        const file = run.at ? where.slice(run.at.length + 1) : where;
-        if (scratch(file) || classifyPath(file, classRoot) === 'docs') continue;
+        const file = at ? where.slice(at.length + 1) : where;
+        if (!read(file)) continue;
         for (const p of probe) {
           if (file === p || !match.includes(base(p))) continue;
           if (!dataCandidate(file, classRoot)) { direct.add(p); continue; }

test/review-collect.test.ts

@@ -3,7 +3,7 @@ import { spawnSync } from 'node:child_process';
 import * as fs from 'node:fs';
 import * as os from 'node:os';
 import * as path from 'node:path';
-import { collectCommit, collectLive, CONSUMER_SCAN_CAP, loadRegistryAt } from '../lib/review-collect';
+import { collectCommit, collectLive, CONSUMER_SCAN_CAP, grepConsumers, loadRegistryAt, scanConsumers } from '../lib/review-collect';
 import { depthOfCommitInput } from '../lib/review-depth';
 
 const dirs: string[] = [];
@@ -177,3 +177,66 @@ describe('GSTA-167 part 4a: which data files code names (scanConsumers)', () =>
     expect(depthOfCommitInput({ ...c, gateTagged: 0 }).class).toBe('code');
   }, 60_000);
 });
+
+describe('GST-544: the consumer scan reads only files that can match, under one overall deadline', () => {
+  const rows = Array.from({ length: 40 }, (_, i) => `{"i":${i}}`).join('\n') + '\n';
+  const blob = Buffer.concat([Buffer.from([0, 1, 2]), Buffer.from(' rows.json post.json\n')]);
+  const mixed = () => {
+    const r = repo({ 'src/old.ts': "read('rules.json')\n", 'src/manifest.ts': "load('index.json')\n", 'docs/wiki.md': 'see orphan.json and rows.json\n', 'bench/blob.bin': blob, 'bench/notes.ts': 'no data names here, only .json and .csv\n' });
+    const head = r.commit({ 'src/old.ts': null, 'src/load.ts': "require('./data/rows.json'); require('my rows.json')\n", 'data/rows.json': rows, 'data/my rows.json': rows, 'assets/rules.json': rows, 'assets/index.json': '["post.json"]\n', 'assets/post.json': rows, 'wiki/orphan.json': rows, 'wiki/live.csv': 'a\n', 'bench/overlap.ts': "'xrows.jsonl' 'ab.csv' 'b.csv'\n" });
+    r.write({ 'src/untracked.ts': "read('live.csv')\n" });
+    return { ...r, head };
+  };
+  const patterns = ['rows.json', 'my rows.json', 'rules.json', 'index.json', 'post.json', 'orphan.json', 'live.csv', 'b.csv'];
+  const wholeTree = (dir: string, at: string) => {
+    const r = spawnSync('git', ['-c', 'core.quotePath=false', 'grep', '-I', '-F', '-o', '--null', '--no-line-number', '--no-column', '--no-color', ...patterns.flatMap(b => ['-e', b]), ...(at ? [at, '--'] : ['--untracked', '--'])], { cwd: dir, encoding: 'utf8', timeout: 10_000 });
+    expect([0, 1]).toContain(r.status);
+    return r.stdout.split('\n').filter(Boolean).sort();
+  };
+  // Value: protects=the narrowed scan prints exactly the lines one whole-tree git grep prints, at a commit, at the merge base
+  //   and in the working tree with untracked files; fails_when=the extension pre-pass or the pathspecs drop a consumer;
+  //   seam=git grep output, compared with the single grep the scan ran before GST-544
+  test('grepConsumers prints the same lines as one whole-tree git grep at each rev and in the working tree', () => {
+    const r = mixed();
+    for (const at of [r.baseSha, r.head, '']) {
+      const got = grepConsumers(r.dir, patterns, at, () => true, Date.now() + 10_000);
+      expect(Array.isArray(got)).toBe(true);
+      expect((got as string[]).filter(Boolean).sort()).toEqual(wholeTree(r.dir, at));
+    }
+    expect(wholeTree(r.dir, '').length).toBeGreaterThan(5);
+  });
+  test('the pathspec chunks give the same lines when every file is its own git grep call', () => {
+    const r = mixed();
+    const got = grepConsumers(r.dir, patterns, '', () => true, Date.now() + 10_000, 1);
+    expect((got as string[]).filter(Boolean).sort()).toEqual(wholeTree(r.dir, ''));
+    expect(new Set(wholeTree(r.dir, '').map(l => l.split('\0')[0])).size).toBeGreaterThan(3);
+  });
+  test('consumed sets on the mixed fixture: chains through a data manifest, a live untracked consumer, docs ignored', () => {
+    const r = mixed();
+    const commit = collectCommit(r.dir, r.baseSha, r.head);
+    expect([commit.consumed, commit.errors]).toEqual([['assets/index.json', 'assets/post.json', 'assets/rules.json', 'data/my rows.json', 'data/rows.json'], []]);
+    const live = collectLive(r.dir, 'main');
+    expect([live.consumed, live.errors]).toEqual([['assets/index.json', 'assets/post.json', 'assets/rules.json', 'data/my rows.json', 'data/rows.json', 'wiki/live.csv'], []]);
+  });
+  // Value: protects=a scan that runs out of time keeps every data file code and says it timed out, with the deadline;
+  //   fails_when=a slow grep drops data to docs or reads as a git error; seam=the spawnSync timeout
+  test('a scan past its deadline fails closed with consumer_scan_failed naming the timeout and the deadline', () => {
+    const r = mixed();
+    const errors: Parameters<typeof scanConsumers>[5] = [];
+    expect(scanConsumers(r.dir, ['data/rows.json', 'wiki/orphan.json'], [r.head], true, r.dir, errors, 1)).toBe('all');
+    expect(errors).toEqual([expect.objectContaining({ code: 'consumer_scan_failed', detail: expect.stringMatching(/timed out.*0\.001 s deadline.*GSTACK_CONSUMER_SCAN_TIMEOUT_MS/) })]);
+    const saved = process.env.GSTACK_CONSUMER_SCAN_TIMEOUT_MS;
+    process.env.GSTACK_CONSUMER_SCAN_TIMEOUT_MS = '1';
+    try {
+      const c = collectCommit(r.dir, r.baseSha, r.head);
+      expect(c.consumed).toBe('all');
+      expect(c.errors.map(e => [e.code, /timed out/.test(e.detail)])).toEqual([['consumer_scan_failed', true]]);
+    } finally {
+      if (saved === undefined) delete process.env.GSTACK_CONSUMER_SCAN_TIMEOUT_MS; else process.env.GSTACK_CONSUMER_SCAN_TIMEOUT_MS = saved;
+    }
+    r.git('config', 'grep.lineNumber', 'not-a-bool');
+    const failed: Parameters<typeof scanConsumers>[5] = [];
+    expect(scanConsumers(r.dir, ['data/rows.json'], [r.head], false, r.dir, failed)).toBe('all');
+    expect(failed[0]!.detail).toMatch(/^git grep exited \d+: .*\(deadline 300 s\)$/);
+  });
+});