fix: Step 11 review fixes (executables and symlinks are code with their lines read whatever their docs, data or config name; never open a symlink)

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

lib/review-collect.ts

@@ -234,16 +234,16 @@ function drawingsAmong(perPath: readonly PathLines[], classRoot: string | readon
 }
 
 /**
- * The changed paths the data rules could move to docs (a data or run-log name) that are executables: the exec bit, or text
+ * The changed paths a docs, data or run-log rule could move out of code that are executables: the exec bit, a symlink, or text
  * that starts with "#!" (GSTA-258 L7). `isExec` answers for one path and fails closed (true) when it cannot read it.
  */
 function executablesAmong(perPath: readonly PathLines[], classRoot: string | readonly string[], isExec: (e: PathLines) => boolean): string[] {
-  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();
+  return perPath.filter(e => (BINARY_DATA_PATH.test(e.path) && classifyPath(e.path, classRoot) === 'code') || dataCandidate(e.path, classRoot) || svgCandidate(e.path, classRoot) || classifyPath(e.path, classRoot) === 'docs').filter(isExec).map(e => e.path).sort();
 }
 
 /**
- * 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.
+ * The executables among the changed paths at `headSha`: tree mode 100755 or 120000 (a symlink, as collectLive sees it), or a text
+ * blob whose first two bytes are "#!". 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]);
@@ -252,7 +252,7 @@ function executablesAt(root: string, headSha: string, perPath: readonly PathLine
     if (!tree.ok) return true;
     const t = entries.get(e.path);
     if (!t) return false;
-    if (t.mode === '100755') return true;
+    if (t.mode === '100755' || t.mode === '120000') return true;
     if (e.binary) return false;
     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';
@@ -322,7 +322,8 @@ export function collectLive(root: string, base: string): Collected {
   const executables = executablesAmong(diff.perPath, root, e => {
     try {
       const f = path.join(root, e.path);
-      if (fs.statSync(f).mode & 0o111) return true;
+      const st = fs.lstatSync(f);
+      if (!st.isFile() || st.mode & 0o111) return true;
       if (e.binary) return false;
       const fd = fs.openSync(f, 'r');
       const buf = Buffer.alloc(2);

lib/review-depth.ts

@@ -237,7 +237,7 @@ export interface ClassInput {
   created?: readonly string[];
   /** The SVG candidates (svgCandidate) whose whole text the collector read as a drawing (svgDrawing); any other SVG stays code. */
   drawings?: readonly string[];
-  /** Data- or log-named paths the collector found executable (exec bit or a "#!" first line): they stay code whatever their name or directory. */
+  /** Docs-, data- or log-named paths the collector found executable (exec bit, a symlink or a "#!" first line): code whatever their name or directory, never judged by path alone. */
   executables?: readonly string[];
   /** Added and deleted lines per path (lib/review-collect.ts): the risk classes read them; without them no line signal applies. */
   added?: Record<string, readonly string[]>;
@@ -256,7 +256,7 @@ const sanitize = (s: string) => s.replace(/[\0-\x1f]+/g, ' ').slice(0, 200);
 const isOutput = (i: ClassInput, p: string) => !!i.registry?.outputs.length && matchesAny(i.registry.outputs, p);
 const unconsumedData = (i: ClassInput, p: string) => !i.executables?.includes(p) && Array.isArray(i.consumed) && !i.consumed.includes(p) && (dataCandidate(p, i.root) || (!!i.drawings?.includes(p) && svgCandidate(p, i.root)));
 const binaryData = (i: ClassInput, p: string) => !i.executables?.includes(p) && BINARY_DATA_PATH.test(p) && classifyPath(p, i.root) === 'code' && ![...AGENT_PROSE, ...GATE_PATH, ...DATA_CONFIG].some(r => r.test(p));
