From 8d712d63ddb883379c371f9c0752854eba97eeb0 Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Thu, 5 Nov 2026 20:33:54 -0800 Subject: [PATCH] ship finish: a declared mechanical version retake after a base merge binds without a new review (GST-1560) (#4053) * test: failing-first founderstar#563 retake shape, refusals and planner agreement at the cap (GST-1560) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * feat(ship): finish routes a declared mechanical retake with no review; --post-review plans the inserted Step 9 (GST-1560) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * docs(ship): mechanical_retake, ROUTE_MECHANICAL_REFUSED and the Step 16 planner line (GST-1560) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * test: keep ship-mechanical-retake.test.ts off the Windows scoped lane (bin shebang spawns) (GST-1560) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * fix: pre-landing review fixes finish dropped a post-review path named like an Object.prototype key (constructor, toString, __proto__) from behavior through 'p in proven' on a plain object; proven is now a null-prototype map read with Object.hasOwn, with a regression test. Review-Finding: v1/outside-voice#1 reader=outside-voice Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * docs: changelog fragment prose lint (GST-1560) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --------- Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- bin/gstack-review-plan | 10 +- bin/gstack-ship-receipt | 6 +- .../capy-gst-1560-mechanical-retake.md | 1 + docs/ship-policy.md | 62 +++++++ docs/troubleshooting.md | 22 +++ lib/headless-artifacts.ts | 3 + lib/ship-mechanical.ts | 161 ++++++++++++++++++ lib/ship-policy-keys.ts | 3 +- lib/ship-policy-mechanical.ts | 63 +++++++ lib/ship-policy.ts | 6 +- lib/ship-receipt-prose.ts | 3 +- lib/ship-receipt.ts | 2 +- lib/ship-release.ts | 24 ++- scripts/lib/windows-curation.ts | 4 + ship/SKILL.md | 25 ++- ship/SKILL.md.tmpl | 25 ++- ship/sections/pr-body.md | 2 +- ship/sections/pr-body.md.tmpl | 2 +- test/fixtures/golden/agy-ship-SKILL.md | 25 ++- test/fixtures/golden/claude-ship-SKILL.md | 25 ++- test/fixtures/golden/codex-ship-SKILL.md | 25 ++- test/fixtures/golden/factory-ship-SKILL.md | 25 ++- test/ship-bounded-route.test.ts | 2 +- test/ship-finish-stack-parent.test.ts | 2 +- test/ship-mechanical-retake.test.ts | 145 ++++++++++++++++ test/ship-rereview-triggers.test.ts | 8 +- 26 files changed, 581 insertions(+), 100 deletions(-) create mode 100644 changelog.d/capy-gst-1560-mechanical-retake.md create mode 100644 lib/ship-mechanical.ts create mode 100644 lib/ship-policy-mechanical.ts create mode 100644 test/ship-mechanical-retake.test.ts diff --git a/bin/gstack-review-plan b/bin/gstack-review-plan index b5c87d80c..71522415a 100755 --- a/bin/gstack-review-plan +++ b/bin/gstack-review-plan @@ -5,7 +5,7 @@ * gstack-review-plan --base [--pass ] [--last none|full|fix-check] [--fixed-blocking] [--fixed-below] * [--blocking-open] [--touched-gate] [--critical-found] [--adversarial-ran] [--gate-tagged ] [--gated a,b] [--force a,b|all] * [--reviewed-numbers ] [--depth-floor standard|full] [--range-base ] [--pass1-depth light|standard|full] [--no-tiers] - * [--targeted-ran ] [--grant ] [--fix-checks ] [--pr ] [--json] + * [--targeted-ran ] [--grant ] [--fix-checks ] [--post-review] [--pr ] [--json] * gstack-review-plan --fix-check-prompt --fix-list --hunks --tests [--callers ] * * Reads the diff against the merge base with (committed, tracked uncommitted @@ -75,6 +75,10 @@ const HELP = `gstack-review-plan — the review depth of the next /ship step (do gate-class edit plans that fix-check instead; a fix-check never earns another; any other row is refused (exit 3) --fix-checks fix-checks already run in this invocation: the owner's grant adds one only after the first, in the lean loop and after a round-2 fix-check in the full loop (GEVA-223) + --post-review plan the Step 9 that /ship Step 16 stage 2.1 inserted for edits after the review bound (finish's + behavior STOP): they plan as a fix to a gate-class path, never a note, so below the round cap + it is a pass on the range (lean: a fix-check), and at the cap it stops for the owner, whose + --grant adds one scoped fix-check; pass --pass, --last and --fix-checks as the loop left them --pr the PR whose platform review counts under review.platform on (default: this branch's PR) --declare-risk risk classes the lane declares for the diff (comma list of ${RISK_CLASSES.join(', ')}): they join the detected classes and only raise the depth; RISK_DECLARED: prints both (GST-1129) @@ -215,11 +219,13 @@ function main(): number { const targeted = targetedClasses({ ...collected, ...(active ? { paths: paths.filter(p => !active.excluded.has(p)), added: kept(added), removed: kept(collected.removed) } : {}), scope, gateTagged }); const patch = collected.mergeBase ? targetedPatchId(process.cwd(), collected.mergeBase) : 'none'; const brief = targetedBriefSha(); + const postReview = has('--post-review'); const plan = planReview({ ...collected, ...exclusionInput, ...riskPaths, declared: declared as RiskClass[], shape: { panel: review.panel, readerModel: review.reader_model, platform: review.platform, head, platformReview: platform?.ok ? platform.review : null }, scope, gateTagged, targeted: { classes: targeted.classes, ran: !['uncommitted', 'none'].includes(patch) && flag('--targeted-ran') === patch }, numbers: scanNumbers(kept(added)), force, gated: list('--gated'), floor, tiers: auditEffort !== 'host', criticalFound: has('--critical-found'), adversarialRan: has('--adversarial-ran'), reviewedNumbers: flag('--reviewed-numbers'), - history: { fullPasses: pass, last, fixedBlocking: has('--fixed-blocking'), fixedBelow: has('--fixed-below') && last !== 'fix-check', blockingOpen: has('--blocking-open'), touchedGate: has('--touched-gate') || !!fixRange?.touched_gate, pass1Depth, rangeRuntimeLines: fixRange?.runtime_lines, granted: grant, fixChecks: has('--fix-checks') ? fixChecks : undefined }, + history: { fullPasses: pass, last, fixedBlocking: has('--fixed-blocking'), fixedBelow: (has('--fixed-below') || (postReview && pass > 0)) && last !== 'fix-check', blockingOpen: has('--blocking-open'), touchedGate: has('--touched-gate') || !!fixRange?.touched_gate || (postReview && pass > 0), pass1Depth, rangeRuntimeLines: fixRange?.runtime_lines, granted: grant, fixChecks: has('--fix-checks') ? fixChecks : undefined }, }); + if (postReview && pass > 0) plan.next.reason = `edits after the review bound (Step 16 stage 2.1) plan as a fix to a gate-class path, never a note: ${plan.next.reason}`; const selection = policySelection(active && { state: 'active', specs: active.specs, keep: active.keep, excerpt: active.excerpt }, readers); plan.dispatches.forEach((d, i) => { if (d.role !== 'fix-check') plan.dispatches[i] = selectFor(d, selection); }); const gateScan = { tagged: gateTagged, head }; diff --git a/bin/gstack-ship-receipt b/bin/gstack-ship-receipt index 04369c030..50c73537b 100755 --- a/bin/gstack-ship-receipt +++ b/bin/gstack-ship-receipt @@ -57,6 +57,7 @@ import { checkReceiptProse, proseLine } from '../lib/ship-receipt-prose'; import { resolveStateRoot } from '../lib/state-root'; import { readPause, receiptBaseRed } from '../lib/base-red'; import { readRuledNotes, ruledNoteText } from '../lib/ruled-notes'; +import { readMechanicalLines } from '../lib/ship-mechanical'; import { isRegistryUnavailable, loadRegistryFromBase } from '../lib/regen'; const HELP = `gstack-ship-receipt — the \`\`\`gstack-ship-receipt\`\`\` block (docs/ship-policy.md#ship-receipt) @@ -264,6 +265,9 @@ function cmdWrite(rest: string[], json: boolean): number { const ruledNotes = readRuledNotes(repoRoot); const unlisted = bodyFile ? ruledNotes.map(n => `Ruled note: ${ruledNoteText(n)}`).find(l => !bodyText.split(/\r?\n/).some(b => b.trim() === l)) : undefined; if (unlisted) { process.stderr.write(resultLine('gstack-ship-receipt write', 'RECEIPT_INVALID', `${bodyFile} does not list ruled note ${JSON.stringify(unlisted)} (gstack-approvals rulings prints each line the body carries); nothing written`) + '\n'); return EXIT.fail; } + const mechanical = readMechanicalLines(repoRoot, git(['rev-parse', `${head}^{tree}`], repoRoot) ?? ''); + const unlistedRetake = bodyFile ? mechanical.find(l => !bodyText.split(/\r?\n/).some(b => b.trim() === l)) : undefined; + if (unlistedRetake) { process.stderr.write(resultLine('gstack-ship-receipt write', 'RECEIPT_INVALID', `${bodyFile} does not list ${JSON.stringify(unlistedRetake)} (finish's MECHANICAL: lines; the body carries each as one line); nothing written`) + '\n'); return EXIT.fail; } const declared = (flag(rest, '--declare-risk') ?? '').split(',').map(s => s.trim()).filter(Boolean); if (rest.includes('--declare-risk') && (!declared.length || declared.some(c => !(RISK_CLASSES as readonly string[]).includes(c)))) { process.stderr.write(resultLine('gstack-ship-receipt write', 'RECEIPT_INVALID', `--declare-risk ${flag(rest, '--declare-risk') ?? ''}: each class is one of ${RISK_CLASSES.join(', ')}; nothing written`) + '\n'); return EXIT.fail; } const review = receiptReviewAtHead(repoRoot, base, path.dirname(Bun.fileURLToPath(import.meta.url)), headSha ?? head, declared as RiskClass[]); @@ -319,7 +323,7 @@ function cmdWrite(rest: string[], json: boolean): number { policy: flag(rest, '--policy') ?? tree?.policy, queue_mode: flag(rest, '--queue-mode'), preregistration: flag(rest, '--preregistration'), history: flag(rest, '--history'), ...pregate, issues: issuesFile ? receiptIssuesFrom(issuesFile) : undefined, approval: approval.kind === 'ok' ? approval.approval : undefined, review_loop: reviewLoop, ...(docs?.ok ? { docs: docs.docs } : {}), ...product, - gstack_version: installedGstackVersion(), review_depth: reviewDepth, review_loop_exception: loopException?.ok ? loopException.exception : undefined, ruled_notes: ruledNotes.length ? ruledNotes : undefined, review_targeted: targeted?.ok ? targeted.targeted : undefined, + gstack_version: installedGstackVersion(), review_depth: reviewDepth, review_loop_exception: loopException?.ok ? loopException.exception : undefined, ruled_notes: ruledNotes.length ? ruledNotes : undefined, mechanical: mechanical.length ? mechanical : undefined, review_targeted: targeted?.ok ? targeted.targeted : undefined, base_red: pause ? receiptBaseRed(pause) : undefined, review_exclude: receiptExclude(review.exclude), no_stop: noStopArg(flag(rest, '--no-stop')), diff --git a/changelog.d/capy-gst-1560-mechanical-retake.md b/changelog.d/capy-gst-1560-mechanical-retake.md new file mode 100644 index 000000000..cd306ab55 --- /dev/null +++ b/changelog.d/capy-gst-1560-mechanical-retake.md @@ -0,0 +1 @@ +- **/ship's finish routes a declared version or rules-number retake after a base merge as mechanical, with no review round.** founderstar#563 merged its base after two full passes and retook its rules number 80 → 81: one line in `scripts/core/game.gd`, `save_version` in 8 save fixtures and one golden. finish routed all 10 paths as behavior and said to insert Steps 5–11.5, while `gstack-review-plan` at the round cap said `done`, so the only exit was a fresh /ship. A base policy can now declare the constant under `mechanical_retake` (its file and one-line template, the fixture fields that carry the value, and the `.gstack/generated.json` entries that rebuild its goldens). finish proves each changed path from the diff. It needs one literal on the constant's line, a value above the reviewed one and the base's, the same new value in every changed field, goldens that regenerate byte-identically, and nothing else changed. It prints `MECHANICAL:` per path and `mechanical=` on `ROUTE:`. Any other edit in those files, a value that doesn't match or a golden that doesn't regenerate keeps its old route with a `ROUTE_MECHANICAL_REFUSED:` reason. The receipt lists the paths as `Mechanical retake: …` lines, and receipt write refuses a PR body missing one. finish's behavior STOP now names `gstack-review-plan --post-review`, which plans that Step 9 as a fix to a gate-class path: a pass on the range below the round cap, and a stop for the owner's grant of one scoped fix-check at the cap, never `done`. Prior art: #3966 (GSTA-167's `carry_mechanical`). (GST-1560) diff --git a/docs/ship-policy.md b/docs/ship-policy.md index 9e2d9b26f..2746ffbf0 100644 --- a/docs/ship-policy.md +++ b/docs/ship-policy.md @@ -69,6 +69,7 @@ refuses the file (`POLICY_GSTACK_TOO_OLD`). `validate` ends with | `land` | `{ merged_checks }` or `null` | `null` | Under `merge: direct`, the shell commands (1 to 16, one line of at most 512 characters each, 15 minutes each) [`land --pin`](#land-direct) runs on the pin merged onto the base tip when the base moved past the pin's merge base; it lands only when every one exits 0. They are repo commands, so land runs them only with `--allow-repo-commands`, never under `--dry-run`. `null` lands a stale base untested with a `STALE_BASE: warn` line. Read from the base revision only. Needs `min_gstack_version`. | | `release_gate` | `{ cadence_hours, gate_workflow, branch, stable_ref, check_name, on_red, bisect_spend_cap_usd }` or `null` | `null` | One full paid gate on main every `cadence_hours` (8 to 168) instead of the per-push main backstop, feeding the stable release; see [release_gate](#release-gate). Every field is required: `on_red` is `hold-release`, `bisect_spend_cap_usd` 0 to 500 (0 skips the bisect). `null` keeps the per-push backstop. Needs `min_gstack_version`. | | `bookkeeping` | glob list | `[]` | Root-anchored globs of bookkeeping tables a verify or ratchet step rewrites (size baselines, serial lists, limits), each ending in `.tsv`, `.csv` or `.txt` and never under `.gstack/` or `.github/` (`["scripts/*-baseline.tsv"]`). A matching file changed at most 20 lines after review binds, first touched or not, takes finish's [one scoped reader](review-policy.md#bounded-post-review-edits) instead of Steps 5–11.5; the reader still reads every changed row. Read from the base revision only. Needs `min_gstack_version`. | +| `mechanical_retake` | `[{ path, template, fields, regen }]` | `[]` | A version or rules constant a branch may have to retake after a base merge, next to `bookkeeping`'s bounded kind: `path` is one literal file, `template` the whole line with one `{n}` (`const RULE_NUMBERS := {n}`), `fields` the `{ glob, field }` fixture fields that carry the same value, and `regen` the [`.gstack/generated.json`](pregate.md#generated) entry ids whose command regenerates its goldens. A post-review edit finish proves to be only that retake routes as `mechanical=`, with no reader and no review round; see [mechanical retakes](#mechanical-retake). At most 8 constants and 16 fields or ids each; no glob, `..`, `.gstack/` or `.github/` path. Read from the base revision only. Needs `min_gstack_version`. | | `base_red` | `{ setup, select, timeout_sec }` or `null` | `null` | How `/ship` Step 5 proves a failure is the base branch's own before pausing on it; see [base_red](#base-red). Read from the base revision only. Needs `min_gstack_version`. | | `changelog_fragments` | `{ dir, required_checks? }` or `null` | `null` | PRs carry unversioned [CHANGELOG fragments](#changelog-fragments) in `dir` (one flat, repo-relative directory, no `..`) and the merge step ([`land`](#land)) assigns the version (under `merge: direct`, the [release PR](#release-stamp) does). `required_checks` (up to 50 distinct names) are the checks [`train land`](#stamped-trains-under-changelog_fragments-train-land) requires present and green on a stamp commit. `null` keeps `[Unreleased]` sections and per-PR stamps. Needs `min_gstack_version`. | @@ -1717,6 +1718,67 @@ merged with the new base in a scratch worktree and, when every lane passes, rewrites the receipt without `base_red`. + +## `mechanical_retake`: a version or rules number retaken after a base merge + +Some repositories number things in one constant: a save version, a rules +number, a schema version. Two branches can take the same next number. When +the base lands the other one first, the branch merges the base and takes the +number after it: one literal on the constant's line, the same value in every +save fixture, and goldens rebuilt from the generator. founderstar#563 did +this after two full review passes (rules number 80 to 81 in +`scripts/core/game.gd`, `save_version` in 8 fixtures under `fixtures/`, one +golden). `finish` routed all 10 paths as behavior and said to insert Steps +5–11.5, while `gstack-review-plan` at the round cap said `done`, so the only +way out was a fresh /ship (GST-1560). + +A base policy that declares the constant lets `finish` prove the retake +instead: + +```json +"mechanical_retake": [{ + "path": "scripts/core/game.gd", + "template": "const RULE_NUMBERS := {n}", + "fields": [{ "glob": "fixtures/saves/*.json", "field": "save_version" }], + "regen": ["rules-golden"] +}] +``` + +`finish` runs with `--reviewed-base` and checks each declared path that +changed since review: + +| Path | Proof | +|---|---| +| the constant (`path`) | Exactly one line matches `template` at the reviewed base, the reviewed tree, the current base and now. The reviewed branch took the number (its value differs from the reviewed base's), and the current value is above both the reviewed value and the current base's. With that one line masked on every side, the file's patch is the reviewed patch (the same comparison base paths use). | +| a fixture (`fields`) | A matching file holds the same number of `field` lines (`field: n`, `field = n` or `"field": n,`) at the reviewed tree and now. Each one either kept its value or moved from the constant's reviewed value to its current value. With those lines masked, its patch is the reviewed patch. | +| a golden (`regen`) | An output of a named entry in the base's `.gstack/generated.json` that the entry's command recreates byte-identically in a scratch worktree (the pregate `regen` check's proof run). | + +Each proven path prints `MECHANICAL: ()` and counts as +`mechanical=` on `ROUTE:`. It needs no reader and no review round, and +the docs audit carries it like a base path. A declared path that fails its +proof keeps its route from before, and `finish` prints +`ROUTE_MECHANICAL_REFUSED: ` +([troubleshooting](troubleshooting.md#route-mechanical-refused)). That covers +any other edit in the constant's file, a fixture value that doesn't match, a +golden that doesn't regenerate, a constant the reviewed branch never took, a +gate path and a landed stack parent. A fixture or golden whose constant isn't a +proven retake is refused too. Nothing is inferred: an undeclared file never +routes as mechanical. + +`finish` records the proven paths for the tree it routed, and +`gstack-ship-receipt write` copies them into the receipt's `mechanical` list +as `Mechanical retake: ()`. With `--body` it refuses a PR body +that doesn't carry each of those lines word for word. + +**When the route still says behavior at the round cap.** `finish`'s behavior +STOP tells the lane to plan the inserted Step 9 with `gstack-review-plan +--post-review` (plus `--pass`, `--last` and `--fix-checks` as the loop left +them). Edits after the review bound plan as a fix to a gate-class path, never +as a note. Below the round cap that's a pass on the range (a fix-check under +the lean panel). At the cap it stops for the owner, whose recorded grant +(`--grant`) adds one scoped fix-check, never `done`. So `finish` never asks for +a step the planner refuses. + ## Carrying a receipt across a base merge or a test-only fix **Merge main into a reviewed branch; never rebase it.** What happens to the diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index 60dc35525..7ccc0bfa2 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -3166,6 +3166,28 @@ lane change removed (a `[]` declaration lands on the base first). Or take Steps 5–11.5 for the edit. Never shrink an edit to get under the cap by moving its lines into another file. + +### `ROUTE_MECHANICAL_REFUSED: ` + +**Meaning.** `gstack-ship-release finish` found a post-review edit to a path +the base policy's `mechanical_retake` declares (the constant's file, a fixture +field or a golden) that failed its proof, so the path keeps its old route, +usually behavior ([ship-policy.md#mechanical-retake](ship-policy.md#mechanical-retake)). +The reason names the failed proof: `needs --reviewed-base`, `is a gate path +or under .gstack/ or .github/`, `does not hold exactly one line matching +