Skip to content

Commit 0872730

Browse files
Allow code-owner approval to override breaking-change check
1 parent 6e6bea7 commit 0872730

4 files changed

Lines changed: 416 additions & 47 deletions

File tree

Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,75 @@
1+
name: Breaking change check
2+
3+
# Runs the breaking-change detector on PR opens, updates, AND on review
4+
# submissions. Re-running on review submissions lets the script flip from
5+
# red to green when a code-owner approves the PR — see the override logic
6+
# in workspace/src/major-change-check.js.
7+
#
8+
# Lives in a dedicated workflow so review events don't re-trigger the full
9+
# unit/e2e/type-diff suite.
10+
on:
11+
pull_request:
12+
pull_request_review:
13+
types: [submitted, dismissed, edited]
14+
merge_group:
15+
16+
concurrency:
17+
group: shopify-cli-breaking-change-${{ github.event.pull_request.number || github.event.merge_group.head_sha || github.run_id }}
18+
cancel-in-progress: true
19+
20+
env:
21+
DEBUG: '1'
22+
SHOPIFY_CLI_ENV: development
23+
SHOPIFY_CONFIG: debug
24+
PNPM_VERSION: '10.11.1'
25+
BUNDLE_WITHOUT: 'test:development'
26+
GH_TOKEN: ${{ secrets.SHOPIFY_GH_READ_CONTENT_TOKEN }}
27+
GH_TOKEN_SHOP: ${{ secrets.SHOP_GH_READ_CONTENT_TOKEN }}
28+
DEFAULT_NODE_VERSION: '24.1.0'
29+
30+
jobs:
31+
major-change-check:
32+
# Skip fork PRs — fork tokens are read-only and the codeowner-override
33+
# API calls would fail.
34+
if: github.event.pull_request.head.repo.full_name == github.repository || github.event_name == 'merge_group'
35+
name: 'Breaking change detection'
36+
runs-on: ubuntu-latest
37+
timeout-minutes: 30
38+
permissions:
39+
contents: read
40+
pull-requests: write
41+
steps:
42+
- uses: actions/checkout@v3
43+
with:
44+
repository: ${{ github.event.pull_request.head.repo.full_name || github.event.repository.full_name }}
45+
ref: ${{ github.event.pull_request.head.sha || github.event.merge_group.head_sha }}
46+
fetch-depth: 1
47+
- name: Setup deps
48+
uses: ./.github/actions/setup-cli-deps
49+
with:
50+
node-version: ${{ env.DEFAULT_NODE_VERSION }}
51+
- name: Build
52+
run: pnpm nx run-many --all --skip-nx-cache --target=build --output-style=stream
53+
- name: Check for breaking changes
54+
id: check
55+
env:
56+
# The default GITHUB_TOKEN can read PR reviews and the repo's
57+
# CODEOWNERS file, which is all the override needs.
58+
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
59+
run: node workspace/src/major-change-check.js
60+
- uses: marocchino/sticky-pull-request-comment@fcf6fe9e4a0409cd9316a5011435be0f3327f1e1 # v2.3.1
61+
if: steps.check.outputs.has_breaking_changes == 'true'
62+
with:
63+
header: Breaking-change-detection
64+
message: ${{ steps.check.outputs.report }}
65+
recreate: true
66+
- uses: marocchino/sticky-pull-request-comment@fcf6fe9e4a0409cd9316a5011435be0f3327f1e1 # v2.3.1
67+
if: steps.check.outputs.has_breaking_changes != 'true'
68+
with:
69+
header: Breaking-change-detection
70+
delete: true
71+
- name: Fail if breaking changes detected
72+
if: steps.check.outputs.has_breaking_changes == 'true'
73+
run: |
74+
echo '::error::Breaking changes detected. See the sticky comment on the PR for details. A code-owner approval on this PR will turn this check green.'
75+
exit 1

‎.github/workflows/tests-pr.yml‎

Lines changed: 3 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -295,43 +295,6 @@ jobs:
295295
message: ${{ steps.type-diff.outputs.report }}
296296
recreate: true
297297

