From 0b69b962ffe450f54901a71d9a71c535a05b9c7d Mon Sep 17 00:00:00 2001 From: Wibias <37517432+Wibias@users.noreply.github.com> Date: Fri, 14 Aug 2026 00:13:28 +0200 Subject: [PATCH] tmp: rebase PR 1633 onto dev (#12) * fix(pr-quality): break CodeRabbit review feedback loop in gate comment The readiness gate wrote a literal '@coderabbitai review' command into its status comment, and it rewrote an unchanged comment body on every wake. - Remove the inline CodeRabbit command from the ready notice. - Skip updateComment when the rebuilt body is unchanged. - Add regression tests for both behaviors. * test(pr-quality): cover unchanged CodeRabbit status wake * test(pr-quality): keep status-wake regression focused * test(pr-quality): cover legacy migration on no-op wake * test(pr-quality): reproduce stale gate comment body * fix(pr-quality): track gate comment body after writes * test(pr-quality): restore trust-boundary context * test(pr-quality): pin repeated failure comment target * test: fix gate body helper --- .github/scripts/enforce-pr-target.test.cjs | 20 ++ .github/workflows/enforce-pr-target.yml | 15 +- ...-coderabbit-readiness-revalidation.test.ts | 172 ++++++++++++++++++ 3 files changed, 205 insertions(+), 2 deletions(-) diff --git a/.github/scripts/enforce-pr-target.test.cjs b/.github/scripts/enforce-pr-target.test.cjs index 499306d009..f975eb39e0 100644 --- a/.github/scripts/enforce-pr-target.test.cjs +++ b/.github/scripts/enforce-pr-target.test.cjs @@ -112,6 +112,26 @@ describe("enforce-pr-target workflow", () => { assert.match(workflow, /reviewReadyDesired/); }); + it("does not embed a literal CodeRabbit review command in the ready notice", () => { + // A literal "@coderabbitai review" inside the gate comment is executed by + // CodeRabbit as a review command even when rendered as inline code. Its + // success status then wakes this workflow again, which rewrites the same + // comment, which CodeRabbit reads as a new command -- a self-sustaining + // loop that only stops on CodeRabbit's per-hour rate limit. The ready + // notice must describe the label without issuing a command (PR #1630). + assert.doesNotMatch(workflow, /coderabbitai review/); + }); + + it("does not rewrite the gate comment when the rebuilt body is unchanged", () => { + // The ready-path rebuild is deterministic: on a CodeRabbit status wake the + // gate recomputes the same READY body and would call updateComment on it. + // That no-op edit is still a mutation event to review bots and restarts the + // loop above, so the upsert must skip the write when body equals the posted + // comment body (PR #1630). + assert.match(workflow, /if \(gateComment\?\.body === body\)/); + assert.match(workflow, /let body = buildGateCommentBody/); + }); + it("keeps CodeRabbit auto-review unfiltered so maintainer PRs are not starved", () => { // A positive `labels:` filter under `reviews.auto_review` in // `.coderabbit.yaml` would restrict ALL automatic reviews to PRs carrying diff --git a/.github/workflows/enforce-pr-target.yml b/.github/workflows/enforce-pr-target.yml index 5cf5f84626..a26ff02273 100644 --- a/.github/workflows/enforce-pr-target.yml +++ b/.github/workflows/enforce-pr-target.yml @@ -300,7 +300,7 @@ jobs: // One consolidated comment. The gate finds its own comment by the // single GATE_MARKER; the legacy enforcer/readiness markers are // matched only to migrate pre-consolidation PRs. - const gateComment = comments.find( + let gateComment = comments.find( comment => comment.user?.login === "github-actions[bot]" && comment.body?.includes(GATE_MARKER) @@ -406,12 +406,22 @@ jobs: ); } if (gateCommentId) { + // Skip the write when the rebuilt body matches what is already + // posted. A no-op comment edit is still a mutation event to + // review bots, so updating an identical body would re-wake the + // CodeRabbit status signal that triggered this run and create + // a self-sustaining loop. + if (gateComment?.body === body) { + await migrateLegacyCommentsIfNeeded(); + return; + } await github.rest.issues.updateComment({ owner, repo, comment_id: gateCommentId, body }); + gateComment.body = body; await migrateLegacyCommentsIfNeeded(); return; } @@ -422,6 +432,7 @@ jobs: body }); gateCommentId = created.data.id; + gateComment = { id: gateCommentId, body }; await migrateLegacyCommentsIfNeeded(); } @@ -1351,7 +1362,7 @@ jobs: ? "This pull request has been marked Ready for Review." : "This pull request is already Ready for Review.", readyMoment - ? `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.` + ? `The ${inlineCode(REVIEW_READY_LABEL)} label marks this PR as ready; review automation runs independently.` : "", notified && maintainers.length > 0 ? `Maintainers notified: ${maintainers diff --git a/tests/zz-pr-coderabbit-readiness-revalidation.test.ts b/tests/zz-pr-coderabbit-readiness-revalidation.test.ts index 560ebe6d3e..d4db288ba5 100644 --- a/tests/zz-pr-coderabbit-readiness-revalidation.test.ts +++ b/tests/zz-pr-coderabbit-readiness-revalidation.test.ts @@ -1,4 +1,8 @@ import { describe, expect, test } from "bun:test"; +import { + callsTo, + runEnforcePrTarget, +} from "./helpers/enforce-pr-target-harness"; type WorkflowJob = { if?: string; @@ -23,6 +27,72 @@ type Workflow = { jobs?: Record; }; +const GATE_MARKER = ""; +const CHECKLIST_START = ""; +const CHECKLIST_END = ""; +const CHECKLIST_ITEMS = [ + "All CI tests are green on my local testing.", + "I pushed my PR to the latest dev commit.", + "I resolved all correct Codex and CodeRabbit findings.", + "My PR is ready for review.", +]; + +const MAINTAINERS_FIXTURE = [ + "## Current maintainers", + "", + "| GitHub account | Project role | Responsibilities |", + "| --- | --- | --- |", + "| [@lidge-jun](https://github.com/lidge-jun) | Project owner | x |", + "| [@Ingwannu](https://github.com/Ingwannu) | Maintainer | x |", + "| [@Wibias](https://github.com/Wibias) | Maintainer | x |", +].join("\n"); + +function completedChecklistBody(): string { + const description = [ + "## Summary", + "", + "Fix the PR-quality gate so an unchanged CodeRabbit status wake cannot rewrite its own READY comment.", + "", + "## Test plan", + "", + "- Run the PR-quality regression tests.", + ].join("\n"); + return [ + description, + CHECKLIST_START, + "## Review readiness checklist", + "", + ...CHECKLIST_ITEMS.map(item => `- [x] ${item}`), + CHECKLIST_END, + ].join("\n"); +} + +async function readGateScript(): Promise { + 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; + const script = workflow.jobs?.["enforce-target"]?.steps?.find( + step => step.name === "Enforce PR target, ancestry, and description", + )?.with?.script; + if (typeof script !== "string") { + throw new Error("enforce-target step has no inline script"); + } + return script; +} + +function gateBodyFrom( + result: Awaited>, +): string { + const updates = callsTo(result, "issues.updateComment") as Array<{ body: string }>; + const creates = callsTo(result, "issues.createComment") as Array<{ body: string }>; + const body = updates.at(-1)?.body ?? creates.at(-1)?.body; + if (!body?.includes(GATE_MARKER)) { + throw new Error("scenario recorded no PR gate comment body"); + } + return body; +} + describe("workflow comment-spam hardening", () => { test("PR gate consumes CodeRabbit commit status from the trusted default branch", async () => { const text = await Bun.file( @@ -85,6 +155,108 @@ describe("workflow comment-spam hardening", () => { expect(script).toContain("unresolvedFindingsClaim"); }); + test("an unchanged READY comment is not rewritten on a CodeRabbit status wake", async () => { + const script = await readGateScript(); + const body = completedChecklistBody(); + + const initial = await runEnforcePrTarget(script, { + pr: { base: { ref: "dev" }, draft: true, body }, + maintainersFile: MAINTAINERS_FIXTURE, + }); + const transitionBody = gateBodyFrom(initial); + + const steady = await runEnforcePrTarget(script, { + pr: { base: { ref: "dev" }, draft: false, body }, + maintainersFile: MAINTAINERS_FIXTURE, + labels: ["review-ready"], + comments: [{ + id: 7, + user: { login: "github-actions[bot]" }, + body: transitionBody, + }], + }); + const steadyBody = gateBodyFrom(steady); + expect(steadyBody).toContain("This pull request is already Ready for Review."); + expect(steadyBody).not.toContain("@coderabbitai review"); + + const statusWake = await runEnforcePrTarget(script, { + pr: { base: { ref: "dev" }, draft: false, body }, + eventName: "status", + maintainersFile: MAINTAINERS_FIXTURE, + labels: ["review-ready"], + comments: [ + { + id: 7, + user: { login: "github-actions[bot]" }, + body: steadyBody, + }, + { + id: 8, + user: { login: "github-actions[bot]" }, + body: "\nlegacy enforcer comment", + }, + { + id: 9, + user: { login: "github-actions[bot]" }, + body: "\nlegacy readiness comment", + }, + ], + }); + + expect(callsTo(statusWake, "issues.updateComment")).toEqual([]); + expect(callsTo(statusWake, "issues.createComment")).toEqual([]); + expect(callsTo(statusWake, "issues.addLabels")).toEqual([]); + expect(callsTo(statusWake, "issues.removeLabel")).toEqual([]); + expect(callsTo(statusWake, "pulls.update")).toEqual([]); + expect(callsTo(statusWake, "issues.deleteComment")).toEqual([ + { owner: "lidge-jun", repo: "opencodex", comment_id: 8 }, + { owner: "lidge-jun", repo: "opencodex", comment_id: 9 }, + ]); + + const graphqlCalls = callsTo(statusWake, "graphql") as Array<{ query: string }>; + expect(graphqlCalls.some(call => call.query.includes("convertPullRequestToDraft"))).toBe(false); + expect(graphqlCalls.some(call => call.query.includes("markPullRequestReadyForReview"))).toBe(false); + }); + + test("a repeated draft-conversion failure restores the failure body after its checkpoint write", async () => { + const script = await readGateScript(); + const body = completedChecklistBody().replace( + "- [x] My PR is ready for review.", + "- [ ] My PR is ready for review.", + ); + + const firstFailure = await runEnforcePrTarget(script, { + pr: { base: { ref: "dev" }, draft: false, body }, + maintainersFile: MAINTAINERS_FIXTURE, + failGraphqlOn: ["convertPullRequestToDraft"], + }); + const previousFailureBody = gateBodyFrom(firstFailure); + expect(previousFailureBody).toContain('"autoDraftedByBot":false'); + expect(previousFailureBody).toContain("Automatic draft conversion failed"); + + const repeatedFailure = await runEnforcePrTarget(script, { + pr: { base: { ref: "dev" }, draft: false, body }, + maintainersFile: MAINTAINERS_FIXTURE, + failGraphqlOn: ["convertPullRequestToDraft"], + comments: [{ + id: 7, + user: { login: "github-actions[bot]" }, + body: previousFailureBody, + }], + }); + + const updates = callsTo(repeatedFailure, "issues.updateComment") as Array<{ + body: string; + comment_id: number; + }>; + expect(callsTo(repeatedFailure, "issues.createComment")).toEqual([]); + expect(updates).toHaveLength(2); + expect(updates.map(update => update.comment_id)).toEqual([7, 7]); + expect(updates[0]?.body).toContain('"autoDraftedByBot":true'); + expect(updates[1]?.body).toContain('"autoDraftedByBot":false'); + expect(updates[1]?.body).toContain("Automatic draft conversion failed"); + }); + 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),