From 1ec488583e9d5360e0f976020f67d03baa3abbb4 Mon Sep 17 00:00:00 2001 From: garrytan Date: Wed, 28 Oct 2026 23:08:53 +0000 Subject: [PATCH] feat(review): proportional /ship review depth (light, standard, full) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- bin/gstack-review-plan | 79 ++++--- docs/review-policy.md | 99 ++++++-- lib/review-collect.ts | 137 +++++++++++ lib/review-depth.ts | 219 ++++++++++++++---- scripts/resolvers/review-army.ts | 2 +- scripts/review-dispatch-compare.ts | 23 +- ship/SKILL.md | 10 +- ship/SKILL.md.tmpl | 10 +- ship/sections/adversarial.md | 3 +- ship/sections/adversarial.md.tmpl | 3 +- ship/sections/review-army.md | 25 +- ship/sections/review-army.md.tmpl | 23 +- test/fixtures/golden/agy-ship-SKILL.md | 10 +- test/fixtures/golden/claude-ship-SKILL.md | 10 +- test/fixtures/golden/codex-ship-SKILL.md | 10 +- test/fixtures/golden/factory-ship-SKILL.md | 10 +- test/review-collect.test.ts | 110 +++++++++ test/review-depth.test.ts | 130 ++++++++++- test/review-dispatch-compare.test.ts | 2 +- test/review-plan-cli.test.ts | 48 ++++ .../ship/gsta147-review-depth.json | 4 + 21 files changed, 802 insertions(+), 165 deletions(-) create mode 100644 lib/review-collect.ts create mode 100644 test/review-collect.test.ts create mode 100644 test/size-ledger/ship/gsta147-review-depth.json diff --git a/bin/gstack-review-plan b/bin/gstack-review-plan index a5424c25a..06f3e3976 100755 --- a/bin/gstack-review-plan +++ b/bin/gstack-review-plan @@ -4,20 +4,25 @@ * * gstack-review-plan --base [--pass ] [--last none|full|fix-check] [--fixed-blocking] [--fixed-below] * [--blocking-open] [--touched-gate] [--critical-found] [--adversarial-ran] [--gate-tagged ] [--gated a,b] [--force a,b|all] - * [--reviewed-numbers ] [--no-tiers] [--json] + * [--reviewed-numbers ] [--depth-floor standard|full] [--no-tiers] [--json] * gstack-review-plan --fix-check-prompt --fix-list --hunks --tests [--callers ] * - * Reads the diff against the merge base with (committed, working tree and - * untracked), runs gstack-diff-scope and, unless --gate-tagged is given, - * gstack-gate-diff; prints REVIEW_CLASS:, REVIEW_PLAN:, one DISPATCH: line per - * reviewer and one SKIP: line per reviewer left out. Logic: lib/review-depth.ts. + * Reads the diff against the merge base with (committed, tracked uncommitted + * edits and untracked files, through lib/review-collect.ts collectLive), runs + * gstack-diff-scope and, unless --gate-tagged is given, gstack-gate-diff; prints + * REVIEW_CLASS:, REVIEW_DEPTH: (depth, runtime lines, the rules hash, reasons and + * result codes), GATE_SCAN:, REVIEW_PLAN:, one DEPTH_ERROR: per input that failed + * the depth closed, one DISPATCH: plus DIFF_CMD: (and DIFF_EXCLUDE: none) per + * reviewer and one SKIP: per reviewer left out. A plan that fails closed to full + * exits 0; a diff git cannot list exits 1. Logic: lib/review-depth.ts. */ -import { spawnSync } from 'node:child_process'; import * as fs from 'node:fs'; import * as path from 'node:path'; +import { spawnSync } from 'node:child_process'; import { EXIT, exitTableHelp } from '../lib/headless-artifacts'; import { runScriptSync } from '../lib/script-invocation'; -import { planReview, reviewPlanLine, scanNumbers, type History } from '../lib/review-depth'; +import { collectLive } from '../lib/review-collect'; +import { diffCommand, planReview, reviewDepthLine, reviewPlanLine, scanNumbers, type Depth, type History } from '../lib/review-depth'; import { renderFixCheckPrompt } from '../scripts/resolvers/fix-check'; const HELP = `gstack-review-plan — the review depth of the next /ship step (docs/review-policy.md#review-depth) @@ -25,19 +30,23 @@ const HELP = `gstack-review-plan — the review depth of the next /ship step (do --base the base branch (merge base with HEAD is the diff base); required --pass full review passes already run in this invocation (default 0) --last none|full|fix-check the step that just finished (default none: plan pass 1) - --fixed-blocking the finished step fixed a blocking finding + --fixed-blocking the finished step fixed a blocking finding (raises light to standard) --fixed-below the finished step fixed findings below the floor --blocking-open a blocking finding is still open in fix scope --touched-gate a fix touched a gate-class path - --critical-found a specialist reported a CRITICAL finding (Red Team on code class) + --critical-found a specialist reported a CRITICAL finding (Red Team on code class; raises light to standard) --adversarial-ran Step 11's native adversarial pass already ran (a repeat skips it) --gate-tagged gate hunks the gate scan tagged (default: run gstack-gate-diff; failure means unavailable) --gated a,b specialists the hit-rate stats tag GATE_CANDIDATE --force a,b|all specialists forced by prompt flags (--all-specialists is all) --reviewed-numbers the numeric-claim fingerprint the outside voice already reviewed + --depth-floor standard|full raise the depth to at least this (never lowers it); /ship passes its prompt flag on every run --no-tiers audits and watchers at the host's default effort too (as gstack-config review_effort_tiers off does) - --json the plan as JSON + --json the plan as JSON (depth, runtime_lines, rules, errors with input and repair, gate_scan, per-dispatch exclude) --fix-check-prompt print the fix-check reviewer's prompt from --fix-list, --hunks, --tests and --callers files + +Depth: light (runtime lines <= 50), standard (above 50), full (gate class or any DEPTH_ERROR). A full plan +from an error exits 0; exit 1 means no plan could be built (git could not list the diff). ${exitTableHelp()}`; const args = process.argv.slice(2); @@ -65,34 +74,17 @@ function main(): number { if (!['none', 'full', 'fix-check'].includes(last)) { process.stderr.write('gstack-review-plan: --last must be none, full or fix-check\n'); return EXIT.usage; } const pass = Number(flag('--pass') ?? '0'); if (!Number.isInteger(pass) || pass < 0) { process.stderr.write('gstack-review-plan: --pass needs an integer >= 0\n'); return EXIT.usage; } + const floor = flag('--depth-floor') as Depth | undefined; + if (has('--depth-floor') && floor !== 'standard' && floor !== 'full') { process.stderr.write('gstack-review-plan: --depth-floor must be standard or full (a floor only raises the depth)\n'); return EXIT.usage; } const root = git(['rev-parse', '--show-toplevel'])?.trim(); if (!root) { process.stderr.write('gstack-review-plan: run it inside a git checkout\n'); return EXIT.fail; } process.chdir(root); - const ref = git(['rev-parse', '--verify', '-q', `origin/${base}`]) !== null ? `origin/${base}` : base; - const mergeBase = git(['merge-base', ref, 'HEAD'])?.trim(); - if (!mergeBase) { process.stderr.write(`gstack-review-plan: no merge base with ${ref} (fetch the base first)\n`); return EXIT.fail; } - const nameStatusRaw = git(['diff', '--name-status', '--no-renames', mergeBase]); - const untrackedRaw = git(['ls-files', '--others', '--exclude-standard']); - const numstatRaw = git(['diff', '--numstat', mergeBase]); - if (nameStatusRaw === null || untrackedRaw === null || numstatRaw === null) { process.stderr.write('gstack-review-plan: git could not list the diff; no plan is safer than a plan for an empty diff\n'); return EXIT.fail; } - const scratch = (p: string) => p.startsWith('.gstack/tmp/'); - const nameStatus = nameStatusRaw.split('\n').filter(Boolean).map(l => l.split('\t')).filter(c => !scratch(c[1] ?? '')); - const untracked = untrackedRaw.split('\n').filter(Boolean).filter(p => !scratch(p)); - const paths = [...new Set([...nameStatus.map(c => c[1]!).filter(Boolean), ...untracked])].sort(); - const deleted = nameStatus.filter(c => c[0] === 'D').map(c => c[1]!); - const stampOnly = paths.filter(p => p.endsWith('package.json') && !untracked.includes(p)).filter(p => { - const changed = (git(['diff', '-U0', '--no-color', mergeBase, '--', p]) ?? '').split('\n').filter(l => /^[+-]/.test(l) && !/^(\+\+\+|---) /.test(l)); - return changed.length > 0 && changed.every(l => /^[+-]\s*"version"\s*:/.test(l)); - }); - const diffLines = numstatRaw.split('\n').reduce((n, l) => { const [a, d] = l.split('\t'); return n + (Number(a) || 0) + (Number(d) || 0); }, 0) - + untracked.reduce((n, p) => { try { return n + fs.readFileSync(p, 'utf8').split('\n').length; } catch { return n; } }, 0); - const added: Record = {}; - let current = ''; - for (const line of (git(['diff', '-U0', '--no-color', mergeBase]) ?? '').split('\n')) { - if (line.startsWith('+++ b/')) { current = line.slice(6); continue; } - if (line.startsWith('+') && !line.startsWith('+++') && current) (added[current] ??= []).push(line.slice(1)); + const collected = collectLive(root, base); + if (collected.errors.length) { + for (const e of collected.errors) process.stderr.write(`gstack-review-plan: git could not list the diff (${e.input}: ${e.detail}); fix: ${e.repair}; no plan is safer than a plan for an empty diff\n`); + return EXIT.fail; } - for (const p of untracked) { try { added[p] = fs.readFileSync(p, 'utf8').split('\n'); } catch { /* unreadable: no claims */ } } + const head = git(['rev-parse', 'HEAD'])?.trim() ?? 'unknown'; const binDir = path.dirname(Bun.fileURLToPath(import.meta.url)); const scopeRun = runScriptSync(path.join(binDir, 'gstack-diff-scope'), [base], { encoding: 'utf8', timeout: 60_000 }); const scope: Record = {}; @@ -109,14 +101,25 @@ function main(): number { const force = flag('--force') === 'all' ? 'all' as const : list('--force'); const tiersSetting = runScriptSync(path.join(binDir, 'gstack-config'), ['get', 'review_effort_tiers'], { encoding: 'utf8', timeout: 15_000 }); const auditEffort = has('--no-tiers') || (tiersSetting.stdout ?? '').trim() === 'off' ? 'host' : 'light'; + const { added, diffLines, paths, deleted, stampOnly, perPath, registry } = collected; const plan = planReview({ - paths, deleted, stampOnly, root, scope, gateTagged, diffLines, numbers: scanNumbers(added), force, gated: list('--gated'), + ...collected, scope, gateTagged, numbers: scanNumbers(added), force, gated: list('--gated'), floor, criticalFound: has('--critical-found'), adversarialRan: has('--adversarial-ran'), reviewedNumbers: flag('--reviewed-numbers'), history: { fullPasses: pass, last, fixedBlocking: has('--fixed-blocking'), fixedBelow: has('--fixed-below'), blockingOpen: has('--blocking-open'), touchedGate: has('--touched-gate') }, }); - if (has('--json')) { process.stdout.write(JSON.stringify({ ...plan, diff_lines: diffLines, pass, audit_effort: auditEffort, scope, gate_tagged: gateTagged, paths, deleted, stamp_only: stampOnly }) + '\n'); return EXIT.ok; } - const out = [`REVIEW_CLASS: ${plan.class} (${plan.reasons.slice(0, 3).join('; ') || 'no signals'})`, reviewPlanLine(plan, pass + (plan.next.step === 'full' ? 1 : 0)), `NEXT: ${plan.next.step} (${plan.next.reason})`]; - for (const d of plan.dispatches) out.push(`DISPATCH: ${d.role} ${d.name} effort=${d.effort}`); + const gateScan = { tagged: gateTagged, head }; + if (has('--json')) { + const { runtimeLines, stepDepth, depthReasons, ...rest } = plan; + process.stdout.write(JSON.stringify({ ...rest, step_depth: stepDepth, runtime_lines: runtimeLines, depth_reasons: depthReasons, depth_floor: floor ?? null, gate_scan: gateScan, 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 }) + '\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}`, reviewPlanLine(plan, pass + (plan.next.step === 'full' ? 1 : 0)), `NEXT: ${plan.next.step} (${plan.next.reason})`]; + for (const e of plan.errors) out.push(`DEPTH_ERROR: ${e.code} input=${e.input} detail=${e.detail}; fix: ${e.repair}${e.result ? ` (${e.result})` : ''}`); + for (const d of plan.dispatches) { + const key = d.name.split(' ')[0]; + out.push(`DISPATCH: ${d.role} ${d.name} effort=${d.effort}${d.checklists ? ` checklists=${d.checklists.join(',')}` : ''}${d.reason ? ` reason=${d.reason}` : ''}`, `DIFF_CMD: ${key} ${diffCommand(d)}`); + if (!d.exclude.length) out.push(`DIFF_EXCLUDE: ${key} none`); + } for (const s of plan.skipped) out.push(`SKIP: ${s}`); out.push(`AUDIT_EFFORT: ${auditEffort} (coverage, plan-completion and docs audits, decision-trail reviewers, watchers; reviewers above stay full)`); process.stdout.write(out.join('\n') + '\n'); diff --git a/docs/review-policy.md b/docs/review-policy.md index 149abc89b..c6847c73f 100644 --- a/docs/review-policy.md +++ b/docs/review-policy.md @@ -128,26 +128,75 @@ applies no edits is its exit too. ## Review depth by diff class -`gstack-review-plan --base ` classifies the diff and prints the reviewers -the next step dispatches (`REVIEW_CLASS:`, `REVIEW_PLAN:`, one `DISPATCH:` per -reviewer with its effort, one `SKIP:` per reviewer left out, `--json`). A mixed -diff takes its most restrictive class; a `gstack-diff-scope` error fails closed -to code, and code with the gate scan unavailable fails closed to gate. +`gstack-review-plan --base ` classifies the diff, picks a **depth** and prints +the reviewers the next step dispatches: `REVIEW_CLASS:`, `REVIEW_DEPTH: +runtime_lines= rules= reasons=<…> codes=<…>`, `GATE_SCAN: tagged=|unavailable +head=`, one `DEPTH_ERROR:` per input that failed closed, one `DISPATCH:` per reviewer +with its effort and a shell-quoted `DIFF_CMD:` (plus `DIFF_EXCLUDE: none` when it +reads everything), one `SKIP:` per reviewer left out, and `--json` with the same fields. +A plan that fails closed to full exits 0; a diff git cannot list exits 1. -| Class | What it is | Pass 1 dispatches | +**Classes** are unchanged. A mixed diff takes its most restrictive class: **docs** +(markdown no agent or gate reads), **tests** (test files and fixtures), **code** +(everything else, including every `SKILL.md` and template, markdown in a skill +directory, `*/sections/**`, `scripts/resolvers/**`, rule docs, checklists and bins) and +**gate** (code plus auth or migration scope, a hunk the gate scan tags, a deleted code +or test path, CI workflows, credential, state-root and the paths below). + +**Runtime lines** are the changed lines of code-class paths, minus outputs the base +commit's `.gstack/generated.json` registers. Docs, tests and release metadata are never +runtime; a `.tmpl` edit plus its regenerated `SKILL.md` counts the template once. + +| Depth | When | Pass 1 dispatches | |---|---|---| -| **docs** | markdown no agent or gate reads: README, CHANGELOG, TODOS, how-to and explanation docs | the parent's core checklist only | -| **tests** | test files and fixtures only | core plus the testing specialist | -| **code** | everything else, including every `SKILL.md` and template, any markdown in a skill directory, `*/sections/**`, `scripts/resolvers/**`, `docs/*-policy.md`, `docs/captain.md`, `docs/unattended.md`, `docs/pregate.md`, checklists, specialists and bins | specialists by scope (as before), the native adversarial pass, the outside voice; the Red Team past 200 lines or after a CRITICAL | -| **gate** | code plus auth or migration scope, a hunk the gate scan tags (an RH rule: a relaxed threshold, a skipped test), a deleted code file, CI workflows, credential or state-root paths | code's set, plus the Red Team always, at full effort, and no hit-rate gating | +| **light**, runtime 0 | docs and tests only, or only registered outputs | the core checklist only; the outside voice when the diff publishes numbers | +| **light**, runtime 1–50 | `SMALL_DIFF_LINES` or fewer runtime lines | core, one **batched** specialist (testing, maintainability and security checklists, the specialists scope selects and the Red Team prompt in one context), the native adversarial pass (kept: backtest inconclusive, GSTA-147 D5) and the outside voice on numbers | +| **standard** | more than 50 runtime lines | the code set with every threshold (specialists at 50, simplification at 100, Red Team at 200) read on runtime lines; specialists leave docs and registered outputs out of their diff, and every specialist but Testing leaves tests out too; adversarial and outside voice read everything | +| **full** | gate class, or any classifier error | the gate set, unchanged: every reviewer on the whole diff, the Red Team always, no hit-rate gating | -**Numbers keep the outside voice.** On a docs or tests diff the outside voice is -skipped unless the diff publishes numeric claims: `gstack-review-plan` scans the -added lines of every path for percentages, money, multipliers, durations, token -or credit counts and score, rate or speed claims (`NUMBERS: yes`), and a -claim always dispatches it. The claims' fingerprint (`fingerprint=`) is passed -back as `--reviewed-numbers`; when a later fix changes a claim, only the -outside voice reruns, on the numbers. +A fixed blocking finding, a CRITICAL or an open blocker raises a light diff to +standard for the next step (`step=standard`): its specialists read at least 50 lines +and the Red Team runs, so a raise never dispatches less than light did. A fix-check +is the same at every depth. + +**Fail closed.** Each of these makes the depth full and is listed in `errors[]` with +the input it could not read, a sanitized cause and the repair: a `gstack-diff-scope` +error, a gate scan that is unavailable, a base registry that cannot be read or is +invalid (`REGEN_REGISTRY_INVALID` or `REGEN_CONTAINMENT` in `codes=`), a numstat git +could not produce, and a binary code-class path (it is never counted as 0 lines). + +**The registry rule.** Only the base commit's registry counts (the merge base's +`origin/` commit for live /ship, the receipt's base sha for the queue), and only +entries with a `command`, which pregate's regen check proves for that tree. An entry +added on the branch never excludes anything; `command: null` entries, vendored code +and lockfiles stay runtime. To adopt it, commit an entry like this on the base branch: + +```json +{ + "$schema": "https://github.com/garrytan/gstack/docs/pregate.md#generated-v1", + "entries": [ + { "id": "skill-docs", "command": "bun run gen:skill-docs --host all", "outputs": ["*/SKILL.md", "*/sections/*.md"], "inputs": ["*/SKILL.md.tmpl", "*/sections/*.tmpl"] } + ] +} +``` + +**Gate paths for the machinery itself.** An edit to `lib/review-depth.ts`, +`lib/review-collect.ts`, `bin/gstack-review-plan`, `lib/review-scope.ts`, +`lib/ship-queue*.ts`, `lib/regen.ts`, `.gstack/generated.json`, +`ship/sections/review-army.md.tmpl`, `ship/sections/adversarial.md.tmpl` or +`scripts/resolvers/review-army.ts` is gate class: changing how much review runs gets +the most review. `rules=` (`DEPTH_RULES`) hashes the deciding constants and patterns, +not file bytes, so a comment edit keeps it. + +**`--depth-floor standard|full`** (a /ship prompt flag, passed to every plan run) +raises the depth and never lowers it. + +**Numbers keep the outside voice.** At light depth the outside voice runs only when +the diff publishes numeric claims: `gstack-review-plan` scans the added lines of +prose and data files for percentages, money, multipliers, durations, token or credit +counts and score, rate or speed claims (`NUMBERS: yes`). The claims' fingerprint +(`fingerprint=`) is passed back as `--reviewed-numbers`; when a later fix changes a +claim, only the outside voice reruns, on the numbers. It always reads the whole diff. **The adversarial and outside voices run once per invocation** (Step 11, on the final tree), as the last full-pass plan lists them; after they ran, decision 2 @@ -166,6 +215,15 @@ default. `gstack-config set review_effort_tiers off` (or `--no-tiers`) runs the audits and watchers at the host default too ([model-policy.md](model-policy.md#reasoning-effort)). +### Behavior changes + +- A tests-only diff (runtime 0) no longer dispatches the Testing specialist: the + core checklist and the gate scan review it. +- Light no longer runs a separate Red Team: the batched specialist carries its + prompt. At runtime 0 light runs no adversarial pass either. +- Standard reads thresholds on runtime lines, not the whole diff, so a test-heavy + diff with few runtime lines dispatches fewer specialists. + ## What the receipt records `gstack-review-scope summary --since ` reads this invocation's @@ -268,9 +326,10 @@ trailers, appended }`. ## Where this is enforced -- `scripts/resolvers/review-army.ts`: the floor and `NO FINDINGS` in every specialist and Red Team prompt (rendered into `review/sections/review-army.md` and `ship/sections/review-army.md`), and the lead judgment's "On record" line that turns a [ruled class](#rulings-below-the-floor) into a note; in `/ship` every specialist and Red Team prompt it renders carries its diff base as a literal sha the parent writes in (`DIFF_BASE=`: the merge base on pass 1, `RANGE_BASE` from pass 2 on), because a reviewer is its own process and never sees the parent's shell variables; the fan-out batches above. -- `ship/sections/review-army.md` Step 9: passes, `RANGE_BASE`, classify, defer, render, the cap check before any edit, the Step 11 late-defer row, scratch files under `.gstack/tmp/`, the cap decisions; `ship/sections/adversarial.md` Step 11: the outside voice reads the same range and scopes queued fixes the same way. -- `lib/review-depth.ts`, `bin/gstack-review-plan`: the diff class, the transition table, the numeric-claim scan and each role's effort tier (`test/review-depth.test.ts`, `test/review-plan-cli.test.ts`). +- `scripts/resolvers/review-army.ts`: the floor and `NO FINDINGS` in every specialist and Red Team prompt (rendered into `review/sections/review-army.md` and `ship/sections/review-army.md`), and the lead judgment's "On record" line that turns a [ruled class](#rulings-below-the-floor) into a note; in `/ship` every specialist and Red Team prompt it renders carries its diff base as a literal sha the parent writes in (`DIFF_BASE=`: the merge base on pass 1, `RANGE_BASE` from pass 2 on) beside its plan's `DIFF_CMD` excludes, because a reviewer is its own process and never sees the parent's shell variables; the fan-out batches above. +- `ship/sections/review-army.md` Step 9: the plan's `REVIEW_DEPTH`, `GATE_SCAN` and `--depth-floor` on every run, the depth fields of the review-log row, passes, `RANGE_BASE`, classify, defer, render, the cap check before any edit, the Step 11 late-defer row, scratch files under `.gstack/tmp/`, the cap decisions; `ship/sections/adversarial.md` Step 11: the outside voice reads the same range and scopes queued fixes the same way. +- `lib/review-depth.ts`, `bin/gstack-review-plan`: the diff class, the depth (`depthFor`, `DEPTH_RULES`, `depthOfCommitInput`), each dispatch's `exclude` pathspecs and `DIFF_CMD`, the transition table, the numeric-claim scan and each role's effort tier (`test/review-depth.test.ts`, `test/review-plan-cli.test.ts`). +- `lib/review-collect.ts`: `collectCommit` (git objects only, from any checkout) and `collectLive` (the working tree), per-path lines and the base commit's registry (`test/review-collect.test.ts`). - `lib/review-scope.ts`, `bin/gstack-review-scope`: `classify`, `defer`, `render`, `summary` (with `fix_checks`, `dispatches`, `est_input_tokens`); refusals are `REVIEW_SCOPE_INPUT` and `REVIEW_SCOPE_RANGE` ([troubleshooting.md](troubleshooting.md#review-scope-input)). - `lib/ship-policy.ts`: the `review_scope` key and its `ship: review_scope …` consequence line. - `review/checklist.md`: `prereg-with-results` under Gate Integrity and `test-keyboard-count-loop` under Pass 2; `bin/gstack-coauthor-trailers` for the contributor-credit trailers of a port. diff --git a/lib/review-collect.ts b/lib/review-collect.ts new file mode 100644 index 000000000..b6c236a83 --- /dev/null +++ b/lib/review-collect.ts @@ -0,0 +1,137 @@ +/** + * review-collect — the git reads behind a review depth (docs/review-policy.md#review-depth), + * kept out of lib/review-depth.ts so the policy stays pure. collectCommit reads git + * objects only (a range between two commits, from any checkout, dirty or not); + * collectLive reads the working tree as /ship reviews it (committed, tracked + * uncommitted edits and untracked files). Both return a ClassInput with per-path + * line counts, the base commit's generated-output registry and collector errors; + * the caller adds the gate-scan result. + */ +import { spawnSync } from 'node:child_process'; +import * as fs from 'node:fs'; +import * as path from 'node:path'; +import { GENERATED_PATH, parseRegistry } from './regen'; +import { RESULT_CODES } from './result-codes'; +import type { ClassInput, DepthError, PathLines, RegistryInfo } from './review-depth'; + +export interface Collected extends ClassInput { + perPath: PathLines[]; + registry: RegistryInfo; + errors: DepthError[]; + deleted: string[]; + stampOnly: string[]; + /** Added lines per path, for scanNumbers. */ + added: Record; + diffLines: number; + mergeBase: string | null; + untracked: string[]; +} + +function git(root: string, args: string[]): { ok: boolean; out: string; err: string } { + const r = spawnSync('git', ['-c', 'core.quotePath=false', ...args], { cwd: root, encoding: 'utf8', timeout: 60_000, maxBuffer: 256 * 1024 * 1024 }); + return { ok: r.status === 0, out: r.stdout ?? '', err: (r.stderr ?? '').trim() }; +} +const scratch = (p: string) => p.startsWith('.gstack/tmp/'); +const oneLine = (s: string) => s.replace(/[\0-\x1f]+/g, ' ').trim().slice(0, 200); +const unavailable = (input: string, detail: string): RegistryInfo => ({ sha: null, outputs: [], error: { code: 'registry_unavailable', detail: oneLine(detail), input, repair: 'fetch the base branch (git fetch origin ) so its commit and registry resolve' } }); + +/** The registry as committed at `sha`: outputs of entries with a command (a `command: null` entry never excludes); a missing file is an empty registry. */ +export function loadRegistryAt(root: string, sha: string): RegistryInfo { + const commit = git(root, ['rev-parse', '--verify', '--quiet', `${sha}^{commit}`]); + if (!commit.ok) return unavailable(`${sha}:${GENERATED_PATH}`, `${sha} is not a commit in this repository`); + const resolved = commit.out.trim(); + const input = `${resolved.slice(0, 12)}:${GENERATED_PATH}`; + if (!git(root, ['cat-file', '-e', `${resolved}:${GENERATED_PATH}`]).ok) return { sha: resolved, outputs: [] }; + const show = git(root, ['show', `${resolved}:${GENERATED_PATH}`]); + if (!show.ok) return unavailable(input, show.err || 'git show failed'); + const { registry, errors } = parseRegistry(show.out); + const e = errors[0]; + if (e) return { sha: resolved, outputs: [], error: { code: 'registry_invalid', detail: oneLine(`${e.path || GENERATED_PATH}: ${e.message}`), input, repair: RESULT_CODES[e.code].fix, result: e.code } }; + return { sha: resolved, outputs: registry.entries.filter(x => x.command !== null).flatMap(x => x.outputs) }; +} + +/** Numstat, deletions and the -U0 patch for `range` (`[mergeBase, head]` or `[mergeBase]` for the working tree). */ +function readDiff(root: string, range: string[], errors: DepthError[]): Pick { + const numstat = git(root, ['diff', '--numstat', '-z', '--no-renames', ...range]); + const status = git(root, ['diff', '--name-status', '-z', '--no-renames', '--diff-filter=D', ...range]); + const patch = git(root, ['diff', '-U0', '--no-color', '--no-ext-diff', '--no-renames', ...range]); + const empty = { perPath: [], deleted: [], stampOnly: [], added: {} }; + for (const [r, what] of [[numstat, '--numstat'], [status, '--name-status'], [patch, '-U0']] as const) { + if (!r.ok) { errors.push({ code: 'numstat_failed', detail: oneLine(r.err || 'git diff failed'), input: `git diff ${what} ${range.join(' ')}`, repair: 'check that both commits exist locally (git fetch origin ) and rerun' }); return empty; } + } + const perPath: PathLines[] = []; + for (const rec of numstat.out.split('\0').filter(Boolean)) { + const [a, d, ...rest] = rec.split('\t'); + const p = rest.join('\t'); + if (!scratch(p)) perPath.push({ path: p, lines: (Number(a) || 0) + (Number(d) || 0), binary: a === '-' && d === '-' }); + } + const statusParts = status.out.split('\0'); + const deleted = statusParts.flatMap((s, k) => (s === 'D' && statusParts[k + 1] ? [statusParts[k + 1]!] : [])).filter(p => !scratch(p)); + const added: Record = {}; + const changed: Record = {}; + let cur = ''; + let minus = ''; + let inHunk = false; + for (const line of patch.out.split('\n')) { + if (line.startsWith('diff --git ')) { inHunk = false; cur = ''; minus = ''; continue; } + if (!inHunk) { + if (line.startsWith('--- ')) minus = line === '--- /dev/null' ? '' : line.slice(6).replace(/\t$/, ''); + else if (line.startsWith('+++ ')) cur = line === '+++ /dev/null' ? minus : line.slice(6).replace(/\t$/, ''); + else if (line.startsWith('@@')) inHunk = true; + continue; + } + if (!cur || scratch(cur) || line.startsWith('@@')) continue; + if (line[0] === '+') (added[cur] ??= []).push(line.slice(1)); + if (line[0] === '+' || line[0] === '-') (changed[cur] ??= []).push(line); + } + const stampOnly = perPath.map(e => e.path).filter(p => p.endsWith('package.json') && !!changed[p]?.length && changed[p]!.every(l => /^[+-]\s*"version"\s*:/.test(l))); + return { perPath, deleted, stampOnly, added }; +} + +function result(diff: ReturnType, extra: Pick): Collected { + const paths = diff.perPath.map(e => e.path).sort(); + return { ...diff, ...extra, paths, gateTagged: null, diffLines: diff.perPath.reduce((n, e) => n + e.lines, 0) }; +} + +/** The range `baseSha`...`headSha` from git objects alone; the registry is the base commit's, never the head's. */ +export function collectCommit(root: string, baseSha: string, headSha: string): Collected { + const errors: DepthError[] = []; + const registry = loadRegistryAt(root, baseSha); + const mb = git(root, ['merge-base', baseSha, headSha]); + if (!mb.ok) { + errors.push({ code: 'numstat_failed', detail: oneLine(mb.err || 'no merge base'), input: `git merge-base ${baseSha} ${headSha}`, repair: 'check that both commits exist locally (git fetch origin ) and rerun' }); + return result({ perPath: [], deleted: [], stampOnly: [], added: {} }, { root: [], registry, errors, mergeBase: null, untracked: [] }); + } + const mergeBase = mb.out.trim(); + const diff = readDiff(root, [mergeBase, headSha], errors); + const tree = git(root, ['ls-tree', '-r', '--name-only', '-z', headSha]); + const skillDirs = [...new Set(tree.out.split('\0').flatMap(p => /^([^/]+)\/SKILL\.md(\.tmpl)?$/.exec(p)?.[1] ?? []))].sort(); + return result(diff, { root: skillDirs, registry, errors, mergeBase, untracked: [] }); +} + +/** The working tree against the merge base with origin/ (or ), untracked files counted per path; the registry is the base ref's commit. */ +export function collectLive(root: string, base: string): Collected { + const errors: DepthError[] = []; + const ref = git(root, ['rev-parse', '--verify', '-q', `origin/${base}^{commit}`]).ok ? `origin/${base}` : base; + const registry = loadRegistryAt(root, ref); + const mb = git(root, ['merge-base', ref, 'HEAD']); + if (!mb.ok) { + errors.push({ code: 'numstat_failed', detail: oneLine(mb.err || `no merge base with ${ref}`), input: `git merge-base ${ref} HEAD`, repair: `fetch the base first (git fetch origin ${base})` }); + return result({ perPath: [], deleted: [], stampOnly: [], added: {} }, { root, registry, errors, mergeBase: null, untracked: [] }); + } + const mergeBase = mb.out.trim(); + const diff = readDiff(root, [mergeBase], errors); + const others = git(root, ['ls-files', '--others', '--exclude-standard', '-z']); + if (!others.ok) errors.push({ code: 'numstat_failed', detail: oneLine(others.err || 'git ls-files failed'), input: 'git ls-files --others --exclude-standard', repair: 'run the command by hand and fix what git reports' }); + const untracked = others.out.split('\0').filter(p => p && !scratch(p)).sort(); + for (const p of untracked) { + let buf: Buffer; + try { buf = fs.readFileSync(path.join(root, p)); } catch { diff.perPath.push({ path: p, lines: 0, binary: true }); continue; } + const binary = buf.subarray(0, 8000).includes(0); + const text = binary ? '' : buf.toString('utf8'); + const lines = text ? text.split('\n').length - (text.endsWith('\n') ? 1 : 0) : 0; + diff.perPath.push({ path: p, lines, binary }); + if (!binary) diff.added[p] = text.split('\n'); + } + return result(diff, { root, registry, errors, mergeBase, untracked }); +} diff --git a/lib/review-depth.ts b/lib/review-depth.ts index f7331dbba..0cae65d45 100644 --- a/lib/review-depth.ts +++ b/lib/review-depth.ts @@ -1,17 +1,22 @@ /** * review-depth — the one policy owner for how much review a /ship run dispatches - * (docs/review-policy.md#review-depth). A pure function from the changed paths, - * gstack-diff-scope flags, the diff size, the gate scan and the pass history to a - * diff class, the next step of the loop and the reviewers that step dispatches, - * each with its reasoning-effort tier. bin/gstack-review-plan prints it; the + * (docs/review-policy.md#review-depth). A pure function from the changed paths and + * their line counts, gstack-diff-scope flags, the base registry's generated outputs, + * the gate scan and the pass history to a diff class, a review depth (light, + * standard, full), the next step of the loop and the reviewers that step + * dispatches, each with its effort tier and the pathspecs it leaves out. Git reads + * live in lib/review-collect.ts; bin/gstack-review-plan prints the plan; the * review-army resolver and the skills read the same definitions. */ import { createHash } from 'node:crypto'; import * as fs from 'node:fs'; import * as path from 'node:path'; +import type { ResultCodeName } from './result-codes'; +import { matchesAny } from './ship-policy'; export type DiffClass = 'docs' | 'tests' | 'code' | 'gate'; export type PathClass = 'docs' | 'tests' | 'code'; +export type Depth = 'light' | 'standard' | 'full'; export type Effort = 'light' | 'full' | 'host'; export type Role = 'specialist' | 'red-team' | 'adversarial-native' | 'outside-voice' | 'fix-check'; export type Step = 'full' | 'fix-check' | 'done' | 'stop'; @@ -22,7 +27,12 @@ export const FULL_PASS_CAP = 2; export const SMALL_DIFF_LINES = 50; export const RED_TEAM_LINES = 200; export const LARGE_DIFF_LINES = 100; +/** The checklists light's batched specialist always reads, whatever the scope. */ +export const LIGHT_FIXED: readonly Specialist[] = ['testing', 'maintainability', 'security']; +/** Why light with runtime above 0 keeps the native adversarial pass as its own dispatch. */ +export const LIGHT_ADVERSARIAL_REASON = 'backtest inconclusive (GSTA-147 D5)'; const CLASS_RANK: Record = { docs: 0, tests: 1, code: 2, gate: 3 }; +const DEPTH_RANK: Record = { light: 0, standard: 1, full: 2 }; const AGENT_PROSE = [/(^|\/)SKILL\.md(\.tmpl)?$/, /(^|\/)sections\//, /^scripts\/resolvers\//, /^docs\/[^/]*-policy\.md$/, /^docs\/(captain|unattended|pregate)\.md$/, /(^|\/)DRIVER\.md(\.tmpl)?$/, /(^|\/)templates\//, /(^|\/)(AGENTS|CLAUDE|ETHOS)\.md$/, /(^|\/)specialists\//, /(^|\/)checklist\.md$/, /^\.gstack\//]; /** gstack-diff-scope's TEST_PATH_RE (the test-bootstrap rule), plus fixture directories. */ @@ -30,19 +40,41 @@ const TEST_PATH = [/(^|\/)(tests?|spec|__tests__)\/|(^|\/)tests?\.py$|(^|\/)test const DOC_PATH = /\.(md|mdx|markdown|rst|txt|adoc)$/i; /** Release metadata a version stamp writes: never a reason for review depth on its own. */ const RELEASE_METADATA = [/^CHANGELOG\.md$/, /^VERSION$/, /^agents-digest\//]; -const GATE_PATH = [/(^|\/)\.github\/workflows\//, /(^|\/)(secrets?|credentials?|keychain|token[s]?)([._/-]|$)/i, /state-root/, /(^|\/)(pregate|gate|ship-receipt|ship-policy)[^/]*\.(ts|sh|json)$/, /(^|\/)bin\/gstack-(pregate|gate|ship-receipt|verify-gate|ci-gate)$/]; +/** Gate paths, the review-depth machinery included: an edit to how much review runs gets the most review. */ +const GATE_PATH = [ + /(^|\/)\.github\/workflows\//, /(^|\/)(secrets?|credentials?|keychain|token[s]?)([._/-]|$)/i, /state-root/, /(^|\/)(pregate|gate|ship-receipt|ship-policy)[^/]*\.(ts|sh|json)$/, /(^|\/)bin\/gstack-(pregate|gate|ship-receipt|verify-gate|ci-gate)$/, + /^lib\/review-(depth|collect|scope)\.ts$/, /^bin\/gstack-review-plan$/, /^lib\/ship-queue[^/]*\.ts$/, /^lib\/regen\.ts$/, /^\.gstack\/generated\.json$/, /^ship\/sections\/(review-army|adversarial)\.md\.tmpl$/, /^scripts\/resolvers\/review-army\.ts$/, +]; -/** One changed path's class. Markdown an agent or a gate reads (skills, sections, resolvers, policy and rule docs, templates, any markdown in a skill directory under `root`) is code: it is gstack's product. */ -export function classifyPath(p: string, root?: string): PathClass { +/** A hash of the constants and patterns that decide a depth, not of this file's bytes: a comment edit keeps it, a threshold edit changes it. */ +export const DEPTH_RULES = createHash('sha256').update(JSON.stringify({ + SMALL_DIFF_LINES, RED_TEAM_LINES, LARGE_DIFF_LINES, FULL_PASS_CAP, LIGHT_FIXED, + patterns: Object.entries({ GATE_PATH, AGENT_PROSE, TEST_PATH, RELEASE_METADATA, DOC_PATH: [DOC_PATH] }).map(([k, v]) => [k, v.map(r => `/${r.source}/${r.flags}`)]), +})).digest('hex').slice(0, 16); + +/** + * One changed path's class. Markdown an agent or a gate reads (skills, sections, resolvers, policy and rule docs, + * templates, any markdown in a skill directory) is code: it is gstack's product. `root` is the repository root + * (skill directories probed on disk) or the top-level skill directory names read from a commit. + */ +export function classifyPath(p: string, root?: string | readonly string[]): PathClass { if (RELEASE_METADATA.some(r => r.test(p))) return 'docs'; if (AGENT_PROSE.some(r => r.test(p))) return 'code'; if (TEST_PATH.some(r => r.test(p))) return 'tests'; if (!DOC_PATH.test(p)) return 'code'; const top = p.includes('/') ? p.split('/')[0]! : ''; - if (root && top && ['SKILL.md', 'SKILL.md.tmpl'].some(f => fs.existsSync(path.join(root, top, f)))) return 'code'; - return 'docs'; + if (!top || !root) return 'docs'; + if (typeof root === 'string') return ['SKILL.md', 'SKILL.md.tmpl'].some(f => fs.existsSync(path.join(root, top, f))) ? 'code' : 'docs'; + return root.includes(top) ? 'code' : 'docs'; } +export type DepthErrorCode = 'scope_error' | 'gate_scan_unavailable' | 'registry_unavailable' | 'registry_invalid' | 'numstat_failed' | 'binary_code_path'; +/** Why a depth failed closed to full: the input it could not read, a sanitized cause and the repair; `result` is the result code behind it when one exists. */ +export interface DepthError { code: DepthErrorCode; detail: string; input: string; repair: string; result?: ResultCodeName } +export interface PathLines { path: string; lines: number; binary: boolean } +/** The base commit's `.gstack/generated.json`: outputs of entries with a command only; `error` when it could not be read or validated. */ +export interface RegistryInfo { sha: string | null; outputs: readonly string[]; error?: DepthError } + export interface ClassInput { paths: readonly string[]; /** gstack-diff-scope output (SCOPE_* keys). */ @@ -53,23 +85,38 @@ export interface ClassInput { deleted?: readonly string[]; /** Paths whose whole change is a version stamp (package.json's version key): classified as docs. */ stampOnly?: readonly string[]; - /** The repository root: markdown inside a skill directory there is code. */ - root?: string; + /** The repository root, or the skill directory names at the reviewed commit: markdown inside a skill directory is code. */ + root?: string | readonly string[]; + /** Changed lines per path; without it a code diff counts every line as runtime. */ + perPath?: readonly PathLines[]; + registry?: RegistryInfo; + /** Collector failures (a diff git could not list): each fails the depth closed. */ + errors?: readonly DepthError[]; } -export interface ClassResult { class: DiffClass; reasons: string[] } +export interface ClassResult { class: DiffClass; reasons: string[]; errors: DepthError[]; runtimeLines: number | null } -/** The diff's class: the most restrictive of its paths, raised to gate by any gate signal; errors fail closed. */ +const sanitize = (s: string) => s.replace(/[\0-\x1f]+/g, ' ').slice(0, 200); +const isOutput = (i: ClassInput, p: string) => !!i.registry?.outputs.length && matchesAny(i.registry.outputs, p); +const pathClass = (i: ClassInput, p: string): PathClass => ((i.stampOnly ?? []).includes(p) ? 'docs' : classifyPath(p, i.root)); + +/** The diff's class, raised to gate by any gate signal; `errors` lists every input that fails the depth closed; `runtimeLines` counts code-class lines outside the base registry's outputs. */ export function classifyDiff(i: ClassInput): ClassResult { const reasons: string[] = []; + const errors: DepthError[] = [...(i.errors ?? [])]; let cls: DiffClass = 'docs'; const raise = (c: DiffClass, why: string) => { if (CLASS_RANK[c] > CLASS_RANK[cls]) cls = c; reasons.push(why); }; if (!i.paths.length) raise('docs', 'no changed paths'); - for (const p of i.paths) { const c = (i.stampOnly ?? []).includes(p) ? 'docs' : classifyPath(p, i.root); if (CLASS_RANK[c] > CLASS_RANK[cls]) raise(c, `${c}: ${p}`); } + for (const p of i.paths) { const c = pathClass(i, p); if (CLASS_RANK[c] > CLASS_RANK[cls]) raise(c, `${c}: ${p}`); } const s = i.scope ?? {}; - if (s.SCOPE_ERROR) raise('code', `scope error: ${s.SCOPE_ERROR}`); + if (s.SCOPE_ERROR) { + raise('code', `scope error: ${s.SCOPE_ERROR}`); + errors.push({ code: 'scope_error', detail: sanitize(s.SCOPE_ERROR), input: 'gstack-diff-scope ', repair: 'run gstack-diff-scope by hand and fix what it reports' }); + } if (CLASS_RANK[cls] >= CLASS_RANK.tests) { - if (i.gateTagged === null) raise('gate', 'gate scan unavailable'); - else if (i.gateTagged > 0) raise('gate', `gate scan tagged ${i.gateTagged} hunk(s)`); + if (i.gateTagged === null) { + raise('gate', 'gate scan unavailable'); + errors.push({ code: 'gate_scan_unavailable', detail: 'gstack-gate-diff reported no GATE_SUMMARY', input: 'gstack-gate-diff ', repair: 'run gstack-gate-diff and read its GATE_ERROR line, or pass the recorded scan as --gate-tagged ' }); + } else if (i.gateTagged > 0) raise('gate', `gate scan tagged ${i.gateTagged} hunk(s)`); const deletedGuarded = (i.deleted ?? []).filter(p => classifyPath(p, i.root) !== 'docs'); if (deletedGuarded.length) raise('gate', `deletes ${deletedGuarded.length} code or test path(s)`); } @@ -77,8 +124,38 @@ export function classifyDiff(i: ClassInput): ClassResult { if (s.SCOPE_AUTH === 'true') raise('gate', 'auth paths'); if (s.SCOPE_MIGRATIONS === 'true') raise('gate', 'migrations'); for (const p of i.paths) if (GATE_PATH.some(r => r.test(p))) { raise('gate', `gate path: ${p}`); break; } + if (i.registry?.error) errors.push(i.registry.error); } - return { class: cls, reasons }; + if (!i.perPath) return { class: cls, reasons, errors, runtimeLines: null }; + let runtimeLines = 0; + for (const e of i.perPath) { + if (pathClass(i, e.path) !== 'code' || isOutput(i, e.path)) continue; + if (e.binary) errors.push({ code: 'binary_code_path', detail: 'git reports a binary change, so its lines cannot be counted', input: e.path, repair: "register it as an output of an entry with a command in the base branch's .gstack/generated.json, or accept the full review" }); + else runtimeLines += e.lines; + } + return { class: cls, reasons, errors, runtimeLines }; +} + +/** Any classifier error or gate class is full; runtime above SMALL_DIFF_LINES is standard; otherwise light. A floor only raises. */ +export function depthFor(o: { cls: DiffClass; runtimeLines: number; errors: readonly DepthError[]; floor?: Depth }): Depth { + const d: Depth = o.errors.length || o.cls === 'gate' ? 'full' : o.runtimeLines > SMALL_DIFF_LINES ? 'standard' : 'light'; + return o.floor && DEPTH_RANK[o.floor] > DEPTH_RANK[d] ? o.floor : d; +} + +export interface DepthResult { depth: Depth; class: DiffClass; runtimeLines: number; reasons: string[]; errors: DepthError[]; codes: ResultCodeName[]; rules: string } + +function decideDepth(c: ClassResult, runtimeLines: number, floor?: Depth): DepthResult { + const unfloored = depthFor({ cls: c.class, runtimeLines, errors: c.errors }); + const reasons = [...c.errors.map(e => `error:${e.code}`), ...(c.class === 'gate' ? ['gate-class'] : []), `runtime=${runtimeLines}${runtimeLines > SMALL_DIFF_LINES ? '>' : '<='}${SMALL_DIFF_LINES}`]; + if (floor && DEPTH_RANK[floor] > DEPTH_RANK[unfloored]) reasons.push(`floor:${floor}`); + const codes = [...new Set(c.errors.flatMap(e => (e.result ? [e.result] : [])))]; + return { depth: depthFor({ cls: c.class, runtimeLines, errors: c.errors, floor }), class: c.class, runtimeLines, reasons, errors: c.errors, codes, rules: DEPTH_RULES }; +} + +/** The depth of a collected range (lib/review-collect.ts) plus a recorded gate-scan result: what the queue recomputes at a head. */ +export function depthOfCommitInput(input: ClassInput & { floor?: Depth }): DepthResult { + const c = classifyDiff(input); + return decideDepth(c, c.runtimeLines ?? 0, input.floor); } // --------------------------------------------------------------------------- @@ -153,34 +230,48 @@ export interface PlanInput extends ClassInput { force?: readonly string[] | 'all'; /** Specialists the hit-rate stats tag GATE_CANDIDATE. */ gated?: readonly string[]; - /** A specialist produced a CRITICAL finding (Red Team activation on code class). */ + /** A specialist produced a CRITICAL finding (Red Team activation on code class; raises light to standard). */ criticalFound?: boolean; /** Step 11's native adversarial pass already ran in this invocation. */ adversarialRan?: boolean; /** The numeric-claim fingerprint the outside voice last reviewed; a change reruns it. */ reviewedNumbers?: string; + /** --depth-floor: raises the depth, never lowers it. */ + floor?: Depth; } -export interface Dispatch { role: Role; name: string; effort: Effort } -export interface ReviewPlan { class: DiffClass; reasons: string[]; next: NextStep; numbers: NumbersResult; dispatches: Dispatch[]; skipped: string[] } +/** One reviewer. `exclude` holds git pathspec argv items the reviewer's diff leaves out; `checklists` names what light's batched specialist reads. */ +export interface Dispatch { role: Role; name: string; effort: Effort; exclude: string[]; checklists?: string[]; reason?: string } +export interface ReviewPlan extends DepthResult { + /** The depth this step runs at: light raised to standard by a fixed blocker, a CRITICAL or an open blocker. */ + stepDepth: Depth; + depthReasons: string[]; + next: NextStep; numbers: NumbersResult; dispatches: Dispatch[]; skipped: string[]; +} + +const scopeWants: Partial) => boolean>> = { + performance: s => s.SCOPE_BACKEND === 'true' || s.SCOPE_FRONTEND === 'true', + 'data-migration': s => s.SCOPE_MIGRATIONS === 'true', + 'api-contract': s => s.SCOPE_API === 'true', + design: s => s.SCOPE_FRONTEND === 'true', + 'state-machines': s => s.SCOPE_STATE_MACHINE === 'true' || s.SCOPE_STATE_MACHINE === 'unknown' || s.SCOPE_STATE_MACHINE === undefined, +}; +const forcedBy = (i: PlanInput) => (n: string) => i.force === 'all' || (Array.isArray(i.force) && i.force.includes(n)); /** The specialists a full pass selects for a class: class sets the eligible set, hit-rate gating applies inside it, gate class ignores gating. */ export function specialistsFor(cls: DiffClass, i: PlanInput, skipped: string[] = []): Specialist[] { const s = i.scope ?? {}; - const forced = (n: string) => i.force === 'all' || (Array.isArray(i.force) && i.force.includes(n)); + const forced = forcedBy(i); if (cls === 'docs') { skipped.push('specialists (docs class)'); return SPECIALISTS.filter(forced); } if (cls === 'tests') { skipped.push('specialists except testing (tests class)'); return SPECIALISTS.filter(n => n === 'testing' || forced(n)); } const want = (n: Specialist): boolean => { if (forced(n)) return true; - if (n === 'state-machines') return s.SCOPE_STATE_MACHINE === 'true' || s.SCOPE_STATE_MACHINE === 'unknown' || s.SCOPE_STATE_MACHINE === undefined; + if (n === 'state-machines') return scopeWants[n]!(s); if (i.diffLines < SMALL_DIFF_LINES) return false; switch (n) { case 'testing': case 'maintainability': return true; case 'security': return s.SCOPE_AUTH === 'true' || (s.SCOPE_BACKEND === 'true' && i.diffLines > LARGE_DIFF_LINES); - case 'performance': return s.SCOPE_BACKEND === 'true' || s.SCOPE_FRONTEND === 'true'; - case 'data-migration': return s.SCOPE_MIGRATIONS === 'true'; - case 'api-contract': return s.SCOPE_API === 'true'; - case 'design': return s.SCOPE_FRONTEND === 'true'; case 'simplification': return i.diffLines > LARGE_DIFF_LINES; + default: return scopeWants[n]!(s); } }; return SPECIALISTS.filter(n => { @@ -190,34 +281,76 @@ export function specialistsFor(cls: DiffClass, i: PlanInput, skipped: string[] = }); } -/** The reviewers the next step dispatches, with each role's effort tier. */ +/** A path as one git exclude pathspec; a glob character or a leading colon makes it literal. */ +const excludeSpec = (p: string) => `:(exclude${/[*?[\]\\]/.test(p) || p.startsWith(':') ? ',literal' : ''})${p}`; +const shellQuote = (s: string) => `'${s.replace(/'/g, "'\\''")}'`; + +/** The diff command a dispatched reviewer runs; `` is the literal sha the parent writes in. */ +export function diffCommand(d: Dispatch): string { + return d.exclude.length ? `git diff -- . ${d.exclude.map(shellQuote).join(' ')}` : 'git diff '; +} + +/** The reviewers the next step dispatches at its depth, with each role's effort tier and excluded pathspecs. */ export function planReview(i: PlanInput): ReviewPlan { - const { class: cls, reasons } = classifyDiff(i); + const c = classifyDiff(i); + const depth = decideDepth(c, c.runtimeLines ?? (CLASS_RANK[c.class] >= CLASS_RANK.code ? i.diffLines : 0), i.floor); + const { class: cls, runtimeLines } = depth; const next = nextStep(i.history); + const h = i.history; + const raise = depth.depth === 'light' && (h.fixedBlocking || h.blockingOpen || i.criticalFound); + const stepDepth: Depth = raise ? 'standard' : depth.depth; + const depthReasons = [...depth.reasons, ...(raise ? [`history:${h.fixedBlocking ? 'fixed-blocking' : h.blockingOpen ? 'blocking-open' : 'critical-found'}`] : [])]; const skipped: string[] = []; const dispatches: Dispatch[] = []; + const add = (role: Role, name: string, effort: Effort, exclude: string[] = [], extra: Partial = {}) => dispatches.push({ role, name, effort, exclude, ...extra }); + const docsAndOutputs = i.paths.filter(p => pathClass(i, p) === 'docs' || isOutput(i, p)).map(excludeSpec); + const tests = i.paths.filter(p => pathClass(i, p) === 'tests' && !isOutput(i, p)).map(excludeSpec); const numbersChanged = i.numbers.publishes && i.reviewedNumbers !== undefined && i.reviewedNumbers !== i.numbers.fingerprint; - if (next.step === 'full') { - for (const n of specialistsFor(cls, i, skipped)) dispatches.push({ role: 'specialist', name: n, effort: 'full' }); - const redTeam = cls === 'gate' || (cls === 'code' && (i.diffLines > RED_TEAM_LINES || !!i.criticalFound)); - if (redTeam) dispatches.push({ role: 'red-team', name: 'red-team', effort: 'full' }); - else skipped.push(`red-team (${cls} class${cls === 'code' ? `, ${i.diffLines} lines, no critical` : ''})`); - if (i.history.fullPasses === 0 || (!i.adversarialRan && (cls === 'code' || cls === 'gate'))) { - if (cls === 'docs' || cls === 'tests') skipped.push(`adversarial-native (${cls} class)`); - else dispatches.push({ role: 'adversarial-native', name: 'adversarial', effort: 'full' }); - if (cls === 'code' || cls === 'gate' || i.numbers.publishes) dispatches.push({ role: 'outside-voice', name: 'outside-voice', effort: 'host' }); - else skipped.push(`outside-voice (${cls} class, no numeric claims)`); + const firstVoices = h.fullPasses === 0 || !i.adversarialRan; + if (next.step === 'full' && stepDepth === 'light') { + const forced = forcedBy(i); + if (runtimeLines > 0) { + const scoped = SPECIALISTS.filter(n => !LIGHT_FIXED.includes(n) && (forced(n) || (scopeWants[n]?.(i.scope ?? {}) && !(i.gated ?? []).includes(n)))); + add('specialist', 'batched', 'full', docsAndOutputs, { checklists: [...LIGHT_FIXED, ...scoped, 'red-team'] }); + skipped.push('red-team (light depth: the batched specialist carries the Red Team prompt)'); + if (firstVoices) add('adversarial-native', 'adversarial', 'full', [], { reason: LIGHT_ADVERSARIAL_REASON }); + } else { + for (const n of SPECIALISTS.filter(forced)) add('specialist', n, 'full'); + skipped.push('specialists, red-team and adversarial-native (light depth, runtime 0)'); + } + if (h.fullPasses === 0 && i.numbers.publishes) add('outside-voice', 'outside-voice', 'host'); + else if (numbersChanged) add('outside-voice', 'outside-voice (numbers changed)', 'host'); + else skipped.push(`outside-voice (${cls} class, no numeric claims)`); + } else if (next.step === 'full') { + const full = stepDepth === 'full'; + const dcls: DiffClass = full ? 'gate' : cls; + const raised = !full && runtimeLines <= SMALL_DIFF_LINES; + const lines = full ? i.diffLines : Math.max(runtimeLines, raised ? SMALL_DIFF_LINES : 0); + for (const n of specialistsFor(dcls, { ...i, diffLines: lines }, skipped)) add('specialist', n, 'full', full ? [] : n === 'testing' ? docsAndOutputs : [...docsAndOutputs, ...tests]); + const redTeam = dcls === 'gate' || (dcls === 'code' && (lines > RED_TEAM_LINES || !!i.criticalFound || raised)); + if (redTeam) add('red-team', 'red-team', 'full'); + else skipped.push(`red-team (${dcls} class${dcls === 'code' ? `, ${lines} runtime lines, no critical` : ''})`); + const codeLike = dcls === 'code' || dcls === 'gate'; + if (h.fullPasses === 0 || (!i.adversarialRan && codeLike)) { + if (codeLike) add('adversarial-native', 'adversarial', 'full'); + else skipped.push(`adversarial-native (${dcls} class)`); + if (codeLike || i.numbers.publishes) add('outside-voice', 'outside-voice', 'host'); + else skipped.push(`outside-voice (${dcls} class, no numeric claims)`); } else { skipped.push('adversarial-native and outside-voice (run once per invocation)'); - if (numbersChanged) dispatches.push({ role: 'outside-voice', name: 'outside-voice (numbers changed)', effort: 'host' }); + if (numbersChanged) add('outside-voice', 'outside-voice (numbers changed)', 'host'); } } else if (next.step === 'fix-check') { - dispatches.push({ role: 'fix-check', name: 'fix-check', effort: 'full' }); - if (numbersChanged) dispatches.push({ role: 'outside-voice', name: 'outside-voice (numbers changed)', effort: 'host' }); + add('fix-check', 'fix-check', 'full'); + if (numbersChanged) add('outside-voice', 'outside-voice (numbers changed)', 'host'); } - return { class: cls, reasons, next, numbers: i.numbers, dispatches, skipped }; + return { ...depth, stepDepth, depthReasons, reasons: c.reasons, next, numbers: i.numbers, dispatches, skipped }; } export function reviewPlanLine(p: ReviewPlan, pass: number): string { return `REVIEW_PLAN: class=${p.class} pass=${pass} next=${p.next.step} dispatches=${p.dispatches.length} numbers=${p.numbers.publishes ? `yes(${p.numbers.claims})` : 'no'} fingerprint=${p.numbers.fingerprint}`; } + +export function reviewDepthLine(p: ReviewPlan): string { + return `REVIEW_DEPTH: ${p.depth} runtime_lines=${p.runtimeLines} rules=${p.rules} reasons=${p.depthReasons.join(',')} codes=${p.codes.join(',') || 'none'}${p.stepDepth !== p.depth ? ` step=${p.stepDepth}` : ''}`; +} diff --git a/scripts/resolvers/review-army.ts b/scripts/resolvers/review-army.ts index 1c174ac94..f4142ee0e 100644 --- a/scripts/resolvers/review-army.ts +++ b/scripts/resolvers/review-army.ts @@ -139,7 +139,7 @@ Every other specialist, including Testing, reads the full diff with fixtures. 5. Instructions: "You are a specialist code reviewer. Read the checklist at {checklist path}, then run -\`${diffBase(ctx)} && git diff "$DIFF_BASE"\` to get the ${ctx.skillName === 'ship' ? 'diff (``: write the sha in before dispatch, the merge base on pass 1 and `RANGE_BASE` from pass 2 on; never a shell variable)' : 'full diff'}. Apply the checklist against the diff. +\`${diffBase(ctx)} && git diff "$DIFF_BASE"${ctx.skillName === 'ship' ? ' ' : ''}\` to get the ${ctx.skillName === 'ship' ? 'diff (``: write the sha in before dispatch, the merge base on pass 1 and `RANGE_BASE` from pass 2 on; never a shell variable; ``: everything after `` on this reviewer\'s `DIFF_CMD:` line, verbatim; empty on `DIFF_EXCLUDE: none`)' : 'full diff'}. Apply the checklist against the diff. \`NO FINDINGS\` alone is the expected, complete result for a clean diff; never invent an item. Otherwise output one JSON object per finding per line, 400 words at most: {\\"severity\\":\\"CRITICAL|INFORMATIONAL\\",\\"confidence\\":N,\\"path\\":\\"file\\",\\"line\\":N,\\"category\\":\\"category\\",\\"summary\\":\\"description\\",\\"fix\\":\\"recommended fix\\",\\"fingerprint\\":\\"path:line:category\\",\\"specialist\\":\\"name\\"} diff --git a/scripts/review-dispatch-compare.ts b/scripts/review-dispatch-compare.ts index 966e4e1e6..d38198e64 100755 --- a/scripts/review-dispatch-compare.ts +++ b/scripts/review-dispatch-compare.ts @@ -17,7 +17,7 @@ import * as fs from 'node:fs'; import * as os from 'node:os'; import * as path from 'node:path'; import { runScriptSync } from '../lib/script-invocation'; -import { planReview, specialistsFor, type DiffClass, type Dispatch, type History, type NumbersResult } from '../lib/review-depth'; +import { planReview, specialistsFor, type DiffClass, type History, type NumbersResult, type PathLines } from '../lib/review-depth'; const ROOT = path.resolve(import.meta.dir, '..'); const BYTES_PER_TOKEN = 4; @@ -30,7 +30,8 @@ const DEFAULT_OUT: Record = { 'outside-voice': { full: 6000, light: 6000 }, 'fix-check': { full: 4000, light: 2000 }, audit: { full: 5000, light: 2500 }, 'autoplan-voice': { full: 9000, light: 4500 }, }; -export interface Fixture { name: string; commit: string; paths: string[]; deleted: string[]; stamp_only?: string[]; scope: Record; gate_tagged: number | null; diff_lines: number; diff_bytes: number; numbers: NumbersResult } +/** per_path and registry_outputs (captured since GSTA-147) feed the runtime-line count; an older fixture counts every code-class line as runtime. */ +export interface Fixture { name: string; commit: string; paths: string[]; deleted: string[]; stamp_only?: string[]; scope: Record; gate_tagged: number | null; diff_lines: number; diff_bytes: number; numbers: NumbersResult; per_path?: PathLines[]; registry_outputs?: string[] } /** clean: pass 1 finds nothing; nits: pass 1 fixes below-floor findings; blocker: pass 1 fixes a blocker; adversarial-fix: Step 11 finds a should that is fixed; fix-regression: a pass-1 fix introduces a blocker the next step finds. */ type Scenario = 'clean' | 'nits' | 'blocker' | 'adversarial-fix' | 'fix-regression'; const SCENARIOS: Scenario[] = ['clean', 'nits', 'blocker', 'adversarial-fix', 'fix-regression']; @@ -62,7 +63,7 @@ function legacyShip(f: Fixture, sc: Scenario): Call[] { } function base(f: Fixture) { - return { paths: f.paths, deleted: f.deleted, stampOnly: f.stamp_only ?? [], scope: f.scope, gateTagged: f.gate_tagged, diffLines: f.diff_lines, numbers: f.numbers }; + return { paths: f.paths, deleted: f.deleted, stampOnly: f.stamp_only ?? [], scope: f.scope, gateTagged: f.gate_tagged, diffLines: f.diff_lines, numbers: f.numbers, perPath: f.per_path, registry: f.registry_outputs ? { sha: null, outputs: f.registry_outputs } : undefined }; } /** The new policy, step by step through lib/review-depth's own transition table. */ @@ -121,10 +122,10 @@ function capture(sha: string, name: string, out: string): number { if (r.status !== 0) throw new Error(`gstack-review-plan failed: ${r.stderr}`); const plan = JSON.parse(r.stdout); const diffBytes = Buffer.byteLength(git(['diff', `${sha}^`, sha], dir).stdout ?? ''); - const fixture: Fixture = { name, commit: sha, paths: plan.paths, deleted: plan.deleted, stamp_only: plan.stamp_only, scope: plan.scope, gate_tagged: plan.gate_tagged, diff_lines: plan.diff_lines, diff_bytes: diffBytes, numbers: plan.numbers }; + const fixture: Fixture = { name, commit: sha, paths: plan.paths, deleted: plan.deleted, stamp_only: plan.stamp_only, scope: plan.scope, gate_tagged: plan.gate_tagged, diff_lines: plan.diff_lines, diff_bytes: diffBytes, numbers: plan.numbers, per_path: plan.per_path, registry_outputs: plan.registry.outputs }; fs.mkdirSync(path.dirname(out), { recursive: true }); fs.writeFileSync(out, JSON.stringify(fixture, null, 2) + '\n'); - process.stdout.write(`CAPTURED: ${name} ${sha} class=${plan.class} paths=${plan.paths.length} lines=${plan.diff_lines} -> ${out}\n`); + process.stdout.write(`CAPTURED: ${name} ${sha} class=${plan.class} depth=${plan.depth} runtime=${plan.runtime_lines} paths=${plan.paths.length} lines=${plan.diff_lines} -> ${out}\n`); return 0; } finally { git(['worktree', 'remove', '--force', dir]); @@ -135,12 +136,13 @@ function capture(sha: string, name: string, out: string): number { export function compare(fixtures: Fixture[], out: typeof DEFAULT_OUT) { const rows: Array> = []; for (const f of fixtures) { - const cls: DiffClass = planReview({ ...base(f), history: { fullPasses: 0, last: 'none' } }).class; + const first = planReview({ ...base(f), history: { fullPasses: 0, last: 'none' } }); + const cls: DiffClass = first.class; for (const sc of SCENARIOS) { const legacy = legacyShip(f, sc); const next = newShip(f, sc); const lt = tokens(legacy, out); const nt = tokens(next.calls, out); - rows.push({ fixture: f.name, class: cls, scenario: sc, legacy_dispatches: legacy.length, new_dispatches: next.calls.length, legacy_tokens: lt, new_tokens: nt, ratio: Math.round((nt / lt) * 100) / 100, ending: next.ending }); + rows.push({ fixture: f.name, class: cls, depth: first.depth, runtime_lines: first.runtimeLines, scenario: sc, legacy_dispatches: legacy.length, new_dispatches: next.calls.length, legacy_tokens: lt, new_tokens: nt, ratio: Math.round((nt / lt) * 100) / 100, ending: next.ending }); } } return rows; @@ -158,8 +160,8 @@ function main(argv: string[]): number { const rows = compare(fixtures, out); const ap = fixtures.map(f => ({ fixture: f.name, runs: autoplanRuns(planReview({ ...base(f), history: { fullPasses: 0, last: 'none' } }).class === 'docs') })); if (argv.includes('--json')) { process.stdout.write(JSON.stringify({ tokens_source: tokFile ?? 'DEFAULT', ship: rows, autoplan: ap }) + '\n'); return 0; } - const lines = [`TOKENS: ${tokFile ? `measured (${tokFile})` : 'DEFAULT estimates (pass --tokens for measured output)'}`, '', '| fixture | class | scenario | /ship dispatches legacy → new | est. tokens legacy → new | ratio | ending |', '|---|---|---|---|---|---|---|']; - for (const r of rows) lines.push(`| ${r.fixture} | ${r.class} | ${r.scenario} | ${r.legacy_dispatches} → ${r.new_dispatches} | ${r.legacy_tokens} → ${r.new_tokens} | ${r.ratio} | ${r.ending} |`); + const lines = [`TOKENS: ${tokFile ? `measured (${tokFile})` : 'DEFAULT estimates (pass --tokens for measured output)'}`, '', '| fixture | class | depth (runtime lines) | scenario | /ship dispatches legacy → new | est. tokens legacy → new | ratio | ending |', '|---|---|---|---|---|---|---|---|']; + for (const r of rows) lines.push(`| ${r.fixture} | ${r.class} | ${r.depth} (${r.runtime_lines}) | ${r.scenario} | ${r.legacy_dispatches} → ${r.new_dispatches} | ${r.legacy_tokens} → ${r.new_tokens} | ${r.ratio} | ${r.ending} |`); lines.push('', '| fixture | /autoplan scenario | voice dispatches legacy → new |', '|---|---|---|'); for (const a of ap) for (const r of a.runs) lines.push(`| ${a.fixture} | ${r.scenario} | ${r.legacy} → ${r.next} |`); const median = (v: number[]) => { const m = Math.floor(v.length / 2); return v.length % 2 ? v[m]! : Math.round(((v[m - 1]! + v[m]!) / 2) * 100) / 100; }; @@ -168,6 +170,9 @@ function main(argv: string[]): number { const byClass = new Map(); for (const r of rows) byClass.set(String(r.class), [...(byClass.get(String(r.class)) ?? []), Number(r.ratio)].sort((x, y) => x - y)); lines.push(`SHIP_TOKEN_RATIO_BY_CLASS: ${[...byClass].map(([c, v]) => `${c} median=${median(v)} (n=${v.length})`).join('; ')}`); + const byDepth = new Map(); + for (const r of rows) byDepth.set(String(r.depth), [...(byDepth.get(String(r.depth)) ?? []), Number(r.ratio)].sort((x, y) => x - y)); + lines.push(`SHIP_TOKEN_RATIO_BY_DEPTH: ${[...byDepth].map(([d, v]) => `${d} median=${median(v)} (n=${v.length})`).join('; ')}`); process.stdout.write(lines.join('\n') + '\n'); return 0; } diff --git a/ship/SKILL.md b/ship/SKILL.md index 881cd3934..5846b96d3 100644 --- a/ship/SKILL.md +++ b/ship/SKILL.md @@ -202,11 +202,11 @@ token; `--finish` stamps the binding fields automatically. Never borrow or repla `gstack-wtree` prints a Git tree hash covering tracked and non-ignored untracked files, not a commit ID. Use `git diff ` to compare these snapshots. -**Review policy** (docs/review-policy.md): a finding blocks another review pass -only when it would fail a test, break production, or lose data or secrets; -everything below that floor ships as a named note in the PR body. Pass 1 reviews -the diff; a later pass reads only the range; two full passes plus one fix-check -is the cap. `NO FINDINGS` is a complete reviewer result. `ship: review_scope` (Step 3) +**Review policy** (docs/review-policy.md): only a finding that would fail a test, +break production, or lose data or secrets blocks a pass; the rest is a PR-body +note. Step 9's plan sets both reviews' depth (`--depth-floor standard|full` only +raises it). Pass 1 reads the diff, later passes the range; the cap is two full +passes and a fix-check. `NO FINDINGS` is complete. `ship: review_scope` (Step 3) is the knob; the receipt's `review_loop` records the outcome. ### Ship control flow diff --git a/ship/SKILL.md.tmpl b/ship/SKILL.md.tmpl index 16f65656e..810bbd990 100644 --- a/ship/SKILL.md.tmpl +++ b/ship/SKILL.md.tmpl @@ -68,11 +68,11 @@ token; `--finish` stamps the binding fields automatically. Never borrow or repla `gstack-wtree` prints a Git tree hash covering tracked and non-ignored untracked files, not a commit ID. Use `git diff ` to compare these snapshots. -**Review policy** (docs/review-policy.md): a finding blocks another review pass -only when it would fail a test, break production, or lose data or secrets; -everything below that floor ships as a named note in the PR body. Pass 1 reviews -the diff; a later pass reads only the range; two full passes plus one fix-check -is the cap. `NO FINDINGS` is a complete reviewer result. `ship: review_scope` (Step 3) +**Review policy** (docs/review-policy.md): only a finding that would fail a test, +break production, or lose data or secrets blocks a pass; the rest is a PR-body +note. Step 9's plan sets both reviews' depth (`--depth-floor standard|full` only +raises it). Pass 1 reads the diff, later passes the range; the cap is two full +passes and a fix-check. `NO FINDINGS` is complete. `ship: review_scope` (Step 3) is the knob; the receipt's `review_loop` records the outcome. ### Ship control flow diff --git a/ship/sections/adversarial.md b/ship/sections/adversarial.md index 3436b5c24..620f17248 100644 --- a/ship/sections/adversarial.md +++ b/ship/sections/adversarial.md @@ -2,7 +2,8 @@ **Step 9's last full-pass plan governs this step:** run the native pass only on `DISPATCH: adversarial-native` and the outside voice only on -`DISPATCH: outside-voice`. Without the native line, print `ADVERSARIAL: skipped (review plan)`; +`DISPATCH: outside-voice`. Without the native line (light depth at runtime 0; +above 0 the plan lists it beside the batched specialist), print `ADVERSARIAL: skipped (review plan)`; with neither line, take decision 3 of Finish the adversarial phase. ## Step 11: Adversarial review diff --git a/ship/sections/adversarial.md.tmpl b/ship/sections/adversarial.md.tmpl index abb9615ae..df982e205 100644 --- a/ship/sections/adversarial.md.tmpl +++ b/ship/sections/adversarial.md.tmpl @@ -1,6 +1,7 @@ **Step 9's last full-pass plan governs this step:** run the native pass only on `DISPATCH: adversarial-native` and the outside voice only on -`DISPATCH: outside-voice`. Without the native line, print `ADVERSARIAL: skipped (review plan)`; +`DISPATCH: outside-voice`. Without the native line (light depth at runtime 0; +above 0 the plan lists it beside the batched specialist), print `ADVERSARIAL: skipped (review plan)`; with neither line, take decision 3 of Finish the adversarial phase. {{ADVERSARIAL_STEP}} diff --git a/ship/sections/review-army.md b/ship/sections/review-army.md index 32f18e440..06419d047 100644 --- a/ship/sections/review-army.md +++ b/ship/sections/review-army.md @@ -14,13 +14,19 @@ Every repeat starts before the checklist read and captures a fresh REVIEW_START. Finish the complete review and QA before applying any fix in Step 9.4. **Review plan.** Pass 1 runs `~/.claude/skills/gstack/bin/gstack-review-plan ---base `; a repeat uses the plan run that picked it. Save its `REVIEW_PLAN:` line and -fingerprint. Dispatch exactly its `DISPATCH:` lines (specialists, Red Team, Step -11's voices), each at its `effort=` on the same model family, in place of 9.1's and -9.2's own selection rules; a `SKIP:` reviewer -logs `{"dispatched":false,"reason":"scope"}`. Audits run at `AUDIT_EFFORT:`. The -core checklist, gate scan and QA always run. Item 6's row adds -`"dispatches":[{"role":R,"prompt_bytes":N,"diff_bytes":N}]`. +--base `, plus `--depth-floor ` on every plan run when the /ship prompt +gave `--depth-floor ` (it only raises); a repeat uses the plan run that picked it. +Save its `REVIEW_PLAN:` line and fingerprint, `REVIEW_DEPTH:` (depth, +`runtime_lines`, `rules`) and `GATE_SCAN:`. Dispatch exactly its `DISPATCH:` lines +(specialists, Red Team, Step 11's voices), each at its `effort=` on the same model +family and reading its `DIFF_CMD:`, in place of 9.1's and 9.2's own selection rules; +`specialist batched` (light depth) is one subagent reading every checklist its +`checklists=` names, Red Team included, tagging each finding with its checklist. +A `SKIP:` reviewer logs `{"dispatched":false,"reason":"scope"}`. Audits run at +`AUDIT_EFFORT:`. The core checklist, gate scan and QA always run. Item 6's row adds +`"dispatches":[{"role":R,"prompt_bytes":N,"diff_bytes":N}]`, `"depth"`, +`"runtime_lines"`, `"depth_rules"` (the `rules` hash) and +`"gate_scan":{"tagged":N|null,"head":""}` from those lines. **Pass 2+ reads the range, not the tree** (`review_scope frozen`): the core read in item 2 becomes `git diff $RANGE_BASE`, and every dispatched prompt (specialists, @@ -391,7 +397,7 @@ Every other specialist, including Testing, reads the full diff with fixtures. 5. Instructions: "You are a specialist code reviewer. Read the checklist at {checklist path}, then run -`DIFF_BASE= && git diff "$DIFF_BASE"` to get the diff (``: write the sha in before dispatch, the merge base on pass 1 and `RANGE_BASE` from pass 2 on; never a shell variable). Apply the checklist against the diff. +`DIFF_BASE= && git diff "$DIFF_BASE" ` to get the diff (``: write the sha in before dispatch, the merge base on pass 1 and `RANGE_BASE` from pass 2 on; never a shell variable; ``: everything after `` on this reviewer's `DIFF_CMD:` line, verbatim; empty on `DIFF_EXCLUDE: none`). Apply the checklist against the diff. `NO FINDINGS` alone is the expected, complete result for a clean diff; never invent an item. Otherwise output one JSON object per finding per line, 400 words at most: {\"severity\":\"CRITICAL|INFORMATIONAL\",\"confidence\":N,\"path\":\"file\",\"line\":N,\"category\":\"category\",\"summary\":\"description\",\"fix\":\"recommended fix\",\"fingerprint\":\"path:line:category\",\"specialist\":\"name\"} @@ -802,7 +808,8 @@ After persistence, record missing output, CYCLES and fixes in the invocation rec Decision 1 always applies first. Then, under `frozen`, run `gstack-review-plan --base --pass $PASS --last full` with each flag that holds (`--fixed-blocking`, `--fixed-below`, `--blocking-open`, `--touched-gate` for a fix to an item 2.5 path, `--critical-found` when this pass -logged a CRITICAL, `--adversarial-ran` once Step 11's native pass ran, `--reviewed-numbers `); +logged a CRITICAL, `--adversarial-ran` once Step 11's native pass ran, `--reviewed-numbers `, +`--depth-floor ` when given); its `NEXT:` alone picks the next decision: `stop` 2, `full` 3, `fix-check` 3.5, `done` 4. Other scopes apply decisions 2–4 in order. diff --git a/ship/sections/review-army.md.tmpl b/ship/sections/review-army.md.tmpl index 6317b79c6..704bfb6b1 100644 --- a/ship/sections/review-army.md.tmpl +++ b/ship/sections/review-army.md.tmpl @@ -12,13 +12,19 @@ Every repeat starts before the checklist read and captures a fresh REVIEW_START. Finish the complete review and QA before applying any fix in Step 9.4. **Review plan.** Pass 1 runs `~/.claude/skills/gstack/bin/gstack-review-plan ---base `; a repeat uses the plan run that picked it. Save its `REVIEW_PLAN:` line and -fingerprint. Dispatch exactly its `DISPATCH:` lines (specialists, Red Team, Step -11's voices), each at its `effort=` on the same model family, in place of 9.1's and -9.2's own selection rules; a `SKIP:` reviewer -logs `{"dispatched":false,"reason":"scope"}`. Audits run at `AUDIT_EFFORT:`. The -core checklist, gate scan and QA always run. Item 6's row adds -`"dispatches":[{"role":R,"prompt_bytes":N,"diff_bytes":N}]`. +--base `, plus `--depth-floor ` on every plan run when the /ship prompt +gave `--depth-floor ` (it only raises); a repeat uses the plan run that picked it. +Save its `REVIEW_PLAN:` line and fingerprint, `REVIEW_DEPTH:` (depth, +`runtime_lines`, `rules`) and `GATE_SCAN:`. Dispatch exactly its `DISPATCH:` lines +(specialists, Red Team, Step 11's voices), each at its `effort=` on the same model +family and reading its `DIFF_CMD:`, in place of 9.1's and 9.2's own selection rules; +`specialist batched` (light depth) is one subagent reading every checklist its +`checklists=` names, Red Team included, tagging each finding with its checklist. +A `SKIP:` reviewer logs `{"dispatched":false,"reason":"scope"}`. Audits run at +`AUDIT_EFFORT:`. The core checklist, gate scan and QA always run. Item 6's row adds +`"dispatches":[{"role":R,"prompt_bytes":N,"diff_bytes":N}]`, `"depth"`, +`"runtime_lines"`, `"depth_rules"` (the `rules` hash) and +`"gate_scan":{"tagged":N|null,"head":""}` from those lines. **Pass 2+ reads the range, not the tree** (`review_scope frozen`): the core read in item 2 becomes `git diff $RANGE_BASE`, and every dispatched prompt (specialists, @@ -216,7 +222,8 @@ After persistence, record missing output, CYCLES and fixes in the invocation rec Decision 1 always applies first. Then, under `frozen`, run `gstack-review-plan --base --pass $PASS --last full` with each flag that holds (`--fixed-blocking`, `--fixed-below`, `--blocking-open`, `--touched-gate` for a fix to an item 2.5 path, `--critical-found` when this pass -logged a CRITICAL, `--adversarial-ran` once Step 11's native pass ran, `--reviewed-numbers `); +logged a CRITICAL, `--adversarial-ran` once Step 11's native pass ran, `--reviewed-numbers `, +`--depth-floor ` when given); its `NEXT:` alone picks the next decision: `stop` 2, `full` 3, `fix-check` 3.5, `done` 4. Other scopes apply decisions 2–4 in order. diff --git a/test/fixtures/golden/agy-ship-SKILL.md b/test/fixtures/golden/agy-ship-SKILL.md index 7494fe0c4..59c5baac2 100644 --- a/test/fixtures/golden/agy-ship-SKILL.md +++ b/test/fixtures/golden/agy-ship-SKILL.md @@ -202,11 +202,11 @@ token; `--finish` stamps the binding fields automatically. Never borrow or repla `gstack-wtree` prints a Git tree hash covering tracked and non-ignored untracked files, not a commit ID. Use `git diff ` to compare these snapshots. -**Review policy** (docs/review-policy.md): a finding blocks another review pass -only when it would fail a test, break production, or lose data or secrets; -everything below that floor ships as a named note in the PR body. Pass 1 reviews -the diff; a later pass reads only the range; two full passes plus one fix-check -is the cap. `NO FINDINGS` is a complete reviewer result. `ship: review_scope` (Step 3) +**Review policy** (docs/review-policy.md): only a finding that would fail a test, +break production, or lose data or secrets blocks a pass; the rest is a PR-body +note. Step 9's plan sets both reviews' depth (`--depth-floor standard|full` only +raises it). Pass 1 reads the diff, later passes the range; the cap is two full +passes and a fix-check. `NO FINDINGS` is complete. `ship: review_scope` (Step 3) is the knob; the receipt's `review_loop` records the outcome. ### Ship control flow diff --git a/test/fixtures/golden/claude-ship-SKILL.md b/test/fixtures/golden/claude-ship-SKILL.md index 881cd3934..5846b96d3 100644 --- a/test/fixtures/golden/claude-ship-SKILL.md +++ b/test/fixtures/golden/claude-ship-SKILL.md @@ -202,11 +202,11 @@ token; `--finish` stamps the binding fields automatically. Never borrow or repla `gstack-wtree` prints a Git tree hash covering tracked and non-ignored untracked files, not a commit ID. Use `git diff ` to compare these snapshots. -**Review policy** (docs/review-policy.md): a finding blocks another review pass -only when it would fail a test, break production, or lose data or secrets; -everything below that floor ships as a named note in the PR body. Pass 1 reviews -the diff; a later pass reads only the range; two full passes plus one fix-check -is the cap. `NO FINDINGS` is a complete reviewer result. `ship: review_scope` (Step 3) +**Review policy** (docs/review-policy.md): only a finding that would fail a test, +break production, or lose data or secrets blocks a pass; the rest is a PR-body +note. Step 9's plan sets both reviews' depth (`--depth-floor standard|full` only +raises it). Pass 1 reads the diff, later passes the range; the cap is two full +passes and a fix-check. `NO FINDINGS` is complete. `ship: review_scope` (Step 3) is the knob; the receipt's `review_loop` records the outcome. ### Ship control flow diff --git a/test/fixtures/golden/codex-ship-SKILL.md b/test/fixtures/golden/codex-ship-SKILL.md index c4d937f91..afd639848 100644 --- a/test/fixtures/golden/codex-ship-SKILL.md +++ b/test/fixtures/golden/codex-ship-SKILL.md @@ -200,11 +200,11 @@ token; `--finish` stamps the binding fields automatically. Never borrow or repla `gstack-wtree` prints a Git tree hash covering tracked and non-ignored untracked files, not a commit ID. Use `git diff ` to compare these snapshots. -**Review policy** (docs/review-policy.md): a finding blocks another review pass -only when it would fail a test, break production, or lose data or secrets; -everything below that floor ships as a named note in the PR body. Pass 1 reviews -the diff; a later pass reads only the range; two full passes plus one fix-check -is the cap. `NO FINDINGS` is a complete reviewer result. `ship: review_scope` (Step 3) +**Review policy** (docs/review-policy.md): only a finding that would fail a test, +break production, or lose data or secrets blocks a pass; the rest is a PR-body +note. Step 9's plan sets both reviews' depth (`--depth-floor standard|full` only +raises it). Pass 1 reads the diff, later passes the range; the cap is two full +passes and a fix-check. `NO FINDINGS` is complete. `ship: review_scope` (Step 3) is the knob; the receipt's `review_loop` records the outcome. ### Ship control flow diff --git a/test/fixtures/golden/factory-ship-SKILL.md b/test/fixtures/golden/factory-ship-SKILL.md index 374d879a4..e2c3e30bc 100644 --- a/test/fixtures/golden/factory-ship-SKILL.md +++ b/test/fixtures/golden/factory-ship-SKILL.md @@ -202,11 +202,11 @@ token; `--finish` stamps the binding fields automatically. Never borrow or repla `gstack-wtree` prints a Git tree hash covering tracked and non-ignored untracked files, not a commit ID. Use `git diff ` to compare these snapshots. -**Review policy** (docs/review-policy.md): a finding blocks another review pass -only when it would fail a test, break production, or lose data or secrets; -everything below that floor ships as a named note in the PR body. Pass 1 reviews -the diff; a later pass reads only the range; two full passes plus one fix-check -is the cap. `NO FINDINGS` is a complete reviewer result. `ship: review_scope` (Step 3) +**Review policy** (docs/review-policy.md): only a finding that would fail a test, +break production, or lose data or secrets blocks a pass; the rest is a PR-body +note. Step 9's plan sets both reviews' depth (`--depth-floor standard|full` only +raises it). Pass 1 reads the diff, later passes the range; the cap is two full +passes and a fix-check. `NO FINDINGS` is complete. `ship: review_scope` (Step 3) is the knob; the receipt's `review_loop` records the outcome. ### Ship control flow diff --git a/test/review-collect.test.ts b/test/review-collect.test.ts new file mode 100644 index 000000000..f611fe57a --- /dev/null +++ b/test/review-collect.test.ts @@ -0,0 +1,110 @@ +import { afterEach, describe, expect, test } from 'bun:test'; +import { spawnSync } from 'node:child_process'; +import * as fs from 'node:fs'; +import * as os from 'node:os'; +import * as path from 'node:path'; +import { collectCommit, collectLive, loadRegistryAt } from '../lib/review-collect'; +import { depthOfCommitInput } from '../lib/review-depth'; + +const dirs: string[] = []; +afterEach(() => { while (dirs.length) fs.rmSync(dirs.pop()!, { recursive: true, force: true }); }); +const lines = (n: number, tag = 'v') => Array.from({ length: n }, (_, i) => `export const ${tag}${i} = ${i};`).join('\n') + '\n'; +const REGISTRY = JSON.stringify({ entries: [{ id: 'docs', command: 'bun run gen', outputs: ['*/SKILL.md'] }, { id: 'manual', command: null, outputs: ['src/hand.ts'] }] }); + +function repo(baseFiles: Record) { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'review-collect-')); + dirs.push(dir); + const git = (...a: string[]) => { const r = spawnSync('git', a, { cwd: dir, encoding: 'utf8', timeout: 10_000 }); if (r.status !== 0) throw new Error(`git ${a.join(' ')}: ${r.stderr}`); return r.stdout.trim(); }; + const write = (files: Record) => { for (const [p, text] of Object.entries(files)) { const f = path.join(dir, p); if (text === null) { fs.rmSync(f); continue; } fs.mkdirSync(path.dirname(f), { recursive: true }); fs.writeFileSync(f, text); } }; + git('init', '-q', '-b', 'main'); git('config', 'user.email', 't@t'); git('config', 'user.name', 't'); git('config', 'commit.gpgsign', 'false'); + write({ 'README.md': '# x\n', ...baseFiles }); git('add', '-A'); git('commit', '-qm', 'base'); + const baseSha = git('rev-parse', 'HEAD'); + git('checkout', '-qb', 'feat'); + const commit = (files: Record) => { write(files); git('add', '-A'); git('commit', '-qm', 'feat'); return git('rev-parse', 'HEAD'); }; + return { dir, git, write, commit, baseSha }; +} + +describe('collectCommit: git objects only', () => { + test('a registered glob output is excluded, a command:null entry is not, and a .tmpl edit counts once', () => { + const r = repo({ '.gstack/generated.json': REGISTRY, 'ship/SKILL.md.tmpl': 'a\n', 'ship/SKILL.md': 'a\n' }); + const head = r.commit({ 'ship/SKILL.md.tmpl': 'a\nb\nc\n', 'ship/SKILL.md': lines(120), 'src/hand.ts': lines(5) }); + const c = collectCommit(r.dir, r.baseSha, head); + expect(c.errors).toEqual([]); + expect(c.registry).toEqual({ sha: r.baseSha, outputs: ['*/SKILL.md'] }); + const d = depthOfCommitInput({ ...c, gateTagged: 0 }); + expect([d.depth, d.runtimeLines]).toEqual(['light', 2 + 5]); + }); + test('a head-only registry entry never excludes: the registry is the base commit\'s', () => { + const r = repo({ 'src/keep.ts': lines(1) }); + const head = r.commit({ '.gstack/generated.json': JSON.stringify({ entries: [{ id: 'gen', command: 'gen', outputs: ['src/gen.ts'] }] }), 'src/gen.ts': lines(60) }); + const c = collectCommit(r.dir, r.baseSha, head); + expect(c.registry).toEqual({ sha: r.baseSha, outputs: [] }); + const d = depthOfCommitInput({ ...c, gateTagged: 0 }); + expect(d.runtimeLines).toBeGreaterThanOrEqual(60); + expect(d.depth).toBe('full'); + }); + test('a rename into src/ counts the new path as runtime', () => { + const r = repo({ 'notes/a.txt': lines(30) }); + fs.mkdirSync(path.join(r.dir, 'src')); + r.git('mv', 'notes/a.txt', 'src/a.ts'); + r.git('commit', '-qm', 'mv'); + const c = collectCommit(r.dir, r.baseSha, r.git('rev-parse', 'HEAD')); + expect(c.deleted).toEqual(['notes/a.txt']); + expect(c.perPath.find(e => e.path === 'src/a.ts')).toEqual({ path: 'src/a.ts', lines: 30, binary: false }); + expect(depthOfCommitInput({ ...c, gateTagged: 0 })).toMatchObject({ depth: 'light', runtimeLines: 30 }); + }); + test('a binary code-class path is an error (full), never 0 lines; a deleted test is full', () => { + const r = repo({ 'test/old.test.ts': 'x\n' }); + const head = r.commit({ 'lib/x.wasm': Buffer.from([0, 97, 115, 109, 0, 1, 2]) }); + const d = depthOfCommitInput({ ...collectCommit(r.dir, r.baseSha, head), gateTagged: 0 }); + expect(d.depth).toBe('full'); + expect(d.errors).toEqual([expect.objectContaining({ code: 'binary_code_path', input: 'lib/x.wasm' })]); + const t = repo({ 'test/old.test.ts': 'x\n', 'test/keep.test.ts': 'x\n' }); + const del = t.commit({ 'test/old.test.ts': null }); + const deleted = depthOfCommitInput({ ...collectCommit(t.dir, t.baseSha, del), gateTagged: 0 }); + expect([deleted.depth, deleted.runtimeLines, deleted.errors]).toEqual(['full', 0, []]); + }); + test('an invalid base registry is an error with its result code; an unresolvable sha is unavailable', () => { + const r = repo({ '.gstack/generated.json': '{"entries": [{"id": "x"}]}' }); + const head = r.commit({ 'src/a.ts': lines(3) }); + const d = depthOfCommitInput({ ...collectCommit(r.dir, r.baseSha, head), gateTagged: 0 }); + expect(d.depth).toBe('full'); + expect(d.codes).toEqual(['REGEN_REGISTRY_INVALID']); + expect(d.errors[0]).toMatchObject({ code: 'registry_invalid', result: 'REGEN_REGISTRY_INVALID' }); + expect(d.errors[0]!.repair).toContain('gstack-regen validate'); + expect(loadRegistryAt(r.dir, 'f'.repeat(40)).error?.code).toBe('registry_unavailable'); + const missing = collectCommit(r.dir, 'f'.repeat(40), head); + expect(missing.errors.map(e => e.code)).toContain('numstat_failed'); + expect(depthOfCommitInput({ ...missing, gateTagged: 0 }).depth).toBe('full'); + }); + test('from a checkout at another HEAD with a dirty tree it equals the clean result at head', () => { + const r = repo({ '.gstack/generated.json': REGISTRY, 'src/a.ts': lines(2), 'ship/SKILL.md': 'a\n', 'autoplan/SKILL.md.tmpl': 'x\n' }); + const head = r.commit({ 'src/a.ts': lines(40), 'ship/SKILL.md': lines(9), 'docs/b.md': '# b\n12% faster\n', 'autoplan/notes.md': 'n\n', 'package.json': '{"version":"1.0.1"}\n' }); + const clean = collectCommit(r.dir, r.baseSha, head); + r.git('checkout', '-q', r.baseSha); + r.write({ 'src/a.ts': lines(500, 'dirty'), 'src/untracked.ts': lines(80), 'autoplan/SKILL.md.tmpl': null }); + const dirty = collectCommit(r.dir, r.baseSha, head); + expect(dirty).toEqual(clean); + expect(depthOfCommitInput({ ...dirty, gateTagged: 0 })).toEqual(depthOfCommitInput({ ...clean, gateTagged: 0 })); + expect(clean.root).toEqual(['autoplan', 'ship']); + expect(clean.added['docs/b.md']).toEqual(['# b', '12% faster']); + }); +}); + +describe('collectLive: the working tree', () => { + test('tracked uncommitted edits and untracked files count per path; scratch under .gstack/tmp/ does not', () => { + const r = repo({ 'src/a.ts': lines(1) }); + r.commit({ 'docs/a.md': '# a\n' }); + r.write({ 'src/a.ts': lines(11), 'src/new.ts': lines(20), '.gstack/tmp/x.ts': lines(500) }); + const c = collectLive(r.dir, 'main'); + expect(c.errors).toEqual([]); + expect(c.untracked).toEqual(['src/new.ts']); + expect(c.perPath.find(e => e.path === 'src/new.ts')).toEqual({ path: 'src/new.ts', lines: 20, binary: false }); + expect(c.paths).toEqual(['docs/a.md', 'src/a.ts', 'src/new.ts']); + expect(depthOfCommitInput({ ...c, gateTagged: 0 })).toMatchObject({ depth: 'light', runtimeLines: 20 + 10 }); + }); + test('no merge base is a collector error the CLI refuses on', () => { + const r = repo({}); + expect(collectLive(r.dir, 'no-such-branch').errors.map(e => e.code)).toEqual(['numstat_failed']); + }); +}); diff --git a/test/review-depth.test.ts b/test/review-depth.test.ts index d8c0ab4fa..db2ea9b05 100644 --- a/test/review-depth.test.ts +++ b/test/review-depth.test.ts @@ -1,6 +1,9 @@ import { describe, test, expect } from 'bun:test'; +import { spawnSync } from 'node:child_process'; +import * as fs from 'node:fs'; +import * as os from 'node:os'; import * as path from 'node:path'; -import { classifyDiff, classifyPath, nextStep, planReview, scanNumbers, FULL_PASS_CAP, type PlanInput } from '../lib/review-depth'; +import { classifyDiff, classifyPath, depthFor, depthOfCommitInput, diffCommand, nextStep, planReview, scanNumbers, DEPTH_RULES, FULL_PASS_CAP, type DepthError, type PlanInput } from '../lib/review-depth'; const ROOT = path.join(import.meta.dir, '..'); const none = scanNumbers({}); @@ -73,9 +76,10 @@ describe('planReview', () => { const p = planReview(base({ paths: ['docs/bench.md'], scope: {}, numbers: scanNumbers({ 'docs/bench.md': ['3x faster'] }) })); expect(roles(p)).toEqual(['outside-voice:outside-voice:host']); }); - test('tests class: testing specialist only', () => { + test('tests class: runtime 0 is light, core only (Behavior changes: no Testing specialist)', () => { const p = planReview(base({ paths: ['test/a.test.ts'], scope: { SCOPE_TESTS: 'true' } })); - expect(roles(p)).toEqual(['specialist:testing:full']); + expect(p.depth).toBe('light'); + expect(roles(p)).toEqual([]); }); test('code class pass 1: every reviewer at full effort, the outside voice at its own', () => { const p = planReview(base({})); @@ -88,7 +92,7 @@ describe('planReview', () => { }); test('code class honours hit-rate gating and small-diff skip', () => { expect(roles(planReview(base({ gated: ['simplification'] })))).not.toContain('specialist:simplification:full'); - expect(planReview(base({ diffLines: 20 })).dispatches.filter(d => d.role === 'specialist')).toEqual([]); + expect(planReview(base({ diffLines: 20 })).dispatches.filter(d => d.role === 'specialist').map(d => d.name)).toEqual(['batched']); }); test('pass 2 reruns the army on the range but not the adversarial or outside voice once Step 11 ran', () => { const p = planReview(base({ adversarialRan: true, history: { fullPasses: 1, last: 'full', fixedBlocking: true } })); @@ -167,3 +171,121 @@ describe('GSTA-131 coverage: Red Team and specialist arms', () => { expect(roles(planReview(base({ paths: ['docs/a.md'], force: ['security'] })))).toEqual(['specialist:security']); }); }); + +const per = (o: Record) => Object.entries(o).map(([p, lines]) => ({ path: p, lines, binary: false })); +const mixed = (o: Partial = {}) => base({ paths: Object.keys(o.perPath ? Object.fromEntries(o.perPath.map(e => [e.path, 1])) : {}), ...o }); +const err = (code: DepthError['code'], result?: DepthError['result']): DepthError => ({ code, detail: 'd', input: 'i', repair: 'r', ...(result ? { result } : {}) }); + +describe('review depth: the matrix', () => { + test('runtime 0 (tests and docs only) is light: core only, nothing dispatched', () => { + const p = planReview(mixed({ perPath: per({ 'test/a.test.ts': 300, 'docs/a.md': 40 }), diffLines: 340 })); + expect([p.depth, p.runtimeLines, p.dispatches]).toEqual(['light', 0, []]); + }); + test('runtime 1-50 is light plus one batched specialist: the fixed set, scope picks and the Red Team in one context', () => { + const p = planReview(mixed({ perPath: per({ 'src/app.ts': 48, 'test/a.test.ts': 190, 'docs/a.md': 9 }), diffLines: 247 })); + expect([p.depth, p.runtimeLines]).toEqual(['light', 48]); + expect(p.dispatches).toEqual([ + { role: 'specialist', name: 'batched', effort: 'full', exclude: [':(exclude)docs/a.md'], checklists: ['testing', 'maintainability', 'security', 'performance', 'red-team'] }, + { role: 'adversarial-native', name: 'adversarial', effort: 'full', exclude: [], reason: 'backtest inconclusive (GSTA-147 D5)' }, + ]); + expect(p.skipped.join('; ')).toContain('red-team (light depth'); + expect(planReview(mixed({ perPath: per({ 'src/app.ts': 48 }), adversarialRan: true, history: { fullPasses: 1, last: 'full', touchedGate: true, fixedBelow: true } })).dispatches.some(d => d.role === 'adversarial-native')).toBe(false); + }); + test('runtime 51 is standard: thresholds read runtime lines, reviewers read their own hunks', () => { + const p = planReview(mixed({ perPath: per({ 'src/app.ts': 51, 'test/a.test.ts': 400, 'README.md': 3 }), diffLines: 454 })); + expect([p.depth, p.runtimeLines]).toEqual(['standard', 51]); + expect(roles(p)).toEqual(['specialist:testing:full', 'specialist:maintainability:full', 'specialist:performance:full', 'adversarial-native:adversarial:full', 'outside-voice:outside-voice:host']); + const ex = Object.fromEntries(p.dispatches.map(d => [d.name, d.exclude])); + expect(ex.testing).toEqual([':(exclude)README.md']); + expect(ex.maintainability).toEqual([':(exclude)README.md', ':(exclude)test/a.test.ts']); + expect([ex.adversarial, ex['outside-voice']]).toEqual([[], []]); + }); + test.each([ + ['scope error', { scope: { SCOPE_ERROR: 'boom' } }, 'scope_error', []], + ['gate scan unavailable', { gateTagged: null }, 'gate_scan_unavailable', []], + ['base registry invalid', { registry: { sha: 'abc', outputs: [], error: err('registry_invalid', 'REGEN_REGISTRY_INVALID') } }, 'registry_invalid', ['REGEN_REGISTRY_INVALID']], + ['base registry unavailable', { registry: { sha: null, outputs: [], error: err('registry_unavailable') } }, 'registry_unavailable', []], + ['numstat failure', { errors: [err('numstat_failed')] }, 'numstat_failed', []], + ['binary code path', { perPath: [...per({ 'src/app.ts': 5 }), { path: 'lib/x.wasm', lines: 0, binary: true }], paths: ['src/app.ts', 'lib/x.wasm'] }, 'binary_code_path', []], + ] as const)('%s fails closed to full with its code', (_n, o, code, codes) => { + const p = planReview(mixed({ perPath: per({ 'src/app.ts': 5 }), ...(o as Partial) })); + expect(p.depth).toBe('full'); + expect(p.errors.map(e => e.code)).toContain(code); + expect(p.codes).toEqual([...codes]); + expect(roles(p)).toContain('red-team:red-team:full'); + }); + test.each([ + ['a gate path', { paths: ['src/app.ts', '.github/workflows/ci.yml'] }], + ['a gate tag', { gateTagged: 1 }], + ['a deleted test', { deleted: ['test/old.test.ts'] }], + ['an edit to the registry', { paths: ['.gstack/generated.json'] }], + ['an edit to SMALL_DIFF_LINES', { paths: ['lib/review-depth.ts'] }], + ['the collector', { paths: ['lib/review-collect.ts'] }], + ['the plan CLI', { paths: ['bin/gstack-review-plan'] }], + ['the queue', { paths: ['lib/ship-queue-train.ts'] }], + ['the Step 9 template', { paths: ['ship/sections/review-army.md.tmpl'] }], + ] as const)('%s is full', (_n, o) => { + const paths = (o as any).paths ?? ['src/app.ts']; + expect(planReview(mixed({ perPath: per(Object.fromEntries(paths.map((p: string) => [p, 2]))), ...(o as Partial) })).depth).toBe('full'); + }); + test('a registered glob output is excluded from runtime; a .tmpl edit plus its generated SKILL.md counts the tmpl only', () => { + const p = planReview(mixed({ perPath: per({ 'ship/SKILL.md.tmpl': 30, 'ship/SKILL.md': 200 }), registry: { sha: 'abc', outputs: ['*/SKILL.md'] } })); + expect([p.depth, p.runtimeLines]).toEqual(['light', 30]); + expect(p.dispatches[0]!.exclude).toEqual([':(exclude)ship/SKILL.md']); + }); + test('a path no base-registry entry names stays runtime', () => { + expect(planReview(mixed({ perPath: per({ 'src/gen.ts': 60 }), registry: { sha: 'abc', outputs: ['lib/dom.js'] } })).runtimeLines).toBe(60); + }); + test('numbers in light dispatch the outside voice with an empty exclude', () => { + const p = planReview(mixed({ perPath: per({ 'docs/bench.md': 4 }), numbers: scanNumbers({ 'docs/bench.md': ['3x faster'] }) })); + expect(p.depth).toBe('light'); + expect(p.dispatches).toEqual([{ role: 'outside-voice', name: 'outside-voice', effort: 'host', exclude: [] }]); + }); + test('a fixed blocker in light raises the next full step to the standard set, Red Team included', () => { + const p = planReview(mixed({ perPath: per({ 'src/app.ts': 30 }), history: { fullPasses: 1, last: 'full', fixedBlocking: true } })); + expect([p.depth, p.stepDepth]).toEqual(['light', 'standard']); + expect(roles(p)).toEqual(['specialist:testing:full', 'specialist:maintainability:full', 'specialist:performance:full', 'red-team:red-team:full', 'adversarial-native:adversarial:full', 'outside-voice:outside-voice:host']); + }); + test('--depth-floor raises and never lowers', () => { + expect(depthFor({ cls: 'code', runtimeLines: 10, errors: [], floor: 'full' })).toBe('full'); + expect(depthFor({ cls: 'code', runtimeLines: 10, errors: [], floor: 'standard' })).toBe('standard'); + expect(depthFor({ cls: 'gate', runtimeLines: 10, errors: [], floor: 'standard' })).toBe('full'); + expect(depthFor({ cls: 'code', runtimeLines: 80, errors: [] })).toBe('standard'); + const p = planReview(mixed({ perPath: per({ 'src/app.ts': 10 }), floor: 'full' })); + expect(p.depth).toBe('full'); + expect(p.depthReasons).toContain('floor:full'); + }); + test('depthOfCommitInput gives the same depth as the plan for the same input', () => { + const i = { paths: ['src/app.ts', 'test/a.test.ts'], perPath: per({ 'src/app.ts': 51, 'test/a.test.ts': 9 }), gateTagged: 0 }; + expect(depthOfCommitInput(i)).toMatchObject({ depth: 'standard', runtimeLines: 51, rules: DEPTH_RULES, errors: [] }); + expect(depthOfCommitInput({ ...i, gateTagged: null }).depth).toBe('full'); + }); +}); + +describe('review depth: diff commands and the rules hash', () => { + test('DIFF_CMD shell-quotes each pathspec; a glob character makes the pathspec literal', () => { + const p = planReview(mixed({ perPath: per({ 'src/app.ts': 10, "docs/my notes*.md": 3, "docs/it's.md": 1 }) })); + const ex = p.dispatches[0]!.exclude; + expect(ex).toEqual([':(exclude,literal)docs/my notes*.md', ":(exclude)docs/it's.md"]); + expect(diffCommand(p.dispatches[0]!)).toBe("git diff -- . ':(exclude,literal)docs/my notes*.md' ':(exclude)docs/it'\\''s.md'"); + expect(diffCommand({ role: 'red-team', name: 'red-team', effort: 'full', exclude: [] })).toBe('git diff '); + }); + test('DEPTH_RULES hashes the deciding constants: a comment-only edit keeps it, a threshold edit changes it', () => { + expect(DEPTH_RULES).toMatch(/^[0-9a-f]{16}$/); + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'depth-rules-')); + try { + const src = fs.readFileSync(path.join(ROOT, 'lib/review-depth.ts'), 'utf8'); + fs.symlinkSync(path.join(ROOT, 'lib/ship-policy.ts'), path.join(dir, 'ship-policy.ts')); + const hashOf = (text: string) => { + fs.writeFileSync(path.join(dir, 'review-depth.ts'), text); + fs.writeFileSync(path.join(dir, 'print.ts'), "import { DEPTH_RULES } from './review-depth';\nconsole.log(DEPTH_RULES);\n"); + const r = spawnSync(process.execPath, ['--no-env-file', '--config=/dev/null', path.join(dir, 'print.ts')], { encoding: 'utf8', timeout: 30_000 }); + expect(r.status, r.stderr).toBe(0); + return r.stdout.trim(); + }; + expect(hashOf('// a comment-only edit\n' + src)).toBe(DEPTH_RULES); + expect(hashOf(src.replace('SMALL_DIFF_LINES = 50', 'SMALL_DIFF_LINES = 60'))).not.toBe(DEPTH_RULES); + } finally { fs.rmSync(dir, { recursive: true, force: true }); } + }); +}); + diff --git a/test/review-dispatch-compare.test.ts b/test/review-dispatch-compare.test.ts index 804d9e0cf..2771d0a95 100644 --- a/test/review-dispatch-compare.test.ts +++ b/test/review-dispatch-compare.test.ts @@ -34,7 +34,7 @@ describe('scripts/review-dispatch-compare.ts', () => { const table = run(['--tokens', tok]); expect(table).toContain(`TOKENS: measured (${tok})`); expect(table).toMatch(/^SHIP_TOKEN_RATIO: median=0\.\d+ min=0\.\d+ max=\d\.\d+ \(n=10\)$/m); - expect(table).toMatch(/^SHIP_TOKEN_RATIO_BY_CLASS: docs median=0\.\d+ \(n=5\); code median=0\.\d+ \(n=5\)$/m); + expect(table).toMatch(/^SHIP_TOKEN_RATIO_BY_CLASS: docs median=0\.\d+ \(n=5\); gate median=0\.\d+ \(n=5\)$/m); } finally { fs.rmSync(dir, { recursive: true, force: true }); } diff --git a/test/review-plan-cli.test.ts b/test/review-plan-cli.test.ts index c8aedb90e..ba8d77d9b 100644 --- a/test/review-plan-cli.test.ts +++ b/test/review-plan-cli.test.ts @@ -109,3 +109,51 @@ describe('gstack-review-plan', () => { expect(ran).toContain('SKIP: adversarial-native and outside-voice (run once per invocation)'); }); }); + +describe('gstack-review-plan: review depth', () => { + const code = (n: number) => Array.from({ length: n }, (_, i) => `export const v${i} = ${i};`).join('\n') + '\n'; + test('prints REVIEW_DEPTH with runtime lines and the rules hash, and GATE_SCAN bound to the head', () => { + const r = repo({ 'src/a.ts': code(10), 'test/a.test.ts': code(200) }); + const out = run(r, []).stdout; + const head = spawnSync('git', ['rev-parse', 'HEAD'], { cwd: r.dir, encoding: 'utf8', timeout: 10_000 }).stdout.trim(); + expect(out).toMatch(/^REVIEW_DEPTH: light runtime_lines=10 rules=[0-9a-f]{16} reasons=runtime=10<=50 codes=none$/m); + expect(out).toContain(`GATE_SCAN: tagged=0 head=${head}`); + expect(out).toContain('DISPATCH: specialist batched effort=full checklists=testing,maintainability,security,performance,state-machines,red-team'); + expect(out).toContain('DISPATCH: adversarial-native adversarial effort=full reason=backtest inconclusive (GSTA-147 D5)'); + }); + test('DIFF_CMD is shell-quoted per dispatch and excludes what the reviewer does not read; DIFF_EXCLUDE: none when empty', () => { + const r = repo({ 'src/a.ts': code(80), 'docs/my notes*.md': '# n\n', 'test/a.test.ts': code(3) }); + const out = run(r, []).stdout; + expect(out).toContain("DIFF_CMD: testing git diff -- . ':(exclude,literal)docs/my notes*.md'"); + expect(out).toContain("DIFF_CMD: maintainability git diff -- . ':(exclude,literal)docs/my notes*.md' ':(exclude)test/a.test.ts'"); + expect(out).toContain('DIFF_CMD: adversarial git diff \nDIFF_EXCLUDE: adversarial none'); + const cmd = /^DIFF_CMD: maintainability (.*)$/m.exec(out)![1]!.replace('', 'main'); + const diff = spawnSync('bash', ['-c', cmd.replace('git diff main', 'git diff --name-only main')], { cwd: r.dir, encoding: 'utf8' }); + expect(diff.stdout.trim().split('\n')).toEqual(['src/a.ts']); + }); + test('--json carries depth, runtime_lines, rules, errors, gate_scan and every dispatch\'s exclude array', () => { + const r = repo({ 'src/a.ts': code(80), 'README.md': '# y\n' }); + const plan = JSON.parse(run(r, ['--json']).stdout); + expect(plan).toMatchObject({ depth: 'standard', step_depth: 'standard', runtime_lines: 80, errors: [], codes: [], gate_scan: { tagged: 0 } }); + expect(plan.rules).toMatch(/^[0-9a-f]{16}$/); + expect(plan.dispatches.find((d: any) => d.name === 'maintainability').exclude).toEqual([':(exclude)README.md']); + expect(plan.dispatches.find((d: any) => d.role === 'outside-voice').exclude).toEqual([]); + }); + test('--depth-floor full raises a light diff, never lowers a full one, and an invalid floor is a usage error', () => { + const r = repo({ 'src/a.ts': code(5) }); + expect(run(r, ['--depth-floor', 'full']).stdout).toMatch(/^REVIEW_DEPTH: full runtime_lines=5 .*reasons=runtime=5<=50,floor:full /m); + const gate = repo({ 'src/a.ts': code(5), '.github/workflows/ci.yml': 'on: push\n' }); + expect(run(gate, ['--depth-floor', 'standard']).stdout).toMatch(/^REVIEW_DEPTH: full /m); + expect(run(r, ['--depth-floor', 'light']).status).toBe(2); + }); + test('a conservative full plan exits 0 with its errors; a plan that cannot be built exits 1', () => { + const r = repo({ 'lib/x.wasm': '\u0000asm\u0000' }); + const res = run(r, ['--json']); + expect(res.status).toBe(0); + expect(JSON.parse(res.stdout)).toMatchObject({ depth: 'full', errors: expect.arrayContaining([expect.objectContaining({ code: 'binary_code_path', input: 'lib/x.wasm' })]) }); + expect(run(r, []).stdout).toContain('DEPTH_ERROR: binary_code_path input=lib/x.wasm'); + const none = spawnSync(BIN, ['--base', 'no-such-branch', '--gate-tagged', '0'], { cwd: r.dir, encoding: 'utf8', timeout: 60_000, env: { ...process.env, GSTACK_STATE_ROOT: r.state } }); + expect(none.status).toBe(1); + }); +}); + diff --git a/test/size-ledger/ship/gsta147-review-depth.json b/test/size-ledger/ship/gsta147-review-depth.json new file mode 100644 index 000000000..79b65d6cc --- /dev/null +++ b/test/size-ledger/ship/gsta147-review-depth.json @@ -0,0 +1,4 @@ +{ + "bytes": 745, + "why": "GSTA-147 review depth: Step 9 saves REVIEW_DEPTH/GATE_SCAN, passes --depth-floor, records depth fields and reads each reviewer's DIFF_CMD; the batched light specialist; Step 11's light note; the skeleton's Review policy paragraph names the depth and --depth-floor (trimmed to hold the skeleton cap)" +}