feat: 3-tier release strategy with hotfix, release, and CI workflows#1289
feat: 3-tier release strategy with hotfix, release, and CI workflows#1289
Conversation
trek-e
left a comment
There was a problem hiding this comment.
PR Review: #1289 -- 3-tier release strategy
Security
Medium: interpolation inside run blocks
The PR description claims no interpolation in run blocks and uses environment variables. This is not accurate. Both hotfix.yml and release.yml extensively use inputs.version, needs.validate-version.outputs, and inputs.dry_run directly inside run blocks via shell variable assignment.
Example from hotfix.yml line 45:
run: |
VERSION="${{ inputs.version }}"The safer pattern is:
env:
VERSION: ${{ inputs.version }}
run: |
echo "$VERSION"Practical risk is low since workflow_dispatch inputs require repo write access and the version regex validation runs early. But there is a TOCTOU subtlety: the validate step assigns the version and validates it with regex. If an attacker could inject shell metacharacters, the injection would occur during the assignment line before the regex runs. The regex itself is sound but it is not a defense because the damage happens at interpolation time.
Since workflow_dispatch inputs require write permission, the actual exploit surface is minimal. Still, the pattern should be fixed to match the stated claim and follow GitHub security best practices.
Count of interpolations in run blocks: hotfix.yml ~12 instances, release.yml ~16 instances.
Version Validation Regex
Both regexes are correct:
- Hotfix:
^[0-9]+\.[0-9]+\.[1-9][0-9]*$-- correctly requires patch > 0 - Release:
^[0-9]+\.[0-9]+\.0$-- correctly requires .0 suffix - Major detection:
^[0-9]+\.0\.0$-- correct
Workflow Logic
Hotfix flow is sound: create checks out from base tag, bumps version, pushes branch. Finalize runs tests, publishes to latest, tags, creates merge-back PR.
Release flow is sound with one documentation gap (see below).
Documentation vs implementation mismatch: VERSIONING.md shows a major release progression as beta.1 -> beta.2 -> rc.1 -> rc.2 -> stable, implying major releases go through both beta AND rc phases. However, release.yml only supports one pre-release type per version -- major gets -beta.N and minor gets -rc.N. There is no mechanism to transition from beta to rc within the same release. Either add a phase transition action or update the docs to match.
Test ordering concern: The rc job in release.yml commits the pre-release version bump and pushes the tag (lines 140-145) before running npm ci and test:coverage (lines 147-149). If tests fail, a tagged commit with a broken version has already been pushed. Consider running tests before tagging.
CI/Support Workflows
- auto-branch.yml: Uses actions/github-script (no shell injection risk). Logic is clean.
- branch-naming.yml: Advisory only (warning, not failure). Correctly exempts dependabot/, renovate/, gsd/, claude/ branches.
- pr-gate.yml: Size labeling is reasonable. Uses per_page:100 which may miss files in very large PRs -- acceptable.
- stale.yml: Standard configuration, no issues.
- dependabot.yml: Standard, no issues.
Overlap with Other PRs
PRs #1208 and #1210 are already CLOSED. No overlap with currently open PRs (#1282, #1268, #1190 are unrelated features).
VERSIONING.md
Well-written and comprehensive. The beta->rc->stable progression note mentioned above is the only inaccuracy relative to the implementation.
Verdict
Request changes on two items:
- Move interpolation expressions out of run blocks into env mappings (to match the PR own security claims and GitHub best practices)
- Fix the VERSIONING.md beta->rc progression documentation to match what the workflow actually implements (major = beta only, no beta->rc transition)
- Reorder the rc job to run tests before tagging/pushing
Everything else looks good -- the release strategy is well-designed, permissions are appropriately scoped, the npm-publish environment gate is a solid safety net, and the supporting CI workflows are clean.
trek-e
left a comment
There was a problem hiding this comment.
Adversarial Review: PR #1289 -- 3-tier release strategy
Reviewed at commit 5ea096e (fix commit addressing prior review). Every workflow file read line-by-line.
Prior Review Findings -- Verification
The three issues flagged in the prior review (expression injection, test-before-tag ordering, VERSIONING.md mismatch) are all FIXED in commit 5ea096e:
- All
${{ }}expressions moved fromrun:blocks toenv:mappings in both hotfix.yml and release.yml. Confirmed zero remaining expressions inside run blocks. - rc job in release.yml now runs "Install and test" (npm ci + test:coverage) BEFORE "Tag and push". Confirmed.
- VERSIONING.md now correctly states major = beta only, minor = rc only, with no beta-to-rc progression. Confirmed.
New Findings
HIGH-1: Dry run does not prevent git tag/push in release.yml rc and finalize jobs
In release.yml, the rc job's "Tag and push" step (lines 150-156) has no dry_run guard. When dry_run=true, the workflow still:
- Creates and pushes the git tag (
v1.28.0-rc.1) - Pushes the version bump commit to the release branch
Only npm publish is gated by dry_run. Compare with hotfix.yml finalize, where "Tag and push" correctly has if: ${{ !inputs.dry_run }}.
The same issue exists in release.yml finalize job -- "Tag and push" (lines 212-218) is also missing the dry_run guard.
Fix: Add if: ${{ !inputs.dry_run }} to the "Tag and push" step in both the rc and finalize jobs of release.yml. Also guard the git push in "Bump to pre-release version" (the commit is created unconditionally, and the push happens via "Tag and push" -- but on dry run, the commit still lands on the local checkout and the branch push would be skipped, leaving the remote branch out of sync with what was tested).
HIGH-2: Hotfix publish-before-tag ordering creates inconsistency risk
In hotfix.yml finalize, the step order is:
- Install and test
- Publish to npm (line 119)
- Tag and push (line 125)
If step 3 fails (e.g., git push permission error, network issue), the package is published to npm without a corresponding git tag. The tag should be pushed before publishing to npm -- a failed publish can be retried, but an untagged published package requires manual cleanup.
release.yml finalize has the correct order (tag at line 212, publish at line 221). Only hotfix.yml has this reversed.
Fix: Swap the order in hotfix.yml finalize: "Tag and push" before "Publish to npm".
MEDIUM-1: Actions pinned to mutable tags, not SHA hashes
All actions use tag references:
actions/checkout@v4actions/setup-node@v4actions/github-script@v7actions/stale@v9
These are mutable -- a compromised or hijacked tag could inject malicious code. GitHub's security hardening guide recommends pinning to full SHA hashes. The dependabot.yml configuration will propose updates to GitHub Actions, which is good, but the initial deployment should start with pinned SHAs for a release pipeline that publishes to npm.
Fix: Pin each action to its current commit SHA (e.g., actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2). Dependabot will keep these updated.
MEDIUM-2: No concurrency groups on release workflows
Neither hotfix.yml nor release.yml defines concurrency: groups. If the same workflow is triggered twice for the same version (double-click, two people), both runs execute in parallel causing duplicate tags, npm publish races, or conflicting version bumps.
Fix: Add concurrency groups keyed on the version input:
concurrency:
group: hotfix-${{ inputs.version }}
cancel-in-progress: falseMEDIUM-3: No branch existence check in create jobs
Both hotfix.yml and release.yml create jobs use git checkout -b "$BRANCH" without checking if the branch already exists on the remote. A previous failed create attempt leaves a dangling branch, and re-running fails with an unhelpful git error.
Fix: Add a pre-flight check:
if git ls-remote --exit-code origin "$BRANCH" >/dev/null 2>&1; then
echo "::error::Branch $BRANCH already exists."
exit 1
fiMEDIUM-4: branch-naming.yml missing permissions block
branch-naming.yml has no permissions: key. It inherits repo defaults, which could be overly broad. Since it only emits a warning via actions/github-script, it needs no write permissions.
Fix: Add permissions: {} or permissions: { contents: read }.
LOW-1: npm publish has no post-publish verification
If npm publish fails partway (network timeout, partial upload), there is no verification step (e.g., npm view get-shit-done-cc@VERSION). A partial publish could leave the registry inconsistent. Acceptable for v1 but worth noting.
LOW-2: pr-gate.yml per_page:100 may undercount large PRs
The listFiles call uses per_page: 100. PRs with 100+ changed files will have an incorrect size label. Acceptable since XL PRs already get a warning.
INFO-1: VERSIONING.md Semver Rules table contradicts Release Tiers table
The Semver Rules table says MINOR = "New features, new commands, non-breaking enhancements." The Release Tiers table says Minor = "Fixes + enhancements" and reserves features for Major. One of these is wrong.
INFO-2: dry_run does not apply to create action
Both create jobs push branches unconditionally. This means dry_run=true on create still creates a real remote branch. If intentional, document it. If not, gate the push.
Summary
| Severity | Count | Items |
|---|---|---|
| CRITICAL | 0 | -- |
| HIGH | 2 | Dry run incomplete in release.yml (HIGH-1), publish-before-tag in hotfix.yml (HIGH-2) |
| MEDIUM | 4 | Unpinned actions (M1), no concurrency (M2), no branch check (M3), missing permissions (M4) |
| LOW | 2 | No publish verify (L1), per_page limit (L2) |
| INFO | 2 | Doc inconsistency (I1), dry_run scope (I2) |
HIGH-1 and HIGH-2 should be fixed before merge. The MEDIUM items are strongly recommended.
trek-e
left a comment
There was a problem hiding this comment.
Re-Review: PR #1289 at commit 93f7ef4 (security hardening)
Reviewing the delta from 5ea096e to 93f7ef4. All findings from prior reviews verified against the latest code.
Prior Findings -- All Resolved
Every item from the two previous reviews has been addressed:
| Finding | Status |
|---|---|
| HIGH-1: Dry run not gating tag/push in release.yml | FIXED -- if: \${{ !inputs.dry_run }} added to rc and finalize Tag and push steps |
| HIGH-2: Hotfix publish-before-tag ordering | FIXED -- tag now pushed before npm publish in hotfix finalize |
| MEDIUM-1: Actions not SHA-pinned | FIXED -- all pinned with comment annotations |
| MEDIUM-2: No concurrency groups | FIXED -- version-keyed groups with cancel-in-progress: false |
| MEDIUM-3: No branch existence check | FIXED -- pre-flight check in both create jobs |
| MEDIUM-4: branch-naming.yml missing permissions | FIXED -- permissions: {} added |
| LOW-1: No post-publish verification | FIXED -- npm view + dist-tag verification after every publish |
| INFO-1: VERSIONING.md semver rules mismatch | FIXED -- MINOR now says "enhancements" consistently |
New Findings in 93f7ef4
LOW: stale.yml still uses mutable tag
actions/stale@v9 is the only remaining action not pinned to a SHA hash. All other actions across hotfix.yml, release.yml, auto-branch.yml, branch-naming.yml, and pr-gate.yml were pinned in this commit. stale.yml was missed.
Not blocking -- dependabot will propose the pin automatically, and stale.yml does not handle secrets or publish artifacts.
INFO: Dry-run publish validation step added
The rc and finalize jobs in release.yml now include a npm publish --dry-run step before the actual publish. This is a good addition that validates the package can be published without actually publishing. The --dry-run validation step correctly runs only when dry_run is false (since it validates against the real registry).
Verdict
All HIGH and MEDIUM items from prior reviews are resolved. The security hardening commit is thorough -- SHA pinning, provenance, concurrency groups, branch existence checks, post-publish verification, and dry-run validation are all implemented correctly. The one remaining item (stale.yml tag pinning) is low-severity and will be caught by dependabot.
This PR is ready to merge. @trek-e
Supersedes PRs #1208 and #1210 with a consolidated approach: - VERSIONING.md: Strategy document with 3 release tiers (patch/minor/major) - hotfix.yml: Emergency patch releases to latest - release.yml: Standard release cycle with RC/beta pre-releases to next - auto-branch.yml: Create branches from issue labels - branch-naming.yml: Convention validation (advisory) - pr-gate.yml: PR size analysis and labeling - stale.yml: Weekly cleanup of inactive issues/PRs - dependabot.yml: Automated dependency updates npm dist-tags: latest (stable) and next (pre-release) only, following Angular/Next.js convention. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…rectness
- Move all ${{ }} expression interpolation from run: blocks into env: mappings
in both hotfix.yml (~12 instances) and release.yml (~16 instances) to prevent
potential command injection via GitHub Actions expression evaluation
- Reorder rc job in release.yml to run npm ci and test:coverage before pushing
the git tag, preventing broken tagged commits when tests fail
- Update VERSIONING.md to accurately describe the implementation: major releases
use beta pre-releases only, minor releases use rc pre-releases only (no
beta-then-rc progression)
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… guards Addresses deep adversarial review + best practices research: HIGH: - Fix release.yml rc/finalize: dry_run now gates tag+push (not just npm publish) - Fix hotfix.yml finalize: reorder tag-before-publish (was publish-before-tag) MEDIUM — Security hardening: - Pin ALL actions to SHA hashes (actions/checkout@11bd7190, actions/setup-node@39370e39, actions/github-script@60a0d830) - Add --provenance --access public to all npm publish commands - Add id-token: write permission for npm provenance OIDC - Add concurrency groups (cancel-in-progress: false) on both workflows - Add branch-naming.yml permissions: {} (deny-all default) - Scope permissions per-job instead of workflow-level where possible MEDIUM — Reliability: - Add post-publish verification (npm view + dist-tag check) after every publish - Add npm publish --dry-run validation step before actual publish - Add branch existence pre-flight check in create jobs LOW: - Fix VERSIONING.md Semver Rules: MINOR = "enhancements" not "new features" (aligns with Release Tiers table) Tests: 1166/1166 pass Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Last remaining action using a mutable version tag. Now all actions across all workflow files are pinned to immutable SHA hashes. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
d151caa to
271d10f
Compare
There was a problem hiding this comment.
Pull request overview
This PR introduces a documented 3-tier release strategy (hotfix/patch, release minor/major with RC/beta) and adds GitHub Actions automation for branching, PR gating, publishing, cleanup, and dependency maintenance.
Changes:
- Add
VERSIONING.mddescribing the release tiers, branching model, and dist-tag strategy. - Add
hotfix.ymlandrelease.ymlworkflows to automate patch and minor/major releases (includingnextprereleases andlatestpromotion). - Add supporting repo automation workflows/config: auto-branching from issue labels, advisory branch-name checks, PR-size labeling, stale cleanup, and Dependabot updates.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
| VERSIONING.md | Documents semver tiers, dist-tags, branch structure, and workflow usage. |
| .github/workflows/hotfix.yml | Patch/hotfix branch creation + publish-to-latest flow. |
| .github/workflows/release.yml | Minor/major release branch + RC/beta prerelease to next + finalize to latest. |
| .github/workflows/auto-branch.yml | Creates branches automatically when issues receive specific labels. |
| .github/workflows/branch-naming.yml | Warn-only branch naming convention validation for PRs. |
| .github/workflows/pr-gate.yml | PR size calculation + size label application. |
| .github/workflows/stale.yml | Scheduled stale issue/PR cleanup using actions/stale. |
| .github/dependabot.yml | Weekly npm + GitHub Actions dependency update PRs. |
| .github/workflows/test.yml | CI matrix adjustment (per PR description: drop Node 20). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Configure git identity in all committing jobs (hotfix + release) - Base hotfix on latest patch tag instead of vX.Y.0 - Add issues: write permission for PR size labeling - Remove stale size labels before adding new one - Make tagging and PR creation idempotent for reruns - Run dry-run publish validation unconditionally - Paginate listFiles for large PRs - Fix VERSIONING.md table formatting and docs accuracy Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
trek-e
left a comment
There was a problem hiding this comment.
No blocking security issues found. Reviewed the full workflow set:
- Version inputs are validated with regex before use and passed via
env:rather than interpolated directly intorun:blocks — no shell injection risk. - NPM_TOKEN is scoped to the
npm-publishenvironment with an approval gate. - Workflow permissions are explicitly scoped (pr-gate and auto-branch use only
contents: write+issues: writeor minimal permissions; stale uses actions permissions). - auto-branch.yml creates branches from issue titles but uses the GitHub script API (not shell exec), with the title going through a safe slug transform before branch naming.
- The PR is marked
review: approved (merge conflict)on #1579 and #1412 already — this PR likely needs a rebase. Check for merge conflicts before merging.
One observation: the stale.yml will auto-close issues with no activity in the configured days. Confirm the stale window is aligned with the maintainer's policy — the message currently says "if there is no further activity" without specifying the window. Users who see the stale warning should know how long they have.
trek-e
left a comment
There was a problem hiding this comment.
Code Review
Verdict: APPROVE
Security
No issues found.
All ${{ }} expression interpolations in run: blocks are routed through env: mappings — no direct interpolation into shell. All actions are pinned to immutable SHA hashes with version comments. The npm-publish environment gate requires human approval before any publish step. --provenance --access public is included on all publish commands with id-token: write scoped correctly. Version inputs are validated with a strict regex before being used in shell.
Logic / Correctness
One minor observation worth noting for awareness:
hotfix.ymldry-run scope: TheTag and pushstep is correctly gated on!inputs.dry_run, butnpm publishand the downstream verify/PR-creation steps are also individually gated. The flow is correct but slightly verbose — not a bug.release.ymlrc job ordering: The version bump commit happens beforenpm ci+test:coverageruns. If tests fail, the bump commit has already been made to the branch (but not tagged or pushed in dry_run mode). In the real path (!dry_run), the tag-and-push step follows the test step, so a test failure leaves a version bump commit on the branch without a tag. This is a minor cleanup concern (operator would need to amend or revert the bump commit manually), not a data-loss or security issue.- Tag auto-increment race: The
rcjob finds the next pre-release number by checking existing local tags inside the checked-out ref. If tworcruns are triggered in rapid succession, both could compute the sameNbefore either has pushed. Theconcurrency: cancel-in-progress: falsegroup mitigates this at the workflow level — only one run proceeds at a time per version. Correct. hotfix.ymlbase-tag selection: Usesawk -v target=$TARGET_TAG '$1 < target'with string comparison on version tags. For tags likev1.9.xvsv1.10.x, string sort would produce incorrect ordering. The precedingsort -V(version sort) is applied first, producing a correctly sorted list —awkthen finds the last entry before the target by string comparison on the already-sorted output. This is correct as long as all tags in the list come from the samesort -Vpass, which they do.
Test Coverage
This PR adds only CI/workflow YAML and a documentation file — no application logic. No unit tests are expected or applicable. All 1166 existing tests pass across all CI matrix targets (macOS, Ubuntu 22/24, Windows).
Style
- All actions pinned with inline version comments — good hygiene.
dependabot.ymlomitsversioning-strategyfor npm; the default (auto) is fine for this use case.stale.ymldoes not setexempt-all-milestonedorexempt-all-assignees; this is a policy choice, not a defect.- VERSIONING.md is thorough and accurate relative to the implementation.
Summary
Solid, well-hardened release infrastructure. The only substantive observation is that the rc job in release.yml commits a version bump before running tests, which can leave a dangling bump commit on the branch if tests fail — worth a follow-up but not a blocker.
trek-e
left a comment
There was a problem hiding this comment.
Code Review Update (Pass 2)
Verdict: APPROVE — CI passing, issues remain advisory only
Prior Review Resolution
The prior review approved the 3-tier release strategy. All identified issues were advisory (hotfix dry-run verbosity, release.yml version bump before tests, hotfix PR target branch safety). CI passes on all platforms with all checks including size-check and branch naming.
Outstanding Advisory Items (Not Blocking)
release.ymlversion bump before tests: If tests fail post-bump, the bump commit exists on the branch. This is an existing workflow design choice and not introduced by this PR.- The PR is 14 days old. Consider rebasing onto current main to resolve any potential conflicts with the many changes that have landed since March 21.
Summary
Ready to merge. CI is green, prior review is APPROVED, no blocking issues.
Summary
Supersedes #1208 and #1210 with a consolidated, npm-standard release strategy.
Three release tiers
1.27.1latesthotfix.yml1.28.0(after RC)next→latestrelease.yml2.0.0(after beta)next→latestrelease.ymlTwo npm dist-tags only (
latest+next), following Angular/Next.js convention. Pre-release stability communicated via version string (-rc.1vs-beta.1).Files added/changed
VERSIONING.md.github/workflows/hotfix.ymllatest.github/workflows/release.ymlnext→ finalize tolatest.github/workflows/auto-branch.yml.github/workflows/branch-naming.yml.github/workflows/pr-gate.yml.github/workflows/stale.yml.github/dependabot.ymlSecurity improvements over #1208/#1210
${{ }}interpolation inrun:blocks — uses environment variablesnpm-publishenvironment required for publish steps (human approval gate)Required setup before use
npm-publishwith protection rulesNPM_TOKENsecret to the environmentsize/S,size/M,size/L,size/XL) if using pr-gateCloses #1208
Closes #1210
Test plan
🤖 Generated with Claude Code