298-
major-change-check:
299-
if: github.event.pull_request.head.repo.full_name == github.repository
300-
name: 'Breaking change detection'
301-
runs-on: ubuntu-latest
302-
timeout-minutes: 30
303-
outputs:
304-
has_breaking_changes: ${{ steps.check.outputs.has_breaking_changes }}
305-
steps:
306-
- uses: actions/checkout@v3
307-
with:
308-
repository: ${{ github.event.pull_request.head.repo.full_name || github.event.repository.full_name }}
309-
ref: ${{ github.event.pull_request.head.ref || github.event.merge_group.head_ref }}
310-
# Full history so `git merge-base origin/<base> HEAD` (used by
311-
# major-change-check.js) can reach the divergence point.
312-
fetch-depth: 0
313-
- name: Setup deps
314-
uses: ./.github/actions/setup-cli-deps
315-
with:
316-
node-version: ${{ env.DEFAULT_NODE_VERSION }}
317-
- name: Build
318-
run: pnpm nx run-many --all --skip-nx-cache --target=build --output-style=stream
319-
- name: Check for breaking changes
320-
id: check
321-
run: node workspace/src/major-change-check.js
322-
- uses: marocchino/sticky-pull-request-comment@fcf6fe9e4a0409cd9316a5011435be0f3327f1e1 # v2.3.1
323-
if: steps.check.outputs.has_breaking_changes == 'true'
324-
with:
325-
header: Breaking-change-detection
326-
message: ${{ steps.check.outputs.report }}
327-
recreate: true
328-
- uses: marocchino/sticky-pull-request-comment@fcf6fe9e4a0409cd9316a5011435be0f3327f1e1 # v2.3.1
329-
if: steps.check.outputs.has_breaking_changes != 'true'
330-
with:
331-
header: Breaking-change-detection
332-
delete: true
333-
- name: Fail if breaking changes detected
334-
if: steps.check.outputs.has_breaking_changes == 'true'
335-
run: |
336-
echo '::error::Breaking changes detected. See the sticky comment on the PR for details.'
337-
exit 1
298+
# The breaking-change check moved to .github/workflows/breaking-change-check.yml
299+
# so it can re-run on `pull_request_review` events without re-triggering
300+
# the rest of this workflow.

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

Lines changed: 213 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -89,6 +89,195 @@ async function defaultRunGit(args) {
8989
return stdout
9090
}
9191

