Skip to content

ci: detect breaking-change commits in PRs#350

Open
MarioCadenas wants to merge 1 commit intomainfrom
ci/pr-breaking-change-check
Open

ci: detect breaking-change commits in PRs#350
MarioCadenas wants to merge 1 commit intomainfrom
ci/pr-breaking-change-check

Conversation

@MarioCadenas
Copy link
Copy Markdown
Collaborator

Summary

Adds .github/workflows/pr-breaking-change.yml, a workflow that scans every commit in a PR for Conventional Commits breaking-change markers and gates the merge.

  • Walks git rev-list base..head over the paths tracked by .release-it.json (packages/appkit, packages/appkit-ui, packages/shared) so docs/tooling-only commits don't trigger noise.
  • Matches both subject markers (feat!:, feat(scope)!:, etc. across all conventional types) and BREAKING CHANGE: / BREAKING-CHANGE: footers.
  • Upserts a sticky PR comment via actions/[email protected] (identified by an HTML-comment marker so re-runs update/remove the same comment).
  • Fails the job unless the PR carries an allow-breaking-change label, providing an explicit escape hatch for intentional major bumps.

Today, .release-it.json has bumpStrict: true, so a single BREAKING CHANGE: or feat! commit landing on main silently forces a major version on the next release. This check makes that visible at PR time.

Follow-ups (not in this PR)

  • Add PR Breaking Change Check / Detect Breaking Commits to the required status checks for the main branch protection rule. Without that, the comment will appear but the merge button is not blocked.
  • Create the allow-breaking-change label so reviewers can opt in when a major bump is intentional.
  • If main is configured for squash merge, the per-commit messages are discarded on merge and release-it only sees the squash commit (already covered by pr-title.yml); in that case this check is informational. With merge commit or rebase merge strategies, the check is load-bearing.

Test plan

  • On the PR for this branch, confirm the workflow runs and reports "no breaking commits found" (no comment posted, job green).
  • In a follow-up scratch PR, push a commit like feat(appkit)!: test breaking marker touching packages/appkit/** and verify:
    • Sticky comment appears listing the SHA + subject.
    • Job fails red.
    • Adding the allow-breaking-change label re-runs the workflow and turns it green; comment updates to acknowledge the intentional bump.
    • Removing the label flips it back to red.
  • Push a commit with feat!: touching docs/** only and confirm the check stays green (path filter working).
  • Amend the breaking commit away and verify the sticky comment is deleted on the next workflow run.

@MarioCadenas MarioCadenas requested a review from a team as a code owner May 6, 2026 11:15
@MarioCadenas MarioCadenas requested a review from pkosiec May 6, 2026 11:15
@MarioCadenas MarioCadenas force-pushed the ci/pr-breaking-change-check branch from 0de2b5f to fe49bf2 Compare May 6, 2026 11:20
Adds a workflow that scans every commit in a pull request for
Conventional Commits breaking-change markers (`type!:` or
`BREAKING CHANGE:` footer) limited to the packages tracked by
.release-it.json (appkit, appkit-ui, shared).

When a breaking commit is found, the job posts a sticky PR comment
explaining the major-version impact and fails the check, unless the
PR carries an `allow-breaking-change` label. This prevents accidental
major bumps from landing on main once the new check is added to
branch-protection required checks.

Signed-off-by: MarioCadenas <[email protected]>
@MarioCadenas MarioCadenas force-pushed the ci/pr-breaking-change-check branch from fe49bf2 to 85f7249 Compare May 6, 2026 11:24
Copy link
Copy Markdown
Member

@pkosiec pkosiec May 6, 2026

Choose a reason for hiding this comment

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

Maybe we can keep it within the same workflow (but as a separate job) as the PR title check?

Let's call it PR metadata verification or similar 🤔

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

consider using ts/js for that, to be consistent with the tools 🤔

But primarily, do we need that? We squash those commits so maybe we should enforce e.g. empty PR description?

Image

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.

we could also do that, that wouldn't cover the case for feat!: ... is the only thing

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

gotcha, so the script is useful - do you mind to unify it with other tools to use ts?

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