diff --git a/lib/review-collect.ts b/lib/review-collect.ts index ab70cedac..d0452b165 100644 --- a/lib/review-collect.ts +++ b/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(); - 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 }; } diff --git a/lib/review-risk.ts b/lib/review-risk.ts index 5535a4243..ef32d4512 100644 --- a/lib/review-risk.ts +++ b/lib/review-risk.ts @@ -68,7 +68,8 @@ export const SECURITY_LINE = [ /(? { + 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', () => { diff --git a/test/review-depth-lean.test.ts b/test/review-depth-lean.test.ts index 9badbbe60..a1429c6bf 100644 --- a/test/review-depth-lean.test.ts +++ b/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'); });