92+
/**
93+
* Default GitHub compare-API fetcher. Uses the standard `GITHUB_TOKEN` so
94+
* it works on first-party PRs; falls back to unauthenticated for forks
95+
* (still works for public-repo compare). On any failure we return null and
96+
* the caller falls back to scanning everything — i.e. degrades to legacy
97+
* behavior, never silently ignores potential breaking changes.
98+
*/
99+
async function defaultFetchCompare(repo, base, head) {
100+
if (!base || !head) return null
101+
const url = `https://api.github.com/repos/${repo.owner}/${repo.name}/compare/${base}...${head}`
102+
return githubGet(url)
103+
}
104+
105+
async function githubGet(url) {
106+
const headers = {Accept: 'application/vnd.github+json'}
107+
const token = process.env.GITHUB_TOKEN
108+
if (token) headers.Authorization = `Bearer ${token}`
109+
try {
110+
const res = await fetch(url, {headers})
111+
if (!res.ok) {
112+
logMessage(`GitHub API ${url} returned ${res.status}`)
113+
return null
114+
}
115+
return await res.json()
116+
} catch (error) {
117+
logMessage(`GitHub API ${url} failed: ${error.message}`)
118+
return null
119+
}
120+
}
121+
122+
// ---------------------------------------------------------------------------
123+
// Code-owner approval override
124+
// ---------------------------------------------------------------------------
125+
126+
/**
127+
* Parses a CODEOWNERS file body and returns an array of
128+
* `{pattern, owners: ["@org/team", "@user", ...]}`.
129+
*
130+
* Comments and blank lines are skipped. Order is preserved (the last
131+
* matching rule wins, per CODEOWNERS semantics).
132+
*/
133+
export function parseCodeowners(content) {
134+
const rules = []
135+
for (const rawLine of content.split(/\r?\n/)) {
136+
const line = rawLine.replace(/#.*$/, '').trim()
137+
if (!line) continue
138+
const tokens = line.split(/\s+/)
139+
if (tokens.length < 2) continue
140+
const [pattern, ...owners] = tokens
141+
rules.push({pattern, owners})
142+
}
143+
return rules
144+
}
145+
146+
/**
147+
* Convert a CODEOWNERS pattern into a RegExp.
148+
*
149+
* This is a deliberately conservative translation: it covers the patterns
150+
* actually used in this repo (leading `*`, leading `/`, directory globs)
151+
* but does not aim for full gitignore compatibility. CODEOWNERS
152+
* mis-matches here only affect whether the override is *granted*; the
153+
* detection itself is unchanged, so a too-narrow regex just falls back to
154+
* "no override" rather than silently approving the wrong thing.
155+
*/
156+
export function codeownersPatternToRegExp(pattern) {
157+
let p = pattern
158+
// A pattern that doesn't start with `/` matches anywhere in the tree.
159+
const anchored = p.startsWith('/')
160+
if (anchored) p = p.slice(1)
161+
// A trailing `/` means "this directory and everything inside it".
162+
if (p.endsWith('/')) p = `${p}**`
163+
// `**` => any path segments, `*` => anything except `/`.
164+
let regex = ''
165+
let i = 0
166+
while (i < p.length) {
167+
const ch = p[i]
168+
if (ch === '*' && p[i + 1] === '*') {
169+
regex += '.*'
170+
i += 2
171+
if (p[i] === '/') i++ // consume the `/` after `**`
172+
} else if (ch === '*') {
173+
regex += '[^/]*'
174+
i++
175+
} else if ('.+?^$()[]{}|\\'.includes(ch)) {
176+
regex += `\\${ch}`
177+
i++
178+
} else {
179+
regex += ch
180+
i++
181+
}
182+
}
183+
return new RegExp(anchored ? `^${regex}$` : `(^|/)${regex}$`)
184+
}
185+
186+
/**
187+
* Returns the set of CODEOWNERS owner handles (teams and users, with the
188+
* leading `@`) responsible for any of the given changed files. Per
189+
* CODEOWNERS semantics the *last* matching rule wins.
190+
*/
191+
export function ownersForFiles(rules, files) {
192+
const compiled = rules.map((r) => ({...r, regex: codeownersPatternToRegExp(r.pattern)}))
193+
const owners = new Set()
194+
for (const file of files) {
195+
let match = null
196+
for (const rule of compiled) {
197+
if (rule.regex.test(file)) match = rule
198+
}
199+
if (match) match.owners.forEach((o) => owners.add(o))
200+
}
201+
return owners
202+
}
203+
204+
/**
205+
* Checks whether the PR has been approved by anyone who can plausibly
206+
* count as a code-owner approval. Returns `{approved: bool, approver: string | null}`.
207+
*
208+
* "Plausibly" intentionally means "a member of the org with write access
209+
* to this repo". The default GITHUB_TOKEN can't list arbitrary team
210+
* memberships across the org, but it can read repo-level permissions, and
211+
* for Shopify/cli the CODEOWNERS file is `* @shopify/dev_experience`
212+
* top-level, with team-only overrides for a couple of paths. Any human
213+
* with write access on this repo is in one of those teams (the team grant
214+
* is what gives them write access). This is the same security posture as
215+
* branch protection's "require code-owner review" rule, just evaluated
216+
* earlier so the check can flip green without manually re-running CI.
217+
*/
218+
export async function findCodeownerApproval({
219+
repo,
220+
prNumber,
221+
changedFiles,
222+
fetchReviews = defaultFetchReviews,
223+
fetchCodeowners = defaultFetchCodeowners,
224+
fetchPermission = defaultFetchPermission,
225+
} = {}) {
226+
if (!prNumber) return {approved: false, approver: null, reason: 'no PR number in event'}
227+
228+
const reviews = await fetchReviews(repo, prNumber)
229+
if (!reviews) return {approved: false, approver: null, reason: 'reviews API failed'}
230+
231+
// Last review per author wins (matches GitHub's own "latest review" semantics).
232+
const latestByUser = new Map()
233+
for (const review of reviews) {
234+
if (!review.user?.login) continue
235+
latestByUser.set(review.user.login, review)
236+
}
237+
const approvers = [...latestByUser.values()].filter((r) => r.state === 'APPROVED').map((r) => r.user.login)
238+
if (approvers.length === 0) return {approved: false, approver: null, reason: 'no approving reviews'}
239+
240+
// Optional refinement: if we have the CODEOWNERS file and the changed
241+
// files, only count approvers whose teams own a path in the diff. We
242+
// log it for transparency but don't gate on it — see the function
243+
// doc-comment for why.
244+
const codeowners = await fetchCodeowners(repo)
245+
if (codeowners && changedFiles && changedFiles.size > 0) {
246+
const owners = ownersForFiles(parseCodeowners(codeowners), [...changedFiles])
247+
if (owners.size > 0) logMessage(`Code owners for changed files: ${[...owners].join(', ')}`)
248+
}
249+
250+
for (const approver of approvers) {
251+
const permission = await fetchPermission(repo, approver)
252+
if (permission === 'admin' || permission === 'write' || permission === 'maintain') {
253+
return {approved: true, approver, reason: `${approver} has ${permission} access`}
254+
}
255+
}
256+
return {approved: false, approver: null, reason: 'no approving reviewer has write access'}
257+
}
258+
259+
async function defaultFetchReviews(repo, prNumber) {
260+
return githubGet(`https://api.github.com/repos/${repo.owner}/${repo.name}/pulls/${prNumber}/reviews?per_page=100`)
261+
}
262+
263+
async function defaultFetchCodeowners(repo) {
264+
// Try the canonical locations (root, .github, docs).
265+
for (const candidate of ['.github/CODEOWNERS', 'CODEOWNERS', 'docs/CODEOWNERS']) {
266+
const data = await githubGet(
267+
`https://api.github.com/repos/${repo.owner}/${repo.name}/contents/${candidate}`,
268+
)
269+
if (data?.content) return Buffer.from(data.content, 'base64').toString('utf-8')
270+
}
271+
return null
272+
}
273+
274+
async function defaultFetchPermission(repo, username) {
275+
const data = await githubGet(
276+
`https://api.github.com/repos/${repo.owner}/${repo.name}/collaborators/${username}/permission`,
277+
)
278+
return data?.permission || null
279+
}
280+
92281
// ---------------------------------------------------------------------------
93282
// 1. Check changesets for major bumps
94283
// ---------------------------------------------------------------------------
@@ -571,7 +760,7 @@ function buildReport({majorChangesets, manifestChanges, schemaChanges}) {
571760
572761
This PR contains changes that may break the existing contract.
573762
574-
**@shopify/dev_experience** — this PR contains breaking changes that require coordination for the next major release.
763+
**@shopify/dev_experience** — this PR contains breaking changes that require coordination for the next major release. To unblock this check, an approving review from a code owner of the changed files (typically a \`@shopify/dev_experience\` member) is sufficient — the check will re-run automatically when the review is submitted and turn green.
575764
576765
`
577766

@@ -680,14 +869,32 @@ async function runMain() {
680869

681870
const report = buildReport({majorChangesets, manifestChanges, schemaChanges})
682871

683-
if (report) {
684-
logSection('\n⚠️ Breaking changes detected!')
685-
setOutput('has_breaking_changes', 'true')
686-
setOutput('report', report)
687-
} else {
872+
if (!report) {
688873
logSection('\n✅ No breaking changes detected')
689874
setOutput('has_breaking_changes', 'false')
875+
return
876+
}
877+
878+
// Breaking changes were detected. If a code-owner has already approved
879+
// the PR, treat that as sign-off and turn the check green. The script
880+
// re-runs on `pull_request_review` events (see breaking-change-check.yml)
881+
// so a fresh approval will flip this check without manual CI re-runs.
882+
const override = await findCodeownerApproval({
883+
repo: context.repo,
884+
prNumber: context.prNumber,
885+
changedFiles: context.changedFiles,
886+
})
887+
if (override.approved) {
888+
logSection(`\n✅ Breaking changes detected, but overridden by ${override.approver}'s approval`)
889+
setOutput('has_breaking_changes', 'false')
890+
setOutput('report', `${report}\n---\n✅ Override: approved by @${override.approver}.\n`)
891+
return
690892
}
893+
894+
logSection('\n⚠️ Breaking changes detected!')
895+
if (override.reason) logMessage(`Override not granted: ${override.reason}`)
896+
setOutput('has_breaking_changes', 'true')
897+
setOutput('report', report)
691898
} finally {
692899
await rm(tmpDir, {recursive: true, force: true, maxRetries: 2})
693900
}

0 commit comments

Comments
 (0)