Require a version bump when the shipped plugin changes (invariant 12)#7
Conversation
…ariant 12) 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/<name>/ 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.
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Warning Review limit reached
Next review available in: 42 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a POSIX checker requiring version changes for modified plugins, comprehensive shell regression tests, CI enforcement, and synchronized architecture, policy, release, and hardening documentation. The ChangesPlugin version-bump enforcement
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant PullRequest
participant CI
participant VersionBumpChecker
participant Git
PullRequest->>CI: trigger pull-request workflow
CI->>VersionBumpChecker: pass base ref
VersionBumpChecker->>Git: resolve merge-base and changed plugins
VersionBumpChecker->>Git: compare manifest versions
VersionBumpChecker-->>CI: return pass or violations
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR mechanically enforces invariant 12 — every PR that changes a plugin directory must bump that plugin's manifest version — by introducing
Confidence Score: 4/5Safe to merge; the checker logic is sound and the CI wiring is correct. The two noted items are non-blocking style and test-setup observations. The checker correctly handles every documented edge case — C-quoted directory names, commit-only reads, manifest-only deletion, and per-call git error detection — and the test suite verifies each branch including mutation testing. The two observations are: the
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A(["`sh check-version-bump.sh base-ref`"]) --> B{exactly 1 arg?}
B -- no --> C([exit 2 / usage])
B -- yes --> D[cd to repo root]
D --> E[git merge-base base-ref HEAD]
E -- fails --> F([die: no merge-base])
E -- ok --> G[git ls-tree -d HEAD plugins/]
G -- fails --> H([die: ls-tree of plugins/ failed])
G -- ok --> I[for each dir in dirs]
I --> J{name matches A-Za-z0-9._- ?}
J -- no --> K([die: invalid dir name])
J -- yes --> L[git diff --quiet mb HEAD dir/]
L -- exit 0 --> M[skip: unchanged]
M --> I
L -- exit 1 --> N[changed: continue]
L -- other --> O([die: diff failed])
N --> P[tree_has HEAD manifest?]
P -- absent --> Q([die: no manifest])
P -- present --> R[git show HEAD:manifest]
R -- fails --> S([die: show failed])
R -- ok --> T[extract_version HEAD]
T -- 0 or 2+ matches --> U([die: version ambiguous])
T -- backslash in value --> V([die: escape in version])
T -- ok --> W[tree_has mb manifest?]
W -- absent --> X[base_ver = empty, new plugin]
W -- present --> Y[git show mb:manifest]
Y -- fails --> Z([die: show failed])
Y -- ok --> AA[extract_version base]
AA -- error --> AB([die])
AA --> AC{head_ver == base_ver?}
X --> AC
AC -- equal --> AD[fail: record plugin name]
AD --> I
AC -- different --> I
I -- done --> AE{rc == 0?}
AE -- yes --> AF([exit 0: ok])
AE -- no --> AG([exit 1: prints offenders])
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A(["`sh check-version-bump.sh base-ref`"]) --> B{exactly 1 arg?}
B -- no --> C([exit 2 / usage])
B -- yes --> D[cd to repo root]
D --> E[git merge-base base-ref HEAD]
E -- fails --> F([die: no merge-base])
E -- ok --> G[git ls-tree -d HEAD plugins/]
G -- fails --> H([die: ls-tree of plugins/ failed])
G -- ok --> I[for each dir in dirs]
I --> J{name matches A-Za-z0-9._- ?}
J -- no --> K([die: invalid dir name])
J -- yes --> L[git diff --quiet mb HEAD dir/]
L -- exit 0 --> M[skip: unchanged]
M --> I
L -- exit 1 --> N[changed: continue]
L -- other --> O([die: diff failed])
N --> P[tree_has HEAD manifest?]
P -- absent --> Q([die: no manifest])
P -- present --> R[git show HEAD:manifest]
R -- fails --> S([die: show failed])
R -- ok --> T[extract_version HEAD]
T -- 0 or 2+ matches --> U([die: version ambiguous])
T -- backslash in value --> V([die: escape in version])
T -- ok --> W[tree_has mb manifest?]
W -- absent --> X[base_ver = empty, new plugin]
W -- present --> Y[git show mb:manifest]
Y -- fails --> Z([die: show failed])
Y -- ok --> AA[extract_version base]
AA -- error --> AB([die])
AA --> AC{head_ver == base_ver?}
X --> AC
AC -- equal --> AD[fail: record plugin name]
AD --> I
AC -- different --> I
I -- done --> AE{rc == 0?}
AE -- yes --> AF([exit 0: ok])
AE -- no --> AG([exit 1: prints offenders])
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
scripts/check-version-bump.test.sh:222
**Silent rebase failure masks test setup**
The `|| true` suppresses the rebase exit status entirely. If `gitc rebase` fails for any reason — e.g., git leaves a mid-rebase state on an unexpected conflict or the working tree is in an unusual condition — the test continues with `feature` still pointing at the pre-rebase commit. In that case, `git merge-base main HEAD` would return the original `base` commit instead of `o5-base`, the malformed manifest would never be at the merge-base, and O5 would either pass for the wrong reason or fail with a diagnostic that points nowhere near the rebase failure. This pattern also silently hides O6's setup failure by the same mechanism. Capturing the return code or at minimum printing a diagnostic on non-zero would make a setup failure visible without changing normal test behaviour.
### Issue 2 of 2
scripts/check-version-bump.sh:139-154
**Inconsistent IFS/globbing cleanup before `die`**
The grammar-check path (lines 141–143) and the diff-failure path (line 153) restore `IFS` and undo `set -f` before calling `die`. The subsequent `die` calls in the same loop — from `tree_has`, `git show`, and `extract_version` failures — do not. Since `die` unconditionally calls `exit 1`, no cleanup code ever executes after it in any of these paths; the difference has no functional effect. However, the inconsistency could lead a future editor to believe that cleanup before `die` is sometimes required, or to incorrectly omit it when it actually would matter (e.g., if `die` were ever softened to a non-exit path). Making the convention uniform — either always restore or rely on the fact that `die` exits — would remove the ambiguity.
Reviews (1): Last reviewed commit: "feat(ci): require a version bump when th..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| ( 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 ) |
There was a problem hiding this comment.
The || true suppresses the rebase exit status entirely. If gitc rebase fails for any reason — e.g., git leaves a mid-rebase state on an unexpected conflict or the working tree is in an unusual condition — the test continues with feature still pointing at the pre-rebase commit. In that case, git merge-base main HEAD would return the original base commit instead of o5-base, the malformed manifest would never be at the merge-base, and O5 would either pass for the wrong reason or fail with a diagnostic that points nowhere near the rebase failure. This pattern also silently hides O6's setup failure by the same mechanism. Capturing the return code or at minimum printing a diagnostic on non-zero would make a setup failure visible without changing normal test behaviour.
Prompt To Fix With AI
This is a comment left during a code review.
Path: scripts/check-version-bump.test.sh
Line: 222
Comment:
**Silent rebase failure masks test setup**
The `|| true` suppresses the rebase exit status entirely. If `gitc rebase` fails for any reason — e.g., git leaves a mid-rebase state on an unexpected conflict or the working tree is in an unusual condition — the test continues with `feature` still pointing at the pre-rebase commit. In that case, `git merge-base main HEAD` would return the original `base` commit instead of `o5-base`, the malformed manifest would never be at the merge-base, and O5 would either pass for the wrong reason or fail with a diagnostic that points nowhere near the rebase failure. This pattern also silently hides O6's setup failure by the same mechanism. Capturing the return code or at minimum printing a diagnostic on non-zero would make a setup failure visible without changing normal test behaviour.
How can I resolve this? If you propose a fix, please make it concise.
There was a problem hiding this comment.
Valid, applied in 8041708. This is the failure mode the suite exists to prevent, one level up: a fixture that silently isn't what its row assumes. Replaced with a rebase_onto_main helper that reports a named setup FAIL and aborts a partial rebase.
Verified rather than assumed — pointing the helper at a non-existent branch produces:
FAIL - o5 setup: rebase onto main failed; the fixture is not what the row assumes
FAIL - O5 merge-base manifest has no version field (exited 0)
and the suite is 36/36 again after restore.
| 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 |
There was a problem hiding this comment.
die
The grammar-check path (lines 141–143) and the diff-failure path (line 153) restore IFS and undo set -f before calling die. The subsequent die calls in the same loop — from tree_has, git show, and extract_version failures — do not. Since die unconditionally calls exit 1, no cleanup code ever executes after it in any of these paths; the difference has no functional effect. However, the inconsistency could lead a future editor to believe that cleanup before die is sometimes required, or to incorrectly omit it when it actually would matter (e.g., if die were ever softened to a non-exit path). Making the convention uniform — either always restore or rely on the fact that die exits — would remove the ambiguity.
Prompt To Fix With AI
This is a comment left during a code review.
Path: scripts/check-version-bump.sh
Line: 139-154
Comment:
**Inconsistent IFS/globbing cleanup before `die`**
The grammar-check path (lines 141–143) and the diff-failure path (line 153) restore `IFS` and undo `set -f` before calling `die`. The subsequent `die` calls in the same loop — from `tree_has`, `git show`, and `extract_version` failures — do not. Since `die` unconditionally calls `exit 1`, no cleanup code ever executes after it in any of these paths; the difference has no functional effect. However, the inconsistency could lead a future editor to believe that cleanup before `die` is sometimes required, or to incorrectly omit it when it actually would matter (e.g., if `die` were ever softened to a non-exit path). Making the convention uniform — either always restore or rely on the fact that `die` exits — would remove the ambiguity.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Valid, applied in 8041708 — resolved by deletion rather than by adding the missing restores. die is an unconditional exit 1, so both cleanups were dead code, and your point about a future editor reading them as load-bearing is the actual cost. The convention is now stated once above the loop: die exits, restores happen after the loop.
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.
1 participant
The finding
Field finding,
major, sourcemanual: changes to the shipped plugin can merge without a version bump, so installed copies silently stay stale. An installed plugin lives under a version-keyed cache path, so an un-bumped change never propagates.Two incidents, plus one found while writing the spec:
fe5e296and793e234changed the plugin after 0.4.0 shipped and were never released. This PR ships them as 0.4.1.Hardened at rung 2 via
dev-workflow:harden-finding.The check
scripts/check-version-bump.shfails a PR that changes any path under aplugins/<name>/directory still present at HEAD without changing that plugin's manifestversionto a value different from the merge-base's.ls-tree -d), not a filesystem glob.examples/included. It ships in the package, and an exemption list would make the check judge which paths "matter", which is what failed as a convention.What it does not catch, stated identically in the script header, invariant 12 and the ledger row (a partial list is an overclaim): semantic correctness of the bump; direction; anything outside a
pull_requestevent; deletion of an entire plugin directory; any change to which directory the marketplace entry points at; two PRs colliding on the same new version; release/tag status.Tests
scripts/check-version-bump.test.sh— 36 assertions over real throwaway git repos, in three groups:examples/, two plugins with both named, reformatted manifest, uncommitted bump, dirty deletion, subdirectory invocation.gitwrapper failing each load-bearing call by full argv, argument arity,\uXXXXescapes, C-quoted directory names, manifest-only deletion.plugins/.Mutation-verified: replacing the version comparison with
falsefails exactly the 8 policy rows and nothing else.Also here
plugins/dev-workflow/CHANGELOG.md, backfilled 0.1.0 → 0.4.1 fromgit log, with heading completeness and interval coverage checked mechanically against history.examples/ships in the package but is never scaffolded — reading that as "not shipped" is what made it look out of scope).artifact-version-not-bumpedminted indocs/hardening-taxonomy.md, with why it is notdependency-unpinned(that one runs inbound, this one outbound).docs/superpowers/.Gates
name— after review found three new defects inside the fix; it is a documented gap now, not a silent one.NO FINDINGSon both spec-compliance and quality. Pass 3 changed behaviour (manifest-only deletion went from carve-out to fail-closed) and the spec, plan, CHANGELOG, invariant and ledger were updated in the same commit.git diff --checkclean.One thing to watch on this PR: the PR-only CI step has never executed in a real
pull_requestcontext before now. A skipped step leaves the job green, so the acceptance test is that step's own log — not the overall colour.Summary by CodeRabbit
New Features
dev-workflowplugin version to 0.4.1.Documentation
Chores