stack 2/5: gate review readiness and CodeRabbit - #901
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
Stack navigation
Review and merge bottom-up. Each PR targets the preceding stack branch, so its Files changed view contains only that layer. |
[shipping-github] Verdict: gatedPR: Semantic propagation
UsefulnessHigh: this layer is what makes the stack safe to run. It separates "author must act" from "automation still running", so #900's five-day stale closure cannot close PRs that are merely pending CI, and it prevents human review from starting against red or incomplete gates. Auto-draft ownership ( Bugs / correctness
Security
Spec / standards
Reviews
Base / CI
GateDraft gate: the PR remains a GitHub draft, so this verdict is Simplification (approved S1)
Bottom lineSubstantively clean and approve-worthy: the race fix closes the one real correctness hole, permissions are job-scoped, and the branch is current with its parent. Merge remains gated by the draft status, the missing maintainer review request (security boundary), and the stack's standing requirement that the synthetic fork test pass before these checks become required. When #902 is reviewed, it will need the same base update - it is still based on the old |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 53ed63a1c8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (pending.length > 0 || observed === 0) { | ||
| return { state: "validating", failed: [], pending }; | ||
| } | ||
| return { state: "maintainer", failed: [], pending: [] }; |
There was a problem hiding this comment.
When a new PR passes admission before CodeRabbit or path-filtered CI has created a status/check run, the successful PR admission / admission run makes observed nonzero, so this branch immediately returns maintainer. The workflow can then apply awaiting-maintainer and restore an auto-drafted PR even though CodeRabbit and CI have not reported—and it remains incorrectly ready if an expected integration never reports at all. Track the required gate contexts explicitly, or exclude admission from the evidence that all post-admission gates have appeared.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
[shipping-github] Fixed in 1acd01d4: admission check runs are now excluded from post-admission evidence (ADMISSION_NAMES), so a PR whose only check is a successful admission run stays intake: validating until CodeRabbit/CI report. Covered by new unit tests and a harness probe.
| permissions: {} | ||
|
|
||
| concurrency: | ||
| group: pr-readiness-${{ github.event.pull_request.number || github.event.check_run.head_sha || github.event.sha || 'sweep' }} |
There was a problem hiding this comment.
The concurrency key uses the PR number for pull_request_target, the head SHA for status/check events, and sweep for scheduled runs, so reconciliations of the same PR can run concurrently instead of cancelling one another. If one run reads pending checks while another reads their completed state, whichever finishes its label/draft mutations last can overwrite the newer verdict—for example, restoring ready-for-review after a newly pending check or re-drafting after all checks pass—until another event or the next 15-minute sweep repairs it. Route every event through a PR-specific concurrency key.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
[shipping-github] Fixed in 1acd01d4: the concurrency key now routes PR, status, and check_run events through the head SHA (github.event.pull_request.head.sha || check_run.head_sha || sha), so reconciliations of the same head serialize instead of running concurrently. The scheduled sweep keeps its own group as before (residual, self-healing).
| const checkRuns = await github.paginate(github.rest.checks.listForRef, { | ||
| owner, repo, ref: pr.head.sha, filter: "latest", per_page: 100, | ||
| }); |
There was a problem hiding this comment.
The Checks API's latest filter returns the latest run from each check suite, not the single latest run for each check name, so repeated workflow invocations on an unchanged head SHA remain in this array. For example, editing a PR body after a failed admission run starts a new suite that can pass and apply intake: admitted, but the earlier failed PR admission / admission run is still classified as a failure and leaves the PR in awaiting-author until a new commit changes the SHA. Reduce runs with the same check name to the newest invocation before assessing readiness.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
[shipping-github] Fixed in 1acd01d4: latestByCheckName now reduces runs per check name to the newest invocation (by started_at) before classification, so a superseded failed admission run no longer keeps the PR in awaiting-author after a newer run passes. Unit-tested.
| await convertToDraft(pr.node_id); | ||
| await add("intake: auto-drafted"); |
There was a problem hiding this comment.
If convertToDraft succeeds but the following label API call fails transiently, the catch only logs a warning after the PR has already become a draft. Later reconciliations see pr.draft === true, skip this entire block, and can no longer distinguish that draft from an author-created one, so intake: auto-drafted is never applied and the workflow will never restore ready-for-review when checks pass. Make the conversion and ownership marker recoverable as separate operations, such as recording ownership first and rolling it back if conversion fails.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
[shipping-github] Fixed in 1acd01d4: intake: auto-drafted is now applied before convertToDraft and rolled back if the conversion fails, so a draft produced by the workflow always carries its ownership marker and remains restorable.
| if (result.state === "maintainer") { | ||
| await remove("awaiting-author"); | ||
| await remove("intake: validating"); | ||
| await add("awaiting-maintainer"); | ||
| if (labels.has("intake: auto-drafted") && pr.draft) { |
There was a problem hiding this comment.
When an author-created draft has passing checks and lacks intake: auto-drafted, this branch still applies awaiting-maintainer and posts that maintainer review is next; the ownership check only prevents the GraphQL ready mutation. Maintainers triaging by this new label are therefore directed to review work the author has explicitly left as a draft. Treat pr.draft && !labels.has("intake: auto-drafted") as an author-controlled draft state and withhold the maintainer label/comment until a ready_for_review event.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
[shipping-github] Fixed in 1acd01d4: the maintainer branch now treats pr.draft && !labels.has("intake: auto-drafted") as an author-controlled draft and withholds awaiting-maintainer and the review-next comment until a ready_for_review event. Harness-verified.
| const IGNORED_NAMES = new Set([ | ||
| "PR readiness / reconcile", | ||
| "PR readiness", | ||
| ]); |
There was a problem hiding this comment.
GitHub Actions exposes the job name in check_run.name, so these jobs report as reconcile and admission, not the workflow/job display strings stored here. Consequently the current in-progress reconcile run is included in every readiness assessment and keeps pending nonempty, while admissionCheckPending fails to recognize an admission job that is still running; the gate can therefore never reach maintainer during 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.
[shipping-github] Fixed in 1acd01d4: check-run names now match the actual job names (reconcile / admission) with the display-name forms kept as fallbacks, in IGNORED_NAMES, ADMISSION_NAMES, and the job-level self-trigger guard. This also reactivates the admission-pending classification from 14417405, which previously never matched. Unit-tested for both name forms.
| @@ -0,0 +1,121 @@ | |||
| "use strict"; | |||
|
|
|||
| const { describe, it } = require("node:test"); | |||
There was a problem hiding this comment.
This new test file is not exercised by bun run test, which only runs tests/, and the unchanged issue-quality-tests.yml neither includes the readiness files in its path filters nor invokes this test. As a result, future changes can break the readiness classifier while every automated test job remains green; add both readiness files to that workflow's pull-request/push paths and run node --test .github/scripts/pr-readiness.test.cjs with the other CommonJS validator tests.
Useful? React with 👍 / 👎.
…pe runs, serialize reconciles, honor manual drafts
|
Holding this one, and I want to be specific about why, because the other four in the stack are not in the same position. I measured the lane before touching any of this. Over 33 recent Cross-platform CI runs: Windows was the last job to finish in 23 of 23 current-schema runs (median 17m41s, p90 18m54s) against ubuntu 5m58s and macos 5m06s, with a 43.5% failure rate of which 4 of 10 were runner flake rather than defects. That is the bottleneck, and #899 addresses it directly — its measured runs land at 4m26–4m30s wall clock. Against that, the readiness gate moves in the opposite direction: 1. It makes LLM judgment blocking. 2. It scales with the thing #899 is shrinking. It triggers per PR event, per status event, and per 3. It cannot stand alone. Without #900's admission label it holds PRs in author-action indefinitely. What I would merge: the same reconciler consuming the single aggregate Not closing this; it is a rebase and a config change away from something I want. Please say if you would rather I push those changes onto your branch than hand them back. |
|
Closing this one, with the reasoning already on the record above. The mechanism is not the problem — an auto-draft reconciler that moves a PR out of the review queue when it needs author action is something I want. Two things make this version unmergeable as written: The input is subjective. The cost scales the wrong way. It triggers per PR event, per status event, and per What I would merge: the same reconciler reading the single |
…n the docs Consolidates the parts of @Wibias's five-PR governance stack (lidge-jun#900, lidge-jun#902, lidge-jun#905) that reduce risk without adding friction, and drops the parts that do the opposite. The measurement behind the ordering: Windows was the last job to finish in 23 of 23 recent CI runs at a 17m41s median, so lidge-jun#899 was the actual bottleneck and everything here is judged by whether it makes the lane worse. Kept, from lidge-jun#902's trust lane: authentication, credential handling, GitHub Actions workflows, release automation, and dependency installation need a maintainer to sponsor the change before it merges. MAINTAINERS.md already requires security review for exactly these; this makes the requirement visible on the pull request instead of relying on a reviewer noticing. It runs inside the existing hygiene job rather than adding a workflow, and it applies to every contributor — blast radius does not depend on how many PRs someone has merged, which is why the upstream first-timer exemption is gone. Dropped, from the same PR: the 500-line cap and the one-open-PR limit. A provider preset with its registry rows, adapter wiring, tests, and five locales clears 500 lines by itself, and several good first contributions here have. Telling a newcomer their fix is too big is a worse failure than reviewing a large diff. Dropped, from lidge-jun#900: the admission gate requiring a pre-approved issue, and the five-day auto-close. The DeepSeek reasoning replay, the Cursor Grok parameters, the AgentRouter EOF tolerance, the tool-result image forwarding — every one arrived as an unplanned PR from someone who hit the bug. A gate that required a planning discussion first would have lost all of them. Kept, from lidge-jun#905: CODEOWNERS entries for the high-impact runtime directories, and the contributor documentation — rewritten to describe what is actually enforced. The submitted version documented the approved-for-work gate, the size caps, and the automatic closure timers, none of which exist here, and publishing rules the repository does not enforce is worse than publishing none. Not included: lidge-jun#901's readiness gate. It makes CodeRabbit's judgment blocking and triggers per check_run, which scales with the job count lidge-jun#899 just raised. Co-authored-by: Wibias <37517432+Wibias@users.noreply.github.com>
Stack
2/5 — review readiness and CodeRabbit gate
Base:
agent/pr-contribution-firewall(#900)Next: new-contributor trust lane
Summary
awaiting-author,intake: validating, orawaiting-maintainerVerification
node --test .github/scripts/pr-readiness.test.cjs— 7 passed, 0 failedRollout
Promote the trusted files to the default branch and exercise a synthetic external PR before making readiness or CodeRabbit required checks.