-const pathClass = (i: ClassInput, p: string): PathClass => (i.excluded?.has(p) || (i.stampOnly ?? []).includes(p) || unconsumedData(i, p) || binaryData(i, p) ? 'docs' : classifyPath(p, i.root));
+const pathClass = (i: ClassInput, p: string): PathClass => i.executables?.includes(p) && !i.excluded?.has(p) ? 'code' : (i.excluded?.has(p) || (i.stampOnly ?? []).includes(p) || unconsumedData(i, p) || binaryData(i, p) ? 'docs' : classifyPath(p, i.root));
 
 /** The diff's class, raised to gate by any gate signal; `errors` lists every input that fails the depth closed; `runtimeLines` counts code-class lines outside the base registry's outputs. */
 export function classifyDiff(i: ClassInput): ClassResult {
@@ -287,7 +287,7 @@ export function classifyDiff(i: ClassInput): ClassResult {
     for (const p of i.paths) if (GATE_PATH.some(r => r.test(p))) { raise('gate', `gate path: ${p}`); break; }
     if (i.registry?.error) errors.push(i.registry.error);
   }
-  const riskHits = CLASS_RANK[cls] >= CLASS_RANK.code ? matchRisk({ codePaths: i.paths.filter(p => pathClass(i, p) === 'code'), added: i.added, removed: i.removed, scopeAuth: s.SCOPE_AUTH === 'true', ...(i.riskPaths?.length ? { isRiskPath: (p: string) => matchesAny(i.riskPaths!, p) && !DATA_PATH.test(p) } : {}) }) : new Map<RiskClass, string>();
+  const riskHits = CLASS_RANK[cls] >= CLASS_RANK.code ? matchRisk({ codePaths: i.paths.filter(p => pathClass(i, p) === 'code'), executables: i.executables, added: i.added, removed: i.removed, scopeAuth: s.SCOPE_AUTH === 'true', ...(i.riskPaths?.length ? { isRiskPath: (p: string) => matchesAny(i.riskPaths!, p) && !DATA_PATH.test(p) } : {}) }) : new Map<RiskClass, string>();
   const docSecret = i.paths.find(p => pathClass(i, p) === 'docs' && DOC_PATH.test(p) && (i.added?.[p] ?? []).some(l => DOC_SECRET_LINE.some(r => r.test(l))));
   if (docSecret && !riskHits.has('security')) riskHits.set('security', `docs that can print a credential: ${docSecret}`);
   const byRank = (a: RiskClass, b: RiskClass) => RISK_CLASSES.indexOf(a) - RISK_CLASSES.indexOf(b);

lib/review-risk.ts

@@ -134,6 +134,8 @@ export interface RiskInput {
   removed?: Record<string, readonly string[]>;
   /** gstack-diff-scope's SCOPE_AUTH: the auth class needs it and an auth token in the path. */
   scopeAuth: boolean;
+  /** Executables (lib/review-collect.ts): never a config file or results script, so their lines are always read. */
+  executables?: readonly string[];
   /** The base policy's review.risk_paths matcher: a code path it matches is risk class `policy`. */
   isRiskPath?: (p: string) => boolean;
 }
@@ -144,9 +146,10 @@ export function matchRisk(i: RiskInput): Map<RiskClass, string> {
   const hit = (c: RiskClass, reason: string) => { if (!why.has(c)) why.set(c, reason); };
   for (const p of i.codePaths) {
     if (i.isRiskPath?.(p)) hit('policy', 'risk_paths: ' + p);
-    const stats = STATS_SCRIPT.test(p);
+    const exe = !!i.executables?.includes(p);
+    const stats = !exe && STATS_SCRIPT.test(p);
     const t = stats ? [] : tokens(p);
-    const config = CONFIG_DATA_PATH.test(p);
+    const config = !exe && CONFIG_DATA_PATH.test(p);
     const pathOnly = config || stats;
     const prose = PROSE_PATH.test(p);
     if (i.scopeAuth && t.some(x => AUTH_PATH_TOKENS.includes(x) || AUTH_PATH_PREFIXES.some(a => x.startsWith(a)))) hit('auth', 'auth path: ' + p);

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

@@ -1 +1 @@
-{"sha":"17617e1ecfc9cf91cd1df564363a67439bb1d451e07e98d38350b79905ec49ec","replay":"GSTA-258-L7"}
+{"sha":"2c08a34c47094a808963e87e6a5eb95e8529d6f68b1896174b8dfa613562468e","replay":"GSTA-258-L7"}

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

@@ -1 +1 @@
-{"sha":"2b28089481c2209d319d8bd9e7a5feb4045a50aae6bccee1ca6c450804af87cc","replay":"GSTA-258-L7"}
+{"sha":"1d03d5cd8f0cae5f01af85502004849c9fa2545009957f2c4c44a1359d9d0fc0","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 (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)"}
+{"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 an executable or symlink is code with its lines read whatever its docs, data or config name) 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 ; reruns after the pass 1 and Step 11 review fixes give the same 200 rows (GStack Minder drive handoffs/GSTA-258/L7-planner-noloss/, l7-200-v2.tsv, l7-200-v3.tsv)"}

test/review-collect.test.ts

@@ -130,6 +130,23 @@ describe('GSTA-258 L7 Addendum 1 and 7: executables among data-named paths', ()
     r.write({ 'r.log': null });
     expect(collectLive(r.dir, 'main').executables).toEqual([]);
   });
+  // Value: protects=a docs or config name never hides an executable's lines, and a symlink is never opened (outside voice on #3972);
+  //   fails_when=a 755 prune.txt or prune.json with rm -rf classes light with no security, or a logs/ symlink is read through;
+  //   seam=collectors -> classifyDiff -> matchRisk
+  test('executable .txt and .json scripts are code with security lines; a symlink counts as executable without being read', () => {
+    const r = repo({});
+    const script = '#!/bin/sh\nrm -rf "$1"\n';
+    r.write({ 'scripts/logs/prune.txt': script, 'tools/prune.json': script });
+    for (const p of ['scripts/logs/prune.txt', 'tools/prune.json']) fs.chmodSync(path.join(r.dir, p), 0o755);
+    fs.mkdirSync(path.join(r.dir, 'logs'));
+    fs.symlinkSync('/dev/null', path.join(r.dir, 'logs/run'));
+    const head = r.commit({});
+    for (const c of [collectCommit(r.dir, r.baseSha, head), collectLive(r.dir, 'main')]) {
+      expect(c.executables).toEqual(['logs/run', 'scripts/logs/prune.txt', 'tools/prune.json']);
+      const d = depthOfCommitInput({ ...c, gateTagged: 0 });
+      expect([d.class, d.risk.includes('security'), d.depth]).toEqual(['code', true, 'full']);
+    }
+  });
 });
 
 describe('collectLive: the working tree', () => {