mirror of
https://github.com/garrytan/gstack.git
synced 2026-11-04 07:47:27 +01:00
fix: pre-landing review fixes (Step 11: re-check refuses binary paths and packets over 64 KB; fingerprint trailers bind findings with a ledger_id; the runs fence closes only on its own line)
Review-Finding: v/outside-voice#1 reader=outside-voice Review-Finding: v/outside-voice#2 reader=outside-voice Review-Finding: v/adversarial-native#1 reader=adversarial-native Review-Finding: v/targeted#1 reader=targeted Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>
This commit is contained in:
1 parent
8788d24027
commit
64cab9f711
7 files changed
+38
-7
No files matched your search
@@ -765,7 +765,9 @@ function cmdCarry(rest: string[], json: boolean): number {
|
||||
const named = changedProduct.filter(p => !p.startsWith('(') && !p.includes(' ('));
|
||||
const inter = named.length ? spawnSync('git', ['diff', '--no-color', '--no-ext-diff', from, head, '--', ...named.map(p => `:(literal)${p}`)], { cwd: repoRoot, encoding: 'utf8', timeout: 30_000, maxBuffer: 64 * 1024 * 1024 }) : null;
|
||||
const fullRange = rangeRaw.status === 0 && inter?.status === 0 ? `${rangeRaw.stdout.trim()}\n${inter.stdout.trim()}` : null;
|
||||
const r = planRecheck({ code: 'RECEIPT_CARRY_PRODUCT_CHANGED', ancestor, retarget, rangeDiff: fullRange, unnamedPaths: named.length !== changedProduct.length, legacy: !receipt.review_depth, walkCut, contextPaths: contextPaths.length });
|
||||
const numstat = named.length ? spawnSync('git', ['diff', '--numstat', from, head, '--', ...named.map(p => `:(literal)${p}`)], { cwd: repoRoot, encoding: 'utf8', timeout: 30_000, maxBuffer: 16 * 1024 * 1024 }) : null;
|
||||
const binary = numstat?.status !== 0 || (numstat?.stdout ?? '').split('\n').some(l => l.startsWith('-\t-\t'));
|
||||
const r = planRecheck({ code: 'RECEIPT_CARRY_PRODUCT_CHANGED', ancestor, retarget, rangeDiff: fullRange, unnamedPaths: named.length !== changedProduct.length, legacy: !receipt.review_depth, walkCut, contextPaths: contextPaths.length, binary });
|
||||
const record = flag(rest, '--recheck');
|
||||
if (!r.ok) process.stderr.write(`RECHECK: not eligible (${r.reason}); run /ship on ${head.slice(0, 12)}\n`);
|
||||
else if (record) {
|
||||
|
||||
+3
-2
@@ -423,7 +423,7 @@ export function summarizeReviewLoop(rows: readonly Record<string, any>[], since:
|
||||
const rowKey = (r: Record<string, any>) => JSON.stringify([r.skill, r.pass ?? null, r.commit_full ?? r.commit ?? '', (Array.isArray(r.findings) ? r.findings : []).map((f: any) => (f && typeof f === 'object' ? (typeof f.ledger_id === 'string' ? f.ledger_id : findingIdentity(f)) : '')).sort()]);
|
||||
const loopRows = [...new Map(rows.filter(r => inRun(r) && loopSkill(r)).map((r, i) => [r.commit_full || r.commit ? rowKey(r) : `#${i}`, r] as const)).values()];
|
||||
const passes = loopRows.filter(r => r.skill === 'review');
|
||||
type Seen = { locus: string | null; raiser: string | null; classified: boolean; blocks: boolean; candidate: boolean; disposed: boolean; ledgerId?: string };
|
||||
type Seen = { locus: string | null; raiser: string | null; classified: boolean; blocks: boolean; candidate: boolean; disposed: boolean; ledgerId?: string; fingerprint?: string };
|
||||
const seen = new Map<string, Seen>();
|
||||
const deferred = new Set<string>();
|
||||
const noted = new Set<string>();
|
||||
@@ -447,6 +447,7 @@ export function summarizeReviewLoop(rows: readonly Record<string, any>[], since:
|
||||
candidate: !!prior?.candidate || f.severity === undefined || rank(f.severity) === 4 || rank(f.raw_severity) === 4 || f.blocking === true,
|
||||
disposed: !!prior?.disposed || DISPOSITIONS.includes(f.action),
|
||||
...(ledgerId ?? prior?.ledgerId ? { ledgerId: ledgerId ?? prior?.ledgerId } : {}),
|
||||
...(prior?.fingerprint ?? findingIdentity(f) ? { fingerprint: prior?.fingerprint ?? findingIdentity(f) } : {}),
|
||||
});
|
||||
if (f.action === 'deferred') deferred.add(id);
|
||||
if (f.action === 'noted') noted.add(id);
|
||||
@@ -479,7 +480,7 @@ export function summarizeReviewLoop(rows: readonly Record<string, any>[], since:
|
||||
}
|
||||
for (const [id, n] of lastPaired) if (followed(n)) followedIds.add(id);
|
||||
for (const t of trailers) {
|
||||
const hit = [...seen].find(([k, s]) => k === t.finding || s.ledgerId === t.finding);
|
||||
const hit = [...seen].find(([k, s]) => k === t.finding || s.ledgerId === t.finding || s.fingerprint === t.finding);
|
||||
if (hit) { hit[1].disposed = true; fixed.add(hit[0]); continue; }
|
||||
const only = ledgerOnlyKeys.find(l => l.key === t.finding || l.id === t.finding);
|
||||
if (only) { only.fixed = true; continue; }
|
||||
|
||||
+1
-1
@@ -99,7 +99,7 @@ export function bindReceipt(body: string, pr: string | undefined, accepted: read
|
||||
*/
|
||||
export const RUNS_FENCE = 'gstack-receipt-runs';
|
||||
export const RECEIPT_RUNS_KEPT = 30;
|
||||
const RUNS_RE = /```gstack-receipt-runs[ \t]*\r?\n([\s\S]*?)\r?\n?```\n?/;
|
||||
const RUNS_RE = /```gstack-receipt-runs[ \t]*\r?\n([\s\S]*?)\r?\n```[ \t]*(?:\r?\n|$)/;
|
||||
export type ReceiptRun = Pick<ShipReceipt, 'head' | 'version' | 'gstack_version' | 'run' | 'review_loop' | 'review_loop_exception'> & { depth?: string; carried_from?: string };
|
||||
export function receiptRuns(body: string): ReceiptRun[] {
|
||||
const m = RUNS_RE.exec(body);
|
||||
|
||||
+9
-2
@@ -8,19 +8,23 @@
|
||||
* Qualifies: RECEIPT_CARRY_PRODUCT_CHANGED on linear history (the receipt's head is an ancestor of HEAD) against the same base.
|
||||
* Does not qualify: RECEIPT_CARRY_TREE_MISMATCH (rewritten history: the reviewed commits are gone, so there is no range to
|
||||
* diff), a retargeted stack (the reviewed range was against another base), a legacy receipt with no review_depth, more
|
||||
* changed product paths than the carry walk names, a path it cannot name, base context moved next to reviewed hunks,
|
||||
* an unreadable or empty range-diff, a range-diff over RECHECK_RANGE_DIFF_CAP lines, and every RECEIPT_CARRY_REFUSED / RECEIPT_CARRY_PREGATE_STALE (those need their inputs
|
||||
* changed product paths than the carry walk names, a path it cannot name, a binary path, base context moved next to reviewed hunks,
|
||||
* an unreadable or empty range-diff, a packet over RECHECK_RANGE_DIFF_CAP lines or RECHECK_PACKET_BYTES bytes, and every RECEIPT_CARRY_REFUSED / RECEIPT_CARRY_PREGATE_STALE (those need their inputs
|
||||
* repaired, not a review).
|
||||
*/
|
||||
import { createHash } from 'node:crypto';
|
||||
import * as fs from 'node:fs';
|
||||
|
||||
export const RECHECK_RANGE_DIFF_CAP = 400;
|
||||
/** The packet's byte cap: 400 lines of unbounded width (a minified bundle) would not bound the one reader's input. */
|
||||
export const RECHECK_PACKET_BYTES = 64 * 1024;
|
||||
/** rangeDiff: \`git range-diff\` of the two ranges followed by the diff of the changed product paths between the heads (a range-diff names added commits without their content). */
|
||||
export type RecheckInput = {
|
||||
code: string; ancestor: boolean; retarget: boolean; rangeDiff: string | null; unnamedPaths?: boolean;
|
||||
/** The receipt records no review_depth (a legacy receipt), so carry cannot compare HEAD's risk class with the reviewed one. */
|
||||
legacy?: boolean;
|
||||
/** A changed product path is binary, so the packet would show only "Binary files … differ" for it. */
|
||||
binary?: boolean;
|
||||
/** The changed-path walk stopped at CARRY_NAMED_PATHS, so later changed product paths would be missing from the packet. */
|
||||
walkCut?: boolean;
|
||||
/** Paths whose only difference is base context next to reviewed hunks: they need RECEIPT_CARRY_SCOPED_REVIEW's packet, not this one. */
|
||||
@@ -38,12 +42,15 @@ export function planRecheck(i: RecheckInput): RecheckPlan {
|
||||
if (i.legacy) return { ok: false, reason: 'the receipt records no review_depth, so a changed risk class could not be caught' };
|
||||
if (i.walkCut) return { ok: false, reason: 'more changed product paths than the walk names, so the packet would miss some' };
|
||||
if (i.unnamedPaths) return { ok: false, reason: 'a changed product path could not be named, so the packet would miss it' };
|
||||
if (i.binary) return { ok: false, reason: 'a changed product path is binary, so the packet cannot show its contents' };
|
||||
if (i.contextPaths) return { ok: false, reason: 'base context also moved next to reviewed hunks, which needs the scoped review of those hunks' };
|
||||
if (i.rangeDiff === null) return { ok: false, reason: 'git range-diff failed or timed out' };
|
||||
const text = i.rangeDiff.trim();
|
||||
if (!text) return { ok: false, reason: 'git range-diff is empty, so it shows nothing for a reader to check' };
|
||||
const lines = text.split('\n').length;
|
||||
if (lines > RECHECK_RANGE_DIFF_CAP) return { ok: false, reason: `git range-diff is ${lines} lines, over the scoped re-check cap of ${RECHECK_RANGE_DIFF_CAP}` };
|
||||
const bytes = Buffer.byteLength(text);
|
||||
if (bytes > RECHECK_PACKET_BYTES) return { ok: false, reason: `the packet is ${bytes} bytes, over the scoped re-check cap of ${RECHECK_PACKET_BYTES}` };
|
||||
return { ok: true, lines, sha256: sha(text) };
|
||||
}
|
||||
|
||||
|
||||
@@ -68,6 +68,12 @@ describe('review loop counts bind by finding id (GST-1168)', () => {
|
||||
expect(s.unbound).toEqual(['pass 1 (ledger p1): no review-log row, so its fixes and dispositions cannot be counted']);
|
||||
});
|
||||
|
||||
test('a fingerprint trailer binds a finding that also carries a ledger_id (outside voice #2)', () => {
|
||||
const rows = [row('review', 1, [{ fingerprint: 'src/auth.ts:10:correctness', severity: 'CRITICAL', blocking: true, raised_by: 'red-team', ledger_id: 'p1/red-team#1' }])];
|
||||
const s = summarizeReviewLoop(rows, SINCE, [[blocker('p1', 'red-team#1', 'src/auth.ts:10')]], [pass(0, ['p1/red-team#1'])], parseFixTrailers('abc', ['src/auth.ts:10:correctness reader=red-team']));
|
||||
expect({ fixed: s.fixed, unrecorded: s.unrecorded, unbound: s.unbound }).toEqual({ fixed: 1, unrecorded: undefined, unbound: undefined });
|
||||
});
|
||||
|
||||
test('rows without ledger_id keep pairing by path:line, as before', () => {
|
||||
const rows = [row('review', 1, [{ fingerprint: 'src/a.ts:10:correctness', severity: 'CRITICAL', action: 'fixed', blocking: true, raised_by: 'specialist' }])];
|
||||
const s = summarizeReviewLoop(rows, SINCE, [[blocker('p1', 'specialist/security#1', 'src/a.ts:10')]]);
|
||||
|
||||
@@ -20,6 +20,15 @@ describe('receipt runs block', () => {
|
||||
expect(body.match(/## Summary/g)!.length).toBe(1);
|
||||
});
|
||||
|
||||
test('a fence inside a recorded reason never closes the runs block (adversarial p-voices #1)', () => {
|
||||
const withReason = (head: string) => orderReceipt({ ...receipt(head, { fix_checks: 1 }), review_loop_exception: { accepted_by: 'Garry', date: '2026-11-03', via: 'x', reason: 'the \x60\x60\x60retry\x60\x60\x60 block is fine', fix_check: { head, findings: 0 } } });
|
||||
let body = placeReceiptFirst('## Summary\nprose\n', withReason('b'.repeat(40)));
|
||||
body = placeReceiptFirst(body, withReason('c'.repeat(40)));
|
||||
body = placeReceiptFirst(body, withReason('d'.repeat(40)));
|
||||
expect(receiptRuns(body).map(r => r.head[0])).toEqual(['b', 'c']);
|
||||
expect(body.endsWith('## Summary\nprose\n')).toBe(true);
|
||||
});
|
||||
|
||||
test('rewriting the same receipt adds no run, and the block keeps the newest RECEIPT_RUNS_KEPT', () => {
|
||||
const same = receipt('d'.repeat(40));
|
||||
expect(receiptRuns(placeReceiptFirst(placeReceiptFirst('', same), same))).toEqual([]);
|
||||
|
||||
@@ -11,7 +11,7 @@ import * as os from 'node:os';
|
||||
import * as path from 'node:path';
|
||||
import { EXIT } from '../lib/headless-artifacts';
|
||||
import { extractReceipt, placeReceiptFirst } from '../lib/ship-receipt';
|
||||
import { planRecheck, readRecheck, RECHECK_RANGE_DIFF_CAP } from '../lib/ship-recheck';
|
||||
import { planRecheck, readRecheck, RECHECK_PACKET_BYTES, RECHECK_RANGE_DIFF_CAP } from '../lib/ship-recheck';
|
||||
import { cleanup, makeFixtureRepo, runBin, type FixtureRepo } from './helpers/restamp-fixture';
|
||||
|
||||
const repos: FixtureRepo[] = [];
|
||||
@@ -33,6 +33,8 @@ describe('planRecheck: which refusals qualify', () => {
|
||||
['walk cut at the named-path limit', { ...OK, walkCut: true }, 'more changed product paths'],
|
||||
['unnamed product path', { ...OK, unnamedPaths: true }, 'could not be named'],
|
||||
['base context moved', { ...OK, contextPaths: 1 }, 'base context'],
|
||||
['binary product path', { ...OK, binary: true }, 'binary'],
|
||||
['wide packet over the byte cap', { ...OK, rangeDiff: 'x'.repeat(RECHECK_PACKET_BYTES + 1) }, 'bytes'],
|
||||
['range-diff failed', { ...OK, rangeDiff: null }, 'failed or timed out'],
|
||||
['empty range-diff', { ...OK, rangeDiff: '\n' }, 'empty'],
|
||||
['over the cap', { ...OK, rangeDiff: Array.from({ length: RECHECK_RANGE_DIFF_CAP + 1 }, (_, i) => `l${i}`).join('\n') }, 'over the scoped re-check cap'],
|
||||
@@ -130,6 +132,10 @@ describe('carry with a scoped re-check (CLI)', () => {
|
||||
const wide = reviewed(many);
|
||||
wide.commit(Object.fromEntries(Object.keys(many).map(f => [f, 'export const changed = 2;\n'])), 'fix: all');
|
||||
expect(runBin(wide.repo, 'gstack-ship-receipt', ['carry', '--body', wide.body], wide.env).err).toContain('RECHECK: not eligible (more changed product paths');
|
||||
|
||||
const bin = reviewed({ 'src/app.wasm': '\0asm\x01\0\0\0' });
|
||||
bin.commit({ 'src/app.wasm': '\0asm\x01\0\0\0' + 'x'.repeat(4096) + '\0' }, 'fix: new wasm');
|
||||
expect(runBin(bin.repo, 'gstack-ship-receipt', ['carry', '--body', bin.body], bin.env).err).toContain('RECHECK: not eligible (a changed product path is binary');
|
||||
});
|
||||
|
||||
test('base context moved next to a reviewed hunk plus a product edit elsewhere is not eligible (red-team p1 repro)', () => {
|
||||
|
||||
Reference in new issue
Block a user