review-collect: whole-tree grep for non-UTF-8 names, no argument spread, name spawn errors (review fix)

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

lib/review-collect.ts

@@ -39,7 +39,7 @@ function gitUntil(root: string, args: string[], deadline: number): { status: num
   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 };
+  return { status: r.status, out: r.stdout ?? '', err: (r.stderr ?? '').trim() || ((r.error as NodeJS.ErrnoException | undefined)?.code ?? ''), 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);
@@ -123,15 +123,21 @@ export type GrepFailure = { timedOut: boolean; status: number | null; err: strin
  * 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
- * paths per call under --literal-pathspecs (an inherited GIT_GLOB_PATHSPECS makes git refuse, which fails closed). Each call gets what is left of `deadline` (epoch ms).
+ * paths per call under --literal-pathspecs (an inherited GIT_GLOB_PATHSPECS makes git refuse, which fails closed). A listed name
+ * that is not valid UTF-8 cannot be passed back as a path, so then one grep reads the whole tree, as the scan did before GST-544. 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 names = listed.out.split('\0').filter(Boolean).map(f => (rev ? f.slice(rev.length + 1) : f));
   const flags = ['--literal-pathspecs', 'grep', '-I', '-F', '-o', '--null', '--no-line-number', '--no-column', '--no-color', ...patterns.flatMap(b => ['-e', b]), ...where];
+  if (names.some(f => f.includes('\uFFFD'))) {
+    const whole = gitUntil(root, flags, deadline);
+    return whole.status !== 0 && whole.status !== 1 ? whole : whole.out.split('\n');
+  }
+  const files = names.filter(keep);
   const lines: string[] = [];
   for (let i = 0; i < files.length;) {
     const chunk: string[] = [];
@@ -143,7 +149,7 @@ export function grepConsumers(root: string, patterns: readonly string[], rev: st
     }
     const r = gitUntil(root, [...flags, ...chunk], deadline);
     if (r.status !== 0 && r.status !== 1) return r;
-    lines.push(...r.out.split('\n'));
+    for (const line of r.out.split('\n')) lines.push(line);
   }
   return lines;
 }

test/review-collect.test.ts

@@ -247,13 +247,32 @@ describe('GST-544: the consumer scan reads only files that can match, under one
   // Value: protects=an inherited GIT_LITERAL_PATHSPECS (git --literal-pathspecs exports it to children) cannot empty the narrowed
   //   search and drop data to docs; a fractional override cannot crash spawnSync; fails_when=the pathspecs use magic the variable
   //   turns off, or the timeout is not an integer; seam=the process environment
-  test('an inherited GIT_LITERAL_PATHSPECS keeps the same lines; a fractional timeout override is whole milliseconds', () => {
+  test('an inherited GIT_LITERAL_PATHSPECS keeps the same lines; a fractional timeout is whole milliseconds; a spawn error is named', () => {
     const r = mixed();
     withEnv('GIT_LITERAL_PATHSPECS', '1', () => {
       for (const at of [r.head, '']) expect((grepConsumers(r.dir, patterns, at, () => true, Date.now() + 10_000) as string[]).filter(Boolean).sort()).toEqual(wholeTree(r.dir, at));
     });
     withEnv('GSTACK_CONSUMER_SCAN_TIMEOUT_MS', '1500.5', () => expect(consumerScanTimeoutMs()).toBe(1500));
+    const errors: Parameters<typeof scanConsumers>[5] = [];
+    withEnv('PATH', path.join(r.dir, 'no-bin'), () => expect(scanConsumers(r.dir, ['data/rows.json'], [r.head], false, r.dir, errors, 10_000)).toBe('all'));
+    expect(errors[0]!.detail).toMatch(/^git grep exited null: .*ENOENT/);
   });
+  // Value: protects=a consumer whose name is not valid UTF-8 still counts, and a file with a million references does not crash the
+  //   planner; fails_when=the -l pass hands back a mangled name as a pathspec, or the matches are spread into one call;
+  //   seam=git tree objects and git grep output size
+  test('a consumer named with invalid UTF-8 bytes still counts; a million matches in one file are all returned', () => {
+    const r = repo({ 'src/keep.ts': 'x\n' });
+    const head = r.commit({ 'data/results.json': rows });
+    const run = (args: string[], input?: Buffer) => { const x = spawnSync('git', args, { cwd: r.dir, input, timeout: 10_000 }); if (x.status !== 0) throw new Error(`git ${args.join(' ')}: ${x.stderr}`); return x.stdout as Buffer; };
+    const sha = run(['hash-object', '-w', '--stdin'], Buffer.from("read('results.json')\n")).toString().trim();
+    const entry = Buffer.concat([Buffer.from(`100644 blob ${sha}\tloader`), Buffer.from([0xff]), Buffer.from('.py\0')]);
+    const tree = run(['mktree', '-z'], Buffer.concat([run(['ls-tree', '-z', head]), entry])).toString().trim();
+    const named = run(['commit-tree', tree, '-p', head, '-m', 'consumer'], Buffer.alloc(0)).toString().trim();
+    expect(collectCommit(r.dir, r.baseSha, named).consumed).toEqual(['data/results.json']);
+    r.write({ 'src/many.ts': 'rows.json\n'.repeat(1_100_000) });
+    const got = grepConsumers(r.dir, ['rows.json'], '', () => true, Date.now() + 30_000);
+    expect(Array.isArray(got) && got.filter(Boolean).length).toBe(1_100_000);
+  }, 30_000);
   // Value: protects=a slow scan ends in its own fail-closed consumer_scan_failed, not in the parent killing gstack-review-plan;
   //   fails_when=the scan deadline reaches the timeout the dispatch and preflight callers give the whole plan; seam=caller source
   test('the default scan deadline leaves a minute of the plan timeout its callers set', () => {