From d39ea5d6318cfe5732fdce08303b0bdd6e30ff92 Mon Sep 17 00:00:00 2001 From: garrytan Date: Mon, 26 Oct 2026 21:46:04 +0000 Subject: [PATCH] fix: pre-landing review fixes (pass 1) Twelve informational findings from the /ship pass-1 review, all below the blocking floor, fixed in place: - docs-caps reads docs.caps from origin/'s policy (--base ), never the branch's copy; an unavailable base policy refuses with POLICY_SOURCE_UNAVAILABLE instead of substituting DEFAULT_POLICY - receiptVersion returns "none" only when the stamp file is absent; other errors surface as undefined - checkReceiptProse groups counters by leaf key and uses word boundaries that cover hyphenated names, so shared leaf keys and hyphenated words no longer report RECEIPT_PROSE_MISMATCH - gstack-coauthor-trailers refuses a --range/--base starting with "-", prefixes origin/ when origin/ resolves (slash-named branches), and passes --end-of-options to git log - REVIEW_SCOPE_KEY_VERSION and DOCS_CAPS_KEY_VERSION name the releases that first read their keys - the Step 11 late-defer row logs under skill review-late-defer so a tokenless review row never reads UNVERIFIED; summarizeReviewLoop counts both - the "already filed" literal is one constant shared by bin and renderer - receiptDocs (dead) removed - Step 14.5 clears .gstack/tmp/docs-audit.json on first entry - dispatched review prompts carry a literal diff-base sha () instead of a shell variable the reviewer process never sees - the coauthor-trailers tests launch the bin through runScriptSync so the Windows lane can run them Tests extended for each change; SKILL.md, sections and goldens regenerated. Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- bin/gstack-coauthor-trailers | 9 +++- bin/gstack-review-scope | 4 +- bin/gstack-ship-receipt | 52 ++++++++++++------ docs/review-policy.md | 2 +- lib/result-codes-release.ts | 2 +- lib/review-scope.ts | 16 +++--- lib/ship-policy.ts | 7 ++- lib/ship-receipt-docs.ts | 6 --- lib/ship-receipt-prose.ts | 20 ++++--- scripts/resolvers/review-army.ts | 11 ++-- ship/sections/documentation.md | 9 ++-- ship/sections/documentation.md.tmpl | 9 ++-- ship/sections/review-army.md | 20 +++---- ship/sections/review-army.md.tmpl | 16 +++--- test/gstack-coauthor-trailers.test.ts | 22 +++++++- test/gstack-ship-policy.test.ts | 8 +-- test/gstack-ship-receipt-docs.test.ts | 72 +++++++++++++++++++++++-- test/helpers/ship-frozen-scope-actor.ts | 5 +- test/review-fix-range.test.ts | 3 +- test/review-scope-followups.test.ts | 16 ++++-- test/ship-receipt-prose.test.ts | 10 ++++ test/ship-workflow-clarity.test.ts | 13 +++-- 22 files changed, 241 insertions(+), 91 deletions(-) diff --git a/bin/gstack-coauthor-trailers b/bin/gstack-coauthor-trailers index 67cd78852..a5f3e0665 100755 --- a/bin/gstack-coauthor-trailers +++ b/bin/gstack-coauthor-trailers @@ -48,15 +48,20 @@ function main(argv: string[]): number { else if (range) { process.stderr.write(`gstack-coauthor-trailers: one range only\n${HELP}\n`); return EXIT.usage; } else range = arg; } + if ((range ?? base ?? '').startsWith('-')) { process.stderr.write(`gstack-coauthor-trailers: a range or base cannot start with -\n${HELP}\n`); return EXIT.usage; } const root = git(['rev-parse', '--show-toplevel']); if (!root.ok) { process.stderr.write('gstack-coauthor-trailers: not inside a git repository\n'); return EXIT.fail; } if (!range) { + // The base branch name may itself hold a slash (release/1.2), so "has a slash" is not "already qualified": + // origin/ wins when it resolves; a sha or a ref that only resolves as given is used as given. const ref = base ?? resolveBaseBranch(root.out); - range = `${/^[0-9a-f]{7,64}$/i.test(ref) || ref.includes('/') ? ref : `origin/${ref}`}..HEAD`; + const qualified = /^[0-9a-f]{7,64}$/i.test(ref) || ref.startsWith('origin/') ? ref + : git(['rev-parse', '--verify', '--quiet', `origin/${ref}^{commit}`]).ok ? `origin/${ref}` : ref; + range = `${qualified}..HEAD`; } const self = /<([^>]*)>/.exec(git(['var', 'GIT_COMMITTER_IDENT']).out)?.[1]?.toLowerCase(); if (self) excludes.add(self); - const log = git(['log', '--no-merges', '--reverse', '--format=%an%x1f%ae', range]); + const log = git(['log', '--no-merges', '--reverse', '--format=%an%x1f%ae', '--end-of-options', range]); if (!log.ok) { process.stderr.write(`gstack-coauthor-trailers: cannot read ${range}: ${log.err.split('\n')[0] ?? ''}\n`); return EXIT.fail; } const authors = new Map(); for (const line of log.out.split('\n').filter(Boolean)) { diff --git a/bin/gstack-review-scope b/bin/gstack-review-scope index ee24f6506..569aba495 100755 --- a/bin/gstack-review-scope +++ b/bin/gstack-review-scope @@ -27,7 +27,7 @@ import { isUnavailable, loadPolicyFromBase, policyUnavailableLine, resolveBaseBr import { EXIT as POST_EXIT, runPost } from '../lib/gstack-post'; import { parseHunks, renderDeferredIssue, renderDeferredSection, reviewLoopLine, scopeFindings, scopeLine, summarizeReviewLoop, - DEFERRED_MARKER, findingIdentity, findingWhere, REVIEW_SCOPE_POLICIES, type DeferredRecord, type ReviewScopePolicy, type ScopeFinding, type ScopedFinding, + ALREADY_FILED_REASON, 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) @@ -201,7 +201,7 @@ function cmdDefer(rest: string[], json: boolean): number { 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; } + if (hit) { record.issue = hit.number; record.url = hit.url; record.reason = ALREADY_FILED_REASON; continue; } const created = publishIssue(repoRoot, repo, title, body); if (created.ok) { const url = created.out.split('\n').map(l => l.trim()).filter(l => /\/issues\/\d+$/.test(l)).pop() ?? ''; diff --git a/bin/gstack-ship-receipt b/bin/gstack-ship-receipt index d0aff6a75..533262e14 100755 --- a/bin/gstack-ship-receipt +++ b/bin/gstack-ship-receipt @@ -13,7 +13,7 @@ * gstack-ship-receipt result (--pr [--repo ] | --body ) [--head ] [--out ] [--json] * gstack-ship-receipt census --repo --since [--cohort ] [--limit ] [--json] * gstack-ship-receipt carry (--pr [--repo ] | --body ) [--base ] [--head ] [--dry-run] [--out ] [--json] - * gstack-ship-receipt docs-caps [--policy ] [--json] + * gstack-ship-receipt docs-caps [--base ] [--policy ] [--json] * * `write` validates against `gstack-artifact schema ship-receipt`, prints the * fenced block (and SHIP_RECEIPT: on stderr), writes it to --out, @@ -36,7 +36,7 @@ import { EXIT, exitTableHelp, type ShipReceipt } from '../lib/headless-artifacts import { resultLine, resultLineWithDocs } from '../lib/result-codes'; import { versionAtRef } from '../lib/restamp'; import { readPregateFile, verifyPregate } from '../lib/pregate/index'; -import { DEFAULT_POLICY, SHIP_POLICY_PATH, isUnavailable, loadPolicyFromBase, matchesAny, resolveBaseBranch, stampFilesInTree, validatePolicy, type ShipPolicy } from '../lib/ship-policy'; +import { isUnavailable, loadPolicyFromBase, matchesAny, policyUnavailableLine, resolveBaseBranch, stampFilesInTree, validatePolicy, type ShipPolicy } from '../lib/ship-policy'; import { approvalForReceipt } from '../lib/approval-citations'; import { CARRY_METADATA_GLOBS, CARRY_NAMED_PATHS, CARRY_TEST_GLOBS, @@ -66,8 +66,9 @@ const HELP = `gstack-ship-receipt — the \`\`\`gstack-ship-receipt\`\`\` block posts the body (gh pr edit) and prints GSTACK_RESULT: skill=ship status=complete; RECEIPT_CARRY_PRODUCT_CHANGED / RECEIPT_CARRY_REFUSED leave the body unchanged (exit 3); needs git >= 2.39 (RECEIPT_GIT_TOO_OLD) - docs-caps [--policy ] /ship Step 14.5: one DOCS_CAP: / ok|over line per docs.caps entry of the - policy (default .gstack/ship-policy.json); any over (or absent) file exits 1 (DOCS_AUDIT_BLOCKED) + docs-caps [--base ] /ship Step 14.5: one DOCS_CAP: / ok|over line per docs.caps entry of the + reviewed policy (origin/'s .gstack/ship-policy.json; --policy reads that file + instead); any over (or absent) file exits 1 (DOCS_AUDIT_BLOCKED) write flags: --pr --head --base --version --gated-tree --predecessor --gate --ci --spend-usd --session-kind --artifacts-consumed / --run --thread --receipt --policy --preregistration @@ -115,16 +116,20 @@ function gh(args: string[]): { ok: boolean; out: string; err: string } { * policy stamp file and no CHANGELOG at HEAD has no release version to record, * and the receipt says so with `"none"` (docs/ship-policy.md#ship-receipt); * `read`, `result` and the queue accept it like any other version string. + * Only an absent source is `"none"`: a VERSION that exists but holds no + * parsable version, a pin that escapes the repository, or an unreadable base + * policy keeps the existing no-version refusal. */ function receiptVersion(repoRoot: string, head: string, base: string): string | undefined { const atHead = versionAtRef(repoRoot, head); if ('version' in atHead) return atHead.version; + if (!/ does not exist at /.test(atHead.error)) return undefined; const show = (rel: string) => git(['show', `${head}:${rel}`], repoRoot); try { const v = JSON.parse(show('package.json') ?? 'null')?.version; if (typeof v === 'string' && v.trim()) return v.trim(); } catch { /* not a manifest */ } const load = loadPolicyFromBase(repoRoot, base); - const policy = isUnavailable(load) ? DEFAULT_POLICY : load.policy; - const changelogs = [...new Set([policy.changelog, 'CHANGELOG.md'].filter((p): p is string => !!p))]; - if (stampFilesInTree(repoRoot, head, policy.stamp_files).length || changelogs.some(p => show(p) !== undefined)) return undefined; + if (isUnavailable(load)) return undefined; + const changelogs = [...new Set([load.policy.changelog, 'CHANGELOG.md'].filter((p): p is string => !!p))]; + if (stampFilesInTree(repoRoot, head, load.policy.stamp_files).length || changelogs.some(p => show(p) !== undefined)) return undefined; return 'none'; } @@ -524,26 +529,39 @@ function cmdCensus(rest: string[], json: boolean): number { } /** - * /ship Step 14.5's doc-cap check: every `docs.caps` entry of the policy against - * the working tree, one DOCS_CAP: line each; any `over` (an absent file counts) - * exits 1 so the audit blocks before the verification gate. + * /ship Step 14.5's doc-cap check: every `docs.caps` entry of the reviewed + * policy (origin/'s .gstack/ship-policy.json, like every other policy + * read here; the branch cannot raise its own cap) against the working tree, + * one DOCS_CAP: line each; any `over` (an absent file counts) exits 1 so the + * audit blocks before the verification gate. `--policy ` reads that + * file instead, for a repository bootstrapping its policy. */ function cmdDocsCaps(rest: string[], json: boolean): number { const repoRoot = git(['rev-parse', '--show-toplevel']) ?? process.cwd(); - const policyFile = flag(rest, '--policy') ?? path.join(repoRoot, SHIP_POLICY_PATH); - let policy: Partial = {}; - if (fs.existsSync(policyFile)) { + const policyFile = flag(rest, '--policy'); + let caps: NonNullable['caps'] = []; + let source: string; + if (policyFile !== undefined) { + if (!fs.existsSync(policyFile)) { process.stderr.write(`gstack-ship-receipt docs-caps: ${policyFile} does not exist\n`); return EXIT.usage; } let raw: unknown; try { raw = JSON.parse(fs.readFileSync(policyFile, 'utf8')); } catch (e: any) { process.stderr.write(`gstack-ship-receipt docs-caps: ${policyFile} is not JSON: ${e.message}\n`); return EXIT.fail; } const errors = validatePolicy(raw).filter(e => e.path === 'docs' || e.path.startsWith('docs.')); if (errors.length) { for (const e of errors) process.stderr.write(resultLine('gstack-ship-receipt docs-caps', e.code, `${e.message} at ${e.path}`) + '\n'); return EXIT.fail; } - policy = raw as Partial; - } else if (flag(rest, '--policy')) { process.stderr.write(`gstack-ship-receipt docs-caps: ${policyFile} does not exist\n`); return EXIT.usage; } - const caps = policy.docs?.caps ?? []; + caps = (raw as Partial).docs?.caps ?? []; + source = policyFile; + } else { + const base = flag(rest, '--base') ?? resolveBaseBranch(repoRoot); + const load = loadPolicyFromBase(repoRoot, base); + if (isUnavailable(load)) { process.stderr.write(policyUnavailableLine('gstack-ship-receipt docs-caps', base, `${load.detail}; pass --policy to check caps without the base`) + '\n'); return EXIT.refused; } + const errors = load.errors.filter(e => e.path === 'docs' || e.path.startsWith('docs.')); + if (errors.length) { for (const e of errors) process.stderr.write(resultLine('gstack-ship-receipt docs-caps', e.code, `${e.message} at ${e.path} (${load.source.label})`) + '\n'); return EXIT.fail; } + caps = load.policy.docs?.caps ?? []; + source = load.source.label; + } const results = checkDocsCaps(caps, rel => { try { const st = fs.statSync(path.join(repoRoot, rel)); return st.isFile() ? st.size : null; } catch { return null; } }); const over = results.filter(r => r.verdict === 'over'); const lines = results.length ? results.map(docsCapLine) : ['DOCS_CAP: none declared']; - process.stdout.write(json ? JSON.stringify({ policy: policyFile, caps: results, over: over.length }) + '\n' : lines.join('\n') + '\n'); + process.stdout.write(json ? JSON.stringify({ policy: source, caps: results, over: over.length }) + '\n' : lines.join('\n') + '\n'); if (!over.length) return EXIT.ok; process.stderr.write(resultLineWithDocs('gstack-ship-receipt docs-caps', 'DOCS_AUDIT_BLOCKED', `${over.length} of ${results.length} documentation cap${results.length === 1 ? '' : 's'} over: ${over.map(r => r.path).join(', ')}`) + '\n'); return EXIT.fail; diff --git a/docs/review-policy.md b/docs/review-policy.md index 5f4c35afd..cd9ec9849 100644 --- a/docs/review-policy.md +++ b/docs/review-policy.md @@ -88,7 +88,7 @@ puts in the receipt: |---|---| | `review_loop.passes` | review passes this ship ran (1 to 3) | | `review_loop.blocking_found` | distinct blocking findings across the passes | -| `review_loop.deferred` | blocking findings in unchanged code filed as issues (a Step 11 late defer, logged as a `review` row with `phase: late-defer`, counts here and is not a pass) | +| `review_loop.deferred` | blocking findings in unchanged code filed as issues (a Step 11 late defer, logged as a `review-late-defer` row so it never displaces the newest `review` pass, counts here and is not a pass) | | `review_loop.noted` | below-floor findings listed in the PR body | | `review_loop.fixed` | findings fixed in this PR | | `review_loop.scope` | the `review_scope` the loop ran under | diff --git a/lib/result-codes-release.ts b/lib/result-codes-release.ts index be4944612..8611405b1 100644 --- a/lib/result-codes-release.ts +++ b/lib/result-codes-release.ts @@ -231,7 +231,7 @@ export const RELEASE_RESULT_CODES = { }, DOCS_AUDIT_BLOCKED: { anchor: 'docs-audit-blocked', - summary: 'the documentation audit (/ship Step 14.5) is blocked and no owner exception is recorded, or a declared doc cap is over, so gstack-ship-receipt write refused to record the ship as audited', + summary: 'the documentation audit (/ship Step 14.5) is blocked: gstack-ship-receipt docs-caps found a declared doc cap over, or gstack-ship-receipt write was given a blocked audit with no owner exception and refused to record the ship as audited', fix: 'repair the audit (or trim the doc named by the DOCS_CAP: over line) and rerun Step 14.5; when the owner accepts the named risk, record it as docs.exception { accepted_by, date, via, reason } and carry the same line in the PR body\'s Documentation section', }, RECEIPT_PROSE_MISMATCH: { diff --git a/lib/review-scope.ts b/lib/review-scope.ts index 0d1ed4221..f6e77d1fc 100644 --- a/lib/review-scope.ts +++ b/lib/review-scope.ts @@ -209,6 +209,8 @@ 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 } +/** `defer`'s reason when the marker search found the finding's issue already open; the section renders such a record as a plain `#N`. */ +export const ALREADY_FILED_REASON = 'already filed'; /** 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 { @@ -216,7 +218,7 @@ export function renderDeferredSection(deferred: readonly DeferredRecord[], noted if (deferred.length) { 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}${d.reason && d.reason !== 'already filed' ? ` (${oneLine(d.reason)})` : ''}` : `unfiled${d.reason ? ` (${oneLine(d.reason)})` : ''}`; + const ref = d.issue ? `#${d.issue}${d.reason && d.reason !== ALREADY_FILED_REASON ? ` (${oneLine(d.reason)})` : ''}` : `unfiled${d.reason ? ` (${oneLine(d.reason)})` : ''}`; out.push(`- Deferred: ${ref} — ${oneLine(d.severity)} \`${oneLine(d.path)}\` ${oneLine(d.summary)}`); } out.push(''); @@ -251,16 +253,18 @@ function rowTime(row: Record): number { /** * A Step 11 late defer (a queued adversarial finding scoped `defer` after the - * last Step 9 pass) is logged as a `review` row with this phase: its deferred - * findings count, but it is not a pass. + * last Step 9 pass) is logged under this skill, not as a `review` row: its + * deferred findings count here, it is not a pass, and the dashboards and + * readiness gates that pick the newest `review` row never see it (a `review` + * row without a start token would read as UNVERIFIED and displace the pass). */ -export const LATE_DEFER_PHASE = 'late-defer'; +export const LATE_DEFER_SKILL = 'review-late-defer'; export function summarizeReviewLoop(rows: readonly Record[], since: string): ReviewLoopSummary { const sinceMs = Date.parse(since); - const loopRows = rows.filter(r => r && r.skill === 'review' && r.via === 'ship' && Number.isFinite(rowTime(r)) + const loopRows = rows.filter(r => r && (r.skill === 'review' || r.skill === LATE_DEFER_SKILL) && r.via === 'ship' && Number.isFinite(rowTime(r)) && (!Number.isFinite(sinceMs) || rowTime(r) >= sinceMs)); - const passes = loopRows.filter(r => r.phase !== LATE_DEFER_PHASE); + const passes = loopRows.filter(r => r.skill === 'review'); const blocking = new Set(); const deferred = new Set(); const noted = new Set(); diff --git a/lib/ship-policy.ts b/lib/ship-policy.ts index 96dcaf7c5..5336a797a 100644 --- a/lib/ship-policy.ts +++ b/lib/ship-policy.ts @@ -159,8 +159,11 @@ export const POLICY_EXPLANATIONS: Record = { export const POLICY_COMPAT_FLOOR = '1.91.98.0'; export const SHIP_QUEUE_KEYS_VERSION = '1.91.108.0'; export const TRAIN_KEYS_VERSION = '1.91.146.0'; -export const REVIEW_SCOPE_KEY_VERSION = '1.91.249.0'; -export const DOCS_CAPS_KEY_VERSION = '1.91.250.0'; +// Each constant is the release that first reads its key: a policy declaring the key must name at least +// that version as min_gstack_version, so an older gstack refuses the policy by name instead of failing on +// an unknown key. review_scope shipped in 1.91.256.0 (#3481); docs.caps ships in the release after it. +export const REVIEW_SCOPE_KEY_VERSION = '1.91.256.0'; +export const DOCS_CAPS_KEY_VERSION = '1.91.257.0'; export const POLICY_KEY_INTRODUCED: Record = { bump: '1.91.73.0', queue_mode: '1.91.73.0', release_tool: '1.91.73.0', release_outputs: '1.91.73.0', stamp_paths: '1.91.73.0', changelog: '1.91.73.0', mirror: '1.91.73.0', history: '1.91.73.0', preregistration_shas: '1.91.73.0', stamp_sensitive_gates: '1.91.73.0', diff --git a/lib/ship-receipt-docs.ts b/lib/ship-receipt-docs.ts index 5f3882e2e..ceb64c1f6 100644 --- a/lib/ship-receipt-docs.ts +++ b/lib/ship-receipt-docs.ts @@ -47,12 +47,6 @@ export function docsExceptionLine(docs: ReceiptDocs): string | null { return `Documentation exception: accepted by ${e.accepted_by} ${e.date} via ${e.via}: ${e.reason}`; } -/** The `docs` object a receipt carries, when it validates; receipts written before this record have none. */ -export function receiptDocs(receipt: Record): ReceiptDocs | undefined { - const parsed = parseReceiptDocs(receipt.docs); - return parsed.ok ? parsed.docs : undefined; -} - // --------------------------------------------------------------------------- // doc caps: `docs.caps` in .gstack/ship-policy.json // --------------------------------------------------------------------------- diff --git a/lib/ship-receipt-prose.ts b/lib/ship-receipt-prose.ts index 09cc86b3f..471cc8e26 100644 --- a/lib/ship-receipt-prose.ts +++ b/lib/ship-receipt-prose.ts @@ -30,22 +30,28 @@ const ESCAPE = /[.*+?^${}()|[\]\\]/g; /** * Every `=`, `: ` and ` ` in the prose (the body * without its receipt block), where `` is a counter's leaf key with `_` - * matched as `_`, space or `-`, compared with the receipt. A leaf key shared - * by several counters (gate.passed and ci.passed) is checked against each. + * matched as `_`, space or `-` and not part of a longer word (`3 pass-through` + * is not a `pass` claim), compared with the receipt. A leaf key shared by + * several counters (`pass` under every gate lane, `passed` under gate and ci) + * names any of them: the claim matches when its number equals one of those + * counters, and a mismatch names the first of them. */ export function checkReceiptProse(receipt: Record, body: string): ProseCheck { const prose = body.replace(BLOCK_RE, ''); - const counters = receiptCounters(receipt); + const byLeaf = new Map>(); + for (const [field, value] of receiptCounters(receipt)) { + const leaf = field.split('.').at(-1)!; + byLeaf.set(leaf, [...(byLeaf.get(leaf) ?? []), { field, value }]); + } const mismatches: ProseClaim[] = []; let checked = 0; - for (const [field, value] of counters) { - const leaf = field.split('.').at(-1)!; + for (const [leaf, counters] of byLeaf) { const name = leaf.replace(ESCAPE, '\\$&').replaceAll('_', '[ _-]'); - const re = new RegExp(`(?:\\b${name}\\s*[=:]\\s*(\\d+)\\b|\\b(\\d+)\\s+${name}\\b)`, 'gi'); + const re = new RegExp(`(?:(? c.value === n)) mismatches.push({ field: counters[0]!.field, prose: n, receipt: counters[0]!.value }); } } return { checked, mismatches }; diff --git a/scripts/resolvers/review-army.ts b/scripts/resolvers/review-army.ts index 7b06114c3..67fc6f125 100644 --- a/scripts/resolvers/review-army.ts +++ b/scripts/resolvers/review-army.ts @@ -88,9 +88,14 @@ Print the selection: "Dispatching N specialists: [names]. Skipped: [names] (scope not detected). Gated: [names] (0 findings in N+ reviews)."`; } -/** The diff command every dispatched reviewer runs: in /ship, pass 2+ reads the range since the previous pass (RANGE_BASE), never the whole branch again. */ +/** + * The diff command every dispatched reviewer runs. In /ship the parent writes the + * base sha into the prompt (the merge base on pass 1, RANGE_BASE from pass 2 on): + * a reviewer is its own process and never sees the parent's shell variables, so a + * `$RANGE_BASE` reference there would silently fall back to the whole branch. + */ function diffBase(ctx: TemplateContext): string { - return ctx.skillName === 'ship' ? 'DIFF_BASE=${RANGE_BASE:-$(git merge-base origin/ HEAD)}' : 'DIFF_BASE=$(git merge-base origin/ HEAD)'; + return ctx.skillName === 'ship' ? 'DIFF_BASE=' : 'DIFF_BASE=$(git merge-base origin/ HEAD)'; } function generateSpecialistDispatch(ctx: TemplateContext): string { @@ -134,7 +139,7 @@ Every other specialist, including Testing, reads the full diff with fixtures. 5. Instructions: "You are a specialist code reviewer. Read the checklist at {checklist path}, then run -\`${diffBase(ctx)} && git diff "$DIFF_BASE"\` to get the ${ctx.skillName === 'ship' ? 'diff (the whole branch on pass 1; from pass 2 on, RANGE_BASE is set and the diff is the range since the previous pass)' : 'full diff'}. Apply the checklist against the diff. +\`${diffBase(ctx)} && git diff "$DIFF_BASE"\` to get the ${ctx.skillName === 'ship' ? 'diff (``: write the sha in before dispatch, the merge base on pass 1 and `RANGE_BASE` from pass 2 on; never a shell variable)' : 'full diff'}. Apply the checklist against the diff. \`NO FINDINGS\` alone is the expected, complete result for a clean diff; never invent an item to show work. Otherwise output one JSON object per finding, on its own line: {\\"severity\\":\\"CRITICAL|INFORMATIONAL\\",\\"confidence\\":N,\\"path\\":\\"file\\",\\"line\\":N,\\"category\\":\\"category\\",\\"summary\\":\\"description\\",\\"fix\\":\\"recommended fix\\",\\"fingerprint\\":\\"path:line:category\\",\\"specialist\\":\\"name\\"} diff --git a/ship/sections/documentation.md b/ship/sections/documentation.md index 45027dffc..f8de75088 100644 --- a/ship/sections/documentation.md +++ b/ship/sections/documentation.md @@ -10,8 +10,8 @@ or inline takeover, including failed launches; inline work follows the same validation gates. A stale snapshot is neither a new attempt nor a current audit. Save the child handle. An exited child with missing output is stopped, but its audit is blocked. -**Entry:** First entry always launches the initial audit. -On reentry, reuse only this invocation's validated audit or named-risk decision whose accepted +**Entry:** First entry runs `rm -f .gstack/tmp/docs-audit.json` (an earlier run's +record would pass as this run's audit) and launches the initial audit. On reentry, reuse only this invocation's validated audit or named-risk decision whose accepted base/input hashes still match; retain its actual status and scope. Otherwise use Blocked recovery, not an unconditional launch. Reentry never resets the count or authorizes a launch. @@ -86,9 +86,12 @@ A failed check or `blocked` result goes to recovery, even with valid JSON. Otherwise run the doc caps the base policy declares, before the verification gate: ```bash -~/.claude/skills/gstack/bin/gstack-ship-receipt docs-caps --policy .gstack/ship-policy.json +~/.claude/skills/gstack/bin/gstack-ship-receipt docs-caps --base ``` +It reads `docs.caps` from `origin/`'s `.gstack/ship-policy.json`, never the +branch's copy, so a PR cannot raise its own cap. + It prints one `DOCS_CAP: / ok|over` line per `docs.caps` entry (`DOCS_CAP: none declared` without any) and exits 1 on any `over` (an absent file counts): that is a blocked audit whose repair is trimming the named file, diff --git a/ship/sections/documentation.md.tmpl b/ship/sections/documentation.md.tmpl index 81fe4952d..b1e9684e6 100644 --- a/ship/sections/documentation.md.tmpl +++ b/ship/sections/documentation.md.tmpl @@ -8,8 +8,8 @@ or inline takeover, including failed launches; inline work follows the same validation gates. A stale snapshot is neither a new attempt nor a current audit. Save the child handle. An exited child with missing output is stopped, but its audit is blocked. -**Entry:** First entry always launches the initial audit. -On reentry, reuse only this invocation's validated audit or named-risk decision whose accepted +**Entry:** First entry runs `rm -f .gstack/tmp/docs-audit.json` (an earlier run's +record would pass as this run's audit) and launches the initial audit. On reentry, reuse only this invocation's validated audit or named-risk decision whose accepted base/input hashes still match; retain its actual status and scope. Otherwise use Blocked recovery, not an unconditional launch. Reentry never resets the count or authorizes a launch. @@ -84,9 +84,12 @@ A failed check or `blocked` result goes to recovery, even with valid JSON. Otherwise run the doc caps the base policy declares, before the verification gate: ```bash -~/.claude/skills/gstack/bin/gstack-ship-receipt docs-caps --policy .gstack/ship-policy.json +~/.claude/skills/gstack/bin/gstack-ship-receipt docs-caps --base ``` +It reads `docs.caps` from `origin/`'s `.gstack/ship-policy.json`, never the +branch's copy, so a PR cannot raise its own cap. + It prints one `DOCS_CAP: / ok|over` line per `docs.caps` entry (`DOCS_CAP: none declared` without any) and exits 1 on any `over` (an absent file counts): that is a blocked audit whose repair is trimming the named file, diff --git a/ship/sections/review-army.md b/ship/sections/review-army.md index dbe534f23..df0d0dec1 100644 --- a/ship/sections/review-army.md +++ b/ship/sections/review-army.md @@ -15,12 +15,12 @@ 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`): the core read in item 2 becomes `git diff $RANGE_BASE`, and every dispatched prompt (specialists, -Red Team, the Step 11 native and outside reviewers) runs -`DIFF_BASE=${RANGE_BASE:-$(git merge-base origin/ HEAD)}`, so with `RANGE_BASE` -exported from pass 2 on every reviewer reads what changed since the previous pass -and an outside backend receives the range `RANGE_BASE..HEAD`, never the whole branch -again. Unset `RANGE_BASE` on pass 1 and under `review_scope all`, where every pass -reads the full diff. The gate scan (2.5) and the QA probes keep the whole candidate: +Red Team, the Step 11 reviewers) carries a literal sha you write in: +`DIFF_BASE=`, the merge base on pass 1 and `RANGE_BASE` from pass 2 on. +A reviewer never sees your shell's `RANGE_BASE`; a variable left in a prompt +re-reads the whole branch. An outside backend receives the range `RANGE_BASE..HEAD`. +Under `review_scope all` every pass reads the full diff. 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. ## Confidence Calibration @@ -382,7 +382,7 @@ Every other specialist, including Testing, reads the full diff with fixtures. 5. Instructions: "You are a specialist code reviewer. Read the checklist at {checklist path}, then run -`DIFF_BASE=${RANGE_BASE:-$(git merge-base origin/ HEAD)} && git diff "$DIFF_BASE"` to get the diff (the whole branch on pass 1; from pass 2 on, RANGE_BASE is set and the diff is the range since the previous pass). Apply the checklist against the diff. +`DIFF_BASE= && git diff "$DIFF_BASE"` to get the diff (``: write the sha in before dispatch, the merge base on pass 1 and `RANGE_BASE` from pass 2 on; never a shell variable). Apply the checklist against the diff. `NO FINDINGS` alone is the expected, complete result for a clean diff; never invent an item to show work. Otherwise output one JSON object per finding, on its own line: {\"severity\":\"CRITICAL|INFORMATIONAL\",\"confidence\":N,\"path\":\"file\",\"line\":N,\"category\":\"category\",\"summary\":\"description\",\"fix\":\"recommended fix\",\"fingerprint\":\"path:line:category\",\"specialist\":\"name\"} @@ -553,7 +553,7 @@ The Red Team subagent receives: Prompt, after the defensive framing and fixture handling from item 4 of the specialist dispatch: "You are a red team reviewer. The code has already been reviewed by N specialists who found the following issues: {merged findings summary}. Your job is to find what they -MISSED. Read the checklist at {red-team checklist path}, list changed files with `DIFF_BASE=${RANGE_BASE:-$(git merge-base origin/ HEAD)} && git diff --name-status "$DIFF_BASE"`, read them as that fixture handling says, and look for gaps. +MISSED. Read the checklist at {red-team checklist path}, list changed files with `DIFF_BASE= && git diff --name-status "$DIFF_BASE"`, read them as that fixture handling says, and look for gaps. Output findings as JSON objects (same schema as the specialists), or `NO FINDINGS` alone, the expected result when they missed nothing. Focus on cross-cutting concerns, integration boundary issues, and failure modes that specialist checklists don't cover." @@ -722,8 +722,8 @@ but missing dispatched output still blocks continuation, even with a QA exceptio Step 19's PR body. Deferred and noted findings are never edited. **Late defers from Step 11.** When Step 11's finish rules run this item for a queued adversarial finding after this pass's items 5–6 were logged, append one - row so the receipt's summary counts the deferral (it is not a pass): - `~/.claude/skills/gstack/bin/gstack-review-log '{"skill":"review","via":"ship","phase":"late-defer","timestamp":"TIMESTAMP","status":"issues_found","issues_found":0,"critical":0,"informational":0,"pass":PASS,"review_scope":"SCOPE","findings":FINDINGS_JSON}'` + row under its own skill (a tokenless `review` row would read UNVERIFIED): + `~/.claude/skills/gstack/bin/gstack-review-log '{"skill":"review-late-defer","via":"ship","timestamp":"TIMESTAMP","status":"issues_found","issues_found":0,"critical":0,"informational":0,"pass":PASS,"review_scope":"SCOPE","findings":FINDINGS_JSON}'` with `findings` holding only the deferred records (`action: "deferred"`, `issue`, `locus`, `kind`, `blocking` as in item 6) and no `--finish` token. diff --git a/ship/sections/review-army.md.tmpl b/ship/sections/review-army.md.tmpl index 2439cb15c..1daa1035c 100644 --- a/ship/sections/review-army.md.tmpl +++ b/ship/sections/review-army.md.tmpl @@ -13,12 +13,12 @@ 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`): the core read in item 2 becomes `git diff $RANGE_BASE`, and every dispatched prompt (specialists, -Red Team, the Step 11 native and outside reviewers) runs -`DIFF_BASE=${RANGE_BASE:-$(git merge-base origin/ HEAD)}`, so with `RANGE_BASE` -exported from pass 2 on every reviewer reads what changed since the previous pass -and an outside backend receives the range `RANGE_BASE..HEAD`, never the whole branch -again. Unset `RANGE_BASE` on pass 1 and under `review_scope all`, where every pass -reads the full diff. The gate scan (2.5) and the QA probes keep the whole candidate: +Red Team, the Step 11 reviewers) carries a literal sha you write in: +`DIFF_BASE=`, the merge base on pass 1 and `RANGE_BASE` from pass 2 on. +A reviewer never sees your shell's `RANGE_BASE`; a variable left in a prompt +re-reads the whole branch. An outside backend receives the range `RANGE_BASE..HEAD`. +Under `review_scope all` every pass reads the full diff. 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. {{CONFIDENCE_CALIBRATION}} @@ -138,8 +138,8 @@ but missing dispatched output still blocks continuation, even with a QA exceptio Step 19's PR body. Deferred and noted findings are never edited. **Late defers from Step 11.** When Step 11's finish rules run this item for a queued adversarial finding after this pass's items 5–6 were logged, append one - row so the receipt's summary counts the deferral (it is not a pass): - `~/.claude/skills/gstack/bin/gstack-review-log '{"skill":"review","via":"ship","phase":"late-defer","timestamp":"TIMESTAMP","status":"issues_found","issues_found":0,"critical":0,"informational":0,"pass":PASS,"review_scope":"SCOPE","findings":FINDINGS_JSON}'` + row under its own skill (a tokenless `review` row would read UNVERIFIED): + `~/.claude/skills/gstack/bin/gstack-review-log '{"skill":"review-late-defer","via":"ship","timestamp":"TIMESTAMP","status":"issues_found","issues_found":0,"critical":0,"informational":0,"pass":PASS,"review_scope":"SCOPE","findings":FINDINGS_JSON}'` with `findings` holding only the deferred records (`action: "deferred"`, `issue`, `locus`, `kind`, `blocking` as in item 6) and no `--finish` token. diff --git a/test/gstack-coauthor-trailers.test.ts b/test/gstack-coauthor-trailers.test.ts index 3c08d1cdd..419b9e71b 100644 --- a/test/gstack-coauthor-trailers.test.ts +++ b/test/gstack-coauthor-trailers.test.ts @@ -11,6 +11,7 @@ import * as os from 'node:os'; import * as path from 'node:path'; import { spawnSync } from 'node:child_process'; import { EXIT } from '../lib/headless-artifacts'; +import { runScriptSync } from '../lib/script-invocation'; const BIN = path.resolve(import.meta.dir, '../bin/gstack-coauthor-trailers'); const temps: string[] = []; @@ -52,7 +53,7 @@ function repo() { git(dir, ['checkout', '-q', 'port/contrib']); return dir; } -const run = (cwd: string, args: string[]) => spawnSync(BIN, args, { cwd, env: ENV, encoding: 'utf8', timeout: 60_000 }); +const run = (cwd: string, args: string[]) => runScriptSync(BIN, args, { cwd, env: ENV, encoding: 'utf8', timeout: 60_000 }); test('the default range is origin/HEAD..HEAD; authors print once each in first-appearance order without the porter', () => { const dir = repo(); @@ -79,3 +80,22 @@ test('--message appends only the missing trailers and is idempotent; a bad range expect(run(dir, ['nope..HEAD']).status).toBe(EXIT.fail); expect(run(dir, ['--bogus']).status).toBe(EXIT.usage); }); + +test('--base takes a bare branch, an origin ref or a sha; a slash-named default branch still resolves through origin/; a leading dash is refused', () => { + const dir = repo(); + const sha = git(dir, ['rev-parse', 'origin/main']); + for (const base of ['main', 'origin/main', sha]) { + const r = run(dir, ['--base', base]); + expect(r.status, base).toBe(EXIT.ok); + expect(r.stdout, base).toBe('Co-authored-by: Ada Contributor \nCo-authored-by: Bo Helper \n'); + } + expect(run(dir, ['--base', 'main']).stderr.trim()).toBe('COAUTHORS: 2 in origin/main..HEAD'); + git(dir, ['update-ref', 'refs/remotes/origin/release/1.2', 'refs/remotes/origin/main']); + git(dir, ['symbolic-ref', 'refs/remotes/origin/HEAD', 'refs/remotes/origin/release/1.2']); + const slash = run(dir, []); + expect(slash.status).toBe(EXIT.ok); + expect(slash.stderr.trim()).toBe('COAUTHORS: 2 in origin/release/1.2..HEAD'); + const dash = run(dir, ['--base', '--output=/dev/null']); + expect(dash.status).toBe(EXIT.usage); + expect(dash.stderr).toContain('cannot start with -'); +}); diff --git a/test/gstack-ship-policy.test.ts b/test/gstack-ship-policy.test.ts index 83297b770..30a0a3724 100644 --- a/test/gstack-ship-policy.test.ts +++ b/test/gstack-ship-policy.test.ts @@ -7,7 +7,7 @@ import { afterEach, describe, expect, test } from 'bun:test'; import * as fs from 'node:fs'; import * as path from 'node:path'; import { EXIT } from '../lib/headless-artifacts'; -import { DEFAULT_POLICY, POLICY_KEYS, globToRegExp, matchesAny, validatePolicy } from '../lib/ship-policy'; +import { DEFAULT_POLICY, POLICY_KEYS, REVIEW_SCOPE_KEY_VERSION, globToRegExp, matchesAny, validatePolicy } from '../lib/ship-policy'; import { fillDue } from '../lib/ship-queue-train'; import { BASE_POLICY, cleanup, makeFixtureRepo, runBin, type FixtureRepo } from './helpers/restamp-fixture'; @@ -32,8 +32,10 @@ describe('validatePolicy (schema + containment, pure)', () => { expect(codes({ pr_count: { default_pr_count: 0, split_requires_owner: 'yes' } })).toEqual(['POLICY_INVALID:pr_count.default_pr_count', 'POLICY_INVALID:pr_count.split_requires_owner']); expect(codes({ review_scope: 'loose' })).toEqual(['POLICY_INVALID:review_scope', 'POLICY_INVALID:review_scope']); expect(codes({ review_scope: 'all' })).toEqual(['POLICY_INVALID:review_scope']); - expect(codes({ review_scope: 'all', min_gstack_version: '1.91.249.0' })).toEqual([]); - expect(codes({ review_scope: 'diff-plus-blocking', min_gstack_version: '1.91.249.0' })).toEqual([]); + expect(codes({ review_scope: 'all', min_gstack_version: REVIEW_SCOPE_KEY_VERSION })).toEqual([]); + expect(codes({ review_scope: 'diff-plus-blocking', min_gstack_version: REVIEW_SCOPE_KEY_VERSION })).toEqual([]); + // The constant is the release that first read the key (1.91.256.0, #3481); an earlier minimum would admit a gstack that fails on the unknown key. + expect(codes({ review_scope: 'all', min_gstack_version: '1.91.249.0' })).toEqual(['POLICY_INVALID:review_scope']); expect(codes({ review_scope: 'frozen' })).toEqual([]); expect(codes([])).toEqual(['POLICY_INVALID:']); }); diff --git a/test/gstack-ship-receipt-docs.test.ts b/test/gstack-ship-receipt-docs.test.ts index 120a6ce58..459ab02ff 100644 --- a/test/gstack-ship-receipt-docs.test.ts +++ b/test/gstack-ship-receipt-docs.test.ts @@ -11,9 +11,9 @@ import * as os from 'node:os'; import * as path from 'node:path'; import { EXIT } from '../lib/headless-artifacts'; import { CARRY_NAMED_PATHS, extractReceipt, planCarry, stampOnlyText } from '../lib/ship-receipt'; -import { checkDocsCaps, docsCapLine, docsExceptionLine, parseReceiptDocs, receiptDocs } from '../lib/ship-receipt-docs'; +import { checkDocsCaps, docsCapLine, docsExceptionLine, parseReceiptDocs } from '../lib/ship-receipt-docs'; import { DOCS_CAPS_KEY_VERSION, validatePolicy } from '../lib/ship-policy'; -import { cleanup, makeFixtureRepo, makePr, runBin, type FixtureRepo } from './helpers/restamp-fixture'; +import { BASE_POLICY, cleanup, makeFixtureRepo, makePr, runBin, type FixtureRepo } from './helpers/restamp-fixture'; import { gitArgvIn } from './helpers/scratch-repo'; const repos: FixtureRepo[] = []; @@ -64,6 +64,29 @@ describe('E18: a repository without a version source or CHANGELOG', () => { expect(only.status).toBe(EXIT.ok); expect('version' in JSON.parse(only.out)).toBe(false); }); + + test('"none" means absent: a package.json version is used, and a malformed VERSION, a policy stamp file or an unreadable base keep the no-version path', () => { + const version = (repo: FixtureRepo, args: string[] = []) => JSON.parse(runBin(repo, 'gstack-ship-receipt', ['write', '--base', 'main', '--session-kind', 'x', '--json', ...args]).out).version; + const manifest = bareRepo(); + fs.writeFileSync(path.join(manifest.work, 'package.json'), JSON.stringify({ name: 'x', version: '4.5.6' }) + '\n'); + manifest.git('add', '-A'); + manifest.git('commit', '-qm', 'manifest only'); + expect(version(manifest)).toBe('4.5.6'); + const malformed = bareRepo(); + fs.writeFileSync(path.join(malformed.work, 'VERSION'), 'not-a-version\n'); + malformed.git('add', '-A'); + malformed.git('commit', '-qm', 'malformed VERSION'); + expect(version(malformed)).toBeUndefined(); + const stamped = fixture({ policy: { ...BASE_POLICY, stamp_files: ['RELEASE'] } }); + fs.writeFileSync(path.join(stamped.work, 'RELEASE'), 'build 7\n'); + for (const f of ['VERSION', 'package.json', 'package-lock.json', 'CHANGELOG.md']) fs.rmSync(path.join(stamped.work, f)); + stamped.git('add', '-A'); + stamped.git('commit', '-qm', 'only a policy stamp file'); + expect(version(stamped)).toBeUndefined(); + const unreachable = bareRepo(); + unreachable.git('update-ref', '-d', 'refs/remotes/origin/main'); + expect(version(unreachable)).toBeUndefined(); + }); }); describe('E11: the documentation audit record and the doc caps', () => { @@ -79,8 +102,6 @@ describe('E11: the documentation audit record and the doc caps', () => { expect(full).toEqual({ ok: true, docs: { status: 'blocked', scope: 'docs/, README.md', exception: { accepted_by: 'owner', date: '2026-10-26', via: 'PR #12 comment', reason: 'the removed page is republished next release' } } }); expect(docsExceptionLine((full as any).docs)).toBe('Documentation exception: accepted by owner 2026-10-26 via PR #12 comment: the removed page is republished next release'); expect(docsExceptionLine({ status: 'current', scope: 'x' })).toBeNull(); - expect(receiptDocs({ docs: { status: 'updated', scope: 'docs/' } })).toEqual({ status: 'updated', scope: 'docs/' }); - expect(receiptDocs({})).toBeUndefined(); }); test('write --docs refuses a blocked audit with no exception and a body without the exception line, and records the exception otherwise', () => { @@ -143,6 +164,24 @@ describe('E11: the documentation audit record and the doc caps', () => { expect(runBin(repo, 'gstack-ship-receipt', ['docs-caps', '--policy', path.join(repo.root, 'nope.json')]).status).toBe(EXIT.usage); }); + test('docs-caps reads the caps from the base policy, so a branch that raises its own cap in the working tree still reports over', () => { + const repo = fixture({ policy: { ...BASE_POLICY, docs: { caps: [{ path: 'CHANGELOG.md', max_bytes: 16 }] }, min_gstack_version: DOCS_CAPS_KEY_VERSION } }); + const raised = JSON.stringify({ ...BASE_POLICY, docs: { caps: [{ path: 'CHANGELOG.md', max_bytes: 1_000_000 }] }, min_gstack_version: DOCS_CAPS_KEY_VERSION }, null, 2) + '\n'; + fs.writeFileSync(path.join(repo.work, '.gstack', 'ship-policy.json'), raised); + repo.git('add', '-A'); + repo.git('commit', '-qm', 'raise my own cap'); + const r = runBin(repo, 'gstack-ship-receipt', ['docs-caps', '--base', 'main']); + expect(r.status).toBe(EXIT.fail); + expect(r.out.trim()).toBe(`DOCS_CAP: CHANGELOG.md ${fs.statSync(path.join(repo.work, 'CHANGELOG.md')).size}/16 over`); + expect(r.err).toContain('DOCS_AUDIT_BLOCKED'); + const explicit = runBin(repo, 'gstack-ship-receipt', ['docs-caps', '--policy', path.join(repo.work, '.gstack', 'ship-policy.json')]); + expect(explicit.status).toBe(EXIT.ok); + repo.git('update-ref', '-d', 'refs/remotes/origin/main'); + const gone = runBin(repo, 'gstack-ship-receipt', ['docs-caps', '--base', 'main']); + expect(gone.status).toBe(EXIT.refused); + expect(gone.err).toContain('POLICY_SOURCE_UNAVAILABLE'); + }); + test('the policy docs key validates its shape and needs min_gstack_version', () => { const codes = (raw: Record) => validatePolicy(raw, { gstackVersion: '9.9.9.9' }).map(e => `${e.code}:${e.path}`); expect(codes({ docs: null })).toEqual([]); @@ -195,4 +234,29 @@ describe('carry follow-ups: git floor, named refusal, branch-touched walk, digit expect(changed.err).toContain('RECEIPT_CARRY_PRODUCT_CHANGED'); expect(changed.err).toContain('src.ts'); }); + + test('the carry walk names the branch product path and not a product path a base merge brought in', () => { + const repo = fixture(); + makePr(repo, 'pr-walk', { expects: '1.2.4.0' }); + const body = path.join(repo.root, 'body.md'); + fs.writeFileSync(body, '## Summary\n'); + expect(runBin(repo, 'gstack-ship-receipt', ['write', '--pr', '4', '--base', 'main', '--session-kind', 'x', '--body', body]).status).toBe(EXIT.ok); + repo.git('checkout', '-q', 'main'); + fs.writeFileSync(path.join(repo.work, 'lib-base-only.ts'), 'export const base = 1;\n'); + repo.git('add', 'lib-base-only.ts'); + repo.git('commit', '-qm', 'base change'); + repo.git('push', '-q', 'origin', 'main'); + repo.git('checkout', '-q', 'pr-walk'); + repo.git('merge', '-q', '--no-edit', 'origin/main'); + const merged = runBin(repo, 'gstack-ship-receipt', ['carry', '--body', body, '--base', 'main']); + expect(merged.status).toBe(EXIT.ok); + expect(merged.out).toContain('CARRY: carried'); + fs.writeFileSync(path.join(repo.work, 'src.ts'), 'export const x = 2;\n'); + repo.git('add', 'src.ts'); + repo.git('commit', '-qm', 'product change after the merge'); + const changed = runBin(repo, 'gstack-ship-receipt', ['carry', '--body', body, '--base', 'main']); + expect(changed.status).toBe(EXIT.refused); + expect(changed.err).toMatch(/product diff changed between [0-9a-f]{12} and [0-9a-f]{12}: src\.ts/); + expect(changed.err).not.toContain('lib-base-only.ts'); + }); }); diff --git a/test/helpers/ship-frozen-scope-actor.ts b/test/helpers/ship-frozen-scope-actor.ts index 542b8aed4..85c6dca67 100644 --- a/test/helpers/ship-frozen-scope-actor.ts +++ b/test/helpers/ship-frozen-scope-actor.ts @@ -165,7 +165,7 @@ export function createShipFrozenScopeFixture(workflow: string, root = fs.mkdtemp Supported interactions, declared before launch: - Read: ${workflowPath}, ${invoice}, ${payments}, and any file under ${tmp}/ once it exists. -- Edit or Write: ${invoice} only, and only to apply a fix for a finding the classified output scopes \`fix\`. ${payments} is never written in this fixture. Write also accepts ${draft}, the item-6 review record of the current pass: the JSON object the gstack-review-log example takes (skill, timestamp, status, issues_found, critical, informational, quality_score 10.0, specialists {}, findings with fingerprint, severity, action and the copied locus, kind and blocking, commit, via "ship", completed, converged, cycles, pass, review_scope, range_base). +- Edit or Write: ${invoice} only, and only to apply a fix for a finding the classified output scopes \`fix\`. ${payments} is never written in this fixture. Write also accepts ${draft}, the item-6 review record of the current pass: the JSON object the gstack-review-log example takes (skill, timestamp, status, issues_found, critical, informational, quality_score 10.0, specialists {}, findings with fingerprint, severity, action and the copied locus, kind and blocking, commit, via "ship", completed, converged, cycles, pass, review_scope, range_base). The record's commit is the short HEAD: ${head.slice(0, 7)} now, and the sha the commit adapter prints after it commits a fix; no git command is needed for it. - Bash accepts exactly these single commands, without chaining, redirection or backgrounding. Each is the bound adapter for the generated helper command it names and does not decide which item or decision applies: ${commands.classify} runs gstack-review-scope classify for the current pass against this pass's .gstack/tmp/review-findings-p$PASS.json (--range-base is bound from pass 2 on); it writes .gstack/tmp/review-classified-p$PASS.json and prints the REVIEW_SCOPE line and the classified JSON. ${commands.defer} runs gstack-review-scope defer on the current pass's classified file; it writes .gstack/tmp/review-deferred.json and prints the DEFERRED lines and the rendered PR-body sections (gh is absent here, so a deferred finding stays unfiled with its reason). @@ -198,7 +198,8 @@ export async function shipFrozenScopeFailures(fixture: ReturnType { if (!ok) failures.push(message); }; check(result.exitReason === 'success', `actor ended: ${result.exitReason}`); - check(evidence.refusals.length === 0 && !evidence.executions.some(event => !event.allowed), `undeclared interaction: ${evidence.refusals.join('; ')}`); + const denied = evidence.executions.filter(event => !event.allowed).map(event => `${event.tool} ${JSON.stringify(event.input).slice(0, 160)}`); + check(evidence.refusals.length === 0 && denied.length === 0, `undeclared interaction: ${[...evidence.refusals, ...denied].join('; ')}`); check(evidence.payments === evidence.originalPayments, 'pre-existing payments.ts was edited: the frozen scope fixes only the diff'); try { const url = `${pathToFileURL(fixture.invoice).href}?v=${randomUUID()}`; diff --git a/test/review-fix-range.test.ts b/test/review-fix-range.test.ts index b3b909fd7..c3f6a791a 100644 --- a/test/review-fix-range.test.ts +++ b/test/review-fix-range.test.ts @@ -211,7 +211,8 @@ 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('`DIFF_BASE=${RANGE_BASE:-$(git merge-base origin/ HEAD)}`, so with `RANGE_BASE` exported from pass 2 on every reviewer reads what changed since the previous pass'); + expect(army).toContain('`DIFF_BASE=`, the merge base on pass 1 and `RANGE_BASE` from pass 2 on.'); + expect(army).toContain('A reviewer never sees your shell\'s `RANGE_BASE`; a variable left in a prompt re-reads the whole branch.'); 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-scope-followups.test.ts b/test/review-scope-followups.test.ts index 93eedd873..ec72f31c5 100644 --- a/test/review-scope-followups.test.ts +++ b/test/review-scope-followups.test.ts @@ -10,7 +10,8 @@ import { afterEach, describe, expect, test } from 'bun:test'; import * as fs from 'node:fs'; import * as path from 'node:path'; import { EXIT } from '../lib/headless-artifacts'; -import { LATE_DEFER_PHASE, renderDeferredSection, reviewLoopLine, summarizeReviewLoop } from '../lib/review-scope'; +import { ALREADY_FILED_REASON, LATE_DEFER_SKILL, renderDeferredSection, reviewLoopLine, summarizeReviewLoop } from '../lib/review-scope'; +import { reviewFreshness } from '../lib/review-evidence'; import { cleanup, makeFixtureRepo, runBin, type FixtureRepo } from './helpers/restamp-fixture'; const repos: FixtureRepo[] = []; @@ -18,19 +19,23 @@ const fixture = () => { const r = makeFixtureRepo({ prefix: 'review-scope-2-' }) afterEach(() => { while (repos.length) cleanup(repos.pop()!); }); describe('late defers from Step 11', () => { - test('a review row with phase late-defer adds its deferred findings and is not counted as a pass', () => { + test('a review-late-defer row adds its deferred findings, is not counted as a pass, and is invisible to review freshness', () => { + const late = { skill: LATE_DEFER_SKILL, via: 'ship', timestamp: '2026-10-26T10:30:00Z', pass: 2, review_scope: 'frozen', findings: [ + { fingerprint: 'c:9:z', blocking: true, action: 'deferred', issue: null }, { fingerprint: 'b:1:y', blocking: true, action: 'deferred', issue: 5 }] }; const rows = [ { skill: 'review', via: 'ship', timestamp: '2026-10-26T10:00:00Z', pass: 1, review_scope: 'frozen', findings: [ { fingerprint: 'a:1:x', blocking: true, action: 'auto-fixed' }, { fingerprint: 'b:1:y', blocking: true, action: 'deferred', issue: 5 }] }, { skill: 'review', via: 'ship', timestamp: '2026-10-26T10:20:00Z', pass: 2, review_scope: 'frozen', findings: [] }, - { skill: 'review', via: 'ship', phase: LATE_DEFER_PHASE, timestamp: '2026-10-26T10:30:00Z', pass: 2, review_scope: 'frozen', findings: [ - { fingerprint: 'c:9:z', blocking: true, action: 'deferred', issue: null }, { fingerprint: 'b:1:y', blocking: true, action: 'deferred', issue: 5 }] }, + late, { skill: 'adversarial-review', via: 'ship', timestamp: '2026-10-26T10:25:00Z', findings: [{ fingerprint: 'd:1:w', blocking: true, action: 'deferred' }] }, ]; const s = summarizeReviewLoop(rows, '2026-10-26T09:30:00Z'); expect(s).toEqual({ passes: 2, blocking_found: 3, deferred: 2, noted: 0, fixed: 1, scope: 'frozen' }); expect(reviewLoopLine(s)).toBe('REVIEW_LOOP: passes=2 blocking_found=3 deferred=2 noted=0 fixed=1 scope=frozen'); - expect(LATE_DEFER_PHASE).toBe('late-defer'); + expect(LATE_DEFER_SKILL).toBe('review-late-defer'); + // A `review` row logged without a start token would be the newest review row and read UNVERIFIED; the late-defer row is not a diff review at all. + expect(reviewFreshness(late, 'abc')).toBeUndefined(); + expect(reviewFreshness({ ...late, skill: 'review' }, 'abc')?.status).toBe('UNVERIFIED'); }); }); @@ -68,6 +73,7 @@ describe('deferred-issue search page and the repository argument gh accepts', () expect(records[0]).toMatchObject({ issue: 900, reason: 'filed; the marker search was truncated at 200, so an older issue may duplicate it' }); expect(fs.readFileSync(full.log, 'utf8')).toContain('--limit 200'); expect(renderDeferredSection(records)).toContain('- Deferred: #900 (filed; the marker search was truncated at 200, so an older issue may duplicate it) — CRITICAL `lib/legacy.ts:7` Unbounded delete'); + expect(renderDeferredSection([{ ...records[0], issue: 9, reason: ALREADY_FILED_REASON }])).toContain('- Deferred: #9 — CRITICAL `lib/legacy.ts:7` Unbounded delete'); const repo2 = fixture(); const short = shim(repo2, 'PRIVATE', 199); const r2 = runBin(repo2, 'gstack-review-scope', ['defer', '--findings', classified, '--pr', '42', '--repo', 'acme/app', '--json'], { PATH: `${short.dir}:${process.env.PATH}` }); diff --git a/test/ship-receipt-prose.test.ts b/test/ship-receipt-prose.test.ts index 77858b9ff..f352e5765 100644 --- a/test/ship-receipt-prose.test.ts +++ b/test/ship-receipt-prose.test.ts @@ -38,6 +38,16 @@ describe('checkReceiptProse', () => { expect(proseLine(bad)).toBe('PROSE: mismatch review_loop.passes body=3 receipt=2; review_loop.deferred body=2 receipt=1'); expect(proseLine(checkReceiptProse(receipt, 'PR #7 adds the docs.'))).toBe('PROSE: no receipt counter claims in the body'); }); + + test('a leaf key shared by several counters matches any of them, and a hyphenated word is not a claim', () => { + // /ship writes the gate per lane ({"":{pass,fail,skip}}), so `pass` names every lane and `passed` names gate and ci alike. + const lanes = { ...receipt, gate: { unit: { pass: 41, fail: 0, skip: 2 }, e2e: { pass: 7, fail: 0, skip: 0 } }, ci: { status: 'success', passed: 31, failed: 0 } }; + expect(checkReceiptProse(lanes, 'unit: pass=41, e2e pass: 7; CI passed: 31.')).toEqual({ checked: 3, mismatches: [] }); + expect(checkReceiptProse(lanes, '3 pass-through tests and the fail-fast path; blocking-found: 2.')).toEqual({ checked: 1, mismatches: [] }); + const bad = checkReceiptProse(lanes, 'pass: 40'); + expect(bad.mismatches).toEqual([{ field: 'gate.unit.pass', prose: 40, receipt: 41 }]); + expect(checkReceiptProse(receipt, 'Gate passed: 41. CI passed=41.').mismatches).toEqual([]); + }); }); describe('gstack-ship-receipt read --check-prose', () => { diff --git a/test/ship-workflow-clarity.test.ts b/test/ship-workflow-clarity.test.ts index 1fc42d0d2..33277beba 100644 --- a/test/ship-workflow-clarity.test.ts +++ b/test/ship-workflow-clarity.test.ts @@ -217,7 +217,9 @@ test('a repository without a version source or CHANGELOG ships with one skipped }); test('Step 14.5 runs the doc caps before the verification gate and records the audit for the receipt, with the owner exception shape (E11)', () => { - expectOrdered(docsTemplate, ['### Continue or recover', 'gstack-ship-receipt docs-caps --policy .gstack/ship-policy.json', '`DOCS_CAP: / ok|over`', '.gstack/tmp/docs-audit.json', '## Blocked recovery'], 'ship documentation section'); + // The caps come from origin/'s policy (a branch cannot raise its own cap), and the per-invocation record is cleared before the first launch. + expectOrdered(docsTemplate, ['rm -f .gstack/tmp/docs-audit.json', '### Continue or recover', 'gstack-ship-receipt docs-caps --base ', 'never the\nbranch\'s copy', '`DOCS_CAP: / ok|over`', 'write the receipt\'s record to `.gstack/tmp/docs-audit.json`', '## Blocked recovery'], 'ship documentation section'); + expectAbsent(docsTemplate, ['docs-caps --policy .gstack/ship-policy.json'], 'ship documentation section cap source'); expectTokens(docs, ['`DOCS_CAP: none declared`', '"status":"updated|current"', '"status":"blocked"', '"exception":{"accepted_by"', '`DOCS_AUDIT_BLOCKED`', 'An unattended run has no owner to accept: it stops here.'], 'ship documentation section'); const compose = between(prTemplate, 'Handoff record first', '**Existing open PR/MR**'); expectTokens(compose, ['--docs @.gstack/tmp/docs-audit.json', '`DOCS_AUDIT_BLOCKED`'], 'ship pr-body receipt write'); @@ -226,13 +228,16 @@ test('Step 14.5 runs the doc caps before the verification gate and records the a test('Step 9.4 applies the cap before any edit, names its scratch files under .gstack/tmp/, logs Step 11 late defers, and hands reviewers the range from pass 2 (PR 1 review notes)', () => { const fixFirst = between(reviewTemplate, '## Step 9.4:', '### Decide whether to repeat Step 9'); - expectOrdered(fixFirst, ['0. **Scope the findings.**', '.gstack/tmp/review-findings-p$PASS.json', '.gstack/tmp/review-classified-p$PASS.json', '1. **Apply the cap before any edit, then classify.**', '2. **Auto-fix all AUTO-FIX items.**', '**Late defers from Step 11.**', '"phase":"late-defer"', '6. Persist the review result'], 'ship Step 9.4'); + expectOrdered(fixFirst, ['0. **Scope the findings.**', '.gstack/tmp/review-findings-p$PASS.json', '.gstack/tmp/review-classified-p$PASS.json', '1. **Apply the cap before any edit, then classify.**', '2. **Auto-fix all AUTO-FIX items.**', '**Late defers from Step 11.**', '"skill":"review-late-defer"', '6. Persist the review result'], 'ship Step 9.4'); expectTokens(compact(fixFirst), ['`.gstack/tmp/review-deferred.json`', '`CYCLES >= 3`', '`PASS >= 3`', 'no `--finish` token'], 'ship Step 9.4'); - expectTokens(reviewFlow, ['`DIFF_BASE=${RANGE_BASE:-$(git merge-base origin/ HEAD)}`', 'RANGE_BASE..HEAD'], 'ship Step 9 range'); + // The sha is written into each dispatched prompt: a reviewer's shell never sees the parent's RANGE_BASE, so a variable there would silently widen the read. + expectTokens(reviewFlow, ['`DIFF_BASE=`', 'RANGE_BASE..HEAD', 'never sees your shell\'s `RANGE_BASE`'], 'ship Step 9 range'); + expectAbsent(reviewFlow, ['DIFF_BASE=${RANGE_BASE'], 'ship Step 9 range fallback'); for (const host of ALL_HOST_CONFIGS) { const army = generateReviewArmy({ host: host.name, skillName: 'ship', tmplPath: '', paths: HOST_PATHS[host.name] }); if (!army) continue; - expect(army.match(/DIFF_BASE=\$\{RANGE_BASE:-\$\(git merge-base origin\/ HEAD\)\}/g)?.length, `${host.name} ship review army range`).toBe(2); + expect(army.match(/DIFF_BASE=/g)?.length, `${host.name} ship review army range`).toBe(2); + expectAbsent(army, ['${RANGE_BASE'], `${host.name} ship review army fallback`); expectTokens(army, ['--review-fanout auto|serial|parallel', /Fan-out: N specialists in\s+M batches of K/], `${host.name} ship review army fan-out`); const review = generateReviewArmy({ host: host.name, skillName: 'review', tmplPath: '', paths: HOST_PATHS[host.name] }); expectAbsent(review, ['RANGE_BASE', '--review-fanout'], `${host.name} /review army`);