From d74dc0f4e527bbf740caaaa83e71003236d29b52 Mon Sep 17 00:00:00 2001 From: Matsu Date: Tue, 19 May 2026 14:17:59 +0300 Subject: [PATCH] ci: Add reviewer recommendations based on CODEOWNERS (#30690) Co-authored-by: Claude Opus 4.7 (1M context) --- .github/scripts/github-helpers.mjs | 22 +++ .../scripts/owners-review-recommendations.mjs | 104 +++++++++++ .../owners-review-recommendations.test.mjs | 77 ++++++++ .github/scripts/owners.mjs | 160 ++++++++++++++++ .github/scripts/owners.test.mjs | 173 ++++++++++++++++++ .../ci-owners-review-recommendations.yml | 51 ++++++ 6 files changed, 587 insertions(+) create mode 100644 .github/scripts/owners-review-recommendations.mjs create mode 100644 .github/scripts/owners-review-recommendations.test.mjs create mode 100644 .github/scripts/owners.mjs create mode 100644 .github/scripts/owners.test.mjs create mode 100644 .github/workflows/ci-owners-review-recommendations.yml diff --git a/.github/scripts/github-helpers.mjs b/.github/scripts/github-helpers.mjs index 7a2d1f14a5d..e1931bd8392 100644 --- a/.github/scripts/github-helpers.mjs +++ b/.github/scripts/github-helpers.mjs @@ -351,6 +351,28 @@ export async function getPullRequestById(pullRequestId) { return pullRequest.data; } +/** + * Returns the set of files changed in a PR, including previous filenames for renames. + * + * @param { number } pullRequestNumber + * @returns { Promise> } + * */ +export async function getChangedFiles(pullRequestNumber) { + const { octokit, owner, repo } = initGithub(); + + const files = await octokit.paginate(octokit.rest.pulls.listFiles, { + owner, + repo, + pull_number: pullRequestNumber, + per_page: 100, + }); + + return new Set([ + ...files.map((file) => file.filename), + ...files.map((file) => file.previous_filename).filter((filename) => filename !== undefined), + ]); +} + /** * @param {string} tag */ diff --git a/.github/scripts/owners-review-recommendations.mjs b/.github/scripts/owners-review-recommendations.mjs new file mode 100644 index 00000000000..dfdc1e0732f --- /dev/null +++ b/.github/scripts/owners-review-recommendations.mjs @@ -0,0 +1,104 @@ +import { ensureEnvVar, getChangedFiles, initGithub } from "./github-helpers.mjs"; +import { assignOwnership, ownershipsToAllocations, parseOwnersFile } from "./owners.mjs"; + +/** @typedef {import('./owners.mjs').Allocation} Allocation */ + +/** + * @param { number } pullRequestNumber + * */ +export async function getReviewRecommendations(pullRequestNumber) { + const changedFiles = await getChangedFiles(pullRequestNumber); + const owners = parseOwnersFile(); + + const ownerships = assignOwnership(changedFiles, owners); + const allocations = ownershipsToAllocations(ownerships); + + const topAllocations = allocations.sort((a, b) => b.fileCount - a.fileCount).slice(0, 3); + + await commentOnPrWithRecommendations(pullRequestNumber, topAllocations, changedFiles); +} + +const BOT_MARKER = ""; + +/** + * Build the PR comment body listing the top reviewer teams with their share + * of ownership over the changed files. + * + * @param { Allocation[] } allocations + * @param { Set } changedFiles + * @returns { string } + * */ +export function buildRecommendationsBody(allocations, changedFiles) { + const total = changedFiles.size; + + if (allocations.length === 0 || total === 0) { + return [ + BOT_MARKER, + "## Recommended reviewers", + "", + "_No owning teams matched the files changed in this PR._", + ].join("\n"); + } + + const rows = allocations.map(({ team, fileCount }) => { + const pct = Math.round((fileCount / total) * 100); + return `| ${team} | ${fileCount} | ${pct}% |`; + }); + + return [ + BOT_MARKER, + "## Recommended reviewers", + "", + `Based on ownership of the ${total} changed file${total === 1 ? "" : "s"} in this PR:`, + "", + "| Team | Files owned | Share |", + "| --- | ---: | ---: |", + ...rows, + ].join("\n"); +} + +/** + * Post the recommendations as a PR comment, or update the existing one if a + * previous run already left one (identified by BOT_MARKER). + * + * @param { number } pullRequestNumber + * @param { Allocation[] } allocations + * @param { Set } changedFiles + * */ +export async function commentOnPrWithRecommendations(pullRequestNumber, allocations, changedFiles) { + const { octokit, owner, repo } = initGithub(); + + const body = buildRecommendationsBody(allocations, changedFiles); + + const comments = await octokit.paginate(octokit.rest.issues.listComments, { + owner, + repo, + issue_number: pullRequestNumber, + per_page: 100, + }); + + const existing = comments.find(c => c.body?.includes(BOT_MARKER)); + + if (existing) { + await octokit.rest.issues.updateComment({ + owner, + repo, + comment_id: existing.id, + body, + }); + } else { + await octokit.rest.issues.createComment({ + owner, + repo, + issue_number: pullRequestNumber, + body, + }); + } +} + +// only run when executed directly, not when imported by tests +if (import.meta.url === `file://${process.argv[1]}`) { + const pullRequestNumber = parseInt(ensureEnvVar("PULL_REQUEST_NUMBER")); + + await getReviewRecommendations(pullRequestNumber); +} diff --git a/.github/scripts/owners-review-recommendations.test.mjs b/.github/scripts/owners-review-recommendations.test.mjs new file mode 100644 index 00000000000..d8d304a9882 --- /dev/null +++ b/.github/scripts/owners-review-recommendations.test.mjs @@ -0,0 +1,77 @@ +import { 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-review-recommendations.test.mjs + * */ + +// mock.module must be called before the module under test is imported, +// because static imports are hoisted and resolve before any code runs. +mock.module('./github-helpers.mjs', { + namedExports: { + ensureEnvVar: () => {}, // no-op in tests + initGithub: () => {}, // no-op in tests + getChangedFiles: () => Promise.resolve(new Set()), // no-op in tests + }, +}); + +/** @type { (allocations: { team: string, fileCount: number }[], changedFiles: Set) => string } */ +let buildRecommendationsBody; +const { buildRecommendationsBody: imported } = await import('./owners-review-recommendations.mjs'); +buildRecommendationsBody = imported; + +describe('buildRecommendationsBody', () => { + const marker = ''; + + it('returns the fallback message when there are no allocations', () => { + const body = buildRecommendationsBody([], new Set(['a.ts'])); + + assert.ok(body.startsWith(marker), 'body must start with the bot marker'); + assert.match(body, /No owning teams matched/); + }); + + it('returns the fallback message when no files changed', () => { + const body = buildRecommendationsBody( + [{ team: '@n8n-io/cli-team', fileCount: 0 }], + new Set(), + ); + + assert.match(body, /No owning teams matched/); + }); + + it('renders a table with team name, file count, and rounded percentage', () => { + const body = buildRecommendationsBody( + [ + { team: '@n8n-io/cli-team', fileCount: 6 }, + { team: '@n8n-io/catalysts', fileCount: 3 }, + { team: '@n8n-io/ai-team', fileCount: 1 }, + ], + new Set(['1', '2', '3', '4', '5', '6', '7', '8', '9', '10']), + ); + + assert.ok(body.startsWith(marker)); + assert.match(body, /\| @n8n-io\/cli-team \| 6 \| 60% \|/); + assert.match(body, /\| @n8n-io\/catalysts \| 3 \| 30% \|/); + assert.match(body, /\| @n8n-io\/ai-team \| 1 \| 10% \|/); + }); + + it('uses singular "file" when exactly one file changed', () => { + const body = buildRecommendationsBody( + [{ team: '@n8n-io/cli-team', fileCount: 1 }], + new Set(['only.ts']), + ); + + assert.match(body, /ownership of the 1 changed file in this PR/); + }); + + it('uses plural "files" for more than one changed file', () => { + const body = buildRecommendationsBody( + [{ team: '@n8n-io/cli-team', fileCount: 2 }], + new Set(['a.ts', 'b.ts']), + ); + + assert.match(body, /ownership of the 2 changed files in this PR/); + }); +}); diff --git a/.github/scripts/owners.mjs b/.github/scripts/owners.mjs new file mode 100644 index 00000000000..457da7c3e1f --- /dev/null +++ b/.github/scripts/owners.mjs @@ -0,0 +1,160 @@ +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 new file mode 100644 index 00000000000..39078e4deae --- /dev/null +++ b/.github/scripts/owners.test.mjs @@ -0,0 +1,173 @@ +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/workflows/ci-owners-review-recommendations.yml b/.github/workflows/ci-owners-review-recommendations.yml new file mode 100644 index 00000000000..f5ca25cfef8 --- /dev/null +++ b/.github/workflows/ci-owners-review-recommendations.yml @@ -0,0 +1,51 @@ +name: 'CI: Owners Review Recommendations' + +# Posts (or updates) a PR comment recommending which @n8n-io teams should +# review the PR, based on file ownership defined in .github/OWNERS. +# +# Advisory only — does not gate merging. + +on: + pull_request: + types: + - opened + - synchronize + - reopened + branches: + - master + +jobs: + recommend-reviewers: + name: Recommend reviewers + # Skipped for: + # - PRs from forks (no token to comment with) + # - Bot-authored PRs (Dependabot, Renovate, etc. — noise, not useful) + if: | + github.event.pull_request.head.repo.full_name == github.repository && + github.event.pull_request.user.type != 'Bot' + runs-on: ubuntu-latest + timeout-minutes: 5 + permissions: + contents: read + pull-requests: write + steps: + - 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 }} + + - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + + - name: Setup Node.js + uses: ./.github/actions/setup-nodejs + with: + build-command: '' + install-command: pnpm install --frozen-lockfile --dir ./.github/scripts --ignore-workspace + + - name: Post review recommendations + env: + GITHUB_TOKEN: ${{ steps.app-token.outputs.token }} + PULL_REQUEST_NUMBER: ${{ github.event.pull_request.number }} + run: node .github/scripts/owners-review-recommendations.mjs