Skip to content

fix: prevent false major release signoff - #404

Merged
cameronapak merged 2 commits into
mainfrom
fix/major-signoff-race
Sep 15, 2026
Merged

cameronapak merged 2 commits into
mainfrom
fix/major-signoff-race

Conversation

@cameronapak

@cameronapak cameronapak commented Sep 15, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • isolate bot-authored issue_comment runs before workflow concurrency can cancel an active PR evaluation
  • keep unknown or untrusted previews fail-closed without calling them confirmed breaking changes
  • limit signoff instructions and breaking-change failures to successful trusted major previews
  • remove stale <!-- major-signoff-required --> comments after successful non-major evaluations
  • add focused structural regression coverage and an empty changeset

Verified race

On PR #393, the pull request run computed a non-major preview (introduced_major=false). While that preview run was active, changeset-bot[bot] updated a comment. Because workflow-level concurrency grouped every event by PR with cancel-in-progress: true, the bot's issue_comment run cancelled the valid preview before job-level conditions could skip it.

The gate then mapped the cancelled preview result to is_major=1. Its later comment, status, and failure steps ignored the existing blocked reason and incorrectly claimed that the PR contained a breaking change. The replacement bot-triggered run posted non-major success, but the stale blocking comment remained. PR #396 showed the same cancellation pattern.

This change gives bot comment runs unique concurrency keys and skips their jobs, so they cannot cancel human or pull-request evaluations and workflow-authored comment cleanup cannot create an event loop.

Security

The trust boundary is unchanged. The preview job may execute checked-out PR data but has read-only contents permission. The gate has comment and status write permissions, never checks out PR code, and keeps forks or unevaluable previews blocked as unknown impact.

Verification

  • pnpm test:ci-scripts - 21 passed
  • Actionlint 1.7.12 - passed for major-release-signoff.yml
  • Prettier check - passed
  • bash -n .github/scripts/major-release-signoff.test.sh - passed
  • git diff --check - passed
  • pnpm lint - package builds passed, then Oxlint crashed internally in oxc_allocator with exit 134 before lint completed

Greptile Summary

The PR fixes the major-release signoff workflow’s cancellation and classification behavior while addressing both previously reported stale-comment cleanup issues.

  • Isolates bot-authored comment runs from active PR evaluation concurrency.
  • Distinguishes unknown release impact from confirmed major releases.
  • Removes stale workflow-owned signoff instructions before publishing non-major or unknown-impact statuses.
  • Adds structural regression coverage and an intentional empty changeset.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename Overview
.github/workflows/major-release-signoff.yml Separates bot concurrency, fail-closed unknown outcomes, stale-comment cleanup, and trusted-major signoff handling without leaving either prior reported issue outstanding.
.github/scripts/major-release-signoff.test.sh Adds focused structural assertions covering concurrency isolation, decision classification, ownership filtering, and cleanup ordering.
package.json Adds the workflow regression script to the existing CI-script test command.
.changeset/forty-pens-throw.md Supplies the required intentional empty changeset for this CI-only change.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  E[Pull request or human comment] --> C[Resolve PR context]
  B[Bot-authored issue comment] --> S[Isolated run; jobs skipped]
  C --> P[Compute trusted release preview]
  P --> D{Decision}
  D -->|Unknown or invalid| U[Remove stale workflow-owned instructions]
  U --> F[Publish unknown-impact failure]
  D -->|Trusted non-major| N[Remove stale workflow-owned instructions]
  N --> OK[Publish success]
  D -->|Trusted major| A{Valid collaborator signoff?}
  A -->|Yes| MS[Publish success]
  A -->|No| BC[Upsert signoff instructions and fail gate]
Loading

Reviews (3): Last reviewed commit: "fix: clean stale signoff comments safely" | Re-trigger Greptile

@changeset-bot

changeset-bot Bot commented Sep 15, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: d03268c

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

@cameronapak cameronapak self-assigned this Sep 15, 2026
Comment thread .github/workflows/major-release-signoff.yml
Comment thread .github/workflows/major-release-signoff.yml
@Kyleasmth

Copy link
Copy Markdown
Collaborator

Fix is right, and proven on this PR: changeset-bot commented 1s after the run started and the run survived. The same 1 to 4s gap cancelled runs on #393, #403 and cp/hooks-split.

Splitting the concurrency key is better than cancel-in-progress: false, which would have let stale runs race to post statuses.

Three notes:

  1. A human comment still shares the evaluation key, so commenting mid-run still cancels it and reds the SHA until the comment run finishes. Should a cancelled run exit quietly rather than post failure?
  2. The test greps YAML strings, so it catches deletions but not refactors, and assert_not_contains 'is_major=1' will fire falsely the first time that line is legitimately needed. Worth naming it a canary rather than shipping it under test:ci-scripts.
  3. Empty changeset here, so the path where a real major blocks is not exercised by the PR or the tests.

Forks are still a dead end (is_major=0 plus blocked means the signoff search never runs). That is YPE-5849, flagging so it is not closed out here.

@cameronapak
cameronapak force-pushed the fix/major-signoff-race branch from e99e7dc to d03268c Compare September 15, 2026 17:25
@cameronapak
cameronapak merged commit 487b1d1 into main Sep 15, 2026
18 checks passed
@cameronapak
cameronapak deleted the fix/major-signoff-race branch September 15, 2026 17:29
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