ci: require human signoff before a major release - #387
Conversation
🦋 Changeset detectedLatest commit: 5d1d291 The changes in this PR will be included in the next version bump. This PR includes changesets to release 0 packagesWhen 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 |
|
@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 The one thing that would change this diff: I gate on "this PR added a @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. Still needs someone with admin: the gate only actually blocks once |
Did you mean to tag @cameronapak? |
cameronapak
left a comment
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| const added = git( | ||
| 'diff', | ||
| '--name-only', | ||
| '--diff-filter=A', |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Done. All three pinned to full commit SHAs with the tag kept in a trailing comment.
|
suggestion: Prefer GitHub-native approvals over a custom comment-driven release gate. For Agents: simpler release controlA simpler approach would be to assign release maintainers as CODEOWNERS for That removes the 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. |
Yes, thank you. |
🚨 Breaking change detected — signoff required for
|
|
All three addressed in Privileged workflow executing PR codeSplit 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.
Workflow-level is now That alone was not enough, though. A same-repo author could still edit
Modified changesets bypassing the gateConfirmed exactly as described, against real commits. Editing a pending changeset from
So the workflow posted "No breaking changeset added; signoff not required" while Changesets computed a real major. Now
Mutable action tagsAll three pinned to full SHAs with the tag in a trailing comment. One more, not in the original reviewA signoff was not bound to the head it was given on. Approve 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 Known residuals
|
| SHORT_SHA=${HEAD_SHA:0:7} | ||
| SHA_RE="(^|[^0-9A-Za-z])${SHORT_SHA}[0-9A-Fa-f]*([^0-9A-Za-z]|$)" |
There was a problem hiding this 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.
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.
Closes YPE-2523 — pending 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/*.mdfrontmatter 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:So the gate is the same; only the detector changes.
scripts/preview-release.mjskeeps the Swift name and output contract, but askschangeset statusinstead 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
mainwould 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:
is_majorintroduced_majormajor(gates)Verified behaviour
Run against real commits on this branch, not reasoned about:
nextis_majorintroduced_majorpatchchangesetmajorchangesetThe last row is the one a straight port would get wrong.
Also confirmed: the three packages are a
fixedgroup in.changeset/config.json, so one major changeset bumps all three to the same3.0.0. That keeps Swift's single-version signoff format valid — worth knowing, because iffixedwere 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:
deleted, so a cachedsuccesscannot outlive its evidence.>quote markers and whitespace, so GitHub's "Quote reply" matches.Not done, needs someone with admin
The workflow only blocks if
major-release-signoffis added as a required status check onmain. 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.
@changesets/parsefor 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
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 endPrompt To Fix All With AI
Reviews (4): Last reviewed commit: "fix(ci): bind a signoff to the commit ha..." | Re-trigger Greptile