diff --git a/.github/scripts/enforce-pr-target.test.cjs b/.github/scripts/enforce-pr-target.test.cjs index 997333a62..33bfda42c 100644 --- a/.github/scripts/enforce-pr-target.test.cjs +++ b/.github/scripts/enforce-pr-target.test.cjs @@ -49,17 +49,17 @@ describe("enforce-pr-target workflow", () => { assert.match(workflow, /synchronize/); }); - it("re-runs on issue_comment so a maintainer GUI waiver takes effect", () => { - // The GUI-screenshot gate is waived by a maintainer issue comment - // ("not touching gui"). `pull_request_target` types do not include issue - // comments, so without this trigger the waiver sits unread until a PR - // edit or push re-runs the gate. - assert.match(workflow, /^ issue_comment:/m); - assert.match(workflow, /- created/); - assert.match(workflow, /- edited/); - // The script resolves the PR number from the issue payload, which is what - // an issue_comment event delivers instead of a pull_request object. - assert.match(workflow, /context\.payload\.issue\?\.number/); + it("uses label events for GUI waivers and a trusted CodeRabbit status signal", () => { + assert.doesNotMatch(workflow, /^ issue_comment:/m); + assert.match(workflow, /- labeled/); + assert.match(workflow, /- unlabeled/); + assert.match(workflow, /^ status:/m); + 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, /listPullRequestsAssociatedWithCommit/); + assert.match(workflow, /candidate\.head\?\.sha === statusSha/); + assert.match(workflow, /candidates\.length !== 1/); }); it("does not add review events that would break the trusted-base model", () => { @@ -141,13 +141,12 @@ describe("enforce-pr-target workflow", () => { .split("- name: Checkout trusted PR-quality scripts")[1] .split(/\n {6}- name:/)[0]; assert.match(checkoutStep, /actions\/checkout@[0-9a-f]{40}/); - // `pull_request_target` pins the PR base SHA. Privileged `issue_comment` - // runs must source scripts from the repository default branch, matching - // the branch that supplied the workflow itself; unpromoted `dev` scripts - // must never execute under the write-capable token. + // `pull_request_target` pins the PR base SHA. Trusted `status` + // revalidation has no pull_request payload, so it sources scripts from the + // repository default branch that supplied the privileged workflow itself. assert.match( checkoutStep, - /ref:\s*\$\{\{\s*github\.event_name\s*==\s*'issue_comment'\s*&&\s*github\.event\.repository\.default_branch\s*\|\|\s*github\.event\.pull_request\.base\.sha\s*\}\}/, + /ref:\s*\$\{\{\s*github\.event_name\s*==\s*'status'\s*&&\s*github\.event\.repository\.default_branch\s*\|\|\s*github\.event\.pull_request\.base\.sha\s*\}\}/, ); assert.doesNotMatch(checkoutStep, /\|\|\s*'dev'/); // The readiness ping reads MAINTAINERS.md from the same trusted checkout. diff --git a/.github/workflows/enforce-issue-quality.yml b/.github/workflows/enforce-issue-quality.yml index ba828712f..40abd02f9 100644 --- a/.github/workflows/enforce-issue-quality.yml +++ b/.github/workflows/enforce-issue-quality.yml @@ -443,7 +443,7 @@ jobs: translate-comment: name: Translate non-English issue comments - if: github.event_name == 'issue_comment' + if: github.event_name == 'issue_comment' && github.event.issue.pull_request == null && github.event.comment.user.type != 'Bot' runs-on: ubuntu-latest concurrency: # Shares the per-issue queue with the `translate` job: both jobs RMW the diff --git a/.github/workflows/enforce-pr-target.yml b/.github/workflows/enforce-pr-target.yml index cd32bdce1..e032e6cca 100644 --- a/.github/workflows/enforce-pr-target.yml +++ b/.github/workflows/enforce-pr-target.yml @@ -6,17 +6,15 @@ on: - opened - reopened - edited + - labeled + - unlabeled - ready_for_review - synchronize - # A maintainer issue comment ("not touching gui") waives the GUI-screenshot - # gate. CodeRabbit also edits its normal PR status comment when a review - # finishes, which gives this privileged workflow a safe signal to re-check - # review findings even for fork PRs. `pull_request_target` types do not - # include issue comments, so both cases use this separate trigger. - issue_comment: - types: - - created - - edited + # CodeRabbit publishes a legacy commit status named `CodeRabbit` on the + # reviewed head SHA. `status` workflows are loaded only from the default + # branch, so a PR cannot suppress or rewrite this signal path. The status is + # only a wake-up signal; the gate re-reads live reviews before any write. + status: # pull-requests:write covers title/comment/label updates. # contents:write is required for convertPullRequestToDraft / @@ -27,40 +25,94 @@ permissions: contents: write pull-requests: write -concurrency: - # `issue_comment` events carry the issue number, not the PR number. The - # group is shared with the hygiene workflow: both read-modify-write the same - # consolidated gate comment, so serializing them under one key prevents a - # concurrent update from clobbering the other's section. - group: pr-gate-comment-${{ github.event.pull_request.number || github.event.issue.number }} - jobs: - enforce-target: - # `issue_comment` fires for comments on ANY issue or PR. This gate is - # write-capable, so only two trusted sources may start that path: a - # canonical maintainer (GUI-waiver case) or CodeRabbit's own PR status - # comment, whose create/edit event is used only as a signal to re-read the - # live review threads. All other pull_request_target events run normally. + 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 != 'issue_comment' || - (github.event.issue.pull_request != null && - (github.event.comment.user.login == 'coderabbitai[bot]' || - github.event.comment.author_association == 'OWNER' || - github.event.comment.author_association == 'COLLABORATOR' || - github.event.comment.author_association == 'MEMBER')) + (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_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 # the workflow's write-capable token. - ref: ${{ github.event_name == 'issue_comment' && github.event.repository.default_branch || github.event.pull_request.base.sha }} + ref: ${{ github.event_name == 'status' && github.event.repository.default_branch || github.event.pull_request.base.sha }} persist-credentials: false sparse-checkout: | .github/scripts @@ -68,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"); @@ -142,54 +196,29 @@ jobs: const TITLE_PREFIX = "[WRONG BRANCH] "; const LEGACY_COMMENT_MARKER = ""; const REVIEW_READY_LABEL = "review-ready"; + const GUI_SCREENSHOT_WAIVER_LABEL = "gui-screenshot-waived"; const MAINTAINERS_FILE = "MAINTAINERS.md"; - const CODE_RABBIT_LOGIN = "coderabbitai[bot]"; - const { owner, repo } = context.repo; - // `issue_comment` events carry the PR's issue object, not a - // `pull_request` object. The issue number is the PR number either - // way, so resolve it from whichever payload the event delivered. - const pull_number = - context.payload.pull_request?.number ?? - context.payload.issue?.number; + const resolvedPullNumber = process.env.RESOLVED_PULL_NUMBER ?? ""; + const pull_number = /^\d+$/.test(resolvedPullNumber) + ? Number.parseInt(resolvedPullNumber, 10) + : Number.NaN; - // Defensive re-check of the job-level guard. `issue_comment` events - // carry a `comment` object with the author's association. A normal - // user comment is trusted only when it comes from a canonical - // maintainer; CodeRabbit's own PR status comment is separately - // allowed as a signal to re-read live review threads. The comment - // body itself is never trusted as gate evidence. - if (context.eventName === "issue_comment") { - const isPrComment = - context.payload.issue?.pull_request != null; - const association = context.payload.comment?.author_association; - const commenter = context.payload.comment?.user?.login; - const isCodeRabbit = commenter === CODE_RABBIT_LOGIN; - // The association is a cheap prefilter, but OWNER/COLLABORATOR/ - // MEMBER is broader than this repository's canonical maintainer - // list. A collaborator or member who is not a maintainer must not - // start this write-capable gate. - const maintainerLogins = new Set( - readMaintainerLogins().map(login => login.toLowerCase()) - ); - const isCanonicalMaintainer = - typeof commenter === "string" && - maintainerLogins.has(commenter.toLowerCase()); - if ( - !isPrComment || - (!isCodeRabbit && - (![ - "OWNER", - "COLLABORATOR", - "MEMBER" - ].includes(association) || - !isCanonicalMaintainer)) - ) { - core.info( - "issue_comment is neither CodeRabbit nor a canonical maintainer on a PR; skipping the gate." - ); - return; - } + // `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; + } + + // 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; } const { data: pr } = await github.rest.pulls.get({ @@ -490,7 +519,7 @@ jobs: } } - const failures = collectPrQualityFailures({ + let failures = collectPrQualityFailures({ baseRef: pr.base.ref, allowedBases: ALLOWED_BASES, title: pr.title, @@ -511,7 +540,68 @@ 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 screenshotWaived = hasGuiOverride({ comments }); + 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" + ); + } + const screenshotWaived = + screenshotWaivedByLabel || hasGuiOverride({ comments }); + const screenshotWaiverNotice = screenshotWaived + ? (screenshotWaivedByLabel + ? `UI screenshot waived by the ${inlineCode(GUI_SCREENSHOT_WAIVER_LABEL)} label.` + : "UI screenshot waived by a maintainer comment.") + : null; // The readiness gate applies to contributors (no push permission). // Maintainers keep the failure-only contract: draft while quality @@ -558,14 +648,14 @@ 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 ?? - (context.eventName === "issue_comment" + (context.eventName === "status" ? "" : pr.head.sha); const completionHeadSha = @@ -946,9 +1036,7 @@ jobs: // the waiver flag, and the prefix notice when the bot owns it. const failureNotices = [ ...revalidationNotice, - ...(screenshotWaived - ? ["UI screenshot waived by a maintainer comment."] - : []), + ...(screenshotWaiverNotice ? [screenshotWaiverNotice] : []), ...(hasWrongBase && state.titlePrefixedByBot ? [`Its title has been prefixed with ${inlineCode(TITLE_PREFIX.trim())}.`] : []) @@ -1038,6 +1126,7 @@ jobs: checklistRequired, notices: [ ...revalidationNotice, + ...(screenshotWaiverNotice ? [screenshotWaiverNotice] : []), "This PR stays in draft until every box above is ticked." ] }); @@ -1057,6 +1146,7 @@ jobs: checklistRequired, notices: [ ...revalidationNotice, + ...(screenshotWaiverNotice ? [screenshotWaiverNotice] : []), "This PR stays in draft until every box above is ticked." ] }); @@ -1089,6 +1179,7 @@ jobs: checklistRequired, notices: [ ...revalidationNotice, + ...(screenshotWaiverNotice ? [screenshotWaiverNotice] : []), "Automatic draft conversion failed. Please convert this pull request to a draft manually until every box above is ticked." ] }); @@ -1144,13 +1235,14 @@ jobs: readyState.version = 1; const notices = [ + screenshotWaiverNotice, readyConversionFailed ? "Automatic ready-for-review conversion failed; please mark the pull request ready manually if it is still a draft." : readyConverted ? "This pull request has been marked Ready for Review." : "This pull request is already Ready for Review.", readyMoment - ? `CodeRabbit/Codex review was requested via the ${inlineCode(REVIEW_READY_LABEL)} label. If no review appears, comment ${inlineCode("@coderabbitai review")} to request one.` + ? `The ${inlineCode(REVIEW_READY_LABEL)} label marks this PR as ready; review automation runs independently. If no CodeRabbit review appears, comment ${inlineCode("@coderabbitai review")} to request one.` : "", notified && maintainers.length > 0 ? `Maintainers notified: ${maintainers @@ -1214,7 +1306,7 @@ jobs: actions: [], readiness, checklistRequired, - notices: [] + notices: screenshotWaiverNotice ? [screenshotWaiverNotice] : [] }); return; } @@ -1227,7 +1319,7 @@ jobs: actions: [], readiness, checklistRequired, - notices: [] + notices: screenshotWaiverNotice ? [screenshotWaiverNotice] : [] } ); return; diff --git a/.github/workflows/pr-hygiene.yml b/.github/workflows/pr-hygiene.yml index 7d42712cb..86838dd97 100644 --- a/.github/workflows/pr-hygiene.yml +++ b/.github/workflows/pr-hygiene.yml @@ -11,10 +11,9 @@ permissions: {} concurrency: # Shared with the enforce-target gate: both workflows read-modify-write the - # same consolidated gate comment, so one per-PR 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. + # 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 diff --git a/devlog/_plan/260808_workflow_comment_spam_hardening/000_plan.md b/devlog/_plan/260808_workflow_comment_spam_hardening/000_plan.md new file mode 100644 index 000000000..a12e5883a --- /dev/null +++ b/devlog/_plan/260808_workflow_comment_spam_hardening/000_plan.md @@ -0,0 +1,71 @@ +# Workflow comment-spam hardening implementation plan + +> **For agentic workers:** execute this plan test-first. Do not broaden workflow permissions or execute pull-request head code with a write-capable token. + +**Goal:** Reduce GitHub Actions noise and runner consumption caused by `issue_comment` while preserving issue-comment translation and the PR readiness gate's ability to invalidate a ready PR when CodeRabbit reports new findings. + +**Architecture:** Keep `issue_comment` only where GitHub offers no narrower native trigger: real-time issue-comment translation. Revalidate CodeRabbit readiness from its `CodeRabbit` commit status using the default-branch-only `status` event, then resolve the status SHA to exactly one open PR before the privileged gate writes anything. Make `gui-screenshot-waived` the immediate maintainer-controlled waiver trigger while preserving legacy maintainer-comment recognition on later PR events for compatibility. + +**Tech stack:** GitHub Actions YAML, `actions/github-script`, Bun tests, existing PR-gate scripts. + +## Global constraints + +- PR targets `dev`. +- Workflow changes become live only after promotion to default branch `main`. +- Never checkout or execute PR-head code in a workflow with write permissions. +- Preserve real-time non-English issue-comment translation. +- Preserve CodeRabbit/Codex review-thread verification as the source of truth; review/comment bodies are trigger signals only, never trusted gate evidence. +- Do not claim that a job-level `if` removes an `issue_comment` workflow-run entry: it only prevents runner allocation for filtered comments. + +## Task 1: Stop PR and bot comments from allocating issue-quality runners + +**Files:** +- Modify: `.github/workflows/enforce-issue-quality.yml` +- Modify: `tests/ci-workflows.test.ts` + +- [ ] Add regression assertions requiring the `translate-comment` job to run only for `issue_comment` events on real issues and non-bot authors. +- [ ] Run the focused workflow test and confirm it fails against the current workflow. +- [ ] Add the minimal job-level guard: exclude `github.event.issue.pull_request != null` and bot-authored comments before checkout/setup/AI steps. +- [ ] Re-run the focused workflow test and confirm it passes. + +## Task 2: Replace CodeRabbit status-comment gate triggers with a trusted commit-status signal + +**Files:** +- Modify: `.github/workflows/enforce-pr-target.yml` +- Replace: `tests/zz-pr-coderabbit-readiness-revalidation.test.ts` + +- [ ] Require no `issue_comment`, `pull_request_review`, or PR-controlled signal workflow for CodeRabbit revalidation. +- [ ] Consume CodeRabbit's successful `CodeRabbit` commit status through the default-branch-only `status` event. +- [ ] Resolve the status SHA with `listPullRequestsAssociatedWithCommit` and continue only when exactly one open PR has that SHA as its current head. +- [ ] Treat status-triggered runs as signal-only head evidence and re-read live review threads/bodies before any write. +- [ ] Keep the write-capable checkout pinned to the trusted default branch for status events. + +## Task 3: Move GUI screenshot waiver from maintainer comments to a label + +**Files:** +- Modify: `.github/workflows/enforce-pr-target.yml` +- Modify: `tests/ci-workflows.test.ts` +- Modify: `docs-site/src/content/docs/contributing/pr-quality.md` + +- [ ] Add regression assertions for `labeled` / `unlabeled` PR-target events and `gui-screenshot-waived` semantics. +- [ ] Confirm the new assertions fail against current behavior. +- [ ] Use `gui-screenshot-waived` as the only immediate GUI-waiver trigger, while preserving legacy maintainer-comment recognition on later PR events for compatibility. +- [ ] Document that the label is maintainer-controlled and that adding/removing it immediately re-evaluates the gate. +- [ ] Re-run focused workflow tests. + +## Task 4: Verification and PR + +- [ ] Run `bun test tests/zz-pr-coderabbit-readiness-revalidation.test.ts tests/ci-workflows.test.ts`. +- [ ] Run `node --test .github/scripts/*.test.cjs` because the gate still consumes those helpers. +- [ ] Run `bun run typecheck`. +- [ ] Run `git diff --check`. +- [ ] Verify the final diff contains no temporary implementation workflow or helper. +- [ ] Open a draft PR against `dev` with deployment note: event-driven workflow changes take effect only after promotion to `main`. + +## Expected effect + +- CodeRabbit PR status-comment edits no longer invoke `enforce-pr-target`. +- Ordinary maintainer PR comments no longer invoke `enforce-pr-target` merely to carry a GUI waiver. +- PR and bot comments still create an `Enforce issue quality` workflow-run record because GitHub cannot filter `issue_comment` by PR-vs-issue at trigger time, but the translation job is skipped before runner allocation. +- Real issue comments from humans continue to translate in real time. +- New CodeRabbit reviews can still invalidate a previously completed findings claim through CodeRabbit's commit status and a default-branch, write-capable gate without executing untrusted PR code or trusting an ambiguous SHA-to-PR association. diff --git a/docs-site/src/content/docs/contributing/pr-quality.md b/docs-site/src/content/docs/contributing/pr-quality.md index a7948d96b..e29619e5a 100644 --- a/docs-site/src/content/docs/contributing/pr-quality.md +++ b/docs-site/src/content/docs/contributing/pr-quality.md @@ -40,12 +40,12 @@ tells you exactly what to change: plan** (or equivalent substance). When the title or description mentions `gui`, the description must include a screenshot of the UI change; the check keeps the PR a draft and comments until the screenshot is present. A - maintainer (OWNER / COLLABORATOR / MEMBER — repository owners, - collaborators, and members) can waive the screenshot - requirement with an issue comment saying the change does not touch the GUI - (for example "no gui changes"); a contributor PR author cannot self-waive - (a maintainer who authors the PR can waive, but they already hold push - permission and are not gated by the contributor checklist). + maintainer can waive a false-positive GUI cue by adding the + `gui-screenshot-waived` label; adding or removing that label immediately + re-evaluates the gate. Legacy maintainer comments such as "no gui changes" + are still recognised on the next PR event for compatibility, but comments + themselves no longer trigger the privileged PR gate. A contributor cannot + self-waive the screenshot requirement. Contributor PRs (authors without repository push permission) open in draft and stay there until a four-box review-readiness checklist in the description is complete: local CI green, the branch on the latest `dev` @@ -71,6 +71,13 @@ tells you exactly what to change: unticks the matching box and keeps the PR a draft. When the checklist is complete and every gate is green, the gate adds a `review-ready` label as a visible status marker at the ready moment. + CodeRabbit status-comment edits do not trigger the PR gate. CodeRabbit's + successful `CodeRabbit` commit status wakes the trusted default-branch gate + through the `status` event. The gate maps that status SHA to exactly one open + PR whose current head still matches, then re-reads live review threads and + review bodies before changing checklist, label, comment, or draft state. An + ambiguous or stale SHA association is ignored, and no PR-head code is + executed with the gate's write-capable token. - **Hygiene.** Behavior changes need a test; new lint or type suppressions, focused or skipped tests, empty catch blocks, edited generated output, and a @@ -98,11 +105,12 @@ right; say why when it is wrong. It does not block a merge. ### When a workflow change takes effect -`enforce-target` and `label` run on `pull_request_target`, which GitHub always -loads from the repository **default branch**. A change to either takes effect -only after it is promoted to `main` — merging it to `dev` does not change live -behavior. The cross-platform CI workflow runs on `pull_request` and takes effect -as soon as it is on the branch being targeted. +`enforce-target` and `label` use trusted default-branch automation. The PR gate +runs on `pull_request_target` and on CodeRabbit `status` events, both loaded +from the repository default branch; the write-capable behavior therefore +changes only after the gate revision is promoted to `main`. The cross-platform +CI workflow runs on `pull_request` and takes effect as soon as it is on the +branch being targeted. ## Sponsored surfaces diff --git a/structure/06_docs-and-release.md b/structure/06_docs-and-release.md index 8274487b5..63b053032 100644 --- a/structure/06_docs-and-release.md +++ b/structure/06_docs-and-release.md @@ -44,7 +44,7 @@ bun run build | `.github/workflows/release.yml` | Manual dispatch only | npm publish/dry-run workflow. It requires the exact `GITHUB_SHA` to have a successful Cross-platform CI run before publish or dry-run. | | `.github/workflows/deploy-docs.yml` | `push` to `main` touching `docs-site/**` or the workflow, or manual dispatch | Build and publish the Astro/Starlight docs site to GitHub Pages. | | `.github/workflows/service-lifecycle.yml` | `pull_request` to `main`/`dev` and `push`, both filtered on the service path set (`src/service.ts`, `src/cli.ts`, `src/cli/index.ts`, `src/lib/bun-runtime.ts`, `package.json`, `bun.lock`, the workflow), or manual dispatch | Service-lifecycle smoke on three platforms: Linux systemd, macOS launchd, and Windows Scheduled Tasks. Each installs, verifies, stops via `ocx stop`, and uninstalls. The path list is kept in sync with the `release.yml` service-gate regex. | -| `.github/workflows/enforce-pr-target.yml` | `pull_request_target` (opened, reopened, edited, ready_for_review, synchronize) | The `enforce-target` gate: rejects pull requests whose head ancestry sits on the `main` tip while far behind `dev`, rejects empty or malformed descriptions, requires a GUI screenshot when the title/body mentions `gui` (waivable by a maintainer comment), keeps contributor PRs in draft until a four-box readiness checklist is complete, verifies the CI / latest-dev / Codex+CodeRabbit-findings claims (review threads plus current-head CodeRabbit review-body findings outside the diff range), and adds a `review-ready` status label at the ready moment. Stacked child PRs targeting another open PR's head skip the wrong-base gate. | +| `.github/workflows/enforce-pr-target.yml` | `pull_request_target` (opened, reopened, edited, labeled, unlabeled, ready_for_review, synchronize) plus default-branch `status` events filtered to successful `CodeRabbit` statuses | The `enforce-target` gate: rejects pull requests whose head ancestry sits on the `main` tip while far behind `dev`, rejects empty or malformed descriptions, requires a GUI screenshot when the title/body mentions `gui` (immediately waivable with the maintainer-controlled `gui-screenshot-waived` label; legacy maintainer comments remain compatibility evidence on later PR events), keeps contributor PRs in draft until a four-box readiness checklist is complete, verifies the CI / latest-dev / Codex+CodeRabbit-findings claims (review threads plus current-head CodeRabbit review-body findings outside the diff range), and adds a `review-ready` status label at the ready moment. CodeRabbit status SHAs must resolve to exactly one open current-head PR before writes. Stacked child PRs targeting another open PR's head skip the wrong-base gate. | | `.github/workflows/enforce-issue-quality.yml` | `issues` (opened, edited, reopened), `issue_comment` (created, edited), or manual dispatch with an issue number | Issue-template compliance gate. | | `.github/workflows/issue-quality-tests.yml` | `pull_request` and `push` filtered on the issue/PR automation scripts, templates, and their workflows | Tests the issue and PR automation scripts themselves, so the gates cannot rot silently. | | `.github/workflows/issue-triage.yml` | `issues` (opened) | Duplicate detection and triage labeling for new issues. | diff --git a/tests/ci-workflows.test.ts b/tests/ci-workflows.test.ts index 67be862ea..fa6ca5cc7 100644 --- a/tests/ci-workflows.test.ts +++ b/tests/ci-workflows.test.ts @@ -679,6 +679,10 @@ describe("GitHub Actions hardening", () => { on?: { pull_request_target?: { types?: string[] }; issue_comment?: { types?: string[] }; + workflow_run?: { workflows?: string[]; types?: string[] }; + pull_request_review?: { types?: string[] }; + pull_request_review_comment?: { types?: string[] }; + status?: unknown; }; permissions?: Record | string; concurrency?: Record & { group?: string }; @@ -823,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", @@ -839,14 +842,12 @@ describe("GitHub Actions hardening", () => { // workflow YAML under a write token against base-pinned scripts — a // mismatch that crashes the gate and breaks the trusted-base model. // - // `issue_comment` is the one extra trigger: a maintainer's GUI-waiver - // comment ("not touching gui") must re-run the gate, and issue comments - // are not a `pull_request_target` activity type. It never touches PR head - // code — the checkout stays on the trusted base/default branch — so it - // does not open the escalation path review events would. + // `status` is the only extra trigger. CodeRabbit publishes a legacy + // commit status; this privileged workflow is loaded from the default branch + // and re-reads live review evidence before any mutation. expect(Object.keys(workflow.on ?? {}).sort()).toEqual([ - "issue_comment", "pull_request_target", + "status", ]); // And the trigger is exactly a `types:` list — nothing else. @@ -859,7 +860,7 @@ describe("GitHub Actions hardening", () => { // additive, both look like ordinary scoping in a diff, and neither failed a // single assertion. expect(Object.keys(workflow.on?.pull_request_target ?? {})).toEqual(["types"]); - expect(Object.keys(workflow.on?.issue_comment ?? {})).toEqual(["types"]); + expect(Object.prototype.hasOwnProperty.call(workflow.on ?? {}, "status")).toBe(true); // Exactly the scopes this gate needs. `pull-requests: write` covers title // and comment updates. `contents: write` is required for the draft GraphQL @@ -871,53 +872,59 @@ describe("GitHub Actions hardening", () => { "pull-requests": "write", }); - // One run per PR, so two rapid events cannot race on the title/draft state, - // and no `cancel-in-progress` — cancelling the in-flight run mid-mutation is - // how the bot ends up having prefixed the title but not recorded that it did. - // `issue_comment` events carry the PR's number under `issue`, not - // `pull_request`, so the group resolves from whichever payload exists. - expect(workflow.concurrency).toEqual({ - group: - "pr-gate-comment-${{ github.event.pull_request.number || github.event.issue.number }}", + 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.number }}", ); - // 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 restricts the `issue_comment` trigger to maintainer comments on - // PRs — a comment on a plain issue, or from a non-maintainer, must not - // start this write-capable gate. On `pull_request_target` events the guard - // is always true, so it never disables the gate. - // 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 != 'issue_comment'"); - expect(job["if"]).toContain("github.event.issue.pull_request != null"); - expect(job["if"]).toContain("coderabbitai[bot]"); - expect(job["if"]).toContain("'OWNER'"); - expect(job["if"]).toContain("'COLLABORATOR'"); - expect(job["if"]).toContain("'MEMBER'"); + 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. @@ -933,19 +940,21 @@ describe("GitHub Actions hardening", () => { "sparse-checkout", ]); expect(checkout.with).toEqual({ - // The event's base commit, not the repository default: pull_request_target - // runs this workflow from the base revision, and the scripts must match - // it — a merged gate would otherwise run against pre-promotion `main` - // scripts. The immutable SHA pins the checkout to the event's base commit. + // Normal PR events pin scripts to the event's base SHA. A trusted + // `status` event has no pull_request payload, so it loads scripts from + // the same default-branch trust boundary that owns the event. ref: - "${{ github.event_name == 'issue_comment' && github.event.repository.default_branch || github.event.pull_request.base.sha }}", + "${{ github.event_name == 'status' && github.event.repository.default_branch || github.event.pull_request.base.sha }}", "persist-credentials": false, // MAINTAINERS.md rides along so the completion ping reads the canonical // maintainer list from the same trusted base revision as the scripts. "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 @@ -984,20 +993,18 @@ describe("GitHub Actions hardening", () => { const types = workflow.on?.pull_request_target?.types ?? []; expect([...types].sort()).toEqual([ "edited", + "labeled", "opened", "ready_for_review", "reopened", "synchronize", + "unlabeled", ]); - // A maintainer's GUI-waiver comment must re-run the gate. Issue comments - // are delivered as the `issue_comment` event, which is the only way the - // waiver can take effect without a PR edit or push. - expect(workflow.on?.issue_comment?.types).toBeDefined(); - expect([...(workflow.on?.issue_comment?.types ?? [])].sort()).toEqual([ - "created", - "edited", - ]); + // GUI-waiver labels re-run immediately through pull_request_target. + // CodeRabbit reviews re-run through the default-branch status event instead + // of using bot status-comment edits as workflow synchronisation. + expect(Object.prototype.hasOwnProperty.call(workflow.on ?? {}, "status")).toBe(true); // Review events must NOT be added: they load the workflow from the PR // head branch, breaking the base-pinned checkout (`pull_request_review` @@ -1018,18 +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. - // `issue_comment` events carry the number under `issue`, so the resolution - // falls back from the PR object to the issue object — both are immutable - // event fields, never author-controlled title text. - expect(script).toMatch( - /const pull_number =\s*context\.payload\.pull_request\?\.number \?\?\s*context\.payload\.issue\?\.number;/, - ); - expect(script.match(/pull_number\s*=/g) ?? []).toHaveLength(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 @@ -1129,6 +1129,8 @@ describe("GitHub Actions hardening", () => { name !== "github.rest.pulls.listReviews" && 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", ); @@ -2659,12 +2661,7 @@ describe("GitHub Actions hardening", () => { expect(lastEnforcerCommentBody(result)).toContain("UI screenshot waived by a maintainer comment"); }); - test("an issue_comment event re-runs the gate and the waiver takes effect", async () => { - // This is the scenario that PR #1119 hit: a maintainer posts the waiver - // as an issue comment, and the gate must re-evaluate on that event — - // `pull_request_target` types do not include issue comments, so the - // separate `issue_comment` trigger carries it. The payload has no - // `pull_request` object; the PR number comes from `issue.number`. + test("the gui-screenshot-waived label clears the sole screenshot failure and reports the waiver", async () => { const result = await run({ pr: { base: { ref: "dev" }, @@ -2677,51 +2674,115 @@ describe("GitHub Actions hardening", () => { "- Ran bun test tests/ci-workflows.test.ts", ].join("\n"), }, - eventName: "issue_comment", - eventAction: "created", - comments: [ - { id: 1, user: { login: "wibias" }, author_association: "COLLABORATOR", body: "not touching gui" }, - ], + 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); expect(lastEnforcerCommentBody(result)).not.toContain("UI screenshot required"); - expect(lastEnforcerCommentBody(result)).toContain("UI screenshot waived by a maintainer comment"); + expect(lastEnforcerCommentBody(result)).toContain("UI screenshot waived by the `gui-screenshot-waived` label"); }); - test("an issue_comment rerun does not accept a checklist with no recorded head", async () => { - // `issue_comment` events carry no `pull_request.head.sha`. A contributor - // who ticked the readiness checklist, then pushed, must not have that - // stale attestation accepted by a maintainer-waiver comment rerun — the - // gate must reset the boxes and re-draft. + + test("the gui-screenshot-waived label is reported after it clears the sole failure on the ready path", async () => { const result = await run({ pr: { base: { ref: "dev" }, + draft: true, title: "GUI: fix provider list spacing", body: readinessChecklistBody(4), }, - eventName: "issue_comment", - eventAction: "created", - comments: [ - { id: 1, user: { login: "wibias" }, author_association: "COLLABORATOR", body: "not touching gui" }, - readinessComment({ - version: 2, - autoDraftedByBot: false, - maintainersPinged: true, - completedAtHeadSha: null, - }), - ], + 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); + expect(lastEnforcerCommentBody(result)).toContain("## ✅ READY"); + expect(lastEnforcerCommentBody(result)).not.toContain("UI screenshot required"); + expect(lastEnforcerCommentBody(result)).toContain("UI screenshot waived by the `gui-screenshot-waived` label"); + }); + + 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" }, + 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" }, + }], + }); + + // 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("a missing resolved PR number fails closed before PR lookup", async () => { + const result = await run({ + pr: { base: { ref: "dev" } }, + resolvedPullNumber: "", }); - // The comment-triggered rerun delivers no head SHA, so the completed - // checklist cannot be attributed to the live head: the gate resets the - // boxes and keeps the PR in draft. - const resetBody = callsTo(result, "pulls.update") as [{ body: string }]; - expect(resetBody[0]!.body).toContain(CHECKLIST_START); - expect(resetBody[0]!.body).not.toContain("- [x]"); - expect(resetBody[0]!.body).toContain("- [ ] All CI tests are green on my local testing."); - expect(resetBody[0]!.body).toContain("- [ ] My PR is ready for review."); + 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, "graphql")).toEqual([]); + }); + + test("the write gate consumes the resolved PR number without re-resolving status SHA", async () => { + const result = await run({ + pr: { base: { ref: "dev" }, number: 4242 }, + eventName: "status", + resolvedPullNumber: 4242, + }); + + 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 9f4710a21..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; @@ -83,12 +91,21 @@ export type RunOptions = { */ eventAction?: string; /** - * Webhook event name. Defaults to `"pull_request_target"`. Pass - * `"issue_comment"` to exercise the GUI-waiver re-run path: the payload then - * carries `issue` and `comment` (never `pull_request`), exactly as GitHub - * delivers an issue comment on a PR. + * Webhook event name. Defaults to `"pull_request_target"`. `issue_comment` + * remains available for fail-closed compatibility tests; `status` models the + * default-branch CodeRabbit wake-up path. */ eventName?: string; + /** SHA carried by a `status` event. Defaults to the live PR head SHA. */ + statusSha?: string; + /** Legacy commit-status context. Defaults to `CodeRabbit`. */ + statusContext?: string; + /** Legacy commit-status state. Defaults to `success`. */ + statusState?: string; + /** Shorthand for a single associated-PR response page. */ + associatedPullRequests?: unknown[]; + /** Page-specific PRs returned by repos.listPullRequestsAssociatedWithCommit. */ + associatedPullRequestPages?: unknown[][]; /** * `author_association` of the commenter on an `issue_comment` event. * Defaults to `"COLLABORATOR"`. The gate only re-runs for maintainer @@ -116,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[]; /** @@ -182,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; }; /** @@ -578,10 +609,20 @@ 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] : []); - const paginatePageCount = Math.max(pages.length, openPullPages.length, 1); + const associatedPullRequestPages: unknown[][] = + options.associatedPullRequestPages ?? [options.associatedPullRequests ?? [pr]]; + const paginatePageCount = Math.max( + pages.length, + issueEventPages.length, + openPullPages.length, + associatedPullRequestPages.length, + 1, + ); /** * Record the call, then either reject or return a plausible payload. Every @@ -684,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, {}), @@ -706,6 +751,14 @@ export async function runEnforcePrTarget( const basehead = String((args as { basehead?: string })?.basehead ?? ""); return respond("repos.compareCommitsWithBasehead", args, compareResult(basehead)); }, + listPullRequestsAssociatedWithCommit: (args: unknown) => { + const page = Number((args as { page?: number })?.page ?? 1); + return respond( + "repos.listPullRequestsAssociatedWithCommit", + args, + associatedPullRequestPages[page - 1] ?? [], + ); + }, }, }; @@ -763,8 +816,8 @@ export async function runEnforcePrTarget( respond("request", { route, params }); /** * `github.paginate(fn, params)` — walk every page and concatenate, the way - * Octokit does. Page count covers both comment and open-PR fixtures so a - * stacked parent on page two is still visible. + * Octokit does. Page count covers comment, open-PR, and associated-PR + * fixtures so a relevant record on page two is still visible. */ paginate = Object.assign( async (fn: (args: unknown) => Promise<{ data: unknown[] }>, params: unknown) => { @@ -870,7 +923,13 @@ export async function runEnforcePrTarget( author_association: options.commentAuthorAssociation ?? "COLLABORATOR", }, } - : { pull_request: eventPr }), + : options.eventName === "status" + ? { + sha: options.statusSha ?? pr.head.sha, + context: options.statusContext ?? "CodeRabbit", + state: options.statusState ?? "success", + } + : { pull_request: eventPr }), repository: { id: 987654321, name: "opencodex", @@ -880,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, }; @@ -1007,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, @@ -1033,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 e65183cb8..6071489d5 100644 --- a/tests/zz-pr-coderabbit-readiness-revalidation.test.ts +++ b/tests/zz-pr-coderabbit-readiness-revalidation.test.ts @@ -1,54 +1,107 @@ import { describe, expect, test } from "bun:test"; +type WorkflowJob = { + if?: string; + "runs-on"?: string; + steps?: Array<{ + name?: string; + uses?: string; + run?: string; + env?: Record; + with?: Record; + }>; +}; + type Workflow = { on?: { issue_comment?: { types?: string[] }; + pull_request_target?: { types?: string[] }; + pull_request_review?: { types?: string[] }; + workflow_run?: { workflows?: string[]; types?: string[] }; + status?: unknown; }; - jobs?: Record< - string, - { - if?: string; - steps?: Array<{ - name?: string; - with?: Record; - }>; - } - >; + jobs?: Record; }; -describe("CodeRabbit readiness revalidation", () => { - test("CodeRabbit PR status comments can rerun the findings gate", async () => { +describe("workflow comment-spam hardening", () => { + test("PR gate consumes CodeRabbit commit status from the trusted default branch", async () => { const text = await Bun.file( new URL("../.github/workflows/enforce-pr-target.yml", import.meta.url), ).text(); const workflow = Bun.YAML.parse(text) as Workflow; - expect(workflow.on?.issue_comment?.types).toEqual(["created", "edited"]); + expect(workflow.on?.issue_comment).toBeUndefined(); + expect(workflow.on?.pull_request_review).toBeUndefined(); + expect(workflow.on?.workflow_run).toBeUndefined(); + expect(Object.prototype.hasOwnProperty.call(workflow.on ?? {}, "status")).toBe(true); + expect(workflow.on?.pull_request_target?.types).toEqual(expect.arrayContaining([ + "edited", + "labeled", + "ready_for_review", + "synchronize", + "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"]; - expect(job).toBeDefined(); - expect(job?.if).toContain("github.event.issue.pull_request != null"); - expect(job?.if).toContain("github.event.comment.user.login == 'coderabbitai[bot]'"); + expect(job?.if).toBe("needs.resolve-pr.outputs.pull-number != ''"); const checkoutStep = job?.steps?.find( step => step.name === "Checkout trusted PR-quality scripts", ); expect(checkoutStep?.with?.ref).toBe( - "${{ github.event_name == 'issue_comment' && github.event.repository.default_branch || github.event.pull_request.base.sha }}", + "${{ github.event_name == 'status' && github.event.repository.default_branch || github.event.pull_request.base.sha }}", ); const gateStep = job?.steps?.find( step => step.name === "Enforce PR target, ancestry, and description", ); const script = gateStep?.with?.script ?? ""; - - expect(script).toContain('const CODE_RABBIT_LOGIN = "coderabbitai[bot]"'); - expect(script).toContain("const isCodeRabbit = commenter === CODE_RABBIT_LOGIN"); - expect(script).toContain("!isCodeRabbit"); - expect(script).toContain("isCanonicalMaintainer"); - expect(script).toMatch( - /!isPrComment\s*\|\|\s*\(\s*!isCodeRabbit\s*&&\s*\(\s*!\[[\s\S]{0,300}?\.includes\(association\)\s*\|\|\s*!isCanonicalMaintainer\s*\)\s*\)/, + 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"); }); + + test("issue-comment translation rejects PR and bot comments before runner allocation", async () => { + const text = await Bun.file( + new URL("../.github/workflows/enforce-issue-quality.yml", import.meta.url), + ).text(); + const workflow = Bun.YAML.parse(text) as Workflow; + const jobIf = workflow.jobs?.["translate-comment"]?.if ?? ""; + + expect(jobIf).toContain("github.event_name == 'issue_comment'"); + expect(jobIf).toContain("github.event.issue.pull_request == null"); + expect(jobIf).toContain("github.event.comment.user.type != 'Bot'"); + }); + + test("contributor docs describe the label waiver and commit-status trust boundary", async () => { + const docs = await Bun.file( + new URL("../docs-site/src/content/docs/contributing/pr-quality.md", import.meta.url), + ).text(); + + expect(docs).toContain("gui-screenshot-waived"); + expect(docs).toContain("`CodeRabbit` commit status"); + expect(docs).toContain("`status` event"); + expect(docs).toContain("exactly one open"); + expect(docs).toContain("CodeRabbit status-comment edits do not trigger the PR gate"); + }); });