feat(release): single VERSION source of truth + auto tag/Release on merge#28
feat(release): single VERSION source of truth + auto tag/Release on merge#28mattmaynes wants to merge 1 commit into
Conversation
…erge Implements spec/plan 0009 (supersedes the stale, closed PR #16), refreshed for current main. The plugin version lived hardcoded in seven manifests with no source of truth and no enforcement, so bumps drifted - two marketplace manifests sat at 0.1.1 across three releases until #27. This stands up one enforced version and an automated release. - VERSION (root) is the single source of truth (1.0.2); the seven manifests are derivatives. - scripts/bump-version.sh X.Y.Z rewrites VERSION + all 7 manifests via a format-preserving surgical substitution (validate in memory, write VERSION last, no half-bumped tree); --check is the inverse invariant; semver-only, no v prefix; SPECTRA_ROOT override for sandboxed tests. - ci.yml gains a release job: push-only, needs [test, readme-drift], job-scoped contents: write (workflow stays read-only). Idempotent via gh release view; creates the bare-semver tag + GitHub Release from docs/releases/<x.y.z>.md when present, else --generate-notes. Composes with whats-new.yml (Release -> README headline). - test.sh section 11: --check invariant, semver accept/reject, and a SPECTRA_ROOT sandbox write that never touches the real tree. - docs/releases/README.md documents the per-version notes convention. - Identity: marketplace owner + plugin author -> rogueoak (personal name/email dropped, post org-transfer). - AGENTS.md "Releasing" (outside the spectra block); docs/overview features + architecture reflected. Nothing under spectra/ changes except the version string + author field, so README token figures are unaffected. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
mattmaynes
left a comment
There was a problem hiding this comment.
🔒 Spectra Security - approve
Least-privilege is done right: workflow default stays contents: read, the write scope is job-scoped to release only, it's gated if: github.event_name == 'push' (fork PRs fire as pull_request, so untrusted PR code can never reach the write token), needs: [test, readme-drift] blocks publishing unvalidated code, and it uses the ambient github.token (no PAT). The run script reads VERSION via cat and quotes every expansion - no ${{ }} event data is interpolated into the shell, so no template/command injection. bump-version.sh passes ROOT/NEW/MANIFESTS to python via env vars (never re-evaluated by a shell), and validates+writes in memory before committing to disk. Dropping the personal email in favor of rogueoak also removes PII from the shipped manifests. One should-fix below (checkout pin) and one nit; neither blocks.
| env: | ||
| GH_TOKEN: ${{ github.token }} | ||
| steps: | ||
| - uses: actions/checkout@v4 |
There was a problem hiding this comment.
🔒 Spectra Security - minor
This job carries a contents: write token but checks out via the mutable tag actions/checkout@v4, while whats-new.yml deliberately pins the same action to a full commit SHA because it holds write tokens (its comment: "Pinned to a full SHA (v4.2.2) - this job holds write tokens and self-merges to main."). A mutable tag can be repointed if the upstream action is compromised, and it would then run with a token that can create tags/releases. Pin to the same SHA the repo already trusts for consistency and supply-chain hardening:
- uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2| if check; then exit 0; else exit 1; fi | ||
| ;; | ||
| *) | ||
| if ! printf '%s' "$1" | grep -Eq '^[0-9]+\.[0-9]+\.[0-9]+$'; then |
There was a problem hiding this comment.
🔒 Spectra Security - nit
printf '%s' "$1" | grep -Eq '^...$' matches if any line of a multiline arg matches, so a value like 1.2.3$'\n'malicious slips past the semver gate. It's contained here - the arg only ever reaches python as an env var (no shell eval) and a newline-bearing version makes json.loads fail, aborting before any write - and this path is never fed untrusted input (CI reads VERSION, it doesn't call this with an arg). Still, a tighter gate makes the rejection clean, e.g.:
case "$1" in
*[!0-9.]* | '' ) ok=0 ;;
*) printf '%s' "$1" | grep -Eq '^[0-9]+\.[0-9]+\.[0-9]+$' && ok=1 || ok=0 ;;
esacor pass -z/use grep -Ex. Optional.
mattmaynes
left a comment
There was a problem hiding this comment.
Section 11 is solid on the happy paths and the sandbox isolation is clean (real VERSION proven untouched). One material gap holds up approval: the suite never asserts that --check fails on drift, so the feature's headline guarantee ("test.sh fails CI if they ever drift") is itself unproven. Two inline comments. The release job's notes-file vs --generate-notes branch lives in a gh release create step and isn't unit-testable here - acceptable, just noting it.
| synced=1; [ "$(cat "$T/VERSION")" = "9.9.9" ] || synced=0 | ||
| for f in $mans; do [ "$(grep -c '"version": *"9.9.9"' "$T/$f")" = 1 ] || synced=0; done | ||
| [ "$synced" = 1 ] && ok "all 7 manifests + VERSION now read 9.9.9" || bad "bump-version 9.9.9 left a manifest unsynced" | ||
| SPECTRA_ROOT="$T" "$bv" --check >/dev/null 2>&1 && ok "sandbox --check converges after write" || bad "sandbox --check did not converge" |
There was a problem hiding this comment.
🧪 Spectra Tester - major
Both --check assertions (line 312 and here) only assert it passes. The drift-detection failure path - the entire reason --check exists in CI - is never asserted to return non-zero. A regression where check() always exits 0 (e.g. a botched loop or a swallowed exit) would sail through this suite, silently disabling the drift guard. That's a false-positive: the test stays green even when the code it guards is broken. I confirmed the guard currently fires (exit 1) on a hand-edited manifest, so a negative test is feasible and green today. Add one in the sandbox right after this line:
# negative: drift must FAIL --check (proves the guard catches, not just that a synced tree passes)
printf '9.9.10\n' > "$T/VERSION"
SPECTRA_ROOT="$T" "$bv" --check >/dev/null 2>&1 && bad "--check passed despite VERSION/manifest drift" || ok "--check fails on drift"| for f in $mans VERSION; do mkdir -p "$T/$(dirname "$f")"; cp "$ROOT/$f" "$T/$f"; done | ||
| if SPECTRA_ROOT="$T" "$bv" 9.9.9 >/dev/null 2>&1; then ok "bump-version 9.9.9 writes the sandbox"; else bad "bump-version 9.9.9 failed in sandbox"; fi | ||
| synced=1; [ "$(cat "$T/VERSION")" = "9.9.9" ] || synced=0 | ||
| for f in $mans; do [ "$(grep -c '"version": *"9.9.9"' "$T/$f")" = 1 ] || synced=0; done |
There was a problem hiding this comment.
🧪 Spectra Tester - minor
The script's two integrity guards - the len(found) != 1 "exactly one version token" check and the missing-manifest branch (both in check() and write()) - have no coverage. These are the guards that stop a half-bumped or malformed tree from shipping; I verified a planted second "version" token trips --check (exit 1), so they're cheaply testable in the sandbox. Nice-to-have alongside the drift test, e.g.:
# a second version token must fail --check
perl -0pi -e 's/"version"/"version": "0.0.0",\n "version"/' "$T/spectra/.claude-plugin/plugin.json"
SPECTRA_ROOT="$T" "$bv" --check >/dev/null 2>&1 && bad "--check passed with two version tokens" || ok "--check rejects a stray second version token"| gh release create "$v" --target "$GITHUB_SHA" --notes-file "$notes" | ||
| else | ||
| echo "Publishing release $v with auto-generated notes (no $notes)" | ||
| gh release create "$v" --target "$GITHUB_SHA" --generate-notes |
There was a problem hiding this comment.
📐 Spectra Architect - minor
The headline contract holds for the --notes-file path but breaks on this --generate-notes fallback. whats-new.sh takes the first non-blank, non-heading line of the release body; auto-generated notes are ## What's Changed followed by * <PR title> by @user in #N. The heading is skipped but the bullet is not, so a release with no docs/releases/<v>.md puts a raw commit bullet (* feat: ... by @user in #N) into the README "What's new" headline. The two notes-source paths don't both satisfy the "first non-heading line = clean headline" contract that docs/releases/README.md documents. Either strip a leading list marker in whats-new.sh's headline(), or have the fallback synthesize a plain one-line headline (e.g. Release <v>) instead of relying on --generate-notes output for the headline.
| # Landing branch only, and only after the gating checks pass. This is the one privileged | ||
| # job (it can create tags/releases), so it must never run on untrusted PR code - hence | ||
| # push-only + needs. Everything else stays read-only at the workflow level. | ||
| if: github.event_name == 'push' |
There was a problem hiding this comment.
📐 Spectra Architect - minor
This new privileged release job is governed by the workflow-level concurrency group ${{ github.workflow }}-${{ github.ref }} with cancel-in-progress: true (lines 15-17). All pushes to main share refs/heads/main, so two VERSION bumps merged in quick succession collide on that group: the later push cancels the earlier run, and if the earlier run's release job hasn't finished gh release create yet, that version is never tagged or released - a silent missed release, no error. The idempotency guard protects against duplicate releases but not against this dropped one, so the "composes without races" claim has a gap. Give the release job its own non-cancelling concurrency group keyed to the commit/version, e.g. concurrency: { group: release-${{ github.sha }}, cancel-in-progress: false }, so a superseded CI run can still finish publishing its tag.
| # Landing branch only, and only after the gating checks pass. This is the one privileged | ||
| # job (it can create tags/releases), so it must never run on untrusted PR code - hence | ||
| # push-only + needs. Everything else stays read-only at the workflow level. | ||
| if: github.event_name == 'push' |
There was a problem hiding this comment.
🔧 Spectra Engineer - minor
The workflow-level concurrency block (lines 15-17) uses cancel-in-progress: true keyed only on github.ref. For a pull_request that is what you want, but it also applies to push: main: a newer main push cancels an older still-running run, including its release job. Two failure modes follow:
- Common case (bump commit, then an unrelated main commit lands quickly): the bump run is cancelled, the newer run still sees the bumped
VERSIONand releases - but--target $GITHUB_SHAnow tags the release at the later commit, not the bump commit. - Double-bump case (bump to X, then a second bump to Y lands before X's release runs): X's run is cancelled and Y's run only sees
VERSION=Y, so X is never tagged/released - a silently dropped release.
Concrete fix: don't cancel landing-branch runs, e.g. cancel-in-progress: ${{ github.event_name == 'pull_request' }}.
| env: | ||
| GH_TOKEN: ${{ github.token }} | ||
| steps: | ||
| - uses: actions/checkout@v4 |
There was a problem hiding this comment.
🔧 Spectra Engineer - minor
This privileged job runs with contents: write but pins actions/checkout to the @v4 tag. The repo's own docs/overview/architecture.md states the whats-new.yml checkout is pinned to a full commit SHA because the job carries write tokens. This job carries the same write token and should follow the same convention - pin to the checkout commit SHA (with a # v4.x.x comment) for consistency with the stated supply-chain rule.
|
On hold - do not merge. This is green and fully reviewed (engineer/tester/architect/security - no blockers), but we're going to generalize this release pattern into Trellis as an optional, shareable plugin-release template that Spectra then consumes, so the two repos operate identically. Plan: build + harden it in Trellis (review, merge, release Trellis) -> update Spectra to consume it via Keeping this branch as the proven reference implementation. The review findings to carry into the Trellis version:
|
|
Closing - this work is moving to Trellis as an optional, shareable plugin-release template (standalone release.yml via workflow_run, opt-in via a trellis --template flag, config-driven manifest list). The reference implementation and the carried review findings remain in this PR's history. Spectra will adopt the template via trellis-update once it ships. |
Implements spec/plan 0009 - Plugin versioning & releases (supersedes the stale, closed #16, refreshed for current
main).Problem
The plugin version was hardcoded across seven manifests with no source of truth and no enforcement. Bumps drift: two marketplace manifests sat at
0.1.1across three releases until #27. Each agent gates update detection on the manifestversion, so a stranded manifest can freeze installed users on an old version. Releases were also cut by hand.Changes
VERSION(root) - single source of truth (1.0.2); the 7 manifests are derivatives.scripts/bump-version.sh-X.Y.ZrewritesVERSION+ all 7 manifests via a format-preserving surgical substitution (validate in memory, writeVERSIONlast - no half-bumped tree);--checkis the inverse invariant; semver-only (nov);SPECTRA_ROOToverride for sandboxed tests..github/workflows/ci.yml- newreleasejob: push-only,needs: [test, readme-drift], job-scopedcontents: write(workflow stays read-only). Idempotent viagh release view; creates the bare-semver tag + Release fromdocs/releases/<x.y.z>.mdwhen present (author-controlled headline), else--generate-notes. Composes withwhats-new.yml(Release -> README headline).test.shsection 11 ---checkinvariant, semver accept/reject, and aSPECTRA_ROOTsandbox write that never touches the real tree.docs/releases/README.md- per-version notes convention.owner+ pluginauthor->rogueoak(personal name/email dropped, post org-transfer).AGENTS.md"Releasing" (outside the spectra block);docs/overview/features + architecture reflected.Nothing under
spectra/changes except the version string + author field, so README token figures are unaffected.Testing
./test.sh->PASS(incl. new section 11);scripts/token-report.sh --checkandscripts/bump-version.sh --checkexit 0; all 7 manifests parse; sandbox write verified to propagate to all 7 +VERSIONand leave the real tree untouched.🤖 Generated with Claude Code