diff --git a/bin/gstack-review-plan b/bin/gstack-review-plan index 818d3c6a8..d524072a0 100755 --- a/bin/gstack-review-plan +++ b/bin/gstack-review-plan @@ -25,6 +25,8 @@ import { EXIT, exitTableHelp } from '../lib/headless-artifacts'; import { runScriptSync } from '../lib/script-invocation'; import { collectCommit, collectLive } from '../lib/review-collect'; import { classifyDiff, diffCommand, planReview, reviewDepthLine, reviewPlanLine, scanNumbers, SCOPED_RANGE_LINES, type Depth, type History } from '../lib/review-depth'; +import { DIFF_REVIEWS } from '../lib/review-evidence'; +import { reviewPaths } from '../lib/ship-budget'; import { classList, targetedBriefSha, targetedClasses, targetedPatchId } from '../lib/review-targeted'; import { renderFixCheckPrompt } from '../scripts/resolvers/fix-check'; @@ -58,7 +60,9 @@ const HELP = `gstack-review-plan — the review depth of the next /ship step (do Depth: light (runtime lines <= 50), standard (above 50), full (gate class or any DEPTH_ERROR). TARGETED: the money, gate, auth and secret classes the diff touches; any class dispatches the targeted reader at every depth. NEXT: full, scoped -(a full pass on the range: fix-check, Red Team, native adversarial), fix-check, stop or done. A full plan +(a full pass on the range: fix-check, Red Team, native adversarial), fix-check, stop or done. Pass 1 at gate class or past +200 runtime lines skips the Red Team only when this branch's review log (gstack-review-log) has no diff-review row from an +earlier run; an unreadable log keeps it (--json: reviewed_before true, false or null). A full plan from an error exits 0; exit 1 means no plan could be built (git could not list the diff). ${exitTableHelp()}`; @@ -137,18 +141,23 @@ function main(): number { : { base: rangeBase, runtime_lines: range.runtimeLines, ...(range.class === 'gate' ? { touched_gate: true as const } : {}) }; } } + let reviewedBefore: boolean | null; + try { + const rows = fs.readFileSync(reviewPaths(root, process.env).reviewLog, 'utf8').split('\n').filter(Boolean); + reviewedBefore = rows.some(l => { try { return DIFF_REVIEWS.has(JSON.parse(l)?.skill); } catch { return false; } }); + } catch (e: any) { reviewedBefore = e?.code === 'ENOENT' ? false : null; } const targeted = targetedClasses({ ...collected, scope, gateTagged }); const patch = collected.mergeBase ? targetedPatchId(process.cwd(), collected.mergeBase) : 'none'; const brief = targetedBriefSha(); const plan = planReview({ ...collected, scope, gateTagged, targeted: { classes: targeted.classes, ran: !['uncommitted', 'none'].includes(patch) && flag('--targeted-ran') === patch }, numbers: scanNumbers(added), force, gated: list('--gated'), floor, tiers: auditEffort !== 'host', - criticalFound: has('--critical-found'), adversarialRan: has('--adversarial-ran'), reviewedNumbers: flag('--reviewed-numbers'), + criticalFound: has('--critical-found'), adversarialRan: has('--adversarial-ran'), reviewedNumbers: flag('--reviewed-numbers'), reviewedBefore: reviewedBefore !== false, 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 }, }); const gateScan = { tagged: gateTagged, head }; if (has('--json')) { const { runtimeLines, stepDepth, depthReasons, ...rest } = plan; - process.stdout.write(JSON.stringify({ ...rest, targeted: { ...targeted, brief, targeted_patch_id: patch }, step_depth: stepDepth, runtime_lines: runtimeLines, depth_reasons: depthReasons, depth_floor: floor ?? null, gate_scan: gateScan, fix_range: fixRange ?? null, registry: { sha: registry.sha, outputs: registry.outputs }, per_path: perPath, diff_lines: diffLines, pass, audit_effort: auditEffort, scope, gate_tagged: gateTagged, paths, deleted, stamp_only: stampOnly, consumed: collected.consumed }) + '\n'); + process.stdout.write(JSON.stringify({ ...rest, targeted: { ...targeted, brief, targeted_patch_id: patch }, step_depth: stepDepth, runtime_lines: runtimeLines, depth_reasons: depthReasons, depth_floor: floor ?? null, gate_scan: gateScan, fix_range: fixRange ?? null, reviewed_before: reviewedBefore, registry: { sha: registry.sha, outputs: registry.outputs }, per_path: perPath, diff_lines: diffLines, pass, audit_effort: auditEffort, scope, gate_tagged: gateTagged, paths, deleted, stamp_only: stampOnly, consumed: collected.consumed }) + '\n'); return EXIT.ok; } const out = [`REVIEW_CLASS: ${plan.class} (${plan.reasons.slice(0, 3).join('; ') || 'no signals'})`, reviewDepthLine(plan), `GATE_SCAN: ${gateTagged === null ? 'unavailable' : `tagged=${gateTagged}`} head=${head}`, diff --git a/changelog.d/gsta-167-first-pass-red-team.md b/changelog.d/gsta-167-first-pass-red-team.md index f9950bbdf..3243a19bf 100644 --- a/changelog.d/gsta-167-first-pass-red-team.md +++ b/changelog.d/gsta-167-first-pass-red-team.md @@ -1,7 +1,8 @@ -**`/ship` no longer runs the standalone Red Team on the first full review pass of an invocation at gate class or past 200 runtime lines.** The preregistered no-loss replay (GSTA-167 Round 7, handoffs/GSTA-167/noloss7-results.md) removed those Red Team lines from the recorded reviewer outputs. The rest of the stack still found 32 of the 41 panel blockers against 33 with them, a one-sided non-inferiority p of 0.00074 at a margin of 4. The Red Team still runs on a small diff raised to standard, on every post-fix full pass and on the scoped pass. A first pass that logs a CRITICAL gets it only if a post-fix full or scoped pass follows, because `/ship` passes `--critical-found` only to the repeat plan. A new `/ship` after a stop starts counting passes again, so its first full pass has none either. +**`/ship` no longer runs the standalone Red Team on a branch's first full review pass at gate class or past 200 runtime lines.** The preregistered no-loss replay (GSTA-167 Round 7, handoffs/GSTA-167/noloss7-results.md) removed those Red Team lines from the recorded reviewer outputs. The rest of the stack still found 32 of the 41 panel blockers against 33 with them, a one-sided non-inferiority p of 0.00074 at a margin of 4. The Red Team still runs on a small diff raised to standard, on every post-fix full pass and on the scoped pass. A first pass that logs a CRITICAL gets it only if a post-fix full or scoped pass follows, because `/ship` passes `--critical-found` only to the repeat plan. The cut applies only when the branch's review log has no earlier review row. A new `/ship` after a stop, on a branch an earlier run already reviewed, keeps the Red Team on its first full pass. So does a run whose review log can't be read. ### Changed - **`lib/review-depth.ts`**: on a first full pass (`fullPasses` 0 and no earlier step, so a repeat plan that leaves out `--pass` keeps the Red Team), the gate-class term (a depth error included, since it runs as gate class) and the more-than-`RED_TEAM_LINES` term no longer add the Red Team. The plan prints `SKIP: red-team (first full pass; Round 7 no-loss)`. The CRITICAL and raised small-diff terms are unchanged, and post-fix full passes keep every term. The scoped pass, the fix-check and light depth's batched red-team checklist are unchanged. +- **`bin/gstack-review-plan`**: before the first full pass, it reads this branch's review log through `reviewPaths` (`lib/ship-budget.ts`) and looks for a row from one of the diff-review skills in `lib/review-evidence.ts`. When it finds one, or can't read the log, it passes `reviewedBefore` and the Red Team stays. `--json` adds `reviewed_before`. - **`RED_TEAM_FIRST_PASS`** (false) joins the `DEPTH_RULES` hash, so the hash moves. ### Docs @@ -9,6 +10,7 @@ ### Tests - `test/review-depth.test.ts`: separate histories for a first gate pass, a first pass reached through a depth error and a first pass past 200 lines (no Red Team), a first pass with a CRITICAL and a raised small diff (Red Team), post-fix full passes, the scoped pass and the fix-check, light's checklist, and the `DEPTH_RULES` move. +- `test/review-depth.test.ts` and `test/review-plan-cli.test.ts`: a restarted `/ship` on a branch with earlier review records keeps the Red Team on its first full pass. A branch with no records still takes the cut, and an unreadable log keeps the Red Team. Both tests were written first, and both fail at db1405e. - `test/review-pass.test.ts`: the pass program creates no Red Team on a first full pass, and a failed Red Team on a post-fix pass stops it. `test/review-dispatch.test.ts`: a trimmed Red Team brief is `BRIEF_MISMATCH` and the exact re-send voids it. ### Gate integrity diff --git a/docs/review-policy.md b/docs/review-policy.md index acd7efffb..f5021d71d 100644 --- a/docs/review-policy.md +++ b/docs/review-policy.md @@ -208,9 +208,8 @@ next step is a scoped pass, the raise applies to the scoped set only. A fix-chec is the same at every depth. -**The first full pass has no standalone Red Team (GSTA-167 Round 7).** When an -invocation's first full pass (`fullPasses` 0 and no earlier step, so a repeat -plan that leaves out `--pass` still counts as post-fix) is gate class or has more than +**The first full pass has no standalone Red Team (GSTA-167 Round 7).** When a +branch's first full pass is gate class or has more than `RED_TEAM_LINES` (200) runtime lines, the plan skips the Red Team and says why: `SKIP: red-team (first full pass; Round 7 no-loss)`. A full plan reached through a classifier error (`DEPTH_ERROR`) or `--depth-floor full` runs as gate class, so the same rule covers it; @@ -220,10 +219,19 @@ the scoped pass, and the plan adds it whenever `--critical-found` is given. In `/ship` that flag reaches only the repeat plan, so a first pass that logs a CRITICAL gets the Red Team only if a post-fix full or scoped pass follows. When that CRITICAL is deferred or noted (`done`), fixed below the floor (a fix-check) -or left open (a stop), no Red Team reads the change. `fullPasses` -counts passes in one `/ship` invocation, so a new `/ship` after a stop (a fix made -by hand between runs) starts at 0 and its first full pass has no Red Team either. -A fix-check never had one. `RED_TEAM_FIRST_PASS` (false) records the rule in +or left open (a stop), no Red Team reads the change. A fix-check never had one. + +A pass is the branch's first only when three things hold: `fullPasses` is 0, no +step ran earlier in this invocation (so a repeat plan that leaves out `--pass` +still counts as post-fix), and the branch's review log has no diff-review row. The +review log is the `-reviews.jsonl` file that `gstack-review-log` writes and +`gstack-ship-budget` reads. A diff-review row is any row from the `review`, +`review-fix-check`, `adversarial-review`, `codex-review`, `design-review-lite` or +`ship` skills. So a new `/ship` after a stop, on a branch an earlier run already +reviewed, runs the Red Team on its first full pass, as it did before this rule. +When the log can't be read, the plan keeps the Red Team too. `gstack-review-plan +--json` reports the lookup as `reviewed_before`: true, false, or null when the log +was unreadable. `RED_TEAM_FIRST_PASS` (false) records the rule in `DEPTH_RULES`. The evidence is the preregistered no-loss replay @@ -303,8 +311,9 @@ the specialists, audits and watchers at the host default too ### Behavior changes -- An invocation's first full pass at gate class (a depth error included) or past 200 - runtime lines no longer dispatches the standalone Red Team +- A branch's first full pass at gate class (a depth error included) or past 200 + runtime lines, when the branch's review log has no earlier review row, no longer + dispatches the standalone Red Team ([first full pass](#first-pass-red-team), GSTA-167 Round 7). This is a gate edit: the `DEPTH_RULES` hash moves, so receipts written before it carry the old `rules` value. diff --git a/lib/review-depth.ts b/lib/review-depth.ts index 21ed1f521..8f41159b8 100644 --- a/lib/review-depth.ts +++ b/lib/review-depth.ts @@ -299,6 +299,8 @@ export interface PlanInput extends ClassInput { tiers?: boolean; /** The targeted classes (lib/review-targeted.ts) and whether the targeted reader already read this product patch. */ targeted?: { classes: readonly string[]; ran?: boolean }; + /** This branch's review log holds a diff-review row from an earlier run, or could not be read: the first full pass keeps the Red Team. */ + reviewedBefore?: boolean; } /** One reviewer. `exclude` holds git pathspec argv items the reviewer's diff leaves out; `checklists` names what the batched specialist reads. */ export interface Dispatch { role: Role; name: string; effort: Effort; exclude: string[]; checklists?: string[]; reason?: string } @@ -413,7 +415,7 @@ export function planReview(i: PlanInput): ReviewPlan { if (checklists.length) add('specialist', 'batched', specialist, full ? [] : docsAndOutputs, { checklists }); const codeLike = dcls === 'code' || dcls === 'gate'; const sized = dcls === 'gate' || (dcls === 'code' && lines > RED_TEAM_LINES); - const firstPassCut = sized && h.fullPasses === 0 && h.last === 'none' && !RED_TEAM_FIRST_PASS; + const firstPassCut = sized && h.fullPasses === 0 && h.last === 'none' && !i.reviewedBefore && !RED_TEAM_FIRST_PASS; if (codeLike && (!!i.criticalFound || raised || (sized && !firstPassCut))) add('red-team', 'red-team', 'full'); else if (firstPassCut) skipped.push('red-team (first full pass; Round 7 no-loss)'); else skipped.push(`red-team (${dcls} class${dcls === 'code' ? `, ${lines} runtime lines, no critical` : ''})`); diff --git a/lib/review-evidence.ts b/lib/review-evidence.ts index 777971f50..9ad8afb57 100644 --- a/lib/review-evidence.ts +++ b/lib/review-evidence.ts @@ -5,7 +5,7 @@ import { join } from 'node:path'; import { deriveReviewStatus, readJsonl } from './headless-artifacts'; import { resultLine, type ResultCodeName } from './result-codes'; -const DIFF_REVIEWS = new Set(['review', 'review-fix-check', 'adversarial-review', 'codex-review', 'design-review-lite', 'ship']); +export const DIFF_REVIEWS: ReadonlySet = new Set(['review', 'review-fix-check', 'adversarial-review', 'codex-review', 'design-review-lite', 'ship']); const SHARED_LIBS_COVERAGE_VERSION = 1; const HASH = /^(?:[0-9a-f]{40}|[0-9a-f]{64})$/; diff --git a/test/review-depth.test.ts b/test/review-depth.test.ts index 8d4712029..a3bd3af9f 100644 --- a/test/review-depth.test.ts +++ b/test/review-depth.test.ts @@ -513,7 +513,7 @@ describe('the first full pass drops the standalone Red Team (GSTA-167 Round 7 no const rt = (p: ReturnType) => p.dispatches.some(d => d.role === 'red-team'); const NOTE = 'red-team (first full pass; Round 7 no-loss)'; const postFix = { fullPasses: 1, last: 'full', fixedBlocking: true, rangeRuntimeLines: 300 } as const; - // Value: protects=a change's first full pass at gate class (or full from a depth error) or past RED_TEAM_LINES dispatches no + // Value: protects=the first full pass of a branch with no earlier review record, at gate class (or full from a depth error) or past RED_TEAM_LINES dispatches no // standalone Red Team and says why in SKIP; fails_when=the Round 7 cut is missing, too broad or unexplained; // why_new=V7 passed (handoffs/GSTA-167/noloss7-results.md: 32 of 41 against 33, p_NI 0.00074); seam=planReview test('a first full pass on gate class has no Red Team', () => { @@ -535,6 +535,16 @@ describe('the first full pass drops the standalone Red Team (GSTA-167 Round 7 no expect(rt(p)).toBe(false); expect(p.skipped).toContain(NOTE); }); + // Value: protects=a /ship restarted after a stop on a branch whose review log already has a pass keeps the Red Team on its first + // full pass; fails_when=the cut reads run state (fullPasses, last) instead of the branch's review records; + // why_new=GSTA-167 option a (a restart reset both counters and dropped the Red Team); seam=planReview reviewedBefore + test('a restarted /ship on a branch with earlier review records keeps the Red Team on its first full pass', () => { + for (const o of [{ diffLines: 20, gateTagged: 1 }, { diffLines: 20, scope: { SCOPE_ERROR: 'unmatched path' } }, { diffLines: 201 }]) { + const p = planReview(base({ ...o, reviewedBefore: true })); + expect([p.next.step, rt(p), p.skipped.includes(NOTE)]).toEqual(['full', true, false]); + expect(rt(planReview(base({ ...o, reviewedBefore: false })))).toBe(false); + } + }); test('a first full pass with a CRITICAL keeps the Red Team, on code and gate class', () => { for (const o of [{ diffLines: 120 }, { diffLines: 201 }, { diffLines: 20, gateTagged: 1 }]) expect(rt(planReview(base({ ...o, criticalFound: true })))).toBe(true); }); diff --git a/test/review-packets-removed.test.ts b/test/review-packets-removed.test.ts index 3039100fb..ac2654358 100644 --- a/test/review-packets-removed.test.ts +++ b/test/review-packets-removed.test.ts @@ -56,7 +56,7 @@ const NO_LOSS_REPLAYS = { 'G1': "GSTA-167 variant G Arm G1: live no-loss replay of both prose voices with part 1's prompts, 2 passes, normalizer v2, paired rule against pooled C' (parts 1 and 3; specialists at medium as in C and C2)", 'L': "GSTA-167 variant G Arm L: live no-loss replay of one batched reader (testing and security plus scoped checklists) and the Red Team on its findings, 2 passes, scored L∘G1 against G1 on the paired rule (part 2)", 'GST-257': "GST-257 PR 1 dispatch-superset no-loss check (test/review-targeted.test.ts): on every fixture diff the branch dispatches main's reviewers at 8be7f21 unchanged plus only the targeted reader, and over the last 100 non-release commits on main it lost none; adds the targeted role and brief to the G1 and L bytes, whose existing reviewer text is unchanged", - 'V7': "GSTA-167 no-loss Round 7 arm V7 (handoffs/GSTA-167/noloss7-results.md): L∘G1 without the standalone Red Team on a change's first full pass found 32 of 41 panel blockers against R's 33, p_NI 0.00074 at delta 4; certifies only the first-full-pass gate-class and >200-line trigger in lib/review-depth.ts", + 'V7': "GSTA-167 no-loss Round 7 arm V7 (handoffs/GSTA-167/noloss7-results.md): L∘G1 without the standalone Red Team on the first full pass of a branch with no earlier review record found 32 of 41 panel blockers against R's 33, p_NI 0.00074 at delta 4; certifies only the first-full-pass gate-class and >200-line trigger in lib/review-depth.ts", } as const; type Replay = keyof typeof NO_LOSS_REPLAYS; const REVIEWER_BRIEFS: Record = { @@ -78,7 +78,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: 'a64130f2f2fd13416994dde01f4ed53349013f7134c41f87d43f59d73c448d0a', replay: 'V7' }, + 'lib/review-depth.ts': { sha: '709e82486d6845fd98baf1068c791758316e39fbb7291783e16ab430408ff1c3', replay: 'V7' }, }; describe('reviewer briefs stay at their no-loss-tested bytes (GSTA-167)', () => { diff --git a/test/review-plan-cli.test.ts b/test/review-plan-cli.test.ts index f81dc7d7a..d1eed7a36 100644 --- a/test/review-plan-cli.test.ts +++ b/test/review-plan-cli.test.ts @@ -258,3 +258,35 @@ describe('gstack-review-plan: the scoped second pass (GSTA-152 D6)', () => { expect(run(r, [...pass2, '--range-base', rangeBase, '--depth-floor', 'full']).stdout).toMatch(/^NEXT: full \(a blocking finding was fixed and the diff is at full depth/m); }); }); + +describe('the first-pass Red Team cut reads the branch review log (GSTA-167 option a)', () => { + const LOG = path.join(import.meta.dir, '..', 'bin', 'gstack-review-log'); + const NOTE = 'SKIP: red-team (first full pass; Round 7 no-loss)'; + const RT = /^DISPATCH: red-team red-team effort=full$/m; + const big = () => repo({ 'src/a.ts': Array.from({ length: 210 }, (_, i) => `export const v${i} = ${i};`).join('\n') + '\n' }); + // Value: protects=a /ship restarted after a stop on a branch whose review log already holds a pass plans the Red Team on its + // first full pass, a branch with no records still takes the Round 7 cut, and an unreadable log keeps the Red Team; + // fails_when=the cut reads only --pass/--last (a restart resets both) or a lookup failure drops the Red Team; + // why_new=GSTA-167 option a; seam=bin/gstack-review-plan through lib/ship-budget reviewPaths and the real gstack-review-log + test('a restart on a branch with an earlier review row keeps the Red Team; no row takes the cut; an unreadable log keeps it', () => { + const r = big(); + const first = run(r, []); + expect(first.stdout).toContain(NOTE); + expect(first.stdout).not.toMatch(RT); + expect(JSON.parse(run(r, ['--json']).stdout).reviewed_before).toBe(false); + const logged = spawnSync(LOG, ['{"skill":"review","timestamp":"2026-10-31T00:00:00Z","status":"clean","via":"ship","pass":1}'], { cwd: r.dir, encoding: 'utf8', timeout: 30_000, env: { ...process.env, GSTACK_STATE_ROOT: r.state } }); + expect(logged.status, logged.stderr).toBe(0); + const restarted = run(r, []); + expect(restarted.stdout).toMatch(RT); + expect(restarted.stdout).not.toContain(NOTE); + expect(JSON.parse(run(r, ['--json']).stdout).reviewed_before).toBe(true); + const u = big(); + const dir = path.join(u.state, 'projects'); + expect(run(u, []).stdout).toContain(NOTE); + fs.rmSync(dir, { recursive: true, force: true }); + fs.writeFileSync(dir, 'not a directory'); + const unread = run(u, []); + expect(unread.stdout).toMatch(RT); + expect(JSON.parse(run(u, ['--json']).stdout).reviewed_before).toBe(null); + }, 60_000); +});