From 3aa1ab7d0040ef374afaddcb58f2d134b6ae2d18 Mon Sep 17 00:00:00 2001 From: simonfaltum Date: Tue, 16 Jun 2026 12:06:08 +0200 Subject: [PATCH] Validate OWNERS/OWNERTEAMS consistency in CI The maintainer-approval gate trusts .github/OWNERS and .github/OWNERTEAMS, but nothing checked the two files for internal consistency. Onboarding a team is a hand-edit of both with no safety net, so a typo'd path or an undefined team alias slips through silently (an undefined team: resolves to zero owners and quietly falls back to maintainer-only approval). Add a 'node .github/scripts/owners.js validate' mode and run it in CI. Errors (block merge): a rule references a team not defined in OWNERTEAMS, a rule resolves to zero owners, or a rule maps a path that does not exist. Warnings (non-blocking): a defined team has no GitHub team-page URL. Remove the dead /acceptance/labs/ OWNERS rule (its directory does not exist), and run the validator on a full checkout so path existence can be verified. Co-authored-by: Isaac --- .github/OWNERS | 1 - .github/scripts/owners.js | 135 ++++++++++++++++- .github/scripts/owners.test.js | 167 ++++++++++++++++++++++ .github/workflows/test-owners-scripts.yml | 11 +- 4 files changed, 307 insertions(+), 7 deletions(-) diff --git a/.github/OWNERS b/.github/OWNERS index ad3ec69e0ad..6d60d50d3b6 100644 --- a/.github/OWNERS +++ b/.github/OWNERS @@ -13,7 +13,6 @@ # Labs /cmd/labs/ @alexott @asnare -/acceptance/labs/ @alexott @asnare # Apps /cmd/apps/ team:eng-apps-devex diff --git a/.github/scripts/owners.js b/.github/scripts/owners.js index a158fa4af5a..158b872ef2b 100644 --- a/.github/scripts/owners.js +++ b/.github/scripts/owners.js @@ -137,4 +137,137 @@ function getOwnershipGroups(filenames, rules) { return groups; } -module.exports = { parseOwnerTeams, parseOwnersFile, ownersMatch, findOwners, getMaintainers, getOwnershipGroups }; +/** + * Parse OWNERS into raw rules WITHOUT expanding team aliases. + * Unlike parseOwnersFile, this keeps the original tokens ("team:x", "@user") + * so the validator can tell defined from undefined teams and count owners. + * + * @param {string} filePath - absolute path to the OWNERS file + * @returns {Array<{pattern: string, tokens: string[]}>} + */ +function parseOwnersRules(filePath) { + return readDataLines(filePath).map((parts) => ({ + pattern: parts[0], + tokens: parts.slice(1), + })); +} + +/** + * Parse the GitHub team-page URLs declared in the OWNERTEAMS header comment. + * Format: "# : https://github.com/orgs/databricks/teams/" + * Returns the set of "team:" aliases that have a documented page. + * + * @param {string} filePath - absolute path to the OWNERTEAMS file + * @returns {Set} + */ +function parseTeamPageUrls(filePath) { + let content; + try { + content = fs.readFileSync(filePath, "utf-8"); + } catch (e) { + if (e.code === "ENOENT") return new Set(); + throw e; + } + const pages = new Set(); + for (const raw of content.split("\n")) { + const m = raw.match(/^#\s*([a-z0-9-]+):\s*https?:\/\//); + if (m) pages.add("team:" + m[1]); + } + return pages; +} + +/** + * Validate OWNERS and OWNERTEAMS for internal consistency. + * + * Errors (block CI): + * - a rule references a "team:" alias not defined in OWNERTEAMS + * - a rule resolves to zero owners (only a maintainer could ever approve it) + * - a rule maps a path that does not exist in the repository + * + * Warnings (reported, non-blocking): a defined team has no team-page URL in the + * OWNERTEAMS header. A team may legitimately predate its GitHub team page, so + * this never blocks a merge. + * + * fileExists is injectable so tests can validate synthetic rules without a tree. + * + * @param {string} ownersPath + * @param {string} teamsPath + * @param {{ repoRoot?: string, fileExists?: (p: string) => boolean }} [opts] + * @returns {{ errors: string[], warnings: string[] }} + */ +function validateOwners(ownersPath, teamsPath, opts) { + const repoRoot = (opts && opts.repoRoot) || process.cwd(); + const fileExists = (opts && opts.fileExists) || ((p) => fs.existsSync(p)); + const teams = parseOwnerTeams(teamsPath); + const pageUrls = parseTeamPageUrls(teamsPath); + const errors = []; + const warnings = []; + + for (const { pattern, tokens } of parseOwnersRules(ownersPath)) { + let ownerCount = 0; + let undefinedTeam = false; + for (const token of tokens) { + if (token.startsWith("team:")) { + if (teams.has(token)) { + ownerCount += teams.get(token).length; + } else { + errors.push(`rule "${pattern}" references undefined team "${token}"; define it in .github/OWNERTEAMS`); + undefinedTeam = true; + } + } else if (token.startsWith("@")) { + ownerCount += 1; + } + } + // Skip the zero-owner error when an undefined team already explains it. + if (ownerCount === 0 && !undefinedTeam) { + errors.push(`rule "${pattern}" resolves to zero owners`); + } + if (pattern !== "*") { + const rel = pattern.replace(/^\//, "").replace(/\/$/, ""); + if (rel && !fileExists(path.join(repoRoot, rel))) { + errors.push(`rule "${pattern}" maps a path that does not exist in the repo`); + } + } + } + + for (const team of teams.keys()) { + if (!pageUrls.has(team)) { + warnings.push(`team "${team}" has no GitHub team-page URL in the .github/OWNERTEAMS header comment`); + } + } + + return { errors, warnings }; +} + +module.exports = { + parseOwnerTeams, + parseOwnersFile, + parseOwnersRules, + parseTeamPageUrls, + ownersMatch, + findOwners, + getMaintainers, + getOwnershipGroups, + validateOwners, +}; + +// CLI entrypoint: `node .github/scripts/owners.js validate` +if (require.main === module) { + if (process.argv[2] !== "validate") { + console.error("usage: node .github/scripts/owners.js validate"); + process.exit(2); + } + const root = process.cwd(); + const { errors, warnings } = validateOwners( + path.join(root, ".github", "OWNERS"), + path.join(root, ".github", "OWNERTEAMS"), + { repoRoot: root }, + ); + for (const w of warnings) console.warn(`warning: ${w}`); + for (const e of errors) console.error(`error: ${e}`); + if (errors.length > 0) { + console.error(`OWNERS validation failed: ${errors.length} error(s), ${warnings.length} warning(s)`); + process.exit(1); + } + console.log(`OWNERS validation passed: ${warnings.length} warning(s)`); +} diff --git a/.github/scripts/owners.test.js b/.github/scripts/owners.test.js index 5594d297505..ccfc942cf57 100644 --- a/.github/scripts/owners.test.js +++ b/.github/scripts/owners.test.js @@ -8,9 +8,12 @@ const { parseOwnerTeams, ownersMatch, parseOwnersFile, + parseOwnersRules, + parseTeamPageUrls, findOwners, getMaintainers, getOwnershipGroups, + validateOwners, } = require("./owners"); // --- ownersMatch --- @@ -348,3 +351,167 @@ describe("getOwnershipGroups", () => { assert.ok(groups.has("*")); }); }); + +// --- parseOwnersRules --- + +describe("parseOwnersRules", () => { + let tmpDir; + let ownersPath; + + before(() => { + tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "owners-raw-")); + ownersPath = path.join(tmpDir, "OWNERS"); + }); + + after(() => { + fs.rmSync(tmpDir, { recursive: true }); + }); + + it("keeps team aliases and @users un-expanded", () => { + fs.writeFileSync(ownersPath, "/cmd/auth/ team:platform @carol\n* @alice\n"); + const rules = parseOwnersRules(ownersPath); + assert.equal(rules.length, 2); + assert.deepEqual(rules[0], { + pattern: "/cmd/auth/", + tokens: ["team:platform", "@carol"], + }); + assert.deepEqual(rules[1], { pattern: "*", tokens: ["@alice"] }); + }); +}); + +// --- parseTeamPageUrls --- + +describe("parseTeamPageUrls", () => { + let tmpDir; + let teamsPath; + + before(() => { + tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "team-urls-")); + teamsPath = path.join(tmpDir, "OWNERTEAMS"); + }); + + after(() => { + fs.rmSync(tmpDir, { recursive: true }); + }); + + it("collects team-page URLs from header comments", () => { + fs.writeFileSync( + teamsPath, + [ + "# GitHub team pages:", + "# platform: https://github.com/orgs/databricks/teams/cli-platform", + "# bundle: https://github.com/orgs/databricks/teams/cli-maintainers", + "team:platform @alice", + ].join("\n") + ); + const pages = parseTeamPageUrls(teamsPath); + assert.equal(pages.size, 2); + assert.ok(pages.has("team:platform")); + assert.ok(pages.has("team:bundle")); + }); + + it("ignores header lines that are not team-page URLs", () => { + fs.writeFileSync( + teamsPath, + [ + '# Use "team:" in OWNERS to reference a team.', + "# Format: team: @member1", + "team:platform @alice", + ].join("\n") + ); + assert.equal(parseTeamPageUrls(teamsPath).size, 0); + }); +}); + +// --- validateOwners --- + +describe("validateOwners", () => { + let tmpDir; + let ownersPath; + let teamsPath; + const allExist = () => true; + + before(() => { + tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "owners-validate-")); + ownersPath = path.join(tmpDir, "OWNERS"); + teamsPath = path.join(tmpDir, "OWNERTEAMS"); + }); + + after(() => { + fs.rmSync(tmpDir, { recursive: true }); + }); + + function write(owners, teams) { + fs.writeFileSync(ownersPath, owners); + fs.writeFileSync(teamsPath, teams); + } + + it("passes a consistent OWNERS/OWNERTEAMS pair", () => { + write( + "* @alice\n/cmd/auth/ team:platform\n", + "# platform: https://github.com/orgs/databricks/teams/cli-platform\nteam:platform @bob @carol\n" + ); + const { errors, warnings } = validateOwners(ownersPath, teamsPath, { + fileExists: allExist, + }); + assert.deepEqual(errors, []); + assert.deepEqual(warnings, []); + }); + + it("errors on a team alias not defined in OWNERTEAMS", () => { + write("* @alice\n/cmd/auth/ team:platfrom\n", "team:platform @bob\n"); + const { errors } = validateOwners(ownersPath, teamsPath, { + fileExists: allExist, + }); + assert.equal(errors.length, 1); + assert.match(errors[0], /undefined team "team:platfrom"/); + }); + + it("does not also report zero owners when a team is undefined", () => { + write("/cmd/auth/ team:nope\n", "team:platform @bob\n"); + const { errors } = validateOwners(ownersPath, teamsPath, { + fileExists: allExist, + }); + assert.equal(errors.length, 1); + assert.match(errors[0], /undefined team/); + }); + + it("errors when a rule resolves to zero owners (org team missing @)", () => { + write("/cmd/auth/ databricks/eng-apps-devex\n", "team:platform @bob\n"); + const { errors } = validateOwners(ownersPath, teamsPath, { + fileExists: allExist, + }); + assert.equal(errors.length, 1); + assert.match(errors[0], /zero owners/); + }); + + it("errors when a rule maps a path that does not exist", () => { + write( + "* @alice\n/acceptance/ghost/ team:platform\n", + "# platform: https://github.com/orgs/databricks/teams/cli-platform\nteam:platform @bob\n" + ); + const { errors } = validateOwners(ownersPath, teamsPath, { + fileExists: (p) => !p.includes("ghost"), + }); + assert.equal(errors.length, 1); + assert.match(errors[0], /does not exist/); + }); + + it("does not path-check the * catch-all rule", () => { + write("* @alice\n", "team:platform @bob\n"); + const { errors } = validateOwners(ownersPath, teamsPath, { + fileExists: () => false, + }); + assert.deepEqual(errors, []); + }); + + it("warns (does not error) when a defined team has no team-page URL", () => { + write("* @alice\n/cmd/auth/ team:newteam\n", "team:newteam @bob\n"); + const { errors, warnings } = validateOwners(ownersPath, teamsPath, { + fileExists: allExist, + }); + assert.deepEqual(errors, []); + assert.equal(warnings.length, 1); + assert.match(warnings[0], /team-page URL/); + }); +}); diff --git a/.github/workflows/test-owners-scripts.yml b/.github/workflows/test-owners-scripts.yml index 14acbb6df0d..9538eb5178b 100644 --- a/.github/workflows/test-owners-scripts.yml +++ b/.github/workflows/test-owners-scripts.yml @@ -6,7 +6,9 @@ on: - '.github/scripts/**' - '.github/workflows/maintainer-approval.js' - '.github/workflows/maintainer-approval.test.js' + - '.github/workflows/test-owners-scripts.yml' - '.github/OWNERS' + - '.github/OWNERTEAMS' jobs: test: @@ -15,11 +17,10 @@ jobs: labels: ubuntu-latest-deco timeout-minutes: 5 steps: + # Full checkout (no sparse-checkout): `owners.js validate` verifies that + # every OWNERS path exists in the tree, so it needs the whole repo. - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 - with: - sparse-checkout: | - .github/scripts - .github/workflows - .github/OWNERS - name: Run OWNERS script tests run: node --test .github/scripts/owners.test.js .github/workflows/maintainer-approval.test.js + - name: Validate OWNERS and OWNERTEAMS + run: node .github/scripts/owners.js validate