fix: pre-landing review fixes (deleted paths are not executables, read two bytes of candidates only, wider egress line rules)

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

lib/review-collect.ts

@@ -241,30 +241,22 @@ function executablesAmong(perPath: readonly PathLines[], classRoot: string | rea
   return perPath.filter(e => (BINARY_DATA_PATH.test(e.path) && classifyPath(e.path, classRoot) === 'code') || dataCandidate(e.path, classRoot) || svgCandidate(e.path, classRoot)).filter(isExec).map(e => e.path).sort();
 }
 
-/** The executables among the changed paths at `headSha`: tree mode 100755, or a text blob that starts with "#!"; an unreadable tree or blob counts as executable. */
+/**
+ * The executables among the changed paths at `headSha`: tree mode 100755, or a text blob whose first two bytes are "#!" (only
+ * those bytes are read). A path the head tree lacks is a deletion, never an executable; an unreadable tree or blob counts as one.
+ */
 function executablesAt(root: string, headSha: string, perPath: readonly PathLines[], classRoot: readonly string[]): string[] {
   const tree = git(root, ['ls-tree', '-r', '-z', headSha]);
   const entries = new Map(tree.out.split('\0').flatMap(rec => { const m = /^(\d+) blob ([0-9a-f]+)\t(.*)$/s.exec(rec); return m ? [[m[3]!, { mode: m[1]!, sha: m[2]! }] as const] : []; }));
-  const firstBytes = new Map<string, string>();
-  const text = perPath.flatMap(e => { const t = entries.get(e.path); return !e.binary && t && t.mode !== '100755' ? [t.sha] : []; });
-  if (text.length) {
-    const r = spawnSync('git', ['cat-file', '--batch'], { cwd: root, input: text.join('\n') + '\n', timeout: 60_000, maxBuffer: 256 * 1024 * 1024 });
-    const out = r.status === 0 && !r.error ? r.stdout : null;
-    for (let at = 0; out && at < out.length;) {
-      const nl = out.indexOf(10, at);
-      const m = nl < 0 ? null : /^([0-9a-f]+) blob (\d+)$/.exec(out.subarray(at, nl).toString('latin1'));
-      if (!m) break;
-      firstBytes.set(m[1]!, out.subarray(nl + 1, nl + 3).toString('latin1'));
-      at = nl + 1 + Number(m[2]) + 1;
-    }
-  }
   return executablesAmong(perPath, classRoot, e => {
+    if (!tree.ok) return true;
     const t = entries.get(e.path);
-    if (!tree.ok || !t) return true;
+    if (!t) return false;
     if (t.mode === '100755') return true;
     if (e.binary) return false;
-    const head = firstBytes.get(t.sha);
-    return head === undefined || head === '#!';
+    const r = spawnSync('git', ['cat-file', 'blob', t.sha], { cwd: root, timeout: 30_000, maxBuffer: 2 });
+    const read = r.status === 0 || (r.error as NodeJS.ErrnoException | undefined)?.code === 'ENOBUFS';
+    return !read || (r.stdout ?? Buffer.alloc(0)).subarray(0, 2).toString('latin1') === '#!';
   });
 }
 
@@ -336,7 +328,7 @@ export function collectLive(root: string, base: string): Collected {
       const buf = Buffer.alloc(2);
       try { fs.readSync(fd, buf, 0, 2, 0); } finally { fs.closeSync(fd); }
       return buf.toString('latin1') === '#!';
-    } catch { return true; }
+    } catch (err) { return (err as NodeJS.ErrnoException).code !== 'ENOENT'; }
   });
   return { ...result(diff, { root, registry, errors, mergeBase, untracked, consumed, drawings, executables }), base: resolved };
 }

lib/review-risk.ts

