From f00ae61694a6159a5b2fff85d4c37c285b8c91c5 Mon Sep 17 00:00:00 2001 From: garrytan Date: Tue, 3 Nov 2026 04:17:29 +0000 Subject: [PATCH] review: platform evidence only from an approving or commenting review; a lean grant needs the fix-check count; hermetic receipt tests; key version past main's 1.94.0.0 Repins lib/review-depth.ts under GSTA-258-C: the edit touches only the opt-in lean grant (review.panel risk/off), so the default panel-all plan is unchanged. Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- bin/gstack-review-plan | 2 +- lib/review-depth.ts | 2 +- lib/review-platform.ts | 5 +++-- lib/ship-policy.ts | 2 +- test/review-declared-risk.test.ts | 9 +++++++-- test/review-depth-lean.test.ts | 1 + test/review-packets-removed.test.ts | 2 +- test/ship-receipt-platform.test.ts | 24 +++++++++++++++++------- 8 files changed, 32 insertions(+), 15 deletions(-) diff --git a/bin/gstack-review-plan b/bin/gstack-review-plan index bb5e5ab9f..70eabf8fd 100755 --- a/bin/gstack-review-plan +++ b/bin/gstack-review-plan @@ -208,7 +208,7 @@ function main(): number { const plan = planReview({ ...collected, ...exclusionInput, ...riskPaths, declared: declared as RiskClass[], shape: { panel: review.panel, readerModel: review.reader_model, platform: review.platform, head, platformReview: platform?.ok ? platform.review : null }, scope, gateTagged, targeted: { classes: targeted.classes, ran: !['uncommitted', 'none'].includes(patch) && flag('--targeted-ran') === patch }, numbers: scanNumbers(kept(added)), force, gated: list('--gated'), floor, tiers: auditEffort !== 'host', criticalFound: has('--critical-found'), adversarialRan: has('--adversarial-ran'), reviewedNumbers: flag('--reviewed-numbers'), - history: { fullPasses: pass, last, fixedBlocking: has('--fixed-blocking'), fixedBelow: has('--fixed-below') && last !== 'fix-check', blockingOpen: has('--blocking-open'), touchedGate: has('--touched-gate') || !!fixRange?.touched_gate, pass1Depth, rangeRuntimeLines: fixRange?.runtime_lines, granted: grant, fixChecks }, + history: { fullPasses: pass, last, fixedBlocking: has('--fixed-blocking'), fixedBelow: has('--fixed-below') && last !== 'fix-check', blockingOpen: has('--blocking-open'), touchedGate: has('--touched-gate') || !!fixRange?.touched_gate, pass1Depth, rangeRuntimeLines: fixRange?.runtime_lines, granted: grant, fixChecks: has('--fix-checks') ? fixChecks : undefined }, }); const gateScan = { tagged: gateTagged, head }; if (has('--json')) { diff --git a/lib/review-depth.ts b/lib/review-depth.ts index f1fc52688..b94393889 100644 --- a/lib/review-depth.ts +++ b/lib/review-depth.ts @@ -401,7 +401,7 @@ function leanStep(h: History, depth?: Depth): NextStep { if (h.last === 'none') return { step: 'full', reason: 'pass 1: one reviewer reads the whole diff (R65)' }; if (h.last === 'fix-check') { if (!h.blockingOpen && !h.fixedBlocking && !h.touchedGate) return { step: 'done', reason: h.fixedBelow ? 'round cap 2 reached: the below-floor fixes ship as notes in the PR body and receipt' : 'the fix-check found nothing blocking' }; - if (h.granted && !h.blockingOpen && (h.fixChecks ?? 1) < 2) return { step: 'fix-check', reason: `the owner's grant ${h.granted} adds one scoped fix-check after round ${FULL_PASS_CAP} (review_loop_exception), never a second` }; + if (h.granted && !h.blockingOpen && h.fixChecks === 1) return { step: 'fix-check', reason: `the owner's grant ${h.granted} adds one scoped fix-check after round ${FULL_PASS_CAP} (review_loop_exception), never a second` }; return { step: 'stop', reason: `the fix-check ${h.blockingOpen || h.fixedBlocking ? 'found a blocking finding' : 'edited a gate-class path'}: name it for the owner, whose grant may add one scoped fix-check, never a third round` }; } if (h.blockingOpen) return { step: 'stop', reason: 'a blocking finding is still open in fix scope: name it for the owner' }; diff --git a/lib/review-platform.ts b/lib/review-platform.ts index e9949d4e3..5a34366cd 100644 --- a/lib/review-platform.ts +++ b/lib/review-platform.ts @@ -2,7 +2,8 @@ * review-platform — the exact-head evidence that a platform review read a PR (design C6, FOUN-32 on founderstar#496). * Capy's automatic review posts a pull-request review by capy-ai[bot] whose commit_id is the head it read; that review's * id is the run id the plan's planned skip and the receipt's review_depth platform record. Anything else (no PR, no such - * review, a review at another commit, gh unavailable) is no evidence, and the single light reader runs. + * review, a review at another commit, one that requested changes or was dismissed, gh unavailable) is no evidence, and the + * single light reader runs. */ import { spawnSync } from 'node:child_process'; @@ -23,7 +24,7 @@ export function platformReviewAt(root: string, head: string, o: { pr?: string; i if (!list.ok) return { ok: false, reason: `gh cannot list the reviews of PR #${pr}` }; let reviews: Array<{ id?: number; user?: { login?: string }; commit_id?: string; state?: string }>; try { reviews = (JSON.parse(list.out) as unknown[]).flat() as typeof reviews; } catch { return { ok: false, reason: `PR #${pr}'s reviews are not JSON` }; } - const at = reviews.filter(r => r.user?.login === PLATFORM_REVIEWER && r.state !== 'PENDING' && r.commit_id?.toLowerCase() === head.toLowerCase() && r.id !== undefined); + const at = reviews.filter(r => r.user?.login === PLATFORM_REVIEWER && (r.state === 'APPROVED' || r.state === 'COMMENTED') && r.commit_id?.toLowerCase() === head.toLowerCase() && r.id !== undefined); const hit = o.id ? at.find(r => String(r.id) === o.id) : at.at(-1); if (!hit) return { ok: false, reason: o.id ? `review ${o.id} on PR #${pr} is not a ${PLATFORM_REVIEWER} review at ${head.slice(0, 12)}` : `no ${PLATFORM_REVIEWER} review of PR #${pr} at ${head.slice(0, 12)}` }; return { ok: true, review: { id: String(hit.id), head: head.toLowerCase(), reviewer: PLATFORM_REVIEWER } }; diff --git a/lib/ship-policy.ts b/lib/ship-policy.ts index 5365a69fd..bed67950c 100644 --- a/lib/ship-policy.ts +++ b/lib/ship-policy.ts @@ -241,7 +241,7 @@ export const BASE_RED_KEY_VERSION = '1.91.374.0'; /** `train.gates` and `train.land` are values of the older train key, so compatErrors checks them by value, as for bump: never. */ export const TRAIN_GATES_KEY_VERSION = '1.92.0.0'; export const REVIEW_EXCLUDE_KEY_VERSION = '1.93.33.0'; -export const REVIEW_POLICY_KEY_VERSION = '1.94.0.0'; +export const REVIEW_POLICY_KEY_VERSION = '1.94.1.0'; export const POLICY_KEY_INTRODUCED: Record = { bump: '1.91.73.0', queue_mode: '1.91.73.0', release_tool: '1.91.73.0', release_outputs: '1.91.73.0', stamp_paths: '1.91.73.0', changelog: '1.91.73.0', mirror: '1.91.73.0', history: '1.91.73.0', preregistration_shas: '1.91.73.0', stamp_sensitive_gates: '1.91.73.0', diff --git a/test/review-declared-risk.test.ts b/test/review-declared-risk.test.ts index d4145ddb3..b5cacbf86 100644 --- a/test/review-declared-risk.test.ts +++ b/test/review-declared-risk.test.ts @@ -85,9 +85,14 @@ describe('GST-1129 item 2: a declared risk class only raises the depth', () => { }, 60_000); test('the receipt records declared and detected classes beside the raised depth, and the schema accepts them', () => { const r = repo(); - const d = receiptReviewAtHead(r.dir, 'main', BIN, r.head, ['money']).depth; + const prior = process.env.GSTACK_STATE_ROOT; + process.env.GSTACK_STATE_ROOT = r.state; + let d: ReturnType['depth']; + try { + d = receiptReviewAtHead(r.dir, 'main', BIN, r.head, ['money']).depth; + expect(receiptReviewAtHead(r.dir, 'main', BIN, r.head).depth).not.toHaveProperty('declared'); + } finally { if (prior === undefined) delete process.env.GSTACK_STATE_ROOT; else process.env.GSTACK_STATE_ROOT = prior; } expect([d.depth, d.risk, d.declared, d.detected]).toEqual(['full', ['money'], ['money'], []]); - expect(receiptReviewAtHead(r.dir, 'main', BIN, r.head).depth).not.toHaveProperty('declared'); const errors: ValidationError[] = []; checkSchema({ schema_version: 1, head: r.head, base: 'main', review_depth: d }, ARTIFACT_SCHEMAS['ship-receipt'], '', errors); expect(errors.filter(e => JSON.stringify(e).includes('review_depth'))).toEqual([]); diff --git a/test/review-depth-lean.test.ts b/test/review-depth-lean.test.ts index 45d520a46..367d965bc 100644 --- a/test/review-depth-lean.test.ts +++ b/test/review-depth-lean.test.ts @@ -166,6 +166,7 @@ describe('R65 lean loop: one fix round, never more than 2 rounds', () => { expect(nextStep(h({ last: 'fix-check', fixedBlocking: true, fixChecks: 1 }), 'full').step).toBe('stop'); expect(nextStep(h({ last: 'fix-check', fixedBlocking: true, fixChecks: 1, granted: 'apr_1' }), 'full').step).toBe('fix-check'); expect(nextStep(h({ last: 'fix-check', fixedBlocking: true, fixChecks: 2, granted: 'apr_1' }), 'full').step).toBe('stop'); + expect(nextStep(h({ last: 'fix-check', fixedBlocking: true, granted: 'apr_1' }), 'full').step).toBe('stop'); }); test('planReview marks the loop lean and a fix-check dispatches one full-effort reader', () => { expect(roles(planReview(base({ paths: ['lib/billing.ts'], history: { fullPasses: 1, last: 'full', fixedBlocking: true }, shape: lean({ panel: 'off' }) })))).toEqual(['fix-check:fix-check:full:-']); diff --git a/test/review-packets-removed.test.ts b/test/review-packets-removed.test.ts index 70344cd38..ed87bdfdc 100644 --- a/test/review-packets-removed.test.ts +++ b/test/review-packets-removed.test.ts @@ -95,7 +95,7 @@ const REVIEWER_BRIEFS: Record = { 'review/specialists/testing.md': { sha: 'd6cc2bef7be562387d0c0a22fefe646dff9c295f1d34d8453a6cb496584150e4', replay: '#3638' }, 'review/specialists/targeted.md': { sha: '7f92a0d0a8319c3c0e076fb3b1a4689c45120b2be2c14dfc7d4cb03aafa9d6a5', replay: 'GST-257' }, 'scripts/resolvers/fix-check.ts': { sha: '712bf5b0c439e96bac32f19cbe25394dabb4c94aa66600100bfde9b100e15a08', replay: '#3638' }, - 'lib/review-depth.ts': { sha: '7d5e18436404ed8d620bfe6a7d042dff9251956a790d978407445ec7444a70ee', replay: 'GSTA-258-C' }, + 'lib/review-depth.ts': { sha: '7a5ca1f98318902c5ebd3e537f37b7cef05689232ff307a429923397d60abdac', replay: 'GSTA-258-C' }, 'lib/review-risk.ts': { sha: '2c5b96b921b8be5b977b734b3787361ed80e7d3f779ff6c4a20948f85e4d4474', replay: 'GSTA-258-C' }, }; diff --git a/test/ship-receipt-platform.test.ts b/test/ship-receipt-platform.test.ts index 1c7124e95..6cd1d5a10 100644 --- a/test/ship-receipt-platform.test.ts +++ b/test/ship-receipt-platform.test.ts @@ -1,4 +1,4 @@ -import { describe, test, expect, beforeAll } from 'bun:test'; +import { describe, test, expect, beforeAll, afterAll } from 'bun:test'; import { spawnSync } from 'node:child_process'; import * as fs from 'node:fs'; import * as os from 'node:os'; @@ -27,15 +27,23 @@ function repo(policy: object, files: Record): { dir: string; hea } let fakeBin = ''; -const withGh = (head: string, fn: () => T): T => { - const prev = process.env.PATH, prevHead = process.env.FAKE_REVIEW_HEAD; - process.env.PATH = fakeBin + path.delimiter + prev; process.env.FAKE_REVIEW_HEAD = head; - try { return fn(); } finally { process.env.PATH = prev; if (prevHead === undefined) delete process.env.FAKE_REVIEW_HEAD; else process.env.FAKE_REVIEW_HEAD = prevHead; } +const withGh = (head: string, fn: () => T, state = 'COMMENTED'): T => { + const prev = process.env.PATH, prevHead = process.env.FAKE_REVIEW_HEAD, prevState = process.env.FAKE_REVIEW_STATE; + process.env.PATH = fakeBin + path.delimiter + prev; process.env.FAKE_REVIEW_HEAD = head; process.env.FAKE_REVIEW_STATE = state; + try { return fn(); } finally { + process.env.PATH = prev; + if (prevHead === undefined) delete process.env.FAKE_REVIEW_HEAD; else process.env.FAKE_REVIEW_HEAD = prevHead; + if (prevState === undefined) delete process.env.FAKE_REVIEW_STATE; else process.env.FAKE_REVIEW_STATE = prevState; + } }; +let priorState: string | undefined; beforeAll(() => { fakeBin = fs.mkdtempSync(path.join(os.tmpdir(), 'fakegh-')); - fs.writeFileSync(path.join(fakeBin, 'gh'), '#!/bin/sh\ncase "$1" in\n pr) echo \'{"number":7}\' ;;\n api) echo "[[{\\"id\\":99,\\"user\\":{\\"login\\":\\"capy-ai[bot]\\"},\\"commit_id\\":\\"$FAKE_REVIEW_HEAD\\",\\"state\\":\\"COMMENTED\\"}]]" ;;\nesac\n', { mode: 0o755 }); + priorState = process.env.GSTACK_STATE_ROOT; + process.env.GSTACK_STATE_ROOT = path.join(fakeBin, 'state'); + fs.writeFileSync(path.join(fakeBin, 'gh'), '#!/bin/sh\ncase "$1" in\n pr) echo \'{"number":7}\' ;;\n api) echo "[[{\\"id\\":99,\\"user\\":{\\"login\\":\\"capy-ai[bot]\\"},\\"commit_id\\":\\"$FAKE_REVIEW_HEAD\\",\\"state\\":\\"$FAKE_REVIEW_STATE\\"}]]" ;;\nesac\n', { mode: 0o755 }); }); +afterAll(() => { if (priorState === undefined) delete process.env.GSTACK_STATE_ROOT; else process.env.GSTACK_STATE_ROOT = priorState; }); describe('C6 receipt: review_depth platform, honest and bound to the exact head (FOUN-63)', () => { test('a docs diff under review.platform on with capy-ai[bot]\'s review at HEAD records platform, and the queue accepts it at that head only', () => { @@ -44,7 +52,7 @@ describe('C6 receipt: review_depth platform, honest and bound to the exact head expect([d.depth, d.mode]).toEqual(['light', 'panel']); const p = withGh(head, () => platformReceiptDepth(dir, 'main', d, head, '99')); expect(p.ok && [p.depth.depth, p.depth.mode, p.depth.platform_review]).toEqual(['platform', 'platform', { id: '99', head, reviewer: 'capy-ai[bot]' }]); - const r = { gstack_version: '1.94.0.0', review_depth: (p as any).depth } as ShipReceipt; + const r = { gstack_version: REVIEW_POLICY_KEY_VERSION, review_depth: (p as any).depth } as ShipReceipt; expect(receiptDepthVerdict(dir, 'origin/main', r, head)).toMatchObject({ ok: true, line: expect.stringContaining('platform_review=99@') }); const moved = { ...r, review_depth: { ...r.review_depth!, platform_review: { id: '99', head: 'b'.repeat(40), reviewer: 'capy-ai[bot]' } } } as ShipReceipt; expect(receiptDepthVerdict(dir, 'origin/main', moved, head).ok).toBe(false); @@ -61,6 +69,8 @@ describe('C6 receipt: review_depth platform, honest and bound to the exact head const { dir, head } = repo({ review: { platform: 'on' } }, { 'docs/howto.md': 'x\n' }); const d = receiptReviewAtHead(dir, 'main', BIN, head).depth; expect(withGh('c'.repeat(40), () => platformReceiptDepth(dir, 'main', d, head, '99'))).toMatchObject({ ok: false, message: expect.stringContaining('capy-ai[bot]') }); + for (const state of ['CHANGES_REQUESTED', 'DISMISSED']) expect(withGh(head, () => platformReceiptDepth(dir, 'main', d, head, '99'), state).ok).toBe(false); + expect(withGh(head, () => platformReceiptDepth(dir, 'main', d, head, '99'), 'APPROVED').ok).toBe(true); }); test('the receipt records the risk_paths class and the panel mode the base policy gives', () => { const { dir, head } = repo({ review: { risk_paths: ['src/balance/**'] } }, { 'src/balance/x.ts': 'export const x = 1;\n' });