-
Notifications
You must be signed in to change notification settings - Fork 678
feat(ci): verify checklist claims and reset readiness on head drift #986
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
476688b
feat(ci): reset PR readiness checklist when new commits land after co…
Wibias bc9147f
fix(ci): bind checklist completion to the attested head and recover p…
Wibias 387da10
fix(ci): only reject completions when the checklist is actually ticked
Wibias ccc1098
refactor(ci): split enforce-target inline script into focused modules
Wibias dfaa8fd
refactor(ci): name PR-quality modules by responsibility
Wibias bbb20a2
fix(ci): harden readiness split helpers and synchronize provenance
Wibias c9c6309
fix(ci): match exact H2 heading in maintainer parser
Wibias cc48263
test(ci): cover valid CRLF maintainers heading
Wibias b1a59df
feat(ci): verify checklist claims on completion before lifting the draft
Wibias File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,35 @@ | ||
| "use strict"; | ||
|
|
||
| /** | ||
| * Maintainers from `MAINTAINERS.md` text. Only the current-maintainers table | ||
| * is authoritative; the change log below it can mention retired accounts. | ||
| * Missing the section heading means we cannot identify current maintainers, | ||
| * so the recipient list is empty rather than scanning the whole file. | ||
| */ | ||
| function parseMaintainerLogins(text) { | ||
| const heading = /^## Current maintainers[ \t]*\r?$/m.exec(text ?? ""); | ||
| if (heading === null) { | ||
| return []; | ||
| } | ||
|
|
||
| const sectionStart = heading.index; | ||
| const nextHeading = text.indexOf( | ||
| "\n## ", | ||
| sectionStart + "## Current maintainers".length | ||
| ); | ||
| const section = text.slice( | ||
| sectionStart, | ||
| nextHeading === -1 ? text.length : nextHeading | ||
| ); | ||
| const logins = [ | ||
| ...section.matchAll( | ||
| /\[\@([A-Za-z0-9_-]+)\]\(https:\/\/github\.com\/[^)]*\)/g | ||
| ) | ||
| ].map(match => match[1]); | ||
|
|
||
| return [...new Set(logins)]; | ||
| } | ||
|
|
||
| module.exports = { | ||
| parseMaintainerLogins | ||
| }; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,75 @@ | ||
| "use strict"; | ||
|
|
||
| const { describe, it } = require("node:test"); | ||
| const assert = require("node:assert/strict"); | ||
| const { | ||
| parseMaintainerLogins | ||
| } = require("./pr-maintainers.cjs"); | ||
|
|
||
| const FIXTURE = [ | ||
| "## Current maintainers", | ||
| "", | ||
| "| GitHub account | Project role | Responsibilities |", | ||
| "| --- | --- | --- |", | ||
| "| [@lidge-jun](https://github.com/lidge-jun) | Project owner | x |", | ||
| "| [@Ingwannu](https://github.com/Ingwannu) | Maintainer | x |", | ||
| "| [@Wibias](https://github.com/Wibias) | Maintainer | x |", | ||
| "", | ||
| "## Change log", | ||
| "", | ||
| "- [@Wibias](https://github.com/Wibias) was added as a maintainer.", | ||
| "- [@retired](https://github.com/retired) stepped down.", | ||
| ].join("\n"); | ||
|
|
||
| describe("parseMaintainerLogins", () => { | ||
| it("reads the current-maintainers table and excludes the change log", () => { | ||
| assert.deepEqual(parseMaintainerLogins(FIXTURE), [ | ||
| "lidge-jun", | ||
| "Ingwannu", | ||
| "Wibias", | ||
| ]); | ||
| }); | ||
|
|
||
| it("returns an empty list when the section heading is missing", () => { | ||
| const text = "- [@only](https://github.com/only) is listed."; | ||
| assert.deepEqual(parseMaintainerLogins(text), []); | ||
| }); | ||
| it("does not match a ### subsection or prose mentioning the heading", () => { | ||
| const subsection = [ | ||
| "### Current maintainers", | ||
| "| [@subsection](https://github.com/subsection) | x |", | ||
| ].join("\n"); | ||
| assert.deepEqual(parseMaintainerLogins(subsection), []); | ||
|
|
||
| const prose = [ | ||
| "See the ## Current maintainers section below.", | ||
| "| [@prose](https://github.com/prose) | x |", | ||
| ].join("\n"); | ||
| assert.deepEqual(parseMaintainerLogins(prose), []); | ||
| }); | ||
|
|
||
| it("accepts a valid CRLF heading with trailing whitespace and a following H2", () => { | ||
| const crlf = [ | ||
| "## Current maintainers \t", | ||
| "| [@crlf](https://github.com/crlf) | x |", | ||
| "## Changelog", | ||
| "| [@retired](https://github.com/retired) | y |", | ||
| ].join("\r\n"); | ||
| assert.deepEqual(parseMaintainerLogins(crlf), ["crlf"]); | ||
| }); | ||
|
|
||
|
|
||
| it("handles empty and duplicate-free output", () => { | ||
| assert.deepEqual(parseMaintainerLogins(""), []); | ||
| assert.deepEqual( | ||
| parseMaintainerLogins( | ||
| [ | ||
| "## Current maintainers", | ||
| "| [@dup](https://github.com/dup) | x |", | ||
| "| [@dup](https://github.com/dup) | y |", | ||
| ].join("\n"), | ||
| ), | ||
| ["dup"], | ||
| ); | ||
| }); | ||
| }); | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,207 @@ | ||
| "use strict"; | ||
|
|
||
| const { | ||
| REVIEW_READINESS_ITEMS | ||
| } = require("./pr-quality.cjs"); | ||
| const { | ||
| readinessStateMarker, | ||
| READINESS_LATEST_DEV_BEHIND_MAX | ||
| } = require("./pr-quality-state.cjs"); | ||
|
|
||
| /** Marks the bot's review-readiness checklist message. */ | ||
| const READINESS_MARKER = "<!-- pr-quality-readiness -->"; | ||
|
|
||
| function inlineCode(value) { | ||
| const text = String(value); | ||
| const longestBacktickRun = Math.max( | ||
| 0, | ||
| ...(text.match(/`+/g) ?? []).map(run => run.length) | ||
| ); | ||
| const delimiter = "`".repeat(longestBacktickRun + 1); | ||
| return `${delimiter}${text}${delimiter}`; | ||
| } | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
|
|
||
| function readinessChecklistLines(readiness) { | ||
| return REVIEW_READINESS_ITEMS.map( | ||
| (item, index) => | ||
| `- ${readiness.items?.[index]?.checked ? "✅" : "⬜"} ${item}` | ||
| ); | ||
| } | ||
|
|
||
| /** | ||
| * The full readiness-message body: marker, serialized state, mirror lines for | ||
| * the tickable boxes, the tick count, and the path-specific extra lines. | ||
| */ | ||
| function buildReadinessCommentBody(state, readiness, extra) { | ||
| const complete = readiness.present && readiness.complete; | ||
|
|
||
| return [ | ||
| READINESS_MARKER, | ||
| readinessStateMarker(state), | ||
| "", | ||
| "## Review readiness checklist", | ||
| "", | ||
| readiness.present | ||
| ? "This PR is kept in **draft** until every requirement below is fulfilled. The tickable checklist has been added to your PR description — tick all four boxes there." | ||
| : "The review readiness checklist is not required for this author.", | ||
| "", | ||
| ...(readiness.present ? readinessChecklistLines(readiness) : []), | ||
| "", | ||
| readiness.present | ||
| ? complete | ||
| ? "✅ **4/4** boxes ticked." | ||
| : `**${readiness.checked}/${readiness.total}** boxes ticked.` | ||
| : "", | ||
| "", | ||
| ...extra | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| ]; | ||
| } | ||
|
|
||
| function descriptionFailureLines(reason) { | ||
| switch (reason) { | ||
| case "empty": | ||
| return [ | ||
| "The pull request body is empty after stripping HTML comments.", | ||
| "", | ||
| "Include a real description: a **Summary** of what changed and why, plus a **Test plan** (or equivalent substance)." | ||
| ]; | ||
| case "placeholder": | ||
| return [ | ||
| "The pull request body contains only placeholder text (for example `N/A`, `TODO`, or `No response`).", | ||
| "", | ||
| "Replace placeholders with a **Summary** and **Test plan**, or another description with at least two substantive sections or paragraphs." | ||
| ]; | ||
| case "escaped_newlines": | ||
| return [ | ||
| "The pull request body uses literal `\\n` escape sequences instead of real line breaks.", | ||
| "", | ||
| "Fix the formatting so the body uses normal markdown line breaks, then add a **Summary** and **Test plan**." | ||
| ]; | ||
| case "thin": | ||
| default: | ||
| return [ | ||
| "The pull request description is too thin to review.", | ||
| "", | ||
| "Add a **Summary** and **Test plan** (two sections with at least 40 characters each), or an unstructured body of at least 120 characters with two paragraphs or bullet groups." | ||
| ]; | ||
| } | ||
| } | ||
|
|
||
| function buildFailureSections(failures, { pr, allowedBases, defaultBase }) { | ||
| const sections = []; | ||
|
|
||
| if (failures.some(failure => failure.code === "wrong_base")) { | ||
| sections.push( | ||
| "⚠️ **Wrong target branch**", | ||
| "", | ||
| `This pull request currently targets ${inlineCode(pr.base.ref)}, but pull requests must target one of ${allowedBases.map(inlineCode).join(" or ")}.`, | ||
| "", | ||
| `@${pr.user.login} Please retarget this PR to ${inlineCode(defaultBase)}. All contributions go to ${inlineCode(defaultBase)}; \`main\` receives only release promotions. See our [Contributing guide](https://lidge-jun.github.io/opencodex/contributing/) for details. Thanks! 🙏` | ||
| ); | ||
| } | ||
|
|
||
| if (failures.some(failure => failure.code === "wrong_ancestry")) { | ||
| sections.push( | ||
| "⚠️ **Wrong branch ancestry**", | ||
| "", | ||
| `This pull request targets ${inlineCode(pr.base.ref)}, but its head appears to sit on the current ${inlineCode("main")} tip while being far behind ${inlineCode(pr.base.ref)}.`, | ||
| "", | ||
| `@${pr.user.login} Rebase onto the current ${inlineCode(pr.base.ref)} branch instead of opening from ${inlineCode("main")}. That keeps already-released commits out of the integration branch.` | ||
| ); | ||
| } | ||
|
|
||
| const badDescription = failures.find( | ||
| failure => failure.code === "bad_description" | ||
| ); | ||
| if (badDescription) { | ||
| sections.push( | ||
| "⚠️ **Pull request description**", | ||
| "", | ||
| ...descriptionFailureLines(badDescription.reason) | ||
| ); | ||
| } | ||
|
|
||
| if ( | ||
| failures.some( | ||
| failure => failure.code === "missing_ui_screenshot" | ||
| ) | ||
| ) { | ||
| sections.push( | ||
| "⚠️ **UI screenshot required**", | ||
| "", | ||
| `This pull request mentions ${inlineCode("gui")} in its title or description, so it is treated as a GUI change.`, | ||
| "", | ||
| `@${pr.user.login} Please add a screenshot of the UI change to the description — drag and drop the image into the description editor, or paste a markdown image such as ${inlineCode("")}. The check re-runs automatically once the description is edited.` | ||
| ); | ||
| } | ||
|
|
||
| return sections; | ||
| } | ||
|
|
||
| function failureSummary(failures, { pr }) { | ||
| return failures | ||
| .map(failure => { | ||
| if (failure.code === "wrong_base") { | ||
| return `wrong base (${pr.base.ref})`; | ||
| } | ||
| if (failure.code === "wrong_ancestry") { | ||
| return "wrong ancestry"; | ||
| } | ||
| if (failure.code === "bad_description") { | ||
| return `bad description (${failure.reason})`; | ||
| } | ||
| if (failure.code === "missing_ui_screenshot") { | ||
| return "missing UI screenshot"; | ||
| } | ||
| return failure.code; | ||
| }) | ||
| .join("; "); | ||
| } | ||
|
|
||
| /** The notice shown when the gate's own claim check disproves a ticked box. */ | ||
| 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") { | ||
| lines.push( | ||
| `The PR is more than ${READINESS_LATEST_DEV_BEHIND_MAX} commits behind ${inlineCode("dev")}; the **latest dev** box has been unticked.` | ||
| ); | ||
| } | ||
| } | ||
| lines.push( | ||
| "The checklist has been reset: re-test against the latest code and tick the boxes again." | ||
| ); | ||
| return lines; | ||
| } | ||
|
|
||
| /** The reset notice shown when a completion no longer covers the live head. */ | ||
| function buildStaleNotice({ completionHeadSha, liveHeadSha, eventAction }) { | ||
| let lead; | ||
| if (completionHeadSha !== null) { | ||
| lead = `New commits were pushed after the checklist was completed on ${inlineCode(String(completionHeadSha).slice(0, 7))}; the current head is ${inlineCode(liveHeadSha.slice(0, 7))}.`; | ||
| } else if (eventAction === "synchronize") { | ||
| lead = `A complete checklist was found on a synchronize event with no recorded completion head; the current head is ${inlineCode(liveHeadSha.slice(0, 7))}.`; | ||
| } else { | ||
| lead = `The checklist was ticked before the current head ${inlineCode(liveHeadSha.slice(0, 7))} was pushed.`; | ||
| } | ||
| return [ | ||
| lead, | ||
| "The checklist has been reset: re-test against the latest code and tick all four boxes again." | ||
| ]; | ||
| } | ||
|
|
||
| module.exports = { | ||
| READINESS_MARKER, | ||
| inlineCode, | ||
| readinessChecklistLines, | ||
| buildReadinessCommentBody, | ||
| descriptionFailureLines, | ||
| buildFailureSections, | ||
| failureSummary, | ||
| buildStaleNotice, | ||
| buildClaimCheckNotice | ||
| }; | ||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.