From 83da789708f6377e87f65f3f3417df6ae058fb03 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20S=C3=A4nger?= <20968534+dsnger@users.noreply.github.com> Date: Sun, 19 Jul 2026 11:29:04 +0200 Subject: [PATCH 1/2] feat(ci): require a version bump when the shipped plugin changes (invariant 12) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An installed plugin lives under a version-keyed cache path, so a change that merges without a bump never reaches an installed copy. That happened twice: a machine ran 0.1.0 while main was at 0.4.0, and the 0.4.0 bump had to be asked for during PR #4. main was still carrying two un-released plugin commits when this was written. scripts/check-version-bump.sh fails a PR that changes any path under a plugins// directory still present at HEAD without changing that plugin's manifest version. Both versions are read from commits, every git call's status is checked, and it fails closed on anything unexpected — a false skip would be a silent green on exactly the defect it exists to catch. 36 assertions, mutation-verified. Also here: a backfilled plugin CHANGELOG (0.1.0-0.4.1), the artifact-version-not-bumped taxonomy class and its ledger row, and the version bump this very check now requires. Rung 2 per dev-workflow:harden-finding. --- .github/workflows/ci.yml | 58 ++- AGENTS.md | 64 ++- README.md | 16 +- docs/architecture.md | 5 +- docs/hardening-log.md | 1 + docs/hardening-taxonomy.md | 12 + .../2026-07-19-plugin-version-bump-check.md | 294 ++++++++++++ .../2026-07-19-plugin-version-bump-check.md | 438 ++++++++++++++++++ .../dev-workflow/.claude-plugin/plugin.json | 2 +- plugins/dev-workflow/CHANGELOG.md | 79 ++++ plugins/dev-workflow/examples/README.md | 10 +- scripts/check-invariants.sh | 4 +- scripts/check-version-bump.sh | 192 ++++++++ scripts/check-version-bump.test.sh | 373 +++++++++++++++ 14 files changed, 1512 insertions(+), 36 deletions(-) create mode 100644 docs/superpowers/plans/2026-07-19-plugin-version-bump-check.md create mode 100644 docs/superpowers/specs/2026-07-19-plugin-version-bump-check.md create mode 100644 plugins/dev-workflow/CHANGELOG.md create mode 100644 scripts/check-version-bump.sh create mode 100644 scripts/check-version-bump.test.sh diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 6625884..8f17a2b 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -22,7 +22,7 @@ jobs: # shellcheck image and claude CLI avoid. The runner is pinned to an OS release # rather than `ubuntu-latest` — GitHub still refreshes that image's contents # weekly, so this bounds the drift, it does not eliminate it. Full image - # reproducibility would need a container, which this four-check battery + # reproducibility would need a container, which this check battery # doesn't warrant. runs-on: ubuntu-24.04 steps: @@ -32,15 +32,22 @@ jobs: # later step — or anything a step runs — can read it. Nothing here needs # authenticated git after the clone, so don't leave it lying around. persist-credentials: false + # The version-bump check diffs against the merge-base with the PR's base + # branch, and the default depth-1 clone has neither the base branch nor a + # merge-base. ~50 commits, so the full history costs nothing here. + fetch-depth: 0 - # The hook, the invariant checker and their two suites are the only executables here, so lint plus their test - # suite are the only mechanical gates we have. Everything else here is a prompt, - # and prompts have no typechecker (see README, "Contributing"). + # The hook, the two checkers and their three suites are the only executables here, + # so lint plus those suites are the only mechanical gates we have. Everything else + # here is a prompt, and prompts have no typechecker (see README, "Contributing"). - # Enforces the POSIX-sh invariant mechanically: the hook runs under whatever - # /bin/sh a user has, so a bash-ism is a portability bug, not a style opinion. + # POSIX sh, for two different reasons that must not be blurred: the HOOK is bound + # by invariant 4, because it runs under whatever /bin/sh a user has, so a bash-ism + # there is a portability bug rather than a style opinion; the two repo-local + # checkers are bound by this workflow invoking them with `sh`, which is their own + # requirement and not invariant 4's reach. # Pinned to an exact image tag — a moving linter makes the gate irreproducible. - - name: Lint hook scripts (POSIX sh) + - name: Lint shell scripts (POSIX sh) run: | docker run --rm -v "$PWD:/mnt" -w /mnt koalaman/shellcheck:v0.11.0 \ --shell=sh plugins/dev-workflow/hooks/codex-gate.sh @@ -50,23 +57,48 @@ jobs: # every other check still applies to this file. docker run --rm -v "$PWD:/mnt" -w /mnt koalaman/shellcheck:v0.11.0 \ --shell=sh --exclude=SC2015 plugins/dev-workflow/hooks/codex-gate.test.sh - # The invariant checker is the repo's second executable artifact and is a - # CI gate itself, so it gets the same POSIX lint as the hook. + # The two repo-local checkers are CI gates themselves, so they get the same + # POSIX lint as the hook. docker run --rm -v "$PWD:/mnt" -w /mnt koalaman/shellcheck:v0.11.0 \ --shell=sh scripts/check-invariants.sh docker run --rm -v "$PWD:/mnt" -w /mnt koalaman/shellcheck:v0.11.0 \ --shell=sh --exclude=SC2015 scripts/check-invariants.test.sh + # No exclusion on this pair: unlike the two suites above, it has no + # `[ c ] && pass || fail` lines, so every rule applies as-is. + docker run --rm -v "$PWD:/mnt" -w /mnt koalaman/shellcheck:v0.11.0 \ + --shell=sh scripts/check-version-bump.sh + docker run --rm -v "$PWD:/mnt" -w /mnt koalaman/shellcheck:v0.11.0 \ + --shell=sh scripts/check-version-bump.test.sh - name: Hook state-machine tests run: sh plugins/dev-workflow/hooks/codex-gate.test.sh - # Invariants 5 and 6, mechanically. Both were prose first and both were - # violated anyway — a floating action ref shipped in this very workflow, and a - # duplicate hooks manifest key stopped the plugin loading in 0.2.1. - - name: Invariant checks (pinning, manifest) + # Invariants 5 and 6 mechanically, plus BOTH checkers' regression suites. The + # invariant-12 checker itself is not here — it needs a PR base and runs in the + # step below, so naming this step "version bump" would show a green version-bump + # label on a push that was never version-bump checked. + # Each rule was prose first and each was violated anyway — a floating action ref + # shipped in this very workflow, a duplicate hooks manifest key stopped the plugin + # loading in 0.2.1, and the plugin shipped changes without a bump twice. + - name: Invariant checks (pinning, manifest) + both checker suites run: | sh scripts/check-invariants.test.sh sh scripts/check-invariants.sh + sh scripts/check-version-bump.test.sh + + # Invariant 12 itself needs a base to diff against, so it runs only on pull + # requests — a push to main has no PR base, and diffing the push range would just + # re-check what the PR run already gated. Consequence, stated rather than implied: + # a commit pushed straight to main bypasses this check entirely. + # + # base_ref goes through env, never into the run script via ${{ }}: git ref names + # may contain shell metacharacters, and direct interpolation would splice a branch + # name in as code rather than pass it as data. + - name: Version bump required for plugin changes (invariant 12) + if: github.event_name == 'pull_request' + env: + BASE_REF: ${{ github.base_ref }} + run: sh scripts/check-version-bump.sh "origin/$BASE_REF" - uses: actions/setup-node@820762786026740c76f36085b0efc47a31fe5020 # v7.0.0 with: diff --git a/AGENTS.md b/AGENTS.md index c5ec0f3..4d0823a 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -26,7 +26,7 @@ under `.mcp/`) is deliberately absent, not overlooked. ``` README.md # what the kit is, setup, daily use, contributing -MANIFEST.md # what each shipped file is and where it comes from +MANIFEST.md # inventory of source-files/, the frozen extraction seed AGENTS.md # this file — the invariants both gates check CLAUDE.md # discipline rules + the two review gates todos.md # backlog; `pending` ledger rows point here by ref @@ -36,13 +36,16 @@ todos.md # backlog; `pending` ledger rows point here by .github/workflows/ci.yml # lint + hook tests + invariant checks + validate scripts/check-invariants.sh # invariants 5 and 6, mechanically (rung 2) scripts/check-invariants.test.sh # its regression suite — reject/accept pairs +scripts/check-version-bump.sh # invariant 12, mechanically — PR-only (rung 2) +scripts/check-version-bump.test.sh # its regression suite — policy/operational/accept plugins/dev-workflow/ .claude-plugin/plugin.json # metadata only — no component keys (invariant 6) + CHANGELOG.md # every manifest version, newest first skills/{intake,harden-finding}/SKILL.md agents/finding-triage.md # read-only PR-comment checker (convention-loaded) commands/{workflow-init,process-pr-review}.md hooks/{hooks.json,codex-gate.sh,codex-gate.test.sh} - examples/ # read, don't install — one stack's answers + examples/ # ships, but never scaffolded — one stack's answers docs/ architecture.md # layout + the two non-obvious design decisions coding-workflow.md # the methodology this plugin encodes @@ -51,14 +54,17 @@ docs/ hardening-log.md # append-only findings ledger (union-merged) hardening-taxonomy.md # this project's fingerprint classes pr-review-bots.md # the Wait-for routing list + where each bot's findings appear + superpowers/{specs,plans,stories}/ # the approved artifacts behind past changes source-files/ # the extraction seed this repo was built from ``` **Boundaries.** `skills/`, `commands/`, `agents/` and `hooks/hooks.json` are loaded by convention -from their paths. The executable artifacts are the hook and its test, plus -`scripts/check-invariants.sh` and its test (the hook ships in the plugin; the checker -is repo-local CI); everything else is text -read by a model. `examples/` is reference material, outside the loaded surface. +from their paths. The executable artifacts are the hook and its test, plus the two +repo-local CI checkers and their tests (`scripts/check-invariants.{sh,test.sh}` and +`scripts/check-version-bump.{sh,test.sh}`) — the hook ships in the plugin, the checkers +do not; everything else is text read by a model. `examples/` is reference material, +outside the loaded surface: never scaffolded or copied into a user's project, though it +does ship inside the plugin package. **Dependency direction.** The plugin depends on superpowers (skills it hands off to) and on a Codex MCP server exposing both `exec` and `review` (the gates key on those @@ -109,8 +115,34 @@ reader can judge whether it still holds. `commands/`, `agents/` and `hooks/hooks.json` load automatically; a manifest key for them is redundant at best and fatal for hooks (duplicate-hooks error → the plugin does not load at all; fixed in 0.2.1). Manifest keys only for files outside convention paths. -7. **`examples/` is read-only reference.** Never installed, never copied by a command, - never presented as a default — it encodes one stack's answers and will not transfer. +7. **`examples/` is read-only reference.** Never scaffolded or copied into a user's + project, never presented as a default — it encodes one stack's answers and will not + transfer. It *does* ship inside the plugin package (the cache copies the plugin + directory wholesale), which is why invariant 12 covers it: shipped-but-not-scaffolded + is not the same claim as not-shipped, and reading it as the latter once made + `examples/` look out of scope for the version-bump rule. +12. **A plugin change requires a version bump.** A pull request that changes any path + under a `plugins//` directory **that still exists at HEAD** — `examples/` + included, and no exemptions among the paths inside such a directory — must also + change that plugin manifest's `version`, or CI fails. (Deleting a whole plugin + directory is the one shape outside the rule, since nothing of it ships afterwards; + deleting only its manifest while the directory survives fails closed.) The checker is + `scripts/check-version-bump.sh`, run **on pull requests only**, with + `scripts/check-version-bump.test.sh` as its suite. An installed copy lives under a + version-keyed cache path, so an un-bumped change never reaches it: the machine keeps + running the old code with no signal that it is stale. This was a convention first, + and it failed twice — a machine ran 0.1.0 while main was at 0.4.0, and the 0.4.0 + bump had to be asked for during review; main was still carrying two un-released + plugin commits when the check was written. + **What the check does not catch** — the same list the script header and the ledger + row carry, because a partial list is an overclaim: it verifies that a bump is + *present*, not that it is right. It does not check: semantic correctness (a patch + where a minor was due passes); direction (any different string passes, including a + decrease); anything outside a `pull_request` event (a direct push to main bypasses it + entirely); deletion of an entire plugin directory; any change to which directory the + marketplace entry + points at (rename, copy, or `source` repoint); two PRs branched from the same version + each bumping to the same new one; and whether the version was ever released or tagged. ### Prompts and scaffolding @@ -172,11 +204,12 @@ Every command below was run in this session and observed to exit 0. | Role | Command | |---|---| -| quality (the whole battery — what CI runs) | `shellcheck --shell=sh plugins/dev-workflow/hooks/codex-gate.sh && shellcheck --shell=sh --exclude=SC2015 plugins/dev-workflow/hooks/codex-gate.test.sh && shellcheck --shell=sh scripts/check-invariants.sh && shellcheck --shell=sh --exclude=SC2015 scripts/check-invariants.test.sh && sh plugins/dev-workflow/hooks/codex-gate.test.sh && sh scripts/check-invariants.test.sh && sh scripts/check-invariants.sh && claude plugin validate . --strict` | +| quality (the whole battery — what CI runs) | `shellcheck --shell=sh plugins/dev-workflow/hooks/codex-gate.sh && shellcheck --shell=sh --exclude=SC2015 plugins/dev-workflow/hooks/codex-gate.test.sh && shellcheck --shell=sh scripts/check-invariants.sh && shellcheck --shell=sh --exclude=SC2015 scripts/check-invariants.test.sh && shellcheck --shell=sh scripts/check-version-bump.sh && shellcheck --shell=sh scripts/check-version-bump.test.sh && sh plugins/dev-workflow/hooks/codex-gate.test.sh && sh scripts/check-invariants.test.sh && sh scripts/check-invariants.sh && sh scripts/check-version-bump.test.sh && sh scripts/check-version-bump.sh main && claude plugin validate . --strict` | | typecheck | n/a — no typed sources (shell + markdown) | -| lint | `shellcheck --shell=sh plugins/dev-workflow/hooks/codex-gate.sh && shellcheck --shell=sh --exclude=SC2015 plugins/dev-workflow/hooks/codex-gate.test.sh && shellcheck --shell=sh scripts/check-invariants.sh && shellcheck --shell=sh --exclude=SC2015 scripts/check-invariants.test.sh` | +| lint | `shellcheck --shell=sh plugins/dev-workflow/hooks/codex-gate.sh && shellcheck --shell=sh --exclude=SC2015 plugins/dev-workflow/hooks/codex-gate.test.sh && shellcheck --shell=sh scripts/check-invariants.sh && shellcheck --shell=sh --exclude=SC2015 scripts/check-invariants.test.sh && shellcheck --shell=sh scripts/check-version-bump.sh && shellcheck --shell=sh scripts/check-version-bump.test.sh` | | test | `sh plugins/dev-workflow/hooks/codex-gate.test.sh` | | invariant checks (5 pinning, 6 manifest) | `sh scripts/check-invariants.test.sh && sh scripts/check-invariants.sh` | +| invariant check (12 version bump) | `sh scripts/check-version-bump.test.sh && sh scripts/check-version-bump.sh main` | | build | n/a — nothing is compiled or bundled | **Prerequisites and pinning.** The quality command needs `shellcheck` (0.11.0 locally; @@ -189,5 +222,14 @@ disable: every other shellcheck rule still applies to that file. Its hits are al mode is a broken stdout — which runs `fail` as well, producing a spurious FAIL rather than a false pass. Revisit if `pass`/`fail` ever gain logic that can legitimately fail. +**`check-version-bump.sh main` has a precondition**, unlike everything else in the +battery: it compares *commits*, so run it once the work is committed (the Gate-B WIP +commit is the natural point) and against a base ref that is current. Run mid-loop with +the plugin edits still in the working tree, it reports clean — correctly, and +uselessly. CI passes the PR's own base ref instead of `main`. It is in the quality row +because that row claims to be what CI runs, and as of invariant 12 that includes this. + CI runs the parts as separate steps for readable failures; the chained form above -is the single command a human runs. +is the single command a human runs. One difference is deliberate: CI's version-bump +step is `pull_request`-only, while the local battery always runs it (on `main`, where +the merge-base is HEAD, it passes trivially). diff --git a/README.md b/README.md index 0eb395d..a61ac3c 100644 --- a/README.md +++ b/README.md @@ -111,11 +111,17 @@ honest gap ([reasoning](docs/coding-workflow.md#adapting-it-to-another-project)) ## Contributing CI ([`.github/workflows/ci.yml`](.github/workflows/ci.yml)) runs four checks on every -PR and push to main: `shellcheck --shell=sh` over both executables and their test -files, the hook's test suite, the invariant checks -([`scripts/check-invariants.sh`](scripts/check-invariants.sh) and its own suite), and -`claude plugin validate . --strict`. The single command that runs the whole battery -locally is in [`AGENTS.md`](AGENTS.md) under "Commands". +PR and push to main: `shellcheck --shell=sh` over all three executables and their test +files, the hook's test suite, +[`scripts/check-invariants.sh`](scripts/check-invariants.sh) (invariants 5 and 6) plus +both checkers' regression suites, and `claude plugin validate . --strict`. + +A fifth check runs **on pull requests only**: +[`scripts/check-version-bump.sh`](scripts/check-version-bump.sh) (invariant 12), which +needs a base branch to diff against. Its *suite* runs unconditionally with the others; +the checker itself does not, so a commit pushed straight to main is never version-bump +checked. The single command that runs the whole battery locally is in +[`AGENTS.md`](AGENTS.md) under "Commands". Everything else here is a **prompt**, and prompts have no typechecker — they are reviewed against [`docs/prompt-standards.md`](docs/prompt-standards.md), this repo's own diff --git a/docs/architecture.md b/docs/architecture.md index e52577e..00ab9d9 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -16,15 +16,18 @@ README.md, MANIFEST.md, AGENTS.md, CLAUDE.md, todos.md .claude-plugin/marketplace.json .github/workflows/ci.yml # lint + hook tests + invariant checks + validate scripts/check-invariants.sh # invariants 5 and 6, mechanically (+ .test.sh) +scripts/check-version-bump.sh # invariant 12, PR-only, mechanically (+ .test.sh) plugins/dev-workflow/ .claude-plugin/plugin.json + CHANGELOG.md # every manifest version, newest first skills/{intake,harden-finding}/SKILL.md agents/finding-triage.md commands/{workflow-init,process-pr-review}.md hooks/{hooks.json,codex-gate.sh,codex-gate.test.sh} - examples/ # read, don't install — one stack's answers + examples/ # ships, but never scaffolded — one stack's answers docs/{getting-started,coding-workflow,prompt-standards,architecture}.md docs/{hardening-log,hardening-taxonomy,pr-review-bots}.md +docs/superpowers/{specs,plans,stories}/ # approved artifacts behind past changes source-files/ # the extraction seed this repo was built from ``` diff --git a/docs/hardening-log.md b/docs/hardening-log.md index 8543fc2..8ae2542 100644 --- a/docs/hardening-log.md +++ b/docs/hardening-log.md @@ -19,3 +19,4 @@ escape `\|`, one line), `source` (gate-a|gate-b|bot|manual), | 2026-07-18 | docs-drift | three docs claimed the plugin manifest declares hooks; it declares none — the 0.2.1 duplicate-hooks load failure | gate-b | major | 1 prose | AGENTS.md Don'ts, "Never state what the manifest declares without reading it" (new rule + grep recipe; invariant 6 already existed and constrains the manifest, not claims about it) | | 2026-07-18 | unverified-enforcement-claim | six citable claims across one spec, its plan and its diff that something was enforced/caught/guaranteed where no mechanism did it — each caught by a gate, none by the author, one written into the same document that records the pattern | gate-a | major | P std | docs/prompt-standards.md item 11 (+ the same item in the workflow-init inline template). Mixed provenance: four from Gate A on the spec, one from Gate A on the plan, one from Gate B on the pre-merge diff; `source` records Gate A as the majority and the trigger | | 2026-07-18 | docs-drift | a Gate-B fix reordered a precedence rule and added a terminal state in process-pr-review; the approved spec was never updated and disagreed until a PR bot found it | bot | major | P std | CLAUDE.md §5 Gate B, "a fix that changes specified behaviour updates the spec in the same commit" (+ the workflow-init inline template). Escalated from the 2026-07-18 `1 prose` row, whose AGENTS.md rule was scoped to manifest claims and could not reach spec-vs-implementation drift | +| 2026-07-19 | artifact-version-not-bumped | plugin changes merged without a version bump, so installed copies silently stayed stale — a machine ran 0.1.0 while main was at 0.4.0, the 0.4.0 bump had to be requested during PR #4, and main still carried two un-released plugin commits when this was written | manual | major | 2 lint | scripts/check-version-bump.sh + .test.sh, wired PR-only in ci.yml; AGENTS.md invariant 12. It verifies that a bump is PRESENT, not that it is right. It does not check: semantic correctness (a patch where a minor was due passes); direction (any different string passes, including a decrease); anything outside a pull_request event (a direct push to main bypasses it entirely); deletion of an entire plugin directory; any change to which directory the marketplace entry points at (rename, copy, or `source` repoint); two PRs branched from the same version each bumping to the same new one; and whether the version was ever released or tagged | diff --git a/docs/hardening-taxonomy.md b/docs/hardening-taxonomy.md index 5745eff..ee98a57 100644 --- a/docs/hardening-taxonomy.md +++ b/docs/hardening-taxonomy.md @@ -37,6 +37,18 @@ the synonyms a future reader might search for instead. updating whichever is stale; this one is fixed by verifying the mechanism exists before the sentence is written. +- `artifact-version-not-bumped` — a change to a versioned artifact this repo *ships* + merges without changing the version consumers key on, so installed copies stay on the + old code with no signal. Aliases: stale install, cache key unchanged, forgot to bump, + release not cut, propagation failure, "it's merged but my machine still runs the old + one". + + **Not `dependency-unpinned`**, though both are about versions. That class is about + *this* repo consuming something floating — the risk runs inbound, and the fix is to + pin. This one runs outbound: our own artifact fails to reach its consumers, and the + fix is to force the version forward. Grep this one when the sentence is "nobody got + the change"; grep the other when it is "we don't know what we got". + **Promotion candidate.** These classes are stack-neutral, not project vocabulary, so they belong in the `harden-finding` base list rather than here. They live here because the skill says to mint into this file (the plugin ships the base classes, the project diff --git a/docs/superpowers/plans/2026-07-19-plugin-version-bump-check.md b/docs/superpowers/plans/2026-07-19-plugin-version-bump-check.md new file mode 100644 index 0000000..53fe56f --- /dev/null +++ b/docs/superpowers/plans/2026-07-19-plugin-version-bump-check.md @@ -0,0 +1,294 @@ +# Plan — CI check: a plugin change requires a version bump + +Executes the approved spec (`spec.md`, Gate A spec run closed at pass 9). Branch +`harden/version-bump-check`, already created off `main` (5acd13e). One PR, merge when +green. + +Order is deliberate: the canonical limitation text first (three artifacts must quote it +verbatim), then the checker and its suite, then CI wiring, then prose, then the bump — +so the last change is the one the check itself judges. + +## Step 0 — the canonical limitation list + +Write the seven-item list once, **before** anything quotes it, to +`$SCRATCH/limitations.txt` where `SCRATCH` is the session scratchpad +(`/private/tmp/claude-501/-Users-daniel-DEVELOPMENT-APPS-dev-workflow-kit/e378824d-945f-417d-b8e3-df7763c73241/scratchpad`). +Absolute, and outside the repo: a repo-relative `scratchpad/` would be a fourth untracked +artifact contradicting this plan's own staging audit, and could be staged by accident. +`git status --short` must stay clean of both scratch files. Step 1's script header, step 4's AGENTS.md invariant and the ledger `ref` all +paste from this file. Writing it here rather than inside step 4 avoids the trap of +authoring the header from memory, then editing an already-lint-and-mutation-verified +checker later just to align its wording. + +Items: semantic correctness · direction · PR-only (direct pushes bypass) · deletion of +an *entire* plugin directory (manifest-only deletion fails closed — corrected mid-Gate-B) +· any change to which directory the marketplace entry points at (rename, copy, or +`source` repoint) · concurrent same-version collisions · release/tag status. + +## Step 1 — `scripts/check-version-bump.sh` + +Per spec § Mechanism. Shape, in order: + +1. `set -u`; exactly-one-argument guard → usage on stderr, **exit 2** (the suite asserts + 2, so the CLI contract is pinned rather than described). +2. `cd "$(dirname "$0")/.." || exit 1` — **an explicit failure branch.** `set -u` does + not abort on a failed `cd`; without the `||` the checker would continue in the + caller's directory and report a clean tree from the wrong repo. +3. `git --literal-pathspecs` on every call. `die()` prints and exits non-zero; + `fail()` accumulates a policy offender and sets `rc=1` (mirrors `check-invariants.sh`). +4. `mb=$(git merge-base "$1" HEAD)` — status captured, `die` naming the ref on failure. +5. `dirs=$(git ls-tree -d --name-only HEAD plugins/)` — status captured separately, + `die` on non-zero. +6. A `for` loop over the enumeration with a newline-only `IFS` and globbing off — + **not** `while IFS= read`, which on the right of a pipe runs in a subshell where + `fail` could not accumulate into `rc`. Each basename (after stripping `plugins/`) + must match `[A-Za-z0-9._-]+` → `die` naming the entry otherwise. +7. `git diff --quiet "$mb" HEAD -- "$d/"` → 0 skip, 1 continue, anything else `die`. +8. HEAD manifest presence via `git ls-tree HEAD -- "$m"` (non-zero → `die`; empty → + **also `die`**: the directory is in HEAD's tree and changed, so it still ships, and a + shipped plugin with no manifest has no version to key on. Only a whole directory + disappearing is carved out, and that never reaches this line. Revised mid-Gate-B, + which is why the spec and all three limitation lists changed in the same commit.) +9. Merge-base manifest presence, same call shape against `$mb`. +10. **`git show` per side only when that side's presence lookup found the manifest.** + HEAD is always read (step 8 proved it exists); the base **only if step 9 found it** — + unconditionally reading a known-absent base manifest would `die` and break accept + case A4, turning the new-plugin carve-out into a failure. Status captured, `die` on + non-zero. +11. `extract_version()`: the spec's `grep -o`; count matches, `die` unless exactly one; + strip to the quoted value; `die` if the value contains a backslash. HEAD always; base + only when present (absent → base version empty, differs from any real version, A4 + passes). +12. Compare; equal → `fail` naming the plugin and the version. +13. Print every offender; `exit "$rc"`. + +Header comment: what it catches; **the step-0 list pasted verbatim**; the parser ceilings +(nested `version` key, multi-line JSON fails closed, spelling compared rather than parsed +value); that POSIX `sh` here is this script's own requirement and **not** invariant 4, +which is hook-scoped; and TESTED SPELLINGS ONLY, like its sibling. + +**Verify:** `shellcheck --shell=sh scripts/check-version-bump.sh` → 0. + +## Step 2 — `scripts/check-version-bump.test.sh` + +Throwaway repos under `mktemp -d`, with the sibling suite's `work` guard and `trap` +(abort if `mktemp` produced nothing — `rm -rf "$work/r"` on an empty `work` is `rm -rf /r`). + +Helpers: +- `mkrepo()` — `git init`, deterministic identity passed per-invocation + (`git -c user.name=… -c user.email=…`), never touching the developer's global config; + a base commit on `main`, then a branch. +- `expect_reject NAME PATTERN…` — **accepts one or more required patterns**, all of which + must appear, plus a non-zero exit. P4 (two un-bumped plugins) passes both plugin names, + so a first-failure implementation that names only one fails the row. A single-pattern + contract would let exactly the defect P4 exists to catch slip through. +- `expect_accept NAME`; arity rows additionally assert **exit status 2**. +- `with_failing_git` — a wrapper early on `PATH` matching the **full argv** + (`case "$*"`), `exec`ing the real git otherwise. Written into that case's own temp dir; + `PATH` exported only inside the subshell that runs the checker. + +Rows: P1–P4, P7–P10; O1–O18 (incl. O15b escape, O16 space, O17 C-quoted, O14/O15 arity, +and O18 manifest-only deletion, which began as accept row A6 and became a reject in +Gate B); accepts A1–A5 and A7–A10 (no A6 — it is now O18). Printed in `policy:` / +`operational:` / `accept:` groups so a mutation's effect is readable at a glance. + +**Verify:** `sh scripts/check-version-bump.test.sh` → all assertions pass; +`shellcheck --shell=sh scripts/check-version-bump.test.sh` → 0 (no exclusion: this +suite has no `[ c ] && pass || fail` lines, so nothing needs excluding). + +**Mutation, run and recorded — not asserted from theory:** +1. `cp scripts/check-version-bump.sh "$SCRATCH/checker.pristine"` (the same absolute + scratchpad as step 0, never a repo-relative path); record `shasum` and `ls -l` mode. +2. Replace the version comparison with `false`; re-run the suite. +3. Confirm **exactly the P rows fail**, every O and A row green. Any other row moving + means the suite is wrong, not the mutation. +4. Restore by copying the pristine file back, then **prove the restore with `cmp` + against `checker.pristine`** plus a mode check. (`git diff` proves nothing here: the + file is untracked until step 5, so it reads empty before and after any mutation — + a safeguard that cannot fail is not a safeguard.) +5. Re-run the suite; fully green again. + +## Step 3 — CI wiring (`.github/workflows/ci.yml`) + +- `checkout` gains `fetch-depth: 0`; `persist-credentials: false` stays. +- The shellcheck step gains the two new files against the same pinned image. **Rename the + step** from "Lint hook scripts (POSIX sh)" — it has covered more than the hook since + `check-invariants.sh` — and re-scope its comment: invariant 4 is hook-scoped, so the + hook's POSIX requirement is attributed to invariant 4 and the checkers' to their own + CI-invocation requirement. +- **`scripts/check-invariants.sh`'s own header is corrected in the same pass**: it + currently reads "POSIX sh, no jq (invariant 4)", the exact misattribution this step + says must not be blurred. Leaving it would fix the new file and the CI comment while + the older checker keeps making the claim. Re-run its shellcheck and its suite after + the edit. +- The invariant-checks step gains `sh scripts/check-version-bump.test.sh`. +- New step, `if: github.event_name == 'pull_request'`, with + `env: BASE_REF: ${{ github.base_ref }}` and + `run: sh scripts/check-version-bump.sh "origin/$BASE_REF"`. Comment says why it is + PR-only and that a direct push to main bypasses it. +- No new action, no new image → nothing new to pin (invariant 5 unaffected; the invariant + checker itself confirms this in step 5). + +**Drift sweep, repository-wide and by exact phrase** — not limited to `ci.yml` and the +README, since the same strings appear in AGENTS.md and `docs/architecture.md` and step 4 +edits those for other reasons: +`four checks` · `four-check battery` · `both executables` · `second executable artifact` +· `their two suites` · `only executables` · `Invariants 5 and 6`, each searched with +`grep -rn … | grep -v source-files/`, and **every hit dispositioned** (updated, or +consciously left with a reason). Afterwards the `ci.yml` step names and comments are +re-read whole: a step *name* can go stale in ways no phrase list anticipates. + +**Verify:** honestly, **nothing locally**. `claude plugin validate . --strict` does not +read `.github/workflows/ci.yml`, so it proves nothing here and must not be cited as if it +did; this repo has no pinned YAML/actions validator and adding one is outside this change. +Step 3's verification is step 6.2's observation of the actual run. Recorded as locally +unverified rather than claimed green. + +## Step 4 — prose + +1. `plugins/dev-workflow/CHANGELOG.md` — entries 0.4.1 … 0.1.0 per the spec's interval + rule. **Two distinct verifications, both recorded:** + - *Heading completeness*: the distinct manifest versions in history + (`for c in $(git log --format=%h --reverse -- ) …`) **union the declared + target 0.4.1** must equal the set of `##` headings. The union matters: 0.4.1 does not + exist in history at this point, so a plain comparison would report the required + heading as extra. **Re-run without the union after step 5.2's WIP commit**, where + history does contain it, as the real check. + - *Interval coverage*: for each version, `git log --oneline .. -- plugins/` + and reconcile the entry's bullets against that list. Heading-set equality proves no + release is missing; it does not prove a change landed in the right release, and + misattribution is the likelier error. + - **Both checks re-run after step 5.2's WIP commit**, not just the heading one: until + that commit exists, 0.4.1's interval has no closing commit, so its entry cannot be + reconciled against the plugin changes it actually ships. If the post-commit + reconciliation changes the CHANGELOG, stage it, amend the WIP commit (WIP `-m`), and + repeat both history checks before running the battery. +2. `AGENTS.md`: + - invariant **12** under Packaging (stable global number; nothing renumbered), quoting + step 0's list verbatim; + - architecture tree gains `scripts/check-version-bump.sh`, its `.test.sh`, and the + CHANGELOG; + - § Boundaries executables sentence; + - § Commands: `lint` and `quality` gain the new files; **`test` stays hook-only** + (it names the hook's state-machine suite; the checkers' suites live with the + invariant checks). Plus a **separate** row for invariant 12 + (`invariant check (12 version bump)`) rather than folding the suite into the + existing `invariant checks (5 pinning, 6 manifest)` row. Separate, because + invariant 12's checker takes an argument and carries a commit-derived precondition + that rows 5/6 do not — merging them would have hidden that precondition inside a + row that has none. The row states the precondition explicitly; + - invariant 7 wording; the MANIFEST.md description line. +3. `docs/architecture.md`, `README.md` § Contributing — executables and CI shape. +4. `plugins/dev-workflow/examples/README.md` — the "read, don't install" clarification. +5. `docs/hardening-taxonomy.md` — mint `artifact-version-not-bumped` with alias hints. +6. `docs/hardening-log.md` — re-read, run the anchored grep, append one row, **then run + the anchored grep again** and confirm exactly one `artifact-version-not-bumped` row + with the intended fields. An append is a write that can go wrong; checking only + beforehand checks the wrong moment. + +**4a. Three-way identity check.** After the header, the invariant and the ledger `ref` +all exist, diff each extracted block against `$SCRATCH/limitations.txt` (step 0's +absolute session scratchpad path, not a repo-relative one) and record the +result. This list already drifted once inside the spec itself. + +**4b. Prompt-standards item 11 review of the new invariant text**, recorded — the spec +assigns this deliberately (a voluntary review, not invariant 11 applicability). Every +enforcement claim in invariant 12 must name its mechanism (the checker), its event scope +(pull requests only), and carry the complete limitation list. + +**Verify:** run the AGENTS.md-mandated grep +(`grep -rniE 'declare[sd]?|convention[- ]load' --include='*.md' . | grep -v source-files/`) +**before** editing AGENTS.md/`docs/architecture.md`, and read the hits rather than +trusting the count. + +## Step 5 — bump, snapshot, verify, Gate B + +The checker reads **commits**, so anything run before a commit exists measures the wrong +tree. Sequence is load-bearing. + +1. `plugins/dev-workflow/.claude-plugin/plugin.json` → `0.4.1`. +2. **WIP commit, staged explicitly and audited.** Three of the artifacts are new + untracked files, so `git commit -a` would silently omit them and leave the battery + passing from the working tree while Gate B and CI saw nothing: stage the intended path + set by name, inspect `git diff --cached --stat` and `git status --short`, commit as + `WIP: version-bump check` (the `wip` prefix keeps the hook from reading it as a + cycle-closing commit and discarding the pass counters), then confirm no intended + change remains outside HEAD. +3. **Then** the quality battery — **run the updated AGENTS.md § Commands quality line + verbatim**, not a hand-enumerated approximation (an enumeration already dropped + `sh scripts/check-invariants.sh` once while claiming to be exhaustive). One run, + output kept. +4. **Repository-level negative proof, committed rather than working-tree.** Record the + pre-proof tree id (`git rev-parse HEAD^{tree}`). Edit the manifest to 0.4.0, + **`git add plugins/dev-workflow/.claude-plugin/plugin.json`** — an amend commits the + *index*, so an unstaged edit leaves the amended tree still bumped and the "rejection" + run exits 0, proving nothing while looking like proof — then + **`git commit --amend -m "WIP: version-bump check"` (every amend carries the WIP + `-m`**, never `--no-edit`: the hook classifies from the commit command text, so a bare + amend reads as a real commit, closes the Gate-B cycle and emits a premature STOP). + Audit the committed manifest value (`git show HEAD:`) before trusting the + run. Same staging discipline on the way back. Run + `sh scripts/check-version-bump.sh main` → must exit 1 naming `dev-workflow`. Amend back + to 0.4.1 (same WIP message) → run again → must exit 0. Then **require the restored tree + id to equal the recorded one** and re-run the full battery: two history rewrites sit + between the battery and the final commit, and only the version checker would notice a + tree that came back subtly different. +5. **Gate B**, per CLAUDE.md §5 — the full loop, not its floor: + - Call: `mcp__codex__review` with `instruction` (the original task), + `whatWasImplemented` (the checker, suite, CI wiring, prose, bump), `baseSha` = the + WIP commit's parent, `headSha` = HEAD, `reviewType: full`, and `additionalContext` + carrying: check against AGENTS.md; report **every** finding with severity and + confidence; the one-line format + `MAJOR | high | file:line | what | consequence | fix`; and the literal `NO FINDINGS` + when clean. + - Validate each finding; fix Blocker/Major; collect Minor/Nit without iterating. + - **After each accepted fix: stage it and amend the WIP commit** (with the WIP `-m`), + then re-review the new range. `review` reads the committed range, so re-reviewing + without amending re-reads the stale snapshot and "passes" on unfixed code. + - **Re-run the affected checks after each fix, and the full quality command before the + final clean pass** — a review-driven change can invalidate the green battery. + - Minimum three passes; the only early exit is a pass with zero findings; **pass 3 is + not terminal while Blocker/Major remain** — continue until clean or clearly stuck. + - A codex call that dies at the MCP tool-call timeout is **retried once** before + surfacing; pass state lives in `.context/`, so nothing is lost. + - **Specified-behaviour changes:** the spec is tracked at + `docs/superpowers/specs/2026-07-19-plugin-version-bump-check.md` and this plan + beside it, following the convention the two 2026-07-18 documents established. + (An earlier revision of this plan asserted the repo tracks no spec file; that was + wrong, found while running the drift grep, and corrected here.) Any Gate-B fix that + changes specified behaviour updates that spec **and** the tracked artifacts encoding + the same decisions — AGENTS.md invariant 12, the script header's limitation list, + the ledger `ref` — in the same amended commit, so the next pass reviews a consistent + set. + - **If stuck:** `git reset --soft` back to the WIP commit's parent — index and working + tree preserved, nothing lost — then stop and surface the unresolved findings. + **Not** an amend to a real message: that is the cycle-closing move reserved for a + clean final pass, and using it here would have the hook record a completed cycle, + discard the pass counters, and leave a publishable-looking commit carrying + unresolved Blocker/Major findings. +6. `git commit --amend` to the real message (the one non-WIP amend, closing the cycle); + push; open the PR. + +## Step 6 — PR + +1. **Always** run `/dev-workflow:process-pr-review` — not only "if a bot comments". + CodeRabbit may still be pending and Greptile can post late; the command owns the + waiting and routing per `docs/pr-review-bots.md`, and merge-readiness is its output, + not an inference from a quiet PR. +2. **Confirm the new CI step actually executed.** A skipped step leaves the job green, so + a green PR does not prove the PR-only check ran. Read that step's conclusion and log on + the PR head and confirm it invoked the checker with the expected `origin/` + argument. This — not the job colour — is the acceptance test for step 3, which has + never run in a real `pull_request` context before this PR. +3. Merge when CI is green, step 6.2 is confirmed, and bot processing is complete. + +## Risks + +- **Step 3 is unverifiable locally.** Failure mode is a red PR or a visibly skipped step, + both caught by 6.2 — not a silent pass. +- `fetch-depth: 0` on the runner: ~50 commits, negligible. +- The suite's `git` wrapper escaping its case: written into that case's temp dir, `PATH` + exported only inside the subshell that runs the checker. +- The mutation leaving the checker neutered: guarded by the `cmp`-against-pristine restore + check in 2.4 and the re-run in 2.5. +- Repeated WIP amends losing work: guarded by the tree-id comparison in 5.4. diff --git a/docs/superpowers/specs/2026-07-19-plugin-version-bump-check.md b/docs/superpowers/specs/2026-07-19-plugin-version-bump-check.md new file mode 100644 index 0000000..0c4b7fc --- /dev/null +++ b/docs/superpowers/specs/2026-07-19-plugin-version-bump-check.md @@ -0,0 +1,438 @@ +# Spec — CI check: a plugin change requires a version bump + +## Problem + +Changes to `plugins/dev-workflow/**` can merge without changing the plugin's +`version`. An installed copy lives under a version-keyed cache path, so an un-bumped +change never propagates: the machine keeps running the old code with no signal that +it is stale. + +Evidence (source `manual`, severity `major`): +1. A machine ran plugin 0.1.0 for days while main was at 0.3.0/0.4.0 — bumps only + happened when someone remembered to ask. +2. The 0.4.0 bump had to be requested during PR #4 review rather than falling out of + the process. +3. (Confirmed against git while writing this spec) main is *already* stale: `fe5e296` + and `793e234` changed `plugins/dev-workflow/commands/{process-pr-review,workflow-init}.md` + after the 0.4.0 bump, with no bump of their own. + +The rule "plugin change ⇒ version bump" exists only as convention. Nothing checks it. + +## Rule + +A pull request that changes any path under `plugins//`, **where that directory +still exists at HEAD**, must also change `plugins//.claude-plugin/plugin.json`'s +`version` field to a value different from the one at the merge-base with the PR's base +branch. If the directory survives but the manifest does not, the run **fails closed** — +a plugin that still ships with no version to key on is a defect, not an exemption. + +The "exists at HEAD" clause is the deletion carve-out, and it is about the **directory** +only: a plugin whose whole directory is removed is not enumerated at all, so it has no +version left to bump. **Deleting just the manifest, while the directory keeps shipping, +fails closed** — revised here after Gate B raised it twice. The original carve-out +covered both on the reasoning that a missing manifest leaves nothing to compare; that +was wrong in the case that matters, because the directory's content still reaches users +and "no exemptions" was false while it stood. Removing a plugin is a marketplace-level change — the entry +disappears from `.claude-plugin/marketplace.json` — and this check has nothing to say +about it. **Residual gap, accepted and documented:** deleting a plugin does not force +a bump anywhere, so already-installed copies of the deleted plugin keep working from +their cache. That is out of scope here; no mechanism in this repo addresses it today. + +## Scope decision (explicit) + +**Every path under `plugins//` counts. No exemptions — including `examples/`.** + +Rationale, worded so it does not collide with invariant 7: everything under the +plugin directory is part of the **package a user receives** when the plugin is added +or updated — the cache copies the directory wholesale. Invariant 7 says something +different and still holds: `examples/` is never *installed into a user's project*, +never copied by a command, never presented as a default. Both are true — it ships, +and it is not scaffolded. A stale shipped example is the same propagation failure as +stale skill text. + +The second reason is the one that actually decides it: any exemption list requires the +check to judge which paths "matter", and that judgment is precisely what failed as a +convention. + +`plugin.json` is itself under `plugins//`, so a bump-only commit trivially +satisfies the rule — the changed file *is* the version. + +## Mechanism (rung 2 — mechanical check in CI) + +New `scripts/check-version-bump.sh`, alongside `scripts/check-invariants.sh`, in the +same style (POSIX `sh`, no `jq`, names every offender rather than the first, header +comment stating its own limits) with a companion `scripts/check-version-bump.test.sh` +of reject/accept pairs. + +Invocation: `sh scripts/check-version-bump.sh ` — **exactly one argument**. +Zero or more than one prints a usage line and exits non-zero, rather than dying on +`set -u` or silently ignoring the extras. + +The script `cd`s to its own repository root first (`cd "$(dirname "$0")/.."`, as +`check-invariants.sh` does): the `plugins/` pathspecs below are relative, so running it +from a subdirectory would otherwise enumerate nothing and exit clean — a false pass. + +Every git invocation uses `git --literal-pathspecs`. `--` stops *option* parsing but +does not disable pathspec magic, so a directory name containing `*`, `?`, `[…]` or a +leading `:` would still be interpreted as a pattern. + +1. Resolve `mb=$(git merge-base HEAD)`. **Fail closed** (exit non-zero, + diagnostic naming the ref) if `` does not resolve or there is no + merge-base — an unavailable base is an operational error, not a clean tree. +2. Enumerate the plugin directories **from HEAD's tree** + (`git ls-tree -d --name-only HEAD plugins/` — `-d`, because a plain `ls-tree` lists + blobs too, and a stray `plugins/notes.md` would then be run through the + directory-name grammar or, worse, processed as if it were a plugin), never from a + `plugins/*/` filesystem + glob: a dirty local checkout that deleted a plugin directory would otherwise hide a + committed un-bumped change, a false local pass CI cannot reproduce. **The + enumeration's own exit status is checked** — output and status captured separately — + and a non-zero status fails closed. An unchecked enumeration that errors returns an + empty list, and an empty list means "no plugins changed", i.e. a false clean exit + before any of the per-plugin fail-closed branches can run. + + **Every enumerated directory name must match `[A-Za-z0-9._-]+`; anything else fails + closed**, naming the offending entry. This is a constraint on the repo, not a + limitation of the checker: plugin directory names are ours to choose, and a name with + a space, quote, backslash, newline or leading `:` is not something to support here. + The alternative is worse — `git ls-tree --name-only` emits *C-quoted* pathnames for + exotic names, so a line-oriented loop would feed a quoted display form back to git as + a literal path, `diff` would report that non-existent path unchanged, and the plugin + would be **skipped silently**. A false skip is the dangerous direction; a name the + loop cannot round-trip must stop the run, not slip through it. (POSIX `sh` cannot + read NUL-delimited input, so `-z` is not an option.) + + For each enumerated plugin directory: + - `git diff --quiet "$mb" HEAD -- "plugins//"`. Exit 0 = unchanged, skip; + exit 1 = changed, continue; **any other exit = operational error → fail closed**. + - **Fail closed** if `plugins//.claude-plugin/plugin.json` is absent **from + HEAD's tree** while the directory itself is present and changed: the content still + ships and nothing keys it. (An earlier revision skipped here; that was the + manifest-only-deletion hole Gate B found.) Tested with + `git ls-tree HEAD -- `, whose + states are distinguishable: non-zero exit = error (fail closed), zero exit with + empty output = genuinely absent. `git cat-file -e` cannot be used for this: it + returns 128 both for an absent path and for a lookup failure, so "new plugin" and + "broken object database" would be indistinguishable — and the safe direction is + to fail, not to pass with an empty base version. + - Read the version at HEAD (`git show "HEAD:"`) and at the merge-base + (`git show "$mb:"`). **Both sides are read from commits, never from the + working tree** — otherwise a dirty local checkout could supply an uncommitted + bump for a committed change, giving a local result CI cannot reproduce. + **Each `show`'s output and exit status are captured separately and a non-zero + status fails closed with a read-error diagnostic** — a partial read that still + contains a parseable `"version"` must not be accepted as the version, and a + genuine read failure must not be misreported as a malformed manifest. + - The manifest missing *at the merge-base* is the legitimate "new plugin" case, + established the same way (`git ls-tree "$mb" -- `, empty output = + absent) rather than inferred from `git show` failing. New plugin → base version + empty → any version passes. **Identity is the directory path, full stop** — see + the rename entry under "does NOT catch", and the note below on why matching by + manifest `name` was tried and rejected. + - Version extraction, in two steps: match with + `grep -o '"version"[[:space:]]*:[[:space:]]*"[^"]*"'`, no `jq`, then **strip the + key, colon and surrounding whitespace so only the quoted value is compared.** + Comparing the raw matched fragment would make `"version": "0.4.0"` and + `"version":"0.4.0"` differ — a reformat of the manifest would read as a bump and + let an un-bumped plugin change pass, which is the exact failure being hardened. + Applied to **both** sides. **Exactly one match required in every manifest that + exists** — the absent merge-base manifest of a genuinely new plugin is the one + exemption, and it is established by `ls-tree` before extraction is attempted, not + by extraction returning nothing. Zero or several matches is a fail-closed error + (an ambiguous manifest is not a checked manifest). This is a line-oriented grep, + not a JSON parser, so it compares the *spelling* of the value, not the parsed + value. **A version value containing a backslash fails closed**, one line that + removes the whole escape-equivalence class: a value spelled with a `\uXXXX` + escape (e.g. `0.4.\u0030`) parses as `0.4.0` but + spells differently, so without the guard a manifest could "change version" while + the plugin's actual version stood still — the failure being hardened, wearing a + JSON escape. (An escaped *key* — `\u0076ersion` — needs no separate guard: the + grep finds zero matches and the exactly-one rule already fails closed.) + Three further ceilings, all stated in the header comment: a nested + `"version"` key elsewhere in the manifest is matched too, and a manifest that + splits the key, colon and value across lines — valid JSON — yields zero matches + and **fails closed** rather than passing. Failing a reformatted manifest is the + safe direction and the fix is to put the pair back on one line, but a reader + should not have to discover that from a diagnostic. + - Equal versions → record a failure naming the plugin and both versions. +3. **Accumulate across plugins**, print every offender, exit 1 once — matching + `check-invariants.sh`'s "prints every offending line, not just the first". + +## CI wiring + +- A step gated on `if: github.event_name == 'pull_request'`. The base ref reaches the + script through an `env:` value, **never interpolated into the shell source**: + `env: { BASE_REF: "${{ github.base_ref }}" }` and + `run: sh scripts/check-version-bump.sh "origin/$BASE_REF"`. Git ref names may contain + `$`, backticks, quotes and semicolons; direct `${{ }}` interpolation would splice a + branch name into the generated run script as code rather than passing it as data. +- The existing `actions/checkout` step gains `fetch-depth: 0`, so the base branch and + a merge-base exist at all (default depth 1 has neither). The repo is small. +- The **shellcheck step gains both new files**, `--shell=sh` and **no exclusion for + either**. The existing suites carry `--exclude=SC2015` for their + `[ c ] && pass || fail` lines; this suite has none of those, so every rule applies + as-is. (Written as "matching the existing pair" in an earlier revision; corrected to + match what shipped, since an unnecessary exclusion is a weakened lint.) Same pinned + `koalaman/shellcheck:v0.11.0` image; no new action, nothing new to pin. + Note on attribution: invariant 4 sits under **Hook** and is about the hook running on + machines we do not control. It does not reach a repo-local CI script, so neither the + script's header nor the new invariant may claim invariant 4 covers it. The POSIX-`sh` + requirement for this checker is stated in its own right (it matches + `check-invariants.sh`, and CI invokes both with `sh`). +- The **test suite runs unconditionally**, in the existing invariant-checks step: it + builds throwaway git repos in `mktemp -d` and needs no PR context. + +**On push to main: skipped, deliberately.** A push to main has no PR base to diff +against. The push event *does* carry a before/after range that could be diffed, so this +is a scope choice, not an impossibility: on main the work has already been gated by its +PR run, and re-checking it there would mostly re-report the same verdict. (An earlier +revision justified this by claiming a push-range check "would fail every direct-to-main +commit" — that is simply false: a docs-only commit or a correctly bumped one would pass. +Reviewed out rather than left standing, since a wrong reason for a right decision is how +the decision gets reversed later for the wrong reason.) The assumption — stated rather +than implied — is that plugin changes arrive through PRs; a direct push is unchecked. + +## What this check does NOT catch (honest-claim rule, prompt-standards item 11) + +Stated in the script's header comment, in the AGENTS.md invariant, and in the ledger +`ref`: + +- **Semantic correctness of the bump.** It verifies a bump is *present*, not that it is + *right*: a patch bump where a minor was due passes. +- **Direction.** Any different string passes, including a decrease (`0.4.0` → `0.3.9`). +- **Concurrent PRs colliding on the same new version.** Two PRs branched from 0.4.0 + can each bump to 0.4.1 and each pass; after the first merges, the second still + carries a version already published. The check cannot close this — it only ever sees + its own merge-base. What closes it is the repo setting "require branches to be up to + date before merging", which forces the second PR to rebase and re-run against the + merged 0.4.1. (A merge queue is *not* offered as the fix: a required check would need + `merge_group` wiring this workflow does not have, and a multi-PR group could still + contain two identical bumps.) Documented, not fixed, and not claimed to be fixed by + configuration this repo has not enabled. +- **Anything outside a `pull_request` event** — a commit pushed directly to main + bypasses the check entirely. +- **Deletion of an entire plugin directory** — see the rule's carve-out. Deleting only + the manifest is *not* in this list: that fails closed. +- **Any change to *which directory* the marketplace entry points at** — a rename, a + copy, or repointing `source` in `.claude-plugin/marketplace.json` at a different + existing directory. The check's notion of identity is the directory path, so the old + path reads as a deletion (carve-out) or as unchanged, and the new one as a newly added + plugin: the delivered plugin changes while an unchanged version passes. The rename is + only the most obvious member of this class, and the invariant, the script header and + the ledger all state it as the class, not as "renames". + + This was *specified as closed* in an earlier revision — matching manifest `name` + across the two trees — and then deliberately reopened. Identity-by-`name` needs its + own cross-tree enumeration with its own error handling, an exactly-one normalized + `name` extraction on every candidate manifest, and defined behaviour for duplicate + names, malformed names, swapped directories and a move onto an occupied path. Review + raised each of those as a fresh defect in the fix. That is a large, ambiguity-laden + mechanism for a case that has never occurred in this repo, in a marketplace that + currently ships exactly one plugin. + + **This is accepted residual risk, not a covered case.** "Bump by hand when you move a + plugin" is an unenforced convention — the same kind of convention whose failure + produced this finding — and nothing here makes a reviewer notice the + `marketplace.json` edit. It is written down so the next person inherits a known gap + instead of a false sense of coverage. Revisit when the marketplace grows past one + plugin, or the first time a move actually happens. +- **Whether the version was released or tagged.** It reads the manifest only. + +## Test matrix (each row is one assertion in `check-version-bump.test.sh`) + +**Policy rejects** (reach the version comparison): +- P1 plugin file changed, version identical +- P2 `examples/` file changed, version identical (pins the scope decision) +- P3 two plugins changed, only one bumped → the unbumped one is named, exit 1 +- P4 two plugins changed, *neither* bumped → **both** names appear, one exit 1 + (a first-failure implementation passes P3 but fails here) +- P7 manifest **reformatted** (`"version":"0.4.0"` — whitespace around the colon + changed, value identical) alongside a plugin change → still rejected; pins that the + comparison is on the value, not the matched fragment +- P8 the committed plugin change is un-bumped and the **working tree** carries the bump + (uncommitted) → still rejected; pins the commit-derived read +- P9 the committed plugin change is un-bumped and the working tree has **deleted** the + plugin directory → still rejected; pins HEAD-tree enumeration over a filesystem glob +- P10 run from a **subdirectory** of the repo with an un-bumped plugin change → still + rejected; pins the repo-root anchoring + +**Operational rejects** (fail closed before any comparison): +- O1 base ref that does not resolve → diagnostic names the ref +- O2 valid ref with no merge-base (unrelated histories) → diagnostic names the cause +- O3 HEAD manifest with no `version` field +- O4 HEAD manifest with two `version` matches +- O5 merge-base manifest with no `version` field +- O6 merge-base manifest with two `version` matches +- O7–O13 **git itself failing**, one row per load-bearing call — the full list, since a + call without a row is a fail-closed branch nobody proved: `merge-base` resolution, + directory enumeration (`ls-tree` over `plugins/`), the change test (`diff --quiet`, + exit >1), the **HEAD** manifest presence lookup, the **merge-base** manifest presence + lookup, the HEAD manifest read (`show`), the merge-base manifest read. + Exercised with a `git` wrapper earlier on `PATH`. + **The wrapper matches on the full argument vector, not the subcommand alone** — the + enumeration and both presence lookups are all `ls-tree`, so a subcommand-only wrapper + would trip on the first one and never reach the branches O9–O11 claim to cover, while + the suite still went green. Each row asserts its own diagnostic, so a wrapper that + fires at the wrong call is visible. + Without these rows, an implementation that collapses "git errored" into "absent" or + "unchanged" satisfies the whole rest of the matrix — which is exactly the fix + revision 3 made, left unprotected. +- O14 invoked with no argument, and O15 with two → usage diagnostic, non-zero +- O15b a version value carrying a `\uXXXX` escape (`0.4.\u0030`, which parses as + `0.4.0`) → fail closed, rather than counting as a bump away from `0.4.0` +- O18 the manifest alone deleted while the directory and its changed content remain → + fail closed ("no version to key on"). A5 (whole directory deleted) stays accepted; the + asymmetry is deliberate and each row pins one side of it. +- O16 a plugin directory name containing a space → fail closed naming it (rejected by + the grammar; `ls-tree` does *not* quote spaces, so this row alone does not exercise + the quoting path) +- O17 a plugin directory name git actually C-quotes — one containing a tab or a + backslash → fail closed. This is the row that pins the silent-skip regression; O16 + would stay green even if C-quoted names slipped through. + +**Accepts:** +- A1 plugin file changed, version changed +- A2 nothing under `plugins/` changed (docs-only PR), version identical +- A3 bump-only change (manifest is the sole changed file) +- A4 newly added plugin (no manifest at the merge-base) +- A5 plugin directory deleted at HEAD (carve-out; no manifest to bump) +- A7 two plugins changed, both bumped +- A8 version *decreased* (`0.4.0` → `0.3.9`) — pins the documented "direction is not + checked" limitation as tested behaviour rather than an unverified claim +- A9 a plugin directory renamed **with** a bump — pins that the rename gap is a gap in + detection only, and that a renamed-and-bumped plugin is not spuriously rejected +- A10 a **file** directly under `plugins/` whose name violates the directory grammar — + `plugins/release notes.md` — changed, no bump → accepted and ignored. The name matters: + with a grammar-conforming name like `plugins/notes.md`, an implementation missing `-d` + would enumerate the blob, append `/`, diff a path that does not exist, report + "unchanged" and pass anyway. The spaced name is the one that fails loudly without `-d`. + +**Mutation verification** (the exact mutation, so the claim is reproducible): replace +the version comparison `[ "$base_ver" = "$head_ver" ]` with `false`, so the check can +never report a stale version. Expected: **every P row fails and nothing else** — the O +rows never reach the comparison and must stay green, and no accept row changes. The +suite reports policy and operational rejects as separate groups so an ineffective +mutation cannot look convincing. + +## Prose half + +1. `plugins/dev-workflow/CHANGELOG.md` — one entry for **every version the manifest + has ever carried in git history**: 0.1.0, 0.2.0, 0.2.1, 0.3.0, 0.4.0, 0.4.1. That + enumeration is the completeness criterion (checkable: the set of distinct `version` + values across `git log -- plugins/dev-workflow/.claude-plugin/plugin.json`). + **Interval definition:** an entry covers every plugin-touching commit *after* the + commit that introduced the previous version, up to and including the commit that + first introduced this one. The 0.1.0 entry, having no predecessor, runs from the + first plugin-touching commit in the repository. **Format:** newest version first, + `## ` headings, a few bullets each, no dates (the manifest carries no + release dates and inventing them would be fiction). The reproducibility claim is + scoped to *commit coverage* — which commits belong to which entry — not to wording. + 0.4.1 needs no special rule: `fe5e296` and `793e234` land after the 0.4.0 commit and + before the 0.4.1 one, so the ordinary interval already claims them. They are worth + calling out in the entry's text only because they are the un-released plugin changes + this very check exists to prevent. + Each entry is verified + against `git log`, not written from memory. +2. **AGENTS.md → Key invariants → Packaging, as invariant 12** — *not* "the next + number in the section": Packaging currently ends at 7 and "Prompts and scaffolding" + runs 8–11, which other documents cite by number. Invariant numbers are stable IDs, + not an ordering, so the new rule takes the next *global* number and renumbers + nothing. Its wording must name the PR-only scope, the checker, what "the plugin" + means for identity (the directory path), and **the accepted-limitation list in + full** — an unqualified "plugin changes require a bump" would be exactly the + `unverified-enforcement-claim` this repo already logged once, and a *partial* list + is the same defect wearing a shorter sentence. + + **One list, three places, identical wording.** The script header, the AGENTS.md + invariant and the ledger `ref` all carry it: + semantic correctness · direction · PR-only (direct pushes bypass) · plugin deletion · + *any change to which directory the marketplace entry points at (rename, copy, or + `source` repoint)* · concurrent same-version collisions · release/tag status. + Two of these were dropped from the invariant and the marketplace-relocation class was + narrowed back to "rename" in an earlier revision — which is how a limitation list + quietly becomes an overclaim. +3. **Drift sweep** — this change adds two executables and a fifth CI check, so every + claim about "the only executables" or the shape of CI goes stale at once. Grep and + update: AGENTS.md § Boundaries and its architecture tree (plus the new CHANGELOG + file), `docs/architecture.md`, `README.md` § Contributing, and the explanatory + comment in `.github/workflows/ci.yml`. + Also in the sweep, because this change leans on the distinction: **invariant 7's + wording** ("`examples/` is read-only reference … never installed") and the matching + architecture shorthand ("read, don't install") are ambiguous between "not scaffolded + into the user's project" (what is meant) and "not part of the shipped package" (not + true — the cache copies it). Clarify to *"never scaffolded or copied into a user's + project, never presented as a default; it does ship inside the plugin package"*, + without weakening the read-only prohibition. **`plugins/dev-workflow/examples/README.md` + is in that sweep too** — it is titled "read, don't install" and carries the same + ambiguity, and it is a shipped file, so leaving it behind would be drift inside the + package itself. + **Before editing AGENTS.md or `docs/architecture.md`, run the grep AGENTS.md's own + Don'ts mandate** — `grep -rniE 'declare[sd]?|convention[- ]load' --include='*.md' . | + grep -v source-files/` — and read the hits rather than trusting the count. This + change edits both of the documents that rule was written about, which is exactly + when an ad-hoc sweep repeats the 0.2.1 failure. +4. **AGENTS.md § Commands** — the `lint` and `quality` rows gain the new files. The + `test` row stays hook-only: it names the hook's state-machine suite, and the two + checkers' suites belong with the invariant checks, not with it. Invariant 12 gets its + **own** row rather than joining the existing `invariant checks (5 pinning, 6 + manifest)` row, because its checker takes an argument and carries a commit-derived + precondition that rows 5/6 do not — folding it in would bury that precondition in a + row that has none. (Both details are corrections to an earlier revision, made after + review found the tracked spec describing something other than what shipped.) + The PR-only checker's row carries + the local invocation (`sh scripts/check-version-bump.sh main`) and a note that CI + passes the PR's base ref instead. **That row also states its precondition**, or it + gives false assurance: the checker compares *commits*, so run it after the work is + committed (the WIP commit of the Gate-B cycle is the natural point) and against a + base ref that is actually current. Run mid-loop with the plugin edits still in the + working tree, it reports clean — correctly, and uselessly. **The quality row — which claims to be "the whole + battery, what CI runs" — includes that invocation too**, or it stops being equivalent + to CI the moment this check lands. Per the repo's own rule, every command listed must + have been run in the session that documents it. + +## Ledger + +One row, appended after re-reading the log and re-running the anchored recurrence grep +(`grep -nE '^\| *[0-9-]{10} *\| *artifact-version-not-bumped *\|' docs/hardening-log.md`). +Fields: `source=manual`, `severity=major`, `rung=2 lint`, `ref` naming +`scripts/check-version-bump.sh` + `.test.sh` **and stating what it does not catch** — +the same seven-item list as the invariant and the script header, verbatim, since the +ledger outlives this spec. + +Neither taxonomy has a fitting class: `dependency-unpinned` is about *this* repo +consuming a floating dependency; this is about *our own artifact* failing to reach +consumers. Mint `artifact-version-not-bumped` in `docs/hardening-taxonomy.md` +(project-owned, per the skill), with alias hints. + +## Invariant 11 (prompt-standards) applicability — decided, not assumed + +**Invariant 11 does not formally apply to anything in this change.** Its stated scope +— and `docs/prompt-standards.md`'s — is skills, commands, agent definitions, hook +messages and scaffolded templates. **No file under +`plugins/dev-workflow/{skills,commands,agents,hooks}/` is touched here.** The two shell +files are executables; `CHANGELOG.md`, `README.md` and `docs/architecture.md` are +explanatory prose. + +The new AGENTS.md invariant text nonetheless gets a **voluntary** review against item +11 (honest enforcement claims) — AGENTS.md is read directly by Codex at both gates, so +an overclaiming invariant misleads a reviewer the same way a bad prompt does. Calling +that *applicability* would silently widen invariant 11's scope to AGENTS.md, including +in the scaffolded copy every initialized project gets; widening it is a separate +decision, not a side effect of this change. + +`MANIFEST.md` is deliberately *not* given a CHANGELOG row: its actual content is an +inventory of `source-files/`, the frozen extraction seed, and the new CHANGELOG has no +seed provenance. But AGENTS.md currently describes MANIFEST.md as "what each shipped +file is and where it comes from", which is what made this look like drift in the first +place — **that one-line description is corrected to name it the extraction-seed +inventory in this change**, since leaving the architecture doc contradicting the file +it describes is precisely the docs-drift the sweep exists for. + +## Verification + +- `sh scripts/check-version-bump.test.sh` — the matrix above, plus the named mutation. +- The full quality battery from AGENTS.md § Commands (which already ends in + `claude plugin validate . --strict`), run once, after the battery itself is updated + to include the new files. +- The check must pass on its own PR — which requires the 0.4.1 bump, making this PR its + own first live test case. diff --git a/plugins/dev-workflow/.claude-plugin/plugin.json b/plugins/dev-workflow/.claude-plugin/plugin.json index 8dd0038..5b0ceb2 100644 --- a/plugins/dev-workflow/.claude-plugin/plugin.json +++ b/plugins/dev-workflow/.claude-plugin/plugin.json @@ -1,7 +1,7 @@ { "name": "dev-workflow", "displayName": "Cross-Model Review Workflow", - "version": "0.4.0", + "version": "0.4.1", "description": "Spec-driven workflow with two independent cross-model review gates, an append-only hardening ledger with an escalation ladder, and repo-enforced quality. Requires the superpowers plugin.", "author": { "name": "Daniel Sänger", diff --git a/plugins/dev-workflow/CHANGELOG.md b/plugins/dev-workflow/CHANGELOG.md new file mode 100644 index 0000000..ce8614d --- /dev/null +++ b/plugins/dev-workflow/CHANGELOG.md @@ -0,0 +1,79 @@ +# Changelog — dev-workflow + +Every version the plugin manifest has carried, newest first. Entries are written from +`git log` over `plugins/`, not from memory. + +An entry covers the plugin-touching commits after the previous version's introducing +commit, up to and including the commit that introduced this one. There are no dates: +the manifest records versions, not release dates, and inventing them would be fiction. + +**Bumping is checked**, as of 0.4.1: `scripts/check-version-bump.sh` fails a pull +request that changes a path inside an existing `plugins//` directory without +changing that plugin's manifest version. Before that it was a convention, and it was +missed twice. + +It is a check, not a guarantee, and the difference is written down rather than glossed: +it verifies a bump is *present*, not that it is correct, and it says nothing about an +entire plugin directory being deleted, a file sitting directly under `plugins/`, a +plugin directory being renamed or repointed, or a commit pushed straight to main. A +newly added plugin needs no bump — there is no earlier version to differ from — but it +is not unchecked: a missing manifest, or one whose `version` cannot be read +unambiguously, still fails. Deleting only a plugin's *manifest* while the directory +keeps shipping fails too. +AGENTS.md invariant 12 carries the complete list. + +## 0.4.1 + +- **A plugin change now requires a version bump**, checked in CI on every pull request + (`scripts/check-version-bump.sh` + its suite). This entry exists because the rule was + broken before it was written: 0.4.0 shipped, and then two commits changed the plugin + without a bump — see the two items below, released here for the first time. +- `commands/process-pr-review.md`, `commands/workflow-init.md`: the enforcement-claim + checklist item, and the docs-drift escalation to rung P (`fe5e296`, merged as #5). +- `commands/process-pr-review.md`, `commands/workflow-init.md`: four PRs of observed + Greptile behaviour recorded, plus an opportunistic bot category (`793e234`). + +## 0.4.0 + +- `agents/finding-triage.md`: new read-only PR-comment checker, convention-loaded, that + validates one reviewer claim against the code (`b952d95`, #4). +- `commands/process-pr-review.md`: delegates each claim to that agent after its + instruction-path precheck. +- `skills/harden-finding/SKILL.md`: taxonomy and ladder refinements. +- `commands/workflow-init.md`: scaffolds the mechanized invariant checks; the Codex MCP + preflight now names the cause it can actually diagnose (`bb672eb`, #2). +- `commands/workflow-init.md`: the scaffolded CI template pins `actions/checkout` and + `actions/setup-node` to v7.0.0 SHAs (clearing the Node 20 deprecation), sets + `persist-credentials: false`, and ships verified SHAs in its commented pnpm/node + example instead of `` placeholders (`46366a0`, #3). + +## 0.3.0 + +- `hooks/codex-gate.sh` and its suite, `commands/workflow-init.md`: the changes from + self-initializing this repo with the workflow it ships (`e15d480`, #1) — the plugin's + own gate applied to the plugin's own repo, which is where several of these findings + came from. + +## 0.2.1 + +- `.claude-plugin/plugin.json`: dropped the `hooks` key. `hooks/hooks.json` is loaded by + convention, so declaring it too was a duplicate-hooks error and **the plugin did not + load at all**. Now invariant 6 (`17b87fd`). + +## 0.2.0 + +- `hooks/codex-gate.sh`: Gate-B validity is derived from working-tree **content**, not + from events, so a file changed through Bash no longer leaves a stale ✓ standing. +- `hooks/codex-gate.sh`: counts Codex passes from any MCP server, or says why it cannot; + stays out of projects that never adopted the workflow. +- `commands/workflow-init.md`: checks every prerequisite and reports what is missing; + tri-state Codex preflight with an honest degraded mode. +- `.claude-plugin/plugin.json`: dropped the redundant `skills` and `commands` keys — + both load by convention. +- `skills/intake/SKILL.md`: wording fixes. + +## 0.1.0 + +Initial plugin (`a05976a`): the `intake` and `harden-finding` skills, the +`workflow-init` and `process-pr-review` commands, the `codex-gate` hook with its test +suite, and `examples/` as read-only reference material. diff --git a/plugins/dev-workflow/examples/README.md b/plugins/dev-workflow/examples/README.md index 1b414b6..ecdd331 100644 --- a/plugins/dev-workflow/examples/README.md +++ b/plugins/dev-workflow/examples/README.md @@ -1,8 +1,10 @@ -# Examples — read, don't install +# Examples — read, don't scaffold -Nothing in this directory is plugin content. These files are **one project's** -answers to questions the plugin deliberately leaves open, kept because the *shape* of -a good answer is hard to describe and easy to show. +These files **ship** with the plugin: they arrive with every install, because the cache +copies the plugin directory wholesale. What they are not is *project* content — nothing +here is ever scaffolded or copied into your repo, and none of it is offered as a +default. They are **one project's** answers to questions the plugin deliberately leaves +open, kept because the *shape* of a good answer is hard to describe and easy to show. They are Convex + TypeScript + pnpm specific. Copying them into another stack will not work, and `/dev-workflow:workflow-init` will not install them. diff --git a/scripts/check-invariants.sh b/scripts/check-invariants.sh index 6f20d6c..96be1f8 100755 --- a/scripts/check-invariants.sh +++ b/scripts/check-invariants.sh @@ -7,7 +7,9 @@ # duplicate-hooks manifest key stopped the plugin loading entirely (0.2.1). A rule a # reader has to remember is worth less than one that fails the build. # -# POSIX sh, no jq (invariant 4). Prints every offending line, not just the first. +# POSIX sh, no jq — this script's own requirement, because CI invokes it with `sh`. +# (NOT invariant 4: that one is about the hook running on machines we don't control.) +# Prints every offending line, not just the first. # Regression suite: scripts/check-invariants.test.sh (run by the same CI step). # # Scope, stated so the gaps are known rather than assumed away: this is a diff --git a/scripts/check-version-bump.sh b/scripts/check-version-bump.sh new file mode 100644 index 0000000..c854cda --- /dev/null +++ b/scripts/check-version-bump.sh @@ -0,0 +1,192 @@ +#!/bin/sh +# A change under plugins// must come with a version bump in that plugin's +# manifest — mechanically, on every pull request. +# +# An installed plugin lives under a version-keyed cache path, so a change that ships +# without a new version never reaches an installed copy: the machine keeps running the +# old code with no signal that it is stale. That happened twice here (a machine sat on +# 0.1.0 while main was at 0.4.0; the 0.4.0 bump itself had to be asked for during PR +# review), and main was ALREADY stale when this check was written — two commits had +# changed the plugin after 0.4.0 shipped. "Bump when you change the plugin" was a +# convention, and conventions are what this repo's hardening ledger is a list of. +# +# WHAT IT DOES NOT CATCH. It verifies that a bump is PRESENT, not that it is right. It +# does not check: semantic correctness (a patch where a minor was due passes); +# direction (any different string passes, including a decrease); anything outside a +# pull_request event (a direct push to main bypasses it entirely); deletion of an entire +# plugin directory; any change to which directory the marketplace entry points at +# (rename, copy, or `source` +# repoint); two PRs branched from the same version each bumping to the same new one; +# and whether the version was ever released or tagged. +# +# Scope is every path under plugins//, with no exemptions — examples/ included, +# because it ships inside the package a user receives (it is never scaffolded into a +# user's project, which is invariant 7 and a different claim). An exemption list would +# make the check judge which paths "matter", and that judgment is what failed as a +# convention. +# +# POSIX sh, no jq — the same constraint as scripts/check-invariants.sh, and for the same +# reason: CI invokes it with `sh`. NOT invariant 4, which is about the hook running on +# machines we do not control. +# +# Version reading is a line-oriented grep, not a JSON parser. Three ceilings, stated so +# they are known rather than assumed away: a nested "version" key elsewhere in the +# manifest is matched too; a manifest that splits the key, colon and value across lines +# is valid JSON but yields no match and FAILS CLOSED; and the comparison is on the +# spelling of the value, not the parsed value, which is why a value containing a +# backslash is rejected outright rather than compared. +# +# Every git call's exit status is checked, and anything unexpected fails closed. A false +# skip is the dangerous direction here: a silent green on exactly the defect this script +# exists to catch. +# +# Regression suite: scripts/check-version-bump.test.sh — run unconditionally in CI's +# invariant-suite step, while this checker itself runs in a separate pull-request-only +# step (it needs a base branch to diff against). +# TESTED SPELLINGS ONLY: extend the fixtures before extending the logic. +set -u + +usage() { + printf 'usage: %s \n' "$0" >&2 + printf ' compares HEAD against the merge-base with \n' >&2 + exit 2 +} +[ "$#" -eq 1 ] || usage + +# The pathspecs below are relative, so a run from a subdirectory would enumerate nothing +# and exit clean — a false pass. `set -u` does not abort on a failed cd, hence the `||`. +cd "$(dirname "$0")/.." || exit 1 + +rc=0 + +# Pathspec magic is not disabled by `--`: a directory named `*` or `[x]` would be read +# as a pattern. Every call goes through this wrapper instead of bare `git`. +g() { git --literal-pathspecs "$@"; } + +# An operational problem — a ref that will not resolve, a git call that errors, a +# manifest we cannot parse — is not a clean tree. Stop; do not fall through to a verdict. +die() { printf '\ncheck-version-bump: %s\n' "$1" >&2; exit 1; } + +# A policy violation, by contrast, is collected: every offending plugin is named before +# exiting, matching check-invariants.sh's "prints every offending line, not just the +# first". A first-failure exit hides the second un-bumped plugin. +fail() { rc=1; printf '\n%s\n' "$1"; } + +# Exactly one "version" match, reduced to the quoted value. Two steps, because comparing +# the whole matched fragment would make `"version": "0.4.0"` and `"version":"0.4.0"` +# differ — a reformat would read as a bump and let an un-bumped change through. +# +# Called via $( ), i.e. in a subshell, so `die` here exits only that subshell: every +# call site adds `|| exit 1` to propagate it. Without that, a fail-closed branch would +# print its diagnostic and then carry on with an empty version. +extract_version() { # $1 = manifest text, $2 = label for diagnostics + ev_matches=$(printf '%s\n' "$1" | grep -o '"version"[[:space:]]*:[[:space:]]*"[^"]*"') + if [ -z "$ev_matches" ]; then + ev_count=0 + else + ev_count=$(printf '%s\n' "$ev_matches" | wc -l | tr -d '[:space:]') + fi + [ "$ev_count" -eq 1 ] || + die "$2: expected exactly one \"version\" field, found $ev_count." + ev_value=${ev_matches#*:} + ev_value=$(printf '%s' "$ev_value" | sed -e 's/^[[:space:]]*"//' -e 's/"$//') + # A backslash means a JSON escape, and an escape makes spelling and value diverge: + # `0.4.\u0030` parses as `0.4.0` but does not compare equal to it, so a manifest + # could "change version" while the plugin's actual version stood still. + case "$ev_value" in + *\\*) die "$2: version value contains a backslash escape; refusing to compare spellings." ;; + esac + printf '%s' "$ev_value" +} + +# `ls-tree` rather than `cat-file -e`: cat-file returns 128 both for an absent path and +# for a lookup failure, so "new plugin" and "broken object database" would be +# indistinguishable — and the safe direction is to fail, not to pass with an empty base +# version. ls-tree separates them: non-zero exit = error, empty output = absent. +tree_has() { # $1 = tree-ish, $2 = path -> 0 present, 1 absent, dies on error + th_out=$(g ls-tree "$1" -- "$2") || die "git ls-tree $1 -- $2 failed." + [ -n "$th_out" ] +} + +base_ref=$1 +mb=$(g merge-base "$base_ref" HEAD) || + die "no merge-base between '$base_ref' and HEAD (unresolvable ref, or unrelated histories)." + +# -d, so a blob directly under plugins/ (a stray notes file) is not run through the +# directory-name grammar or, worse, processed as though it were a plugin. Status is +# captured separately from output: an unchecked failure yields an empty list, an empty +# list means "no plugins changed", and that is a false clean exit before any per-plugin +# check can run. +dirs=$(g ls-tree -d --name-only HEAD plugins/) || + die "git ls-tree of plugins/ at HEAD failed." + +# Newline-only IFS and globbing off, so the loop body runs in THIS shell — `fail` has to +# accumulate into rc, which a `while read` on the right of a pipe (a subshell) could not +# do. Every name is validated against the grammar below before it reaches git. +saved_ifs=$IFS +IFS=' +' +set -f +for dir in $dirs; do + [ -n "$dir" ] || continue + name=${dir#plugins/} + # Plugin directory names are ours to choose, so this is a constraint on the repo, not + # a limitation of the checker — and it has to be enforced rather than assumed: + # `ls-tree --name-only` C-quotes exotic names, and feeding a quoted display form back + # to git as a literal path makes `diff` report a non-existent path as unchanged, so + # the plugin would be skipped in silence. POSIX sh cannot read NUL-delimited input, + # so -z is not an option. + case "$name" in + ''|*[!A-Za-z0-9._-]*) + IFS=$saved_ifs; set +f + die "plugin directory name '$name' is outside [A-Za-z0-9._-]+; refusing to guess." + ;; + esac + + manifest="$dir/.claude-plugin/plugin.json" + + # 0 = unchanged (skip), 1 = changed (continue), anything else = git itself failed. + g diff --quiet "$mb" HEAD -- "$dir/" + case $? in + 0) continue ;; + 1) ;; + *) IFS=$saved_ifs; set +f; die "git diff of $dir/ failed." ;; + esac + + # The directory is still in HEAD's tree and something in it changed, so this plugin + # still ships — but it has no manifest, and therefore no version anything could be + # keyed on. That is not a carve-out, it is a broken plugin: fail closed. + # + # (Deleting a plugin *entirely* is different and is genuinely out of scope: the + # directory is absent from the enumeration above, so it is never reached. The + # asymmetry is the point — a whole plugin going away is a marketplace-level change, + # a plugin that ships without a manifest is a defect.) + # + # This was a carve-out at first, on the reasoning that a missing manifest leaves no + # version to bump. Gate B pushed back twice: the directory survives, its content still + # reaches users, and "no exemptions" in invariant 12 was false while this stood. + tree_has HEAD "$manifest" || + die "$dir changed and still exists at HEAD, but has no $manifest — a shipped plugin with no version to key on." + + head_json=$(g show "HEAD:$manifest") || die "git show HEAD:$manifest failed." + head_ver=$(extract_version "$head_json" "HEAD:$manifest") || exit 1 + + # The base side is read ONLY if it exists — an unconditional read of a known-absent + # manifest would die and turn the new-plugin case into a failure. + if tree_has "$mb" "$manifest"; then + base_json=$(g show "$mb:$manifest") || die "git show $mb:$manifest failed." + base_ver=$(extract_version "$base_json" "$mb:$manifest") || exit 1 + else + base_ver='' # new plugin: differs from any real version, so it passes + fi + + if [ "$base_ver" = "$head_ver" ]; then + fail "$dir changed but its version is still $head_ver (base $mb). + Bump \"version\" in $manifest — an installed copy is keyed by it." + fi +done +set +f +IFS=$saved_ifs + +[ "$rc" -eq 0 ] && printf 'version-bump check: ok\n' +exit "$rc" diff --git a/scripts/check-version-bump.test.sh b/scripts/check-version-bump.test.sh new file mode 100644 index 0000000..be96023 --- /dev/null +++ b/scripts/check-version-bump.test.sh @@ -0,0 +1,373 @@ +#!/bin/sh +# Regression suite for check-version-bump.sh. +# +# The checker is a CI gate, so what matters is not that it passes on a clean tree — it +# is that it FAILS on each violation it claims to catch, does not fail on the legitimate +# shapes next door, and fails CLOSED whenever git itself misbehaves. Three groups: +# +# policy: reaches the version comparison and rejects +# operational: fails closed before any comparison (bad input, broken git, bad manifest) +# accept: must stay green +# +# The grouping is not cosmetic. The mutation check for this suite — replace the version +# comparison with `false` — must make exactly the policy rows fail and leave every +# operational and accept row green. Mixed groups would hide an ineffective mutation. +set -u + +CHECKER="$(cd "$(dirname "$0")" && pwd)/check-version-bump.sh" +pass_n=0; fail_n=0 +pass() { pass_n=$((pass_n + 1)); printf 'ok - %s\n' "$1"; } +fail() { fail_n=$((fail_n + 1)); printf 'FAIL - %s\n' "$1"; } + +work=$(mktemp -d) || work='' +# Abort rather than continue with an empty $work: every path below is built as +# "$work/r", so an empty value turns the fixture reset into `rm -rf /r`. +if [ -z "$work" ] || [ ! -d "$work" ]; then + printf 'FAIL - could not create a temporary directory; refusing to run\n' >&2 + exit 1 +fi +trap 'rm -rf "$work"' EXIT + +MANIFEST=.claude-plugin/plugin.json + +# Identity is passed per-invocation rather than written to config: the suite must not +# depend on, or touch, the developer's global git identity. +gitc() { git -c user.name=t -c user.email=t@example.com -c commit.gpgsign=false "$@"; } + +# A repo with `main` holding one plugin at 0.4.0, and a `feature` branch checked out. +# Every fixture starts here and then diverges. +mkrepo() { + rm -rf "${work:?}/r"; mkdir -p "$work/r/scripts" + cp "$CHECKER" "$work/r/scripts/" + ( + cd "$work/r" || exit 1 + gitc init -q -b main . + mkdir -p "plugins/alpha/$(dirname $MANIFEST)" plugins/alpha/skills + printf '{"name": "alpha", "version": "0.4.0"}\n' > "plugins/alpha/$MANIFEST" + printf 'first\n' > plugins/alpha/skills/s.md + printf 'readme\n' > README.md + gitc add -A && gitc commit -qm base + gitc checkout -qb feature + ) +} + +run() { ( cd "$work/r" && sh scripts/check-version-bump.sh "$@" 2>&1 ); } + +# A rejection must fire for the RIGHT reason: accepting any non-zero exit lets a case +# pass on an unrelated violation. Every reject names at least one expected substring, +# and ALL of them must appear — P4 relies on this to prove that both offenders are +# printed, which a single-pattern contract could not distinguish from a first-failure +# implementation that names only one. +expect_reject() { # $1 = name, $2 = base ref, then one or more required substrings + er_name=$1; er_ref=$2; shift 2 + er_out=$(run "$er_ref"); er_st=$? + if [ "$er_st" -eq 0 ]; then fail "$er_name (exited 0)"; return; fi + for er_pat in "$@"; do + if ! printf '%s' "$er_out" | grep -q "$er_pat"; then + fail "$er_name (missing '$er_pat' in: $(printf '%s' "$er_out" | tr '\n' ' '))" + return + fi + done + pass "$er_name" +} + +expect_accept() { # $1 = name, $2 = base ref + ea_out=$(run "$2"); ea_st=$? + if [ "$ea_st" -eq 0 ]; then pass "$1" + else fail "$1 ($(printf '%s' "$ea_out" | tr '\n' ' '))"; fi +} + +# A `git` earlier on PATH that fails ONE exact invocation and delegates everything else. +# Matching the full argument vector matters: enumeration and both presence lookups are +# all `ls-tree`, so a subcommand-only wrapper would trip on the first one and the later +# branches would never be reached while the suite still went green. +with_failing_git() { # $1 = glob matched against "$*", $2 = name, $3 = expected substring + mkdir -p "$work/bin" + real_git=$(command -v git) + cat > "$work/bin/git" <&2; exit 128 ;; +esac +exec "$real_git" "\$@" +EOF + chmod +x "$work/bin/git" + wfg_out=$( cd "$work/r" && PATH="$work/bin:$PATH" sh scripts/check-version-bump.sh main 2>&1 ) + wfg_st=$? + rm -rf "${work:?}/bin" + if [ "$wfg_st" -eq 0 ]; then fail "$2 (exited 0 despite a failing git)" + elif ! printf '%s' "$wfg_out" | grep -q "$3"; then + fail "$2 (wrong diagnostic: $(printf '%s' "$wfg_out" | tr '\n' ' '))" + else pass "$2"; fi +} + +bump() { # $1 = version, $2 = plugin dir name (default alpha) + printf '{"name": "%s", "version": "%s"}\n' "${2:-alpha}" "$1" \ + > "$work/r/plugins/${2:-alpha}/$MANIFEST" +} + +commit_all() { ( cd "$work/r" && gitc add -A && gitc commit -qm "$1" ); } + +printf '\n--- policy rejects ---\n' + +# P1 — the case the check exists for. +mkrepo +printf 'changed\n' >> "$work/r/plugins/alpha/skills/s.md" +commit_all p1 +expect_reject "P1 plugin file changed, version unchanged" main "plugins/alpha" "0.4.0" + +# P2 — pins the scope decision: examples/ ships in the package, so it is in scope. +mkrepo +mkdir -p "$work/r/plugins/alpha/examples" +printf 'example\n' > "$work/r/plugins/alpha/examples/demo.md" +commit_all p2 +expect_reject "P2 examples/ changed, version unchanged" main "plugins/alpha" + +# P3/P4 — two plugins. P4 is the accumulation proof: a first-failure implementation +# passes P3 and fails P4. +mkrepo +mkdir -p "$work/r/plugins/beta/.claude-plugin" +printf '{"name": "beta", "version": "1.0.0"}\n' > "$work/r/plugins/beta/$MANIFEST" +commit_all seed-beta +( cd "$work/r" && gitc checkout -q main && gitc merge -q feature && gitc checkout -q feature ) +printf 'x\n' >> "$work/r/plugins/alpha/skills/s.md" +printf 'y\n' > "$work/r/plugins/beta/note.md" +bump 0.5.0 alpha +commit_all p3 +expect_reject "P3 two plugins changed, only one bumped" main "plugins/beta" + +mkrepo +mkdir -p "$work/r/plugins/beta/.claude-plugin" +printf '{"name": "beta", "version": "1.0.0"}\n' > "$work/r/plugins/beta/$MANIFEST" +commit_all seed-beta +( cd "$work/r" && gitc checkout -q main && gitc merge -q feature && gitc checkout -q feature ) +printf 'x\n' >> "$work/r/plugins/alpha/skills/s.md" +printf 'y\n' > "$work/r/plugins/beta/note.md" +commit_all p4 +expect_reject "P4 two plugins changed, neither bumped: BOTH named" main \ + "plugins/alpha" "plugins/beta" + +# P7 — the comparison is on the value, not on the matched fragment. Without the +# normalization, a reformat reads as a bump. +mkrepo +printf 'changed\n' >> "$work/r/plugins/alpha/skills/s.md" +printf '{"name": "alpha","version":"0.4.0"}\n' > "$work/r/plugins/alpha/$MANIFEST" +commit_all p7 +expect_reject "P7 manifest reformatted, version value identical" main "plugins/alpha" + +# P8 — versions are read from commits: an uncommitted bump must not launder a committed +# un-bumped change. +mkrepo +printf 'changed\n' >> "$work/r/plugins/alpha/skills/s.md" +commit_all p8 +bump 0.5.0 # working tree only, deliberately not committed +expect_reject "P8 bump present only in the working tree" main "plugins/alpha" + +# P9 — enumeration comes from HEAD's tree, not a filesystem glob, so a dirty local +# deletion cannot hide a committed change. +mkrepo +printf 'changed\n' >> "$work/r/plugins/alpha/skills/s.md" +commit_all p9 +rm -rf "$work/r/plugins/alpha" +expect_reject "P9 plugin directory deleted in the working tree only" main "plugins/alpha" + +# P10 — the checker anchors itself at the repo root; a run from a subdirectory must not +# quietly enumerate nothing. +mkrepo +printf 'changed\n' >> "$work/r/plugins/alpha/skills/s.md" +commit_all p10 +p10_out=$( cd "$work/r/scripts" && sh ./check-version-bump.sh main 2>&1 ); p10_st=$? +if [ "$p10_st" -eq 0 ]; then fail "P10 run from a subdirectory (exited 0)" +elif ! printf '%s' "$p10_out" | grep -q "plugins/alpha"; then + fail "P10 run from a subdirectory (wrong diagnostic: $(printf '%s' "$p10_out" | tr '\n' ' '))" +else pass "P10 run from a subdirectory still rejects"; fi + +printf '\n--- operational rejects (fail closed) ---\n' + +mkrepo +printf 'changed\n' >> "$work/r/plugins/alpha/skills/s.md" +commit_all o1 +expect_reject "O1 base ref does not resolve" no-such-ref "no merge-base" + +# O2 — a ref that resolves but shares no history. Built with commit-tree rather than +# `checkout --orphan`: an orphan checkout leaves the whole tree untracked, and switching +# back then aborts on "untracked files would be overwritten". This touches no worktree. +mkrepo +( cd "$work/r" && + o2_tree=$(gitc mktree < /dev/null) && + o2_commit=$(printf 'orphan\n' | gitc commit-tree "$o2_tree") && + gitc branch unrelated "$o2_commit" ) +printf 'changed\n' >> "$work/r/plugins/alpha/skills/s.md" +commit_all o2 +expect_reject "O2 unrelated histories, no merge-base" unrelated "no merge-base" + +mkrepo +printf 'changed\n' >> "$work/r/plugins/alpha/skills/s.md" +printf '{"name": "alpha"}\n' > "$work/r/plugins/alpha/$MANIFEST" +commit_all o3 +expect_reject "O3 HEAD manifest has no version field" main "found 0" + +mkrepo +printf 'changed\n' >> "$work/r/plugins/alpha/skills/s.md" +printf '{"version": "0.5.0", "nested": {"version": "9.9.9"}}\n' > "$work/r/plugins/alpha/$MANIFEST" +commit_all o4 +expect_reject "O4 HEAD manifest has two version matches" main "found 2" + +# O5/O6 — the same two malformations on the BASE side. The exactly-one rule applies to +# both manifests, and only testing HEAD would leave half of it unpinned. +mkrepo +( cd "$work/r" && gitc checkout -q main ) +printf '{"name": "alpha"}\n' > "$work/r/plugins/alpha/$MANIFEST" +commit_all o5-base +( cd "$work/r" && gitc checkout -q feature && gitc rebase -q main >/dev/null 2>&1 || true ) +printf 'changed\n' >> "$work/r/plugins/alpha/skills/s.md" +bump 0.5.0 +commit_all o5 +expect_reject "O5 merge-base manifest has no version field" main "found 0" + +mkrepo +( cd "$work/r" && gitc checkout -q main ) +printf '{"version": "0.4.0", "nested": {"version": "1.1.1"}}\n' > "$work/r/plugins/alpha/$MANIFEST" +commit_all o6-base +( cd "$work/r" && gitc checkout -q feature && gitc rebase -q main >/dev/null 2>&1 || true ) +printf 'changed\n' >> "$work/r/plugins/alpha/skills/s.md" +bump 0.5.0 +commit_all o6 +expect_reject "O6 merge-base manifest has two version matches" main "found 2" + +# O7-O13 — git itself failing, one row per load-bearing call. Each pattern matches the +# FULL argv, so the intended call fails and not an earlier one that shares a subcommand. +mkrepo +printf 'changed\n' >> "$work/r/plugins/alpha/skills/s.md" +commit_all o7 +with_failing_git '*merge-base*' "O7 merge-base resolution fails" "no merge-base" +with_failing_git '*ls-tree*-d*--name-only*' "O8 plugin directory enumeration fails" "ls-tree of plugins/" +with_failing_git '*diff*--quiet*' "O9 change detection fails" "git diff of" +with_failing_git '*ls-tree*HEAD*plugin.json*' "O10 HEAD manifest presence lookup fails" "ls-tree HEAD" +with_failing_git '*show*HEAD:*' "O11 HEAD manifest read fails" "git show HEAD:" + +# O12 needs the base-side lookups to be reached, which means a bumped HEAD. +mkrepo +printf 'changed\n' >> "$work/r/plugins/alpha/skills/s.md" +bump 0.5.0 +commit_all o12 +mb_sha=$( cd "$work/r" && gitc merge-base main HEAD ) +with_failing_git "*ls-tree*$mb_sha*plugin.json*" "O12 base manifest presence lookup fails" "ls-tree $mb_sha" +with_failing_git "*show*$mb_sha:*" "O13 base manifest read fails" "git show $mb_sha:" + +# O14/O15 — the CLI contract is exit 2, asserted rather than described. +mkrepo +o14_out=$( cd "$work/r" && sh scripts/check-version-bump.sh 2>&1 ); o14_st=$? +if [ "$o14_st" -eq 2 ] && printf '%s' "$o14_out" | grep -q usage; then + pass "O14 no argument: usage, exit 2" +else fail "O14 no argument (status $o14_st: $(printf '%s' "$o14_out" | tr '\n' ' '))"; fi + +o15_out=$( cd "$work/r" && sh scripts/check-version-bump.sh main extra 2>&1 ); o15_st=$? +if [ "$o15_st" -eq 2 ] && printf '%s' "$o15_out" | grep -q usage; then + pass "O15 two arguments: usage, exit 2" +else fail "O15 two arguments (status $o15_st: $(printf '%s' "$o15_out" | tr '\n' ' '))"; fi + +# O15b — an escaped value parses equal to the base but spells differently, which would +# otherwise read as a bump. +mkrepo +printf 'changed\n' >> "$work/r/plugins/alpha/skills/s.md" +printf '{"name": "alpha", "version": "0.4.\\u0030"}\n' > "$work/r/plugins/alpha/$MANIFEST" +commit_all o15b +expect_reject "O15b version value carries a backslash escape" main "backslash escape" + +# O18 — the directory survives and its content still ships, but there is no manifest and +# so no version to key on. Carved out at first ("nothing to bump"); Gate B pushed back +# twice, and rightly: full-directory deletion is out of scope, a shipped plugin with no +# manifest is a defect. A5 stays accepted, which is the asymmetry made visible. +mkrepo +rm -f "$work/r/plugins/alpha/$MANIFEST" +# Nothing else is touched: deleting the manifest is itself a change under plugins/alpha/, +# and the pre-existing skills file keeps the directory present at HEAD. Editing a second +# file here would let a regression that ignores manifest-only changes keep this row green. +commit_all o18 +expect_reject "O18 manifest alone deleted, directory and its content remain" main \ + "no version to key on" + +# O16/O17 — names the loop cannot round-trip. O16 (a space) is rejected by the grammar +# but is NOT quoted by git; O17 is, and it is the row that pins the silent-skip +# regression the grammar exists to prevent. +mkrepo +mkdir -p "$work/r/plugins/bad name/.claude-plugin" +printf '{"name": "bad", "version": "1.0.0"}\n' > "$work/r/plugins/bad name/$MANIFEST" +commit_all o16 +expect_reject "O16 plugin directory name with a space" main "outside \[A-Za-z0-9._-\]" + +mkrepo +mkdir -p "$work/r/plugins/bad$(printf '\t')tab/.claude-plugin" +printf '{"name": "bad", "version": "1.0.0"}\n' > "$work/r/plugins/bad$(printf '\t')tab/$MANIFEST" +commit_all o17 +expect_reject "O17 plugin directory name git C-quotes (tab)" main "outside \[A-Za-z0-9._-\]" + +printf '\n--- accepts ---\n' + +mkrepo +printf 'changed\n' >> "$work/r/plugins/alpha/skills/s.md" +bump 0.5.0 +commit_all a1 +expect_accept "A1 plugin changed and bumped" main + +mkrepo +printf 'docs\n' >> "$work/r/README.md" +commit_all a2 +expect_accept "A2 nothing under plugins/ changed" main + +mkrepo +bump 0.5.0 +commit_all a3 +expect_accept "A3 bump-only change" main + +mkrepo +mkdir -p "$work/r/plugins/gamma/.claude-plugin" +printf '{"name": "gamma", "version": "0.1.0"}\n' > "$work/r/plugins/gamma/$MANIFEST" +commit_all a4 +expect_accept "A4 newly added plugin (no manifest at the merge-base)" main + +mkrepo +rm -rf "$work/r/plugins/alpha" +commit_all a5 +expect_accept "A5 plugin directory deleted at HEAD" main + + +mkrepo +mkdir -p "$work/r/plugins/beta/.claude-plugin" +printf '{"name": "beta", "version": "1.0.0"}\n' > "$work/r/plugins/beta/$MANIFEST" +commit_all seed-beta +( cd "$work/r" && gitc checkout -q main && gitc merge -q feature && gitc checkout -q feature ) +printf 'x\n' >> "$work/r/plugins/alpha/skills/s.md" +printf 'y\n' > "$work/r/plugins/beta/note.md" +bump 0.5.0 alpha +bump 1.1.0 beta +commit_all a7 +expect_accept "A7 two plugins changed, both bumped" main + +# A8 — pins the documented "direction is not checked" limitation as tested behaviour. +mkrepo +printf 'changed\n' >> "$work/r/plugins/alpha/skills/s.md" +bump 0.3.9 +commit_all a8 +expect_accept "A8 version decreased (direction is not checked)" main + +# A9 — a rename is a gap in DETECTION only; renamed-and-bumped must not be rejected. +mkrepo +( cd "$work/r" && gitc mv plugins/alpha plugins/alpha-renamed ) +bump 0.5.0 alpha-renamed +commit_all a9 +expect_accept "A9 plugin directory renamed with a bump" main + +# A10 — a blob directly under plugins/. The spaced name is deliberate: with a +# grammar-conforming name, an implementation missing `ls-tree -d` would enumerate the +# blob, append `/`, diff a path that does not exist, report unchanged and pass anyway. +mkrepo +printf 'notes\n' > "$work/r/plugins/release notes.md" +commit_all a10 +expect_accept "A10 non-directory entry under plugins/ is ignored" main + +printf '\n---\n' +if [ "$fail_n" -eq 0 ]; then printf 'all passed (%s assertions)\n' "$pass_n"; else + printf '%s passed, %s FAILED\n' "$pass_n" "$fail_n"; exit 1 +fi From 804170871f10ba6ab44ed4df619acd65936b2b75 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20S=C3=A4nger?= <20968534+dsnger@users.noreply.github.com> Date: Sun, 19 Jul 2026 12:22:52 +0200 Subject: [PATCH 2/2] test(version-bump): fail loudly when the O5/O6 rebase does not happen MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both fixtures rebased `feature` onto main with `|| true`. A failed rebase left the branch at its old base, so the malformed manifest never reached the merge-base and the row passed for the wrong reason — a suite lying about what it covers. `rebase_onto_main` now names the setup in the failure and aborts a partial rebase. Verified by pointing it at a non-existent branch: two setup failures plus both row failures, 36/36 again after restore. Also drops the IFS/`set -f` restore before two `die` calls. `die` exits, so it was dead code that implied the cleanup was sometimes load-bearing. Both raised by Greptile on #7 (P2), validated before applying. --- scripts/check-version-bump.sh | 7 +++++-- scripts/check-version-bump.test.sh | 17 +++++++++++++++-- 2 files changed, 20 insertions(+), 4 deletions(-) diff --git a/scripts/check-version-bump.sh b/scripts/check-version-bump.sh index c854cda..bbf4326 100644 --- a/scripts/check-version-bump.sh +++ b/scripts/check-version-bump.sh @@ -136,9 +136,12 @@ for dir in $dirs; do # to git as a literal path makes `diff` report a non-existent path as unchanged, so # the plugin would be skipped in silence. POSIX sh cannot read NUL-delimited input, # so -z is not an option. + # No IFS/`set -f` restore before any `die` below: `die` exits the shell, so nothing + # after it runs and the restore would be dead code. Two of these paths used to carry + # one, which was harmless but implied the cleanup was sometimes load-bearing. One + # convention, stated once: `die` exits, restores happen after the loop. case "$name" in ''|*[!A-Za-z0-9._-]*) - IFS=$saved_ifs; set +f die "plugin directory name '$name' is outside [A-Za-z0-9._-]+; refusing to guess." ;; esac @@ -150,7 +153,7 @@ for dir in $dirs; do case $? in 0) continue ;; 1) ;; - *) IFS=$saved_ifs; set +f; die "git diff of $dir/ failed." ;; + *) die "git diff of $dir/ failed." ;; esac # The directory is still in HEAD's tree and something in it changed, so this plugin diff --git a/scripts/check-version-bump.test.sh b/scripts/check-version-bump.test.sh index be96023..9e7c884 100644 --- a/scripts/check-version-bump.test.sh +++ b/scripts/check-version-bump.test.sh @@ -108,6 +108,19 @@ bump() { # $1 = version, $2 = plugin dir name (default alpha) commit_all() { ( cd "$work/r" && gitc add -A && gitc commit -qm "$1" ); } +# Put `feature` on top of main, and FAIL LOUDLY if that does not happen. A suppressed +# rebase (`|| true`) leaves `feature` at its old base, so the malformed manifest never +# reaches the merge-base and the row it sets up passes for the wrong reason — the +# quietest way a suite can lie about what it covers. +rebase_onto_main() { # $1 = row name, for the diagnostic + if ! ( cd "$work/r" && gitc checkout -q feature && gitc rebase -q main ) >/dev/null 2>&1 + then + fail "$1 setup: rebase onto main failed; the fixture is not what the row assumes" + ( cd "$work/r" && gitc rebase --abort >/dev/null 2>&1 || true ) + return 1 + fi +} + printf '\n--- policy rejects ---\n' # P1 — the case the check exists for. @@ -219,7 +232,7 @@ mkrepo ( cd "$work/r" && gitc checkout -q main ) printf '{"name": "alpha"}\n' > "$work/r/plugins/alpha/$MANIFEST" commit_all o5-base -( cd "$work/r" && gitc checkout -q feature && gitc rebase -q main >/dev/null 2>&1 || true ) +rebase_onto_main o5 printf 'changed\n' >> "$work/r/plugins/alpha/skills/s.md" bump 0.5.0 commit_all o5 @@ -229,7 +242,7 @@ mkrepo ( cd "$work/r" && gitc checkout -q main ) printf '{"version": "0.4.0", "nested": {"version": "1.1.1"}}\n' > "$work/r/plugins/alpha/$MANIFEST" commit_all o6-base -( cd "$work/r" && gitc checkout -q feature && gitc rebase -q main >/dev/null 2>&1 || true ) +rebase_onto_main o6 printf 'changed\n' >> "$work/r/plugins/alpha/skills/s.md" bump 0.5.0 commit_all o6