Skip to content

ci: require human signoff before a major release - #387

Open
Kyleasmth wants to merge 8 commits into
mainfrom
YPE-2523/major-release-signoff
Open

ci: require human signoff before a major release#387
Kyleasmth wants to merge 8 commits into
mainfrom
YPE-2523/major-release-signoff

Conversation

@Kyleasmth

@Kyleasmth Kyleasmth commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator

Closes YPE-2523pending a scope correction, see below.

Ports the Swift SDK's major-release signoff gate (YPE-2521) to this repo. A PR that declares a breaking change cannot merge until a write-access collaborator other than the author confirms it, by name, with the exact version and a 🚀.

The ticket's stated mechanism does not exist here

YPE-2523 says "semantic-release (current branch) detects a breaking change". This repo has no semantic-release: releases run on Changesets, and the bump level is declared in .changeset/*.md frontmatter rather than inferred from commit footers.

That is not a new discovery. docs/release-hardening-decisions.md (Decision 1, jhampton 2026-07-01) already records it:

In the Swift SDK, conventional commits drive changelog + version math. This repo does not work that way … Version bump = the level declared in each changeset … No semantic-release / conventional-changelog tooling.

So the gate is the same; only the detector changes. scripts/preview-release.mjs keeps the Swift name and output contract, but asks changeset status instead of @semantic-release/commit-analyzer.

One deliberate difference from Swift

Swift gates on "this release is a major". Here that would be wrong. Changesets accumulates changesets until the Version Packages PR ships them, so a major pending on main would make every unrelated PR report major and get blocked until the release went out.

The script therefore reports two separate things, and only the second gates:

Field Means
is_major the pending release would be a major (informational)
introduced_major this PR added a changeset declaring major (gates)

Verified behaviour

Run against real commits on this branch, not reasoned about:

Scenario Gate next is_major introduced_major
CI-only PR (empty changeset) pass false false
Adds a patch changeset pass 2.12.2 false false
Adds a major changeset BLOCKS 3.0.0 true true
Unrelated patch, major already pending pass 3.0.0 true false

The last row is the one a straight port would get wrong.

Also confirmed: the three packages are a fixed group in .changeset/config.json, so one major changeset bumps all three to the same 3.0.0. That keeps Swift's single-version signoff format valid — worth knowing, because if fixed were ever removed the signoff comment would become ambiguous about which version is approved. The script fails loudly rather than picking one if it sees more than one version.

Carried over from Swift unchanged

The parts that are release-tool-agnostic and were the hard-won bits there:

  • Rejects bot comments and the PR author, so an author with write access cannot self-clear the gate.
  • Verifies write/maintain/admin permission, with a per-login cache.
  • Re-runs on comment deleted, so a cached success cannot outlive its evidence.
  • Normalises > quote markers and whitespace, so GitHub's "Quote reply" matches.
  • Upserts one blocking comment rather than posting a new one per push.
  • Fails loudly if a major is detected but no version could be computed, rather than letting a degenerate regex pass the gate.

Not done, needs someone with admin

The workflow only blocks if major-release-signoff is added as a required status check on main. I cannot read branch protection (404 without admin). This is what actually made the Swift gate bite, so it should not be skipped.

Scope

Asking on the ticket for the description to be corrected before this merges, since it currently specifies a mechanism this repo does not have.

Greptile Summary

This PR adds a Changesets-aware major-release approval gate and separates untrusted preview computation from status-writing operations. The new commit-binding check remains vulnerable to deliberately colliding abbreviated SHAs.

  • Adds a workflow that computes whether the PR introduces a major changeset and requires collaborator signoff.
  • Adds a release-preview script that compares changeset declarations between the merge base and PR head.
  • Pins privileged workflow actions and isolates write permissions in the gate job.
  • Adds @changesets/parse for authoritative changeset frontmatter parsing.

Confidence Score: 3/5

The PR is not yet safe to merge because a deliberately colliding abbreviated commit SHA can reuse stale approval and bypass the required human signoff.

The gate binds signoff comments to only a seven-character SHA prefix, so a different head constructed with the same prefix inherits the prior approval and receives a successful required status.

Files Needing Attention: .github/workflows/major-release-signoff.yml

Important Files Changed

Filename Overview
.github/workflows/major-release-signoff.yml Adds the permission-separated signoff workflow and fixes earlier trust issues, but its abbreviated-SHA matcher can accept approval for a different commit with a colliding prefix.
scripts/preview-release.mjs Adds Changesets-based release previewing and correctly detects major declarations introduced through added, modified, or renamed changeset files.
package.json Adds the pinned Changesets parser dependency used by the preview script.
pnpm-lock.yaml Records the new parser dependency and associated peer-resolution updates.
.changeset/odd-towns-unite.md Adds the required intentional empty changeset for this CI-only change.

Sequence Diagram

sequenceDiagram
  participant PR as PR Head
  participant Preview as Preview Job
  participant Gate as Gate Job
  participant Comments as PR Comments
  participant Status as Commit Status API
  PR->>Preview: Checkout current head
  Preview->>Preview: Detect introduced major changeset
  Preview-->>Gate: is_major, next version
  Gate->>Comments: Search for version, rocket, and SHA prefix
  Comments-->>Gate: Existing signoff comment
  alt Seven-character prefix matches
    Gate->>Status: Post success on current head
  else No qualifying signoff
    Gate->>Status: Post failure on current head
  end
Loading

Fix all with Greploop Fix All in Claude Code Fix All in Cursor Fix All in Codex

Prompt To Fix All With AI
### Issue 1
.github/workflows/major-release-signoff.yml:275-276
**Abbreviated SHA reuses stale signoff**

When an author pushes a different commit deliberately constructed with the same seven-character SHA prefix as an approved head, `SHA_RE` accepts the old signoff and posts a successful status for the unreviewed commit. Require and compare the complete `HEAD_SHA` so every push invalidates the previous approval.

**How this was verified:** The matcher truncates the current head to seven characters, accepts arbitrary hexadecimal suffixes, and directly controls the success-status branch.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Reviews (4): Last reviewed commit: "fix(ci): bind a signoff to the commit ha..." | Re-trigger Greptile

Greptile also left 1 inline comment on this PR.

@changeset-bot

changeset-bot Bot commented Sep 8, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 5d1d291

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 0 packages

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@Kyleasmth
Kyleasmth marked this pull request as ready for review September 9, 2026 18:09
Comment thread .github/workflows/major-release-signoff.yml Outdated
Comment thread scripts/preview-release.mjs Outdated
Comment thread .github/workflows/major-release-signoff.yml Outdated
@Kyleasmth

Kyleasmth commented Sep 9, 2026

Copy link
Copy Markdown
Collaborator Author

@jhampton @camrun91 — out of draft and green. Two different asks, so splitting them:

@jhampton — the open question is on YPE-2523, not in this diff. The ticket specifies semantic-release detecting the breaking change; this repo has none, so the detector here is changeset status instead. That is not a new finding — your Decision 1 on YPE-2486 already records that this repo does not work the Swift way. The code is correct either way; the ticket wording is what needs your call.

The one thing that would change this diff: I gate on "this PR added a major changeset", not "the pending release is a major". Gating on the latter would block every unrelated PR while a major sits pending on main. If you want the Version Packages PR gated as a second line of defence, that is a different detector and I would rather add it deliberately than assume it.

@camrun91 — normal review welcome, particularly on whether the workflow fits how this repo runs CI.

Worth knowing, since it is the interesting bit: the gate failed on its own PR when I first pushed it, and the reason was hidden because the script swallowed stderr. changeset status resolves the configured baseBranch as a local ref, and a PR checkout only has origin/main — it worked locally purely because my clone had a real main branch. Fixed by fetching main:refs/heads/main in the workflow, and the script now surfaces Changesets' own message rather than exiting silently. It is passing its own gate now, which is a real test rather than a synthetic one: this PR carries an empty changeset, so introduced_major is false and the check goes green on its own merits.

Still needs someone with admin: the gate only actually blocks once major-release-signoff is a required status check on main. I get a 404 reading branch protection.

@camrun91

camrun91 commented Sep 9, 2026

Copy link
Copy Markdown
Collaborator

@jhampton @camrun91 — out of draft and green. Two different asks, so splitting them:

@jhampton — the open question is on YPE-2523, not in this diff. The ticket specifies semantic-release detecting the breaking change; this repo has none, so the detector here is changeset status instead. That is not a new finding — your Decision 1 on YPE-2486 already records that this repo does not work the Swift way. The code is correct either way; the ticket wording is what needs your call.

The one thing that would change this diff: I gate on "this PR added a major changeset", not "the pending release is a major". Gating on the latter would block every unrelated PR while a major sits pending on main. If you want the Version Packages PR gated as a second line of defence, that is a different detector and I would rather add it deliberately than assume it.

@camrun91 — normal review welcome, particularly on whether the workflow fits how this repo runs CI.

Worth knowing, since it is the interesting bit: the gate failed on its own PR when I first pushed it, and the reason was hidden because the script swallowed stderr. changeset status resolves the configured baseBranch as a local ref, and a PR checkout only has origin/main — it worked locally purely because my clone had a real main branch. Fixed by fetching main:refs/heads/main in the workflow, and the script now surfaces Changesets' own message rather than exiting silently. It is passing its own gate now, which is a real test rather than a synthetic one: this PR carries an empty changeset, so introduced_major is false and the check goes green on its own merits.

Still needs someone with admin: the gate only actually blocks once major-release-signoff is a required status check on main. I get a 404 reading branch protection.

Did you mean to tag @cameronapak?

@cameronapak cameronapak 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.

Review

Jira: YPE-2523. Spec is the PR body.

Summary

Standards: 2 must-fix findings. Spec: 2 must-fix findings. Worst: the comment-triggered path executes PR-controlled install code with write-capable credentials.

For Agents: review basis
  • CI line: major-release-signoff
  • Greptile/bot: prior findings were independently verified.
  • Event: issue_comment
  • HEAD: fb94946456e826775b6fca1d75aeb2f50d94d829

By Code Reviewer bot, sent on behalf of Cam.

cache: 'pnpm'

- name: Install dependencies
run: pnpm install --frozen-lockfile

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.

issue: Do not install PR-controlled code in the write-capable signoff job.

For Agents: untrusted PR execution

The issue_comment path checks out the current PR head and runs pnpm install while this job has pull-requests: write and statuses: write. A fork can change dependency metadata or lifecycle scripts, then trigger this path with a comment; that code runs before the signoff decision and can forge the success status or alter PR comments. Keep PR analysis in a read-only job, and run status/comment mutation only from trusted workflow code without installing the PR head.

By Code Reviewer bot, sent on behalf of Cam.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Done. The job that runs PR code now holds no write scopes, and the one that posts statuses never checks that code out. Install is also --ignore-scripts --ignore-pnpmfile, and the detector plus its lockfile are restored from main so a branch cannot rewrite what gates it.

Comment thread scripts/preview-release.mjs Outdated
const added = git(
'diff',
'--name-only',
'--diff-filter=A',

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.

issue: Include modified changesets when detecting a major bump.

For Agents: changeset diff filter

--diff-filter=A only examines newly added .changeset/*.md files. A PR can modify an existing unconsumed patch/minor changeset to declare major; changeset status will report a major release, but introduced_major remains false and the workflow posts a successful “signoff not required” status. Inspect added and modified changesets, using the final PR contents.

By Code Reviewer bot, sent on behalf of Cam.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Done, and it was real: I reproduced it on commits. Editing a pending changeset from patch to major gave introduced_major=false while is_major=true, so the gate posted "signoff not required" for a genuine major. Now --diff-filter=AMR with rename following, compared per package.

echo "pr_author=$PR_AUTHOR" >> "$GITHUB_OUTPUT"

- name: Checkout PR head
uses: actions/checkout@v5

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.

issue: Pin the actions used by this privileged workflow to full commit SHAs.

For Agents: mutable action references

actions/checkout@v5, pnpm/action-setup@v4, and actions/setup-node@v6 are mutable tag references inside a workflow that can write statuses and PR comments. Pin each to a reviewed full commit SHA, consistent with the pinned privileged actions already used in .github/workflows/release.yml.

By Code Reviewer bot, sent on behalf of Cam.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Done. All three pinned to full commit SHAs with the tag kept in a trailing comment.

@cameronapak

Copy link
Copy Markdown
Collaborator

suggestion: Prefer GitHub-native approvals over a custom comment-driven release gate.

For Agents: simpler release control

A simpler approach would be to assign release maintainers as CODEOWNERS for .changeset/*, require code-owner approval on main, and dismiss stale approvals when new commits are pushed. This makes every release-bearing PR—including a major changeset—require explicit review from someone responsible for release impact, while automatically covering modified changesets.

That removes the issue_comment workflow, exact phrase/version/emoji parsing, PR-head installation, custom status management, and privileged mutable action execution. If approval must happen only at publication time, a protected GitHub Environment with required reviewers on the npm publish job is the next-simplest native option.

The tradeoff is that CODEOWNERS requires approval for every release-bearing changeset rather than only majors, but it directly protects the stated goal with substantially less custom security-sensitive code.

By Code Reviewer bot, sent on behalf of Cam.

@Kyleasmth

Kyleasmth commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator Author

@jhampton @camrun91 — out of draft and green. Two different asks, so splitting them:
@jhampton — the open question is on YPE-2523, not in this diff. The ticket specifies semantic-release detecting the breaking change; this repo has none, so the detector here is changeset status instead. That is not a new finding — your Decision 1 on YPE-2486 already records that this repo does not work the Swift way. The code is correct either way; the ticket wording is what needs your call.
The one thing that would change this diff: I gate on "this PR added a major changeset", not "the pending release is a major". Gating on the latter would block every unrelated PR while a major sits pending on main. If you want the Version Packages PR gated as a second line of defence, that is a different detector and I would rather add it deliberately than assume it.
@camrun91 — normal review welcome, particularly on whether the workflow fits how this repo runs CI.
Worth knowing, since it is the interesting bit: the gate failed on its own PR when I first pushed it, and the reason was hidden because the script swallowed stderr. changeset status resolves the configured baseBranch as a local ref, and a PR checkout only has origin/main — it worked locally purely because my clone had a real main branch. Fixed by fetching main:refs/heads/main in the workflow, and the script now surfaces Changesets' own message rather than exiting silently. It is passing its own gate now, which is a real test rather than a synthetic one: this PR carries an empty changeset, so introduced_major is false and the check goes green on its own merits.
Still needs someone with admin: the gate only actually blocks once major-release-signoff is a required status check on main. I get a 404 reading branch protection.

Did you mean to tag @cameronapak?

Yes, thank you.

@Kyleasmth
Kyleasmth removed the request for review from camrun91 September 10, 2026 18:56
@github-actions

Copy link
Copy Markdown
Contributor

🚨 Breaking change detected — signoff required for v

This PR adds a changeset declaring a major bump, which would ship all three packages as v (they are a fixed group in .changeset/config.json, so they version together). Merging is blocked until a repo collaborator with write access comments on this PR with all three (3) of the following:

  1. The verbatim acknowledgment phrase below,
  2. The precise next version (v or ``), and
  3. A 🚀 (:rocket:) emoji.

I confirm that this is an intentional breaking change, and I have read the release procedures. I understand and have documented its impact upon release.

Copy-paste-ready reply:

I confirm that this is an intentional breaking change, and I have read the release procedures. I understand and have documented its impact upon release.

v 🚀

The check re-runs automatically when a qualifying comment is posted or edited.

@Kyleasmth

Copy link
Copy Markdown
Collaborator Author

All three addressed in 5ba79fc and c5fac06. Thanks both — chasing these turned up more than the original three, so worth writing down what changed.

Privileged workflow executing PR code

Split into three jobs. The one that runs PR code holds no write scopes; the one that can write statuses and comments never checks that code out.

Job Permissions Runs PR code
context pull-requests: read no
preview contents: read yes
gate pull-requests: write, statuses: write no

Workflow-level is now permissions: {}. Fork PRs skip preview entirely and the gate fails closed rather than trusting a preview computed by untrusted code.

That alone was not enough, though. A same-repo author could still edit scripts/preview-release.mjs to return introduced_major: false and the trusted job would post green. So the preview job now restores the detector from main before anything installs: scripts/preview-release.mjs, package.json, pnpm-lock.yaml, pnpm-workspace.yaml, .changeset/config.json, and .npmrc / .pnpmfile.cjs restored-or-deleted. The PR supplies changeset data only.

--ignore-scripts also was not sufficient on its own: pnpm hooks are a separate mechanism, so a PR-authored .pnpmfile.cjs could rewrite the detector after restore. Install is now --ignore-scripts --ignore-pnpmfile.

Modified changesets bypassing the gate

Confirmed exactly as described, against real commits. Editing a pending changeset from patch to major gave:

introduced_major is_major
before false true
after true true

So the workflow posted "No breaking changeset added; signoff not required" while Changesets computed a real major.

Now --diff-filter=AMR with rename following, and the base comparison is per package so an existing major on one package cannot mask a different one being raised. Three further bypasses fell out while testing, each proved with real commits:

  • Renaming a changeset while raising it to major reported as R and was skipped entirely.
  • Filenames git C-quotes (a literal tab, say) failed the changeset name test and dropped out of the scan. Diff parsing is now -z NUL-delimited.
  • YAML forms the line regex missed: {"pkg": major}, anchors/aliases, block scalars, !!str, quoted levels. The hand-written parser is gone; @changesets/parse is a real dependency now, pinned to 0.4.1, the exact version CLI 2.29.7 resolves, so it cannot drift from what changeset status itself computes.

Mutable action tags

All three pinned to full SHAs with the tag in a trailing comment.

One more, not in the original review

A signoff was not bound to the head it was given on. Approve v3.0.0 on commit A, push commit B with additional breaking changes, version is still 3.0.0, and the old comment satisfied the gate. This one can happen by accident rather than by malice. Comments older than the commit under review no longer count.

Also fixed: on a preview failure the job used to abort before reaching the step that posts a red status, so a previously-green status could outlive the signoff that earned it. The decision step now records why it cannot evaluate and the red-status and fail steps run under always().

Known residuals

  • If the context lookup itself fails, gate is skipped and a stale green can remain on the SHA. Cannot be fixed by the job condition alone, since an issue_comment payload carries no head SHA. The mitigation would be setting the status to pending in the trusted phase first; happy to add it if you want it.
  • A same-repo collaborator can still edit the workflow file itself. That is inherent to the gate living in the repo it guards.

Comment thread .github/workflows/major-release-signoff.yml Outdated
Comment on lines +275 to +276
SHORT_SHA=${HEAD_SHA:0:7}
SHA_RE="(^|[^0-9A-Za-z])${SHORT_SHA}[0-9A-Fa-f]*([^0-9A-Za-z]|$)"

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.

P1 security Abbreviated SHA reuses stale signoff

When an author pushes a different commit deliberately constructed with the same seven-character SHA prefix as an approved head, SHA_RE accepts the old signoff and posts a successful status for the unreviewed commit. Require and compare the complete HEAD_SHA so every push invalidates the previous approval.

How this was verified: The matcher truncates the current head to seven characters, accepts arbitrary hexadecimal suffixes, and directly controls the success-status branch.

Prompt To Fix With AI
This is a comment left during a code review.
Path: .github/workflows/major-release-signoff.yml
Line: 275-276

Comment:
**Abbreviated SHA reuses stale signoff**

When an author pushes a different commit deliberately constructed with the same seven-character SHA prefix as an approved head, `SHA_RE` accepts the old signoff and posts a successful status for the unreviewed commit. Require and compare the complete `HEAD_SHA` so every push invalidates the previous approval.

**How this was verified:** The matcher truncates the current head to seven characters, accepts arbitrary hexadecimal suffixes, and directly controls the success-status branch.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Fix in Claude Code Fix in Cursor Fix in Codex

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.

3 participants