From 73af78f8fd63f9adbab506b4857453b6a49acfef Mon Sep 17 00:00:00 2001 From: Wibias <37517432+Wibias@users.noreply.github.com> Date: Sun, 9 Aug 2026 05:14:32 +0200 Subject: [PATCH 1/2] fix(ci): keep Ready blocked while deterministic hygiene fails Stop the PR quality gate from marking review-ready when hygiene still fails (as on #1324), and re-run the gate when sponsorship or exception labels change. --- .github/scripts/enforce-pr-target.test.cjs | 7 +- .github/scripts/pr-hygiene.cjs | 59 +++++++++++++++ .github/scripts/pr-hygiene.test.cjs | 35 +++++++++ .github/workflows/enforce-pr-target.yml | 45 +++++++++++- .github/workflows/pr-hygiene.yml | 36 ++++------ tests/ci-workflows.test.ts | 83 +++++++++++++++++++++- tests/helpers/enforce-pr-target-harness.ts | 28 ++++++++ 7 files changed, 266 insertions(+), 27 deletions(-) diff --git a/.github/scripts/enforce-pr-target.test.cjs b/.github/scripts/enforce-pr-target.test.cjs index 33bfda42c..bf5497d7f 100644 --- a/.github/scripts/enforce-pr-target.test.cjs +++ b/.github/scripts/enforce-pr-target.test.cjs @@ -49,7 +49,7 @@ describe("enforce-pr-target workflow", () => { assert.match(workflow, /synchronize/); }); - it("uses label events for GUI waivers and a trusted CodeRabbit status signal", () => { + it("uses label events for GUI waivers, hygiene sponsorship, and a trusted CodeRabbit status signal", () => { assert.doesNotMatch(workflow, /^ issue_comment:/m); assert.match(workflow, /- labeled/); assert.match(workflow, /- unlabeled/); @@ -57,6 +57,8 @@ describe("enforce-pr-target workflow", () => { assert.match(workflow, /github\.event\.context == 'CodeRabbit'/); assert.match(workflow, /github\.event\.state == 'success'/); assert.match(workflow, /github\.event\.label\.name == 'gui-screenshot-waived'/); + assert.match(workflow, /github\.event\.label\.name == 'intake: hygiene-blocked'/); + assert.match(workflow, /github\.event\.label\.name == 'maintainer-sponsored'/); assert.match(workflow, /listPullRequestsAssociatedWithCommit/); assert.match(workflow, /candidate\.head\?\.sha === statusSha/); assert.match(workflow, /candidates\.length !== 1/); @@ -180,6 +182,9 @@ describe("enforce-pr-target workflow", () => { it("loads pr-quality via require from the checked-out scripts", () => { assert.match(workflow, /pr-quality\.cjs/); assert.match(workflow, /collectPrQualityFailures/); + assert.match(workflow, /pr-hygiene\.cjs/); + assert.match(workflow, /collectDeterministicHygieneFailures/); + assert.match(workflow, /pulls\.listFiles/); }); it("checks stacked bases via open PR heads before wrong_base enforcement", () => { diff --git a/.github/scripts/pr-hygiene.cjs b/.github/scripts/pr-hygiene.cjs index a910be674..e7706de8b 100644 --- a/.github/scripts/pr-hygiene.cjs +++ b/.github/scripts/pr-hygiene.cjs @@ -1,5 +1,7 @@ "use strict"; +const { assessSponsoredSurface } = require("./pr-sponsored-surface.cjs"); + const GENERATED_PREFIXES = [ "gui/dist/", "dist/", @@ -218,9 +220,66 @@ function assessHygiene({ files = [], labels = [] }) { return failures; } +/** + * Human-readable one-liners for each deterministic hygiene failure code. + * Shared by the hygiene workflow comment and the PR quality gate actions. + */ +const HYGIENE_FAILURE_HINTS = { + missing_regression_test: + "Behavior changed under `src/` or `gui/src/` without a test change. Add focused coverage or obtain `test-exception-approved`.", + generated_output: + "Generated build output is committed. Remove it or obtain `generated-change-approved`.", + orphan_lockfile: + "`bun.lock` changed without `package.json`. Revert accidental churn or obtain `dependency-change-approved`.", + new_suppression: + "A new TypeScript, lint, formatter, or similar suppression was added. Fix the underlying issue or obtain `suppression-approved`.", + focused_or_skipped_test: + "A focused or skipped test was added. Restore the complete suite or obtain `test-exception-approved`.", + empty_catch: + "An empty catch block was added. Handle, report, or deliberately propagate the error.", + unsponsored_surface: + "This changes an authentication, workflow, release-automation, or dependency surface. `MAINTAINERS.md` requires security review for these; ask a maintainer to apply `maintainer-sponsored` once they have reviewed it.", +}; + +/** + * Labels that can clear or reinstate a hygiene failure. The quality gate must + * wake on these so READY / DRAFT tracks sponsorship and exception approvals + * without waiting for an unrelated synchronize. + */ +const HYGIENE_GATE_LABELS = [ + "intake: hygiene-blocked", + "maintainer-sponsored", + "test-exception-approved", + "suppression-approved", + "generated-change-approved", + "dependency-change-approved", +]; + +/** + * Combine patch-hygiene and sponsored-surface failures into one list so the + * hygiene workflow and the PR quality gate cannot disagree about Ready. + */ +function collectDeterministicHygieneFailures({ + files = [], + labels = [], + authorHasPushPermission = false, +}) { + return [ + ...assessHygiene({ files, labels }), + ...assessSponsoredSurface({ + authorHasPushPermission, + changedFiles: files.map((file) => file.filename), + labels, + }), + ]; +} + module.exports = { addedLines, assessHygiene, + collectDeterministicHygieneFailures, + HYGIENE_FAILURE_HINTS, + HYGIENE_GATE_LABELS, hasEmptyCatch, hasDeletions, isBehaviorPath, diff --git a/.github/scripts/pr-hygiene.test.cjs b/.github/scripts/pr-hygiene.test.cjs index 16f5f79a3..44395ea25 100644 --- a/.github/scripts/pr-hygiene.test.cjs +++ b/.github/scripts/pr-hygiene.test.cjs @@ -5,6 +5,9 @@ const assert = require("node:assert/strict"); const { addedLines, assessHygiene, + collectDeterministicHygieneFailures, + HYGIENE_FAILURE_HINTS, + HYGIENE_GATE_LABELS, hasEmptyCatch, resultLines, resultLinesByHunk, @@ -203,3 +206,35 @@ describe("assessHygiene", () => { assert.deepEqual(failures, []); }); }); + +describe("collectDeterministicHygieneFailures", () => { + it("combines patch hygiene and sponsored-surface failures", () => { + const failures = collectDeterministicHygieneFailures({ + files: [ + { filename: "src/codex/auth-api.ts", patch: "+change" }, + ], + authorHasPushPermission: false, + }); + assert.deepEqual( + failures.map((failure) => failure.code).sort(), + ["missing_regression_test", "unsponsored_surface"], + ); + }); + + it("skips sponsorship for maintainers with push permission", () => { + const failures = collectDeterministicHygieneFailures({ + files: [ + { filename: "src/codex/auth-api.ts", patch: "+change" }, + { filename: "tests/codex-auth-api.test.ts", patch: "+test" }, + ], + authorHasPushPermission: true, + }); + assert.deepEqual(failures, []); + }); + + it("exposes hints and gate labels for the Ready coupling", () => { + assert.equal(typeof HYGIENE_FAILURE_HINTS.unsponsored_surface, "string"); + assert.ok(HYGIENE_GATE_LABELS.includes("maintainer-sponsored")); + assert.ok(HYGIENE_GATE_LABELS.includes("intake: hygiene-blocked")); + }); +}); diff --git a/.github/workflows/enforce-pr-target.yml b/.github/workflows/enforce-pr-target.yml index e032e6cca..674c314bb 100644 --- a/.github/workflows/enforce-pr-target.yml +++ b/.github/workflows/enforce-pr-target.yml @@ -38,7 +38,13 @@ jobs: github.event.sender.id == 136622811) || (github.event_name == 'pull_request_target' && ((github.event.action != 'labeled' && github.event.action != 'unlabeled') || - github.event.label.name == 'gui-screenshot-waived')) + github.event.label.name == 'gui-screenshot-waived' || + github.event.label.name == 'intake: hygiene-blocked' || + github.event.label.name == 'maintainer-sponsored' || + github.event.label.name == 'test-exception-approved' || + github.event.label.name == 'suppression-approved' || + github.event.label.name == 'generated-change-approved' || + github.event.label.name == 'dependency-change-approved')) runs-on: ubuntu-latest permissions: contents: read @@ -139,6 +145,12 @@ jobs: } = require( path.join(process.cwd(), ".github", "scripts", "pr-quality.cjs"), ); + const { + collectDeterministicHygieneFailures, + HYGIENE_FAILURE_HINTS, + } = require( + path.join(process.cwd(), ".github", "scripts", "pr-hygiene.cjs"), + ); const { parseGateState, gateStateMarker, @@ -536,6 +548,26 @@ jobs: guiOverrideComments: comments }); + // Hygiene is a separate workflow that owns the blocked label and + // the Hygiene comment section, but Ready / review-ready must not + // clear while those checks fail. Re-assess here from the same + // trusted scripts so the gate cannot race ahead of hygiene. + const changedFiles = await github.paginate( + github.rest.pulls.listFiles, + { owner, repo, pull_number, per_page: 100 } + ); + const labelNames = (pr.labels ?? []).map(label => label.name); + failures = [ + ...failures, + ...collectDeterministicHygieneFailures({ + files: changedFiles, + labels: labelNames, + authorHasPushPermission: + !permissionLookupFailed && + authorHasPushPermission(authorPermission), + }), + ]; + // A maintainer issue comment saying the change does not touch // the GUI waives the screenshot gate. The flag is what tells the // author the screenshot is not required, even though the failure @@ -943,6 +975,14 @@ jobs: "Add a screenshot of the UI change to the PR description." ); } + for (const failure of failures) { + const hint = HYGIENE_FAILURE_HINTS[failure.code]; + if (!hint) continue; + const paths = failure.paths?.length + ? ` Paths: ${failure.paths.map(p => inlineCode(p)).join(", ")}.` + : ""; + actions.push(`Fix **${failure.code}** — ${hint}${paths}`); + } if (checklistRequired && !checklistComplete) { actions.push( `Tick all four boxes in the PR description once you're done (currently ${readiness.checked}/${readiness.total}).` @@ -1028,6 +1068,9 @@ jobs: if (failure.code === "missing_ui_screenshot") { return "UI screenshot required."; } + if (HYGIENE_FAILURE_HINTS[failure.code]) { + return `hygiene: ${failure.code}.`; + } return failure.code; }) .join(" "); diff --git a/.github/workflows/pr-hygiene.yml b/.github/workflows/pr-hygiene.yml index 86838dd97..c374842ce 100644 --- a/.github/workflows/pr-hygiene.yml +++ b/.github/workflows/pr-hygiene.yml @@ -39,12 +39,12 @@ jobs: with: script: | const path = require("node:path"); - const { assessHygiene } = require( + const { + collectDeterministicHygieneFailures, + HYGIENE_FAILURE_HINTS, + } = require( path.join(process.cwd(), ".github", "scripts", "pr-hygiene.cjs"), ); - const { assessSponsoredSurface } = require( - path.join(process.cwd(), ".github", "scripts", "pr-sponsored-surface.cjs"), - ); const { GATE_MARKER, HYGIENE_MARKER, @@ -107,16 +107,13 @@ jobs: // Sponsorship is head-independent: it is about which surfaces the // change touches, not about the state of a particular revision, so // it is NOT cleared by the synchronize sweep above. - const failures = [ - ...assessHygiene({ files, labels: [...labels] }), - ...assessSponsoredSurface({ - authorHasPushPermission: ["OWNER", "MEMBER", "COLLABORATOR"].includes( - pr.author_association, - ), - changedFiles: files.map((file) => file.filename), - labels: [...labels], - }), - ]; + const failures = collectDeterministicHygieneFailures({ + files, + labels: [...labels], + authorHasPushPermission: ["OWNER", "MEMBER", "COLLABORATOR"].includes( + pr.author_association, + ), + }); async function setBlocked(blocked) { if (blocked && !labels.has(blockedLabel)) { @@ -169,20 +166,11 @@ jobs: return; } - const explanations = { - missing_regression_test: "Behavior changed under `src/` or `gui/src/` without a test change. Add focused coverage or obtain `test-exception-approved`.", - generated_output: "Generated build output is committed. Remove it or obtain `generated-change-approved`.", - orphan_lockfile: "`bun.lock` changed without `package.json`. Revert accidental churn or obtain `dependency-change-approved`.", - new_suppression: "A new TypeScript, lint, formatter, or similar suppression was added. Fix the underlying issue or obtain `suppression-approved`.", - focused_or_skipped_test: "A focused or skipped test was added. Restore the complete suite or obtain `test-exception-approved`.", - empty_catch: "An empty catch block was added. Handle, report, or deliberately propagate the error.", - unsponsored_surface: "This changes an authentication, workflow, release-automation, or dependency surface. `MAINTAINERS.md` requires security review for these; ask a maintainer to apply `maintainer-sponsored` once they have reviewed it.", - }; const lines = failures.map((failure) => { const paths = failure.paths?.length ? ` Paths: ${failure.paths.map((p) => `\`${p}\``).join(", ")}.` : ""; - return `- **${failure.code}** — ${explanations[failure.code]}${paths}`; + return `- **${failure.code}** — ${HYGIENE_FAILURE_HINTS[failure.code] ?? failure.code}${paths}`; }); await setBlocked(true); diff --git a/tests/ci-workflows.test.ts b/tests/ci-workflows.test.ts index ef2dc02a2..7ae4b3f6a 100644 --- a/tests/ci-workflows.test.ts +++ b/tests/ci-workflows.test.ts @@ -735,6 +735,7 @@ describe("GitHub Actions hardening", () => { "require", "require", "require", + "require", ] as const; /** Reads every allowed-base PR performs before any enforcement writes. */ @@ -746,6 +747,7 @@ describe("GitHub Actions hardening", () => { "repos.getCollaboratorPermissionLevel", "repos.compareCommitsWithBasehead", "repos.compareCommitsWithBasehead", + "pulls.listFiles", ...tail, ]; } @@ -758,6 +760,7 @@ describe("GitHub Actions hardening", () => { "issues.listComments", "repos.getCollaboratorPermissionLevel", "pulls.list", + "pulls.listFiles", ...tail, ]; } @@ -772,6 +775,10 @@ describe("GitHub Actions hardening", () => { "repos.getCollaboratorPermissionLevel", "repos.compareCommitsWithBasehead", "repos.compareCommitsWithBasehead", + // The harness walks every paginate call across the same page count, so + // listFiles appears once per comment page even when the file list is empty. + "pulls.listFiles", + "pulls.listFiles", ...tail, ]; } @@ -910,6 +917,12 @@ describe("GitHub Actions hardening", () => { expect(String(resolver?.["if"] ?? "")).toContain("github.event.sender.login == 'coderabbitai[bot]'"); expect(String(resolver?.["if"] ?? "")).toContain("github.event.sender.id == 136622811"); expect(String(resolver?.["if"] ?? "")).toContain("github.event.label.name == 'gui-screenshot-waived'"); + expect(String(resolver?.["if"] ?? "")).toContain("github.event.label.name == 'intake: hygiene-blocked'"); + expect(String(resolver?.["if"] ?? "")).toContain("github.event.label.name == 'maintainer-sponsored'"); + expect(String(resolver?.["if"] ?? "")).toContain("github.event.label.name == 'test-exception-approved'"); + expect(String(resolver?.["if"] ?? "")).toContain("github.event.label.name == 'suppression-approved'"); + expect(String(resolver?.["if"] ?? "")).toContain("github.event.label.name == 'generated-change-approved'"); + expect(String(resolver?.["if"] ?? "")).toContain("github.event.label.name == 'dependency-change-approved'"); const hygieneWorkflow = Bun.YAML.parse( await readText(".github/workflows/pr-hygiene.yml"), @@ -1032,6 +1045,8 @@ describe("GitHub Actions hardening", () => { // The verdict is a live PR read plus ancestry/description checks. expect(script).toContain("github.rest.pulls.get"); expect(script).toContain("collectPrQualityFailures"); + expect(script).toContain("collectDeterministicHygieneFailures"); + expect(script).toContain("github.rest.pulls.listFiles"); // The GUI screenshot gate reads the title as well as the body. expect(script).toContain("title: pr.title"); expect(script).toContain("github.rest.repos.getCollaboratorPermissionLevel"); @@ -1149,7 +1164,9 @@ describe("GitHub Actions hardening", () => { name !== "github.rest.repos.listPullRequestsAssociatedWithCommit" && name !== "github.rest.issues.listEvents" && // The claim check reads check-runs; it must never count as a write. - name !== "github.rest.checks.listForRef", + name !== "github.rest.checks.listForRef" && + // Hygiene reassessment reads the changed-file list; not a write. + name !== "github.rest.pulls.listFiles", ); expect([...new Set(restWrites)].sort()).toEqual([ "github.rest.issues.addLabels", @@ -1404,6 +1421,70 @@ describe("GitHub Actions hardening", () => { expect(result.warnings.some(w => w.startsWith("setFailed:"))).toBe(false); }); + test("unsponsored restricted surfaces keep Ready and review-ready blocked", async () => { + // Regression for #1324: hygiene failed on auth-api while the quality gate + // still posted READY and applied review-ready. The gate must re-assess + // deterministic hygiene itself and treat those failures like any other + // quality block. + const result = await run({ + pr: { + base: { ref: "dev" }, + draft: false, + body: readinessChecklistBody(4), + }, + maintainersFile: MAINTAINERS_FIXTURE, + files: [ + { filename: "src/codex/auth-api.ts", patch: "+change" }, + { filename: "tests/codex-auth-api.test.ts", patch: "+test" }, + ], + }); + + expect( + result.warnings.some( + w => w.startsWith("setFailed:") && w.includes("unsponsored_surface"), + ), + ).toBe(true); + expect(methodsOf(result)).toContain("pulls.listFiles"); + expect(methodsOf(result)).not.toContain("issues.addLabels"); + expect(methodsOf(result)).not.toContain("markPullRequestReadyForReview"); + const graphqlCalls = callsTo(result, "graphql") as Array<{ query: string }>; + expect( + graphqlCalls.some(call => call.query.includes("markPullRequestReadyForReview")), + ).toBe(false); + expect( + graphqlCalls.some(call => call.query.includes("convertPullRequestToDraft")), + ).toBe(true); + const gateBody = lastReadinessCommentBody(result); + expect(gateBody).toContain("## ⏳ DRAFT"); + expect(gateBody).toContain("unsponsored_surface"); + expect(gateBody).toContain("maintainer-sponsored"); + expect(gateBody).not.toContain("## ✅ READY"); + }); + + test("maintainer-sponsored clears the restricted-surface Ready block", async () => { + const result = await run({ + pr: { + base: { ref: "dev" }, + draft: true, + body: readinessChecklistBody(4), + }, + maintainersFile: MAINTAINERS_FIXTURE, + labels: ["maintainer-sponsored"], + files: [ + { filename: "src/codex/auth-api.ts", patch: "+change" }, + { filename: "tests/codex-auth-api.test.ts", patch: "+test" }, + ], + }); + + expect(result.warnings.some(w => w.startsWith("setFailed:"))).toBe(false); + expect(methodsOf(result)).toContain("issues.addLabels"); + const graphqlCalls = callsTo(result, "graphql") as Array<{ query: string }>; + expect( + graphqlCalls.some(call => call.query.includes("markPullRequestReadyForReview")), + ).toBe(true); + expect(lastReadinessCommentBody(result)).toContain("## ✅ READY"); + }); + test("completing the checklist records the head it was completed on", async () => { // A pre-binding v1 state has no recorded SHA. Completion on the current // head binds forward instead of resetting, so a checklist that was diff --git a/tests/helpers/enforce-pr-target-harness.ts b/tests/helpers/enforce-pr-target-harness.ts index d555f461f..5edcc38ea 100644 --- a/tests/helpers/enforce-pr-target-harness.ts +++ b/tests/helpers/enforce-pr-target-harness.ts @@ -198,6 +198,26 @@ export type RunOptions = { * decide whether to add/remove the `review-ready` label. */ labels?: string[]; + /** + * Changed files `pulls.listFiles` reports for the PR. Used by the embedded + * hygiene reassessment that blocks Ready while deterministic hygiene fails. + * Defaults to an empty list (no hygiene failures). + */ + files?: Array<{ + filename: string; + status?: string; + patch?: string; + previous_filename?: string; + }>; + /** Page-keyed file fixtures for `pulls.listFiles` pagination tests. */ + filePages?: Array< + Array<{ + filename: string; + status?: string; + patch?: string; + previous_filename?: string; + }> + >; /** * GraphQL query fragments that should reject. Unlike `failOn: ["graphql"]`, * which fails the review-threads read, this lets a test fail a specific @@ -616,11 +636,15 @@ export async function runEnforcePrTarget( (options.openPulls && options.openPulls.length > 0 ? [options.openPulls] : []); const associatedPullRequestPages: unknown[][] = options.associatedPullRequestPages ?? [options.associatedPullRequests ?? [pr]]; + const filePages: unknown[][] = + options.filePages ?? + (options.files && options.files.length > 0 ? [options.files] : [[]]); const paginatePageCount = Math.max( pages.length, issueEventPages.length, openPullPages.length, associatedPullRequestPages.length, + filePages.length, 1, ); @@ -717,6 +741,10 @@ export async function runEnforcePrTarget( return respond("pulls.list", args, openPullPages[page - 1] ?? []); }, listReviews: (args: unknown) => respond("pulls.listReviews", args, options.reviews ?? []), + listFiles: (args: unknown) => { + const page = Number((args as { page?: number })?.page ?? 1); + return respond("pulls.listFiles", args, filePages[page - 1] ?? []); + }, }, issues: { // Honours `page`, so a caller that skips `paginate` sees only page one — From 13a20c31a3c758d4d4d5cfb2e9d9b57ab984b65b Mon Sep 17 00:00:00 2001 From: Wibias <37517432+Wibias@users.noreply.github.com> Date: Sun, 9 Aug 2026 05:35:44 +0200 Subject: [PATCH 2/2] fix(ci): address CodeRabbit findings on hygiene gate trust boundary Keep rename sources in sponsored-surface checks, load hygiene scripts from the PR base SHA, and exempt sponsorship only for write-capable repository permissions. --- .github/scripts/pr-hygiene.cjs | 12 +++++++- .github/scripts/pr-hygiene.test.cjs | 43 +++++++++++++++++++++++++++++ .github/workflows/pr-hygiene.yml | 35 +++++++++++++++++++---- 3 files changed, 83 insertions(+), 7 deletions(-) diff --git a/.github/scripts/pr-hygiene.cjs b/.github/scripts/pr-hygiene.cjs index e7706de8b..738e0af53 100644 --- a/.github/scripts/pr-hygiene.cjs +++ b/.github/scripts/pr-hygiene.cjs @@ -264,11 +264,21 @@ function collectDeterministicHygieneFailures({ labels = [], authorHasPushPermission = false, }) { + // Renames must keep the source path: moving a restricted file to a + // non-restricted destination must not drop the sponsorship requirement. + const changedFiles = [ + ...new Set( + files.flatMap((file) => [ + file.filename, + ...(file.previous_filename ? [file.previous_filename] : []), + ]), + ), + ]; return [ ...assessHygiene({ files, labels }), ...assessSponsoredSurface({ authorHasPushPermission, - changedFiles: files.map((file) => file.filename), + changedFiles, labels, }), ]; diff --git a/.github/scripts/pr-hygiene.test.cjs b/.github/scripts/pr-hygiene.test.cjs index 44395ea25..af208f2b8 100644 --- a/.github/scripts/pr-hygiene.test.cjs +++ b/.github/scripts/pr-hygiene.test.cjs @@ -232,9 +232,52 @@ describe("collectDeterministicHygieneFailures", () => { assert.deepEqual(failures, []); }); + it("requires sponsorship when renaming away from a restricted path", () => { + const failures = collectDeterministicHygieneFailures({ + files: [ + { + filename: "docs/moved-release.yml", + previous_filename: ".github/workflows/release.yml", + status: "renamed", + patch: "+moved", + }, + ], + authorHasPushPermission: false, + }); + const unsponsored = failures.find((failure) => failure.code === "unsponsored_surface"); + assert.ok(unsponsored, "expected unsponsored_surface for a restricted rename source"); + assert.deepEqual(unsponsored.paths, [".github/workflows/release.yml"]); + }); + it("exposes hints and gate labels for the Ready coupling", () => { assert.equal(typeof HYGIENE_FAILURE_HINTS.unsponsored_surface, "string"); assert.ok(HYGIENE_GATE_LABELS.includes("maintainer-sponsored")); assert.ok(HYGIENE_GATE_LABELS.includes("intake: hygiene-blocked")); }); }); + +describe("pr-hygiene workflow trust boundary", () => { + const fs = require("node:fs"); + const path = require("node:path"); + const workflow = fs.readFileSync( + path.join(__dirname, "../workflows/pr-hygiene.yml"), + "utf8", + ); + + it("checks out the PR base SHA, not the repository default branch", () => { + assert.match(workflow, /ref:\s*\$\{\{\s*github\.event\.pull_request\.base\.sha\s*\}\}/); + assert.doesNotMatch( + workflow, + /Checkout trusted hygiene script[\s\S]*?ref:\s*\$\{\{\s*github\.event\.repository\.default_branch/, + ); + }); + + it("uses repository permission level for the sponsorship exemption", () => { + assert.match(workflow, /getCollaboratorPermissionLevel/); + assert.match(workflow, /authorHasPushPermission\(authorPermission\)/); + assert.doesNotMatch( + workflow, + /authorHasPushPermission:\s*\["OWNER",\s*"MEMBER",\s*"COLLABORATOR"\]\.includes\(\s*pr\.author_association/, + ); + }); +}); diff --git a/.github/workflows/pr-hygiene.yml b/.github/workflows/pr-hygiene.yml index c374842ce..95b7a408e 100644 --- a/.github/workflows/pr-hygiene.yml +++ b/.github/workflows/pr-hygiene.yml @@ -4,8 +4,8 @@ on: pull_request_target: types: [opened, reopened, synchronize, labeled, unlabeled] -# Trusted default-branch script only. Patches are read through the GitHub API; -# PR-head code is never checked out or executed. +# Trusted scripts from the PR base revision only. Patches are read through the +# GitHub API; PR-head code is never checked out or executed. # Least privilege: no default permissions; the hygiene job grants only what it needs. permissions: {} @@ -30,7 +30,9 @@ jobs: - name: Checkout trusted hygiene script uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 with: - ref: ${{ github.event.repository.default_branch }} + # pull_request_target must pin scripts to the PR base SHA, not the + # repository default branch: PRs target `dev` while `main` may lag. + ref: ${{ github.event.pull_request.base.sha }} persist-credentials: false sparse-checkout: .github/scripts @@ -45,6 +47,9 @@ jobs: } = require( path.join(process.cwd(), ".github", "scripts", "pr-hygiene.cjs"), ); + const { authorHasPushPermission } = require( + path.join(process.cwd(), ".github", "scripts", "pr-quality.cjs"), + ); const { GATE_MARKER, HYGIENE_MARKER, @@ -107,12 +112,30 @@ jobs: // Sponsorship is head-independent: it is about which surfaces the // change touches, not about the state of a particular revision, so // it is NOT cleared by the synchronize sweep above. + // author_association is not enough: a read/triage collaborator can + // be COLLABORATOR without write access. Match the PR quality gate. + let authorPermission = null; + let permissionLookupFailed = false; + try { + const { data: permissionData } = + await github.rest.repos.getCollaboratorPermissionLevel({ + owner, + repo, + username: pr.user.login, + }); + authorPermission = permissionData.permission; + } catch (error) { + permissionLookupFailed = true; + core.warning( + `Could not look up collaborator permission: ${error.message}`, + ); + } const failures = collectDeterministicHygieneFailures({ files, labels: [...labels], - authorHasPushPermission: ["OWNER", "MEMBER", "COLLABORATOR"].includes( - pr.author_association, - ), + authorHasPushPermission: + !permissionLookupFailed && + authorHasPushPermission(authorPermission), }); async function setBlocked(blocked) {