Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 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
30 changes: 30 additions & 0 deletions .github/scripts/pr-quality.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -320,6 +320,35 @@ function stripReviewReadinessSection(body) {
return stripped.replace(/\n{3,}/g, "\n\n").trimEnd();
}

/**
* Replace the bot-managed readiness section with a fresh unticked copy.
* Used when new commits land after the checklist was completed: the old
* attestation covered a different head, so every box resets and the author
* must re-tick against the latest code. Malformed marker sets (duplicates,
* extra pairs) stay untouched, matching `stripReviewReadinessSection`.
*/
function resetReviewReadinessSection(body) {
if (typeof body !== "string") return body;
const start = body.indexOf(REVIEW_READINESS_START);
const end = body.indexOf(REVIEW_READINESS_END);
if (start === -1 || end === -1 || end <= start) return body;
if (
body.split(REVIEW_READINESS_START).length - 1 !== 1 ||
body.split(REVIEW_READINESS_END).length - 1 !== 1
) {
return body;
}
const section = buildReviewReadinessSection();
// Splice only the bounded section: the author's surrounding content —
// including deliberate blank lines and trailing markdown — stays byte for
// byte identical to what they wrote.
return (
body.slice(0, start) +
section +
body.slice(end + REVIEW_READINESS_END.length)
);
}

function collectPrQualityFailures({
baseRef,
allowedBases,
Expand Down Expand Up @@ -389,6 +418,7 @@ module.exports = {
extractReviewReadiness,
appendReviewReadinessSection,
stripReviewReadinessSection,
resetReviewReadinessSection,
collectPrQualityFailures,
hasEscapedNewlines,
stripPrTemplateBoilerplate,
Expand Down
71 changes: 71 additions & 0 deletions .github/scripts/pr-quality.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ const {
extractReviewReadiness,
appendReviewReadinessSection,
stripReviewReadinessSection,
resetReviewReadinessSection,
collectPrQualityFailures,
} = require("./pr-quality.cjs");

Expand Down Expand Up @@ -399,6 +400,76 @@ describe("review readiness checklist", () => {
assert.equal(stripReviewReadinessSection("plain body"), "plain body");
assert.equal(stripReviewReadinessSection(null), null);
});

it("resets every checked box to unticked and keeps the surrounding body", () => {
const body = [
"## Summary",
"Author content.",
"",
SECTION.replaceAll("- [ ] ", "- [x] "),
"",
"## Test plan",
"- Ran the suite.",
].join("\n");
const reset = resetReviewReadinessSection(body);
const extracted = extractReviewReadiness(reset);
assert.equal(extracted.present, true);
assert.equal(extracted.complete, false);
assert.equal(extracted.checked, 0);
assert.equal(extracted.total, 4);
assert.equal((reset.match(/\[x\]/g) || []).length, 0);
assert.ok(reset.includes("Author content."));
assert.ok(reset.includes("## Test plan"));
});

it("resets a partially ticked section as well", () => {
const partial = SECTION.replace(
"- [ ] My PR is ready for review.",
"- [x] My PR is ready for review.",
);
const reset = resetReviewReadinessSection(partial);
const extracted = extractReviewReadiness(reset);
assert.equal(extracted.checked, 0);
assert.equal(extracted.complete, false);
});

it("preserves the surrounding author formatting exactly", () => {
const body = [
"## Summary",
"Author content.",
"",
"",
SECTION.replaceAll("- [ ] ", "- [x] "),
"",
"",
"Trailing note with blank lines above.",
"",
].join("\n");
const reset = resetReviewReadinessSection(body);
// Only the bounded section changed; deliberate blank lines and trailing
// markdown survive byte for byte (no `\n{3,}` collapse, no trimEnd).
assert.equal(reset, body.replaceAll("- [x] ", "- [ ] "));
});

it("is idempotent on an already-unticked section", () => {
const once = resetReviewReadinessSection(
SECTION.replaceAll("- [ ] ", "- [x] "),
);
assert.equal(resetReviewReadinessSection(once), once);
assert.equal(extractReviewReadiness(once).checked, 0);
});

it("leaves markerless and malformed bodies alone", () => {
assert.equal(resetReviewReadinessSection("plain body"), "plain body");
assert.equal(resetReviewReadinessSection(null), null);
const duplicate = SECTION + SECTION;
assert.equal(resetReviewReadinessSection(duplicate), duplicate);
const inverted =
"<!-- pr-quality-readiness-checklist:end -->\n" +
"<!-- pr-quality-readiness-checklist:start -->\n" +
"- [x] orphan box";
assert.equal(resetReviewReadinessSection(inverted), inverted);
});
});

describe("assessPrDescription with the readiness section", () => {
Expand Down
104 changes: 98 additions & 6 deletions .github/workflows/enforce-pr-target.yml
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ jobs:
extractReviewReadiness,
appendReviewReadinessSection,
stripReviewReadinessSection,
resetReviewReadinessSection,
REVIEW_READINESS_ITEMS
} = require(
path.join(process.cwd(), ".github", "scripts", "pr-quality.cjs"),
Expand All @@ -67,7 +68,10 @@ jobs:
const READINESS_MARKER = "<!-- pr-quality-readiness -->";
const READINESS_STATE_PATTERN =
/<!-- pr-quality-readiness-state:([\s\S]*?) -->/;
const READINESS_STATE_VERSION = 1;
// v2 adds `completedAtHeadSha` so a completed checklist is bound
// to the exact head it attested. v1 states (no field) are read the
// same way: the binding only starts on the next completion.
const READINESS_STATE_VERSION = 2;
const MAINTAINERS_FILE = "MAINTAINERS.md";

const { owner, repo } = context.repo;
Expand Down Expand Up @@ -177,7 +181,8 @@ jobs:
return {
version: READINESS_STATE_VERSION,
autoDraftedByBot: false,
maintainersPinged: false
maintainersPinged: false,
completedAtHeadSha: null
};
}

Expand Down Expand Up @@ -580,7 +585,73 @@ jobs:
});
readiness = extractReviewReadiness(injectedBody);
}
const checklistComplete = readiness.present && readiness.complete;
let checklistComplete = readiness.present && readiness.complete;

