diff --git a/.github/scripts/pr-quality-messages.cjs b/.github/scripts/pr-quality-messages.cjs index 6d9b2fff02..e9b642b574 100644 --- a/.github/scripts/pr-quality-messages.cjs +++ b/.github/scripts/pr-quality-messages.cjs @@ -259,14 +259,10 @@ 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 === "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") { + 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 2cd474df7c..ca6e056bd6 100644 --- a/.github/scripts/pr-quality-messages.test.cjs +++ b/.github/scripts/pr-quality-messages.test.cjs @@ -213,23 +213,21 @@ describe("buildStaleNotice", () => { }); describe("buildClaimCheckNotice", () => { - it("names each violated claim and the reset action", () => { + it("names the latest-dev violation and the reset action", () => { const notice = buildClaimCheckNotice( - ["ci_green", "latest_dev"], + ["latest_dev"], "3f1c0de0a6a4d0a3f9a1b2c3d4e5f60718293a4b", ); - 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/); + 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/); }); - it("handles a single violation", () => { + it("ignores a stale ci_green code without inventing GitHub-CI copy", () => { const notice = buildClaimCheckNotice(["ci_green"], "a".repeat(40)); - assert.equal(notice.length, 2); - assert.match(notice[0], /CI is not green/); - assert.match(notice[1], /has been reset/); + assert.equal(notice.length, 1); + assert.match(notice[0], /has been reset/); + assert.doesNotMatch(notice[0], /CI is not green/); }); 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 917e45e759..f2807c4db3 100644 --- a/.github/scripts/pr-quality-state.cjs +++ b/.github/scripts/pr-quality-state.cjs @@ -201,23 +201,21 @@ function migrateLegacyGateState(enforcerState, readinessState) { */ /** - * 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. + * 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. */ 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 f005ff5784..5dc75e4a00 100644 --- a/.github/scripts/pr-quality-state.test.cjs +++ b/.github/scripts/pr-quality-state.test.cjs @@ -229,48 +229,31 @@ describe("completionIsStale", () => { }); describe("readinessClaimViolations", () => { - 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("passes when the head is current enough", () => { + assert.deepEqual(readinessClaimViolations({ behindBase: 0 }), []); + assert.deepEqual(readinessClaimViolations({ behindBase: 10 }), []); }); - it("flags red CI", () => { + it("never treats local CI as a bot-verifiable claim", () => { + // Fork contributors attest local green; repository CI is maintainer-started. assert.deepEqual( - readinessClaimViolations({ ciGreen: false, behindBase: 0 }), - ["ci_green"], + readinessClaimViolations({ behindBase: 0, ciGreen: false }), + [], ); }); 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, }), @@ -280,7 +263,7 @@ describe("readinessClaimViolations", () => { it("honours a custom threshold", () => { assert.deepEqual( - readinessClaimViolations({ ciGreen: true, behindBase: 5, behindMax: 4 }), + readinessClaimViolations({ behindBase: 5, behindMax: 4 }), ["latest_dev"], ); }); diff --git a/.github/scripts/pr-quality.cjs b/.github/scripts/pr-quality.cjs index cc841e3809..f531e511d9 100644 --- a/.github/scripts/pr-quality.cjs +++ b/.github/scripts/pr-quality.cjs @@ -33,11 +33,12 @@ const REVIEW_READINESS_ITEMS = [ /** * Which checklist box each bot-verifiable claim maps to. The order must stay - * 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. + * 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. */ 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 7d61925758..efecf1f682 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.ci_green, + REVIEW_READINESS_CLAIM_INDEX.latest_dev, ]); - 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] 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("- [x] My PR is ready for review.")); }); it("can uncheck several boxes at once", () => { const body = uncheckReviewReadinessBoxes(checkedBody, [ - REVIEW_READINESS_CLAIM_INDEX.ci_green, + 0, 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 73dc2cf943..60cf184e26 100644 --- a/.github/workflows/enforce-pr-target.yml +++ b/.github/workflows/enforce-pr-target.yml @@ -99,10 +99,8 @@ jobs: needs: resolve-pr if: needs.resolve-pr.outputs.pull-number != '' runs-on: ubuntu-latest - # The write job also reads the current head's aggregate check evidence. # Job-scoped permissions replace, rather than extend, the workflow default. permissions: - checks: read contents: write pull-requests: write concurrency: @@ -802,14 +800,16 @@ jobs: checklistComplete = readiness.present && readiness.complete; } - // 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. + // 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. let claimViolations = []; let claimNotice = []; if ( @@ -818,52 +818,7 @@ 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/.github/workflows/issue-quality-tests.yml b/.github/workflows/issue-quality-tests.yml index dbc8f5341f..600345dabc 100644 --- a/.github/workflows/issue-quality-tests.yml +++ b/.github/workflows/issue-quality-tests.yml @@ -11,6 +11,8 @@ on: - ".github/scripts/pr-quality.test.cjs" - ".github/scripts/pr-quality-messages.cjs" - ".github/scripts/pr-quality-messages.test.cjs" + - ".github/scripts/pr-quality-state.cjs" + - ".github/scripts/pr-quality-state.test.cjs" - ".github/scripts/pr-labeler.cjs" - ".github/scripts/pr-labeler.test.cjs" - ".github/scripts/enforce-pr-target.test.cjs" @@ -41,6 +43,8 @@ on: - ".github/scripts/pr-quality.test.cjs" - ".github/scripts/pr-quality-messages.cjs" - ".github/scripts/pr-quality-messages.test.cjs" + - ".github/scripts/pr-quality-state.cjs" + - ".github/scripts/pr-quality-state.test.cjs" - ".github/scripts/pr-labeler.cjs" - ".github/scripts/pr-labeler.test.cjs" - ".github/scripts/enforce-pr-target.test.cjs" @@ -79,6 +83,8 @@ jobs: run: | node --test .github/scripts/issue-quality*.test.cjs node --test .github/scripts/pr-quality.test.cjs + node --test .github/scripts/pr-quality-messages.test.cjs + node --test .github/scripts/pr-quality-state.test.cjs node --test .github/scripts/pr-labeler.test.cjs node --test .github/scripts/enforce-pr-target.test.cjs node --test .github/scripts/pr-hygiene.test.cjs diff --git a/AGENTS.md b/AGENTS.md index 060fa06b9a..bed8589b99 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -192,10 +192,13 @@ 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 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. +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. 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 81e57197bc..3214adfdf1 100644 --- a/MAINTAINERS.md +++ b/MAINTAINERS.md @@ -35,10 +35,13 @@ 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 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. + 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. 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 b8b5c39ce5..27bf69f555 100644 --- a/docs-site/src/content/docs/contributing/pr-quality.md +++ b/docs-site/src/content/docs/contributing/pr-quality.md @@ -64,10 +64,12 @@ 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 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 + 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 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 73bd08e71b..abfeff81e5 100644 --- a/tests/ci-workflows.test.ts +++ b/tests/ci-workflows.test.ts @@ -933,8 +933,7 @@ describe("GitHub Actions hardening", () => { expect(Object.keys(workflow.on?.pull_request_target ?? {})).toEqual(["types"]); expect(Object.prototype.hasOwnProperty.call(workflow.on ?? {}, "status")).toBe(true); - // Exactly the scopes this gate needs. `checks: read` covers the live - // current-head CI evidence lookup. `pull-requests: write` covers title and + // Exactly the scopes this gate needs. `pull-requests: write` covers title and // comment updates. `contents: write` is required for the draft GraphQL // mutations with GITHUB_TOKEN (#626: "Resource not accessible by integration" // when contents was unset). Asserting the whole object pins both presence @@ -994,7 +993,6 @@ describe("GitHub Actions hardening", () => { ]); expect(job?.["runs-on"]).toBe("ubuntu-latest"); expect(job?.permissions).toEqual({ - checks: "read", contents: "write", "pull-requests": "write", }); @@ -1220,8 +1218,6 @@ 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", ); @@ -1465,7 +1461,6 @@ 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", @@ -1567,7 +1562,6 @@ describe("GitHub Actions hardening", () => { }); expect(methodsOf(result)).toEqual(readsAllowedBase([ - "checks.listForRef", "graphql", "pulls.listReviews", "issues.addLabels", @@ -1660,7 +1654,6 @@ describe("GitHub Actions hardening", () => { }); expect(methodsOf(result)).toEqual(readsAllowedBase([ - "checks.listForRef", "graphql", "pulls.listReviews", "issues.addLabels", @@ -1797,14 +1790,14 @@ describe("GitHub Actions hardening", () => { expect(result.warnings.some(w => w.startsWith("setFailed:"))).toBe(false); }); - 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. + 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. const result = await run({ pr: { base: { ref: "dev" }, - draft: false, + draft: true, body: readinessChecklistBody(4), }, maintainersFile: MAINTAINERS_FIXTURE, @@ -1812,40 +1805,28 @@ describe("GitHub Actions hardening", () => { }); expect(methodsOf(result)).toEqual(readsAllowedBase([ - "checks.listForRef", "graphql", "pulls.listReviews", - "pulls.get", - "pulls.update", - "issues.createComment", + "issues.addLabels", "graphql", + "issues.createComment", ])); - 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(callsTo(result, "checks.listForRef")).toEqual([]); + expect(callsTo(result, "pulls.update")).toEqual([]); + const drafts = callsTo(result, "graphql") as [{ query: string }, { query: string }]; expect(drafts[0]!.query).toContain("reviewThreads"); - 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(drafts[1]!.query).toContain("markPullRequestReadyForReview"); + expect(lastReadinessCommentBody(result)).toContain("**4/4** boxes ticked"); 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 (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. + // 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. const result = await run({ pr: { base: { ref: "dev" }, @@ -1854,7 +1835,9 @@ describe("GitHub Actions hardening", () => { body: readinessChecklistBody(4), }, maintainersFile: MAINTAINERS_FIXTURE, - checkRuns: [{ name: "ci", status: "completed", conclusion: "failure" }], + compareByBasehead: { + "dev...3f1c0de0a6a4d0a3f9a1b2c3d4e5f60718293a4b": { ahead_by: 0, behind_by: 11 }, + }, comments: [botComment({ version: 1, active: true, @@ -1870,7 +1853,7 @@ describe("GitHub Actions hardening", () => { const readinessBody = lastReadinessCommentBody(result); expect(readinessBody).toContain('"titlePrefixedByBot":false'); expect(readinessBody).toContain('"autoDraftedByBot":true'); - expect(readinessBody).toContain("GitHub CI is not green"); + expect(readinessBody).toContain("more than 10 commits behind `dev`"); }); test("a complete checklist more than 10 commits behind dev unchecks the latest-dev box and re-drafts", async () => { @@ -1887,7 +1870,6 @@ describe("GitHub Actions hardening", () => { }); expect(methodsOf(result)).toEqual(readsAllowedBase([ - "checks.listForRef", "graphql", "pulls.listReviews", "pulls.get", @@ -1896,7 +1878,7 @@ describe("GitHub Actions hardening", () => { "graphql", ])); const [bodyUpdate] = callsTo(result, "pulls.update") as [{ body: string }]; - // Only the latest-dev box is unticked; CI stays checked. + // Only the latest-dev box is unticked; local 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."); @@ -1912,71 +1894,6 @@ 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: { @@ -1991,7 +1908,6 @@ describe("GitHub Actions hardening", () => { }); expect(methodsOf(result)).toEqual(readsAllowedBase([ - "checks.listForRef", "graphql", "pulls.listReviews", "issues.addLabels", @@ -2005,180 +1921,35 @@ describe("GitHub Actions hardening", () => { expect(drafts[1]!.query).toContain("markPullRequestReadyForReview"); }); - 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: [{ + 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 }], + [{ 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: false, + draft: true, body: readinessChecklistBody(4), }, maintainersFile: MAINTAINERS_FIXTURE, checkRuns, }); - 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"); + 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"); } }); - 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: { @@ -2193,7 +1964,6 @@ describe("GitHub Actions hardening", () => { }); expect(methodsOf(result)).toEqual(readsAllowedBase([ - "checks.listForRef", "graphql", "pulls.listReviews", "pulls.get", @@ -2257,7 +2027,6 @@ describe("GitHub Actions hardening", () => { }); expect(methodsOf(result)).toEqual(readsAllowedBase([ - "checks.listForRef", "graphql", "pulls.listReviews", "issues.addLabels", @@ -2299,7 +2068,6 @@ describe("GitHub Actions hardening", () => { }); expect(methodsOf(result)).toEqual(readsAllowedBase([ - "checks.listForRef", "graphql", "pulls.listReviews", "issues.addLabels", @@ -2363,7 +2131,6 @@ describe("GitHub Actions hardening", () => { }); expect(methodsOf(result)).toEqual(readsAllowedBase([ - "checks.listForRef", "graphql", "pulls.listReviews", "issues.addLabels", @@ -2392,7 +2159,6 @@ describe("GitHub Actions hardening", () => { }); expect(methodsOf(result)).toEqual(readsAllowedBase([ - "checks.listForRef", "graphql", "pulls.listReviews", "issues.addLabels", @@ -2748,7 +2514,6 @@ describe("GitHub Actions hardening", () => { }); expect(methodsOf(result)).toEqual(readsAllowedBase([ - "checks.listForRef", "graphql", "pulls.listReviews", "issues.addLabels", @@ -3422,7 +3187,6 @@ describe("GitHub Actions hardening", () => { }); expect(methodsOf(result)).toEqual(readsAllowedBase([ - "checks.listForRef", "graphql", "pulls.listReviews", "issues.addLabels", @@ -3480,7 +3244,6 @@ describe("GitHub Actions hardening", () => { }); expect(methodsOf(result)).toEqual(readsAllowedBase([ - "checks.listForRef", "graphql", "pulls.listReviews", "issues.addLabels", @@ -3781,7 +3544,6 @@ describe("GitHub Actions hardening", () => { }); expect(methodsOf(result)).toEqual(readsAllowedBase([ - "checks.listForRef", "graphql", "pulls.listReviews", "issues.addLabels", @@ -3931,7 +3693,6 @@ 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", @@ -4054,7 +3815,6 @@ describe("GitHub Actions hardening", () => { expect(callsTo(result, "pulls.update")).toEqual([]); expect(methodsOf(result)).toEqual(readsAllowedBase([ - "checks.listForRef", "graphql", "pulls.listReviews", "issues.addLabels", @@ -4147,7 +3907,6 @@ describe("GitHub Actions hardening", () => { comments: [botComment(active)], }); expect(methodsOf(restored)).toEqual(readsAllowedBase([ - "checks.listForRef", "graphql", "pulls.listReviews", "issues.addLabels", @@ -4206,7 +3965,6 @@ 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", @@ -4231,7 +3989,6 @@ 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", @@ -4404,7 +4161,6 @@ 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 ceac6073b6..e1e69d68f2 100644 --- a/tests/helpers/enforce-pr-target-harness.ts +++ b/tests/helpers/enforce-pr-target-harness.ts @@ -171,9 +171,10 @@ export type RunOptions = { /** Page-keyed open PR fixtures for `pulls.list` (1-based via array index). */ openPullPages?: unknown[][]; /** - * 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. + * 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. */ checkRuns?: Array<{ name: string; @@ -188,7 +189,7 @@ export type RunOptions = { conclusion: string | null; app?: { id: number } | null; }>>; - /** Optional filtered total for proving truncated check evidence fails closed. */ + /** Optional filtered total; unused now that the gate skips check listing. */ checkRunTotalCount?: number; /** * Review threads `pullRequestReviewThreads` (via GraphQL) reports for the PR.