From 022dfaeb91576c8b862d72fde9b95d7bfb6d3753 Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Tue, 3 Nov 2026 17:23:43 -0800 Subject: [PATCH] v1.94.41.0 fix(review-scope): leave an abandoned review pass out of the receipt counts (GST-1168, GBEV-3) (#3931) * fix(review-scope): an abandoned review pass is left out of the receipt's counts, by name (GST-1168, GBEV-3) A pass whose dispatch ledger was never judged and that no review-log row names (the pass a lane abandoned and restarted) no longer adds its collected findings to blocking_found and unrecorded; the summary prints REVIEW_SCOPE_VOIDED for it. A judged pass with no row stays unbound, as before. Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * fix(review-scope): void only a pass a later pass replaced, never a fix-check ledger (review pass 1) A pN ledger never judged and named by no row is voided only when a later pM pass (M > N) was judged or is named; a fix-check ledger and the last pass keep counting, so a skipped judge can no longer hide a final pass's blockers or drop a rowless fix-check. docs/review-policy.md names the REVIEW_SCOPE_VOIDED line. Review-Finding: p1c/specialist-batched#1 reader=specialist-batched Review-Finding: p1c/red-team#1 reader=red-team Review-Finding: p1c/red-team#2 reader=red-team Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * changelog: name when a pass is voided (GBEV-3) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --------- Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- bin/gstack-review-scope | 1 + changelog.d/gsta258-l4-voided-pass.md | 1 + docs/review-policy.md | 1 + lib/review-scope.ts | 19 +++++++++++++--- test/review-loop-binding.test.ts | 31 +++++++++++++++++++++++++++ 5 files changed, 50 insertions(+), 3 deletions(-) create mode 100644 changelog.d/gsta258-l4-voided-pass.md diff --git a/bin/gstack-review-scope b/bin/gstack-review-scope index baf2e7ae2..170510084 100755 --- a/bin/gstack-review-scope +++ b/bin/gstack-review-scope @@ -278,6 +278,7 @@ function cmdSummary(rest: string[], json: boolean): number { const trailers = log.stdout.split('\x1e').flatMap(rec => { const [sha, values] = rec.trim().split('\0'); return sha && values ? parseFixTrailers(sha, values.split('\x1f').map(v => v.trim()).filter(Boolean)) : []; }); const summary = summarizeReviewLoop(rows, since, ledgers, files.map(f => ledgerPass(f, keyOf(f))), trailers); for (const u of summary.unbound ?? []) process.stderr.write(`REVIEW_SCOPE_UNBOUND: ${u}\n`); + for (const v of summary.voided ?? []) process.stderr.write(`REVIEW_SCOPE_VOIDED: ${v}\n`); const line = reviewLoopLine(summary); // A /ship invocation always runs Step 9 once; zero matching rows means the wrong --since or a review log written elsewhere, never a loop to record. if (summary.passes === 0) return refuse('summary', 'REVIEW_SCOPE_INPUT', `no review-log row of this branch is a via:ship review at or after ${since}; check REVIEW_SINCE (${line})`); diff --git a/changelog.d/gsta258-l4-voided-pass.md b/changelog.d/gsta258-l4-voided-pass.md new file mode 100644 index 000000000..ef89e625a --- /dev/null +++ b/changelog.d/gsta258-l4-voided-pass.md @@ -0,0 +1 @@ +- **The review summary leaves an abandoned review pass out of the receipt's counts** (`gstack-review-scope summary`, GST-1168). A pass whose dispatch ledger was never judged, that no review-log row names, and that a later judged or logged pass replaced is left out, and the summary names it on a `REVIEW_SCOPE_VOIDED:` line instead of counting its findings as found and unrecorded. A fix-check ledger and the last pass always count. diff --git a/docs/review-policy.md b/docs/review-policy.md index 3f7411490..2956ed17e 100644 --- a/docs/review-policy.md +++ b/docs/review-policy.md @@ -510,6 +510,7 @@ puts in the receipt: | `review_loop.unrecorded` | blockers found that no row gives a disposition (fixed, deferred, noted, skipped, a gate answer); absent when 0 | | `review_loop.unbound` | what could not bind (GST-1168), each printed as `REVIEW_SCOPE_UNBOUND: : `: a row with no start token and no readable time, a `pN` ledger with no review row of pass N, a row finding's `ledger_id` no ledger collected, a `Review-Finding` trailer naming nothing; absent when 0 | | `review_loop.ledgers_read` | dispatch ledgers the count read; absent under `--no-ledger` | +| `REVIEW_SCOPE_VOIDED:` (stderr, not a receipt field) | an abandoned pass (GBEV-3): a `pN` ledger never judged that no row names, replaced by a later `pM` ledger (M > N) that was judged or is named. Its findings are left out of every count above instead of counting as unbound and unrecorded. A fix-check ledger and the last pass are never voided | Findings bind by id first (GST-1168). A row finding that came from a dispatched reader carries `ledger_id` (`/`, such as `p2/adversarial-native#3`) and pairs with that ledger entry whatever its path:line says, so a native finding with no path:line, or a row logged after its fix moved the line, still pairs. Each fix commit names what it fixed in a `Review-Finding: reader=` trailer, and a trailer counts the finding as fixed. A late defer re-logs its finding under the same id, so it counts once. Rows that record their head dedupe by kind, pass, head and finding ids, so a logger command run twice counts once. Rows and findings without an id keep the path:line pairing above. The native reader's own verdict line ("all FIXABLE, and none blocks", with no file:line, list mark, lead label or Severity/Blocks field) collects at FIXABLE, not CRITICAL; the findings it summarizes collect on their own lines. | `review_loop.deferred` | blocking findings in unchanged code filed as issues (a Step 11 late defer, logged as a `review-late-defer` row so it never displaces the newest `review` pass, counts here and is not a pass) | diff --git a/lib/review-scope.ts b/lib/review-scope.ts index a2bb74aa9..ec15bce11 100644 --- a/lib/review-scope.ts +++ b/lib/review-scope.ts @@ -316,6 +316,8 @@ export interface ReviewLoopSummary { ledgers_read?: number; /** What could not bind (GST-1168), each named with its reason: a row with no start token or readable time, a row finding's ledger_id no ledger holds, a Review-Finding trailer naming no finding. Never counted as zero silently. */ unbound?: string[]; + /** Abandoned passes left out of every count, each named (GBEV-3). */ + voided?: string[]; } /** Review-log rows of one /ship invocation (skill review, via ship, at or after `since`) → the loop summary. */ @@ -379,13 +381,16 @@ function rowLocus(f: Record): string | null { * When a dispatch ledger started (its first event) and whether it holds a fix-check reader's review: a fix-check run through * gstack-review-dispatch (the pass program's `fix-check` step) leaves this ledger even when no review-fix-check row was logged. */ -export interface LedgerPass { started: number; fixCheck: boolean; ids?: string[]; /** The ledger's pass directory (p1, p2, fc): a pN with no review row of pass N is named unbound. */ name?: string } +export interface LedgerPass { + started: number; fixCheck: boolean; ids?: string[]; /** The ledger's pass directory (p1, p2, fc): a pN with no review row of pass N is named unbound. */ name?: string; + /** Whether judge ran on it at all (a rejected judgment counts): a pN pass never judged that no row names, replaced by a later judged or named pass, was abandoned (voided) and counts nothing (GBEV-3); a fix-check ledger and the last pass are never voided. */ judged?: boolean; +} export function ledgerPass(ledger: string, key = (id: string) => id): LedgerPass { const events = readLedger(ledger); const started = Date.parse(String(events[0]?.at ?? '')); const fixCheck = events.some(e => e.event === 'output' && e.reader === 'fix-check' && e.tree_ok !== false && ((Array.isArray(e.ids) && e.ids.length > 0) || e.review === true)); const ids = events.flatMap(e => (e.event === 'output' && Array.isArray(e.ids) ? (e.ids as string[]) : e.event === 'judgment' && e.kind === 'added' && typeof e.id === 'string' ? [e.id] : [])).map(key); - return { started: Number.isFinite(started) ? started : NaN, fixCheck, ids, name: path.basename(path.dirname(path.resolve(ledger))) }; + return { started: Number.isFinite(started) ? started : NaN, fixCheck, ids, name: path.basename(path.dirname(path.resolve(ledger))), judged: events.some(e => e.event === 'judge') }; } /** A fix commit's `Review-Finding: reader=` trailer (receipt attribution): the finding that commit fixed. */ @@ -413,7 +418,7 @@ export function parseFixTrailers(commit: string, values: readonly string[]): Fix * decision 3.5 runs a fix-check only after the loop applied that pass's fixes, and a blocker the loop does not fix is deferred * or noted by a row. A blocker the last pass found stays unrecorded, and with every fix-check logged the rows decide. */ -export function summarizeReviewLoop(rows: readonly Record[], since: string, ledgers: readonly (readonly LedgerBlocker[])[] = [], ledgerPasses: readonly LedgerPass[] = [], trailers: readonly FixTrailer[] = []): ReviewLoopSummary { +export function summarizeReviewLoop(rows: readonly Record[], since: string, allLedgers: readonly (readonly LedgerBlocker[])[] = [], allPasses: readonly LedgerPass[] = [], trailers: readonly FixTrailer[] = []): ReviewLoopSummary { const sinceMs = Date.parse(since); const loopSkill = (r: Record) => r.skill === 'review' || r.skill === LATE_DEFER_SKILL || r.skill === FIX_CHECK_SKILL; const inRun = (r: Record) => r && r.via === 'ship' && Number.isFinite(rowTime(r)) && (!Number.isFinite(sinceMs) || rowTime(r) >= sinceMs); @@ -423,6 +428,13 @@ export function summarizeReviewLoop(rows: readonly Record[], since: const rowKey = (r: Record) => 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'); + const named = (dir: string) => { const n = /^p(\d+)$/.exec(dir)?.[1]; return loopRows.some(r => (n !== undefined && r.skill === 'review' && String(r.pass) === n) || (Array.isArray(r.findings) && r.findings.some((f: any) => typeof f?.ledger_id === 'string' && f.ledger_id.startsWith(`${dir}/`)))); }; + const passNo = (p: LedgerPass) => (p.fixCheck ? undefined : Number(/^p(\d+)$/.exec(p.name ?? '')?.[1] ?? NaN)); + const replaced = (n: number) => allPasses.some(q => (passNo(q) ?? NaN) > n && (q.judged !== false || named(q.name!))); + const voidedAt = new Set(allPasses.flatMap((p, i) => { const n = passNo(p); return n !== undefined && Number.isFinite(n) && p.judged === false && p.ids?.length && !named(p.name!) && replaced(n) ? [i] : []; })); + const voided = [...voidedAt].map(i => `pass ${allPasses[i]!.name}: its ledger was never judged and no review-log row names it (an abandoned pass), so its ${allPasses[i]!.ids!.length} collected findings are left out`); + const ledgers = allLedgers.filter((_, i) => !voidedAt.has(i)); + const ledgerPasses = allPasses.filter((_, i) => !voidedAt.has(i)); type Seen = { locus: string | null; raiser: string | null; classified: boolean; blocks: boolean; candidate: boolean; disposed: boolean; ledgerId?: string; fingerprint?: string }; const seen = new Map(); const deferred = new Set(); @@ -494,6 +506,7 @@ export function summarizeReviewLoop(rows: readonly Record[], since: if (unrecorded) out.unrecorded = unrecorded; if (ledgers.length) out.ledgers_read = ledgers.length; if (unbound.length) out.unbound = unbound; + if (voided.length) out.voided = voided; const fixChecks = Math.max(rowFixChecks, fixCheckStarts.length); if (fixChecks) out.fix_checks = fixChecks; out.rounds = passes.length + fixChecks; diff --git a/test/review-loop-binding.test.ts b/test/review-loop-binding.test.ts index a8c40e84b..03c3871f3 100644 --- a/test/review-loop-binding.test.ts +++ b/test/review-loop-binding.test.ts @@ -59,6 +59,37 @@ describe('review loop counts bind by finding id (GST-1168)', () => { ]); }); + test('GBEV-3: a voided pass (its ledger never judged, no row names it) is left out by name, and the restarted pass counts alone', () => { + const rows = [row('review', 30, [{ fingerprint: 'src/a.ts:10:correctness', severity: 'CRITICAL', action: 'fixed', blocking: true, raised_by: 'red-team', ledger_id: 'p2/red-team#1' }], { pass: 2 })]; + const ledgers = [[blocker('p1', 'red-team#1', 'src/a.ts:10'), blocker('p1', 'red-team#2', 'src/c.ts:5')], [blocker('p2', 'red-team#1', 'src/a.ts:10')]]; + const passes = [{ ...pass(0, ['p1/red-team#1', 'p1/red-team#2']), name: 'p1', judged: false }, { ...pass(20, ['p2/red-team#1']), name: 'p2', judged: true }]; + const s = summarizeReviewLoop(rows, SINCE, ledgers, passes); + expect({ found: s.blocking_found, fixed: s.fixed, unrecorded: s.unrecorded, unbound: s.unbound }).toEqual({ found: 1, fixed: 1, unrecorded: undefined, unbound: undefined }); + expect(s.voided).toEqual(['pass p1: its ledger was never judged and no review-log row names it (an abandoned pass), so its 2 collected findings are left out']); + }); + + test('an unjudged fix-check ledger with no row still counts as a fix-check, never voided', () => { + const rows = [row('review', 20, [], { pass: 1 })]; + const passes = [{ ...pass(0, ['p1/red-team#1']), name: 'p1', judged: true }, { ...pass(40, ['fc/fix-check#1'], true), name: 'fc', judged: false }]; + const s = summarizeReviewLoop(rows, SINCE, [[], []], passes); + expect(s.voided).toBeUndefined(); + expect(s.fix_checks).toBe(1); + }); + + test('the last pass is never voided: an unjudged pass no later pass replaced keeps its blockers', () => { + const ledgers = [[blocker('p1', 'red-team#1', 'src/a.ts:10')]]; + const s = summarizeReviewLoop([], SINCE, ledgers, [{ ...pass(0, ['p1/red-team#1']), name: 'p1', judged: false }]); + expect(s.voided).toBeUndefined(); + expect(s.blocking_found).toBe(1); + }); + + test('a judged pass with no row is still named unbound, never voided', () => { + const rows = [row('review', 30, [], { pass: 2 })]; + const s = summarizeReviewLoop(rows, SINCE, [[blocker('p1', 'red-team#1', 'src/a.ts:10')], []], [{ ...pass(0, ['p1/red-team#1']), name: 'p1', judged: true }, { ...pass(20, []), name: 'p2', judged: true }]); + expect(s.voided).toBeUndefined(); + expect(s.unbound).toEqual(['pass 1 (ledger p1): no review-log row, so its fixes and dispositions cannot be counted']); + }); + test('#3911 at 4340ed4c: a fix-check row written twice counts once, and pass 1 with no row is named', () => { const fc = { skill: 'review-fix-check', via: 'ship', pass: 2, commit: '4340ed4c', review_binding: { started_at: at(40) }, findings: [{ fingerprint: 'lib/p.ts:9:correctness', severity: 'CRITICAL', action: 'fixed', blocking: true, raised_by: 'fix-check', ledger_id: 'fc/fix-check#1' }] }; const rows = [row('review', 20, [], { pass: 2, commit: '1111111' }), fc, { ...fc }];