Skip to content

fix: require reviewed merge commits for stable releases - #2959

Merged
jrusso1020 merged 1 commit into
mainfrom
fix/release-merge-sha
Aug 3, 2026
Merged

fix: require reviewed merge commits for stable releases#2959
jrusso1020 merged 1 commit into
mainfrom
fix/release-merge-sha

Conversation

@jrusso1020

@jrusso1020 jrusso1020 commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • make a merged, reviewed release/vX.Y.Z PR the only stable (latest) publication path
  • pin every publish checkout to an immutable event SHA and verify the executable checkout unconditionally
  • reserve direct tag-push publishing for hyphenated prerelease tags (v*-*)
  • remove manual dispatch; recovery reruns the original immutable merge event
  • reject stable tag publishing in channel validation, including tags reachable only from an unmerged release/v* branch
  • make stable tag creation retry-safe: reuse the tag only at the expected merge commit and fail closed on a mismatch
  • parse the workflow in tests and assert the exact event gate, checkout wiring, guard, and tag-recovery implementation
  • document the protected stable release and recovery workflow

Root cause

The original merged-PR workflow checked out mutable main, so a concurrent merge could change the release contents after review. The first fix closed that arm but left two related gaps: manual dispatch resolved a mutable tag name, and a stable tag on an unmerged release branch could publish latest. Its regex tests also did not prove the guard step was executable.

The updated model has one stable path: merge a reviewed release PR into protected main. That event supplies the exact merge SHA used by checkout, tag creation, npm publication, and the GitHub release. Failed stable publishes are retried by rerunning that same event, not by selecting a new ref. Tag creation is idempotent for that exact commit and rejects an existing tag that points anywhere else.

Event matrix

  • merged release/vX.Y.Z PR → stable release from pull_request.merge_commit_sha
  • vX.Y.Z-channel.N tag push → prerelease from immutable github.sha, with prerelease-branch validation
  • stable tag push → no workflow trigger and validator rejects it defensively
  • manual dispatch → removed and validator rejects it defensively

Verification

  • bun run test:scripts — 110 passed
  • end-to-end temporary-repository test proves first tag creation, same-SHA retry, and mismatched-SHA rejection
  • bunx fallow audit --base origin/main --fail-on-issues --format pr-comment-github — no findings
  • bunx oxfmt --check .github/workflows/publish.yml scripts/publish-workflow.test.mjs scripts/validate-release-channel.mjs scripts/validate-release-channel.test.mjs package.json
  • bunx oxlint scripts/publish-workflow.test.mjs scripts/validate-release-channel.mjs scripts/validate-release-channel.test.mjs
  • pre-commit and commit-message hooks passed

Addresses Magi’s stable-path and structural-test blockers plus Via’s recovery-idempotency and exact-gate observations. The v0.7.90 release remains blocked until exact head 41e603507 is approved and merged.

@miga-heygen miga-heygen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Release Integrity Review — PR #2959 6c598d2b

Event matrix — CORRECT

Three publish triggers, each now pinned to an immutable ref:

