From e4043dc6af457b4dcc49e688c6393c548d84789b Mon Sep 17 00:00:00 2001 From: garrytan Date: Mon, 2 Nov 2026 10:22:21 +0000 Subject: [PATCH] wave review-53cfb5bfe011: /ship pass 2 fixes - lib/review-exclude.ts: binaryContent reads each blob's first 8,000 bytes (git cat-file blob, 8 KB buffer) instead of one --batch read capped at 256 MB, so head mode (receipt, carry, queue) and the plan agree past 256 MB of blobs; an unreadable blob is kept, as in live mode. Failing-first test with two 270 MB blobs. - lib/review-bots.ts: a file-level bot review comment (subject_type file, no line position) is triaged instead of read as outdated. Failing-first test. - test/ship-queue-land.test.ts: the merged-resume exclusion test runs its gate scan under the fixture state root, not the real home (CI free-suite 6 home containment). --- lib/review-bots.ts | 4 ++-- lib/review-exclude.ts | 17 +++++------------ test/review-bots.test.ts | 10 ++++++++++ test/review-exclude.test.ts | 22 ++++++++++++++++++++++ test/ship-queue-land.test.ts | 7 ++++++- 5 files changed, 45 insertions(+), 15 deletions(-) diff --git a/lib/review-bots.ts b/lib/review-bots.ts index 1b8e2efe4..94db9baf4 100644 --- a/lib/review-bots.ts +++ b/lib/review-bots.ts @@ -53,7 +53,7 @@ export interface BotComment { interface GhUser { login?: string; type?: string } interface GhItem { id: number; user?: GhUser | null; body?: string | null; html_url?: string; path?: string; - line?: number | null; position?: number | null; in_reply_to_id?: number | null; submitted_at?: string | null; + line?: number | null; position?: number | null; in_reply_to_id?: number | null; submitted_at?: string | null; subject_type?: string | null; } interface HistoryRow { repo: string; type: string; pattern: string; category: string; bot: string } @@ -144,7 +144,7 @@ function fetch(env: ReviewBotsEnv, args: string[]): number { records.push({ id: item.id, bot, source, url: item.html_url ?? "", body: item.body ?? "", suppress_candidates: [], ...extra }); for (const c of line as GhItem[]) { const bot = ownerOf(c, bots); - if (bot && c.in_reply_to_id == null && c.position != null) add(c, bot, "line", { path: c.path, line: c.line ?? null }); + if (bot && c.in_reply_to_id == null && (c.position != null || c.subject_type === "file")) add(c, bot, "line", { path: c.path, line: c.line ?? null }); } for (const c of top as GhItem[]) { const bot = ownerOf(c, bots); diff --git a/lib/review-exclude.ts b/lib/review-exclude.ts index cc461a7c1..80533bc7d 100644 --- a/lib/review-exclude.ts +++ b/lib/review-exclude.ts @@ -141,18 +141,11 @@ function binaryContent(i: ExcludeInput, files: ReadonlyMap, cla const sha = real(m.newSha) ? m.newSha : real(m.oldSha) ? m.oldSha : ''; if (sha) bySha.set(sha, [...(bySha.get(sha) ?? []), p]); } - if (!bySha.size) return out; - const r = spawnSync('git', ['cat-file', '--batch'], { cwd: i.root, input: [...bySha.keys()].join('\n') + '\n', timeout: 120_000, maxBuffer: MAX_BUFFER }); - const buf = r.stdout ?? Buffer.alloc(0); - let at = 0; - while (at < buf.length) { - const nl = buf.indexOf(10, at); - if (nl < 0) break; - const [sha, , size] = buf.subarray(at, nl).toString('utf8').split(' '); - const n = Number(size); - if (!Number.isFinite(n)) { at = nl + 1; continue; } - if (buf.subarray(nl + 1, nl + 1 + Math.min(n, 8000)).includes(0)) for (const p of bySha.get(sha!) ?? []) out.add(p); - at = nl + 1 + n + 1; + for (const [sha, paths] of bySha) { + const r = spawnSync('git', ['cat-file', 'blob', sha], { cwd: i.root, timeout: 30_000, maxBuffer: 8000 }); + const read = r.status === 0 || (r.error as NodeJS.ErrnoException | undefined)?.code === 'ENOBUFS'; + const binary = read && (r.stdout ?? Buffer.alloc(0)).subarray(0, 8000).includes(0); + for (const p of paths) if (!read) unread.add(p); else if (binary) out.add(p); } return out; } diff --git a/test/review-bots.test.ts b/test/review-bots.test.ts index 55def5a37..43be4a5b9 100644 --- a/test/review-bots.test.ts +++ b/test/review-bots.test.ts @@ -109,6 +109,16 @@ describe("fetch", () => { expect(r.calls.filter(c => c[0] === "gh" && c[1] === "api").every(c => c.includes("--paginate") && !c.includes("--slurp"))).toBe(true); }); + test("a file-level bot comment (no line position) is triaged; an outdated line comment is not", () => { + const r = run({ line: [[ + { id: 1, user: bot("coderabbitai[bot]"), path: "auth.ts", line: null, position: null, subject_type: "file", html_url: "u1", body: "missing authorization check" }, + { id: 2, user: bot("coderabbitai[bot]"), path: "a.ts", line: 4, position: null, subject_type: "line", html_url: "u2", body: "outdated" }, + ]] }, "fetch"); + expect(r.code).toBe(0); + expect(r.records.map(c => [c.id, c.source, c.path])).toEqual([[1, "line", "auth.ts"]]); + expect(r.text).toContain("coderabbit=1"); + }); + test("humans named like a bot and deleted accounts are never triaged", () => { const r = run({ top: [[ { id: 1, user: human("greptile-ci"), body: "fp please" }, diff --git a/test/review-exclude.test.ts b/test/review-exclude.test.ts index 34e046931..a93862931 100644 --- a/test/review-exclude.test.ts +++ b/test/review-exclude.test.ts @@ -359,5 +359,27 @@ describe('a branch cannot hide code under a base pattern', () => { expect(x.kept).toContainEqual({ path: 'receipts/a/shot.png', reason: 'consumer-scan-unavailable' }); } finally { fs.chmodSync(gz, 0o644); } }, 120_000); + + // Value: protects=the receipt (head blobs) and the plan (live files) agree on binary content past 256 MB of blobs; fails_when= + // one git cat-file --batch read stops at its buffer and later blobs fall through to kept code, so the digests differ and + // receipt write refuses on every rerun; why_new=/ship review of the GST-508 wave (Red Team, reproduced at 280 MB); seam=binaryContent + test('head mode reads each blob\'s first 8,000 bytes, so blobs after 256 MB of others are still binary', () => { + const repo = census(); + fs.mkdirSync(path.join(repo.work, 'receipts/big'), { recursive: true }); + const big = Buffer.alloc(270 * 1024 * 1024); + fs.writeFileSync(path.join(repo.work, 'receipts/big/a.dat'), big); + big[big.length - 1] = 1; + fs.writeFileSync(path.join(repo.work, 'receipts/big/c.dat'), big); + fs.writeFileSync(path.join(repo.work, 'receipts/big/b.dat'), GZ); + commit(repo, {}); + const head = repo.git('rev-parse', 'HEAD'); + const baseSha = repo.git('rev-parse', 'origin/main'); + const c = collectCommit(repo.work, baseSha, head); + const atHead = reviewExclude({ root: repo.work, baseSha, ref: 'origin/main', mergeBase: c.mergeBase!, head, perPath: c.perPath, added: c.added, classRoot: c.root ?? [], gateTagged: new Set(), context: 'test' }); + const l = collectLive(repo.work, 'origin/main'); + const live = reviewExclude({ root: repo.work, baseSha, ref: 'origin/main', mergeBase: l.mergeBase!, perPath: l.perPath, added: l.added, classRoot: repo.work, gateTagged: new Set(), context: 'test' }); + expect(atHead.state === 'active' && [...atHead.excluded].filter(p => p.startsWith('receipts/big/')).sort()).toEqual(['receipts/big/a.dat', 'receipts/big/b.dat', 'receipts/big/c.dat']); + expect(atHead.state === 'active' && live.state === 'active' && atHead.record.digest).toBe(live.state === 'active' ? live.record.digest : 'none'); + }, 180_000); }); diff --git a/test/ship-queue-land.test.ts b/test/ship-queue-land.test.ts index e1ed4fea4..18aab27ce 100644 --- a/test/ship-queue-land.test.ts +++ b/test/ship-queue-land.test.ts @@ -591,7 +591,12 @@ describe('land: one pass, base, lock and history', () => { const fx = setup({ ...POLICY, history: 'merge-only', review_exclude_paths: ['data/**'], min_gstack_version: REVIEW_EXCLUDE_KEY_VERSION }); const head = openPr(fx, '15', { edit: work => { fs.mkdirSync(path.join(work, 'data', 'a'), { recursive: true }); fs.writeFileSync(path.join(work, 'data', 'a', 'result.json'), '{"ok": true}\n'); } }); fx.repo.git('checkout', '-q', '--detach', head); - const { depth, exclude } = receiptReviewAtHead(fx.repo.work, 'main', path.join(ROOT, 'bin'), head); + const prior = process.env.GSTACK_STATE_ROOT; + process.env.GSTACK_STATE_ROOT = path.join(fx.repo.root, 'ready-state'); + let review: ReturnType; + try { review = receiptReviewAtHead(fx.repo.work, 'main', path.join(ROOT, 'bin'), head); } + finally { if (prior === undefined) delete process.env.GSTACK_STATE_ROOT; else process.env.GSTACK_STATE_ROOT = prior; } + const { depth, exclude } = review; fx.repo.git('checkout', '-q', '--detach', 'origin/main'); const recorded = receiptExclude(exclude); expect(recorded?.excluded).toBe(1);