Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 2 additions & 6 deletions .github/scripts/pr-quality-messages.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -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.`
);
Expand Down
20 changes: 9 additions & 11 deletions .github/scripts/pr-quality-messages.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -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", () => {
Expand Down
18 changes: 8 additions & 10 deletions .github/scripts/pr-quality-state.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -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");
}
Expand Down
33 changes: 8 additions & 25 deletions .github/scripts/pr-quality-state.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
}),
Expand All @@ -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"],
);
});
Expand Down
7 changes: 4 additions & 3 deletions .github/scripts/pr-quality.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -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
};
Expand Down
8 changes: 4 additions & 4 deletions .github/scripts/pr-quality.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -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."));
Expand Down
65 changes: 10 additions & 55 deletions .github/workflows/enforce-pr-target.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -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 (
Expand All @@ -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
});
Expand Down
6 changes: 6 additions & 0 deletions .github/workflows/issue-quality-tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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"
Expand Down Expand Up @@ -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
Expand Down
11 changes: 7 additions & 4 deletions AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
11 changes: 7 additions & 4 deletions MAINTAINERS.md
Original file line number Diff line number Diff line change
Expand Up @@ -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).
Expand Down
10 changes: 6 additions & 4 deletions docs-site/src/content/docs/contributing/pr-quality.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading
Loading