Event Old ref: New ref: Immutable?
Merged PR (pull_request + merged == true) github.ref (= refs/heads/mainmutable, can advance) github.event.pull_request.merge_commit_sha
Manual dispatch (workflow_dispatch) format('refs/tags/v{0}', inputs.version) Same (unchanged)
Tag push (push on v*) github.ref (= refs/tags/vX.Y.Z — immutable) github.sha (event's commit SHA — immutable)

The merged-PR case was the vulnerability: github.ref is refs/heads/main, and between merge and workflow execution, another commit could land on main. The publish would then tag+npm-publish a commit that was never reviewed. merge_commit_sha is the exact immutable SHA of the merge commit.

Verification step — CORRECT

- name: Verify merged release checkout
  if: github.event_name == 'pull_request'
  run: |
    ACTUAL_SHA="$(git rev-parse HEAD)"
    if [ "$ACTUAL_SHA" != "$EXPECTED_MERGE_SHA" ]; then
      echo "::error::..."
      exit 1
    fi

Fails the workflow if the checkout doesn't match the expected merge SHA. Only runs for merged-PR events (manual dispatch and tag push use different pinning). ✓

Expression chain — CORRECT

GitHub Actions &&/|| ternary chain:

pull_request && merge_commit_sha || workflow_dispatch && tag_ref || github.sha
  • PR: 'pull_request' == 'pull_request' → truthy → && merge_commit_sha → returns SHA
  • Dispatch: first clause falsy → 'workflow_dispatch' == 'workflow_dispatch' → truthy → && tag_ref → returns tag
  • Tag push: both clauses falsy → || github.sha → returns event SHA

Regression guard — ROBUST

scripts/publish-workflow.test.mjs reads the YAML and asserts:

  1. merge_commit_sha is used for PR events ✓
  2. Verification step exists with SHA comparison ✓
  3. github.sha is the fallback (NOT github.ref) ✓
  4. github.ref is explicitly absent ✓

Added to test:scripts in package.json. Cannot be silently removed without test-suite changes visible in PR review.

Bypass analysis

  • Push to main after merge: workflow checks out merge_commit_sha, not branch tip. Safe.
  • merge_commit_sha null/undefined: populated on all merged PRs (merge, squash, rebase). Safe.
  • Skip verification step: only skipped for non-PR events, which use tag refs or github.sha. Safe.
  • Remove regression test: visible in package.json diff. Not silent.
  • Modify workflow file: requires PR review. Regression test catches pattern removal.

Verdict: Approve. Clean fix for a real release-integrity vulnerability. The immutable-ref pinning, verification step, and regression guard are all correct.

— Miga

@miguel-heygen miguel-heygen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The merged-PR arm is materially better: .github/workflows/publish.yml:42-46 selects pull_request.merge_commit_sha, and :48-57 independently fails if checkout does not match the event SHA. Miga covered that arm. Two gaps remain in the requested event-matrix / regression-guard contract:

  • [P1] Manual/stable recovery still does not prove an immutable reviewed commit — .github/workflows/publish.yml:5-17, :28-57; scripts/validate-release-channel.mjs:5, :65-77. workflow_dispatch still checks out the mutable name refs/tags/v<input> and has no SHA comparison because the verification step runs only for pull_request. This repository has no tag ruleset, so the tag can move between dispatch and checkout—the same TOCTOU class as mutable main. More importantly, tag-push and dispatch are accepted for a stable commit merely reachable from origin/release/v*; that branch need not have merged or been reviewed. I reproduced validateReleaseChannel({version:"0.7.90", distTag:"latest", eventName:"workflow_dispatch", remoteBranches:["origin/release/v0.7.90"]}) returning no errors. A tag on an unmerged release branch can therefore publish latest. For stable releases, either make merged-PR runs (and re-runs of that immutable event) the only path, or require an explicit commit SHA, verify the tag peels to it, and require that SHA to be reachable from protected main rather than an unmerged release branch.

  • [important] The regression test does not bind its assertions to the executable checkout/guard — scripts/publish-workflow.test.mjs:7-26. It searches the entire YAML for independent regex fragments. Replacing the guard's if: github.event_name == 'pull_request' with if: false leaves every current assertion green; the same strings could also survive in comments or an unused step. Parse the workflow (or factor the event→SHA resolver/guard into a tested script) and assert the exact checkout with.ref, guard condition, and all three event arms. I ran this mutation in memory and confirmed all current assertions still pass.

Focused evidence: node --test scripts/publish-workflow.test.mjs passes (2/2); git diff --check and syntax check pass. The full script suite could not run in the clean review worktree because dependencies were not installed (tsx absent); CI currently has no failures and six jobs are still running.

Audited: .github/workflows/publish.yml, scripts/publish-workflow.test.mjs, scripts/validate-release-channel.mjs, existing channel tests, repository ruleset/environment policy, and the three event paths. Trusting: unrelated package tests skipped by change detection.

Verdict: REQUEST CHANGES

Reasoning: The merged-PR race is fixed, but the manual/tag stable paths can still publish an unreviewed or tag-raced commit, and the added regression guard does not detect a disabled verification step.

— Magi

@vanceingalls vanceingalls left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Release-integrity re-review — PR #2959 6c598d2b

James asked me to confirm tag / npm / GitHub-release can only cut from an immutable reviewed commit, and to attack the regression guard across the three event types. Independent pass; agree with Miga's verdict, added an adversarial sweep.

Axis 1 — Merged-PR event (pull_request closed + merged, release/v*)

  • ref: resolves to github.event.pull_request.merge_commit_sha — immutable, populated on all merged PRs regardless of merge method. PASS
  • Verify merged release checkout step (if: github.event_name == 'pull_request') does git rev-parse HEAD != EXPECTED_MERGE_SHA → exit 1. No continue-on-error, no secret-gated bypass. Fails closed. PASS
  • Synthesized-event bypass: GitHub controls pull_request.closed+merged=true payloads; no user-driven synthesis. PASS
  • Approval invariant relies on the main ruleset (ruleset_id=14211637): required_approving_review_count=1, require_last_push_approval=true, non_fast_forward, org-wide signed-commit enforcement. Attack (b) — post-approval push landing without re-approval — is prevented by require_last_push_approval. Verified.

Axis 2 — Manual dispatch (workflow_dispatch)

  • Only input is version: string. Workflow shapes it into format('refs/tags/v{0}', inputs.version) — so the checkout must resolve to an existing tag. Dispatch cannot inject an arbitrary sha, only choose among existing v* tags. PASS (scoped to this PR)
  • Deployment protection: environment: npm-publish with deployment_branch_policy.protected_branches=true (dispatch runs from main, which is protected).
  • Follow-up not in scope: dispatch does not re-verify that the tag points at a merge-commit sha (see "Out-of-scope" below).

Axis 3 — Tag-push (on.push.tags: 'v*')

  • ref: falls through the &&/|| chain to github.sha (event's tag commit SHA), replacing the old github.ref. Semantically equivalent for push events but more defensively worded. PASS
  • validate-release-channel.mjs requires the tagged commit be reachable from origin/main or origin/release/v* (stable) / origin/{next,alpha,beta,rc,canary,prerelease/*} (prerelease). Defence-in-depth beyond this PR's scope.

Axis 4 — Immutable-commit + regression guard

  • Tag creation (git tag "v$VERSION" && git push origin "v$VERSION"), npm publish, and GH-release all run AFTER the checkout, and the Verify merged release checkout step guarantees HEAD == merge_commit_sha before any of them. Same SHA across all three artefacts. PASS
  • &&/|| precedence: (pull_request && merge_commit_sha) || (workflow_dispatch && tag_ref) || github.sha — verified operand-by-operand for the three event types. PASS
  • Regression test scripts/publish-workflow.test.mjs asserts (a) merge_commit_sha is used on PR, (b) Verify merged release checkout step is present, (c) github.sha is the final fallback, and (d) assert.doesNotMatch(workflow, /\|\| github\.ref/) explicitly blocks the racy fallback. Wired into test:scripts in package.json — cannot be silently dropped. PASS

Axis 5 — Adversarial bypass sweep

Attack Blocked? Where
(a) self-approve via 2nd account Blocked ruleset requires approval; org has CODEOWNER + 2FA policy; not further weakened by this PR
(b) post-approval push (dismiss-on-push) Blocked ruleset require_last_push_approval: true
(c) dispatch arbitrary ref Blocked input is version: string → shaped into refs/tags/v{ver}, not raw ref
(d) v* tag on malicious commit Not blocked at ruleset (no tag ruleset) but out of scope for this PR — pre-existing surface; mitigated by validate-release-channel branch-reachability check
(e) post-merge additional commits before workflow runs Blocked — this is the exact race this PR fixes; checkout is pinned to merge_commit_sha, not main's tip
(f) concurrent-merge HEAD race Blocked — same as (e); github.ref/github.sha on pull_request events would be main's tip, merge_commit_sha is the specific merge

Fail-open path audit

Grepped the diff for if: bypass, continue-on-error, secrets.SKIP_*, --force, ${{ vars.* }} toggles — none present. The guard is unconditional for pull_request events.

Out of scope for this PR (follow-ups, not blockers)

  • Tag-push and dispatch paths do not verify that the resolved commit was actually approved in a release PR — they lean on validate-release-channel branch reachability, which is a soft check because release/v* branches aren't in the ruleset. Consider adding a step that resolves the checked-out SHA to a merged PR (gh pr list --search "sha:<sha>") and rejects if none exists, or moving release/v* under the ruleset. Filing this as a follow-up rather than a blocker on #2959 because #2959's charter is the merged-PR race specifically, and the release PR (#2958) that motivated it takes the merged-PR path.

Verdict — APPROVE

Clean, correctly scoped fix for the release-integrity race. Immutable-ref pinning across all three event types, fail-closed regression guard, machine-readable test that prevents silent regression. #2958 can safely rebase on top of this once required checks finish (Test / Windows-latest / CLI smoke / GCP BeginFrame currently in-progress on the head SHA).

— Via

@vanceingalls vanceingalls left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reversing my APPROVE — Magi's P1s are correct on independent verification

Posted APPROVE 3s after Magi's CHANGES_REQUESTED — parallel-reviewer race. Re-ran his two claims from a clean worktree at head 6c598d2b; both reproduce. James's ask was explicitly the three-event matrix ("confirm the tag/npm/GitHub release can only use an immutable reviewed commit; and challenge the regression guard if it can be bypassed"), so the gaps Magi flagged are in-scope, not follow-ups.

Confirmed: mutation-escape of the regression test — [important]

Reproduced Magi's mutation. From the head worktree:

sed -i "s|if: github.event_name == 'pull_request'|if: false|" .github/workflows/publish.yml
node --test scripts/publish-workflow.test.mjs
# tests 2  pass 2  fail 0

Setting the guard's if: to if: false disables the runtime verification entirely, yet both regex-fragment assertions still match (the step name, env, and body strings survive in the file). The test asserts text presence, not structural gating or reachability. In practical terms: someone could push a "cleanup" commit that guts the guard and CI would stay green. The regression guard James asked me to challenge does not, in its current form, detect its own removal. This alone matches Magi's block.

Confirmed: manual/tag-push accept unreviewed commits — [P1]

Reproduced Magi's validateReleaseChannel call at head:

validateReleaseChannel({
  version: '0.7.90', distTag: 'latest', eventName: 'workflow_dispatch',
  remoteBranches: ['origin/release/v0.7.90']
})
// → []  (no errors — publish allowed)

STABLE_BRANCH_RE = /^origin\/(main|release\/v.+)$/ (validate-release-channel.mjs:5). release/v* branches are not in the repo ruleset (gh api /repos/heygen-com/hyperframes/rulesets → only main is ruleset-protected; no tag ruleset either). So an attacker with write access can:

  1. Push commit to a new release/v99.99.99 branch (no ruleset).
  2. Push tag v99.99.99 at that commit (no tag ruleset).
  3. Either the tag-push event or a manual workflow_dispatch (version=99.99.99) triggers publish.
  4. validate-release-channel passes because the SHA is reachable from origin/release/v99.99.99.
  5. npm sees hyperframes@99.99.99 published from an unreviewed commit.

Additionally: workflow_dispatch checks out refs/tags/v<input> — a mutable name. Between dispatch and checkout, a compromised tag can be moved without SHA verification (no Verify … step gates the dispatch/tag-push paths). This is the same TOCTOU class as the mutable-main race that this PR is fixing for the PR arm.

What this means for scope

I still agree the merged-PR arm is materially better and closes the specific race that motivated #2958. But James's confirmation was against the three-event matrix, not the merged-PR arm alone, and both of Magi's findings verify. I retract my earlier "out of scope" defence.

Proposed fixes (aligned with Magi)

  1. Make the guard structural, not textual: parse the YAML in scripts/publish-workflow.test.mjs and assert (a) the checkout with.ref expression is exactly the ternary chain, (b) the Verify merged release checkout step has the correct if: gate and script body, (c) github.ref is absent from ref/sha resolution. Mutation on if: should turn the test red.
  2. Bind stable dispatch/tag-push to reviewed commits: either drop release/v* from STABLE_BRANCH_RE (so stable requires reachability from origin/main, which IS ruleset-protected), or add a workflow step that resolves the checked-out SHA to a merged PR (gh pr list --search "<sha>" --state merged) and fails closed.
  3. Extend the Verify … step to dispatch/tag-push: for dispatch, resolve the tag to a SHA and assert the SHA matches an expected input or is reachable from protected main; for tag-push, verify the SHA is reachable from protected main (not release/v*).

Any one of #1 alone would address the regression-guard bypass. #2 is the minimum to close the "immutable reviewed commit" invariant for the manual/tag paths.

Verdict: REQUEST CHANGES (reversing my prior APPROVE at unchanged SHA).

— Via

@jrusso1020
jrusso1020 force-pushed the fix/release-merge-sha branch from 6c598d2 to 3eb2d34 Compare August 3, 2026 04:02
@jrusso1020 jrusso1020 changed the title fix: pin release publishing to merge commit fix: require reviewed merge commits for stable releases Aug 3, 2026

@miga-heygen miga-heygen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Release Integrity v2 Review — PR #2959 3eb2d34b

Substantially stronger than R1. The event model is now one reviewed path — everything else is either eliminated or defensively rejected.

Event model — ONE PATH for stable releases

Event Old behavior New behavior
Merged release/vX.Y.Z PR Published (from mutable github.ref) Published (from immutable merge_commit_sha)
Manual dispatch (workflow_dispatch) Published (from specified tag) Removed entirely
Stable tag push (v0.7.90) Published (from tag ref) Rejected — trigger pattern v*-* excludes stable tags
Prerelease tag push (v0.7.90-alpha.1) Published (from tag ref) Published (from github.sha) — unchanged

Double-guarded stable tag rejection

  1. Workflow trigger: v*-* pattern — stable tags without a hyphenated suffix don't match, workflow never fires.
  2. Channel validation: validateReleaseChannel explicitly blocks stable tag push events: "Stable tag publishing is disabled."

Immutable checkout — UNCONDITIONAL

env:
  EXPECTED_RELEASE_SHA: >-
    ${{ pull_request && merge_commit_sha || github.sha }}
  • Computed once as env var, used by checkout AND verification.
  • Verification step has NO if: condition (runs unconditionally), NO continue-on-error.
  • git rev-parse "${EXPECTED_RELEASE_SHA}^{commit}" — dereferences annotated tags for comparison.

Regression test — STRUCTURAL, NOT PATTERN

Parses the YAML with the yaml library and asserts:

  1. Tag trigger is exactly ["v*-*"]
  2. workflow_dispatch is undefined
  3. EXPECTED_RELEASE_SHA env var uses merge_commit_sha
  4. Checkout ref uses the env var ✓
  5. Guard step name is "Verify immutable release checkout"
  6. Guard has no if: condition (undefined) ✓
  7. Guard has no continue-on-error (undefined) ✓
  8. Guard script content matches exactly (character-for-character) ✓

Mutating any of these — adding if: false, adding continue-on-error: true, changing the tag pattern, restoring workflow_dispatch — fails the test.

Recovery path — IMMUTABLE

Rerunning the original merged-PR workflow event re-checks out the same merge_commit_sha. Cannot select a different commit. Documented in updated contributing guides.

Docs — UPDATED

Both changelog-process.mdx and release-channels.mdx reflect the single-path model. Old instructions to git push origin main --tags are replaced with the PR-based flow.

Bypass analysis

  • Push stable tag manually: workflow doesn't fire (v*-* excludes it). No publish.
  • Force tag push via API: same trigger — doesn't match.
  • Restore workflow_dispatch: test asserts config.on.workflow_dispatch === undefined. Fails.
  • Add if: false to guard: test asserts checkoutGuard.if === undefined. Fails.
  • Add continue-on-error: true: test asserts checkoutGuard['continue-on-error'] === undefined. Fails.
  • Change checkout ref: test asserts exact ${{ env.EXPECTED_RELEASE_SHA }}. Fails.
  • Modify guard script: test asserts exact character-for-character content. Fails.

Verdict: Approve. This is a correct, complete, and well-guarded single-path publish model. No bypass vectors found.

— Miga

@mintlify

mintlify Bot commented Aug 3, 2026

Copy link
Copy Markdown

Preview deployment for your docs. Learn more about Mintlify Previews.

Project Status Preview Updated (UTC)
hyperframes 🟢 Ready View Preview Aug 3, 2026, 4:07 AM

💡 Tip: Enable Workflows to automatically generate PRs for you.

@jrusso1020
jrusso1020 force-pushed the fix/release-merge-sha branch from 3eb2d34 to 6024f44 Compare August 3, 2026 04:07

@vanceingalls vanceingalls left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

R2 verification at head 3eb2d34bb — all six axes clean. APPROVE.

Axis 1 — Manual dispatch removed: workflow_dispatch: is absent from .github/workflows/publish.yml. Belt-and-suspenders: validate-release-channel.mjs explicitly rejects eventName === "workflow_dispatch" with an Unsupported publish event error (asserted verbatim in validate-release-channel.test.mjs). No path from a dispatch trigger reaches npm publish, tag creation, or GH release creation.

Axis 2 — Stable tag push defensively rejected:

  • (a) on.push.tags: ["v*-*"] — pattern requires a hyphen, so stable v1.2.3 cannot match. Test asserts deepEqual(config.on.push.tags, ["v*-*"]) so widening the filter turns the test red.
  • (b) Even if a stable version leaked through, the validator's push branch does if (!isPrerelease) { errors.push("Stable tag publishing is disabled…"); return errors; } — hard block. Two tests (blocks stable tag pushes even when reachable from main, blocks stable tags that only live on an unmerged release branch) pin this rejection.
  • Both the trigger pattern and the reject message are exact-string asserted; flipping either turns tests red.

Axis 3 — Guard test mutation catches:

  • Guard step Verify immutable release checkoutcheckoutGuard.if asserted undefined. Adding if: false → parsed as false, assert.equal(false, undefined) fails. RED.
  • checkoutGuard["continue-on-error"] asserted undefined. Adding continue-on-error: true → RED.
  • Checkout step with.ref asserted verbatim as "${{ env.EXPECTED_RELEASE_SHA }}"; and EXPECTED_RELEASE_SHA itself is asserted whitespace-normalized against the exact expression. Any attacker-controlled ref substitution → RED.

Axis 4 — Recovery via immutable merge event: the workflow reads github.event.pull_request.merge_commit_sha (immutable field on the original event payload, preserved across GitHub re-runs). Checkout pins to EXPECTED_RELEASE_SHA, the guard step re-verifies git rev-parse HEAD against it, and npm/GH release both check-then-skip (idempotent). No new attack surface introduced by re-run: the merge commit is content-addressed, so re-runs cannot smuggle in modified files even if main has since advanced.

Axis 5 — Validator now rejects unmerged release/v*: the R1 bypass required STABLE_BRANCH_RE to include release/v* under a push event. That has been removed. PRERELEASE_BRANCH_RE = /^origin\/(next|alpha|beta|rc|canary|prerelease\/.+)$/ no longer contains release/v*. Mentally executing the previous bypass input (eventName: "push", version: "0.7.90", remoteBranches: ["origin/release/v0.7.90"]) now hits the !isPrerelease short-circuit and returns ["Stable tag publishing is disabled…"]. The prerelease-suffix variant (0.7.90-alpha.1 on origin/release/v0.7.90) falls through to allowedBranch — the release branch does not match PRERELEASE_BRANCH_RE, so it too is rejected. Both bypasses closed.

Axis 6 — Residual bypass paths:

  • No workflow_call: on publish.yml — not reusable.
  • Only codeql.yml has schedule:; not related to publishing.
  • No repository_dispatch: triggers anywhere.
  • NPM_TOKEN/npm-publish environment gate is unique to publish.yml. sync-skills-to-clawhub.yml publishes to ClawHub with a separate secret and does not touch npm.
  • CODEOWNER/admin merge is a repo-policy question outside this workflow's scope (a policy bypass on main would still produce a valid merge_commit_sha; that risk is inherent to trusting protected main and unchanged from before this PR).

Follow-ups (non-blocking):

  • The Create release tag step (git tag "v$VERSION" then git push origin) is not idempotent on re-run. If a prior run succeeded past tag push, actions/checkout@v4 with fetch-depth: 0 may or may not include tags depending on fetch-tags (defaults to false in v4, so this is likely fine in practice), but worth confirming — or making the step skip when the tag already exists — since the docs promise "rerun that immutable merge event for recovery".
  • publish.if is regex-asserted (/…merged == true[\s\S]*release\/v/) rather than string-equal, so a mutation that appends an extra permissive || clause would still match. Not exploitable in isolation because trigger events and validator gates converge on the same requirements, but tightening the assertion (or asserting the full if string like EXPECTED_RELEASE_SHA does) would remove that gap.

108/108 tests passing. Verified locally by tracing the test assertions against each mutation scenario listed in R1.

— Via

@miga-heygen miga-heygen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Release Integrity v3 Review — PR #2959 6024f44f

Clean refactor of validate-release-channel.mjs driven by a Fallow finding. The stable event invariant and parsed executable-guard contract from v2 are unchanged.

Validator refactor

Monolithic validateReleaseChannel decomposed into focused validators:

  • validateDistTag(version, distTag) — dist-tag matches version pattern
  • validateMergedReleasePr({version, prHeadRef}) — branch naming + stable-only gate
  • validatePrereleaseTagPush({version, distTag, remoteBranches}) — stable-tag rejection + branch reachability
  • validateReleaseSource(input) — dispatches via EVENT_VALIDATORS map
  • validateReleaseChannel(input) — orchestrates: version format → dist-tag → source

The EVENT_VALIDATORS map (pull_request → validateMergedReleasePr, push → validatePrereleaseTagPush) makes the event model explicit. Unknown events fall through to the "Unsupported publish event" error. workflow_dispatch has no entry → rejected.

Invariants preserved from v2

  1. Workflow trigger: v*-* only (stable tags excluded) ✓
  2. workflow_dispatch removed
  3. Immutable checkout from EXPECTED_RELEASE_SHA env var ✓
  4. Unconditional verification guard (no if:, no continue-on-error) ✓
  5. Regression test parses YAML and asserts exact values ✓
  6. Stable tag push explicitly blocked in channel validation ✓

Fallow resolution

The refactor eliminates dead branches that Fallow flagged (the old isPrerelease variable governed both PR and push paths in a single function; decomposing removes the shared state). No suppression needed.


Verdict: Approve. Same security properties as v2, cleaner structure. No bypass vectors.

— Miga

@miguel-heygen miguel-heygen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

R3 at exact head 6024f44fd — both prior blockers are closed.

  • .github/workflows/publish.yml:5-13 leaves only hyphenated prerelease tag pushes plus merged PR events; manual dispatch is absent and stable tags cannot trigger the workflow. :24-51 derives one event SHA, checks out that SHA, and runs an unconditional, non-continue-on-error comparison before version resolution, tagging, npm publication, or GitHub release creation.
  • scripts/validate-release-channel.mjs:36-81 now dispatches by event type, accepts stable only from a syntactically valid merged release/vX.Y.Z PR, rejects every stable tag push, rejects unsupported/manual events, and keeps prerelease pushes branch-gated. The Fallow-driven decomposition preserves those decisions and current-head Fallow is green.
  • scripts/publish-workflow.test.mjs:14-40 parses the executable YAML and binds assertions to the actual trigger, checkout ref, guard condition, continue-on-error, and guard body. I independently mutated if:false, continue-on-error:true, the checkout ref, restored manual dispatch, and restored stable-tag matching; all five mutations turned the contract red while the original remained green.

Focused verification: validator matrix and its 10 tests pass; parsed event/guard contract and five mutations pass; git diff --check and both changed scripts' syntax checks pass. Current-head CI has no failures; required Windows, Typecheck, Build, Test, and CLI jobs are still running.

Verdict: APPROVE

Reasoning: Stable publication now has one reviewed, immutable merged-PR path, both former alternate stable paths fail closed, and the structural regression tests catch executable guard/trigger bypasses.

— Magi

@vanceingalls vanceingalls left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-verify at 6024f44fd — APPROVE.

R1's two P1s and R2's single-path claim are all closed at this head. The R2→R3 refactor tightens semantics rather than suppressing anything.


Axis 1 — P1-A (guard-step mutation-catches): PASS

scripts/publish-workflow.test.mjs parses the workflow YAML and asserts content, not just presence:

  • if: false on the "Verify immutable release checkout" step → assert.equal(checkoutGuard.if, undefined) fails.
  • continue-on-error: trueassert.equal(checkoutGuard["continue-on-error"], undefined) fails.
  • Mutating checkout.with.ref off ${{ env.EXPECTED_RELEASE_SHA }} → strict assert.equal on the exact expression fails.
  • The run block is asserted verbatim (line-by-line join), so any tampering with the shell script fails too.

Belt-and-braces: the executable guard step itself does git rev-parse HEAD vs EXPECTED_COMMIT_SHA and exit 1 on mismatch — runtime enforcement even if a static test were somehow bypassed.

Axis 2 — P1-B (validator rejects unmerged release/v*): PASS

STABLE_BRANCH_RE is removed entirely from scripts/validate-release-channel.mjs. The push-event validator (validatePrereleaseTagPush) now short-circuits with Stable tag publishing is disabled… whenever expectedDistTag === "latest", regardless of branch reachability. Test blocks stable tags that only live on an unmerged release branch exercises the exact P1-B scenario (remoteBranches: ["origin/release/v0.4.24"], eventName: "push") and asserts the disabled-message. Stable can now publish only via merged-PR event with RELEASE_PR_RE.test(prHeadRef) (^release/v\d+\.\d+\.\d+$) + pull_request.merged == true gate at the workflow layer.

Axis 3 — refactor didn't reopen bypass: PASS

R2→R3 delta is one commit (6024f44fd); the validator split into validateDistTag / validateMergedReleasePr / validatePrereleaseTagPush dispatched via a Map. No broadened signatures (each helper takes the same input shape), no hidden defaults, no dropped guards. The dispatcher returns Unsupported publish event "…" for anything not in the map — workflow_dispatch is explicitly rejected (asserted in the new "rejects manual publish events" test). Tighter semantics on the previously-permissive "stable tag reachable from main" case (now REJECTED, whereas R1's ancestor allowed).

Axis 4 — single-path: PASS

.github/workflows/publish.yml on: block has only push.tags: ["v*-*"] (hyphenated ⇒ prerelease-only) and pull_request.types: [closed] branches: [main]. Grepped: no workflow_dispatch, no workflow_call, no schedule:, no repository_dispatch. Job-level if: requires either push OR (pull_request.merged == true && startsWith(head.ref, 'release/v')). Even if a stable-shaped v* tag were pushed, it wouldn't match the v*-* trigger; the workflow never fires.

Axis 5 — Fallow "fix" is real, not a suppression: PASS

CI's Fallow audit passes at this head. Grepped tree for fallow-ignore, // fallow, # fallow, --skip, --ignore — no suppression comments, no allowlist entries, no skipped invocations. The refactor is genuinely semantic: helpers split, STABLE_BRANCH_RE removed, dispatch table introduced, and process.exit(1)process.exitCode = 1 in main. Fallow's finding was resolved by reshaping the code, not by silencing the linter.

Followups: none blocking. Optional nit for a future PR: startsWith(github.event.pull_request.head.ref, 'release/v') at the workflow if: layer permits release/vfoo while the validator narrows to ^release/v\d+\.\d+\.\d+$ — validator catches it, but the outer gate could match the validator's shape for symmetry.

— Via

@miga-heygen miga-heygen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Release Integrity v4 Review — PR #2959 41e60350

Addresses Via's recovery/race observations. Tag creation is now idempotent, fail-closed, and noninteractive. All prior invariants preserved.

Idempotent tag creation — CORRECT

The "Create release tag" step (guarded by if: github.event_name == 'pull_request') implements:

  1. Tag exists? git ls-remote --refs origin "refs/tags/$TAG"
    • Yes → verify_remote_tag (fetch, compare SHA, fail if mismatch, skip if same)
    • No → create + push
  2. Push fails? (concurrent retry race) → delete local tag → verify_remote_tag
  3. --no-sign — noninteractive, no GPG prompt

Recovery scenarios:

  • Rerun after npm failure: tag already exists at merge SHA → "already exists at the expected commit" → continues to npm publish. ✓
  • Concurrent retry: first wins the push, second's push fails → falls to verify_remote_tag → same SHA → continues. ✓
  • Manual tag at wrong commit: verify_remote_tag sees SHA mismatch → exit 1. Fail-closed. ✓

End-to-end Git test — ROBUST

Creates a temporary bare repo + checkout, runs the actual createReleaseTag.run script via bash -euo pipefail, tests three scenarios:

  1. First create: tag at correct SHA, exit 0 ✓
  2. Same-SHA retry: "already exists", exit 0 ✓
  3. Different-commit mismatch: "points to ... expected ...", exit 1 ✓

Cleans up with rmSync(root, { recursive: true, force: true }) in finally block.

Structural test updates

  • Asserts createReleaseTag.if === "github.event_name == 'pull_request'"
  • Asserts exact script content character-for-character ✓
  • Asserts full publish-job event gate expression ✓
  • All prior assertions (tag pattern, no workflow_dispatch, unconditional checkout guard) preserved ✓

Invariants preserved from v2/v3

  1. Stable tags excluded from trigger (v*-* only) ✓
  2. workflow_dispatch removed ✓
  3. Immutable EXPECTED_RELEASE_SHA checkout ✓
  4. Unconditional checkout verification (no if:, no continue-on-error) ✓
  5. Channel validation blocks stable tag pushes ✓
  6. Validator decomposed into focused functions ✓

Verdict: Approve. The recovery/race behavior is sound. Tag creation is idempotent for the exact commit, fails closed on mismatch, and the end-to-end test proves all three scenarios. No bypass vectors.

— Miga

@miguel-heygen miguel-heygen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

R4 at exact head 41e60350725653641495088379d1129285fb94d9 — the recovery gap is closed.

  • .github/workflows/publish.yml:86 makes stable-tag creation idempotent without weakening the immutable-release invariant: an existing tag must resolve to the checked-out merge commit, a concurrent same-SHA push race is recovered by fetch-and-verify, and any mismatched target fails closed. There is no force-tag or force-push fallback, and --no-sign keeps creation noninteractive.
  • scripts/publish-workflow.test.mjs:69 pins the executable tag step structurally and then runs that exact script against a temporary bare Git repository. It covers first creation, same-SHA rerun, and rejection when the tag points at a different commit.
  • The earlier protections remain intact: stable publishing has one reviewed merged-release-PR path, checkout and verification use the immutable merge SHA, the verification guard is unconditional, manual dispatch is absent, and stable tag pushes cannot enter the publish workflow.

I independently executed the extracted workflow script against a temporary bare repository and observed all three expected outcomes: first create succeeded, same-SHA retry succeeded, and mismatched-SHA retry failed. I also verified the script contains no force-tag/force-push or interactive operation, ran the release-channel validator suite (10/10), checked the test file parses, and confirmed git diff --check is clean. Current-head CI has no failing checks; remaining jobs are still in progress.

Verdict: APPROVE

Reasoning: The reviewed merge commit remains the sole stable release source, while retries are now safe and idempotent. Existing or racing tags can only be reused when they resolve to that exact commit; all divergent targets fail closed, and the behavior is protected by both structural and real-Git regression coverage.

— Magi

@vanceingalls vanceingalls left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

R4 re-verify APPROVE at 41e603507.

Traced the six-axis threat model against .github/workflows/publish.yml, scripts/publish-workflow.test.mjs, and scripts/validate-release-channel.mjs + tests.

Axis 1 — Idempotent tag creation. Create release tag (lines 86-113) leads with git ls-remote --refs origin "refs/tags/$TAG" — remote authority, not local. Exists → verify_remote_tag() fetches the tag, git rev-parse refs/tags/$TAG^{commit} against EXPECTED_TAG_SHA (git rev-parse HEAD after the immutable checkout), and either prints "already exists at the expected commit — skipping" (exit 0) or ::error:: + exit 1. Fresh path uses git tag --no-sign "$TAG" "$EXPECTED_TAG_SHA" (no -f) and git push origin "refs/tags/$TAG" (no --force). No || true, no --force-with-lease, no bash-swallow. The --force on the fetch refspec is on the local read side, not a push.

Axis 2 — Fail-closed on mismatched SHA. Ordering is verify_remote_tag (in the Create-release-tag step) → subsequent Publish packages / Create GitHub Release steps. Since GH Actions' default bash aborts on nonzero and step failure short-circuits the job, an attacker-pre-created vX.Y.Z at wrong SHA exits before any npm publish. Verified by the E2E test's mismatch case landing status 1 with no push side-effect (only local ref fetched, then diverges).

Axis 3 — Noninteractive. git tag --no-sign explicitly disables GPG prompts. No --interactive, --edit, read -p, or --force-with-lease anywhere in the added script. No git commit -e in the release path.

Axis 4 — E2E test. stable release tag creation survives retries and rejects a mismatched commit (lines 102-133) builds a real bare origin.git + a real working checkout, and drives the actual createReleaseTag.run body through bash via spawnSync. First-create → status 0 + tag at HEAD. Same-SHA retry → status 0 + stdout matches /already exists at the expected commit/. New empty commit moves HEAD → run again → status 1 + stdout matches /points to .* expected/. Real repo, not mocked. Two nits (non-blocking): the three scenarios are chained in one test() rather than isolated it() blocks (scenarios logically depend on each other, so tolerable), and the mismatch case asserts the process exit + stderr but does not explicitly re-assert that refs/tags/v9.8.7 on origin still points at the original SHA — the code path proves it (no push runs) but a direct assertion would close the loop.

Axis 5 — R3 P1 regression check.

  • Guard-step mutation test (the executable checkout guard cannot be conditionally disabled) still asserts checkoutGuard.if === undefined, continue-on-error === undefined, and the exact run body. if: false mutation would flip .if from undefined to false and blow the strict-equal — catch still lives.
  • validate-release-channel.mjs validatePrereleaseTagPush (lines 51-56) unconditionally rejects any push event with expectedDistTag === "latest" regardless of remoteBranches. Test blocks stable tags that only live on an unmerged release branch (lines 43-54) exercises exactly the R1 P1-B scenario. And the tag trigger tightened to v*-* at the workflow level means stable tag pushes cannot even reach the job filter — belt + suspenders both intact.

Axis 6 — Adversarial re-scan.

  • EXPECTED_RELEASE_SHA sourced from github.event.pull_request.merge_commit_sha || github.sha — trigger-context values GitHub controls, not user-injectable via PR body.
  • No pull_request_target anywhere in .github/workflows/* (grepped all 11 files).
  • Workflow-file branch protection: CANT_TELL from the workflow content alone.

Non-blocker follow-ups

  • E2E test: assert origin tag SHA unchanged after the mismatch case.
  • Concurrent race branch (git push fails → delete local → verify) is not directly exercised by the E2E; the shared helper it calls IS covered.
  • R3 non-blocker still open: outer startsWith(head.ref, 'release/v') accepts release/vfoo; validator RELEASE_PR_RE catches it.

APPROVE.

— Via

@jrusso1020
jrusso1020 merged commit d619196 into main Aug 3, 2026
60 of 85 checks passed
@jrusso1020
jrusso1020 deleted the fix/release-merge-sha branch August 3, 2026 04:32
dahans-msft2 pushed a commit to dahans-msft2/hyperframes that referenced this pull request Aug 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants