stack 5/5: define review lifecycle and governance rollout - #905
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. |
|
Two things here, one mechanical and one about content. Mechanical: this PR's head is not a descendant of #903's current head — their merge base is Content: CODEOWNERS and the contributor/governance docs I want. The review-lifecycle tracker I am less sure of: Where this leaves the stack, having reviewed all five together and measured the CI lane first:
If you want to split this into a CODEOWNERS-plus-docs PR against Thanks for the volume of work in this stack — the specs in |
[shipping-github] Verdict: gatedPR: Semantic propagation
UsefulnessHigh: closes the stack with review-round accounting, standardized closure dispositions, the exact owner/admin ruleset checklist, public contributor docs, and the CI wiring that makes every validator testable. The live policy-test run proves the wiring end to end. Bugs / correctness
Security
Spec / standards
Reviews
Base / CI
GateDraft gate: the PR remains a GitHub draft, so this verdict is Bottom lineMechanically clean and approve-worthy after O1 + F1-F3 + S1: the stack link is fixed, the label fights and permission bug are gone, approvals clear rounds, and the CI wiring is proven live. Two things remain before this layer can move: the draft status, and the owner's O2 decision on what counts as a review round - which the author has deferred for a follow-up look. A reply to @lidge-jun acknowledging the two points and stating the plan (base fixed; tracker signal to be redefined and agreed) is drafted for author confirmation. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d3f2e3b043
ℹ️ 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".
| on: | ||
| pull_request_review: | ||
| types: [submitted] |
There was a problem hiding this comment.
For reviews on external fork PRs—the primary scenario documented by this rollout—GitHub downgrades GITHUB_TOKEN to read-only, regardless of the requested issues: write and pull-requests: write permissions. Consequently, the label and comment mutations below fail with Resource not accessible by integration, so review rounds are never recorded for external contributors. Keep the untrusted head unchecked-out, but perform mutations from a trusted write-capable follow-up such as a carefully validated workflow_run.
Useful? React with 👍 / 👎.
| }); | ||
| // An approval supersedes prior rounds: clear the round and limit | ||
| // labels so an approved PR is not still labeled as review churn. | ||
| if (String(review.state || "").toLowerCase() === "approved") { |
There was a problem hiding this comment.
When any user with public read access submits an APPROVED review, this branch clears all review-round and limit labels without checking reviewerHasPushPermission. Non-maintainer approvals do not satisfy GitHub's required-review policy, but here they can erase the workflow's maintainer disposition signal; require the already-computed push-permission check before performing this cleanup.
Useful? React with 👍 / 👎.
| core.info("Approval clears review-round labels."); | ||
| return; |
There was a problem hiding this comment.
If a maintainer approves after one or more rounds and the author subsequently pushes another revision, this branch removes only the visible labels while leaving the hidden comment state unchanged. The next change request resumes the old count and can immediately restore review: limit-reached, contradicting the stated superseding behavior; reset or remove the persisted lifecycle state before returning.
Useful? React with 👍 / 👎.
|
|
||
| if (String(reviewState || "").toLowerCase() !== "changes_requested") return result; | ||
| if (!reviewerHasPushPermission || !isSubstantialReview(reviewBody)) return result; | ||
| if (!headSha || headSha === current.lastCountedHeadSha) return result; |
There was a problem hiding this comment.
When an author force-pushes a previously reviewed head again—for example, the sequence A → B → A—the final A differs from lastCountedHeadSha and is counted a second time. This violates the documented once-per-distinct-SHA rule and can reach the limit with only one unique revision being counted twice; persist the set of counted head SHAs instead of only the last value.
Useful? React with 👍 / 👎.
| if (!result.counted) { | ||
| core.info("Review did not start a new substantial maintainer review round."); | ||
| return; | ||
| } |
There was a problem hiding this comment.
On a fresh rollout, closure reasons such as close: no-approved-issue and close: abandoned do not exist until a substantial maintainer change request is counted, because all other review events return before ensureLabel runs. Those labels are specifically needed for PRs that may be closed before human review, so initialize the taxonomy during rollout or before this early return.
Useful? React with 👍 / 👎.
| ## 4. Enable merge queue | ||
|
|
||
| Enable the merge queue for `dev` after required checks are stable. Require queued commits to rerun the same checks against the current integration state. Do not enable auto-merge as a substitute for approvals or unresolved-thread checks. |
There was a problem hiding this comment.
The rollout instructs the administrator to enable a merge queue and rerun the same required checks, but none of the listed required workflows (enforce-pr-target, admission, readiness, trust lane, hygiene, or cross-platform CI) declares a merge_group trigger. Once the queue is enabled, those checks therefore do not report for the synthetic merge-group commit and queued PRs cannot merge; add merge-group-capable checks and adapt PR-specific jobs before performing this step.
Useful? React with 👍 / 👎.
| - `PR readiness / reconcile` | ||
| - `PR trust lane / trust-lane` | ||
| - `PR hygiene / hygiene` | ||
| - the cross-platform CI jobs required by current release policy |
There was a problem hiding this comment.
The proposed ruleset globally requires the cross-platform CI jobs, while .github/workflows/ci.yml runs on pull requests only when one of its paths entries changes. A documentation-only or governance-only PR therefore never creates those required checks and remains blocked indefinitely; remove the workflow-level path filter or add an always-running required aggregate check that succeeds when the expensive matrix is intentionally skipped.
Useful? React with 👍 / 👎.
| ## Pull request disposition | ||
|
|
||
| Use one standardized closure label and a concise comment naming the reopening condition: |
There was a problem hiding this comment.
The new policy requires every closure to carry one standardized reason, but the existing .github/workflows/stale-author-prs.yml closes inactive awaiting-author PRs without applying close: abandoned. Automated abandonment closures consequently violate the policy and cannot be included reliably in the rollout's closure-by-reason measurements; have the stale workflow apply that label before closing.
Useful? React with 👍 / 👎.
| const result = applyReviewEvent({ | ||
| state: storedState, |
There was a problem hiding this comment.
The workflow invokes round accounting for every maintainer CHANGES_REQUESTED review without checking whether the PR carries awaiting-maintainer. A maintainer comment on a draft, intake-blocked, or already awaiting-author PR therefore consumes one of the two allowed rounds even though the new policy explicitly says human review starts only after readiness gates pass; gate accounting on the live readiness label.
Useful? React with 👍 / 👎.
| reviewState: review.state, | ||
| reviewBody: review.body, | ||
| reviewerHasPushPermission, |
There was a problem hiding this comment.
For a maintainer review that requests changes through detailed inline comments but leaves the optional review summary blank or shorter than 40 characters, only review.body is passed to isSubstantialReview, so the round is silently ignored. Fetch the comments belonging to the submitted review and include their actionable text when deciding whether the review is substantial.
Useful? React with 👍 / 👎.
|
CODEOWNERS and the contributor docs are in #920 with your co-authorship. Closing this rather than leaving it open, since its head was never a descendant of #903 anyway (merge base Kept: the CODEOWNERS entries for Rewritten: the docs described an Dropped: the review-lifecycle tracker. "Substantial review" at 40 characters is satisfiable by padding — "looks good to me, ship it" clears the bar while a precise one-line objection to a specific hunk might not. A heuristic that can be gamed by typing more is not measuring review quality. If you want to bring this back with a signal that resists that, I will read it properly. The specs under |
…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
5/5 — review lifecycle and governance rollout
Base:
agent/pr-hygiene-gate(#903)Final stack layer
Summary
review: limit-reachedafter two unsuccessful rounds without auto-closingVerification
node --test .github/scripts/pr-review-lifecycle.test.cjs— 7 passed, 0 failedImportant limitation
The connected
Wibiasaccount has write access but not repository admin access. This PR documents the exact ruleset and merge-queue settings, but the project owner or another administrator must activate them after the synthetic fork test succeeds.