diff --git a/.github/workflows/enforce-pr-target.yml b/.github/workflows/enforce-pr-target.yml index 427ac15d3..e032e6cca 100644 --- a/.github/workflows/enforce-pr-target.yml +++ b/.github/workflows/enforce-pr-target.yml @@ -25,33 +25,89 @@ permissions: contents: write pull-requests: write -concurrency: - # PR lifecycle events and CodeRabbit statuses both identify the live head SHA. - # PR hygiene uses that same SHA key, so every writer for one live head is - # serialized even when the wake-up events come from different webhook types. - group: pr-gate-comment-${{ github.event.pull_request.head.sha || github.event.sha || github.run_id }} - jobs: - enforce-target: - # Only CodeRabbit's successful legacy commit status may wake the status - # path. Label events are runner-filtered to the one maintainer-controlled - # waiver label so ordinary type labels and the gate's own `review-ready` - # writes do not allocate another privileged runner. + resolve-pr: + # This read-only job resolves every trusted wake-up event to a PR number + # before the write-capable job starts. The PR number is the stable identity + # used by both gate writers even when a contributor pushes a new head SHA. if: >- (github.event_name == 'status' && github.event.context == 'CodeRabbit' && - github.event.state == 'success') || + github.event.state == 'success' && + github.event.sender.login == 'coderabbitai[bot]' && + 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')) runs-on: ubuntu-latest + permissions: + contents: read + pull-requests: read + outputs: + pull-number: ${{ steps.resolve.outputs.pull-number }} + steps: + - name: Resolve trusted gate event to PR + id: resolve + uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9 + with: + script: | + const { owner, repo } = context.repo; + let pullNumber = context.payload.pull_request?.number ?? null; + + if (context.eventName === "status") { + const sender = context.payload.sender; + const trustedCodeRabbit = + context.payload.context === "CodeRabbit" && + context.payload.state === "success" && + sender?.login === "coderabbitai[bot]" && + sender?.id === 136622811; + if (!trustedCodeRabbit) { + core.info("Status producer is not the CodeRabbit GitHub App; skipping."); + return; + } + + const statusSha = context.payload.sha; + const associatedPrs = await github.paginate( + github.rest.repos.listPullRequestsAssociatedWithCommit, + { owner, repo, commit_sha: statusSha, per_page: 100 } + ); + const candidates = associatedPrs.filter( + candidate => + candidate.state === "open" && + candidate.head?.sha === statusSha + ); + if (candidates.length !== 1) { + core.info( + `CodeRabbit status ${statusSha} maps to ${candidates.length} open current-head PRs; skipping ambiguous/stale revalidation.` + ); + return; + } + pullNumber = candidates[0].number; + } + + if (Number.isInteger(pullNumber)) { + core.setOutput("pull-number", String(pullNumber)); + } + + enforce-target: + needs: resolve-pr + if: needs.resolve-pr.outputs.pull-number != '' + runs-on: ubuntu-latest + permissions: + contents: write + pull-requests: write + concurrency: + # Serialize every writer by PR identity, not head SHA. An older-head run + # therefore cannot race a newer-head run that rewrites the same comment. + group: pr-gate-comment-${{ needs.resolve-pr.outputs.pull-number }} + cancel-in-progress: false steps: - name: Checkout trusted PR-quality scripts uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 with: # `pull_request_target` runs from the PR base revision, so use that - # immutable base SHA for the trusted scripts. `issue_comment` runs the + # immutable base SHA for the trusted scripts. `status` runs the # privileged workflow from the repository default branch; source its # scripts and MAINTAINERS.md from that same promoted trust boundary. # This prevents unpromoted `dev` script changes from executing with @@ -64,6 +120,8 @@ jobs: - name: Enforce PR target, ancestry, and description uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9 + env: + RESOLVED_PULL_NUMBER: ${{ needs.resolve-pr.outputs.pull-number }} with: script: | const path = require("path"); @@ -141,39 +199,25 @@ jobs: const GUI_SCREENSHOT_WAIVER_LABEL = "gui-screenshot-waived"; const MAINTAINERS_FILE = "MAINTAINERS.md"; const { owner, repo } = context.repo; - let pull_number = context.payload.pull_request?.number; + const resolvedPullNumber = process.env.RESOLVED_PULL_NUMBER ?? ""; + const pull_number = /^\d+$/.test(resolvedPullNumber) + ? Number.parseInt(resolvedPullNumber, 10) + : Number.NaN; - // `status` is default-branch controlled, but it carries only the - // reviewed commit SHA. Resolve that SHA back to exactly one open PR - // whose current head still equals the status SHA. Ambiguous or stale - // associations fail closed without mutating any pull request. - if (context.eventName === "status") { - const statusSha = context.payload.sha; - const associatedPrs = await github.paginate( - github.rest.repos.listPullRequestsAssociatedWithCommit, - { - owner, - repo, - commit_sha: statusSha, - per_page: 100 - } - ); - const candidates = associatedPrs.filter( - candidate => - candidate.state === "open" && - candidate.head?.sha === statusSha - ); - if (candidates.length !== 1) { - core.info( - `CodeRabbit status ${statusSha} maps to ${candidates.length} open current-head PRs; skipping ambiguous/stale revalidation.` - ); - return; - } - pull_number = candidates[0].number; + // `resolve-pr` is the single authority that maps a trusted event to + // exactly one live PR. The write-capable job consumes only that + // resolved identity so its concurrency key and mutation target + // cannot diverge. + if (!Number.isSafeInteger(pull_number) || pull_number < 1) { + core.info("No pull request could be resolved for this gate event; skipping."); + return; } - if (!Number.isInteger(pull_number)) { - core.info("No pull request could be resolved for this gate event; skipping."); + // Defense in depth: the resolver job is the primary event gate, but + // the write-capable script also rejects event classes this workflow + // never intends to mutate from. + if (!["pull_request_target", "status"].includes(context.eventName)) { + core.info(`Unsupported gate event ${context.eventName}; skipping.`); return; } @@ -496,9 +540,56 @@ jobs: // the GUI waives the screenshot gate. The flag is what tells the // author the screenshot is not required, even though the failure // itself is gone from `failures`. - const screenshotWaivedByLabel = (pr.labels ?? []).some( + const screenshotWaiverLabelPresent = (pr.labels ?? []).some( label => label.name === GUI_SCREENSHOT_WAIVER_LABEL ); + const maintainerLogins = new Set( + readMaintainerLogins().map(login => login.toLowerCase()) + ); + let screenshotWaiverLabelActorLogin = null; + if (screenshotWaiverLabelPresent) { + try { + const issueEvents = await github.paginate( + github.rest.issues.listEvents, + { + owner, + repo, + issue_number: pull_number, + per_page: 100 + } + ); + const waiverEvents = issueEvents + .filter( + event => + (event.event === "labeled" || event.event === "unlabeled") && + event.label?.name === GUI_SCREENSHOT_WAIVER_LABEL + ) + .sort((left, right) => { + const leftTime = Date.parse(left.created_at ?? "") || 0; + const rightTime = Date.parse(right.created_at ?? "") || 0; + if (leftTime !== rightTime) return leftTime - rightTime; + return Number(left.id ?? 0) - Number(right.id ?? 0); + }); + const latestWaiverEvent = waiverEvents.at(-1); + if (latestWaiverEvent?.event === "labeled") { + screenshotWaiverLabelActorLogin = + latestWaiverEvent.actor?.login ?? null; + } + } catch (error) { + core.warning( + `Could not resolve ${GUI_SCREENSHOT_WAIVER_LABEL} label provenance: ${error.message}` + ); + } + } + const screenshotWaivedByLabel = + screenshotWaiverLabelPresent && + typeof screenshotWaiverLabelActorLogin === "string" && + maintainerLogins.has(screenshotWaiverLabelActorLogin.toLowerCase()); + if (screenshotWaiverLabelPresent && !screenshotWaivedByLabel) { + core.info( + `${screenshotWaiverLabelActorLogin ?? "unknown label actor"} is not in MAINTAINERS.md; ignoring ${GUI_SCREENSHOT_WAIVER_LABEL}.` + ); + } if (screenshotWaivedByLabel) { failures = failures.filter( failure => failure.code !== "missing_ui_screenshot" @@ -557,10 +648,10 @@ jobs: // (see `completionIsStale`). When it is stale the gate resets the // boxes and the notification state, re-drafts, and tells the // author to re-test and re-tick against the latest code. - // `issue_comment` events carry no `pull_request.head.sha`, so the + // `status` events carry no `pull_request.head.sha`, so the // fallback to the live head would let a completed checklist with // no recorded completion head pass as if it attested the current - // head. A comment-triggered run must not promote readiness: pass + // head. A status-triggered run must not promote readiness: pass // the live head only when the event actually delivered it. const eventHeadSha = context.payload.pull_request?.head?.sha ?? diff --git a/.github/workflows/pr-hygiene.yml b/.github/workflows/pr-hygiene.yml index 4820c6db3..86838dd97 100644 --- a/.github/workflows/pr-hygiene.yml +++ b/.github/workflows/pr-hygiene.yml @@ -11,11 +11,10 @@ permissions: {} concurrency: # Shared with the enforce-target gate: both workflows read-modify-write the - # same consolidated gate comment, so one live-head SHA group serializes them. - # `cancel-in-progress` stays false (the enforce-target gate also omits it): - # a newer run must queue behind the in-flight one, never cancel it mid - # comment mutation, or the cancelled run's read-modify-write is lost. - group: pr-gate-comment-${{ github.event.pull_request.head.sha }} + # same consolidated gate comment, so one stable PR-number group serializes + # old-head and new-head runs as well as hygiene and gate writes. + # A newer run queues behind the in-flight one instead of cancelling it. + group: pr-gate-comment-${{ github.event.pull_request.number }} cancel-in-progress: false jobs: diff --git a/tests/ci-workflows.test.ts b/tests/ci-workflows.test.ts index f9d7856c8..fa6ca5cc7 100644 --- a/tests/ci-workflows.test.ts +++ b/tests/ci-workflows.test.ts @@ -827,9 +827,8 @@ describe("GitHub Actions hardening", () => { test("PR target enforcement's structure is an exact allowlist, not a deny-list", async () => { const { workflow, jobs, steps } = await readEnforcePrTarget(); - // Top level: these five keys and nothing else. + // Top level: concurrency lives on the write job after read-only PR resolution. expect(Object.keys(workflow).sort()).toEqual([ - "concurrency", "jobs", "name", "on", @@ -873,50 +872,59 @@ describe("GitHub Actions hardening", () => { "pull-requests": "write", }); - // Every writer for one live PR head must resolve to the same SHA lock. - // `pull_request_target` exposes it under pull_request.head.sha; `status` - // exposes the same value as event.sha. The run-id fallback is fail-safe for - // malformed payloads and prevents unrelated runs sharing an empty key. - expect(workflow.concurrency).toEqual({ - group: - "pr-gate-comment-${{ github.event.pull_request.head.sha || github.event.sha || github.run_id }}", + expect(workflow.concurrency).toBeUndefined(); + + const resolver = workflow.jobs?.["resolve-pr"] as WorkflowJob | undefined; + expect(resolver).toBeDefined(); + expect(Object.keys(resolver ?? {}).sort()).toEqual([ + "if", + "outputs", + "permissions", + "runs-on", + "steps", + ]); + expect(resolver?.["runs-on"]).toBe("ubuntu-latest"); + expect(resolver?.permissions).toEqual({ + contents: "read", + "pull-requests": "read", }); + expect(String(resolver?.["if"] ?? "")).toContain("github.event.context == 'CodeRabbit'"); + expect(String(resolver?.["if"] ?? "")).toContain("github.event.state == 'success'"); + 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'"); - // The hygiene workflow reads and rewrites the same consolidated gate - // comment, so it must share the gate's per-PR concurrency group. Separate - // groups would let a gate rebuild and a hygiene update run concurrently - // from stale snapshots, and the last write would drop the other's section. const hygieneWorkflow = Bun.YAML.parse( await readText(".github/workflows/pr-hygiene.yml"), ) as { concurrency?: { group?: string; "cancel-in-progress"?: boolean } }; expect(hygieneWorkflow.concurrency?.group).toBe( - "pr-gate-comment-${{ github.event.pull_request.head.sha }}", + "pr-gate-comment-${{ github.event.pull_request.number }}", ); - expect(workflow.concurrency?.group).toContain("github.event.pull_request.head.sha"); - expect(workflow.concurrency?.group).toContain("github.event.sha"); - // Both comment-writing workflows share the group and neither cancels: - // `cancel-in-progress: true` would kill an in-flight gate mutation when a - // newer hygiene run starts, losing that read-modify-write. expect(hygieneWorkflow.concurrency?.["cancel-in-progress"]).toBe(false); - // One job, and it is this one. An audit round added a `sidecar:` job that - // inherited the PR-write token and un-drafted the PR — every assertion below - // still passed, because they only ever looked at `enforce-target`. - expect(jobs.map(([name]) => name)).toEqual(["enforce-target"]); - - // The job is a runner plus steps, with one deliberate `if:` guard. The - // guard accepts only successful CodeRabbit commit statuses and filters - // label events to the screenshot-waiver label. Other PR lifecycle events run. - // No `permissions:` (a job-level block overrides the narrow workflow-level - // one), no `container:`/`strategy:`/`outputs:`/`env:`/`defaults:`, and no - // `<<:` merge key to reintroduce any of them sideways. - const [, job] = jobs[0]!; - expect(Object.keys(job).sort()).toEqual(["if", "runs-on", "steps"]); - expect(job["runs-on"]).toBe("ubuntu-latest"); - expect(job["if"]).toContain("github.event_name == 'status'"); - expect(job["if"]).toContain("github.event.context == 'CodeRabbit'"); - expect(job["if"]).toContain("github.event.state == 'success'"); - expect(job["if"]).toContain("github.event.label.name == 'gui-screenshot-waived'"); + expect(jobs.map(([name]) => name)).toEqual(["resolve-pr", "enforce-target"]); + + const job = workflow.jobs?.["enforce-target"] as WorkflowJob | undefined; + expect(job).toBeDefined(); + expect(Object.keys(job ?? {}).sort()).toEqual([ + "concurrency", + "if", + "needs", + "permissions", + "runs-on", + "steps", + ]); + expect(job?.["runs-on"]).toBe("ubuntu-latest"); + expect(job?.permissions).toEqual({ + contents: "write", + "pull-requests": "write", + }); + expect(job?.needs).toBe("resolve-pr"); + expect(job?.["if"]).toBe("needs.resolve-pr.outputs.pull-number != ''"); + expect(job?.concurrency).toEqual({ + group: "pr-gate-comment-${{ needs.resolve-pr.outputs.pull-number }}", + "cancel-in-progress": false, + }); // Checkout trusted scripts, then run the gate. Anything more is an extra // privileged action nobody reviewed. @@ -943,7 +951,10 @@ describe("GitHub Actions hardening", () => { "sparse-checkout": ".github/scripts\nMAINTAINERS.md\n", }); - expect(Object.keys(scriptStep).sort()).toEqual(["name", "uses", "with"]); + expect(Object.keys(scriptStep).sort()).toEqual(["env", "name", "uses", "with"]); + expect(scriptStep.env).toEqual({ + RESOLVED_PULL_NUMBER: "${{ needs.resolve-pr.outputs.pull-number }}", + }); // `github-script` is the action, pinned to a 40-hex commit SHA: this // workflow hands a write token to whatever the ref resolves to, so a tag or @@ -1014,17 +1025,11 @@ describe("GitHub Actions hardening", () => { expect(script).toMatch(/const ALLOWED_BASES = \["dev"\];/); expect(script).toMatch(/const DEFAULT_BASE = "dev";/); - // Every mutation targets the PR the event fired for. `pull_number` is the - // only handle the script has, and an audit round repointed it at - // `Number(context.payload.pull_request.title)` — a value the PR author - // controls, which turns the bot into a write primitive against any PR - // number the author can name. Bind it to the immutable event field. - // Status runs resolve the immutable reviewed SHA back to exactly one open - // current-head PR before any write-capable operation. - expect(script).toContain("context.payload.pull_request?.number"); - expect(script).toContain("listPullRequestsAssociatedWithCommit"); - expect(script).toContain("candidate.head?.sha === statusSha"); - expect(script).toContain("candidates.length !== 1"); + // The read-only resolver is the single authority for PR identity. The + // write job consumes exactly that output, so its mutation target and + // concurrency key cannot diverge or perform a second SHA-to-PR lookup. + expect(script).toContain("process.env.RESOLVED_PULL_NUMBER"); + expect(script).not.toContain("listPullRequestsAssociatedWithCommit"); // Nothing may write back into the fetched PR. The audit round preserved the // required comparison line verbatim and defeated it one line earlier with @@ -1125,6 +1130,7 @@ describe("GitHub Actions hardening", () => { name !== "github.rest.repos.getCollaboratorPermissionLevel" && name !== "github.rest.repos.compareCommitsWithBasehead" && 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", ); @@ -2669,6 +2675,17 @@ describe("GitHub Actions hardening", () => { ].join("\n"), }, labels: ["gui-screenshot-waived"], + maintainersFile: MAINTAINERS_FIXTURE, + eventName: "pull_request_target", + eventAction: "synchronize", + senderLogin: "contributor", + issueEvents: [{ + id: 101, + event: "labeled", + created_at: "2026-08-08T06:00:00Z", + actor: { login: "lidge-jun" }, + label: { name: "gui-screenshot-waived" }, + }], }); expect(result.warnings.some((w) => w.startsWith("setFailed:") && w.includes("screenshot"))).toBe(false); @@ -2687,6 +2704,16 @@ describe("GitHub Actions hardening", () => { }, labels: ["gui-screenshot-waived"], maintainersFile: MAINTAINERS_FIXTURE, + eventName: "pull_request_target", + eventAction: "edited", + senderLogin: "contributor", + issueEvents: [{ + id: 101, + event: "labeled", + created_at: "2026-08-08T06:00:00Z", + actor: { login: "lidge-jun" }, + label: { name: "gui-screenshot-waived" }, + }], }); expect(result.warnings.some((w) => w.startsWith("setFailed:") && w.includes("screenshot"))).toBe(false); @@ -2695,87 +2722,67 @@ describe("GitHub Actions hardening", () => { expect(lastEnforcerCommentBody(result)).toContain("UI screenshot waived by the `gui-screenshot-waived` label"); }); - test("an ambiguous CodeRabbit status SHA fails closed before any PR mutation", async () => { - const headSha = "3f1c0de0a6a4d0a3f9a1b2c3d4e5f60718293a4b"; + test("the gui-screenshot-waived label from an unauthorized sender does not waive the screenshot requirement", async () => { const result = await run({ - pr: { base: { ref: "dev" } }, - eventName: "status", - statusSha: headSha, - associatedPullRequests: [ - { number: 42, state: "open", head: { sha: headSha } }, - { number: 77, state: "open", head: { sha: headSha } }, - ], - }); - - expect(methodsOf(result).filter(method => method !== "require")).toEqual(["repos.listPullRequestsAssociatedWithCommit"]); - expect(result.logs.join(" ")).toContain("maps to 2 open current-head PRs; skipping ambiguous/stale revalidation"); - expect(callsTo(result, "pulls.update")).toEqual([]); - expect(callsTo(result, "issues.createComment")).toEqual([]); - expect(callsTo(result, "issues.updateComment")).toEqual([]); - expect(callsTo(result, "issues.addLabels")).toEqual([]); - expect(callsTo(result, "issues.removeLabel")).toEqual([]); - expect(callsTo(result, "graphql")).toEqual([]); - }); - - - test("a single open current-head PR resolves the CodeRabbit status SHA and runs the gate", async () => { - const headSha = "3f1c0de0a6a4d0a3f9a1b2c3d4e5f60718293a4b"; - const result = await run({ - pr: { base: { ref: "dev" }, number: 4242 }, - eventName: "status", - statusSha: headSha, - associatedPullRequests: [ - { number: 4242, state: "open", head: { sha: headSha } }, - ], + pr: { + base: { ref: "dev" }, + title: "GUI: fix provider list spacing", + body: [ + "## Summary", + "This change fixes the provider list spacing in the dashboard.", + "", + "## Test plan", + "- Ran bun test tests/ci-workflows.test.ts", + ].join("\n"), + }, + labels: ["gui-screenshot-waived"], + maintainersFile: MAINTAINERS_FIXTURE, + eventName: "pull_request_target", + eventAction: "synchronize", + senderLogin: "lidge-jun", + issueEvents: [{ + id: 102, + event: "labeled", + created_at: "2026-08-08T06:00:00Z", + actor: { login: "unauthorized-contributor" }, + label: { name: "gui-screenshot-waived" }, + }], }); - expect(callsTo(result, "pulls.get")).toEqual([ - { owner: "lidge-jun", repo: "opencodex", pull_number: 4242 }, - ]); - expect(result.logs.join(" ")).not.toContain("skipping ambiguous/stale revalidation"); - expect(methodsOf(result)).toContain("issues.listComments"); + // The screenshot failure must remain because the label was applied by + // an unauthorized user (not in MAINTAINERS.md). + expect(result.warnings.some((w) => w.startsWith("setFailed:") && w.includes("screenshot"))).toBe(true); + expect(lastEnforcerCommentBody(result)).toContain("UI screenshot required"); + expect(lastEnforcerCommentBody(result)).not.toContain("UI screenshot waived"); + expect(result.logs.join(" ")).toContain("unauthorized-contributor"); + expect(result.logs.join(" ")).toContain("not in MAINTAINERS.md"); }); - test("CodeRabbit status association paginates before deciding uniqueness", async () => { - const headSha = "3f1c0de0a6a4d0a3f9a1b2c3d4e5f60718293a4b"; + test("a missing resolved PR number fails closed before PR lookup", async () => { const result = await run({ pr: { base: { ref: "dev" } }, - eventName: "status", - statusSha: headSha, - associatedPullRequestPages: [ - [{ number: 42, state: "open", head: { sha: headSha } }], - [{ number: 77, state: "open", head: { sha: headSha } }], - ], + resolvedPullNumber: "", }); - expect(callsTo(result, "repos.listPullRequestsAssociatedWithCommit")).toHaveLength(2); - expect(result.logs.join(" ")).toContain("maps to 2 open current-head PRs; skipping ambiguous/stale revalidation"); - expect(callsTo(result, "pulls.update")).toEqual([]); + expect(result.logs.join(" ")).toContain("No pull request could be resolved for this gate event; skipping"); + expect(callsTo(result, "pulls.get")).toEqual([]); expect(callsTo(result, "issues.createComment")).toEqual([]); expect(callsTo(result, "issues.updateComment")).toEqual([]); - expect(callsTo(result, "issues.addLabels")).toEqual([]); - expect(callsTo(result, "issues.removeLabel")).toEqual([]); expect(callsTo(result, "graphql")).toEqual([]); }); - test("CodeRabbit status with no current-head match fails closed", async () => { - const statusSha = "3f1c0de0a6a4d0a3f9a1b2c3d4e5f60718293a4b"; + test("the write gate consumes the resolved PR number without re-resolving status SHA", async () => { const result = await run({ - pr: { base: { ref: "dev" } }, + pr: { base: { ref: "dev" }, number: 4242 }, eventName: "status", - statusSha, - associatedPullRequests: [ - { number: 42, state: "open", head: { sha: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" } }, - ], + resolvedPullNumber: 4242, }); - expect(result.logs.join(" ")).toContain("maps to 0 open current-head PRs; skipping ambiguous/stale revalidation"); - expect(callsTo(result, "pulls.update")).toEqual([]); - expect(callsTo(result, "issues.createComment")).toEqual([]); - expect(callsTo(result, "issues.updateComment")).toEqual([]); - expect(callsTo(result, "issues.addLabels")).toEqual([]); - expect(callsTo(result, "issues.removeLabel")).toEqual([]); - expect(callsTo(result, "graphql")).toEqual([]); + expect(callsTo(result, "pulls.get")).toEqual([ + { owner: "lidge-jun", repo: "opencodex", pull_number: 4242 }, + ]); + expect(callsTo(result, "repos.listPullRequestsAssociatedWithCommit")).toEqual([]); + expect(methodsOf(result)).toContain("issues.listComments"); }); test("a non-maintainer issue_comment does not re-run the gate", async () => { diff --git a/tests/helpers/enforce-pr-target-harness.ts b/tests/helpers/enforce-pr-target-harness.ts index 90a376de8..d555f461f 100644 --- a/tests/helpers/enforce-pr-target-harness.ts +++ b/tests/helpers/enforce-pr-target-harness.ts @@ -63,6 +63,14 @@ export type Comment = { author_association?: string; }; +export type IssueEvent = { + id?: number; + event: string; + created_at?: string; + actor?: { login?: string }; + label?: { name?: string }; +}; + export type RunOptions = { /** The PR as `pulls.get` will report it — the live, authoritative state. */ pr: PullRequestState; @@ -125,6 +133,12 @@ export type RunOptions = { commentPages?: Comment[][]; /** Shorthand for a single page. */ comments?: Comment[]; + /** Issue events used to resolve durable label-application provenance. */ + issueEvents?: IssueEvent[]; + /** Page-specific issue-event fixtures for pagination tests. */ + issueEventPages?: IssueEvent[][]; + /** Resolved PR number passed from the read-only resolver job. */ + resolvedPullNumber?: number | string; /** Method names that should reject, to exercise partial-failure paths. */ failOn?: string[]; /** @@ -191,6 +205,14 @@ export type RunOptions = { * succeeds. Matched case-sensitively against the query text. */ failGraphqlOn?: string[]; + /** + * Login of the event sender (who triggered the webhook, e.g., the user who + * applied a label). Defaults to `"contributor"`. Used to test authorization + * checks that validate the sender against MAINTAINERS.md. + */ + senderLogin?: string; + /** Numeric sender id; status events default to CodeRabbit's stable bot id. */ + senderId?: number; }; /** @@ -587,6 +609,8 @@ export async function runEnforcePrTarget( user: { ...DEFAULT_PR.user, ...(source.user ?? {}) }, }; const pages: Comment[][] = options.commentPages ?? [options.comments ?? []]; + const issueEventPages: IssueEvent[][] = + options.issueEventPages ?? [options.issueEvents ?? []]; const openPullPages: unknown[][] = options.openPullPages ?? (options.openPulls && options.openPulls.length > 0 ? [options.openPulls] : []); @@ -594,6 +618,7 @@ export async function runEnforcePrTarget( options.associatedPullRequestPages ?? [options.associatedPullRequests ?? [pr]]; const paginatePageCount = Math.max( pages.length, + issueEventPages.length, openPullPages.length, associatedPullRequestPages.length, 1, @@ -700,6 +725,10 @@ export async function runEnforcePrTarget( const page = Number((args as { page?: number })?.page ?? 1); return respond("issues.listComments", args, pages[page - 1] ?? []); }, + listEvents: (args: unknown) => { + const page = Number((args as { page?: number })?.page ?? 1); + return respond("issues.listEvents", args, issueEventPages[page - 1] ?? []); + }, createComment: (args: unknown) => respond("issues.createComment", args, { id: 99 }), updateComment: (args: unknown) => respond("issues.updateComment", args, { id: 7 }), deleteComment: (args: unknown) => respond("issues.deleteComment", args, {}), @@ -910,7 +939,17 @@ export async function runEnforcePrTarget( owner: { login: "lidge-jun", id: 12345, type: "User" }, html_url: "https://github.com/lidge-jun/opencodex", }, - sender: { login: "contributor", id: 67890, type: "User" }, + sender: options.eventName === "status" + ? { + login: options.senderLogin ?? "coderabbitai[bot]", + id: options.senderId ?? 136622811, + type: "Bot", + } + : { + login: options.senderLogin ?? "contributor", + id: options.senderId ?? 67890, + type: "User", + }, organization: undefined, installation: undefined, }; @@ -1037,6 +1076,11 @@ export async function runEnforcePrTarget( ); const deferred: (() => unknown)[] = []; + const runtime = nodeLikeRuntime(deferred); + const runtimeProcess = runtime.process as { env: Record }; + runtimeProcess.env.RESOLVED_PULL_NUMBER = String( + options.resolvedPullNumber ?? eventPr.number ?? "", + ); const returnValue = await compileScript(script)({ github, @@ -1063,7 +1107,7 @@ export async function runEnforcePrTarget( // `if (!process.versions.bun) return;` — a no-op in production, green here. // Shadow `process` with something that looks like the Node the workflow // actually gets, so a runtime probe cannot tell the two apart. - ...nodeLikeRuntime(deferred), + ...runtime, }); // Run whatever the script deferred. Node would run these too, with the write diff --git a/tests/zz-pr-coderabbit-readiness-revalidation.test.ts b/tests/zz-pr-coderabbit-readiness-revalidation.test.ts index bab6f22af..6071489d5 100644 --- a/tests/zz-pr-coderabbit-readiness-revalidation.test.ts +++ b/tests/zz-pr-coderabbit-readiness-revalidation.test.ts @@ -7,6 +7,7 @@ type WorkflowJob = { name?: string; uses?: string; run?: string; + env?: Record; with?: Record; }>; }; @@ -41,17 +42,23 @@ describe("workflow comment-spam hardening", () => { "unlabeled", ])); + const resolver = workflow.jobs?.["resolve-pr"]; + const resolverIf = (resolver?.if ?? "").replace(/\s+/g, " ").trim(); + for (const guard of [ + "github.event_name == 'status'", + "github.event.context == 'CodeRabbit'", + "github.event.state == 'success'", + "github.event.sender.login == 'coderabbitai[bot]'", + "github.event.sender.id == 136622811", + "github.event.action != 'labeled'", + "github.event.action != 'unlabeled'", + "github.event.label.name == 'gui-screenshot-waived'", + ]) { + expect(resolverIf).toContain(guard); + } + const job = workflow.jobs?.["enforce-target"]; - const normalize = (value: string | undefined) => - (value ?? "").replace(/\s+/g, " ").trim(); - expect(normalize(job?.if)).toBe(normalize(` - (github.event_name == 'status' && - github.event.context == 'CodeRabbit' && - github.event.state == 'success') || - (github.event_name == 'pull_request_target' && - ((github.event.action != 'labeled' && github.event.action != 'unlabeled') || - github.event.label.name == 'gui-screenshot-waived')) - `)); + expect(job?.if).toBe("needs.resolve-pr.outputs.pull-number != ''"); const checkoutStep = job?.steps?.find( step => step.name === "Checkout trusted PR-quality scripts", @@ -64,12 +71,11 @@ describe("workflow comment-spam hardening", () => { step => step.name === "Enforce PR target, ancestry, and description", ); const script = gateStep?.with?.script ?? ""; - expect(script).toContain("github.paginate"); - expect(script).toContain("listPullRequestsAssociatedWithCommit"); - expect(script).toContain('candidate.state === "open"'); - expect(script).toContain("candidate.head?.sha === statusSha"); - expect(script).toContain("candidates.length !== 1"); - expect(script).toContain('context.eventName === "status"'); + expect(gateStep?.env?.RESOLVED_PULL_NUMBER).toBe( + "${{ needs.resolve-pr.outputs.pull-number }}", + ); + expect(script).toContain("process.env.RESOLVED_PULL_NUMBER"); + expect(script).not.toContain("listPullRequestsAssociatedWithCommit"); expect(script).toContain('const GUI_SCREENSHOT_WAIVER_LABEL = "gui-screenshot-waived"'); expect(script).toContain("screenshotWaiverNotice"); expect(script).toContain("unresolvedFindingsClaim");