-
Notifications
You must be signed in to change notification settings - Fork 687
stack 2/5: gate review readiness and CodeRabbit #901
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
Changes from all commits
96b2191
f853ca7
1441740
53ed63a
1acd01d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,138 @@ | ||
| "use strict"; | ||
|
|
||
| const ACCEPTABLE_CONCLUSIONS = new Set(["success", "neutral", "skipped"]); | ||
| const BLOCKING_CONCLUSIONS = new Set([ | ||
| "failure", | ||
| "cancelled", | ||
| "timed_out", | ||
| "action_required", | ||
| "stale", | ||
| "startup_failure", | ||
| ]); | ||
| // GitHub Actions reports the job name as the check-run name, so the reconcile | ||
| // job's own runs surface as "reconcile". Both the display and job-name forms | ||
| // are matched so an in-progress reconcile run can never block readiness. | ||
| const IGNORED_NAMES = new Set([ | ||
| "reconcile", | ||
| "PR readiness / reconcile", | ||
| "PR readiness", | ||
| ]); | ||
| // Admission state is conveyed by the `intake: admitted` label; admission check | ||
| // runs must not count as post-admission evidence, or a PR whose only check is | ||
| // a successful admission run would be declared maintainer-ready before | ||
| // CodeRabbit or CI ever report. | ||
| const ADMISSION_NAMES = new Set([ | ||
| "admission", | ||
| "PR admission / admission", | ||
| ]); | ||
|
|
||
| function normalizeName(value) { | ||
| return String(value || "").trim(); | ||
| } | ||
|
|
||
| function isManagedCheckName(name) { | ||
| return IGNORED_NAMES.has(name) || ADMISSION_NAMES.has(name); | ||
| } | ||
|
|
||
| // The Checks API's `latest` filter returns the newest run per check suite, not | ||
| // per check name, so repeated invocations on an unchanged head SHA coexist. | ||
| // Keep only the newest run per check name before classifying. | ||
| function latestByCheckName(checkRuns) { | ||
| const latest = new Map(); | ||
| for (const check of checkRuns || []) { | ||
| const name = normalizeName(check.name); | ||
| if (!name) continue; | ||
| const existing = latest.get(name); | ||
| if ( | ||
| !existing || | ||
| String(check.started_at || "") >= String(existing.started_at || "") | ||
| ) { | ||
| latest.set(name, check); | ||
| } | ||
| } | ||
| return [...latest.values()]; | ||
| } | ||
|
|
||
| function classifyStatuses(statuses) { | ||
| const pending = []; | ||
| const failed = []; | ||
| let observed = 0; | ||
|
|
||
| for (const status of statuses || []) { | ||
| const name = normalizeName(status.context); | ||
| if (!name || isManagedCheckName(name)) continue; | ||
| observed += 1; | ||
| if (status.state === "pending") pending.push(name); | ||
| else if (status.state !== "success") failed.push(name); | ||
| } | ||
|
|
||
| return { pending, failed, observed }; | ||
| } | ||
|
|
||
| function classifyCheckRuns(checkRuns) { | ||
| const pending = []; | ||
| const failed = []; | ||
| let observed = 0; | ||
|
|
||
| for (const check of latestByCheckName(checkRuns)) { | ||
| const name = normalizeName(check.name); | ||
| if (!name || isManagedCheckName(name)) continue; | ||
| observed += 1; | ||
| if (check.status !== "completed") { | ||
| pending.push(name); | ||
| continue; | ||
| } | ||
| const conclusion = check.conclusion || ""; | ||
| if (BLOCKING_CONCLUSIONS.has(conclusion)) failed.push(name); | ||
| else if (!ACCEPTABLE_CONCLUSIONS.has(conclusion)) pending.push(name); | ||
| } | ||
|
|
||
| return { pending, failed, observed }; | ||
| } | ||
|
|
||
| function admissionCheckPending(checkRuns) { | ||
| const admission = latestByCheckName(checkRuns).find( | ||
| (check) => ADMISSION_NAMES.has(normalizeName(check.name)), | ||
| ); | ||
| return Boolean(admission && admission.status !== "completed"); | ||
| } | ||
|
|
||
| function assessReadiness({ admissionPassed, statuses = [], checkRuns = [] }) { | ||
| if (!admissionPassed) { | ||
| if (admissionCheckPending(checkRuns)) { | ||
| return { | ||
| state: "validating", | ||
| failed: [], | ||
| pending: ["PR admission"], | ||
| }; | ||
| } | ||
| return { | ||
| state: "author_action", | ||
| failed: ["PR admission"], | ||
| pending: [], | ||
| }; | ||
| } | ||
|
|
||
| const statusResult = classifyStatuses(statuses); | ||
| const checkResult = classifyCheckRuns(checkRuns); | ||
| const failed = [...new Set([...statusResult.failed, ...checkResult.failed])]; | ||
| const pending = [...new Set([...statusResult.pending, ...checkResult.pending])]; | ||
| const observed = statusResult.observed + checkResult.observed; | ||
|
|
||
| if (failed.length > 0) return { state: "author_action", failed, pending }; | ||
| if (pending.length > 0 || observed === 0) { | ||
| return { state: "validating", failed: [], pending }; | ||
| } | ||
| return { state: "maintainer", failed: [], pending: [] }; | ||
|
Comment on lines
+123
to
+126
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a new PR passes admission before CodeRabbit or path-filtered CI has created a status/check run, the successful Useful? React with 👍 / 👎.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [shipping-github] Fixed in |
||
| } | ||
|
|
||
| module.exports = { | ||
| ACCEPTABLE_CONCLUSIONS, | ||
| ADMISSION_NAMES, | ||
| BLOCKING_CONCLUSIONS, | ||
| admissionCheckPending, | ||
| assessReadiness, | ||
| classifyCheckRuns, | ||
| classifyStatuses, | ||
| latestByCheckName, | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,193 @@ | ||
| "use strict"; | ||
|
|
||
| const { describe, it } = require("node:test"); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This new test file is not exercised by Useful? React with 👍 / 👎.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| const assert = require("node:assert/strict"); | ||
| const { | ||
| admissionCheckPending, | ||
| assessReadiness, | ||
| classifyCheckRuns, | ||
| classifyStatuses, | ||
| latestByCheckName, | ||
| } = require("./pr-readiness.cjs"); | ||
|
|
||
| describe("assessReadiness", () => { | ||
| it("keeps failed admission in author-action state", () => { | ||
| assert.deepEqual( | ||
| assessReadiness({ admissionPassed: false }), | ||
| { state: "author_action", failed: ["PR admission"], pending: [] }, | ||
| ); | ||
| }); | ||
|
|
||
| it("keeps PR validating while the admission check is still running", () => { | ||
| const result = assessReadiness({ | ||
| admissionPassed: false, | ||
| checkRuns: [{ name: "PR admission / admission", status: "in_progress" }], | ||
| }); | ||
| assert.equal(result.state, "validating"); | ||
| assert.deepEqual(result.pending, ["PR admission"]); | ||
| }); | ||
|
|
||
| it("recognizes a pending admission check by its job name", () => { | ||
| const result = assessReadiness({ | ||
| admissionPassed: false, | ||
| checkRuns: [{ name: "admission", status: "in_progress" }], | ||
| }); | ||
| assert.equal(result.state, "validating"); | ||
| assert.equal(admissionCheckPending([{ name: "admission", status: "in_progress" }]), true); | ||
| }); | ||
|
|
||
| it("returns author action when admission completed with a failure", () => { | ||
| const result = assessReadiness({ | ||
| admissionPassed: false, | ||
| checkRuns: [ | ||
| { name: "PR admission / admission", status: "completed", conclusion: "failure" }, | ||
| ], | ||
| }); | ||
| assert.equal(result.state, "author_action"); | ||
| }); | ||
|
|
||
| it("keeps PR validating while checks are pending", () => { | ||
| const result = assessReadiness({ | ||
| admissionPassed: true, | ||
| statuses: [{ context: "CodeRabbit", state: "pending" }], | ||
| checkRuns: [{ name: "Cross-platform CI", status: "in_progress" }], | ||
| }); | ||
| assert.equal(result.state, "validating"); | ||
| assert.deepEqual(result.pending.sort(), ["CodeRabbit", "Cross-platform CI"]); | ||
| }); | ||
|
|
||
| it("returns author action for failed status or check", () => { | ||
| const result = assessReadiness({ | ||
| admissionPassed: true, | ||
| statuses: [{ context: "CodeRabbit", state: "failure" }], | ||
| checkRuns: [{ name: "tests", status: "completed", conclusion: "success" }], | ||
| }); | ||
| assert.equal(result.state, "author_action"); | ||
| assert.deepEqual(result.failed, ["CodeRabbit"]); | ||
| }); | ||
|
|
||
| it("returns maintainer only after every observed check passes", () => { | ||
| const result = assessReadiness({ | ||
| admissionPassed: true, | ||
| statuses: [{ context: "CodeRabbit", state: "success" }], | ||
| checkRuns: [ | ||
| { name: "tests", status: "completed", conclusion: "success" }, | ||
| { name: "docs", status: "completed", conclusion: "skipped" }, | ||
| ], | ||
| }); | ||
| assert.deepEqual(result, { state: "maintainer", failed: [], pending: [] }); | ||
| }); | ||
|
|
||
| it("does not treat a successful admission run as post-admission evidence", () => { | ||
| const result = assessReadiness({ | ||
| admissionPassed: true, | ||
| checkRuns: [{ name: "admission", status: "completed", conclusion: "success" }], | ||
| }); | ||
| assert.equal(result.state, "validating"); | ||
| }); | ||
|
|
||
| it("does not claim readiness when no checks were observed", () => { | ||
| assert.equal( | ||
| assessReadiness({ admissionPassed: true }).state, | ||
| "validating", | ||
| ); | ||
| }); | ||
|
|
||
| it("ignores its own readiness check to avoid recursion", () => { | ||
| const result = assessReadiness({ | ||
| admissionPassed: true, | ||
| statuses: [{ context: "CodeRabbit", state: "success" }], | ||
| checkRuns: [ | ||
| { name: "PR readiness / reconcile", status: "in_progress" }, | ||
| ], | ||
| }); | ||
| assert.equal(result.state, "maintainer"); | ||
| }); | ||
|
|
||
| it("ignores the reconcile job by its actual check-run name", () => { | ||
| const result = assessReadiness({ | ||
| admissionPassed: true, | ||
| statuses: [{ context: "CodeRabbit", state: "success" }], | ||
| checkRuns: [ | ||
| { name: "reconcile", status: "in_progress" }, | ||
| ], | ||
| }); | ||
| assert.equal(result.state, "maintainer"); | ||
| }); | ||
| }); | ||
|
|
||
| describe("classifyCheckRuns", () => { | ||
| it("treats action_required and timed_out as failures", () => { | ||
| const result = classifyCheckRuns([ | ||
| { name: "a", status: "completed", conclusion: "action_required" }, | ||
| { name: "b", status: "completed", conclusion: "timed_out" }, | ||
| ]); | ||
| assert.deepEqual(result.failed, ["a", "b"]); | ||
| }); | ||
|
|
||
| it("keeps only the newest run per check name", () => { | ||
| const result = classifyCheckRuns([ | ||
| { name: "Cross-platform CI", status: "completed", conclusion: "failure", started_at: "2026-08-01T00:00:00Z" }, | ||
| { name: "Cross-platform CI", status: "completed", conclusion: "success", started_at: "2026-08-02T00:00:00Z" }, | ||
| ]); | ||
| assert.equal(result.observed, 1); | ||
| assert.deepEqual(result.failed, []); | ||
| }); | ||
|
|
||
| it("ignores admission runs in readiness evidence", () => { | ||
| const result = classifyCheckRuns([ | ||
| { name: "admission", status: "completed", conclusion: "success" }, | ||
| ]); | ||
| assert.equal(result.observed, 0); | ||
| }); | ||
| }); | ||
|
|
||
| describe("latestByCheckName", () => { | ||
| it("picks the newest run when started_at is present", () => { | ||
| const runs = latestByCheckName([ | ||
| { name: "a", started_at: "2026-08-01T00:00:00Z" }, | ||
| { name: "a", started_at: "2026-08-02T00:00:00Z" }, | ||
| { name: "b", started_at: "2026-08-01T00:00:00Z" }, | ||
| ]); | ||
| assert.deepEqual( | ||
| runs.map((r) => r.started_at).sort(), | ||
| ["2026-08-01T00:00:00Z", "2026-08-02T00:00:00Z"], | ||
| ); | ||
| }); | ||
| }); | ||
|
|
||
| describe("classifyStatuses", () => { | ||
| it("ignores readiness and admission status contexts", () => { | ||
| const result = classifyStatuses([ | ||
| { context: "CodeRabbit", state: "success" }, | ||
| { context: "reconcile", state: "pending" }, | ||
| { context: "admission", state: "failure" }, | ||
| ]); | ||
| assert.equal(result.observed, 1); | ||
| assert.deepEqual(result.failed, []); | ||
| }); | ||
| }); | ||
|
|
||
| describe("admissionCheckPending", () => { | ||
| it("treats an in-progress admission check as pending", () => { | ||
| assert.equal( | ||
| admissionCheckPending([ | ||
| { name: "PR admission / admission", status: "in_progress" }, | ||
| ]), | ||
| true, | ||
| ); | ||
| }); | ||
|
|
||
| it("treats a completed admission check as not pending", () => { | ||
| assert.equal( | ||
| admissionCheckPending([ | ||
| { name: "PR admission / admission", status: "completed", conclusion: "failure" }, | ||
| ]), | ||
| false, | ||
| ); | ||
| }); | ||
|
|
||
| it("is false when no admission check is present", () => { | ||
| assert.equal(admissionCheckPending([]), false); | ||
| }); | ||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GitHub Actions exposes the job name in
check_run.name, so these jobs report asreconcileandadmission, not the workflow/job display strings stored here. Consequently the current in-progressreconcilerun is included in every readiness assessment and keepspendingnonempty, whileadmissionCheckPendingfails to recognize an admission job that is still running; the gate can therefore never reachmaintainerduring reconciliation and can transiently classify pending admission as an author failure. Match the actual job names, ideally together with the GitHub Actions app identity, when ignoring and locating these runs.Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[shipping-github] Fixed in
1acd01d4: check-run names now match the actual job names (reconcile/admission) with the display-name forms kept as fallbacks, inIGNORED_NAMES,ADMISSION_NAMES, and the job-level self-trigger guard. This also reactivates the admission-pending classification from14417405, which previously never matched. Unit-tested for both name forms.