// A completed checklist is an attestation about a specific head.
// The attestation is stale when the recorded completion head
// differs from the live head (new commits landed after the last
// completion) or when the boxes were ticked in an event that saw
// an older head than the live one — a push raced the `edited`
// job, so no completion head was recorded yet but the ticks
// predate the code under review. Either way the gate resets the
// boxes and the notification state, re-drafts, and tells the
// author to re-test and re-tick against the latest code. State
// written before this feature existed has no recorded head, so a
// synchronize event whose head matches the live head binds
// forward at the next completion instead of retroactively
// drafting already-ready PRs.
const eventHeadSha =
context.payload.pull_request?.head?.sha ?? pr.head.sha;
const completionHeadSha =
storedReadinessState?.completedAtHeadSha ?? null;
const completionRecordedForLiveHead =
completionHeadSha !== null && completionHeadSha === pr.head.sha;
const ticksPredateLiveHead =
completionHeadSha === null && eventHeadSha !== pr.head.sha;
const headDrifted =
checklistRequired &&
readiness.present &&
((completionHeadSha !== null && !completionRecordedForLiveHead) ||
ticksPredateLiveHead);

let readinessStateOverride = null;
let headDriftNotice = [];
if (headDrifted) {
// Re-fetch the PR so an author edit that landed while this job
// was reading cannot be clobbered by the reset.
const { data: freshPr } = await github.rest.pulls.get({
owner,
repo,
pull_number
});
const freshReadiness = extractReviewReadiness(
freshPr.body ?? ""
);
readinessStateOverride = defaultReadinessState();
headDriftNotice = [
completionHeadSha !== null
? `New commits were pushed after the checklist was completed on ${inlineCode(String(completionHeadSha).slice(0, 7))}; the current head is ${inlineCode(freshPr.head.sha.slice(0, 7))}.`
: `The checklist was ticked before the current head ${inlineCode(freshPr.head.sha.slice(0, 7))} was pushed.`,
"The checklist has been reset: re-test against the latest code and tick all four boxes again."
];
Comment thread
Wibias marked this conversation as resolved.
Outdated
if (freshReadiness.present && freshReadiness.complete) {
const resetBody = resetReviewReadinessSection(
freshPr.body ?? ""
);
if (resetBody !== freshPr.body) {
await github.rest.pulls.update({
owner,
repo,
pull_number,
body: resetBody
});
}
readiness = extractReviewReadiness(resetBody);
} else {
readiness = freshReadiness;
}
checklistComplete = readiness.present && readiness.complete;
}