@@ -68,7 +68,8 @@ export const SECURITY_LINE = [
   /(?<![.\w])eval\s*\(/, /(?:^|[;&|])\s*eval\s+["'$]/, /\bnew\s+Function\s*\(/,
   /\b(?:spawn|spawnSync|execFile|execFileSync)\s*\(\s*['"](?:\/bin\/)?(?:ba|z)?sh['"]/, /\bBun\.spawn(?:Sync)?\s*\(\s*\[\s*['"](?:\/bin\/)?(?:ba|z)?sh['"]/,
   /\brmSync\s*\((?:.{0,500}?recursive\s*:\s*true|[^)]{501})/, /\brm\s*\((?:.{0,500}?recursive\s*:\s*true|[^)]{501})/, /\brm\s+-(?=[a-z]*r)(?=[a-z]*f)[a-z]*[rf]\b/,
-  /\bredact[A-Za-z]{0,40}\s{0,20}\(/, /(?:\bfrom\s{1,20}|\b(?:require|import)\s{0,20}\(\s{0,20})['"][^'"]{0,300}(?:redact|outside-voice)[^'"]{0,300}['"]/, /\b[A-Z0-9_]{0,60}(?:SECRET|CREDENTIAL)[A-Z0-9_]{0,60}\s{0,20}=\s{0,20}\//,
+  /\bredact\w{0,40}\s{0,20}\(/, /(?:\b(?:from|import)\s{1,20}|\b(?:require|import)\s{0,20}\(\s{0,20})['"][^'"]{0,300}(?:redact|outside-voice)[^'"]{0,300}['"]/, /\b(?:from|import)\s{1,20}[\w.]{0,200}(?:redact|outside_voice)/,
+  /\b[A-Z0-9_]{0,60}(?:SECRET|CREDENTIAL)[A-Z0-9_]{0,60}\s{0,20}=\s{0,20}(?:\[\s{0,20})?(?:\/|new\s{1,20}RegExp\b)/,
   /\b(?:innerHTML|outerHTML)\s*\+?=/, /\bdangerouslySetInnerHTML\b/, /\bv-html\b/, /\{@html\b/, /\binsertAdjacentHTML\s*\(/, /\bdocument\.write\s*\(/,
 ];
 /** A recursive delete split across lines (a formatter puts `recursive: true` on its own line), read over a file's changed lines joined. */

test/fixtures/review-pins/files/lib/review-risk.ts.json

@@ -1 +1 @@
-{"sha":"31601b6b07f1be146652ed308f130b4a9688bddc08500d94e03696c372c8f67d","replay":"GSTA-258-L7"}
+{"sha":"2b28089481c2209d319d8bd9e7a5feb4045a50aae6bccee1ca6c450804af87cc","replay":"GSTA-258-L7"}

test/fixtures/review-pins/replays/GSTA-258-L7.json

@@ -1 +1 @@
-{"replay":"GSTA-258-L7","justification":"GSTA-258 L7 planner no-loss check (GST-1177, Addenda 1 and 7): the change only adds triggers (executables stay code; seal, sealed, custody and egress path tokens; redaction calls, redaction or outside-voice imports and SECRET or CREDENTIAL regex constants are security) and removes none; over the last 200 first-parent main commits to bf2cab34c5, planReview and collectCommit from origin/main and from this branch give the same depth, class, risk classes, next step and dispatches on 195, and the other 5 only raise: ed5c948eb6 (#3921, the GST-1164 secret-egress fix) and c197a139b7 (#3918, approval text in public PR bodies) and 514daebf5f (#3569, lib/sealed-guard.ts) go light to full on security, while 90a170c941 and 71acd83aa6 gain security at unchanged full depth and reviewers; no commit's depth drops (GStack Minder drive handoffs/GSTA-258/L7-planner-noloss/)"}
+{"replay":"GSTA-258-L7","justification":"GSTA-258 L7 planner no-loss check (GST-1177, Addenda 1 and 7): the change only adds triggers (executables stay code; seal, sealed, custody and egress path tokens; redaction calls (camelCase or snake_case), redaction or outside-voice imports (from, side-effect, require, dynamic and Python forms) and SECRET or CREDENTIAL constants set to a regex literal, a regex list or new RegExp are security; a deleted file is never an executable) and removes none; over the last 200 first-parent main commits to bf2cab34c5, planReview and collectCommit from origin/main and from this branch give the same depth, class, risk classes, next step and dispatches on 195, and the other 5 only raise: ed5c948eb6 (#3921, the GST-1164 secret-egress fix) and c197a139b7 (#3918, approval text in public PR bodies) and 514daebf5f (#3569, lib/sealed-guard.ts) go light to full on security, while 90a170c941 and 71acd83aa6 gain security at unchanged full depth and reviewers; no commit's depth drops ; rerun after the pass 1 review fixes gives the same 200 rows (GStack Minder drive handoffs/GSTA-258/L7-planner-noloss/, l7-200-v2.tsv)"}

test/review-collect.test.ts

@@ -119,6 +119,17 @@ describe('GSTA-258 L7 Addendum 1 and 7: executables among data-named paths', ()
     expect(pc.executables).toEqual([]);
     expect(depthOfCommitInput({ ...pc, gateTagged: 0 }).class).toBe('docs');
   });
+  // Value: protects=a deleted log is absent, not unreadable, and an unrelated large blob never forces the fail-closed default;
+  //   fails_when=removing logs/run classes as gate, or a big text file in the diff makes r.log executable; seam=executablesAt
+  test('a deleted log is not executable, and only candidates are read (two bytes each)', () => {
+    const r = repo({ 'logs/run': 'ok\n', 'r.log': 'ok\n', 'big.txt': 'a'.repeat(1024 * 1024) });
+    const del = r.commit({ 'logs/run': null, 'r.log': 'ok2\n', 'big.txt': 'b' + 'a'.repeat(1024 * 1024) });
+    const c = collectCommit(r.dir, r.baseSha, del);
+    expect(c.executables).toEqual([]);
+    expect(depthOfCommitInput({ ...c, gateTagged: 0 }).class).toBe('docs');
+    r.write({ 'r.log': null });
+    expect(collectLive(r.dir, 'main').executables).toEqual([]);
+  });
 });
 
 describe('collectLive: the working tree', () => {

test/review-depth-lean.test.ts

@@ -48,7 +48,9 @@ describe('HARN-1 false triggers (harness-evals#231 on 1.93.7.0): data, config an
     expect(sec('lib/autoplan-lean.ts', ['  const redacted = (patch: string) => redactFindingSpans(patch) ?? "";'])).toBe(true);
     expect(sec('lib/autoplan-lean.ts', ['const SECRET_NAME = /(^|\\/)(\\.env[^/]*)$/i;'])).toBe(true);
     expect(sec('lib/review-x.ts', ["import { runOutsideVoice } from './outside-voice-runner';"])).toBe(true);
+    for (const l of ['out = redact_secrets(text)', 'from gstack.redact import scrub', "import './outside-voice';", 'SECRET_PATTERNS = [/sk-[a-z]+/]', 'const CREDENTIAL_RE = new RegExp(src);']) expect(sec('lib/x.ts', [l])).toBe(true);
     expect(sec('lib/autoplan-lean.ts', ['const INLINE_DRAFT_BYTES = 64_000;'])).toBe(false);
+    expect(sec('lib/x.ts', ['const SECRET_COUNT = total / 2;', "import { readFile } from 'node:fs';"])).toBe(false);
     expect(sec('lib/seasonal.ts')).toBe(false);
     expect(classifyDiff({ paths: ['scripts/sealed/seal.ts'], gateTagged: 0 }).risk).toContain('security');
   });