diff --git a/.github/scripts/pr-quality-messages.cjs b/.github/scripts/pr-quality-messages.cjs index e9b642b57..6d9b2fff0 100644 --- a/.github/scripts/pr-quality-messages.cjs +++ b/.github/scripts/pr-quality-messages.cjs @@ -259,10 +259,14 @@ function failureSummary(failures, { pr }) { } /** The notice shown when the gate's own claim check disproves a ticked box. */ -function buildClaimCheckNotice(violations, _liveHeadSha) { +function buildClaimCheckNotice(violations, liveHeadSha) { const lines = []; for (const code of violations) { - if (code === "latest_dev") { + if (code === "ci_green") { + lines.push( + `GitHub CI is not green on the current head ${inlineCode(liveHeadSha.slice(0, 7))}; the **CI green** box has been unticked.` + ); + } else if (code === "latest_dev") { lines.push( `The PR is more than ${READINESS_LATEST_DEV_BEHIND_MAX} commits behind ${inlineCode("dev")}; the **latest dev** box has been unticked.` ); diff --git a/.github/scripts/pr-quality-messages.test.cjs b/.github/scripts/pr-quality-messages.test.cjs index ca6e056bd..2cd474df7 100644 --- a/.github/scripts/pr-quality-messages.test.cjs +++ b/.github/scripts/pr-quality-messages.test.cjs @@ -213,21 +213,23 @@ describe("buildStaleNotice", () => { }); describe("buildClaimCheckNotice", () => { - it("names the latest-dev violation and the reset action", () => { + it("names each violated claim and the reset action", () => { const notice = buildClaimCheckNotice( - ["latest_dev"], + ["ci_green", "latest_dev"], "3f1c0de0a6a4d0a3f9a1b2c3d4e5f60718293a4b", ); - assert.match(notice[0], /more than 10 commits behind `dev`/); - assert.match(notice[0], /\*\*latest dev\*\* box has been unticked/); - assert.match(notice[1], /reset: re-test against the latest code/); + assert.match(notice[0], /CI is not green on the current head `3f1c0de`/); + assert.match(notice[0], /\*\*CI green\*\* box has been unticked/); + assert.match(notice[1], /more than 10 commits behind `dev`/); + assert.match(notice[1], /\*\*latest dev\*\* box has been unticked/); + assert.match(notice[2], /reset: re-test against the latest code/); }); - it("ignores a stale ci_green code without inventing GitHub-CI copy", () => { + it("handles a single violation", () => { const notice = buildClaimCheckNotice(["ci_green"], "a".repeat(40)); - assert.equal(notice.length, 1); - assert.match(notice[0], /has been reset/); - assert.doesNotMatch(notice[0], /CI is not green/); + assert.equal(notice.length, 2); + assert.match(notice[0], /CI is not green/); + assert.match(notice[1], /has been reset/); }); it("returns only the reset line for an empty violation list", () => { diff --git a/.github/scripts/pr-quality-state.cjs b/.github/scripts/pr-quality-state.cjs index f2807c4db..917e45e75 100644 --- a/.github/scripts/pr-quality-state.cjs +++ b/.github/scripts/pr-quality-state.cjs @@ -201,21 +201,23 @@ function migrateLegacyGateState(enforcerState, readinessState) { */ /** - * Bot-side verification of the checklist claim the gate can check itself for - * ancestry. The local-CI box is an author attestation only (fork contributors - * cannot start repository CI; a maintainer has to), so it is never disproved - * here — head-drift still resets every box after a new push. The latest-dev - * box only holds while the head is at most READINESS_LATEST_DEV_BEHIND_MAX - * commits behind the base. Unknown state (compare lookup failed) fails closed: - * an unverifiable claim is a violation, because an attestation must not ride - * on missing evidence. + * Bot-side verification of the two checklist claims the gate can check itself. + * The CI box only holds when the head's `ci` check is green, and the + * latest-dev box only holds while the head is at most + * READINESS_LATEST_DEV_BEHIND_MAX commits behind the base. Unknown state + * (compare or checks lookup failed) fails closed: an unverifiable claim is a + * violation, because an attestation must not ride on missing evidence. */ function readinessClaimViolations({ + ciGreen, behindBase, behindUnknown = false, behindMax = READINESS_LATEST_DEV_BEHIND_MAX }) { const violations = []; + if (!ciGreen) { + violations.push("ci_green"); + } if (behindUnknown || behindBase > behindMax) { violations.push("latest_dev"); } diff --git a/.github/scripts/pr-quality-state.test.cjs b/.github/scripts/pr-quality-state.test.cjs index 5dc75e4a0..f005ff578 100644 --- a/.github/scripts/pr-quality-state.test.cjs +++ b/.github/scripts/pr-quality-state.test.cjs @@ -229,31 +229,48 @@ describe("completionIsStale", () => { }); describe("readinessClaimViolations", () => { - it("passes when the head is current enough", () => { - assert.deepEqual(readinessClaimViolations({ behindBase: 0 }), []); - assert.deepEqual(readinessClaimViolations({ behindBase: 10 }), []); + it("passes when CI is green and the head is current", () => { + assert.deepEqual( + readinessClaimViolations({ ciGreen: true, behindBase: 0 }), + [], + ); + assert.deepEqual( + readinessClaimViolations({ ciGreen: true, behindBase: 10 }), + [], + ); }); - it("never treats local CI as a bot-verifiable claim", () => { - // Fork contributors attest local green; repository CI is maintainer-started. + it("flags red CI", () => { assert.deepEqual( - readinessClaimViolations({ behindBase: 0, ciGreen: false }), - [], + readinessClaimViolations({ ciGreen: false, behindBase: 0 }), + ["ci_green"], ); }); it("flags a head more than the threshold behind the base", () => { assert.deepEqual( readinessClaimViolations({ + ciGreen: true, behindBase: READINESS_LATEST_DEV_BEHIND_MAX + 1, }), ["latest_dev"], ); }); + it("flags both when both claims fail", () => { + assert.deepEqual( + readinessClaimViolations({ + ciGreen: false, + behindBase: READINESS_LATEST_DEV_BEHIND_MAX + 20, + }), + ["ci_green", "latest_dev"], + ); + }); + it("fails closed when the behind count is unknown", () => { assert.deepEqual( readinessClaimViolations({ + ciGreen: true, behindBase: 0, behindUnknown: true, }), @@ -263,7 +280,7 @@ describe("readinessClaimViolations", () => { it("honours a custom threshold", () => { assert.deepEqual( - readinessClaimViolations({ behindBase: 5, behindMax: 4 }), + readinessClaimViolations({ ciGreen: true, behindBase: 5, behindMax: 4 }), ["latest_dev"], ); }); diff --git a/.github/scripts/pr-quality.cjs b/.github/scripts/pr-quality.cjs index f531e511d..cc841e380 100644 --- a/.github/scripts/pr-quality.cjs +++ b/.github/scripts/pr-quality.cjs @@ -33,12 +33,11 @@ const REVIEW_READINESS_ITEMS = [ /** * Which checklist box each bot-verifiable claim maps to. The order must stay - * in sync with REVIEW_READINESS_ITEMS: index 1 is the latest-dev claim and - * index 2 is the Codex/CodeRabbit findings claim. Index 0 (local CI) is an - * author attestation only — fork contributors cannot start repository CI — so - * the gate never disproves it; head-drift still resets every box. + * in sync with REVIEW_READINESS_ITEMS: index 0 is the CI claim, index 1 is + * the latest-dev claim, and index 2 is the Codex/CodeRabbit findings claim. */ const REVIEW_READINESS_CLAIM_INDEX = { + ci_green: 0, latest_dev: 1, review_findings: 2 }; diff --git a/.github/scripts/pr-quality.test.cjs b/.github/scripts/pr-quality.test.cjs index efecf1f68..7d6192575 100644 --- a/.github/scripts/pr-quality.test.cjs +++ b/.github/scripts/pr-quality.test.cjs @@ -586,16 +586,16 @@ describe("uncheckReviewReadinessBoxes", () => { it("unchecks only the requested boxes", () => { const body = uncheckReviewReadinessBoxes(checkedBody, [ - REVIEW_READINESS_CLAIM_INDEX.latest_dev, + REVIEW_READINESS_CLAIM_INDEX.ci_green, ]); - assert.ok(body.includes("- [x] All CI tests are green on my local testing.")); - assert.ok(body.includes("- [ ] I pushed my PR to the latest dev commit.")); + assert.ok(body.includes("- [ ] All CI tests are green on my local testing.")); + assert.ok(body.includes("- [x] I pushed my PR to the latest dev commit.")); assert.ok(body.includes("- [x] My PR is ready for review.")); }); it("can uncheck several boxes at once", () => { const body = uncheckReviewReadinessBoxes(checkedBody, [ - 0, + REVIEW_READINESS_CLAIM_INDEX.ci_green, REVIEW_READINESS_CLAIM_INDEX.latest_dev, ]); assert.ok(body.includes("- [ ] All CI tests are green on my local testing.")); diff --git a/.github/workflows/enforce-pr-target.yml b/.github/workflows/enforce-pr-target.yml index 5cf5f8462..7717658fa 100644 --- a/.github/workflows/enforce-pr-target.yml +++ b/.github/workflows/enforce-pr-target.yml @@ -841,16 +841,14 @@ jobs: checklistComplete = readiness.present && readiness.complete; } - // The bot verifies the checklist claims it can check itself. The - // local-CI box is an author attestation only — fork contributors - // cannot start repository CI (a maintainer has to) — so the gate - // never disproves it; head-drift still resets every box after a - // new push. The latest-dev box only counts while the head is at - // most READINESS_LATEST_DEV_BEHIND_MAX commits behind the base; - // the findings box only counts while every Codex/CodeRabbit - // review thread on the PR is resolved. A disproved claim unchecks - // that box and keeps the PR a draft, exactly like a head-drift - // reset. + // The bot verifies the three checklist claims it can check itself. + // The CI box only counts when the head's `ci` check (the repo's + // documented "CI passed" signal) is green; the latest-dev box only + // counts while the head is at most READINESS_LATEST_DEV_BEHIND_MAX + // commits behind the base; the findings box only counts while every + // Codex/CodeRabbit review thread on the PR is resolved. A disproved + // claim unchecks that box and keeps the PR a draft, exactly like a + // head-drift reset. let claimViolations = []; let claimNotice = []; if ( @@ -859,7 +857,52 @@ jobs: !headDrifted && failures.length === 0 ) { + let ciGreen = false; + try { + // GitHub Actions' immutable App ID. Name alone is not evidence: + // any installed app can publish a check called `ci`. + const githubActionsAppId = 15368; + const { data: checksData } = + await github.rest.checks.listForRef({ + owner, + repo, + ref: pr.head.sha, + app_id: githubActionsAppId, + check_name: "ci", + filter: "latest", + per_page: 100 + }); + const checkRuns = Array.isArray(checksData.check_runs) + ? checksData.check_runs + : []; + const ciChecks = checkRuns.filter( + check => + check.name === "ci" && + check.app?.id === githubActionsAppId + ); + // The readiness claim requires positive CI evidence. A missing, + // pending, unsuccessful, foreign, or conflicting aggregate + // check must fail closed. The exact app/name/latest query should + // be tiny; if GitHub reports more rows than this response holds, + // treat the truncated evidence as unreadable rather than paging + // through an endpoint whose filters already select the latest run. + ciGreen = + Number.isSafeInteger(checksData.total_count) && + checksData.total_count === checkRuns.length && + ciChecks.length > 0 && + ciChecks.every( + check => + check.status === "completed" && + check.conclusion === "success" + ); + } catch (error) { + core.warning( + `Could not list checks for the readiness claim check: ${error.message}` + ); + ciGreen = false; + } claimViolations = readinessClaimViolations({ + ciGreen, behindBase, behindUnknown: ancestryLookupFailed }); diff --git a/AGENTS.md b/AGENTS.md index bed8589b9..060fa06b9 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -192,13 +192,10 @@ listed in `MAINTAINERS.md` (excluding the author). Completion is bound to the exact commit the PR head pointed at: if new commits are pushed afterwards, the gate moves the PR back to draft, resets the checklist and the notification, and asks the author to test and tick the boxes again against the latest code. -Before a completion is accepted, the gate verifies the checklist claims it -can check itself: the branch must be on the latest `dev` commit or at most -10 commits behind it, and Codex/CodeRabbit findings must be resolved. The -local-CI box is an author attestation only — fork contributors cannot start -repository CI; a maintainer has to — so the gate never disproves it; a new -push still resets every box. A disproved claim unticks the matching box and -keeps the PR a draft. +Before a completion is accepted, the gate verifies the two checklist claims it +can check itself: the head's `ci` check must be green, and the branch must be +on the latest `dev` commit or at most 10 commits behind it. A disproved claim +unticks the matching box and keeps the PR a draft. Authors with repository push permission skip the ancestry heuristic only. As with approval requirements in [`MAINTAINERS.md`](./MAINTAINERS.md), this is enforced by convention until branch protection is configured. diff --git a/MAINTAINERS.md b/MAINTAINERS.md index 3214adfdf..81e57197b 100644 --- a/MAINTAINERS.md +++ b/MAINTAINERS.md @@ -35,13 +35,10 @@ see [The retired `dev2-go` line](#the-retired-dev2-go-line). commits are pushed afterwards, the gate moves the PR back to draft, resets the checklist and the notification, and asks the author to test and tick the boxes again against the latest code. - Before a completion is accepted, the gate verifies the checklist claims - it can check itself: the branch must be on the latest `dev` commit or at - most 10 commits behind it, and Codex/CodeRabbit findings must be resolved. - The local-CI box is an author attestation only — fork contributors cannot - start repository CI; a maintainer has to — so the gate never disproves it; - a new push still resets every box. A disproved claim unticks the matching - box and keeps the PR a draft. + Before a completion is accepted, the gate verifies the two checklist claims + it can check itself: the head's `ci` check must be green, and the branch + must be on the latest `dev` commit or at most 10 commits behind it. A + disproved claim unticks the matching box and keeps the PR a draft. Authors with repository push permission skip the ancestry heuristic only. As with the approval requirement above, this is enforced by convention until branch protection is configured (see the note under the change log). diff --git a/docs-site/src/content/docs/contributing/pr-quality.md b/docs-site/src/content/docs/contributing/pr-quality.md index 27bf69f55..b8b5c39ce 100644 --- a/docs-site/src/content/docs/contributing/pr-quality.md +++ b/docs-site/src/content/docs/contributing/pr-quality.md @@ -64,12 +64,10 @@ tells you exactly what to change: `dev` clears the wrong-branch message automatically and is remembered by the gate; the draft stays until the checklist is complete. Before a completion is accepted, the gate verifies the checklist claims it - can check itself: the branch must be on the latest `dev` commit or at most - 10 commits behind it, and every Codex and CodeRabbit review thread authored - by a review bot on the current head must be resolved (unresolved threads - from other authors do not block). The local-CI box is an author attestation - only — fork contributors cannot start repository CI; a maintainer has to — - so the gate never disproves it; a new push still resets every box. CodeRabbit + can check itself: the head's `ci` check must be green, the branch must be on + the latest `dev` commit or at most 10 commits behind it, and every Codex and + CodeRabbit review thread authored by a review bot on the current head must be + resolved (unresolved threads from other authors do not block). CodeRabbit findings that fall outside the diff range and are reported only in a review body on the current head add to the unresolved count while a bot review thread is open; resolving every bot thread clears the box. A disproved claim diff --git a/tests/ci-workflows.test.ts b/tests/ci-workflows.test.ts index f8d67f42b..652b374a6 100644 --- a/tests/ci-workflows.test.ts +++ b/tests/ci-workflows.test.ts @@ -1272,6 +1272,8 @@ describe("GitHub Actions hardening", () => { 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" && // Hygiene reassessment reads the changed-file list; not a write. name !== "github.rest.pulls.listFiles", ); @@ -1538,6 +1540,7 @@ describe("GitHub Actions hardening", () => { // No prior enforcer history: the checklist completion alone lifts the // draft and notifies the maintainers from MAINTAINERS.md. expect(methodsOf(result)).toEqual(readsAllowedBase([ + "checks.listForRef", "graphql", "pulls.listReviews", "issues.addLabels", @@ -1639,6 +1642,7 @@ describe("GitHub Actions hardening", () => { }); expect(methodsOf(result)).toEqual(readsAllowedBase([ + "checks.listForRef", "graphql", "pulls.listReviews", "issues.addLabels", @@ -1731,6 +1735,7 @@ describe("GitHub Actions hardening", () => { }); expect(methodsOf(result)).toEqual(readsAllowedBase([ + "checks.listForRef", "graphql", "pulls.listReviews", "issues.addLabels", @@ -1867,14 +1872,14 @@ describe("GitHub Actions hardening", () => { expect(result.warnings.some(w => w.startsWith("setFailed:"))).toBe(false); }); - test("red GitHub CI does not untick the local-CI attestation", async () => { - // Fork contributors attest local green; repository CI is - // maintainer-started. A red or missing GitHub `ci` check must not - // disprove the local box or block ready-for-review. + test("a complete checklist with red CI unchecks the CI box and re-drafts", async () => { + // The author ticked every box, but the head's `ci` check is red. The + // gate checks the CI claim itself and unticked the CI box instead of + // letting a false attestation lift the draft. const result = await run({ pr: { base: { ref: "dev" }, - draft: true, + draft: false, body: readinessChecklistBody(4), }, maintainersFile: MAINTAINERS_FIXTURE, @@ -1882,28 +1887,40 @@ describe("GitHub Actions hardening", () => { }); expect(methodsOf(result)).toEqual(readsAllowedBase([ + "checks.listForRef", "graphql", "pulls.listReviews", - "issues.addLabels", - "graphql", + "pulls.get", + "pulls.update", "issues.createComment", + "graphql", ])); - expect(callsTo(result, "checks.listForRef")).toEqual([]); - expect(callsTo(result, "pulls.update")).toEqual([]); - const drafts = callsTo(result, "graphql") as [{ query: string }, { query: string }]; + const [bodyUpdate] = callsTo(result, "pulls.update") as [{ body: string }]; + // Only the CI box is unticked; the other three stay checked. + expect(bodyUpdate.body).toContain("- [ ] All CI tests are green on my local testing."); + expect(bodyUpdate.body).toContain("- [x] I pushed my PR to the latest dev commit."); + expect(bodyUpdate.body).toContain("- [x] My PR is ready for review."); + const drafts = callsTo(result, "graphql") as [{ query: string }]; + expect(drafts).toHaveLength(2); expect(drafts[0]!.query).toContain("reviewThreads"); - expect(drafts[1]!.query).toContain("markPullRequestReadyForReview"); - expect(lastReadinessCommentBody(result)).toContain("**4/4** boxes ticked"); + expect(drafts[1]!.query).toContain("convertPullRequestToDraft"); + expect(drafts[1]!.query).not.toContain("markPullRequestReadyForReview"); + const readinessBody = lastReadinessCommentBody(result); + expect(readinessBody).toContain( + "GitHub CI is not green on the current head `3f1c0de`; the **CI green** box has been unticked.", + ); + expect(readinessBody).toContain("**3/4** boxes ticked"); + expect(readinessBody).toContain('"completedAtHeadSha":null'); + expect(readinessBody).toContain('"maintainersPinged":false'); expect(result.warnings.some(w => w.startsWith("setFailed:"))).toBe(false); }); test("a revalidation reset preserves bot ownership of the title prefix", async () => { // A wrong-base PR that the bot prefixed and later had retargeted to dev - // with a complete checklist hits a revalidation failure (stale vs `dev` - // unchecks a box). The reset must preserve `titlePrefixedByBot` long - // enough for the mustDraft strip to fire — otherwise the stale - // `[WRONG BRANCH] ` prefix stays on the title forever because ownership - // was forgotten. + // with a complete checklist hits a revalidation failure (red CI unchecks + // a box). The reset must preserve `titlePrefixedByBot` long enough for + // the mustDraft strip to fire — otherwise the stale `[WRONG BRANCH] ` + // prefix stays on the title forever because ownership was forgotten. const result = await run({ pr: { base: { ref: "dev" }, @@ -1912,9 +1929,7 @@ describe("GitHub Actions hardening", () => { body: readinessChecklistBody(4), }, maintainersFile: MAINTAINERS_FIXTURE, - compareByBasehead: { - "dev...3f1c0de0a6a4d0a3f9a1b2c3d4e5f60718293a4b": { ahead_by: 0, behind_by: 11 }, - }, + checkRuns: [{ name: "ci", status: "completed", conclusion: "failure" }], comments: [botComment({ version: 1, active: true, @@ -1930,7 +1945,7 @@ describe("GitHub Actions hardening", () => { const readinessBody = lastReadinessCommentBody(result); expect(readinessBody).toContain('"titlePrefixedByBot":false'); expect(readinessBody).toContain('"autoDraftedByBot":true'); - expect(readinessBody).toContain("more than 10 commits behind `dev`"); + expect(readinessBody).toContain("GitHub CI is not green"); }); test("a complete checklist more than 10 commits behind dev unchecks the latest-dev box and re-drafts", async () => { @@ -1947,6 +1962,7 @@ describe("GitHub Actions hardening", () => { }); expect(methodsOf(result)).toEqual(readsAllowedBase([ + "checks.listForRef", "graphql", "pulls.listReviews", "pulls.get", @@ -1955,7 +1971,7 @@ describe("GitHub Actions hardening", () => { "graphql", ])); const [bodyUpdate] = callsTo(result, "pulls.update") as [{ body: string }]; - // Only the latest-dev box is unticked; local CI stays checked. + // Only the latest-dev box is unticked; CI stays checked. expect(bodyUpdate.body).toContain("- [x] All CI tests are green on my local testing."); expect(bodyUpdate.body).toContain("- [ ] I pushed my PR to the latest dev commit."); expect(bodyUpdate.body).toContain("- [x] My PR is ready for review."); @@ -1971,6 +1987,71 @@ describe("GitHub Actions hardening", () => { expect(result.warnings.some(w => w.startsWith("setFailed:"))).toBe(false); }); + test("a complete checklist with red CI and a stale dev base unchecks both boxes", async () => { + const result = await run({ + pr: { + base: { ref: "dev" }, + draft: false, + body: readinessChecklistBody(4), + }, + maintainersFile: MAINTAINERS_FIXTURE, + checkRuns: [{ name: "ci", status: "completed", conclusion: "failure" }], + compareByBasehead: { + "dev...3f1c0de0a6a4d0a3f9a1b2c3d4e5f60718293a4b": { ahead_by: 0, behind_by: 42 }, + }, + }); + + expect(methodsOf(result)).toEqual(readsAllowedBase([ + "checks.listForRef", + "graphql", + "pulls.listReviews", + "pulls.get", + "pulls.update", + "issues.createComment", + "graphql", + ])); + const [bodyUpdate] = callsTo(result, "pulls.update") as [{ body: string }]; + expect(bodyUpdate.body).toContain("- [ ] All CI tests are green on my local testing."); + expect(bodyUpdate.body).toContain("- [ ] I pushed my PR to the latest dev commit."); + expect(bodyUpdate.body).toContain("- [x] My PR is ready for review."); + const readinessBody = lastReadinessCommentBody(result); + expect(readinessBody).toContain("GitHub CI is not green on the current head"); + expect(readinessBody).toContain("more than 10 commits behind `dev`"); + expect(readinessBody).toContain("**2/4** boxes ticked"); + expect(result.warnings.some(w => w.startsWith("setFailed:"))).toBe(false); + }); + + test("a checks lookup failure fails closed for the CI claim", async () => { + // Cannot verify CI: the claim is unverifiable, so the box is unticked + // and the PR stays a draft rather than riding on missing evidence. + const result = await run({ + pr: { + base: { ref: "dev" }, + draft: false, + body: readinessChecklistBody(4), + }, + maintainersFile: MAINTAINERS_FIXTURE, + failOn: ["checks.listForRef"], + }); + + expect(methodsOf(result)).toEqual(readsAllowedBase([ + "checks.listForRef", + "graphql", + "pulls.listReviews", + "pulls.get", + "pulls.update", + "issues.createComment", + "graphql", + ])); + const [bodyUpdate] = callsTo(result, "pulls.update") as [{ body: string }]; + expect(bodyUpdate.body).toContain("- [ ] All CI tests are green on my local testing."); + expect(bodyUpdate.body).toContain("- [x] I pushed my PR to the latest dev commit."); + const readinessBody = lastReadinessCommentBody(result); + expect(readinessBody).toContain("GitHub CI is not green on the current head"); + expect(result.warnings.some(w => w.includes("Could not list checks for the readiness claim check"))).toBe(true); + expect(result.warnings.some(w => w.startsWith("setFailed:"))).toBe(false); + }); + test("a head exactly 10 commits behind dev keeps the latest-dev box", async () => { const result = await run({ pr: { @@ -1985,6 +2066,7 @@ describe("GitHub Actions hardening", () => { }); expect(methodsOf(result)).toEqual(readsAllowedBase([ + "checks.listForRef", "graphql", "pulls.listReviews", "issues.addLabels", @@ -1998,35 +2080,180 @@ describe("GitHub Actions hardening", () => { expect(drafts[1]!.query).toContain("markPullRequestReadyForReview"); }); - test("missing or pending GitHub CI does not block a complete local attestation", async () => { - for (const checkRuns of [ - [], - [{ name: "ci", status: "in_progress", conclusion: null }], - [{ + test("a head with no ci check fails closed for the CI claim", async () => { + // No CI run means the claim has no positive evidence, so the box is + // unticked and the PR stays in draft. + const result = await run({ + pr: { + base: { ref: "dev" }, + draft: true, + body: readinessChecklistBody(4), + }, + maintainersFile: MAINTAINERS_FIXTURE, + checkRuns: [], + }); + + expect(methodsOf(result)).toEqual(readsAllowedBase([ + "checks.listForRef", + "graphql", + "pulls.listReviews", + "pulls.get", + "pulls.update", + "issues.createComment", + ])); + const [bodyUpdate] = callsTo(result, "pulls.update") as [{ body: string }]; + expect(bodyUpdate.body).toContain("- [ ] All CI tests are green on my local testing."); + const drafts = callsTo(result, "graphql") as [{ query: string }]; + expect(drafts).toHaveLength(1); + expect(drafts[0]!.query).toContain("reviewThreads"); + expect(lastReadinessCommentBody(result)).toContain( + "GitHub CI is not green on the current head", + ); + }); + + test("a pending ci check cannot attest green", async () => { + const result = await run({ + pr: { + base: { ref: "dev" }, + draft: false, + body: readinessChecklistBody(4), + }, + maintainersFile: MAINTAINERS_FIXTURE, + checkRuns: [{ name: "ci", status: "in_progress", conclusion: null }], + }); + + expect(methodsOf(result)).toEqual(readsAllowedBase([ + "checks.listForRef", + "graphql", + "pulls.listReviews", + "pulls.get", + "pulls.update", + "issues.createComment", + "graphql", + ])); + const [bodyUpdate] = callsTo(result, "pulls.update") as [{ body: string }]; + expect(bodyUpdate.body).toContain("- [ ] All CI tests are green on my local testing."); + const readinessBody = lastReadinessCommentBody(result); + expect(readinessBody).toContain("GitHub CI is not green on the current head"); + }); + + test("a complete filtered trusted ci response attests green", async () => { + const result = await run({ + pr: { + base: { ref: "dev" }, + draft: true, + body: readinessChecklistBody(4), + }, + maintainersFile: MAINTAINERS_FIXTURE, + checkRuns: [{ name: "ci", status: "completed", conclusion: "success" }], + checkRunTotalCount: 1, + }); + + expect(methodsOf(result)).toEqual(readsAllowedBase([ + "checks.listForRef", + "graphql", + "pulls.listReviews", + "issues.addLabels", + "graphql", + "issues.createComment", + ])); + const checkCalls = callsTo(result, "checks.listForRef") as Array<{ + app_id?: number; + check_name?: string; + filter?: string; + }>; + for (const call of checkCalls) { + expect(call.app_id).toBe(15368); + expect(call.check_name).toBe("ci"); + expect(call.filter).toBe("latest"); + } + expect(callsTo(result, "pulls.update")).toEqual([]); + const drafts = callsTo(result, "graphql") as [{ query: string }, { query: string }]; + expect(drafts[1]!.query).toContain("markPullRequestReadyForReview"); + expect(lastReadinessCommentBody(result)).toContain("**4/4** boxes ticked"); + }); + + test("a truncated filtered ci response cannot attest green", async () => { + const result = await run({ + pr: { + base: { ref: "dev" }, + draft: false, + body: readinessChecklistBody(4), + }, + maintainersFile: MAINTAINERS_FIXTURE, + checkRuns: [{ name: "ci", status: "completed", conclusion: "success" }], + checkRunTotalCount: 2, + }); + + const [bodyUpdate] = callsTo(result, "pulls.update") as [{ body: string }]; + expect(bodyUpdate.body).toContain("- [ ] All CI tests are green on my local testing."); + expect(lastReadinessCommentBody(result)).toContain( + "GitHub CI is not green on the current head", + ); + }); + + test("a foreign app check named ci cannot attest green", async () => { + const result = await run({ + pr: { + base: { ref: "dev" }, + draft: false, + body: readinessChecklistBody(4), + }, + maintainersFile: MAINTAINERS_FIXTURE, + checkRuns: [{ name: "ci", status: "completed", conclusion: "success", app: { id: 999999 }, }], - ]) { + }); + + const [bodyUpdate] = callsTo(result, "pulls.update") as [{ body: string }]; + expect(bodyUpdate.body).toContain("- [ ] All CI tests are green on my local testing."); + expect(lastReadinessCommentBody(result)).toContain("GitHub CI is not green on the current head"); + }); + + test("conflicting trusted ci checks fail closed regardless of ordering", async () => { + const green = { name: "ci", status: "completed", conclusion: "success" }; + const pending = { name: "ci", status: "in_progress", conclusion: null }; + const failed = { name: "ci", status: "completed", conclusion: "failure" }; + + for (const checkRuns of [[green, pending], [pending, green], [green, failed], [failed, green]]) { const result = await run({ pr: { base: { ref: "dev" }, - draft: true, + draft: false, body: readinessChecklistBody(4), }, maintainersFile: MAINTAINERS_FIXTURE, checkRuns, }); - expect(callsTo(result, "checks.listForRef")).toEqual([]); - expect(callsTo(result, "pulls.update")).toEqual([]); - const drafts = callsTo(result, "graphql") as [{ query: string }, { query: string }]; - expect(drafts[1]!.query).toContain("markPullRequestReadyForReview"); - expect(lastReadinessCommentBody(result)).toContain("**4/4** boxes ticked"); + const [bodyUpdate] = callsTo(result, "pulls.update") as [{ body: string }]; + expect(bodyUpdate.body).toContain("- [ ] All CI tests are green on my local testing."); + expect(lastReadinessCommentBody(result)).toContain("GitHub CI is not green on the current head"); } }); + test("multiple latest trusted green ci checks are consistent evidence", async () => { + const result = await run({ + pr: { + base: { ref: "dev" }, + draft: true, + body: readinessChecklistBody(4), + }, + maintainersFile: MAINTAINERS_FIXTURE, + checkRuns: [ + { name: "ci", status: "completed", conclusion: "success" }, + { name: "ci", status: "completed", conclusion: "success" }, + ], + }); + + expect(callsTo(result, "pulls.update")).toEqual([]); + const drafts = callsTo(result, "graphql") as [{ query: string }, { query: string }]; + expect(drafts[1]!.query).toContain("markPullRequestReadyForReview"); + }); + test("an unresolved Codex thread unchecks the findings box and re-drafts", async () => { const result = await run({ pr: { @@ -2041,6 +2268,7 @@ describe("GitHub Actions hardening", () => { }); expect(methodsOf(result)).toEqual(readsAllowedBase([ + "checks.listForRef", "graphql", "pulls.listReviews", "pulls.get", @@ -2104,6 +2332,7 @@ describe("GitHub Actions hardening", () => { }); expect(methodsOf(result)).toEqual(readsAllowedBase([ + "checks.listForRef", "graphql", "pulls.listReviews", "issues.addLabels", @@ -2145,6 +2374,7 @@ describe("GitHub Actions hardening", () => { }); expect(methodsOf(result)).toEqual(readsAllowedBase([ + "checks.listForRef", "graphql", "pulls.listReviews", "issues.addLabels", @@ -2208,6 +2438,7 @@ describe("GitHub Actions hardening", () => { }); expect(methodsOf(result)).toEqual(readsAllowedBase([ + "checks.listForRef", "graphql", "pulls.listReviews", "issues.addLabels", @@ -2236,6 +2467,7 @@ describe("GitHub Actions hardening", () => { }); expect(methodsOf(result)).toEqual(readsAllowedBase([ + "checks.listForRef", "graphql", "pulls.listReviews", "issues.addLabels", @@ -2591,6 +2823,7 @@ describe("GitHub Actions hardening", () => { }); expect(methodsOf(result)).toEqual(readsAllowedBase([ + "checks.listForRef", "graphql", "pulls.listReviews", "issues.addLabels", @@ -3385,6 +3618,7 @@ describe("GitHub Actions hardening", () => { }); expect(methodsOf(result)).toEqual(readsAllowedBase([ + "checks.listForRef", "graphql", "pulls.listReviews", "issues.addLabels", @@ -3442,6 +3676,7 @@ describe("GitHub Actions hardening", () => { }); expect(methodsOf(result)).toEqual(readsAllowedBase([ + "checks.listForRef", "graphql", "pulls.listReviews", "issues.addLabels", @@ -3742,6 +3977,7 @@ describe("GitHub Actions hardening", () => { }); expect(methodsOf(result)).toEqual(readsAllowedBase([ + "checks.listForRef", "graphql", "pulls.listReviews", "issues.addLabels", @@ -3891,6 +4127,7 @@ describe("GitHub Actions hardening", () => { // created comment is the readiness checklist message, which did not // exist on the busy PR yet. expect(methodsOf(result)).toEqual(readsAllowedBasePaged([ + "checks.listForRef", "graphql", "pulls.listReviews", "pulls.listReviews", @@ -4013,6 +4250,7 @@ describe("GitHub Actions hardening", () => { expect(callsTo(result, "pulls.update")).toEqual([]); expect(methodsOf(result)).toEqual(readsAllowedBase([ + "checks.listForRef", "graphql", "pulls.listReviews", "issues.addLabels", @@ -4105,6 +4343,7 @@ describe("GitHub Actions hardening", () => { comments: [botComment(active)], }); expect(methodsOf(restored)).toEqual(readsAllowedBase([ + "checks.listForRef", "graphql", "pulls.listReviews", "issues.addLabels", @@ -4163,6 +4402,7 @@ describe("GitHub Actions hardening", () => { comments: [botComment({ version: 1, active: "true", autoDraftedByBot: 1, titlePrefixedByBot: "yes" })], }); expect(methodsOf(loose)).toEqual(readsAllowedBase([ + "checks.listForRef", "graphql", "pulls.listReviews", "issues.addLabels", @@ -4187,6 +4427,7 @@ describe("GitHub Actions hardening", () => { comments: [botComment({ version: 1, active: true, autoDraftedByBot: null, titlePrefixedByBot: 0 })], }); expect(methodsOf(falsy)).toEqual(readsAllowedBase([ + "checks.listForRef", "graphql", "pulls.listReviews", "issues.addLabels", @@ -4359,6 +4600,7 @@ describe("GitHub Actions hardening", () => { // The first comment's state is the one honoured: it says the bot // prefixed and drafted, so both are undone. expect(methodsOf(result)).toEqual(readsAllowedBase([ + "checks.listForRef", "graphql", "pulls.listReviews", "issues.addLabels", diff --git a/tests/helpers/enforce-pr-target-harness.ts b/tests/helpers/enforce-pr-target-harness.ts index b6d90da52..24a89ec54 100644 --- a/tests/helpers/enforce-pr-target-harness.ts +++ b/tests/helpers/enforce-pr-target-harness.ts @@ -178,10 +178,9 @@ export type RunOptions = { /** Page-keyed open PR fixtures for `pulls.list` (1-based via array index). */ openPullPages?: unknown[][]; /** - * Check-runs `checks.listForRef` used to report for readiness claim checks. - * Local CI is now an author attestation only, so the gate no longer lists - * checks; these fixtures remain so older scenarios that pass `checkRuns` - * still construct cleanly without affecting gate behavior. + * Check-runs `checks.listForRef` reports for the head. Defaults to a green + * `ci` check so completed-checklist scenarios pass the claim check. + * Pass a red/pending/missing set to exercise the claim-check reset paths. */ checkRuns?: Array<{ name: string; @@ -196,7 +195,7 @@ export type RunOptions = { conclusion: string | null; app?: { id: number } | null; }>>; - /** Optional filtered total; unused now that the gate skips check listing. */ + /** Optional filtered total for proving truncated check evidence fails closed. */ checkRunTotalCount?: number; /** * Review threads `pullRequestReviewThreads` (via GraphQL) reports for the PR.