diff --git a/bin/gstack-review-dispatch b/bin/gstack-review-dispatch index a9f9e91ee..a72e5cfd4 100755 --- a/bin/gstack-review-dispatch +++ b/bin/gstack-review-dispatch @@ -403,7 +403,7 @@ function pass(): number { const step = p.next.step; const fixChecks = step === 'fix-check' ? [] : p.dispatches.filter(d => d.role === 'fix-check'); const planLines = [...text.stdout.trimEnd().split('\n'), ...fixChecks.map(d => `FIX_CHECK: planned effort=${d.effort} (Step 3.5 runs it on its own; not in this manifest)`)]; - if (step !== 'full' && step !== 'scoped' && step !== 'fix-check') { process.stdout.write([...planLines, `PASS: none next=${step} (no reviewer pass; follow NEXT)`].join('\n') + '\n'); return EXIT.ok; } + if (step !== 'full' && step !== 'scoped' && step !== 'fix-check' && step !== 'targeted') { process.stdout.write([...planLines, `PASS: none next=${step} (no reviewer pass; follow NEXT)`].join('\n') + '\n'); return EXIT.ok; } const rangeBase = step === 'scoped' ? git(['rev-parse', '--verify', '--quiet', `${p.fix_range?.base ?? ''}^{commit}`])?.trim() : undefined; if (step === 'scoped' && !rangeBase) throw new BriefError('a scoped plan names no readable fix range base'); const mergeBase = mergeBaseWith(base); diff --git a/bin/gstack-review-plan b/bin/gstack-review-plan index 71522415a..67f468aba 100755 --- a/bin/gstack-review-plan +++ b/bin/gstack-review-plan @@ -38,8 +38,9 @@ import { loadPolicyAt } from '../lib/ship-policy-at'; import { loadReaders, readersLine } from '../lib/review-fetch'; import { policySelection, selectFor } from '../lib/review-selection'; import { effectiveReaders, READERS_POLICY_DEFAULT } from '../lib/ship-policy-readers'; -import { classifyDiff, diffCommand, keepCommand, planReview, reviewDepthLine, reviewPlanLine, scanNumbers, SCOPED_RANGE_LINES, type Depth, type History } from '../lib/review-depth'; +import { classifyDiff, diffCommand, keepCommand, planReview, reviewDepthLine, reviewPlanLine, scanNumbers, ROLE_EFFORT, SCOPED_RANGE_LINES, type Depth, type History } from '../lib/review-depth'; import { classList, targetedBriefSha, targetedClasses, targetedPatchId } from '../lib/review-targeted'; +import { targetedDelta } from '../lib/review-targeted-delta'; import { RISK_CLASSES, type RiskClass } from '../lib/review-risk'; import { renderFixCheckPrompt } from '../scripts/resolvers/fix-check'; @@ -59,7 +60,8 @@ const HELP = `gstack-review-plan — the review depth of the next /ship step (do --gated a,b specialists the hit-rate stats tag GATE_CANDIDATE --force a,b|all specialists forced by prompt flags (--all-specialists is all): at standard and full depth they join the batched specialist's checklists, where maintainability, performance and simplification are read only when - forced; light runtime-0 and scoped passes dispatch them one reader each + forced; light runtime-0 and scoped passes dispatch them one reader each; targeted is no specialist and + refuses (exit 2): the plan dispatches it itself --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 --range-base the fix range's base (this pass's PASS_HEAD): after a fixed blocker on a diff below full depth, a @@ -68,7 +70,10 @@ const HELP = `gstack-review-plan — the review depth of the next /ship step (do --json the plan as JSON (depth, runtime_lines, rules, errors with input and repair, gate_scan, per-dispatch exclude) --pass1-depth pass 1's REVIEW_DEPTH depth: full, or absent, keeps pass 2 full --targeted-ran the TARGETED line's targeted_patch_id (the whole committed diff, tests included) when the targeted - reader last read it: the same patch skips it; any other id, none, or uncommitted edits dispatch it again + reader last read it: the same patch skips it; any other id, none, or uncommitted edits dispatch it again. + With --range-base naming the commit that read happened on, a loop that is done plans NEXT: targeted + (one targeted reader on HEAD, TARGETED_DELTA: names the range) when the only change since is docs the + receipt cannot carry the record across (a money, security or numeric claim; GST-1598) --grant the owner's grant of one scoped fix-check after round 2, recorded with gstack-approvals add (approved, its ask or scope naming review_loop_grant and the stop head's sha, at or after --range-base and at or before HEAD; needs --range-base): the stop after a round-2 full pass with a fixed blocking finding or a @@ -94,7 +99,7 @@ Depth (R51, risk not size): full on gate class, a risk class (money, auth, secre standard only by --depth-floor or a raise; otherwise light. TARGETED: the money, gate, auth and secret classes the diff touches; any class dispatches the targeted reader at every depth. NEXT: full, scoped (a full pass on the range: fix-check, Red Team, native adversarial), fix-check, voices (gate class: Step 11 before any stop, -then ask again with --adversarial-ran), stop or done. A full plan +then ask again with --adversarial-ran), targeted (the targeted-only read after the loop), stop or done. A full plan from an error exits 0; exit 1 means no plan could be built (git could not list the diff). ${exitTableHelp()}`; @@ -134,6 +139,7 @@ function main(): number { const rangeBase = flag('--range-base'); if (has('--range-base') && (!rangeBase || rangeBase.startsWith('-'))) { process.stderr.write('gstack-review-plan: --range-base needs the fix range\'s base commit (this pass\'s PASS_HEAD)\n'); return EXIT.usage; } if (grant && !rangeBase) { process.stderr.write('gstack-review-plan: --grant needs --range-base (round 2\'s PASS_HEAD), which binds the grant to the stop it answers\n'); return EXIT.usage; } + if (list('--force').includes('targeted')) { process.stderr.write('gstack-review-plan: --force targeted: the targeted reader is no specialist; the plan dispatches it whenever the diff has a targeted class, and after a finished loop for a docs claim added since its read: pass --range-base --targeted-ran (docs/review-policy.md#targeted-only-read-after-the-loop)\n'); return EXIT.usage; } const declared = list('--declare-risk'); if (has('--declare-risk') && (!declared.length || declared.some(c => !(RISK_CLASSES as readonly string[]).includes(c)))) { process.stderr.write(`gstack-review-plan: --declare-risk ${flag('--declare-risk') ?? ''}: each class is one of ${RISK_CLASSES.join(', ')} (a declaration only raises the depth)\n`); return EXIT.usage; } const root = git(['rev-parse', '--show-toplevel'])?.trim(); @@ -219,19 +225,27 @@ function main(): number { const targeted = targetedClasses({ ...collected, ...(active ? { paths: paths.filter(p => !active.excluded.has(p)), added: kept(added), removed: kept(collected.removed) } : {}), scope, gateTagged }); const patch = collected.mergeBase ? targetedPatchId(process.cwd(), collected.mergeBase) : 'none'; const brief = targetedBriefSha(); + const ran = !['uncommitted', 'none'].includes(patch) && flag('--targeted-ran') === patch; const postReview = has('--post-review'); const plan = planReview({ - ...collected, ...exclusionInput, ...riskPaths, declared: declared as RiskClass[], shape: { panel: review.panel, readerModel: review.reader_model, platform: review.platform, head, platformReview: platform?.ok ? platform.review : null }, scope, gateTagged, targeted: { classes: targeted.classes, ran: !['uncommitted', 'none'].includes(patch) && flag('--targeted-ran') === patch }, numbers: scanNumbers(kept(added)), force, gated: list('--gated'), floor, tiers: auditEffort !== 'host', + ...collected, ...exclusionInput, ...riskPaths, declared: declared as RiskClass[], shape: { panel: review.panel, readerModel: review.reader_model, platform: review.platform, head, platformReview: platform?.ok ? platform.review : null }, scope, gateTagged, targeted: { classes: targeted.classes, ran }, numbers: scanNumbers(kept(added)), force, gated: list('--gated'), floor, tiers: auditEffort !== 'host', criticalFound: has('--critical-found'), adversarialRan: has('--adversarial-ran'), reviewedNumbers: flag('--reviewed-numbers'), history: { fullPasses: pass, last, fixedBlocking: has('--fixed-blocking'), fixedBelow: (has('--fixed-below') || (postReview && pass > 0)) && last !== 'fix-check', blockingOpen: has('--blocking-open'), touchedGate: has('--touched-gate') || !!fixRange?.touched_gate || (postReview && pass > 0), pass1Depth, rangeRuntimeLines: fixRange?.runtime_lines, granted: grant, fixChecks: has('--fix-checks') ? fixChecks : undefined }, }); if (postReview && pass > 0) plan.next.reason = `edits after the review bound (Step 16 stage 2.1) plan as a fix to a gate-class path, never a note: ${plan.next.reason}`; + const ranArg = flag('--targeted-ran'); + const delta = plan.next.step === 'done' && rangeBase && ranArg && targeted.classes.length && !ran ? targetedDelta(root, ref, rangeBase, ranArg) : null; + if (delta?.state === 'dispatch') { + plan.next = { step: 'targeted', reason: `the loop is done and the only change since the targeted read at ${delta.range_base.slice(0, 12)} is docs whose claim the record cannot carry (${delta.paths.join(', ')}): one targeted reader reads HEAD for ${classList(targeted.classes)}; it is not a review round` }; + plan.dispatches.push({ role: 'targeted', name: 'targeted', effort: ROLE_EFFORT.targeted, exclude: active ? [...active.specs] : [], classes: [...targeted.classes], ...(active?.keep.length ? { keep: [...active.keep] } : {}) }); + } else if (delta) plan.skipped.push(`targeted-only read (${delta.reason})`); + const targetedDeltaOut = delta?.state === 'dispatch' ? { range_base: delta.range_base, paths: delta.paths, classes: targeted.classes } : null; const selection = policySelection(active && { state: 'active', specs: active.specs, keep: active.keep, excerpt: active.excerpt }, readers); plan.dispatches.forEach((d, i) => { if (d.role !== 'fix-check') plan.dispatches[i] = selectFor(d, selection); }); const gateScan = { tagged: gateTagged, head }; if (has('--json')) { const { runtimeLines, stepDepth, depthReasons, ...rest } = plan; - process.stdout.write(JSON.stringify({ ...rest, review_policy: review, platform_review: platform ? (platform.ok ? platform.review : { none: platform.reason }) : null, readers, targeted: { ...targeted, brief, targeted_patch_id: patch }, step_depth: stepDepth, runtime_lines: runtimeLines, depth_reasons: depthReasons, depth_floor: floor ?? null, gate_scan: gateScan, fix_range: fixRange ?? null, registry: { sha: registry.sha, outputs: registry.outputs }, per_path: perPath, diff_lines: diffLines, base: collected.base, pass, audit_effort: auditEffort, scope, gate_tagged: gateTagged, paths, deleted, stamp_only: stampOnly, consumed: collected.consumed, drawings: collected.drawings, executables: collected.executables, + process.stdout.write(JSON.stringify({ ...rest, review_policy: review, platform_review: platform ? (platform.ok ? platform.review : { none: platform.reason }) : null, readers, targeted: { ...targeted, brief, targeted_patch_id: patch }, step_depth: stepDepth, runtime_lines: runtimeLines, depth_reasons: depthReasons, depth_floor: floor ?? null, gate_scan: gateScan, fix_range: fixRange ?? null, targeted_delta: targetedDeltaOut, registry: { sha: registry.sha, outputs: registry.outputs }, per_path: perPath, diff_lines: diffLines, base: collected.base, pass, audit_effort: auditEffort, scope, gate_tagged: gateTagged, paths, deleted, stamp_only: stampOnly, consumed: collected.consumed, drawings: collected.drawings, executables: collected.executables, ...(exclusion.state === 'head-only' ? { review_exclude: exclusion } : active ? { review_exclude: { state: 'active', ...active.record, specs: active.specs, keep: active.keep, excerpt: active.excerpt, kept_files: active.kept, content: active.content, references: active.references } } : {}) }) + '\n'); return EXIT.ok; } @@ -242,6 +256,7 @@ function main(): number { if (declared.length) out.push(`RISK_DECLARED: classes=${plan.declared.join(',')} detected=${plan.detected.join(',') || 'none'}`); if (platform) out.push(platform.ok ? `PLATFORM_REVIEW: ${platform.review.id} reviewer=${platform.review.reviewer} head=${platform.review.head}` : `PLATFORM_REVIEW: none (${platform.reason})`); if (fixRange) out.push(`FIX_RANGE: ${fixRange.runtime_lines === null ? `unread (${fixRange.error})` : `runtime_lines=${fixRange.runtime_lines}`} base=${fixRange.base}`); + if (targetedDeltaOut) out.push(`TARGETED_DELTA: range_base=${targetedDeltaOut.range_base} paths=${targetedDeltaOut.paths.join(',')} classes=${classList(targetedDeltaOut.classes)}`); out.push(reviewPlanLine(plan, pass + (plan.next.step === 'full' || plan.next.step === 'scoped' ? 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) { diff --git a/changelog.d/gst-1598-targeted-delta.md b/changelog.d/gst-1598-targeted-delta.md new file mode 100644 index 000000000..58a329b49 --- /dev/null +++ b/changelog.d/gst-1598-targeted-delta.md @@ -0,0 +1 @@ +- **A docs claim added after the review no longer leaves /ship stuck.** When the review loop had finished and a later commit added only docs with a money, security or numeric claim (such as a changelog line with dollar figures), `gstack-review-plan` printed `NEXT: done`, so nothing ran the targeted reader the receipt then asked for. Given `--range-base ` and `--targeted-ran `, the plan now prints `NEXT: targeted`, a `TARGETED_DELTA:` line and one targeted reader on HEAD. It is not a review round. A code change, plain docs, a range base the reader did not read, or uncommitted edits keep `done` and print the reason. A main merge that only edits lines next to a reviewed hunk does not count as a branch change. `--force targeted` now refuses and names this route. (GST-1598) diff --git a/docs/review-policy.md b/docs/review-policy.md index d82049b9e..074a3b5d3 100644 --- a/docs/review-policy.md +++ b/docs/review-policy.md @@ -512,6 +512,41 @@ Over the last 100 non-release commits on main (8663c3d2..8be7f213), money hit which hit 43 of 100; those paths already get full depth, so the gate class was narrowed to the gate scan's tagged hunks. +### Targeted-only read after the loop (GST-1598) + +A docs claim added after the review loop finished, such as a changelog +fragment with dollar figures, used to have no way through. The targeted record +names the commit its reader read, and `write` carries it across a later docs +change only when that change is plain docs, so the receipt asked for the +targeted reader on HEAD. But the plan printed `done`, so nothing could +dispatch it (harness-evals #225). Now, when the loop is done, pass +`--range-base ` and `--targeted-ran `. If every path whose branch patch changed since +then is docs class and not binary, and the change is not plain docs, the plan +prints `NEXT: targeted`, one `DISPATCH: targeted` for the head's classes and +`TARGETED_DELTA: range_base= paths= classes=` +(`targeted_delta` in `--json`). The patch-id check proves that range_base +is what the reader read. The branch patch is compared on each side's own +merge base, so a base merge alone adds no path. + +The reader reads the whole merge-base diff at HEAD, like every targeted +reader. It isn't scoped to the docs change, because `render` reads no other +base and a record that names HEAD is the one `write` binds. It is read-only +and applies no fix, so it isn't a review round under R51 and needs no grant. +A `--grant` is still checked against the approval record first, and a row +that isn't there refuses (exit 3). `gstack-review-dispatch pass` builds the +manifest: the one reader in the voices stage, with the `TARGETED_DELTA` line +among its plan lines. After `verify --targeted-out`, pass the new +`targeted_patch_id` and the plan is `done`. + +Every other case stays `done` and prints `SKIP: targeted-only read +()`. That covers a code or binary change, which takes the review loop +as before; plain docs, which the record carries; a range_base the +`--targeted-ran` patch doesn't match; and uncommitted edits. `--force +targeted` refuses (exit 2) and names these flags, because the targeted reader +is no specialist (`lib/review-targeted-delta.ts`, +`test/review-plan-targeted-delta.test.ts`). + ## What readers read under `review_exclude_paths` @@ -690,7 +725,8 @@ and finish prints `ROUTE_BOUNDED_REFUSED: (fix: …)` ([troubleshooting](troubleshooting.md#route-bounded-refused)). A lane change whose other top-level keys differ from the reviewed copy stops too. A targeted record read before the edits does not carry to the new head: -receipt write still asks for the targeted reader on HEAD. A test file whose only removed lines are swapped +receipt write still asks for the targeted reader on HEAD (a docs-only change +plans it on its own: [targeted-only read](#targeted-only-read-after-the-loop)). A test file whose only removed lines are swapped one for one with lines that differ inside plain quoted strings, in statements with no assertion or test call, takes the plain test route (the scoped reader) instead of behavior. A changed expected value in the @@ -938,6 +974,7 @@ ones, and the arm lost blockers. `bin/gstack-review-dispatch` closes both ends: - `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-exclude.ts`: the base policy's `review_exclude_paths` (matched set, kept reasons, the kept-file argument bound, the digest the receipt, carry and the queue compare) and the 2,000,000-byte reader ceiling (`test/review-exclude.test.ts`). +- `lib/review-targeted-delta.ts`: the [targeted-only read after the loop](#targeted-only-read-after-the-loop) (`test/review-plan-targeted-delta.test.ts`). - `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`, `unrecorded`, `ledgers_read`); refusals are `REVIEW_SCOPE_INPUT`, `REVIEW_SCOPE_RANGE` and `REVIEW_SCOPE_LEDGER` ([troubleshooting.md](troubleshooting.md#review-scope-input)). diff --git a/lib/review-depth.ts b/lib/review-depth.ts index 8ed041ff9..ce966e96c 100644 --- a/lib/review-depth.ts +++ b/lib/review-depth.ts @@ -20,7 +20,7 @@ export type PathClass = 'docs' | 'tests' | 'code'; export type Depth = 'light' | 'standard' | 'full'; export type Effort = 'light' | 'medium' | 'full' | 'host'; export type Role = 'specialist' | 'red-team' | 'adversarial-native' | 'outside-voice' | 'fix-check' | 'targeted'; -export type Step = 'full' | 'scoped' | 'fix-check' | 'voices' | 'platform' | 'done' | 'stop'; +export type Step = 'full' | 'scoped' | 'fix-check' | 'voices' | 'platform' | 'targeted' | 'done' | 'stop'; export const SPECIALISTS = ['testing', 'maintainability', 'security', 'performance', 'data-migration', 'api-contract', 'design', 'simplification', 'state-machines'] as const; export type Specialist = (typeof SPECIALISTS)[number]; diff --git a/lib/review-targeted-delta.ts b/lib/review-targeted-delta.ts new file mode 100644 index 000000000..cfb49bbe2 --- /dev/null +++ b/lib/review-targeted-delta.ts @@ -0,0 +1,47 @@ +/** + * review-targeted-delta — the targeted-only read after a finished review loop (GST-1598, + * docs/review-policy.md#targeted-only-read-after-the-loop). A targeted record names the commit its reader read, and + * gstack-ship-receipt write carries it across a later docs change only when that change is plain docs (plainDocsSince, + * GST-1195). A docs claim added after the loop finished (a changelog fragment with dollar figures, harness-evals #225) + * left no exit: the plan printed `done`, so nothing dispatched the reader the receipt asks for. bin/gstack-review-plan + * asks this module when the loop is done and the head has targeted classes the passed --targeted-ran no longer covers. + * It dispatches one targeted reader on HEAD only when range_base is the commit that read happened on (its targeted + * patch-id is --targeted-ran), every path whose branch patch changed since is docs class and not binary, and the change + * is not plain docs. Anything else keeps `done` with the reason. + */ +import { spawnSync } from 'node:child_process'; +import { classifyPath } from './review-depth'; +import { comparePath, touchedPaths } from './ship-patch'; +import { plainDocsSince } from './ship-receipt-depth'; + +export type TargetedDelta = { state: 'dispatch'; range_base: string; paths: string[] } | { state: 'skip'; reason: string }; + +/** Whether the change since `rangeBase` (where the targeted reader read the patch `ran`) earns the targeted-only read. */ +export function targetedDelta(root: string, base: string, rangeBase: string, ran: string): TargetedDelta { + const git = (args: string[], input?: string) => { + const r = spawnSync('git', ['-c', 'core.quotePath=false', ...args], { cwd: root, input, encoding: 'utf8', timeout: 60_000, maxBuffer: 256 * 1024 * 1024 }); + return r.status === 0 ? r.stdout : null; + }; + const skip = (reason: string): TargetedDelta => ({ state: 'skip', reason }); + const read = git(['rev-parse', '--verify', '--quiet', `${rangeBase}^{commit}`])?.trim(); + const head = git(['rev-parse', '--verify', '--quiet', 'HEAD^{commit}'])?.trim(); + if (!read || !head || git(['merge-base', '--is-ancestor', read, head]) === null) return skip(`range_base ${rangeBase} is not a commit on this branch`); + if (git(['diff', '--quiet', 'HEAD', '--']) === null || git(['ls-files', '--others', '--exclude-standard'])?.trim()) return skip('uncommitted or untracked edits: commit them first'); + const readBase = git(['merge-base', base, read])?.trim(); + const headBase = git(['merge-base', base, head])?.trim(); + if (!readBase || !headBase) return skip(`no merge base with ${base}`); + const diff = git(['diff', '--no-color', '--no-ext-diff', readBase, read]); + const readPatch = diff?.trim() ? git(['patch-id', '--verbatim'], diff)?.trim().split(/\s+/)[0] : undefined; + if (!readPatch || readPatch !== ran) return skip(`--targeted-ran ${ran} is not the targeted patch at range_base ${read.slice(0, 12)} (${readPatch ?? 'unreadable'}), so it does not say what the reader read`); + const before = touchedPaths(root, readBase, read); + const now = touchedPaths(root, headBase, head); + if (!before || !now) return skip('git could not list the branch patch at range_base or at HEAD'); + const sides = { reviewedBase: readBase, reviewed: read, currentBase: headBase, current: head }; + const paths = [...new Set([...before, ...now])].filter(p => !['same', 'context'].includes(comparePath(root, sides, p))).sort(); + if (!paths.length) return skip(`the branch patch is unchanged since range_base ${read.slice(0, 12)}`); + const binary = new Set((git(['diff', '--numstat', '-z', '--no-renames', headBase, head]) ?? '').split('\0').filter(l => l.startsWith('-\t-\t')).map(l => l.slice(4))); + const other = paths.filter(p => classifyPath(p, root) !== 'docs' || binary.has(p)); + if (other.length) return skip(`the change since range_base ${read.slice(0, 12)} is not docs-only (${other.join(', ')}), so it takes the review loop`); + if (plainDocsSince(root, read, head, base)) return skip(`the change since range_base ${read.slice(0, 12)} is plain docs, which the targeted record carries (GST-1195)`); + return { state: 'dispatch', range_base: read, paths }; +} diff --git a/scripts/review-dispatch-compare.ts b/scripts/review-dispatch-compare.ts index 815ffc6a0..5ffc72d09 100755 --- a/scripts/review-dispatch-compare.ts +++ b/scripts/review-dispatch-compare.ts @@ -81,7 +81,7 @@ function newShip(f: Fixture, sc: Scenario): { calls: Call[]; ending: string } { let adversarialRan = false; for (let i = 0; i < 6; i++) { const plan = planReview({ ...base(f), history: h, criticalFound: sc === 'blocker' && i === 0, adversarialRan }); - if (plan.next.step === 'done' || plan.next.step === 'stop' || plan.next.step === 'platform') { ending = plan.next.step; break; } + if (plan.next.step === 'done' || plan.next.step === 'stop' || plan.next.step === 'platform' || plan.next.step === 'targeted') { ending = plan.next.step; break; } const diffBytes = plan.next.step === 'fix-check' ? f.diff_bytes * FIX_CHECK_FRACTION : h.fullPasses === 0 ? f.diff_bytes : f.diff_bytes * RANGE_FRACTION; for (const d of plan.dispatches) calls.push({ role: d.role, name: d.name, effort: d.effort, diffBytes: d.role === 'outside-voice' ? f.diff_bytes : diffBytes }); adversarialRan ||= plan.dispatches.some(d => d.role === 'adversarial-native'); diff --git a/ship/sections/pr-body.md b/ship/sections/pr-body.md index 45011a426..e99104884 100644 --- a/ship/sections/pr-body.md +++ b/ship/sections/pr-body.md @@ -227,6 +227,9 @@ NS=$(~/.claude/skills/gstack/bin/gstack-no-stop list | sed -n 's/^NO_STOP_FILE: ~/.claude/skills/gstack/bin/gstack-ship-receipt write --pr --base --gate @.gstack/tmp/ship-gate.json --pregate .gstack/tmp/pregate.json --issues @.gstack/tmp/issues.json --review-loop @.gstack/tmp/review-loop.json --docs @.gstack/tmp/docs-audit.json $([ -s .gstack/tmp/targeted.json ] && echo --review-targeted @.gstack/tmp/targeted.json) ${NS:+--no-stop "@$NS"} --body "$PR_BODY_FILE" ``` +If write refuses `review_targeted` for a docs claim added after the read, rerun Step 9's plan with `--range-base +--targeted-ran `; on `NEXT: targeted`, run that one reader (`pass`, its voices stage, +`verify --targeted-out .gstack/tmp/targeted.json`) and write again. A Step 5 red-base pause adds `--base-red ` (a conditional receipt the queue refuses, `QUEUE_RECEIPT_CONDITIONAL`). `--docs` reads the record Step 14.5 saved (`{status, scope[, exception]}`): the receipt carries it as `docs`, and a `blocked` status without the owner's exception, diff --git a/ship/sections/pr-body.md.tmpl b/ship/sections/pr-body.md.tmpl index 097923aaa..b74d6ec78 100644 --- a/ship/sections/pr-body.md.tmpl +++ b/ship/sections/pr-body.md.tmpl @@ -210,6 +210,9 @@ NS=$(~/.claude/skills/gstack/bin/gstack-no-stop list | sed -n 's/^NO_STOP_FILE: ~/.claude/skills/gstack/bin/gstack-ship-receipt write --pr --base --gate @.gstack/tmp/ship-gate.json --pregate .gstack/tmp/pregate.json --issues @.gstack/tmp/issues.json --review-loop @.gstack/tmp/review-loop.json --docs @.gstack/tmp/docs-audit.json $([ -s .gstack/tmp/targeted.json ] && echo --review-targeted @.gstack/tmp/targeted.json) ${NS:+--no-stop "@$NS"} --body "$PR_BODY_FILE" ``` +If write refuses `review_targeted` for a docs claim added after the read, rerun Step 9's plan with `--range-base +--targeted-ran `; on `NEXT: targeted`, run that one reader (`pass`, its voices stage, +`verify --targeted-out .gstack/tmp/targeted.json`) and write again. A Step 5 red-base pause adds `--base-red ` (a conditional receipt the queue refuses, `QUEUE_RECEIPT_CONDITIONAL`). `--docs` reads the record Step 14.5 saved (`{status, scope[, exception]}`): the receipt carries it as `docs`, and a `blocked` status without the owner's exception, diff --git a/test/fixtures/review-pins/files/lib/review-depth.ts.json b/test/fixtures/review-pins/files/lib/review-depth.ts.json index 960e473cd..704b9003f 100644 --- a/test/fixtures/review-pins/files/lib/review-depth.ts.json +++ b/test/fixtures/review-pins/files/lib/review-depth.ts.json @@ -1 +1 @@ -{"sha":"713ff6413608f1ab75f2c5306286cd1f275d2434ba84bc07b3b68876f4c81a3b","replay":"GST-1276"} +{"sha":"76da578bca44e918a22014b68deb9664a4bdc04c3f4ac6b2a1360cf811f8c820","replay":"GST-1598"} diff --git a/test/fixtures/review-pins/replays/GST-1598.json b/test/fixtures/review-pins/replays/GST-1598.json new file mode 100644 index 000000000..1f4631350 --- /dev/null +++ b/test/fixtures/review-pins/replays/GST-1598.json @@ -0,0 +1 @@ +{"replay":"GST-1598","justification":"GST-1598 (targeted-only read after the loop): lib/review-depth.ts is main's GST-1276 bytes plus one member, 'targeted', in the Step type union. planReview and nextStep never return it; only bin/gstack-review-plan sets it, after planReview printed done, through lib/review-targeted-delta.ts. Deterministic no-loss replay, no paid run: the GST-967 grid (MAIN_PLANS) and the GST-972 grid (MAIN_972_PLANS) in test/review-depth.test.ts hash unchanged, so no plan main builds drops, renames or re-efforts a reader, and no brief body moves (MAIN_SECTIONS unchanged)."} diff --git a/test/review-plan-targeted-delta.test.ts b/test/review-plan-targeted-delta.test.ts new file mode 100644 index 000000000..e5d8f0b33 --- /dev/null +++ b/test/review-plan-targeted-delta.test.ts @@ -0,0 +1,168 @@ +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 { parseReviewTargeted, plainDocsSince, targetedClassesSince } from '../lib/ship-receipt-depth'; + +const BIN = path.join(import.meta.dir, '..', 'bin'); +const dirs: string[] = []; +afterEach(() => { while (dirs.length) fs.rmSync(dirs.pop()!, { recursive: true, force: true }); }); + +const MONEY_CODE = 'export const spendCapUsd = 25;\nexport function charge(amountUsd: number) { if (amountUsd > spendCapUsd) throw new Error("over cap"); return amountUsd; }\n'; + +/** A branch whose reviewed head adds money code (the targeted reader read it), as on harness-evals #225 at 563f858. */ +function reviewed(baseFiles: Record = {}, branchFiles: Record = { 'src/billing.ts': MONEY_CODE }): { dir: string; env: NodeJS.ProcessEnv; git: (...a: string[]) => string; commit: (files: Record, msg: string) => string } { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'targeted-delta-')); + const state = fs.mkdtempSync(path.join(os.tmpdir(), 'targeted-delta-state-')); + dirs.push(dir, state); + const env: NodeJS.ProcessEnv = { ...process.env, GSTACK_STATE_ROOT: state, GSTACK_APPROVALS_MIRROR: '0' }; + delete env.GSTACK_APPROVALS_DIR; + const git = (...a: string[]) => spawnSync('git', a, { cwd: dir, encoding: 'utf8', timeout: 10_000 }).stdout.trim(); + const commit = (files: Record, msg: string) => { + for (const [p, text] of Object.entries(files)) { fs.mkdirSync(path.dirname(path.join(dir, p)), { recursive: true }); fs.writeFileSync(path.join(dir, p), text); } + git('add', '.'); git('commit', '-qm', msg); + return git('rev-parse', 'HEAD'); + }; + git('init', '-q', '-b', 'main'); git('config', 'user.email', 't@t'); git('config', 'user.name', 't'); git('config', 'commit.gpgsign', 'false'); + commit({ 'README.md': '# x\n', ...baseFiles }, 'base'); + git('checkout', '-qb', 'feat'); + commit(branchFiles, 'money'); + return { dir, env, git, commit }; +} +const plan = (r: { dir: string; env: NodeJS.ProcessEnv }, args: string[]) => spawnSync(path.join(BIN, 'gstack-review-plan'), ['--base', 'main', '--gate-tagged', '0', ...args], { cwd: r.dir, encoding: 'utf8', timeout: 60_000, env: r.env }); +const patchOf = (r: { dir: string; env: NodeJS.ProcessEnv }) => JSON.parse(plan(r, ['--json']).stdout).targeted.targeted_patch_id as string; +const dispatches = (stdout: string) => stdout.split('\n').filter(l => l.startsWith('DISPATCH: ')); +/** The loop finished clean: pass 1, a clean fix-check, nothing open. */ +const done = (rangeBase: string, ran: string) => ['--pass', '1', '--last', 'fix-check', '--fixed-below', '--range-base', rangeBase, '--targeted-ran', ran]; + +describe('GST-1598: a post-review docs delta that trips a targeted claim gets one targeted-only reader', () => { + // Value: protects=the #225 shape has an exit: when the loop is done and the only change since the targeted read is a docs file + // with a money claim, the plan dispatches exactly one targeted reader for the head's classes, prints the delta's range_base, + // and once that reader's id is passed back the plan is done with no reader; the receipt binds a record read at HEAD and refuses + // the one read before the claim; fails_when=the plan prints NEXT: done with no dispatch (harness-evals #225), dispatches any + // other reader, or keeps dispatching after the read; seam=bin/gstack-review-plan, parseReviewTargeted with the receipt's own + // carry predicates; no_claim=what the reader finds + test('the #225 shape: one targeted reader on the money claim, then the receipt binds its record', () => { + const r = reviewed(); + const read = r.git('rev-parse', 'HEAD'); + const readPatch = patchOf(r); + expect(dispatches(plan(r, ['--pass', '1', '--last', 'fix-check', '--fixed-below', '--targeted-ran', readPatch]).stdout)).toEqual([]); + const head = r.commit({ 'changelog.d/eval-cost.md': '- Each PR eval run now costs about $4.20 instead of $11.\n' }, 'changelog'); + const p = plan(r, done(read, readPatch)); + expect(p.status, p.stderr).toBe(0); + expect(p.stdout).toContain('NEXT: targeted'); + expect(dispatches(p.stdout)).toEqual(['DISPATCH: targeted targeted effort=full classes=money']); + expect(p.stdout).toContain(`TARGETED_DELTA: range_base=${read} paths=changelog.d/eval-cost.md classes=money`); + const j = JSON.parse(plan(r, [...done(read, readPatch), '--json']).stdout); + expect([j.next.step, j.dispatches.map((d: any) => d.role), j.targeted_delta.range_base]).toEqual(['targeted', ['targeted'], read]); + const headPatch = patchOf(r); + const after = plan(r, done(read, headPatch)); + expect(after.stdout).toContain('NEXT: done'); + expect(dispatches(after.stdout)).toEqual([]); + const record = (at: string) => ({ classes: ['money'], rules: 'r', brief_sha: 'a'.repeat(64), status: 'done', findings: 0, raised_by: 'targeted', head: at }); + const productOf = (c: string) => spawnSync('git', ['patch-id', '--verbatim'], { input: spawnSync('git', ['diff', 'main', c], { cwd: r.dir, encoding: 'utf8', timeout: 10_000 }).stdout, cwd: r.dir, encoding: 'utf8', timeout: 10_000 }).stdout.split(' ')[0]; + const bind = (at: string) => parseReviewTargeted(record(at), productOf(head)!, productOf, from => targetedClassesSince(r.dir, from, head, 'main'), from => plainDocsSince(r.dir, from, head, 'main')); + expect(bind(read).ok).toBe(false); + expect(bind(head)).toMatchObject({ ok: true, targeted: { classes: ['money'], status: 'done' } }); + }); + + // Value: protects=a code change after the loop keeps today's route (the review loop, not a targeted-only read); + // fails_when=the targeted-only step admits a code delta; seam=bin/gstack-review-plan; no_claim=how finish routes it + test('a code delta after the read keeps today\'s behavior: no targeted-only reader', () => { + const r = reviewed(); + const read = r.git('rev-parse', 'HEAD'); + const readPatch = patchOf(r); + r.commit({ 'src/billing.ts': MONEY_CODE.replace('25', '50'), 'changelog.d/eval-cost.md': '- Cap raised to $50.\n' }, 'code'); + const p = plan(r, done(read, readPatch)); + expect(p.status, p.stderr).toBe(0); + expect(p.stdout).toContain('NEXT: done'); + expect(dispatches(p.stdout)).toEqual([]); + expect(p.stdout).toContain('SKIP: targeted-only read (the change since range_base'); + }); + + // Value: protects=a plain docs change still carries the record, so no reader is spent on it (GST-1195), and the range_base must be + // the commit the targeted reader read; fails_when=the plan dispatches on plain docs or on a range_base whose patch the passed + // --targeted-ran does not name; seam=bin/gstack-review-plan; no_claim=receipt carry itself (parseReviewTargeted's tests) + test('plain docs, or a range_base the targeted read did not read, dispatch nothing', () => { + const r = reviewed(); + const read = r.git('rev-parse', 'HEAD'); + const readPatch = patchOf(r); + r.commit({ 'docs/howto.md': '# How to\nRun the setup script first.\n' }, 'plain docs'); + expect(dispatches(plan(r, done(read, readPatch)).stdout)).toEqual([]); + const plainHead = r.git('rev-parse', 'HEAD'); + r.commit({ 'changelog.d/eval-cost.md': '- Each PR eval run now costs about $4.20.\n' }, 'claim'); + const wrong = plan(r, done(plainHead, readPatch)); + expect(dispatches(wrong.stdout)).toEqual([]); + expect(wrong.stdout).toContain('SKIP: targeted-only read (--targeted-ran'); + }); + + // Value: protects=a main merge that only edits lines next to a reviewed branch hunk (comparePath's `context`) is no branch change, + // so the docs claim after it still plans the targeted-only read; fails_when=the delta counts a context-only path as changed and + // keeps NEXT: done with a not-docs-only skip (the GST-1598 dead end after a base merge); seam=bin/gstack-review-plan; + // no_claim=a base merge that edits the reviewed lines themselves + test('a base merge that only moves context next to a branch hunk still plans the targeted-only read', () => { + const billing = (cap: string, note: string) => ['// billing', '// rates', `// ${note}`, '', `export const spendCapUsd = ${cap};`, 'export function charge(amountUsd: number) { if (amountUsd > spendCapUsd) throw new Error("over cap"); return amountUsd; }', ''].join('\n'); + const r = reviewed({ 'src/billing.ts': billing('25', 'old note') }, { 'src/billing.ts': billing('30', 'old note') }); + const read = r.git('rev-parse', 'HEAD'); + const readPatch = patchOf(r); + r.git('checkout', '-q', 'main'); + r.commit({ 'src/billing.ts': billing('25', 'new note') }, 'base comment'); + r.git('checkout', '-q', 'feat'); + r.git('merge', '-q', '--no-edit', 'main'); + r.commit({ 'changelog.d/eval-cost.md': '- The spend cap is now $30.\n' }, 'claim'); + const p = plan(r, done(read, readPatch)); + expect(p.status, p.stderr).toBe(0); + expect(p.stdout).toContain('NEXT: targeted'); + expect(p.stdout).toContain(`TARGETED_DELTA: range_base=${read} paths=changelog.d/eval-cost.md classes=money`); + }); + + // Value: protects=a grant is not what opens this read, and a grant the approval record does not hold still refuses before any plan; + // fails_when=a fabricated --grant prints a plan; seam=bin/gstack-review-plan's grant check; no_claim=who may grant + test('a fabricated grant refuses', () => { + const r = reviewed(); + const read = r.git('rev-parse', 'HEAD'); + const readPatch = patchOf(r); + const head = r.commit({ 'changelog.d/eval-cost.md': '- Each PR eval run now costs about $4.20.\n' }, 'changelog'); + const p = plan(r, [...done(read, readPatch), '--grant', 'a-000000000000']); + expect([p.status, p.stdout]).toEqual([3, '']); + expect(p.stderr).toContain('--grant a-000000000000'); + expect(p.stderr).toContain('review_loop_grant'); + expect(head).toMatch(/^[0-9a-f]{40}$/); + }); + + // Value: protects=--force targeted no longer dispatches zero readers in silence: targeted is not a specialist, so the flag refuses and + // names the route; fails_when=--force targeted exits 0; seam=bin/gstack-review-plan; no_claim=other --force names + test('--force targeted refuses and names the route', () => { + const r = reviewed(); + const p = plan(r, ['--force', 'targeted']); + expect(p.status).toBe(2); + expect(p.stderr).toContain('--range-base'); + expect(p.stderr).toContain('--targeted-ran'); + }); + + // Value: protects=the pass command builds the targeted-only manifest (the brief renders only inside a pass manifest when an exclusion + // is active): one targeted reader in the voices stage on the merge base, the TARGETED_DELTA line in its plan lines, and its voices + // stage renders the brief on HEAD; fails_when=pass prints PASS: none for NEXT: targeted, or the stage renders on another base; + // seam=gstack-review-dispatch pass and stage; no_claim=the reader's reply + test('gstack-review-dispatch pass builds a manifest with the one targeted reader', () => { + const r = reviewed(); + const read = r.git('rev-parse', 'HEAD'); + const readPatch = patchOf(r); + r.commit({ 'changelog.d/eval-cost.md': '- Each PR eval run now costs about $4.20.\n' }, 'changelog'); + const out = fs.mkdtempSync(path.join(os.tmpdir(), 'targeted-delta-out-')); + dirs.push(out); + const p = spawnSync(path.join(BIN, 'gstack-review-dispatch'), ['pass', '--base', 'main', '--out', out, '--pass', 'td1', '--', '--gate-tagged', '0', ...done(read, readPatch)], { cwd: r.dir, encoding: 'utf8', timeout: 120_000, env: r.env }); + expect(p.status, p.stderr).toBe(0); + expect(p.stdout).toMatch(/MANIFEST: file=\S+ pass=td1 readers=1 first=0 specialists=0 red-team=0 voices=1/); + const file = p.stdout.match(/MANIFEST: file=(\S+)/)![1]!; + const m = JSON.parse(fs.readFileSync(file, 'utf8')); + const mergeBase = r.git('merge-base', 'main', 'HEAD'); + expect([m.next, m.readers.map((x: any) => [x.role, x.stage, x.classes, x.diff_base])]).toEqual(['targeted', [['targeted', 'voices', ['money'], mergeBase]]]); + expect(m.plan_lines.filter((l: string) => l.startsWith('TARGETED_DELTA: '))).toEqual([`TARGETED_DELTA: range_base=${read} paths=changelog.d/eval-cost.md classes=money`]); + const stage = spawnSync(path.join(BIN, 'gstack-review-dispatch'), ['stage', '--manifest', file, '--stage', 'voices'], { cwd: r.dir, encoding: 'utf8', timeout: 120_000, env: r.env }); + expect(stage.status, stage.stderr).toBe(0); + const rendered = fs.readFileSync(m.ledger, 'utf8').split('\n').filter(Boolean).map(l => JSON.parse(l)).filter(e => e.event === 'rendered'); + expect(rendered.map(e => [e.reader, e.diff_base, e.head, e.classes])).toEqual([['targeted', mergeBase, r.git('rev-parse', 'HEAD'), ['money']]]); + }); +}); diff --git a/test/size-ledger/ship/gst-1598-targeted-delta.json b/test/size-ledger/ship/gst-1598-targeted-delta.json new file mode 100644 index 000000000..7f326da86 --- /dev/null +++ b/test/size-ledger/ship/gst-1598-targeted-delta.json @@ -0,0 +1,4 @@ +{ + "bytes": 301, + "why": "GST-1598: /ship Step 19 names the targeted-only read when write refuses review_targeted after a post-review docs claim" +}