Skip to content

Allow code-owner approval to override breaking-change check#7469

Draft
alfonso-noriega wants to merge 1 commit intoremove-fake-approval-gatefrom
breaking-change-codeowner-override
Draft

Allow code-owner approval to override breaking-change check#7469
alfonso-noriega wants to merge 1 commit intoremove-fake-approval-gatefrom
breaking-change-codeowner-override

Conversation

@alfonso-noriega
Copy link
Copy Markdown
Contributor

@alfonso-noriega alfonso-noriega commented May 5, 2026

WHY are these changes introduced?

The breaking-change check fails hard (exit 1) whenever the script detects a removed Zod field, command, flag, or major changeset. There's currently no way to override it short of force-merging or admin-bypassing branch protection — and after PR #7468 in this stack, the previous (broken) "approval" path is gone too.

In practice, breaking changes are sometimes intentional: removing a deprecated field at a major-version boundary, restructuring a schema after a migration, etc. The team needs a way to acknowledge "yes, we know this is breaking, ship it" that's auditable and lightweight.

The simplest, most natural mechanism is the one already in place for everything else: a code-owner approval on the PR.

WHAT is this pull request doing?

Adds a code-owner-approval override to the breaking-change check. When a code owner of the changed files approves the PR, the check re-runs automatically (via a new pull_request_review trigger) and turns green.

.github/workflows/breaking-change-check.yml (new)

The breaking-change job moved into its own workflow file so we can trigger it on pull_request_review events without re-running the entire test suite (unit tests, e2e, type-diff, etc.) every time someone clicks Approve. Triggers:

  • pull_request — same as before
  • pull_request_review: types: [submitted, dismissed, edited] — re-runs when an approval is added or withdrawn so the check turns green/red automatically
  • merge_group — same as before

Concurrency is keyed per PR (or per merge-queue head) so the latest review event always wins.

.github/workflows/tests-pr.yml

  • Removes the now-relocated major-change-check job.

workspace/src/major-change-check.js

When breaking changes are detected, the script now:

  1. Fetches the PR's reviews via GET /repos/{owner}/{repo}/pulls/{N}/reviews.
  2. Computes the latest review per author (matches GitHub's "latest review wins" semantics — a later CHANGES_REQUESTED cancels an earlier APPROVED, exactly like dismiss_stale_reviews).
  3. For each approver, checks GET /repos/{owner}/{repo}/collaborators/{user}/permission. If the permission is admin, maintain, or write, treat the approval as a code-owner override.
  4. If override is granted: emit has_breaking_changes=false, log the approver, append an "Override: approved by @user" footer to the sticky comment, and exit cleanly.
  5. Otherwise: behave exactly as before — sticky comment + exit 1.

The script also reads CODEOWNERS (.github/CODEOWNERS) and logs which teams own the changed paths for transparency in the run output. The override decision uses the repo-permission check (not direct team-membership lookup) because the default GITHUB_TOKEN can't list arbitrary org-team memberships across Shopify, but it can read repo-level collaborator permissions. Anyone with write on this repo got that access via a code-owning team grant — same security posture as branch protection's "require code-owner review", evaluated earlier so the check can flip without manual CI re-runs.

The override does not auto-approve when the GitHub API fails — every degradation path returns approved: false. We'd rather over-flag than silently grant.

workspace/src/major-change-check.test.js

10 new tests:

  • parseCodeowners — comments and blank lines stripped, owners preserved
  • codeownersPatternToRegExp*, **, anchored, and unanchored patterns
  • ownersForFiles — last-matching-rule-wins (matches CODEOWNERS semantics, including theme overriding the catch-all)
  • findCodeownerApproval:
    • Approver with write access overrides (✓)
    • Latest review per author wins (a later CHANGES_REQUESTED cancels an earlier APPROVED, sticky-as-dismiss_stale_reviews)
    • No approving reviewer with write access → not approved
    • Missing PR number bails immediately
    • Reviews API failure bails (does NOT silently grant)

Stickiness behaviour

Per the requirement: matches dismiss_stale_reviews. Concretely:

  • If the same author submits a later CHANGES_REQUESTED or DISMISSED, the override is gone (we always look at latest review per author).
  • After a git push, the PR's existing approving reviews remain (since dismiss_stale_reviews: false on main's branch protection), so the override survives a force-push. If branch-protection ever flips dismiss_stale_reviews to true, GitHub will dismiss the reviews and our check will correctly go red again on the next event.

How to test your changes?

cd workspace
node --test src/major-change-check.test.js

All 21 tests pass locally (11 pre-existing + 10 new). End-to-end testing requires a PR that introduces a real or fake breaking change; smoke-test plan:

  1. Open a PR that removes a Zod field. The check should be red with the existing sticky-comment.
  2. A code owner clicks Approve.
  3. The workflow re-triggers (visible in Actions → Breaking change check). The check turns green and the sticky-comment gains the "Override: approved by @user" footer.
  4. The reviewer requests changes. The check turns red again.

Post-release steps

None — the workflow takes effect immediately on first run on main.

Checklist

  • I've considered possible cross-platform impacts (Mac, Linux, Windows) — pure Node + workflow YAML, no platform calls
  • I've considered possible documentation changes
  • I've considered analytics changes to measure impact — N/A (CI tooling)
  • The change is user-facing — N/A (internal CI tooling, no changeset needed)

Copy link
Copy Markdown
Contributor Author

alfonso-noriega commented May 5, 2026

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

This stack of pull requests is managed by Graphite. Learn more about stacking.

@github-actions github-actions Bot added the no-changelog This PR doesn't include a changeset entry. Is an internal only change not relevant to end users. label May 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-changelog This PR doesn't include a changeset entry. Is an internal only change not relevant to end users.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant