From 6de8eacbc64c563cfbc4178216d29eb3dd96ecf7 Mon Sep 17 00:00:00 2001 From: Charlie Kolb Date: Tue, 1 Sep 2026 10:09:39 +0000 Subject: [PATCH] ci: Replace CODEOWNERS with required-review enforcement in OWNERS (#37120) Co-authored-by: Claude Fable 5 --- .github/CLAUDE.md | 2 +- .github/CODEOWNERS | 4 + .github/WORKFLOWS.md | 63 +++- .github/scripts/github-helpers.mjs | 67 ++++ .github/scripts/owners.mjs | 160 -------- .github/scripts/owners.test.mjs | 173 --------- .../assign-reviewers.mjs} | 26 +- .../assign-reviewers.test.mjs} | 14 +- .github/scripts/owners/owners.mjs | 346 ++++++++++++++++++ .github/scripts/owners/owners.test.mjs | 336 +++++++++++++++++ .github/scripts/owners/required-reviews.mjs | 213 +++++++++++ .../scripts/owners/required-reviews.test.mjs | 281 ++++++++++++++ .../review-recommendations.mjs} | 74 +++- .../review-recommendations.test.mjs} | 79 +++- .github/scripts/package.json | 2 +- .github/scripts/quality/check-pr-size.mjs | 18 +- .../scripts/quality/check-pr-size.test.mjs | 9 + .../workflows/ci-codeowners-validation.yml | 23 -- .../workflows/ci-owners-assign-reviewers.yml | 4 +- .../workflows/ci-owners-required-reviews.yml | 165 +++++++++ .../ci-owners-review-recommendations.yml | 10 +- .github/workflows/ci-owners-validation.yml | 39 ++ .github/workflows/ci-pull-requests.yml | 1 + .github/OWNERS => OWNERS | 31 +- 24 files changed, 1714 insertions(+), 426 deletions(-) delete mode 100644 .github/scripts/owners.mjs delete mode 100644 .github/scripts/owners.test.mjs rename .github/scripts/{owners-assign-reviewers.mjs => owners/assign-reviewers.mjs} (83%) rename .github/scripts/{owners-assign-reviewers.test.mjs => owners/assign-reviewers.test.mjs} (91%) create mode 100644 .github/scripts/owners/owners.mjs create mode 100644 .github/scripts/owners/owners.test.mjs create mode 100644 .github/scripts/owners/required-reviews.mjs create mode 100644 .github/scripts/owners/required-reviews.test.mjs rename .github/scripts/{owners-review-recommendations.mjs => owners/review-recommendations.mjs} (78%) rename .github/scripts/{owners-review-recommendations.test.mjs => owners/review-recommendations.test.mjs} (82%) delete mode 100644 .github/workflows/ci-codeowners-validation.yml create mode 100644 .github/workflows/ci-owners-required-reviews.yml create mode 100644 .github/workflows/ci-owners-validation.yml rename .github/OWNERS => OWNERS (91%) diff --git a/.github/CLAUDE.md b/.github/CLAUDE.md index ac305248160..7294c9ce975 100644 --- a/.github/CLAUDE.md +++ b/.github/CLAUDE.md @@ -13,7 +13,7 @@ This folder contains n8n's GitHub Actions infrastructure. | `workflows/` | GitHub Actions workflows | | `actions/` | Reusable composite actions | | `scripts/` | Release & Docker automation | -| `CODEOWNERS` | Team review ownership | +| `scripts/owners/` | Team review ownership scripts; the OWNERS file is at the repo root (`required` entries gate merges) | ### Workflow Naming diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index fc709a835da..213f992ce03 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -1,4 +1,8 @@ +# Temporary: kept side by side with the top-level OWNERS file while the +# "Required Reviews" enforcement is on trial. Do not add entries here; +# add them to OWNERS. See "Transition from CODEOWNERS" in WORKFLOWS.md. packages/@n8n/db/src/migrations/ @n8n-io/migrations-review +/OWNERS @n8n-io/qa-dx .github/workflows @n8n-io/qa-dx .github/scripts @n8n-io/qa-dx .github/actions @n8n-io/qa-dx diff --git a/.github/WORKFLOWS.md b/.github/WORKFLOWS.md index da9a81b3406..2ec1a4d5c25 100644 --- a/.github/WORKFLOWS.md +++ b/.github/WORKFLOWS.md @@ -10,7 +10,7 @@ Complete reference for n8n's `.github/` folder. .github/ ├── WORKFLOWS.md # This document ├── CI-TELEMETRY.md # Telemetry & metrics guide -├── CODEOWNERS # Team ownership for PR reviews +├── CODEOWNERS # Temporary, side by side with OWNERS during the trial ├── pull_request_template.md # PR description template ├── pull_request_title_conventions.md # Title format rules (Angular) ├── actionlint.yml # Workflow linter config @@ -21,6 +21,7 @@ Complete reference for n8n's `.github/` folder. │ ├── config.yml # Routes to community/security │ └── 01-bug.yml # Structured bug report form ├── scripts/ # Automation scripts +│ ├── owners/ # Owners scripts (the OWNERS file lives at the repo root) │ ├── bump-versions.mjs # Calculate next version │ ├── update-changelog.mjs # Generate CHANGELOG │ ├── trim-fe-packageJson.js # Strip frontend devDeps @@ -603,13 +604,63 @@ See **[CI-TELEMETRY.md](CI-TELEMETRY.md)** for: --- -## CODEOWNERS +## OWNERS -Team ownership mappings in `CODEOWNERS`: +Team ownership lives in the top-level `OWNERS` file (this replaces the +GitHub-native `CODEOWNERS` file; see the transition note below). The scripts +that consume it live in `.github/scripts/owners/`. Line format: -| Path Pattern | Team | -|--------------------------------------------------------------|----------------------------| -| `packages/@n8n/db/src/migrations/` | @n8n-io/migrations-review | +``` + <@org/team> [required] +``` + +Patterns are a catch-all (`*`), a directory prefix (`packages/x/`), or an +exact file path. Matching is last-match-wins, so specific rules must come +after general rules. + +The format is strict, enforced by `node .github/scripts/owners/owners.mjs --check`: + +- Tokens on a line come in a fixed order: pattern, one team, then options + such as `required`. +- Directory patterns end with `/` and must be existing directories; all other + patterns must be existing files. Duplicate patterns are rejected. + +Team existence is not checked by `--check` (it needs an org read token, which +fork PRs do not have); a separate workflow covers it (DEVP-891). + +The file drives four workflows: + +| Workflow | Purpose | +|-----------------------------------|--------------------------------------------------------------------------| +| `ci-owners-validation.yml` | Validates OWNERS (syntax, dead paths) via `owners.mjs --check` | +| `ci-owners-review-recommendations.yml` | Advisory PR comment: reviewer teams, line stats, required reviews | +| `ci-owners-assign-reviewers.yml` | Opt-in reviewer auto-assignment (label-triggered) | +| `ci-owners-required-reviews.yml` | Enforces `required` entries via the "Required Reviews" commit status | + +### Required reviews + +An entry with the `required` option makes team approval mandatory: when a PR +changes a file whose winning entry carries `required`, a member of each listed +team must approve the PR. `ci-owners-required-reviews.yml` evaluates this on +PR changes and review events, and reports a commit status +named **Required Reviews** on the head SHA. The ruleset for `master` must list +that status as a required check for the block to take effect. Merge-queue runs +report success on the queue head without re-evaluating: a PR cannot enter the +queue unless the status is green on its head, and the queue does not change +approvals. + +The workflow reads OWNERS and its scripts from the base branch only, so a PR +cannot lift its own review requirement. + +### Transition from CODEOWNERS + +During a trial period, `.github/CODEOWNERS` stays in place next to OWNERS: +GitHub's native code-owner enforcement keeps gating merges while the +"Required Reviews" status runs side by side. The two must agree — CODEOWNERS +holds exactly the `required` entries of OWNERS (plus the OWNERS file itself) +and must not gain new entries; new ownership goes into OWNERS. After the +trial, delete `.github/CODEOWNERS`, remove "Require review from Code Owners" +from the master ruleset, and delete this section (tracked in DEVP-887). --- diff --git a/.github/scripts/github-helpers.mjs b/.github/scripts/github-helpers.mjs index dcead8050a7..4fd07e34709 100644 --- a/.github/scripts/github-helpers.mjs +++ b/.github/scripts/github-helpers.mjs @@ -424,6 +424,73 @@ export async function getPrFiles(pullRequestNumber) { }); } +/** + * Returns all reviews submitted on a PR, in submission order. + * + * @param { number } pullRequestNumber + * @returns { Promise> } + * */ +export async function getPrReviews(pullRequestNumber) { + const { octokit, owner, repo } = initGithub(); + + return await octokit.paginate(octokit.rest.pulls.listReviews, { + owner, + repo, + pull_number: pullRequestNumber, + per_page: 100, + }); +} + +/** + * Test whether a user is an active member of an org team. + * + * Team slugs are the part after the org, e.g. `catalysts` for + * `@n8n-io/catalysts`. Requires a token with org members read access + * (the plain GITHUB_TOKEN cannot read team membership). Returns false + * for pending invitations and for teams that do not exist. + * + * @param { string } teamSlug + * @param { string } username + * @returns { Promise } + * */ +export async function isTeamMember(teamSlug, username) { + const { octokit, owner } = initGithub(); + + try { + const { data } = await octokit.rest.teams.getMembershipForUserInOrg({ + org: owner, + team_slug: teamSlug, + username, + }); + return data.state === 'active'; + } catch (ex) { + if (ex?.status === 404) return false; + throw ex; + } +} + +/** + * Create (or overwrite) a commit status on the given SHA. A ruleset can list + * the status context as a required check to gate merges on it. + * + * @param { string } sha + * @param {{ state: 'success' | 'failure' | 'pending' | 'error', context: string, description: string, targetUrl?: string }} status + */ +export async function setCommitStatus(sha, { state, context, description, targetUrl }) { + const { octokit, owner, repo } = initGithub(); + + await octokit.rest.repos.createCommitStatus({ + owner, + repo, + sha, + state, + context, + // The API rejects descriptions longer than 140 characters. + description: description.length > 140 ? `${description.slice(0, 139)}…` : description, + target_url: targetUrl, + }); +} + /** * Post a PR comment, or update the existing one if a previous run already * left one identified by the provided bot marker. diff --git a/.github/scripts/owners.mjs b/.github/scripts/owners.mjs deleted file mode 100644 index 457da7c3e1f..00000000000 --- a/.github/scripts/owners.mjs +++ /dev/null @@ -1,160 +0,0 @@ -import { readFileSync } from "node:fs"; -import { join } from "node:path"; - -/** - * @typedef Owner - * @property { string } filepath - * @property { string } team - * */ - -/** - * @typedef { Map } Ownerships - * */ - -/** - * @typedef Allocation - * @property { string } team - * @property { number } fileCount - * */ - -// Resolve relative to this file so the path works regardless of cwd -// (workflow runs from repo root; `npm test` runs from .github/scripts). -export const OWNERS_FILE = join(import.meta.dirname, "..", "OWNERS"); - -/** - * Parse OWNERS file content into Owner records. Lines without an `@n8n-io/*` - * team are skipped. - * - * @param { string } content - * @returns { Owner[] } - * */ -export function parseOwnersContent(content) { - return content.split("\n") - .filter(line => line.includes("@n8n-io")) - .map(line => ({ - filepath: line.match(/^\S+/)?.at(0), - team: line.match(/@n8n-io\/.*/)?.at(0) - })) - .filter(/** @returns { owner is Owner } */ (owner) => - owner.filepath !== undefined && owner.team !== undefined - ); -} - -/** - * Read and parse the .github/OWNERS file. - * - * @param { string } [path] Optional override; defaults to OWNERS_FILE. - * @returns { Owner[] } - * */ -export function parseOwnersFile(path = OWNERS_FILE) { - const content = readFileSync(path, "utf8"); - return parseOwnersContent(content); -} - -/** - * Test whether `file` is matched by a CODEOWNERS-style pattern. - * - * The OWNERS file uses three pattern shapes, all handled here: - * "*" catch-all (matches any file) - * "packages/x/" directory pattern (matches every file under packages/x/ recursively) - * "path/to/f.ts" exact path - * - * If richer globs are ever introduced to OWNERS (e.g. `*.ts`, `**\/foo`), - * extend this helper rather than reaching for a dependency. - * - * @param { string } file - * @param { string } pattern - * @returns { boolean } - * */ -export function matchesPattern(file, pattern) { - if (pattern === "*") return true; - if (pattern.endsWith("/")) return file.startsWith(pattern); - return file === pattern; -} - -/** - * Map each changed file to the team that owns it, applying CODEOWNERS - * last-match-wins semantics. Files that match no rule are omitted. - * - * @param { Set } files - * @param { Owner[] } owners - * @returns { Ownerships } team -> files it owns in this changeset - * */ -export function assignOwnership(files, owners) { - /** @type { Ownerships } */ - const teamToFiles = new Map(); - - for (const file of files) { - // Walk rules in reverse so the *last* matching rule wins. - for (let i = owners.length - 1; i >= 0; i--) { - if (matchesPattern(file, owners[i].filepath)) { - const team = owners[i].team; - const bucket = teamToFiles.get(team); - - if (bucket) { - bucket.push(file); - } else { - teamToFiles.set(team, [file]); - } - break; - } - } - } - - return teamToFiles; -} - -/** - * @param { Ownerships } ownerships - * @returns { Allocation[] } - * */ -export function ownershipsToAllocations(ownerships) { - return Array.from(ownerships).map(([team, files]) => ({ - team, - fileCount: files.length, - })); -} - -/** - * Read a newline-delimited list of changed file paths from disk. - * Empty/whitespace-only lines are skipped. - * - * @param { string } path - * @returns { Set } - * */ -export function readChangedFilesList(path) { - return new Set( - readFileSync(path, "utf8") - .split("\n") - .map(line => line.trim()) - .filter(Boolean) - ); -} - -// CLI: `node owners.mjs ` -// Reads the given file (one changed path per line), runs ownership -// allocation against .github/OWNERS. Prints out an object with ownerships for files -// and the ownership counts per team as JSON on stdout. -if (import.meta.url === `file://${process.argv[1]}`) { - const path = process.argv[2]; - if (!path) { - console.error("Usage: node owners.mjs "); - console.error(" : path to a file containing one changed path per line"); - process.exit(1); - } - - const files = readChangedFilesList(path); - const ownerships = assignOwnership(files, parseOwnersFile()); - const totalFiles = files.size; - - const allocations = Array.from(ownerships) - .map(([team, ownedFiles]) => ({ - team, - fileCount: ownedFiles.length, - share: totalFiles === 0 ? 0 : Math.round((ownedFiles.length / totalFiles) * 100), - files: ownedFiles, - })) - .sort((a, b) => b.fileCount - a.fileCount); - - console.log(JSON.stringify({ totalFiles, allocations }, null, 4)); -} diff --git a/.github/scripts/owners.test.mjs b/.github/scripts/owners.test.mjs deleted file mode 100644 index 39078e4deae..00000000000 --- a/.github/scripts/owners.test.mjs +++ /dev/null @@ -1,173 +0,0 @@ -import { describe, it } from 'node:test'; -import assert from 'node:assert/strict'; -import { - assignOwnership, - ownershipsToAllocations, - parseOwnersContent, - parseOwnersFile, -} from './owners.mjs'; - -/** - * Run these tests by running - * - * node --test ./.github/scripts/owners.test.mjs - * */ - -describe('parseOwnersContent', () => { - it('parses well-formed OWNERS lines into Owner records', () => { - const content = [ - '# header comment', - '', - '* @n8n-io/catalysts', - 'packages/cli/ @n8n-io/cli-team', - 'packages/foo/bar.ts @n8n-io/some-team', - ].join('\n'); - - assert.deepEqual(parseOwnersContent(content), [ - { filepath: '*', team: '@n8n-io/catalysts' }, - { filepath: 'packages/cli/', team: '@n8n-io/cli-team' }, - { filepath: 'packages/foo/bar.ts', team: '@n8n-io/some-team' }, - ]); - }); - - it('skips lines without an @n8n-io team', () => { - const content = [ - '# comment', - '* @other-org/team', - 'pkg/ @n8n-io/keepers', - '', - ].join('\n'); - - assert.deepEqual(parseOwnersContent(content), [ - { filepath: 'pkg/', team: '@n8n-io/keepers' }, - ]); - }); - - it('returns an empty array when no lines reference @n8n-io', () => { - assert.deepEqual(parseOwnersContent('# nothing here\n* @someone-else'), []); - }); -}); - -describe('parseOwnersFile', () => { - it('reads the real OWNERS file into well-formed Owner records', () => { - const owners = parseOwnersFile(); - - assert.ok(owners.length > 0, 'OWNERS file should not be empty'); - assert.ok( - owners.every(o => o.team && o.filepath), - 'every parsed entry should have both team and filepath', - ); - assert.ok( - owners.every(o => o.team?.startsWith('@n8n-io/')), - 'every parsed team should belong to the @n8n-io org', - ); - }); -}); - -describe('assignOwnership', () => { - it('assigns every file to the catch-all team when only `*` is defined', () => { - const files = new Set(['a.ts', 'packages/cli/src/index.ts', 'docs/readme.md']); - const owners = [{ filepath: '*', team: '@n8n-io/catalysts' }]; - - const result = assignOwnership(files, owners); - - assert.deepEqual( - result.get('@n8n-io/catalysts')?.sort(), - [...files].sort(), - ); - assert.equal(result.size, 1); - }); - - it('applies last-match-wins: a later specific rule overrides the catch-all', () => { - const files = new Set([ - 'README.md', - 'packages/cli/src/index.ts', - 'packages/cli/src/lib/foo.ts', - ]); - const owners = [ - { filepath: '*', team: '@n8n-io/catalysts' }, - { filepath: 'packages/cli/', team: '@n8n-io/cli-team' }, - ]; - - const result = assignOwnership(files, owners); - - assert.deepEqual(result.get('@n8n-io/catalysts'), ['README.md']); - assert.deepEqual( - result.get('@n8n-io/cli-team')?.sort(), - ['packages/cli/src/index.ts', 'packages/cli/src/lib/foo.ts'].sort(), - ); - }); - - it('matches a directory pattern recursively', () => { - const files = new Set([ - 'packages/cli/src/deep/nested/file.ts', - 'packages/cli/package.json', - ]); - const owners = [{ filepath: 'packages/cli/', team: '@n8n-io/cli-team' }]; - - const result = assignOwnership(files, owners); - - assert.deepEqual( - result.get('@n8n-io/cli-team')?.sort(), - [...files].sort(), - ); - }); - - it('matches an exact file pattern only against that file', () => { - const files = new Set([ - 'packages/cli/src/controllers/ai.controller.ts', - 'packages/cli/src/controllers/other.controller.ts', - ]); - const owners = [ - { - filepath: 'packages/cli/src/controllers/ai.controller.ts', - team: '@n8n-io/ai-team', - }, - ]; - - const result = assignOwnership(files, owners); - - assert.deepEqual(result.get('@n8n-io/ai-team'), [ - 'packages/cli/src/controllers/ai.controller.ts', - ]); - // the other controller matched no rule, so it must be omitted entirely - assert.equal(result.size, 1); - }); - - it('omits files that match no rule (no catch-all present)', () => { - const files = new Set(['unowned/file.ts', 'packages/cli/src/x.ts']); - const owners = [{ filepath: 'packages/cli/', team: '@n8n-io/cli-team' }]; - - const result = assignOwnership(files, owners); - - assert.deepEqual(result.get('@n8n-io/cli-team'), ['packages/cli/src/x.ts']); - assert.equal(result.size, 1); - }); - - it('returns an empty Map when there are no changed files', () => { - const owners = [{ filepath: '*', team: '@n8n-io/catalysts' }]; - const result = assignOwnership(new Set(), owners); - - assert.equal(result.size, 0); - }); -}); - -describe('ownershipsToAllocations', () => { - it('converts a Map of team -> files into Allocation[] with fileCount', () => { - const ownerships = new Map([ - ['@n8n-io/cli-team', ['a.ts', 'b.ts', 'c.ts']], - ['@n8n-io/catalysts', ['README.md']], - ]); - - const result = ownershipsToAllocations(ownerships); - - assert.deepEqual(result, [ - { team: '@n8n-io/cli-team', fileCount: 3 }, - { team: '@n8n-io/catalysts', fileCount: 1 }, - ]); - }); - - it('returns an empty array for an empty Map', () => { - assert.deepEqual(ownershipsToAllocations(new Map()), []); - }); -}); diff --git a/.github/scripts/owners-assign-reviewers.mjs b/.github/scripts/owners/assign-reviewers.mjs similarity index 83% rename from .github/scripts/owners-assign-reviewers.mjs rename to .github/scripts/owners/assign-reviewers.mjs index c79d5cf3780..7bd9b079501 100644 --- a/.github/scripts/owners-assign-reviewers.mjs +++ b/.github/scripts/owners/assign-reviewers.mjs @@ -2,7 +2,7 @@ * Opt-in reviewer auto-assignment. * * When a PR carries the `Auto-assign reviewers` label, request review from - * every team that owns a changed file (per .github/OWNERS). This is strictly + * every team that owns a changed file (per OWNERS). This is strictly * opt-in: without the label the script is a no-op, so it never adds friction * to PRs that don't want it. * @@ -11,7 +11,7 @@ * that marker is skipped. This stops later `synchronize` events from * re-requesting (and re-notifying) the same teams. * - * Complements `owners-review-recommendations.mjs`, which only *suggests* + * Complements `review-recommendations.mjs`, which only *suggests* * reviewer teams in a comment — this one actually requests them. */ @@ -22,23 +22,19 @@ import { readPrLabels, removeLabel, requestTeamReviewers, -} from './github-helpers.mjs'; -import { assignOwnership, ownershipsToAllocations, parseOwnersFile } from './owners.mjs'; +} from '../github-helpers.mjs'; +import { + assignOwnership, + ownershipsToAllocations, + parseOwnersFile, + teamHandleToSlug, +} from './owners.mjs'; + +export { teamHandleToSlug }; export const AUTO_ASSIGN_LABEL = 'Auto-assign reviewers'; export const ASSIGNED_LABEL = 'Reviewers auto-assigned'; -/** - * Convert an OWNERS team handle (`@n8n-io/catalysts`) into the GitHub team slug - * (`catalysts`) expected by the requestReviewers API. - * - * @param { string } team - * @returns { string } - */ -export function teamHandleToSlug(team) { - return team.replace(/^@[^/]+\//, ''); -} - /** * @param { string[] } labels * @returns { boolean } diff --git a/.github/scripts/owners-assign-reviewers.test.mjs b/.github/scripts/owners/assign-reviewers.test.mjs similarity index 91% rename from .github/scripts/owners-assign-reviewers.test.mjs rename to .github/scripts/owners/assign-reviewers.test.mjs index 2114c96bb35..a494f420479 100644 --- a/.github/scripts/owners-assign-reviewers.test.mjs +++ b/.github/scripts/owners/assign-reviewers.test.mjs @@ -4,7 +4,7 @@ import assert from 'node:assert/strict'; /** * Run these tests by running * - * node --test --experimental-test-module-mocks ./.github/scripts/owners-assign-reviewers.test.mjs + * node --test --experimental-test-module-mocks ./.github/scripts/owners/assign-reviewers.test.mjs * */ /** @type {() => string[]} */ @@ -18,7 +18,7 @@ let addLabelImpl = async () => {}; /** @type {(pullRequestNumber: number, label: string) => Promise} */ let removeLabelImpl = async () => {}; -mock.module('./github-helpers.mjs', { +mock.module('../github-helpers.mjs', { namedExports: { ensureEnvVar: () => {}, // no-op in tests readPrLabels: () => readPrLabelsImpl(), @@ -30,9 +30,9 @@ mock.module('./github-helpers.mjs', { }, }); -/** @type {() => Array<{ filepath: string, team: string }>} */ +/** @type {() => Array} */ let parseOwnersFileImpl = () => []; -/** @type {(files: Set, owners: Array<{ filepath: string, team: string }>) => Map} */ +/** @type {(files: Set, entries: Array) => Map} */ let assignOwnershipImpl = () => new Map(); /** @type {(ownerships: Map) => Array<{ team: string, fileCount: number }>} */ let ownershipsToAllocationsImpl = () => []; @@ -40,8 +40,10 @@ let ownershipsToAllocationsImpl = () => []; mock.module('./owners.mjs', { namedExports: { parseOwnersFile: () => parseOwnersFileImpl(), - assignOwnership: (files, owners) => assignOwnershipImpl(files, owners), + assignOwnership: (files, entries) => assignOwnershipImpl(files, entries), ownershipsToAllocations: (ownerships) => ownershipsToAllocationsImpl(ownerships), + // Mirrors the real implementation; the mock replaces the whole module. + teamHandleToSlug: (team) => team.replace(/^@[^/]+\//, ''), }, }); @@ -52,7 +54,7 @@ const { hasAutoAssignLabel, resolveOwnerTeamSlugs, run, -} = await import('./owners-assign-reviewers.mjs'); +} = await import('./assign-reviewers.mjs'); describe('teamHandleToSlug', () => { it('strips the org prefix from an OWNERS team handle', () => { diff --git a/.github/scripts/owners/owners.mjs b/.github/scripts/owners/owners.mjs new file mode 100644 index 00000000000..219391da390 --- /dev/null +++ b/.github/scripts/owners/owners.mjs @@ -0,0 +1,346 @@ +import { readFileSync, statSync } from "node:fs"; +import { join } from "node:path"; + +/** + * @typedef OwnersEntry + * @property { string } pattern + * @property { string } team + * @property { boolean } required A member of the team must approve before merge. + * @property { number } line 1-based line number in the OWNERS file. + * */ + +/** + * @typedef { Map } Ownerships + * */ + +/** + * @typedef Allocation + * @property { string } team + * @property { number } fileCount + * */ + +const REPO_ROOT = join(import.meta.dirname, "..", "..", ".."); + +// Resolve relative to this file so the path works regardless of cwd +// (workflow runs from repo root; `npm test` runs from .github/scripts). +export const OWNERS_FILE = join(REPO_ROOT, "OWNERS"); + +// GitHub team handle, e.g. `@n8n-io/catalysts`. +const TEAM_TOKEN = /^@[\w.-]+\/[\w.-]+$/; + +/** + * Line grammar: ` <@org/team> [required]` + * New option keywords go here. + */ +const OPTION_TOKENS = new Set(["required"]); + +/** + * @param { string } line + * @returns { string } + */ +function stripComment(line) { + const commentStart = line.match(/(^|\s)#/); + return commentStart ? line.slice(0, commentStart.index) : line; +} + +/** + * Parse OWNERS file content into structured entries. + * + * Throws on lines that do not follow the grammar, so a malformed OWNERS + * file fails loudly in every consumer instead of being silently ignored. + * + * @param { string } content + * @returns { OwnersEntry[] } + * */ +export function parseOwnersContent(content) { + /** @type { OwnersEntry[] } */ + const entries = []; + + content.split("\n").forEach((rawLine, index) => { + const lineNumber = index + 1; + const line = stripComment(rawLine).trim(); + if (!line) return; + + const [pattern, ...tokens] = line.split(/\s+/); + /** @type { string | null } */ + let team = null; + let required = false; + let sawOption = false; + + for (const token of tokens) { + if (TEAM_TOKEN.test(token)) { + if (sawOption) { + throw new Error(`OWNERS line ${lineNumber}: team "${token}" must come before options`); + } + if (team) { + throw new Error(`OWNERS line ${lineNumber}: only one team per pattern is supported`); + } + team = token; + } else if (token === "required") { + required = true; + sawOption = true; + } else if (OPTION_TOKENS.has(token)) { + throw new Error(`OWNERS line ${lineNumber}: option "${token}" is not handled`); + } else { + throw new Error(`OWNERS line ${lineNumber}: unknown token "${token}"`); + } + } + + if (!team) { + throw new Error(`OWNERS line ${lineNumber}: no team for pattern "${pattern}"`); + } + + entries.push({ pattern, team, required, line: lineNumber }); + }); + + return entries; +} + +/** + * Read and parse the OWNERS file. + * + * @param { string } [path] Optional override; defaults to OWNERS_FILE. + * @returns { OwnersEntry[] } + * */ +export function parseOwnersFile(path = OWNERS_FILE) { + const content = readFileSync(path, "utf8"); + return parseOwnersContent(content); +} + +/** + * @param { string } pattern + * @returns { 'file' | 'directory' | null } + * */ +function getPathKind(pattern) { + const stats = statSync(join(REPO_ROOT, pattern), { throwIfNoEntry: false }); + if (!stats) return null; + return stats.isDirectory() ? "directory" : "file"; +} + +/** + * Validate parsed OWNERS entries beyond the line grammar: + * - no duplicate patterns + * - directory patterns (trailing `/`) point at existing directories, + * all other patterns (except `*`) at existing files + * + * @param { OwnersEntry[] } entries + * @param { (pattern: string) => 'file' | 'directory' | null } [pathKind] + * @returns { string[] } validation errors, empty when the file is valid + * */ +export function validateOwners(entries, pathKind = getPathKind) { + const errors = []; + /** @type { Map } */ + const seenPatterns = new Map(); + + for (const entry of entries) { + const firstLine = seenPatterns.get(entry.pattern); + if (firstLine !== undefined) { + errors.push(`duplicate pattern "${entry.pattern}" (lines ${firstLine} and ${entry.line})`); + } else { + seenPatterns.set(entry.pattern, entry.line); + } + + if (entry.pattern !== "*") { + const kind = pathKind(entry.pattern); + const expectedKind = entry.pattern.endsWith("/") ? "directory" : "file"; + + if (kind === null) { + errors.push(`pattern "${entry.pattern}" (line ${entry.line}) does not exist in the repository`); + } else if (kind !== expectedKind) { + errors.push( + kind === "directory" + ? `pattern "${entry.pattern}" (line ${entry.line}) is a directory; add a trailing "/"` + : `pattern "${entry.pattern}" (line ${entry.line}) is a file; remove the trailing "/"`, + ); + } + } + } + + return errors; +} + +/** + * Convert an OWNERS team handle (`@n8n-io/catalysts`) into the GitHub team slug + * (`catalysts`) expected by the teams API. + * + * @param { string } team + * @returns { string } + */ +export function teamHandleToSlug(team) { + return team.replace(/^@[^/]+\//, ""); +} + +/** + * Test whether `file` is matched by a CODEOWNERS-style pattern. + * + * The OWNERS file uses three pattern shapes, all handled here: + * "*" catch-all (matches any file) + * "packages/x/" directory pattern (matches every file under packages/x/ recursively) + * "path/to/f.ts" exact path + * + * If richer globs are ever introduced to OWNERS (e.g. `*.ts`, `**\/foo`), + * extend this helper rather than reaching for a dependency. + * + * @param { string } file + * @param { string } pattern + * @returns { boolean } + * */ +export function matchesPattern(file, pattern) { + if (pattern === "*") return true; + if (pattern.endsWith("/")) return file.startsWith(pattern); + return file === pattern; +} + +/** + * Find the entry that owns `file`, applying CODEOWNERS last-match-wins + * semantics. Returns undefined when no entry matches. + * + * @param { string } file + * @param { OwnersEntry[] } entries + * @returns { OwnersEntry | undefined } + * */ +export function findOwningEntry(file, entries) { + for (let i = entries.length - 1; i >= 0; i--) { + if (matchesPattern(file, entries[i].pattern)) return entries[i]; + } + return undefined; +} + +/** + * Map each changed file to the team that owns it, applying CODEOWNERS + * last-match-wins semantics. Files that match no rule are omitted. + * + * @param { Set } files + * @param { OwnersEntry[] } entries + * @returns { Ownerships } team -> files it owns in this changeset + * */ +export function assignOwnership(files, entries) { + /** @type { Ownerships } */ + const teamToFiles = new Map(); + + for (const file of files) { + const entry = findOwningEntry(file, entries); + if (!entry) continue; + + const bucket = teamToFiles.get(entry.team); + + if (bucket) { + bucket.push(file); + } else { + teamToFiles.set(entry.team, [file]); + } + } + + return teamToFiles; +} + +/** + * Determine which teams must approve the changeset: a team is required when + * its `required` entry wins (last-match) for a changed file. + * + * @param { Set } files + * @param { OwnersEntry[] } entries + * @returns { Ownerships } required team -> files that triggered the requirement + * */ +export function resolveRequiredTeams(files, entries) { + /** @type { Ownerships } */ + const teamToFiles = new Map(); + + for (const file of [...files].sort()) { + const entry = findOwningEntry(file, entries); + if (!entry?.required) continue; + + const bucket = teamToFiles.get(entry.team); + + if (bucket) { + bucket.push(file); + } else { + teamToFiles.set(entry.team, [file]); + } + } + + return teamToFiles; +} + +/** + * @param { Ownerships } ownerships + * @returns { Allocation[] } + * */ +export function ownershipsToAllocations(ownerships) { + return Array.from(ownerships).map(([team, files]) => ({ + team, + fileCount: files.length, + })); +} + +/** + * Read a newline-delimited list of changed file paths from disk. + * Empty/whitespace-only lines are skipped. + * + * @param { string } path + * @returns { Set } + * */ +export function readChangedFilesList(path) { + return new Set( + readFileSync(path, "utf8") + .split("\n") + .map(line => line.trim()) + .filter(Boolean) + ); +} + +/** + * @returns { never } + */ +function exitWithUsage() { + console.error("Usage: node owners.mjs | --check"); + console.error(" : path to a file containing one changed path per line"); + console.error(" --check: validate the OWNERS file (syntax, duplicates, dead paths)"); + process.exit(1); +} + +function runCheck() { + /** @type { import('./owners.mjs').OwnersEntry[] } */ + let entries; + try { + entries = parseOwnersFile(); + } catch (error) { + console.error(`OWNERS is invalid: ${error.message}`); + process.exit(1); + } + + const errors = validateOwners(entries); + if (errors.length > 0) { + for (const error of errors) console.error(`OWNERS is invalid: ${error}`); + process.exit(1); + } + + const requiredCount = entries.filter((entry) => entry.required).length; + console.log(`OWNERS is valid: ${entries.length} entries, ${requiredCount} with required review.`); +} + +// CLI: `node owners.mjs ` prints ownership allocations for +// the given changed paths as JSON. `node owners.mjs --check` validates OWNERS. +if (import.meta.url === `file://${process.argv[1]}`) { + const arg = process.argv[2]; + if (!arg) exitWithUsage(); + + if (arg === "--check") { + runCheck(); + } else { + const files = readChangedFilesList(arg); + const ownerships = assignOwnership(files, parseOwnersFile()); + const totalFiles = files.size; + + const allocations = Array.from(ownerships) + .map(([team, ownedFiles]) => ({ + team, + fileCount: ownedFiles.length, + share: totalFiles === 0 ? 0 : Math.round((ownedFiles.length / totalFiles) * 100), + files: ownedFiles, + })) + .sort((a, b) => b.fileCount - a.fileCount); + + console.log(JSON.stringify({ totalFiles, allocations }, null, 4)); + } +} diff --git a/.github/scripts/owners/owners.test.mjs b/.github/scripts/owners/owners.test.mjs new file mode 100644 index 00000000000..0e7bc7e7b5c --- /dev/null +++ b/.github/scripts/owners/owners.test.mjs @@ -0,0 +1,336 @@ +import { describe, it } from 'node:test'; +import assert from 'node:assert/strict'; +import { + assignOwnership, + findOwningEntry, + ownershipsToAllocations, + parseOwnersContent, + parseOwnersFile, + resolveRequiredTeams, + teamHandleToSlug, + validateOwners, +} from './owners.mjs'; + +/** + * Run these tests by running + * + * node --test ./.github/scripts/owners/owners.test.mjs + * */ + +/** @param {Partial} entry */ +function entry(entry) { + return { team: '@n8n-io/some-team', required: false, line: 1, pattern: '*', ...entry }; +} + +describe('parseOwnersContent', () => { + it('parses well-formed OWNERS lines into entries', () => { + const content = [ + '# header comment', + '', + '* @n8n-io/catalysts', + 'packages/cli/ @n8n-io/cli-team', + 'packages/foo/bar.ts @n8n-io/some-team', + ].join('\n'); + + assert.deepEqual(parseOwnersContent(content), [ + { pattern: '*', team: '@n8n-io/catalysts', required: false, line: 3 }, + { pattern: 'packages/cli/', team: '@n8n-io/cli-team', required: false, line: 4 }, + { pattern: 'packages/foo/bar.ts', team: '@n8n-io/some-team', required: false, line: 5 }, + ]); + }); + + it('parses the `required` option after the team', () => { + const entries = parseOwnersContent('pkg/ @n8n-io/keepers required'); + + assert.deepEqual(entries, [ + { pattern: 'pkg/', team: '@n8n-io/keepers', required: true, line: 1 }, + ]); + }); + + it('throws on a second team', () => { + assert.throws( + () => parseOwnersContent('pkg/ @n8n-io/keepers @n8n-io/others'), + /OWNERS line 1: only one team per pattern is supported/, + ); + }); + + it('ignores inline comments', () => { + const entries = parseOwnersContent('pkg/ @n8n-io/keepers # the why'); + + assert.equal(entries[0].team, '@n8n-io/keepers'); + }); + + it('throws on an unknown token with the line number', () => { + assert.throws( + () => parseOwnersContent('# comment\npkg/ @n8n-io/keepers banana'), + /OWNERS line 2: unknown token "banana"/, + ); + }); + + it('throws when a pattern has no team', () => { + assert.throws(() => parseOwnersContent('pkg/ required'), /OWNERS line 1: no team/); + }); + + it('throws when a team comes after an option', () => { + assert.throws( + () => parseOwnersContent('pkg/ @n8n-io/keepers required @n8n-io/others'), + /OWNERS line 1: team "@n8n-io\/others" must come before options/, + ); + }); + + it('returns an empty array for comment-only content', () => { + assert.deepEqual(parseOwnersContent('# nothing here\n\n'), []); + }); +}); + +describe('parseOwnersFile', () => { + it('reads the real OWNERS file into well-formed entries', () => { + const entries = parseOwnersFile(); + + assert.ok(entries.length > 0, 'OWNERS file should not be empty'); + assert.ok( + entries.every((e) => e.team && e.pattern), + 'every parsed entry should have both a team and a pattern', + ); + assert.ok( + entries.every((e) => e.team.startsWith('@n8n-io/')), + 'every parsed team should belong to the @n8n-io org', + ); + }); + + it('the real OWNERS file passes validation', () => { + assert.deepEqual(validateOwners(parseOwnersFile()), []); + }); +}); + +describe('validateOwners', () => { + // Path kind stub matching the pattern shape, for tests about other rules. + const kindFromShape = (pattern) => (pattern.endsWith('/') ? 'directory' : 'file'); + + it('reports duplicate patterns with both line numbers', () => { + const entries = [ + entry({ pattern: 'pkg/', line: 3 }), + entry({ pattern: 'pkg/', line: 7 }), + ]; + + const errors = validateOwners(entries, kindFromShape); + + assert.equal(errors.length, 1); + assert.match(errors[0], /duplicate pattern "pkg\/" \(lines 3 and 7\)/); + }); + + it('reports patterns that do not exist, except the catch-all', () => { + const entries = [entry({ pattern: '*' }), entry({ pattern: 'gone/', line: 2 })]; + + const errors = validateOwners(entries, () => null); + + assert.equal(errors.length, 1); + assert.match(errors[0], /pattern "gone\/" \(line 2\) does not exist/); + }); + + it('reports a directory pattern that points at a file', () => { + const errors = validateOwners([entry({ pattern: 'pkg/', line: 1 })], () => 'file'); + + assert.equal(errors.length, 1); + assert.match(errors[0], /pattern "pkg\/" \(line 1\) is a file; remove the trailing "\/"/); + }); + + it('reports a file pattern that points at a directory', () => { + const errors = validateOwners([entry({ pattern: 'pkg', line: 1 })], () => 'directory'); + + assert.equal(errors.length, 1); + assert.match(errors[0], /pattern "pkg" \(line 1\) is a directory; add a trailing "\/"/); + }); + + it('returns no errors for a valid set of entries', () => { + const entries = [ + entry({ pattern: '*' }), + entry({ pattern: 'pkg/', line: 2 }), + entry({ pattern: 'pkg/a.ts', line: 3 }), + ]; + + assert.deepEqual(validateOwners(entries, kindFromShape), []); + }); +}); + + +describe('teamHandleToSlug', () => { + it('strips the org prefix from an OWNERS team handle', () => { + assert.equal(teamHandleToSlug('@n8n-io/catalysts'), 'catalysts'); + }); + + it('leaves a bare slug untouched', () => { + assert.equal(teamHandleToSlug('catalysts'), 'catalysts'); + }); +}); + +describe('findOwningEntry', () => { + it('returns the last matching entry', () => { + const entries = [ + entry({ pattern: '*', team: '@n8n-io/catalysts' }), + entry({ pattern: 'pkg/', team: '@n8n-io/keepers' }), + ]; + + assert.equal(findOwningEntry('pkg/a.ts', entries), entries[1]); + assert.equal(findOwningEntry('other.ts', entries), entries[0]); + }); + + it('returns undefined when nothing matches', () => { + assert.equal(findOwningEntry('a.ts', [entry({ pattern: 'pkg/' })]), undefined); + }); +}); + +describe('assignOwnership', () => { + it('assigns every file to the catch-all team when only `*` is defined', () => { + const files = new Set(['a.ts', 'packages/cli/src/index.ts', 'docs/readme.md']); + const owners = [entry({ pattern: '*', team: '@n8n-io/catalysts' })]; + + const result = assignOwnership(files, owners); + + assert.deepEqual( + result.get('@n8n-io/catalysts')?.sort(), + [...files].sort(), + ); + assert.equal(result.size, 1); + }); + + it('applies last-match-wins: a later specific rule overrides the catch-all', () => { + const files = new Set([ + 'README.md', + 'packages/cli/src/index.ts', + 'packages/cli/src/lib/foo.ts', + ]); + const owners = [ + entry({ pattern: '*', team: '@n8n-io/catalysts' }), + entry({ pattern: 'packages/cli/', team: '@n8n-io/cli-team' }), + ]; + + const result = assignOwnership(files, owners); + + assert.deepEqual(result.get('@n8n-io/catalysts'), ['README.md']); + assert.deepEqual( + result.get('@n8n-io/cli-team')?.sort(), + ['packages/cli/src/index.ts', 'packages/cli/src/lib/foo.ts'].sort(), + ); + }); + + it('matches a directory pattern recursively', () => { + const files = new Set([ + 'packages/cli/src/deep/nested/file.ts', + 'packages/cli/package.json', + ]); + const owners = [entry({ pattern: 'packages/cli/', team: '@n8n-io/cli-team' })]; + + const result = assignOwnership(files, owners); + + assert.deepEqual( + result.get('@n8n-io/cli-team')?.sort(), + [...files].sort(), + ); + }); + + it('matches an exact file pattern only against that file', () => { + const files = new Set([ + 'packages/cli/src/controllers/ai.controller.ts', + 'packages/cli/src/controllers/other.controller.ts', + ]); + const owners = [ + entry({ + pattern: 'packages/cli/src/controllers/ai.controller.ts', + team: '@n8n-io/ai-team', + }), + ]; + + const result = assignOwnership(files, owners); + + assert.deepEqual(result.get('@n8n-io/ai-team'), [ + 'packages/cli/src/controllers/ai.controller.ts', + ]); + // the other controller matched no rule, so it must be omitted entirely + assert.equal(result.size, 1); + }); + + it('omits files that match no rule (no catch-all present)', () => { + const files = new Set(['unowned/file.ts', 'packages/cli/src/x.ts']); + const owners = [entry({ pattern: 'packages/cli/', team: '@n8n-io/cli-team' })]; + + const result = assignOwnership(files, owners); + + assert.deepEqual(result.get('@n8n-io/cli-team'), ['packages/cli/src/x.ts']); + assert.equal(result.size, 1); + }); + + it('returns an empty Map when there are no changed files', () => { + const owners = [entry({ pattern: '*', team: '@n8n-io/catalysts' })]; + const result = assignOwnership(new Set(), owners); + + assert.equal(result.size, 0); + }); +}); + +describe('resolveRequiredTeams', () => { + const owners = [ + entry({ pattern: '*', team: '@n8n-io/catalysts' }), + entry({ pattern: '.github/', team: '@n8n-io/qa-dx' }), + entry({ pattern: '.github/workflows/', team: '@n8n-io/qa-dx', required: true }), + entry({ pattern: 'db/migrations/', team: '@n8n-io/migrations-review', required: true }), + ]; + + it('collects the teams of `required` entries that win for a changed file', () => { + const result = resolveRequiredTeams( + new Set(['.github/workflows/ci.yml', 'db/migrations/1-init.ts', 'src/a.ts']), + owners, + ); + + assert.deepEqual(result.get('@n8n-io/qa-dx'), ['.github/workflows/ci.yml']); + assert.deepEqual(result.get('@n8n-io/migrations-review'), ['db/migrations/1-init.ts']); + assert.equal(result.size, 2); + }); + + it('ignores files whose winning entry is not required', () => { + const result = resolveRequiredTeams(new Set(['OWNERS', 'src/a.ts']), owners); + + assert.equal(result.size, 0); + }); + + it('a later non-required entry overrides an earlier required one', () => { + const result = resolveRequiredTeams(new Set(['db/migrations/1-init.ts']), [ + entry({ pattern: 'db/', team: '@n8n-io/migrations-review', required: true }), + entry({ pattern: 'db/migrations/', team: '@n8n-io/catalysts' }), + ]); + + assert.equal(result.size, 0); + }); + + it('lists files deterministically (sorted) per team', () => { + const result = resolveRequiredTeams( + new Set(['.github/workflows/z.yml', '.github/workflows/a.yml']), + owners, + ); + + assert.deepEqual(result.get('@n8n-io/qa-dx'), [ + '.github/workflows/a.yml', + '.github/workflows/z.yml', + ]); + }); +}); + +describe('ownershipsToAllocations', () => { + it('converts a Map of team -> files into Allocation[] with fileCount', () => { + const ownerships = new Map([ + ['@n8n-io/cli-team', ['a.ts', 'b.ts', 'c.ts']], + ['@n8n-io/catalysts', ['README.md']], + ]); + + const result = ownershipsToAllocations(ownerships); + + assert.deepEqual(result, [ + { team: '@n8n-io/cli-team', fileCount: 3 }, + { team: '@n8n-io/catalysts', fileCount: 1 }, + ]); + }); + + it('returns an empty array for an empty Map', () => { + assert.deepEqual(ownershipsToAllocations(new Map()), []); + }); +}); diff --git a/.github/scripts/owners/required-reviews.mjs b/.github/scripts/owners/required-reviews.mjs new file mode 100644 index 00000000000..e3b28918d4d --- /dev/null +++ b/.github/scripts/owners/required-reviews.mjs @@ -0,0 +1,213 @@ +/** + * Required-review enforcement for OWNERS entries marked `required`. + * + * Computes the teams whose approval the changeset needs (per OWNERS) + * and reports the verdict as a "Required Reviews" commit status on the PR + * head SHA. A ruleset that lists this status as a required check blocks the + * merge until a member of each required team has approved the PR. Merge-queue + * runs do not reach this script: the workflow reports success on the queue + * head directly, because entering the queue already required a green status. + * + * The job itself always succeeds when the evaluation runs; the commit status + * carries the verdict. The status is set to pending before the evaluation + * starts, so a crash (e.g. the team membership API is unavailable) cannot + * leave an earlier green status in effect — the gate fails closed. + */ + +import { + getChangedFiles, + getEventFromGithubEventPath, + getPrReviews, + getPullRequestById, + isTeamMember, + setCommitStatus, +} from '../github-helpers.mjs'; +import { parseOwnersFile, resolveRequiredTeams, teamHandleToSlug } from './owners.mjs'; + +export const STATUS_CONTEXT = 'Required Reviews'; +const TARGET_BRANCH = 'master'; + +/** + * A PR review as returned by the reviews API. + * + * @typedef Review + * @property { { login: string } | null } user + * @property { string } state + * @property { string } [submitted_at] + */ + +/** + * Resolve the PR number from the triggering event. + * + * @param { string } eventName + * @param { any } event Parsed GITHUB_EVENT_PATH payload. + * @param { string | undefined } pullRequestNumberEnv PULL_REQUEST_NUMBER (workflow_dispatch input). + * @returns { number } + */ +export function resolvePullRequestNumber(eventName, event, pullRequestNumberEnv) { + if (event?.pull_request?.number) return event.pull_request.number; + + const parsed = parseInt(pullRequestNumberEnv ?? ''); + if (Number.isNaN(parsed)) { + throw new Error(`Cannot resolve a PR number for event "${eventName}"`); + } + return parsed; +} + +/** + * Reduce a PR's review history to the latest meaningful state per reviewer, + * mirroring how GitHub itself treats reviews: a later APPROVED or + * CHANGES_REQUESTED (or a dismissal) replaces the reviewer's earlier state; + * COMMENTED and PENDING reviews carry no state. + * + * @param { Review[] } reviews + * @returns { Map } login -> latest review state + */ +export function latestReviewStates(reviews) { + const sorted = reviews + .filter((review) => review.user?.login) + .filter((review) => ['APPROVED', 'CHANGES_REQUESTED', 'DISMISSED'].includes(review.state)) + .toSorted((a, b) => new Date(a.submitted_at ?? 0) - new Date(b.submitted_at ?? 0)); + + const states = new Map(); + for (const review of sorted) { + states.set(review.user.login, review.state); + } + return states; +} + +/** + * @param { Review[] } reviews + * @returns { Set } logins whose latest review state is APPROVED + */ +export function collectApprovers(reviews) { + return new Set( + [...latestReviewStates(reviews)] + .filter(([, state]) => state === 'APPROVED') + .map(([login]) => login), + ); +} + +/** + * @param { string[] } missingTeams Team handles without an approving member. + * @param { number } requiredCount Total number of required teams. + * @returns {{ state: 'success' | 'failure', description: string }} + */ +export function buildStatus(missingTeams, requiredCount) { + if (requiredCount === 0) { + return { state: 'success', description: 'No team approval is required for these changes' }; + } + + if (missingTeams.length === 0) { + return { + state: 'success', + description: `All required team approvals are present (${requiredCount} team${requiredCount === 1 ? '' : 's'})`, + }; + } + + return { + state: 'failure', + description: `Missing approval from: ${missingTeams.map(teamHandleToSlug).join(', ')}`, + }; +} + +function statusTargetUrl() { + const { GITHUB_SERVER_URL, GITHUB_REPOSITORY, GITHUB_RUN_ID } = process.env; + if (!GITHUB_SERVER_URL || !GITHUB_REPOSITORY || !GITHUB_RUN_ID) return undefined; + return `${GITHUB_SERVER_URL}/${GITHUB_REPOSITORY}/actions/runs/${GITHUB_RUN_ID}`; +} + +/** + * @param { number } pullRequestNumber + * @returns { Promise<{ state: 'success' | 'failure', description: string }> } + */ +async function evaluateRequiredReviews(pullRequestNumber) { + const changedFiles = await getChangedFiles(pullRequestNumber); + const requiredTeams = resolveRequiredTeams(changedFiles, parseOwnersFile()); + + /** @type { string[] } */ + const missingTeams = []; + + if (requiredTeams.size > 0) { + const approvers = collectApprovers(await getPrReviews(pullRequestNumber)); + console.log(`Current approvals: ${[...approvers].join(', ') || '(none)'}`); + + for (const [team, files] of requiredTeams) { + const slug = teamHandleToSlug(team); + // Per-approver membership checks instead of fetching the roster: + // approvers are few, teams can be large. + const teamApprovers = []; + for (const login of approvers) { + if (await isTeamMember(slug, login)) teamApprovers.push(login); + } + const verdict = teamApprovers.length > 0 ? `approved by ${teamApprovers.join(', ')}` : 'approval missing'; + + console.log(`${team}: ${verdict} — owns ${files.length} changed file(s):`); + for (const file of files) console.log(` - ${file}`); + + if (teamApprovers.length === 0) missingTeams.push(team); + } + } else { + console.log('No changed file matches a `required` OWNERS entry.'); + } + + return buildStatus(missingTeams, requiredTeams.size); +} + +export async function run() { + const eventName = process.env.GITHUB_EVENT_NAME ?? 'workflow_dispatch'; + const event = getEventFromGithubEventPath(); + + const pullRequestNumber = resolvePullRequestNumber( + eventName, + event, + process.env.PULL_REQUEST_NUMBER, + ); + const pullRequest = await getPullRequestById(pullRequestNumber); + + if (pullRequest.base.ref !== TARGET_BRANCH) { + console.log(`PR #${pullRequestNumber} targets "${pullRequest.base.ref}", not "${TARGET_BRANCH}"; skipping.`); + return; + } + + const statusSha = pullRequest.head.sha; + const targetUrl = statusTargetUrl(); + + // Pending first: it replaces any earlier verdict on this SHA, so a crash + // during evaluation cannot leave a stale green status in effect. + await setCommitStatus(statusSha, { + state: 'pending', + context: STATUS_CONTEXT, + description: 'Evaluating required reviews', + targetUrl, + }); + + /** @type { { state: 'success' | 'failure', description: string } } */ + let status; + try { + status = await evaluateRequiredReviews(pullRequestNumber); + } catch (evaluationError) { + // Best effort: nicer than a stuck pending status. The pending status + // already blocks the merge if this write fails too. + try { + await setCommitStatus(statusSha, { + state: 'error', + context: STATUS_CONTEXT, + description: 'Evaluation failed; see the workflow run', + targetUrl, + }); + } catch {} + throw evaluationError; + } + + console.log(`Setting "${STATUS_CONTEXT}" on ${statusSha} to ${status.state}: ${status.description}`); + await setCommitStatus(statusSha, { + ...status, + context: STATUS_CONTEXT, + targetUrl, + }); +} + +if (import.meta.url === `file://${process.argv[1]}`) { + await run(); +} diff --git a/.github/scripts/owners/required-reviews.test.mjs b/.github/scripts/owners/required-reviews.test.mjs new file mode 100644 index 00000000000..0ac873524b7 --- /dev/null +++ b/.github/scripts/owners/required-reviews.test.mjs @@ -0,0 +1,281 @@ +import { beforeEach, describe, it, mock } from 'node:test'; +import assert from 'node:assert/strict'; + +/** + * Run these tests by running + * + * node --test --experimental-test-module-mocks ./.github/scripts/owners/required-reviews.test.mjs + * */ + +/** @type {() => any} */ +let eventImpl = () => ({}); +/** @type {(pullRequestNumber: number) => Promise} */ +let getPullRequestByIdImpl = async () => ({}); +/** @type {(pullRequestNumber: number) => Promise>} */ +let getChangedFilesImpl = async () => new Set(); +/** @type {(pullRequestNumber: number) => Promise} */ +let getPrReviewsImpl = async () => []; +/** @type {(teamSlug: string, username: string) => Promise} */ +let isTeamMemberImpl = async () => false; +/** @type {(sha: string, status: any) => Promise} */ +let setCommitStatusImpl = async () => {}; + +mock.module('../github-helpers.mjs', { + namedExports: { + getEventFromGithubEventPath: () => eventImpl(), + getPullRequestById: (n) => getPullRequestByIdImpl(n), + getChangedFiles: (n) => getChangedFilesImpl(n), + getPrReviews: (n) => getPrReviewsImpl(n), + isTeamMember: (slug, username) => isTeamMemberImpl(slug, username), + setCommitStatus: (sha, status) => setCommitStatusImpl(sha, status), + }, +}); + +/** @type {() => Array} */ +let parseOwnersFileImpl = () => []; +/** @type {(files: Set, entries: any[]) => Map} */ +let resolveRequiredTeamsImpl = () => new Map(); + +mock.module('./owners.mjs', { + namedExports: { + parseOwnersFile: () => parseOwnersFileImpl(), + resolveRequiredTeams: (files, entries) => resolveRequiredTeamsImpl(files, entries), + // Mirrors the real implementation; the mock replaces the whole module. + teamHandleToSlug: (team) => team.replace(/^@[^/]+\//, ''), + }, +}); + +const { + STATUS_CONTEXT, + buildStatus, + collectApprovers, + latestReviewStates, + resolvePullRequestNumber, + run, +} = await import('./required-reviews.mjs'); + +describe('resolvePullRequestNumber', () => { + it('reads the PR number from pull_request payloads', () => { + assert.equal( + resolvePullRequestNumber('pull_request', { pull_request: { number: 42 } }, undefined), + 42, + ); + }); + + it('falls back to the PULL_REQUEST_NUMBER env value for workflow_dispatch', () => { + assert.equal(resolvePullRequestNumber('workflow_dispatch', {}, '7'), 7); + }); + + it('throws when no source yields a PR number', () => { + assert.throws(() => resolvePullRequestNumber('workflow_dispatch', {}, undefined), /Cannot resolve/); + }); +}); + +describe('latestReviewStates', () => { + it('keeps only the latest meaningful state per reviewer', () => { + const states = latestReviewStates([ + { user: { login: 'ana' }, state: 'CHANGES_REQUESTED', submitted_at: '2026-01-01T00:00:00Z' }, + { user: { login: 'ana' }, state: 'APPROVED', submitted_at: '2026-01-02T00:00:00Z' }, + { user: { login: 'ben' }, state: 'APPROVED', submitted_at: '2026-01-01T00:00:00Z' }, + { user: { login: 'ben' }, state: 'DISMISSED', submitted_at: '2026-01-03T00:00:00Z' }, + ]); + + assert.equal(states.get('ana'), 'APPROVED'); + assert.equal(states.get('ben'), 'DISMISSED'); + }); + + it('ignores COMMENTED and PENDING reviews', () => { + const states = latestReviewStates([ + { user: { login: 'ana' }, state: 'APPROVED', submitted_at: '2026-01-01T00:00:00Z' }, + { user: { login: 'ana' }, state: 'COMMENTED', submitted_at: '2026-01-02T00:00:00Z' }, + { user: { login: 'ben' }, state: 'PENDING' }, + ]); + + assert.equal(states.get('ana'), 'APPROVED'); + assert.equal(states.has('ben'), false); + }); + + it('sorts by submission time, not input order', () => { + const states = latestReviewStates([ + { user: { login: 'ana' }, state: 'APPROVED', submitted_at: '2026-01-02T00:00:00Z' }, + { user: { login: 'ana' }, state: 'CHANGES_REQUESTED', submitted_at: '2026-01-01T00:00:00Z' }, + ]); + + assert.equal(states.get('ana'), 'APPROVED'); + }); + + it('skips reviews without a user', () => { + assert.equal(latestReviewStates([{ user: null, state: 'APPROVED' }]).size, 0); + }); +}); + +describe('collectApprovers', () => { + it('returns reviewers whose latest state is APPROVED', () => { + const approvers = collectApprovers([ + { user: { login: 'ana' }, state: 'APPROVED', submitted_at: '2026-01-01T00:00:00Z' }, + { user: { login: 'ben' }, state: 'CHANGES_REQUESTED', submitted_at: '2026-01-01T00:00:00Z' }, + ]); + + assert.deepEqual([...approvers], ['ana']); + }); +}); + +describe('buildStatus', () => { + it('succeeds when nothing is required', () => { + const status = buildStatus([], 0); + + assert.equal(status.state, 'success'); + assert.match(status.description, /No team approval is required/); + }); + + it('succeeds when all required teams approved', () => { + const status = buildStatus([], 2); + + assert.equal(status.state, 'success'); + assert.match(status.description, /2 teams/); + }); + + it('fails with the missing team slugs', () => { + const status = buildStatus(['@n8n-io/qa-dx', '@n8n-io/migrations-review'], 2); + + assert.equal(status.state, 'failure'); + assert.equal(status.description, 'Missing approval from: qa-dx, migrations-review'); + }); +}); + +describe('run', () => { + let setCommitStatus; + + beforeEach(() => { + process.env.GITHUB_EVENT_NAME = 'pull_request'; + delete process.env.PULL_REQUEST_NUMBER; + + eventImpl = () => ({ pull_request: { number: 42 } }); + getPullRequestByIdImpl = async () => ({ + base: { ref: 'master' }, + head: { sha: 'head-sha' }, + user: { login: 'author' }, + }); + getChangedFilesImpl = async () => new Set(['a.ts']); + getPrReviewsImpl = async () => []; + isTeamMemberImpl = async () => false; + parseOwnersFileImpl = () => []; + resolveRequiredTeamsImpl = () => new Map(); + setCommitStatus = mock.fn(async () => {}); + setCommitStatusImpl = setCommitStatus; + }); + + it('sets a pending status first, then a success status when nothing is required', async () => { + await run(); + + assert.equal(setCommitStatus.mock.calls.length, 2); + + const [pendingSha, pendingStatus] = setCommitStatus.mock.calls[0].arguments; + assert.equal(pendingSha, 'head-sha'); + assert.equal(pendingStatus.context, STATUS_CONTEXT); + assert.equal(pendingStatus.state, 'pending'); + + const [sha, status] = setCommitStatus.mock.calls[1].arguments; + assert.equal(sha, 'head-sha'); + assert.equal(status.context, STATUS_CONTEXT); + assert.equal(status.state, 'success'); + }); + + it('sets a failure status when a required team has not approved', async () => { + resolveRequiredTeamsImpl = () => new Map([['@n8n-io/qa-dx', ['a.ts']]]); + getPrReviewsImpl = async () => [ + { user: { login: 'outsider' }, state: 'APPROVED', submitted_at: '2026-01-01T00:00:00Z' }, + ]; + isTeamMemberImpl = async () => false; + + await run(); + + const [, status] = setCommitStatus.mock.calls.at(-1).arguments; + assert.equal(status.state, 'failure'); + assert.match(status.description, /Missing approval from: qa-dx/); + }); + + it('sets an error status and rethrows when the evaluation fails', async () => { + getChangedFilesImpl = async () => { + throw new Error('API unavailable'); + }; + + await assert.rejects(run(), /API unavailable/); + + assert.equal(setCommitStatus.mock.calls.length, 2); + assert.equal(setCommitStatus.mock.calls[0].arguments[1].state, 'pending'); + assert.equal(setCommitStatus.mock.calls[1].arguments[1].state, 'error'); + }); + + it('sets a success status when a member of each required team approved', async () => { + resolveRequiredTeamsImpl = () => + new Map([ + ['@n8n-io/qa-dx', ['a.ts']], + ['@n8n-io/migrations-review', ['m.ts']], + ]); + getPrReviewsImpl = async () => [ + { user: { login: 'poly' }, state: 'APPROVED', submitted_at: '2026-01-01T00:00:00Z' }, + ]; + // `poly` is a member of both required teams. + isTeamMemberImpl = async (slug, username) => username === 'poly'; + + await run(); + + const [, status] = setCommitStatus.mock.calls.at(-1).arguments; + assert.equal(status.state, 'success'); + }); + + it('checks membership per approver and required team', async () => { + resolveRequiredTeamsImpl = () => new Map([['@n8n-io/qa-dx', ['a.ts']]]); + getPrReviewsImpl = async () => [ + { user: { login: 'poly' }, state: 'APPROVED', submitted_at: '2026-01-01T00:00:00Z' }, + ]; + const isTeamMember = mock.fn(async () => true); + isTeamMemberImpl = isTeamMember; + + await run(); + + assert.deepEqual( + isTeamMember.mock.calls.map((call) => call.arguments), + [['qa-dx', 'poly']], + ); + }); + + it('does not check membership when there is no approval', async () => { + resolveRequiredTeamsImpl = () => new Map([['@n8n-io/qa-dx', ['a.ts']]]); + const isTeamMember = mock.fn(async () => true); + isTeamMemberImpl = isTeamMember; + + await run(); + + assert.equal(isTeamMember.mock.calls.length, 0); + const [, status] = setCommitStatus.mock.calls.at(-1).arguments; + assert.equal(status.state, 'failure'); + }); + + it('does not count an approval that was later dismissed', async () => { + resolveRequiredTeamsImpl = () => new Map([['@n8n-io/qa-dx', ['a.ts']]]); + getPrReviewsImpl = async () => [ + { user: { login: 'poly' }, state: 'APPROVED', submitted_at: '2026-01-01T00:00:00Z' }, + { user: { login: 'poly' }, state: 'DISMISSED', submitted_at: '2026-01-02T00:00:00Z' }, + ]; + isTeamMemberImpl = async (slug, username) => username === 'poly'; + + await run(); + + const [, status] = setCommitStatus.mock.calls.at(-1).arguments; + assert.equal(status.state, 'failure'); + }); + + it('skips PRs that do not target master without setting a status', async () => { + getPullRequestByIdImpl = async () => ({ + base: { ref: 'release-candidate/2.9.x' }, + head: { sha: 'head-sha' }, + }); + + await run(); + + assert.equal(setCommitStatus.mock.calls.length, 0); + }); + +}); diff --git a/.github/scripts/owners-review-recommendations.mjs b/.github/scripts/owners/review-recommendations.mjs similarity index 78% rename from .github/scripts/owners-review-recommendations.mjs rename to .github/scripts/owners/review-recommendations.mjs index e054efc3584..487630d7afb 100644 --- a/.github/scripts/owners-review-recommendations.mjs +++ b/.github/scripts/owners/review-recommendations.mjs @@ -4,14 +4,21 @@ * Posts (or updates) a single PR comment that combines: * - Recommended reviewer teams based on file ownership * - A breakdown of changed lines by category (source code, test files, misc) + * - The required team reviews (OWNERS entries with `required`), with a + * prompt to request review from those teams * - * Advisory only — does not gate merging. + * Advisory only — does not gate merging. Enforcement of required reviews + * lives in required-reviews.mjs. */ -import { minimatch } from 'minimatch'; -import { ensureEnvVar, getPrFiles, postOrUpdateComment } from './github-helpers.mjs'; -import { assignOwnership, ownershipsToAllocations, parseOwnersFile } from './owners.mjs'; -import { MISC_PATTERNS, SIZE_LIMIT, TEST_PATTERNS } from './quality/check-pr-size.mjs'; +import { ensureEnvVar, getPrFiles, postOrUpdateComment } from '../github-helpers.mjs'; +import { categorizeFile, SIZE_LIMIT } from '../quality/check-pr-size.mjs'; +import { + assignOwnership, + ownershipsToAllocations, + parseOwnersFile, + resolveRequiredTeams, +} from './owners.mjs'; /** @typedef {import('./owners.mjs').Allocation} Allocation */ @@ -41,19 +48,9 @@ function createEmptyLineStats() { * @param {{ filename: string, additions: number, deletions: number }} file */ function addFileToLineStats(stats, file) { - const isTest = TEST_PATTERNS.some((p) => minimatch(file.filename, p)); - const isMisc = !isTest && MISC_PATTERNS.some((p) => minimatch(file.filename, p)); - - if (isTest) { - stats.testFilesAdded += file.additions; - stats.testFilesRemoved += file.deletions; - } else if (isMisc) { - stats.miscAdded += file.additions; - stats.miscRemoved += file.deletions; - } else { - stats.sourceCodeAdded += file.additions; - stats.sourceCodeRemoved += file.deletions; - } + const category = categorizeFile(file.filename); + stats[`${category}Added`] += file.additions; + stats[`${category}Removed`] += file.deletions; } /** @@ -192,6 +189,35 @@ export function buildOverviewTable(allocations, changedFiles, totalLineStats, li ].join('\n'); } +/** + * Build the section that lists required team approvals. Returns null when no + * approval is required. + * + * Suggests a team review request instead of naming an individual: GitHub then + * assigns reviewers according to the team's own review settings (assignment + * algorithm, excluded members), which the API does not let us read. + * + * @param { Map } requiredTeamFiles Required team handle -> files that triggered the requirement. + * @returns { string | null } + */ +export function buildRequiredReviewsSection(requiredTeamFiles) { + if (requiredTeamFiles.size === 0) return null; + + const plural = requiredTeamFiles.size > 1; + + return [ + '### Required reviews', + '', + 'Some changed files have a `required` owner in `OWNERS`. A member of each of these teams must approve this PR before it can merge:', + '', + '| Team | Files |', + '| --- | ---: |', + ...[...requiredTeamFiles].map(([team, files]) => `| ${team} | ${files.length} |`), + '', + `Request a review from the team${plural ? 's' : ''} — GitHub assigns reviewers according to the team's review settings. The \`Auto-assign reviewers\` label does this for all owning teams.`, + ].join('\n'); +} + /** * Construct the full PR comment body from reviewer allocations and line stats. * @@ -200,14 +226,19 @@ export function buildOverviewTable(allocations, changedFiles, totalLineStats, li * @param { LineStats } lineStats * @param { Map } lineStatsByTeam * @param { Allocation[] } [otherAllocations] + * @param { string | null } [requiredSection] * @returns { string } */ -export function buildComment(allocations, changedFiles, lineStats, lineStatsByTeam = new Map(), otherAllocations = []) { +export function buildComment(allocations, changedFiles, lineStats, lineStatsByTeam = new Map(), otherAllocations = [], requiredSection = null) { const body = [ BOT_MARKER, buildOverviewTable(allocations, changedFiles, lineStats, lineStatsByTeam, otherAllocations), ]; + if (requiredSection) { + body.push('', requiredSection); + } + if (lineStats.sourceCodeAdded > SIZE_LIMIT) { body.push('', `❗ Source code additions (${lineStats.sourceCodeAdded.toLocaleString()}) exceed the ${SIZE_LIMIT.toLocaleString()}-line limit.`); } @@ -243,7 +274,10 @@ export async function run(pullRequestNumber) { const topAllocations = sortedAllocations.slice(0, 3); const otherAllocations = sortedAllocations.slice(3); - const body = buildComment(topAllocations, changedFiles, lineStats, lineStatsByTeam, otherAllocations); + const requiredTeamFiles = resolveRequiredTeams(changedFiles, owners); + const requiredSection = buildRequiredReviewsSection(requiredTeamFiles); + + const body = buildComment(topAllocations, changedFiles, lineStats, lineStatsByTeam, otherAllocations, requiredSection); await postOrUpdateComment(pullRequestNumber, body, BOT_MARKER); } diff --git a/.github/scripts/owners-review-recommendations.test.mjs b/.github/scripts/owners/review-recommendations.test.mjs similarity index 82% rename from .github/scripts/owners-review-recommendations.test.mjs rename to .github/scripts/owners/review-recommendations.test.mjs index 5826e559970..b55f22e2a0d 100644 --- a/.github/scripts/owners-review-recommendations.test.mjs +++ b/.github/scripts/owners/review-recommendations.test.mjs @@ -4,7 +4,7 @@ import assert from 'node:assert/strict'; /** * Run these tests by running * - * node --test --experimental-test-module-mocks ./.github/scripts/owners-review-recommendations.test.mjs + * node --test --experimental-test-module-mocks ./.github/scripts/owners/review-recommendations.test.mjs * */ /** @type {(pullRequestNumber: number) => Promise>} */ @@ -12,7 +12,7 @@ let getPrFilesImpl = async () => []; /** @type {(pullRequestNumber: number, body: string, botMarker: string) => Promise} */ let postOrUpdateCommentImpl = async () => {}; -mock.module('./github-helpers.mjs', { +mock.module('../github-helpers.mjs', { namedExports: { ensureEnvVar: () => {}, // no-op in tests initGithub: () => {}, // no-op in tests @@ -24,23 +24,32 @@ mock.module('./github-helpers.mjs', { }, }); -/** @type {() => Array<{ filepath: string, team: string }>} */ +/** @type {() => Array} */ let parseOwnersFileImpl = () => []; -/** @type {(files: Set, owners: Array<{ filepath: string, team: string }>) => Map} */ +/** @type {(files: Set, entries: any[]) => Map} */ let assignOwnershipImpl = () => new Map(); /** @type {(ownerships: Map) => Array<{ team: string, fileCount: number }>} */ let ownershipsToAllocationsImpl = () => []; +/** @type {(files: Set, entries: any[]) => Map} */ +let resolveRequiredTeamsImpl = () => new Map(); mock.module('./owners.mjs', { namedExports: { parseOwnersFile: () => parseOwnersFileImpl(), - assignOwnership: (files, owners) => assignOwnershipImpl(files, owners), + assignOwnership: (files, entries) => assignOwnershipImpl(files, entries), ownershipsToAllocations: (ownerships) => ownershipsToAllocationsImpl(ownerships), + resolveRequiredTeams: (files, entries) => resolveRequiredTeamsImpl(files, entries), }, }); -const { buildOverviewTable, buildComment, computeAllocationLineStats, computeLineStats, run } = - await import('./owners-review-recommendations.mjs'); +const { + buildOverviewTable, + buildComment, + buildRequiredReviewsSection, + computeAllocationLineStats, + computeLineStats, + run, +} = await import('./review-recommendations.mjs'); const BOT_MARKER = ''; const EMPTY_LINE_STATS = { @@ -209,6 +218,7 @@ describe('computeLineStats', () => { ['fixtures/ directory', 'src/fixtures/data.json'], ['__mocks__/ directory', 'src/__mocks__/axios.ts'], ['packages/testing/**', 'packages/testing/playwright/spec.ts'], + ['dot-directory *.test.mjs', '.github/scripts/owners/owners.test.mjs'], ]) { it(`classifies ${label} as testFiles`, () => { const stats = computeLineStats([{ filename, additions: 7, deletions: 2 }]); @@ -307,6 +317,38 @@ describe('computeAllocationLineStats', () => { }); }); +describe('buildRequiredReviewsSection', () => { + it('returns null when no approval is required', () => { + assert.equal(buildRequiredReviewsSection(new Map()), null); + }); + + it('lists each required team with its file count', () => { + const section = buildRequiredReviewsSection( + new Map([ + ['@n8n-io/qa-dx', ['a.yml', 'b.yml']], + ['@n8n-io/migrations-review', ['m.ts']], + ]), + ); + + assert.match(section, /### Required reviews/); + assert.match(section, /\| @n8n-io\/qa-dx \| 2 \|/); + assert.match(section, /\| @n8n-io\/migrations-review \| 1 \|/); + }); + + it('prompts to request review from the team, plural when several teams are required', () => { + const singular = buildRequiredReviewsSection(new Map([['@n8n-io/qa-dx', ['a.yml']]])); + const plural = buildRequiredReviewsSection( + new Map([ + ['@n8n-io/qa-dx', ['a.yml']], + ['@n8n-io/migrations-review', ['m.ts']], + ]), + ); + + assert.match(singular, /Request a review from the team —/); + assert.match(plural, /Request a review from the teams —/); + }); +}); + describe('run', () => { let postOrUpdateComment; @@ -317,6 +359,7 @@ describe('run', () => { parseOwnersFileImpl = () => []; assignOwnershipImpl = () => new Map(); ownershipsToAllocationsImpl = () => []; + resolveRequiredTeamsImpl = () => new Map(); }); it('calls postOrUpdateComment with the PR number, generated body, and bot marker', async () => { @@ -385,4 +428,26 @@ describe('run', () => { assert.match(body, /## PR review overview/); assert.match(body, /\| @n8n-io\/cli-team \| 1 \| 100% \| \+50 \/ -5 \|/); }); + + it('omits the required reviews section when no approval is required', async () => { + await run(42); + + const body = postOrUpdateComment.mock.calls[0].arguments[1]; + assert.doesNotMatch(body, /### Required reviews/); + }); + + it('includes the required reviews section with the team review prompt', async () => { + getPrFilesImpl = async () => [{ filename: '.github/workflows/ci.yml', additions: 1, deletions: 0 }]; + parseOwnersFileImpl = () => [ + { pattern: '.github/workflows/', team: '@n8n-io/qa-dx', required: true, line: 1 }, + ]; + resolveRequiredTeamsImpl = () => new Map([['@n8n-io/qa-dx', ['.github/workflows/ci.yml']]]); + + await run(42); + + const body = postOrUpdateComment.mock.calls[0].arguments[1]; + assert.match(body, /### Required reviews/); + assert.match(body, /\| @n8n-io\/qa-dx \| 1 \|/); + assert.match(body, /Request a review from the team —/); + }); }); diff --git a/.github/scripts/package.json b/.github/scripts/package.json index 1d60aad2a3c..3c92ce300b1 100644 --- a/.github/scripts/package.json +++ b/.github/scripts/package.json @@ -6,7 +6,7 @@ }, "packageManager": "pnpm@11.22.0", "scripts": { - "test": "node --test --experimental-test-module-mocks ./*.test.mjs ./docker/*.test.mjs ./quality/*.test.mjs ./slack/*.test.mjs ./stale/*.test.mjs ../../scripts/licenses/*.test.mjs ../../scripts/mutation-health/*.test.mjs", + "test": "node --test --experimental-test-module-mocks ./*.test.mjs ./docker/*.test.mjs ./owners/*.test.mjs ./quality/*.test.mjs ./slack/*.test.mjs ./stale/*.test.mjs ../../scripts/licenses/*.test.mjs ../../scripts/mutation-health/*.test.mjs", "generate-sbom": "FETCH_LICENSE=true cdxgen -t pnpm --no-install-deps --profile license-compliance --spec-version 1.6 -o ../../sbom-source.cdx.json ../../compiled/", "enrich-sbom": "node ../../scripts/licenses/enrich-sbom.mjs ../../sbom-source.cdx.json", "render-licenses-md": "node ../../scripts/licenses/render-licenses-md.mjs ../../sbom-source.cdx.json ../../packages/cli/THIRD_PARTY_LICENSES.md ../../compiled/node_modules", diff --git a/.github/scripts/quality/check-pr-size.mjs b/.github/scripts/quality/check-pr-size.mjs index 2ed56a6f528..03021cbd5f6 100644 --- a/.github/scripts/quality/check-pr-size.mjs +++ b/.github/scripts/quality/check-pr-size.mjs @@ -49,6 +49,22 @@ export const MISC_PATTERNS = [ export const EXCLUDE_PATTERNS = [...TEST_PATTERNS, ...MISC_PATTERNS]; +// Without `dot`, `**` refuses dot segments, so nothing under `.github/` +// would ever match a pattern. +const MATCH_OPTIONS = { dot: true }; + +/** + * Categorize a changed file for line statistics. + * + * @param { string } filename + * @returns { 'testFiles' | 'misc' | 'sourceCode' } + */ +export function categorizeFile(filename) { + if (TEST_PATTERNS.some((pattern) => minimatch(filename, pattern, MATCH_OPTIONS))) return 'testFiles'; + if (MISC_PATTERNS.some((pattern) => minimatch(filename, pattern, MATCH_OPTIONS))) return 'misc'; + return 'sourceCode'; +} + const BOT_MARKER = ''; /** @@ -86,7 +102,7 @@ export async function hasValidOverride(comments, getPermission) { */ export function countFilteredAdditions(files, excludePatterns) { return files - .filter((file) => !excludePatterns.some((pattern) => minimatch(file.filename, pattern))) + .filter((file) => !excludePatterns.some((pattern) => minimatch(file.filename, pattern, MATCH_OPTIONS))) .reduce((sum, file) => sum + file.additions, 0); } diff --git a/.github/scripts/quality/check-pr-size.test.mjs b/.github/scripts/quality/check-pr-size.test.mjs index f5d5d8f3d02..7a109a5b50a 100644 --- a/.github/scripts/quality/check-pr-size.test.mjs +++ b/.github/scripts/quality/check-pr-size.test.mjs @@ -158,6 +158,15 @@ describe('countFilteredAdditions', () => { assert.equal(countFilteredAdditions([], EXCLUDE_PATTERNS), 0); }); + it('excludes test and misc files inside dot-directories', () => { + const files = [ + { filename: '.github/scripts/owners/owners.mjs', additions: 50 }, + { filename: '.github/scripts/owners/owners.test.mjs', additions: 100 }, + { filename: '.github/WORKFLOWS.md', additions: 100 }, + ]; + assert.equal(countFilteredAdditions(files, EXCLUDE_PATTERNS), 50); + }); + it('applies EXCLUDE_PATTERNS to common test file extensions', () => { const files = [ { filename: 'src/service.ts', additions: 50 }, diff --git a/.github/workflows/ci-codeowners-validation.yml b/.github/workflows/ci-codeowners-validation.yml deleted file mode 100644 index 6190f46c2d8..00000000000 --- a/.github/workflows/ci-codeowners-validation.yml +++ /dev/null @@ -1,23 +0,0 @@ -# .github/workflows/ci-codeowners-validation.yml -name: 'CI: Validate CODEOWNERS' - -# Only run when CODEOWNERS or packages change -on: - pull_request: - paths: - - '.github/CODEOWNERS' - - 'packages/**' - -jobs: - validate: - runs-on: ubuntu-latest - steps: - - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 - - - uses: mszostok/codeowners-validator@7f3f5e28c6d7b8dfae5731e54ce2272ca384592f #v0.7.4 - with: - # Start with safe checks only. Add "owners" and - # experimental_checks: "notowned" once the file has settled - # and skip patterns are configured. - checks: 'files,duppatterns,syntax' - github_access_token: '${{ secrets.GITHUB_TOKEN }}' diff --git a/.github/workflows/ci-owners-assign-reviewers.yml b/.github/workflows/ci-owners-assign-reviewers.yml index 73da7ea06fd..5465ca6e656 100644 --- a/.github/workflows/ci-owners-assign-reviewers.yml +++ b/.github/workflows/ci-owners-assign-reviewers.yml @@ -3,7 +3,7 @@ name: 'CI: Owners Assign Reviewers' # Opt-in reviewer auto-assignment. # # When a PR carries the `Auto-assign reviewers` label, requests review from -# every team that owns a changed file (based on .github/OWNERS). Without the +# every team that owns a changed file (based on OWNERS). Without the # label this is a no-op — opting in is entirely up to the PR author. # # Assignment runs once: afterwards the trigger label is swapped for a @@ -60,4 +60,4 @@ jobs: env: GITHUB_TOKEN: ${{ steps.app-token.outputs.token }} PULL_REQUEST_NUMBER: ${{ github.event.pull_request.number }} - run: node .github/scripts/owners-assign-reviewers.mjs + run: node .github/scripts/owners/assign-reviewers.mjs diff --git a/.github/workflows/ci-owners-required-reviews.yml b/.github/workflows/ci-owners-required-reviews.yml new file mode 100644 index 00000000000..6647985e5f4 --- /dev/null +++ b/.github/workflows/ci-owners-required-reviews.yml @@ -0,0 +1,165 @@ +name: 'CI: Owners Required Reviews' +run-name: "${{ github.event_name == 'workflow_dispatch' && format('CI: Owners Required Reviews (PR #{0})', inputs.pr_number) || '' }}" + +# Enforces the `required` entries in OWNERS: when a PR changes a file +# whose winning OWNERS entry carries `required`, a member of the owning team +# must approve the PR before merge. +# +# Replaces the GitHub-native CODEOWNERS enforcement, which could not express +# team ownership metadata beyond "request a review". +# +# Triggers +# - pull_request (opened/synchronize/reopened/ready_for_review): +# re-evaluates whenever the changeset can change. Skipped for fork PRs +# (no secrets): their status stays "expected" — fail closed — until the +# first review event reports it. +# - pull_request_review (submitted/dismissed): re-evaluates when approvals +# change. A privileged event (base repo context, secrets), so it covers +# fork PRs too — and approvals can only change through review events. +# - merge_group: reports success on the queue head without re-evaluating. +# A PR cannot enter the queue unless this status is green on its head, +# and the queue does not change approvals. +# - workflow_dispatch: manual re-check (e.g. after a team membership change, +# or a fork PR push after an approval). +# +# The checkout below always pins the base branch, never the PR merge ref: the +# scripts and the OWNERS file are trusted input, so a PR cannot lift its own +# review requirement or reach the status-writing token. +# +# Output +# - A commit status named "Required Reviews" on the PR head SHA (or +# merge-group head SHA). Add this name to a ruleset's required-checks list +# to gate merges on it. The first job step sets the status to pending, so +# a run that crashes at any later point cannot leave a stale verdict in +# effect. The job itself only fails on errors. + +on: + pull_request: + types: [opened, synchronize, reopened, ready_for_review] + branches: + - master + pull_request_review: + types: [submitted, dismissed] + merge_group: + workflow_dispatch: + inputs: + pr_number: + description: 'Pull request number to re-evaluate' + required: true + type: string + +permissions: + contents: read + +concurrency: + group: >- + owners-required-reviews-${{ github.event.pull_request.number + || github.event.merge_group.head_sha + || inputs.pr_number + || github.ref }} + cancel-in-progress: true + +jobs: + # The queue requires this status on the merge-group head, but there is + # nothing new to evaluate there: entering the queue already required the + # status to be green on the PR head, and queueing does not change approvals. + pass-in-merge-queue: + name: Pass in merge queue + if: github.event_name == 'merge_group' + runs-on: ubuntu-latest + timeout-minutes: 5 + permissions: + statuses: write + steps: + - env: + GH_TOKEN: ${{ github.token }} + SHA: ${{ github.event.merge_group.head_sha }} + run: | + gh api "repos/${GITHUB_REPOSITORY}/statuses/${SHA}" \ + -f state=success \ + -f context='Required Reviews' \ + -f description='Approvals were verified before the PR entered the queue' + + required-reviews: + name: Check required reviews + # Fork PRs are skipped on pull_request (no secrets for the app token); + # pull_request_review covers them. pull_request_review has no `branches` + # filter; the script also skips non-master bases, the guard here keeps + # the early pending status off non-master heads. + if: >- + github.event_name != 'merge_group' && + (github.event_name != 'pull_request' || + github.event.pull_request.head.repo.full_name == github.repository) && + (github.event_name != 'pull_request_review' || + github.event.pull_request.base.ref == 'master') + runs-on: ubuntu-latest + timeout-minutes: 5 + permissions: + contents: read + statuses: write + steps: + # Fail closed before any fallible step: replace a possibly-green status + # with pending, so a failure in token generation, checkout, or setup + # cannot leave a stale verdict in effect. The script sets the verdict. + # Skipped for workflow_dispatch (no SHA in the event); there the script + # sets pending itself. + - name: Set pending status + if: github.event_name != 'workflow_dispatch' + env: + GH_TOKEN: ${{ github.token }} + SHA: ${{ github.event.pull_request.head.sha }} + RUN_URL: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }} + run: | + gh api "repos/${GITHUB_REPOSITORY}/statuses/${SHA}" \ + -f state=pending \ + -f context='Required Reviews' \ + -f description='Evaluating required reviews' \ + -f "target_url=${RUN_URL}" + + - name: Generate App Token + id: app-token + uses: actions/create-github-app-token@29824e69f54612133e76f7eaac726eef6c875baf # v2.2.1 + with: + app-id: ${{ secrets.N8N_ASSISTANT_APP_ID }} + private-key: ${{ secrets.N8N_ASSISTANT_PRIVATE_KEY }} + # Scope the token to what the script needs; without these inputs it + # would inherit every permission of the app installation. + permission-members: read + permission-pull-requests: read + permission-statuses: write + + # Always check out the base branch, never the PR merge ref: the OWNERS + # file and the scripts must be trusted input. The 'master' fallback + # covers workflow_dispatch, so a dispatch from another branch cannot + # run that branch's policy. + - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + with: + ref: ${{ github.event.pull_request.base.ref || 'master' }} + persist-credentials: false + + - name: Setup Node.js + uses: ./.github/actions/setup-nodejs + with: + build-command: '' + install-command: pnpm install --frozen-lockfile --dir ./.github/scripts --ignore-workspace + + - name: Evaluate required reviews + env: + # App token: reading org team membership is beyond GITHUB_TOKEN. + GITHUB_TOKEN: ${{ steps.app-token.outputs.token }} + PULL_REQUEST_NUMBER: ${{ inputs.pr_number || '' }} + STATUS_SHA: ${{ github.event.pull_request.head.sha }} + run: | + # Bootstrap: the checkout is the trusted base branch. Until the + # enforcement script exists there, there is no policy to enforce. + if [ ! -f .github/scripts/owners/required-reviews.mjs ]; then + echo "No enforcement script on the base branch; nothing to enforce." + if [ -n "$STATUS_SHA" ]; then + gh api "repos/${GITHUB_REPOSITORY}/statuses/${STATUS_SHA}" \ + -f state=success \ + -f context='Required Reviews' \ + -f description='Owners enforcement is not on the base branch yet' + fi + exit 0 + fi + node .github/scripts/owners/required-reviews.mjs diff --git a/.github/workflows/ci-owners-review-recommendations.yml b/.github/workflows/ci-owners-review-recommendations.yml index f4770e578bd..7b58f8ab78d 100644 --- a/.github/workflows/ci-owners-review-recommendations.yml +++ b/.github/workflows/ci-owners-review-recommendations.yml @@ -1,10 +1,12 @@ name: 'CI: Owners Review Recommendations' # Posts (or updates) a PR comment with recommended reviewer teams (based on -# file ownership in .github/OWNERS) and a breakdown of changed lines by -# category (source code, test files, misc). +# file ownership in OWNERS), a breakdown of changed lines by category +# (source code, test files, misc), and the required team reviews with a +# prompt to request review from those teams. # -# Advisory only — does not gate merging. +# Advisory only — does not gate merging. Enforcement of required reviews +# lives in ci-owners-required-reviews.yml. on: pull_request: @@ -49,4 +51,4 @@ jobs: env: GITHUB_TOKEN: ${{ steps.app-token.outputs.token }} PULL_REQUEST_NUMBER: ${{ github.event.pull_request.number }} - run: node .github/scripts/owners-review-recommendations.mjs + run: node .github/scripts/owners/review-recommendations.mjs diff --git a/.github/workflows/ci-owners-validation.yml b/.github/workflows/ci-owners-validation.yml new file mode 100644 index 00000000000..e7ecb86512b --- /dev/null +++ b/.github/workflows/ci-owners-validation.yml @@ -0,0 +1,39 @@ +name: 'CI: Validate OWNERS' + +# Validates OWNERS strictly: line syntax (pattern, one team, +# then options), duplicate patterns, and patterns that no longer point at an +# existing file or directory. Runs on OWNERS changes and on changes to the +# owned paths, so a move or delete that orphans an OWNERS entry fails here. +# +# Team names are not checked here: that needs an org read token, which fork +# PRs do not have. A separate workflow covers it (see DEVP-891). + +# The paths cover every top-level path that OWNERS entries point at, so a +# move or delete of an owned path cannot slip through unvalidated. +on: + pull_request: + paths: + - '.github/**' + - 'assets/**' + - 'docker/**' + - 'packages/**' + - 'patches/**' + - 'scripts/**' + - 'security/**' + - '.code-health-baseline.json' + - 'OWNERS' + +permissions: + contents: read + +jobs: + validate: + runs-on: ubuntu-latest + timeout-minutes: 5 + steps: + - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + with: + persist-credentials: false + + # owners.mjs only uses node built-ins, so no dependency install is needed. + - run: node .github/scripts/owners/owners.mjs --check diff --git a/.github/workflows/ci-pull-requests.yml b/.github/workflows/ci-pull-requests.yml index 5f3bf6d290d..13166e5d235 100644 --- a/.github/workflows/ci-pull-requests.yml +++ b/.github/workflows/ci-pull-requests.yml @@ -115,6 +115,7 @@ jobs: workflows: .github/** workflow-scripts: .github/scripts/** + OWNERS scripts/licenses/** scripts/mutation-health/** cubic.yaml diff --git a/.github/OWNERS b/OWNERS similarity index 91% rename from .github/OWNERS rename to OWNERS index 9162377cd56..f586cf47781 100644 --- a/.github/OWNERS +++ b/OWNERS @@ -1,6 +1,20 @@ -# n8n CODEOWNERS +# n8n OWNERS +# +# Format: <@org/team> [required] +# +# Examples: +# packages/foo/ @n8n-io/team-a advisory reviewer recommendation +# packages/foo/bar.ts @n8n-io/team-b required team approval blocks merge # # Last-match-wins: specific rules MUST come AFTER general rules. +# +# The `required` option makes team approval mandatory: a member of the team +# must approve the PR before merge. This is enforced by the +# "Required Reviews" commit status (ci-owners-required-reviews.yml). +# All other entries are advisory (reviewer recommendations and opt-in +# reviewer auto-assignment). +# +# Validated by ci-owners-validation.yml (`node .github/scripts/owners/owners.mjs --check`). # Default catch-all (ensures every file gets at least one reviewer) * @n8n-io/catalysts @@ -9,7 +23,7 @@ packages/core/ @n8n-io/catalysts packages/workflow/ @n8n-io/catalysts -packages/workflow/styker.config.mjs @n8n-io/qa-dx +packages/workflow/stryker.config.mjs @n8n-io/qa-dx packages/@n8n/config/ @n8n-io/catalysts packages/@n8n/backend-common/ @n8n-io/catalysts packages/@n8n/backend-network/ @n8n-io/catalysts @@ -30,13 +44,15 @@ packages/@n8n/extension-sdk/ @n8n-io/catalysts packages/@n8n/eslint-config/ @n8n-io/qa-dx packages/@n8n/typescript-config/ @n8n-io/qa-dx -packages/@n8n/db/src/migrations/ @n8n-io/migrations-review +packages/@n8n/db/src/migrations/ @n8n-io/migrations-review required # Top-level paths scripts/ @n8n-io/qa-dx patches/ @n8n-io/qa-dx assets/ @n8n-io/adore security/ @n8n-io/qa-dx +.code-health-baseline.json @n8n-io/qa-dx required +OWNERS @n8n-io/qa-dx required # @n8n/cli packages/@n8n/cli/ @n8n-io/adore @@ -126,7 +142,6 @@ packages/cli/src/controllers/cta.controller.ts @n8n-io/adore packages/cli/src/controllers/folder.controller.ts @n8n-io/adore packages/cli/src/controllers/tags.controller.ts @n8n-io/adore packages/cli/src/controllers/binary-data.controller.ts @n8n-io/adore -packages/cli/src/controllers/dynamic-templates.controller.ts @n8n-io/adore packages/cli/src/controllers/posthog.controller.ts @n8n-io/adore packages/cli/src/controllers/translation.controller.ts @n8n-io/adore packages/cli/src/controllers/project.controller.ts @n8n-io/ligo @@ -143,7 +158,6 @@ packages/cli/src/services/role-cache.service.ts @n8n-io/iam packages/cli/src/services/password.utility.ts @n8n-io/iam packages/cli/src/services/public-api-key.service.ts @n8n-io/iam packages/cli/src/services/security-settings.service.ts @n8n-io/iam -packages/cli/src/services/ssrf/ @n8n-io/catalysts packages/cli/src/services/static-auth-service.ts @n8n-io/iam packages/cli/src/services/access.service.ts @n8n-io/iam packages/cli/src/services/ai.service.ts @n8n-io/ai @@ -153,7 +167,6 @@ packages/cli/src/services/annotation-tag.service.ee.ts @n8n-io/ai packages/cli/src/services/folder.service.ts @n8n-io/adore packages/cli/src/services/tag.service.ts @n8n-io/adore packages/cli/src/services/cta.service.ts @n8n-io/adore -packages/cli/src/services/dynamic-templates.service.ts @n8n-io/adore packages/cli/src/services/frontend.service.ts @n8n-io/adore packages/cli/src/services/banner.service.ts @n8n-io/adore packages/cli/src/services/project.service.ee.ts @n8n-io/ligo @@ -169,7 +182,6 @@ packages/frontend/editor-ui/ @n8n-io/frontend packages/frontend/editor-ui/src/features/ai/ @n8n-io/ai packages/frontend/editor-ui/src/features/credentials/ @n8n-io/iam packages/frontend/editor-ui/src/features/execution/ @n8n-io/ligo -packages/frontend/editor-ui/src/features/project-roles/ @n8n-io/iam packages/frontend/editor-ui/src/features/integrations/ @n8n-io/ligo packages/frontend/@n8n/design-system/ @n8n-io/design @@ -225,7 +237,12 @@ packages/extensions/insights/ @n8n-io/ligo # CI/CD .github/ @n8n-io/qa-dx +.github/workflows/ @n8n-io/qa-dx required +.github/scripts/ @n8n-io/qa-dx required +.github/actions/ @n8n-io/qa-dx required +.github/poutine-rules/ @n8n-io/qa-dx required docker/ @n8n-io/qa-dx +docker/get-n8n.sh @n8n-io/qa-dx required # QA