// A contributor PR stays a draft while the checklist is open, even
// when every quality gate already passes.
Expand All @@ -589,9 +660,20 @@ jobs:

if (mustDraft) {
let draftConverted = false;
const readinessState = storedReadinessState
? { ...storedReadinessState }
: defaultReadinessState();
const readinessState =
readinessStateOverride ??
(storedReadinessState
? { ...storedReadinessState }
: defaultReadinessState());
if (checklistRequired && checklistComplete) {
// The attestation covers this head even while another quality
// gate keeps the draft: bind it now, because the failure path
// below returns before the completion block that records it.
// A later push then still resets the checklist instead of
// sliding the completion forward onto un-attested code.
readinessState.completedAtHeadSha = pr.head.sha;
readinessState.version = READINESS_STATE_VERSION;
}
const state = storedState?.active
? { ...storedState }
: {
Expand Down Expand Up @@ -640,6 +722,7 @@ jobs:
readinessState,
readiness,
[
...headDriftNotice,
"This PR stays in draft until every box above is ticked."
]
);
Expand Down Expand Up @@ -758,6 +841,7 @@ jobs:
readinessState,
readiness,
[
...headDriftNotice,
checklistComplete
? "✅ **All four boxes are ticked.** This PR still stays in draft until the issues above are resolved."
: pr.draft || draftConverted
Expand Down Expand Up @@ -811,6 +895,7 @@ jobs:
readinessState,
readiness,
[
...headDriftNotice,
pr.draft || draftConverted
? "This PR stays in draft until every box above is ticked."
: "Automatic draft conversion failed. Please convert this pull request to a draft manually until every box above is ticked."
Expand Down Expand Up @@ -930,11 +1015,18 @@ jobs:
notified = true;
}

// Bind the completion to the exact head it attested. A later
// `synchronize` event with a different head resets the checklist
// and the notification state (see `headDrifted` above).
readinessState.completedAtHeadSha = pr.head.sha;
Comment thread
Wibias marked this conversation as resolved.
Comment thread
Wibias marked this conversation as resolved.
Comment thread
Wibias marked this conversation as resolved.
readinessState.version = READINESS_STATE_VERSION;

await upsertReadinessComment(
readinessState,
readiness,
[
"✅ **All four boxes are ticked.**",
`Completed against head ${inlineCode(pr.head.sha.slice(0, 7))}; new commits after this will reset the checklist.`,
readyConverted
? "This pull request has been marked Ready for Review."
: pr.draft
Expand Down
5 changes: 4 additions & 1 deletion AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,10 @@ stay there until a four-box review-readiness checklist in the description is
complete: local CI green, branch on the latest `dev` commit, all correct Codex
and CodeRabbit findings fixed, and the ready-for-review confirmation. When all
four boxes are ticked the gate marks the PR ready and notifies the maintainers
listed in `MAINTAINERS.md` (excluding the author).
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.
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
4 changes: 4 additions & 0 deletions MAINTAINERS.md
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,10 @@ see [The retired `dev2-go` line](#the-retired-dev2-go-line).
all correct Codex and CodeRabbit findings fixed, and the ready-for-review
confirmation. When all four boxes are ticked the gate marks the PR ready and
notifies the maintainers 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.
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
9 changes: 6 additions & 3 deletions docs-site/src/content/docs/contributing/pr-quality.md
Original file line number Diff line number Diff line change
Expand Up @@ -46,9 +46,12 @@ tells you exactly what to change:
commit, all correct Codex and CodeRabbit findings fixed, and the
ready-for-review confirmation. Once every box is ticked the check marks the
PR ready for review and notifies the maintainers listed in `MAINTAINERS.md`
(excluding the author). A retarget to `dev` clears the wrong-branch message
automatically and is remembered by the gate; the draft stays until the
checklist is complete.
(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 maintainer notification, and asks you
to test and tick the boxes again against the latest code. A retarget to
`dev` clears the wrong-branch message automatically and is remembered by the
gate; the draft stays until the checklist is complete.

- **Hygiene.** Behavior changes need a test; new lint or type suppressions,
focused or skipped tests, empty catch blocks, edited generated output, and a
Expand Down
Loading
Loading