From 4ad8cd86714f67293522a80b83e63db66eb58a40 Mon Sep 17 00:00:00 2001 From: Mike Pitre <12040919+mikepitre@users.noreply.github.com> Date: Wed, 28 Oct 2026 11:51:38 -0400 Subject: [PATCH 1/2] ci: pass code-owner review when the author is a code owner GitHub never counts an author's own approval toward code-owner review, so a mobile engineer's change to an owned path waited on a second teammate. The Code owner approval check replaces that rule: it passes when no owned path changed, or the author or an approver has maintain or admin access. Co-Authored-By: Claude Opus 5.5 --- .github/scripts/code-owner-approval.mjs | 128 +++++++++++++++++ .github/scripts/code-owner-approval.test.mjs | 136 +++++++++++++++++++ .github/workflows/code-owner-approval.yml | 39 ++++++ CONTRIBUTING.md | 5 +- 4 files changed, 306 insertions(+), 2 deletions(-) create mode 100644 .github/scripts/code-owner-approval.mjs create mode 100644 .github/scripts/code-owner-approval.test.mjs create mode 100644 .github/workflows/code-owner-approval.yml diff --git a/.github/scripts/code-owner-approval.mjs b/.github/scripts/code-owner-approval.mjs new file mode 100644 index 000000000..28ceb385d --- /dev/null +++ b/.github/scripts/code-owner-approval.mjs @@ -0,0 +1,128 @@ +import fs from 'node:fs'; +import { fileURLToPath } from 'node:url'; + +const TRUSTED_ROLES = new Set(['admin', 'maintain']); + +const GLOB = { '**/': '(?:.*/)?', '/**': '(?:/.*)?', '**': '.*', '*': '[^/]*', '?': '[^/]' }; + +function patternRegex(pattern) { + const directory = pattern.endsWith('/'); + const trimmed = directory ? pattern.slice(0, -1) : pattern; + const anchored = trimmed.includes('/'); + const body = trimmed + .replace(/^\//, '') + .split(/(\*\*\/|\/\*\*|\*\*|\*|\?)/) + .map(part => GLOB[part] ?? part.replace(/[.+^${}()|[\]\\]/g, '\\$&')) + .join(''); + return new RegExp(`^${anchored ? '' : '(?:.*/)?'}${body}${directory ? '/.*' : '(?:/.*)?'}$`); +} + +function parseCodeowners(text) { + return text + .split('\n') + .map(line => line.replace(/(^|\s)#.*$/, '').trim()) + .filter(line => line !== '') + .map(line => { + const [pattern, ...owners] = line.split(/\s+/); + return { regex: patternRegex(pattern), owned: owners.length > 0 }; + }); +} + +// The last matching line wins, and a line with no owners leaves its files unowned, as on GitHub. +function ownedFiles(rules, paths) { + return paths.filter(path => rules.findLast(rule => rule.regex.test(path))?.owned === true); +} + +function approvers(reviews, author) { + const latest = new Map(); + for (const review of reviews) { + if (review.user && review.state !== 'COMMENTED' && review.state !== 'PENDING') { + latest.set(review.user.login, review.state); + } + } + return [...latest].filter(([login, state]) => state === 'APPROVED' && login !== author).map(([login]) => login); +} + +function client({ fetch, api, token, repo }) { + return async (path, { missing } = {}) => { + const response = await fetch(`${api}/repos/${repo}${path}`, { + headers: { accept: 'application/vnd.github+json', authorization: `Bearer ${token}`, 'x-github-api-version': '2022-11-28' }, + }); + if (missing !== undefined && response.status === 404) { + return missing; + } + if (!response.ok) { + throw new Error(`GET ${path} answered ${response.status}: ${(await response.text()).slice(0, 300)}`); + } + return response.json(); + }; +} + +async function paged(get, path) { + const items = []; + for (let page = 1; ; page += 1) { + const batch = await get(`${path}${path.includes('?') ? '&' : '?'}per_page=100&page=${page}`); + items.push(...batch); + if (batch.length < 100) { + return items; + } + } +} + +async function trusted(get, login) { + const { role_name: role } = await get(`/collaborators/${encodeURIComponent(login)}/permission`, { missing: { role_name: 'none' } }); + return TRUSTED_ROLES.has(role); +} + +async function codeOwnerApproval({ fetch, api, token, repo, number, author, baseRef }) { + const get = client({ fetch, api, token, repo }); + const codeowners = await get(`/contents/.github/CODEOWNERS?ref=${encodeURIComponent(baseRef)}`, { missing: null }); + const rules = codeowners === null ? [] : parseCodeowners(Buffer.from(codeowners.content, 'base64').toString('utf8')); + const files = await paged(get, `/pulls/${number}/files`); + const owned = ownedFiles(rules, [...new Set(files.flatMap(file => [file.filename, file.previous_filename].filter(Boolean)))]); + if (owned.length === 0) { + return { pass: true, message: 'No file under CODEOWNERS changed.' }; + } + if (await trusted(get, author)) { + return { pass: true, message: `${author} has maintain or admin access, so their change to CODEOWNERS files needs no other owner.` }; + } + for (const login of approvers(await paged(get, `/pulls/${number}/reviews`), author)) { + if (await trusted(get, login)) { + return { pass: true, message: `${login} approved, and has maintain or admin access.` }; + } + } + const shown = owned.slice(0, 10).join(', '); + return { + pass: false, + message: `This changes ${owned.length} CODEOWNERS file(s) (${shown}${owned.length > 10 ? ', ...' : ''}). It needs an approval from someone with maintain or admin access.`, + }; +} + +async function main(env, { fetch = globalThis.fetch, out = process.stdout } = {}) { + const missing = ['GITHUB_REPOSITORY', 'GITHUB_TOKEN', 'PR_NUMBER', 'PR_AUTHOR', 'BASE_REF'].filter(name => !env[name]); + if (missing.length > 0) { + throw new Error(`missing ${missing.join(', ')}`); + } + const { pass, message } = await codeOwnerApproval({ + fetch, + api: env.GITHUB_API_URL || 'https://api.github.com', + token: env.GITHUB_TOKEN, + repo: env.GITHUB_REPOSITORY, + number: env.PR_NUMBER, + author: env.PR_AUTHOR, + baseRef: env.BASE_REF, + }); + out.write(pass ? `${message}\n` : `::error title=Code owner approval::${message}\n`); + return pass; +} + +export { codeOwnerApproval, main, ownedFiles, parseCodeowners }; + +if (process.argv[1] !== undefined && fs.realpathSync(process.argv[1]) === fileURLToPath(import.meta.url)) { + try { + process.exitCode = (await main(process.env)) ? 0 : 1; + } catch (error) { + process.stderr.write(`${error.message}\n`); + process.exitCode = 1; + } +} diff --git a/.github/scripts/code-owner-approval.test.mjs b/.github/scripts/code-owner-approval.test.mjs new file mode 100644 index 000000000..ccb5f5335 --- /dev/null +++ b/.github/scripts/code-owner-approval.test.mjs @@ -0,0 +1,136 @@ +import assert from 'node:assert/strict'; +import { describe, it } from 'node:test'; + +import { main, ownedFiles, parseCodeowners } from './code-owner-approval.mjs'; + +const REPO = 'clerk/clerk-ios'; +const API = 'https://api.github.test'; + +const CODEOWNERS = `# High-risk paths +/.github/workflows/ @clerk/team-mobile +/Package.swift @clerk/team-mobile +/Sources/ClerkKit/Identity/ @clerk/team-mobile +/Sources/ClerkKit/Identity/Generated/ +*.pem @clerk/team-mobile +`; + +function fakeGitHub({ codeowners = CODEOWNERS, files, reviews = [], roles = {} }) { + const reply = body => ({ ok: true, status: 200, json: async () => body, text: async () => JSON.stringify(body) }); + const notFound = { ok: false, status: 404, json: async () => ({}), text: async () => 'Not Found' }; + return async (url, { headers }) => { + assert.equal(headers.authorization, 'Bearer test-token'); + const { pathname, searchParams } = new URL(url); + const path = pathname.replace(`/repos/${REPO}`, ''); + if (path === '/contents/.github/CODEOWNERS') { + assert.equal(searchParams.get('ref'), 'main'); + return codeowners === null ? notFound : reply({ content: Buffer.from(codeowners).toString('base64') }); + } + const page = Number(searchParams.get('page')); + const slice = items => items.slice((page - 1) * 100, page * 100); + if (path === '/pulls/7/files') { + return reply(slice(files)); + } + if (path === '/pulls/7/reviews') { + return reply(slice(reviews)); + } + const match = /^\/collaborators\/([^/]+)\/permission$/.exec(path); + if (match) { + return match[1] in roles ? reply({ role_name: roles[match[1]] }) : notFound; + } + throw new Error(`unexpected request ${path}`); + }; +} + +async function run(github, author = 'outside-dev') { + const lines = []; + const pass = await main( + { GITHUB_API_URL: API, GITHUB_REPOSITORY: REPO, GITHUB_TOKEN: 'test-token', PR_NUMBER: '7', PR_AUTHOR: author, BASE_REF: 'main' }, + { fetch: fakeGitHub(github), out: { write: line => lines.push(line) } }, + ); + return { pass, output: lines.join('') }; +} + +const review = (login, state) => ({ user: { login }, state }); + +describe('ownedFiles', () => { + const rules = parseCodeowners(CODEOWNERS); + + it('matches anchored directories, exact files, and unanchored globs', () => { + assert.deepEqual( + ownedFiles(rules, ['.github/workflows/pr-ci.yml', 'Package.swift', 'Tools/Nested/Package.swift', 'certs/dev.pem', 'Sources/ClerkKit/Clerk.swift', '.github/workflowsX/a.yml']), + ['.github/workflows/pr-ci.yml', 'Package.swift', 'certs/dev.pem'], + ); + }); + + it('lets a later line with no owners take files back out', () => { + assert.deepEqual(ownedFiles(rules, ['Sources/ClerkKit/Identity/Store.swift', 'Sources/ClerkKit/Identity/Generated/Ids.swift']), ['Sources/ClerkKit/Identity/Store.swift']); + }); +}); + +describe('code owner approval', () => { + it('passes when no file under CODEOWNERS changed', async () => { + const { pass, output } = await run({ files: [{ filename: 'Sources/ClerkKit/Clerk.swift' }] }); + assert.equal(pass, true); + assert.match(output, /No file under CODEOWNERS changed/); + }); + + it('passes when the author has maintain or admin access', async () => { + const { pass } = await run({ files: [{ filename: '.github/workflows/pr-ci.yml' }], roles: { mikepitre: 'admin' } }, 'mikepitre'); + assert.equal(pass, true); + }); + + it('fails for an author with write access and no trusted approval', async () => { + const { pass, output } = await run({ + files: [{ filename: '.github/workflows/pr-ci.yml' }], + reviews: [review('coderabbitai[bot]', 'APPROVED'), review('writer', 'APPROVED')], + roles: { 'outside-dev': 'write', writer: 'write' }, + }); + assert.equal(pass, false); + assert.match(output, /^::error title=Code owner approval::This changes 1 CODEOWNERS file\(s\) \(\.github\/workflows\/pr-ci\.yml\)/); + }); + + it('passes on an approval from someone with maintain access', async () => { + const { pass, output } = await run({ + files: [{ filename: 'Package.swift' }], + reviews: [review('swolfand', 'APPROVED')], + roles: { 'outside-dev': 'write', swolfand: 'maintain' }, + }); + assert.equal(pass, true); + assert.match(output, /swolfand approved/); + }); + + it('ignores an approval that was later dismissed or followed by a change request', async () => { + const { pass } = await run({ + files: [{ filename: 'Package.swift' }], + reviews: [review('swolfand', 'APPROVED'), review('swolfand', 'DISMISSED'), review('seanperez29', 'APPROVED'), review('seanperez29', 'COMMENTED'), review('seanperez29', 'CHANGES_REQUESTED')], + roles: { swolfand: 'maintain', seanperez29: 'admin' }, + }); + assert.equal(pass, false); + }); + + it('keeps an approval when a later review only comments', async () => { + const { pass } = await run({ + files: [{ filename: 'Package.swift' }], + reviews: [review('seanperez29', 'APPROVED'), review('seanperez29', 'COMMENTED')], + roles: { seanperez29: 'admin' }, + }); + assert.equal(pass, true); + }); + + it('counts the old path of a renamed file', async () => { + const { pass } = await run({ files: [{ filename: 'Old/Identity.swift', previous_filename: 'Sources/ClerkKit/Identity/Store.swift' }] }); + assert.equal(pass, false); + }); + + it('reads every page of changed files', async () => { + const files = Array.from({ length: 150 }, (_, i) => ({ filename: `Sources/File${i}.swift` })); + files.push({ filename: '.github/workflows/late.yml' }); + const { pass } = await run({ files }); + assert.equal(pass, false); + }); + + it('passes when the base branch has no CODEOWNERS', async () => { + const { pass } = await run({ codeowners: null, files: [{ filename: '.github/workflows/pr-ci.yml' }] }); + assert.equal(pass, true); + }); +}); diff --git a/.github/workflows/code-owner-approval.yml b/.github/workflows/code-owner-approval.yml new file mode 100644 index 000000000..82233a2fa --- /dev/null +++ b/.github/workflows/code-owner-approval.yml @@ -0,0 +1,39 @@ +name: Code owner approval + +on: + pull_request: + types: [opened, synchronize, reopened, ready_for_review] + pull_request_review: + types: [submitted, dismissed] + merge_group: + +permissions: + contents: read + pull-requests: read + +jobs: + code-owner-approval: + name: Code owner approval + if: github.event_name == 'merge_group' || github.event.pull_request.draft == false + runs-on: ubuntu-latest + timeout-minutes: 5 + steps: + - name: Pass in the merge queue + if: github.event_name == 'merge_group' + run: echo "Each pull request passed this check before it entered the queue." + + - name: Checkout the script + if: github.event_name != 'merge_group' + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + sparse-checkout: .github/scripts + persist-credentials: false + + - name: Check for a code owner's approval + if: github.event_name != 'merge_group' + env: + GITHUB_TOKEN: ${{ github.token }} + PR_NUMBER: ${{ github.event.pull_request.number }} + PR_AUTHOR: ${{ github.event.pull_request.user.login }} + BASE_REF: ${{ github.event.pull_request.base.ref }} + run: node .github/scripts/code-owner-approval.mjs diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index cdc7f587f..a1ba0367b 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -132,8 +132,9 @@ its pre-merge checks pass, and it rates the PR's merge risk moderate or lower. H critical PRs need a human. The pre-merge checks ask for the Testing, Risk, and How to revert sections of the PR template, and for device evidence when the PR changes `Sources/ClerkKit/`, `Sources/ClerkKitUI/`, or `Examples/E2EHost/`. Its approval counts -toward the one approval `main` requires, except on paths in `.github/CODEOWNERS`, which -need a team-mobile approval. Authors can't use `@coderabbitai approve` on their own PRs +toward the one approval `main` requires. A PR that changes a path in `.github/CODEOWNERS` +also needs the `Code owner approval` check. It passes when the author, or someone who +approved, has maintain or admin access to the repository, as the mobile team does. Authors can't use `@coderabbitai approve` on their own PRs or override a failing check. Admins can still merge their own work after dismissing a CodeRabbit "changes requested" review. From 0bfde65ee47355d89731b7ef969ad1844cbf997e Mon Sep 17 00:00:00 2001 From: Mike Pitre <12040919+mikepitre@users.noreply.github.com> Date: Wed, 28 Oct 2026 12:00:24 -0400 Subject: [PATCH 2/2] ci: match GitHub on wildcard paths, and fail on a truncated file list docs/* now owns only direct children, as on GitHub. The check fails when GitHub lists fewer files than the pull request changed, and reruns when the base branch changes. Co-Authored-By: Claude Opus 5.5 --- .github/scripts/code-owner-approval.mjs | 9 ++++++++- .github/scripts/code-owner-approval.test.mjs | 16 +++++++++++++++- .github/workflows/code-owner-approval.yml | 2 +- 3 files changed, 24 insertions(+), 3 deletions(-) diff --git a/.github/scripts/code-owner-approval.mjs b/.github/scripts/code-owner-approval.mjs index 28ceb385d..ae8b18e8b 100644 --- a/.github/scripts/code-owner-approval.mjs +++ b/.github/scripts/code-owner-approval.mjs @@ -14,7 +14,10 @@ function patternRegex(pattern) { .split(/(\*\*\/|\/\*\*|\*\*|\*|\?)/) .map(part => GLOB[part] ?? part.replace(/[.+^${}()|[\]\\]/g, '\\$&')) .join(''); - return new RegExp(`^${anchored ? '' : '(?:.*/)?'}${body}${directory ? '/.*' : '(?:/.*)?'}$`); + // On GitHub `docs/*` owns docs/a.md but not docs/b/c.md, so a wildcard last segment matches no descendants. + const last = trimmed.slice(trimmed.lastIndexOf('/') + 1); + const descendants = directory ? '/.*' : /[*?]/.test(last) && last !== '**' ? '' : '(?:/.*)?'; + return new RegExp(`^${anchored ? '' : '(?:.*/)?'}${body}${descendants}$`); } function parseCodeowners(text) { @@ -79,6 +82,10 @@ async function codeOwnerApproval({ fetch, api, token, repo, number, author, base const codeowners = await get(`/contents/.github/CODEOWNERS?ref=${encodeURIComponent(baseRef)}`, { missing: null }); const rules = codeowners === null ? [] : parseCodeowners(Buffer.from(codeowners.content, 'base64').toString('utf8')); const files = await paged(get, `/pulls/${number}/files`); + const { changed_files: changed } = await get(`/pulls/${number}`); + if (files.length < changed) { + return { pass: false, message: `GitHub listed ${files.length} of the ${changed} changed files, so this check can't tell which are under CODEOWNERS. Split the pull request.` }; + } const owned = ownedFiles(rules, [...new Set(files.flatMap(file => [file.filename, file.previous_filename].filter(Boolean)))]); if (owned.length === 0) { return { pass: true, message: 'No file under CODEOWNERS changed.' }; diff --git a/.github/scripts/code-owner-approval.test.mjs b/.github/scripts/code-owner-approval.test.mjs index ccb5f5335..d0597bbad 100644 --- a/.github/scripts/code-owner-approval.test.mjs +++ b/.github/scripts/code-owner-approval.test.mjs @@ -14,7 +14,7 @@ const CODEOWNERS = `# High-risk paths *.pem @clerk/team-mobile `; -function fakeGitHub({ codeowners = CODEOWNERS, files, reviews = [], roles = {} }) { +function fakeGitHub({ codeowners = CODEOWNERS, files, changed = files.length, reviews = [], roles = {} }) { const reply = body => ({ ok: true, status: 200, json: async () => body, text: async () => JSON.stringify(body) }); const notFound = { ok: false, status: 404, json: async () => ({}), text: async () => 'Not Found' }; return async (url, { headers }) => { @@ -25,6 +25,9 @@ function fakeGitHub({ codeowners = CODEOWNERS, files, reviews = [], roles = {} } assert.equal(searchParams.get('ref'), 'main'); return codeowners === null ? notFound : reply({ content: Buffer.from(codeowners).toString('base64') }); } + if (path === '/pulls/7') { + return reply({ changed_files: changed }); + } const page = Number(searchParams.get('page')); const slice = items => items.slice((page - 1) * 100, page * 100); if (path === '/pulls/7/files') { @@ -62,6 +65,11 @@ describe('ownedFiles', () => { ); }); + it('matches only direct children for a wildcard last segment, as on GitHub', () => { + const wildcard = parseCodeowners('* @clerk/team-mobile\ndocs/*\n'); + assert.deepEqual(ownedFiles(wildcard, ['docs/a.md', 'docs/private/b.md']), ['docs/private/b.md']); + }); + it('lets a later line with no owners take files back out', () => { assert.deepEqual(ownedFiles(rules, ['Sources/ClerkKit/Identity/Store.swift', 'Sources/ClerkKit/Identity/Generated/Ids.swift']), ['Sources/ClerkKit/Identity/Store.swift']); }); @@ -129,6 +137,12 @@ describe('code owner approval', () => { assert.equal(pass, false); }); + it('fails when GitHub lists fewer files than the pull request changed', async () => { + const { pass, output } = await run({ files: [{ filename: 'Sources/ClerkKit/Clerk.swift' }], changed: 3001 }); + assert.equal(pass, false); + assert.match(output, /listed 1 of the 3001 changed files/); + }); + it('passes when the base branch has no CODEOWNERS', async () => { const { pass } = await run({ codeowners: null, files: [{ filename: '.github/workflows/pr-ci.yml' }] }); assert.equal(pass, true); diff --git a/.github/workflows/code-owner-approval.yml b/.github/workflows/code-owner-approval.yml index 82233a2fa..cb947a7b3 100644 --- a/.github/workflows/code-owner-approval.yml +++ b/.github/workflows/code-owner-approval.yml @@ -2,7 +2,7 @@ name: Code owner approval on: pull_request: - types: [opened, synchronize, reopened, ready_for_review] + types: [opened, synchronize, reopened, ready_for_review, edited] pull_request_review: types: [submitted, dismissed] merge_group: