From ee5e2c37dc1333d9062cba45f883a5ac5b5e739c Mon Sep 17 00:00:00 2001 From: garrytan Date: Sat, 31 Oct 2026 03:46:15 +0000 Subject: [PATCH] fix: pre-landing review fixes (the answer gate names every flag that decides the write; Version Gate and validate understand bump never) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- bin/gstack-ship-policy | 6 +++--- docs/ship-policy.md | 4 ++-- lib/ship-release.ts | 9 ++++++--- lib/version-gate-train.ts | 8 ++++++-- scripts/compare-pr-version.ts | 4 ++++ test/gstack-ship-policy.test.ts | 9 +++++++++ test/ship-release.test.ts | 13 ++++++++++--- test/version-gate-fragments.test.ts | 17 +++++++++++++++++ 8 files changed, 57 insertions(+), 13 deletions(-) diff --git a/bin/gstack-ship-policy b/bin/gstack-ship-policy index 7437dfd43..f01db4679 100755 --- a/bin/gstack-ship-policy +++ b/bin/gstack-ship-policy @@ -18,7 +18,7 @@ import * as path from 'node:path'; import { EXIT, exitTableHelp } from '../lib/headless-artifacts'; import { resultLine } from '../lib/result-codes'; import { - POLICY_KEYS, POLICY_KEY_INTRODUCED, SHIP_POLICY_PATH, examplePolicyJson, explainPolicy, isUnavailable, loadPolicyFromBase, loadPolicyFromHead, + BUMP_NEVER_KEY_VERSION, POLICY_KEYS, POLICY_KEY_INTRODUCED, SHIP_POLICY_PATH, examplePolicyJson, explainPolicy, isUnavailable, loadPolicyFromBase, loadPolicyFromHead, minimumReader, policyUnavailableLine, renderEffective, resolveBaseBranch, type LoadedPolicy, } from '../lib/ship-policy'; @@ -87,8 +87,8 @@ function main(argv: string[]): number { const loaded = load(root, rest, cmd === 'validate' ? 'head' : 'base'); if (typeof loaded === 'number') return loaded; const set = POLICY_KEYS.filter(k => loaded.values[k] && loaded.values[k] !== 'default'); - const introduced = Object.fromEntries(set.map(k => [k, POLICY_KEY_INTRODUCED[k]])); - const minReader = set.length ? minimumReader(introduced) : null; + const introduced = Object.fromEntries(set.map(k => [k, k === 'bump' && loaded.policy.bump === 'never' ? BUMP_NEVER_KEY_VERSION : POLICY_KEY_INTRODUCED[k]])); + const minReader = set.length ? minimumReader(Object.fromEntries(set.map(k => [k, loaded.policy[k]]))) : null; if (json) { process.stdout.write(JSON.stringify({ valid: loaded.errors.length === 0, present: loaded.present, source: loaded.source, policy: loaded.policy, values: loaded.values, introduced, min_reader: minReader, errors: loaded.errors }) + '\n'); return loaded.errors.length ? EXIT.fail : EXIT.ok; diff --git a/docs/ship-policy.md b/docs/ship-policy.md index c7b3f3f83..7ffaf1414 100644 --- a/docs/ship-policy.md +++ b/docs/ship-policy.md @@ -39,7 +39,7 @@ refuses the file (`POLICY_GSTACK_TOO_OLD`). `validate` ends with | Key | Values | Default | Meaning | |---|---|---|---| -| `bump` | `patch` \| `ask` \| `never` | `ask` | `patch` skips the MINOR/MAJOR prompt in `/ship` Step 12 and `gstack-restamp --next` bumps PATCH; `ask` keeps the prompt, and in a session no person answers `--bump` stops with `BUMP_ANSWER_REQUIRED` until `--answer ""`; `never` is for a repo whose unit PRs never change VERSION: Step 12 keeps it with no question, refuses `--bump`, and `gstack-restamp --next` needs an explicit `--bump` or `--version` (the release process bumps). `never` needs `min_gstack_version` at its introducing release and cannot be combined with `queue_mode: stamp-at-merge`, `train` or `changelog_fragments`, which stamp a version at every merge. | +| `bump` | `patch` \| `ask` \| `never` | `ask` | `patch` skips the MINOR/MAJOR prompt in `/ship` Step 12 and `gstack-restamp --next` bumps PATCH; `ask` keeps the prompt, and in a session no person answers any version Step 12 would write (`--bump` on a fresh branch, `--version ` on a bumped one) stops with `BUMP_ANSWER_REQUIRED` until `--answer ""`; `never` is for a repo whose unit PRs never change VERSION: Step 12 keeps it with no question, refuses `--bump`, and `gstack-restamp --next` needs an explicit `--bump` or `--version` (the release process bumps). `never` needs `min_gstack_version` at its introducing release and cannot be combined with `queue_mode: stamp-at-merge`, `train` or `changelog_fragments`, which stamp a version at every merge. | | `queue_mode` | `claim-at-open` \| `stamp-at-merge` | `claim-at-open` | See [stamp-at-merge](#stamp-at-merge). `claim-at-open` is today's queue (`gstack-next-version` claims a slot when the PR opens). | | `release_tool` | command or `null` | `null` | The repo's own release command (`bun run release:restamp`). Runs in an isolated staging worktree under the [release-tool contract](#release-tool-contract). | | `release_outputs` | globs | `[]` | Paths the release tool may write beyond the version files, lockfiles, CHANGELOG and `stamp_paths`. Anything else is `RESTAMP_OUTPUT_OUTSIDE_ALLOWED`. | @@ -867,7 +867,7 @@ which carry the workflow consequences the skill prose would otherwise spell out: | `ship: gate-ahead: …` | with `.gstack/tmp/gate-ahead.json` present, Step 16 reuses only the lanes `gstack-tree-receipt … --json > .gstack/tmp/tree-receipt.json` marks reusable; stamp-sensitive and not-eligible lanes run live; `--force-gate ` is the coordinator's call | | `ship: bump patch: …` | Step 12 never asks for MINOR or MAJOR; PATCH or MICRO by size | | `ship: bump never: …` | Step 12 prints `VERSION: kept (policy: bump never; no write)`, asks nothing and refuses `--bump`; a branch that moved VERSION stops until it is put back | -| `ship: bump ask: …` | Step 12 asks for MINOR and MAJOR; on FRESH, `--keep-current` (or `--version `) keeps VERSION, and in a session no person answers any `--bump` waits for `--answer` | +| `ship: bump ask: …` | Step 12 asks for MINOR and MAJOR; on FRESH, `--keep-current` (or `--version `) keeps VERSION, and in a session no person answers any version Step 12 would write waits for `--answer` | | `ship: preregistration: …` | `gstack-evidence ancestor --from-policy --base ` must print only `ANCESTOR: ok` before push | | `ship: release_tool: …` | the command runs only inside `gstack-restamp` (staging worktree, outputs confined to `release_outputs`) | | `ship: review_scope …` | Step 9.4 item 0 runs `gstack-review-scope classify` on each pass's findings; see [review_scope](#review-scope) | diff --git a/lib/ship-release.ts b/lib/ship-release.ts index d02486812..9e379303d 100644 --- a/lib/ship-release.ts +++ b/lib/ship-release.ts @@ -90,7 +90,8 @@ export function version(argv: readonly string[], cwd: string): Report { } const atBase = !!explicit && cmpVersion(parseVersion(explicit)!, parseVersion(j.baseVersion)!) === 0; const keep = fresh && (has('--keep-current') || atBase); - if (keep && asked) return r.stop(`--bump ${asked} moves VERSION and ${has('--keep-current') ? '--keep-current' : `--version ${explicit}`} keeps it: pass one`); + const mover = asked ? `--bump ${asked}` : explicit && !atBase ? `--version ${explicit}` : ''; + if (keep && mover) return r.stop(`${mover} moves VERSION and ${has('--keep-current') ? '--keep-current' : `--version ${explicit}`} keeps it: pass one`); if (keep) return r.add(`VERSION: ${j.baseVersion} kept (FRESH, ${has('--keep-current') ? '--keep-current' : `--version ${explicit}`}; no write)`).next(KEPT_NEXT); const level = fresh ? asked ?? (policy?.bump === 'patch' ? 'patch' : undefined) : asked ?? derivedLevel(j.baseVersion, j.currentVersion) ?? undefined; if (!level) { @@ -98,9 +99,11 @@ export function version(argv: readonly string[], cwd: string): Report { return r.stop(`BUMP_LEVEL needed (FRESH, ${stat || 'no diff'}): MICRO under 50 lines, PATCH 50+ with no feature signal, MINOR/MAJOR ask first (Step 12.2); rerun with --bump , or with --keep-current to leave VERSION at ${j.baseVersion}`); } const answer = flag('--answer'); - if ((fresh ? asked : explicit) && policy?.bump !== 'patch' && !answer) { + if (explicit && cmpVersion(parseVersion(explicit)!, parseVersion(j.baseVersion)!) <= 0) return r.stop(`${explicit} is not above the base version ${j.baseVersion}`); + const rewrites = fresh ? !!asked : !!explicit && cmpVersion(parseVersion(explicit)!, parseVersion(j.currentVersion)!) !== 0; + if (rewrites && policy?.bump !== 'patch' && !answer) { const kind = bin(cwd, 'gstack-session-kind', [], 30_000).stdout.trim() || 'unknown'; - const pick = fresh ? `--bump ${asked}` : `--version ${explicit}`; + const pick = [asked && `--bump ${asked}`, explicit && `--version ${explicit}`].filter(Boolean).join(' '); if (kind !== 'interactive') return r.stop(`BUMP_ANSWER_REQUIRED (${kind} session, bump ask): a version Step 12 writes is the owner's approval, and a brief's "take the recommended option" is not an answer. Ask: ${pick} (recommended), keep ${fresh ? j.baseVersion : j.currentVersion} (--keep-current), or stop; then rerun with ${pick} --answer "". A repo whose unit PRs never bump sets bump never; one that always takes PATCH sets bump patch`); } let q = queue(cwd, base, level, j.baseVersion); diff --git a/lib/version-gate-train.ts b/lib/version-gate-train.ts index 90a4f7ff3..15e79caa5 100644 --- a/lib/version-gate-train.ts +++ b/lib/version-gate-train.ts @@ -17,7 +17,7 @@ import { matchesAny } from './ship-policy'; export type TrainHead = | { kind: 'off' } - | { kind: 'unstamped'; fragments?: number; entry?: 'fragment' | 'unreleased' | 'none-bot' } + | { kind: 'unstamped'; fragments?: number; entry?: 'fragment' | 'unreleased' | 'none-bot' | 'kept' } | { kind: 'stamped'; expected: string; fragments?: boolean } | { kind: 'invalid'; reason: string }; @@ -96,11 +96,15 @@ export function classifyTrainHead(o: { repoRoot: string; baseRef: string; head: let policy: any = null; try { policy = JSON.parse(show(base, '.gstack/ship-policy.json') ?? 'null'); } catch { policy = null; } const fragDir: string | null = typeof policy?.changelog_fragments?.dir === 'string' ? policy.changelog_fragments.dir : null; - if (!policy || ((typeof policy.train !== 'object' || policy.train === null) && !fragDir)) return { kind: 'off' }; + if (!policy || ((typeof policy.train !== 'object' || policy.train === null) && !fragDir && policy.bump !== 'never')) return { kind: 'off' }; const mergeBase = git(o.repoRoot, ['merge-base', base, o.head])?.trim(); if (!mergeBase) return { kind: 'invalid', reason: `no merge base between ${base} and ${o.head}` }; const npmVersion = (rev: string) => { try { return JSON.parse(show(rev, 'package.json') ?? '{}').version ?? null; } catch { return null; } }; + if (policy.bump === 'never') { + return show(mergeBase, 'VERSION') === show(o.head, 'VERSION') && npmVersion(mergeBase) === npmVersion(o.head) ? { kind: 'unstamped', entry: 'kept' } + : { kind: 'invalid', reason: `the base's ship policy says bump never: leave VERSION and package.json's version at the merge base's values` }; + } const changed = (git(o.repoRoot, ['diff', '--name-only', mergeBase, o.head]) ?? '').split('\n').filter(Boolean); const releaseOutputs: string[] = Array.isArray(policy.release_outputs) ? policy.release_outputs : []; // touched: what marks the head as stamped (the labels only feed the length check below) diff --git a/scripts/compare-pr-version.ts b/scripts/compare-pr-version.ts index db1c13f72..0adee42eb 100644 --- a/scripts/compare-pr-version.ts +++ b/scripts/compare-pr-version.ts @@ -41,6 +41,10 @@ const prVersionEnv = (process.env.PR_VERSION ?? "").trim(); if (process.env.BASE_REF && process.env.HEAD_SHA) { const train = classifyTrainHead({ repoRoot: process.cwd(), baseRef: process.env.BASE_REF, head: process.env.HEAD_SHA, author: process.env.PR_AUTHOR }); if (train.kind === "unstamped") { + if (train.entry === "kept") { + console.log(`✓ ${tag} keeps its merge base's VERSION and package.json version, as the base's ship policy (bump never) requires.`); + process.exit(0); + } const entry = train.entry === "fragment" ? `its entry is ${train.fragments} CHANGELOG fragment(s)` : train.entry === "none-bot" ? "it is a bot PR with no entry (the merge step writes one from its title)" : "the CHANGELOG change is confined to [Unreleased]"; console.log(`✓ ${tag} is unstamped under the base's ship policy: VERSION and package.json match its merge base, any changed release output is what the release tool generates at the merge base's VERSION, and ${entry}. The merge step stamps it.`); process.exit(0); diff --git a/test/gstack-ship-policy.test.ts b/test/gstack-ship-policy.test.ts index d59af5267..db0f9a97b 100644 --- a/test/gstack-ship-policy.test.ts +++ b/test/gstack-ship-policy.test.ts @@ -222,6 +222,15 @@ describe('gstack-ship-policy (bin)', () => { expect(runBin(repo, 'gstack-ship-policy', ['show-effective', '--policy-from', 'head']).out).toMatch(/^policy: head \(unreviewed\)/); }); + // Value: protects=validate reports bump never's own floor as the policy's minimum reader; fails_when=the CLI prints the bump key's 1.91.73.0 and a crew upgrades to a gstack that refuses the policy; + // why_new=GST-380 outside-voice review (the CLI passed introduction versions to minimumReader); seam=bin/gstack-ship-policy; no_claim=- + test('validate --json names bump never\'s floor as the minimum reader', () => { + const repo = fixture(); + fs.writeFileSync(path.join(repo.work, '.gstack', 'ship-policy.json'), JSON.stringify({ bump: 'never', min_gstack_version: BUMP_NEVER_KEY_VERSION }) + '\n'); + const j = JSON.parse(runBin(repo, 'gstack-ship-policy', ['validate', '--json']).out); + expect([j.valid, j.introduced.bump, j.min_reader]).toEqual([true, BUMP_NEVER_KEY_VERSION, BUMP_NEVER_KEY_VERSION]); + }); + test('POLICY_SOURCE_UNAVAILABLE (exit 3) when origin/ does not resolve; no fallback to the working tree', () => { const repo = fixture(); const r = runBin(repo, 'gstack-ship-policy', ['show-effective', '--base', 'release']); diff --git a/test/ship-release.test.ts b/test/ship-release.test.ts index 76abee2ba..70162b13c 100644 --- a/test/ship-release.test.ts +++ b/test/ship-release.test.ts @@ -102,8 +102,13 @@ describe('version (Step 12): keeping VERSION and waiting for the bump answer (GS untouched(f); const ok = f.run(RELEASE, ['version', '--base', 'main', '--bump', 'micro', '--answer', 'micro is fine'], UNATTENDED); expect([ok.status, read(f, 'VERSION')]).toEqual([0, '1.0.0.1\n']); - const rebump = f.run(RELEASE, ['version', '--base', 'main', '--bump', 'major', '--version', '2.0.0.0'], UNATTENDED); - expect([rebump.status, /^STOP: BUMP_ANSWER_REQUIRED/m.test(rebump.stdout), read(f, 'VERSION')]).toEqual([3, true, '1.0.0.1\n']); + const rebump = f.run(RELEASE, ['version', '--base', 'main', '--version', '2.0.0.0'], UNATTENDED); + expect([rebump.status, rebump.stdout.includes('BUMP_ANSWER_REQUIRED'), rebump.stdout.includes('Ask: --version 2.0.0.0 (recommended), keep 1.0.0.1'), read(f, 'VERSION')]).toEqual([3, true, true, '1.0.0.1\n']); + expect(f.run(RELEASE, ['version', '--base', 'main', '--version', '1.0.0.0'], UNATTENDED).stdout).toContain('STOP: 1.0.0.0 is not above the base version 1.0.0.0'); + const rewritten = f.run(RELEASE, ['version', '--base', 'main', '--version', '2.0.0.0', '--answer', 'go to 2.0.0.0'], UNATTENDED); + expect([rewritten.status, read(f, 'VERSION')]).toEqual([0, '2.0.0.0\n']); + const both = fx().run(RELEASE, ['version', '--base', 'main', '--bump', 'micro', '--version', '1.0.0.5'], UNATTENDED); + expect([both.status, both.stdout.includes('Ask: --bump micro --version 1.0.0.5 (recommended)')]).toEqual([3, true]); const patch = fx({ '.gstack/ship-policy.json': '{"bump":"patch"}\n' }); expect([patch.run(RELEASE, ['version', '--base', 'main'], UNATTENDED).status, read(patch, 'VERSION')]).toEqual([0, '1.0.1.0\n']); }); @@ -130,12 +135,14 @@ describe('version (Step 12): keeping VERSION and waiting for the bump answer (GS // Value: protects=a --bump beside --keep-current or --version is a contradiction the bin names instead of picking one; fails_when=it silently keeps or silently bumps; // why_new=GST-380 review F8 (flag precedence); seam=none; no_claim=- - test('--bump with --keep-current or --version stops and writes nothing', () => { + test('--bump or --version with --keep-current or --version stops and writes nothing', () => { const f = fx(); for (const args of [['--keep-current'], ['--version', '1.0.0.0']]) { const r = f.run(RELEASE, ['version', '--base', 'main', '--bump', 'micro', ...args]); expect([r.status, r.stdout.includes('STOP: --bump micro moves VERSION and')]).toEqual([3, true]); } + const keepAndMove = f.run(RELEASE, ['version', '--base', 'main', '--keep-current', '--version', '1.0.0.5']); + expect([keepAndMove.status, keepAndMove.stdout.includes('STOP: --version 1.0.0.5 moves VERSION and --keep-current keeps it')]).toEqual([3, true]); untouched(f); }); }); diff --git a/test/version-gate-fragments.test.ts b/test/version-gate-fragments.test.ts index 5431b0559..6a9ed9e8b 100644 --- a/test/version-gate-fragments.test.ts +++ b/test/version-gate-fragments.test.ts @@ -7,6 +7,7 @@ import { afterEach, describe, expect, test } from 'bun:test'; import * as fs from 'node:fs'; import * as path from 'node:path'; +import { BUMP_NEVER_KEY_VERSION } from '../lib/ship-policy'; import { BASE_POLICY, cleanup, makeFixtureRepo, ROOT, type FixtureRepo } from './helpers/restamp-fixture'; const SCRIPT = path.join(ROOT, 'scripts', 'compare-pr-version.ts'); @@ -24,6 +25,22 @@ function run(r: FixtureRepo, head: string, extra: Record = {}, c return { code: out.exitCode, out: out.stdout.toString() + out.stderr.toString() }; } +describe('version gate: bump never', () => { + // Value: protects=under bump never the Version Gate passes a head that keeps VERSION and fails one that moves it; fails_when=the gate rejects every kept-version PR as "VERSION not bumped", or lets a unit PR bump; + // why_new=GST-380 added bump never (outside-voice review); seam=classifyTrainHead, scripts/compare-pr-version.ts; no_claim=the claim queue + test('a kept VERSION passes and a moved VERSION fails', () => { + const r = makeFixtureRepo({ policy: { ...BASE_POLICY, queue_mode: 'claim-at-open', release_tool: null, release_outputs: [], bump: 'never', min_gstack_version: BUMP_NEVER_KEY_VERSION }, prefix: 'version-gate-never-' }); + repos.push(r); + r.git('checkout', '-q', '-b', 'k'); + write(r, 'k.txt', 'k\n'); + const kept = run(r, commit(r, 'k')); + expect([kept.code, kept.out.includes('as the base\'s ship policy (bump never) requires')]).toEqual([0, true]); + write(r, 'VERSION', '1.2.4.0\n'); + const moved = run(r, commit(r, 'bump')); + expect([moved.code, moved.out.includes('the base\'s ship policy says bump never')]).toEqual([1, true]); + }); +}); + describe('version gate: changelog fragments', () => { test('a fragment head passes, a dependency or script edit beside it too', () => { const r = fixture();