stack 5/5: define review lifecycle and governance rollout #905
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
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,67 @@ | ||
| # Contribution firewall rollout | ||
|
|
||
| The workflows in the five-PR stack are inert for external pull requests until their trusted scripts and workflow definitions are on the repository default branch. Do not configure required checks before the synthetic-fork validation below. | ||
|
|
||
| ## 1. Promote trusted automation | ||
|
|
||
| Promote the merged `dev` versions of these files to the default branch: | ||
|
|
||
| - `.github/workflows/enforce-pr-target.yml` | ||
| - `.github/workflows/pr-admission.yml` | ||
| - `.github/workflows/pr-readiness.yml` | ||
| - `.github/workflows/pr-trust-lane.yml` | ||
| - `.github/workflows/pr-hygiene.yml` | ||
| - `.github/workflows/pr-review-lifecycle.yml` | ||
| - `.github/workflows/stale-author-prs.yml` | ||
| - their corresponding `.github/scripts/*.cjs` files | ||
| - `.coderabbit.yaml` | ||
|
|
||
| ## 2. Synthetic fork test | ||
|
|
||
| Open a fork PR against `dev` and prove each transition: | ||
|
|
||
| 1. Missing approved issue and unchecked attestations fail admission and produce `awaiting-author`. | ||
| 2. Correcting intake moves to `intake: validating` while checks run. | ||
| 3. A failing CI or CodeRabbit check returns to `awaiting-author` and keeps the PR draft. | ||
| 4. All checks passing produces `awaiting-maintainer` and restores ready-for-review only when automation owned the draft. | ||
| 5. A first-time contributor is blocked by a second active implementation PR, an unapproved change over 500 lines, and a restricted security/release surface without sponsorship. | ||
| 6. Hygiene fixtures prove missing tests, suppressions, focused tests, empty catches, generated output, and lockfile churn fail. | ||
| 7. Two `CHANGES_REQUESTED` reviews on distinct head SHAs produce `review: limit-reached`; duplicate reviews on one SHA do not increment. | ||
| 8. `awaiting-author` stales after three inactive days and closes after two more; `awaiting-maintainer` never stales. | ||
|
|
||
| ## 3. Configure the `dev` ruleset (owner/admin) | ||
|
|
||
| The connected `Wibias` account has write access but not repository admin access, so the project owner or another administrator must perform this step. | ||
|
|
||
| - Require pull requests before merging. | ||
| - Require at least one approval and prevent author self-approval. | ||
| - Require CODEOWNERS approval. | ||
| - Dismiss stale approvals when new commits are pushed. | ||
| - Require approval of the most recent reviewable push. | ||
| - Require all review conversations to be resolved. | ||
| - Require these checks after their exact names are confirmed by the synthetic test: | ||
| - `Enforce PR target branch / enforce-target` | ||
| - `PR admission / admission` | ||
| - `PR readiness / reconcile` | ||
| - `PR trust lane / trust-lane` | ||
| - `PR hygiene / hygiene` | ||
| - the cross-platform CI jobs required by current release policy | ||
| - CodeRabbit's blocking review check | ||
| - Restrict bypass permissions to emergency owner/maintainer recovery only. | ||
|
|
||
| ## 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. | ||
|
Comment on lines
+52
to
+54
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 ( Useful? React with 👍 / 👎. |
||
|
|
||
| ## 5. Measure before tightening | ||
|
|
||
| For two weeks, record: | ||
|
|
||
| - admission failure rate; | ||
| - abandonment and reopen rate; | ||
| - first-pass CI success; | ||
| - substantial review rounds per merged PR; | ||
| - maintainer review time; | ||
| - closures by standardized reason. | ||
|
|
||
| Change thresholds only from this evidence. Commit count and guessed AI origin are not quality metrics. | ||
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,63 @@ | ||
| "use strict"; | ||
|
|
||
| const MAX_SUBSTANTIAL_REVIEW_ROUNDS = 2; | ||
| const CLOSURE_LABELS = [ | ||
| "close: no-approved-issue", | ||
| "close: not-review-ready", | ||
| "close: abandoned", | ||
| "close: excessive-review-churn", | ||
| "close: scope-too-large", | ||
| "close: wrong-direction", | ||
| "close: insufficient-tests", | ||
| ]; | ||
|
|
||
| function normalizeState(state) { | ||
| return { | ||
| version: 1, | ||
| rounds: Number.isInteger(state?.rounds) && state.rounds >= 0 ? state.rounds : 0, | ||
| lastCountedHeadSha: | ||
| typeof state?.lastCountedHeadSha === "string" ? state.lastCountedHeadSha : null, | ||
| }; | ||
| } | ||
|
|
||
| function isSubstantialReview(body) { | ||
| if (typeof body !== "string") return false; | ||
| const text = body.replace(/<!--[^]*?-->/g, "").trim(); | ||
| return text.length >= 40; | ||
| } | ||
|
|
||
| function applyReviewEvent({ | ||
| state, | ||
| reviewState, | ||
| reviewBody, | ||
| reviewerHasPushPermission, | ||
| headSha, | ||
| }) { | ||
| const current = normalizeState(state); | ||
| const result = { | ||
| ...current, | ||
| counted: false, | ||
| limitReached: current.rounds >= MAX_SUBSTANTIAL_REVIEW_ROUNDS, | ||
| }; | ||
|
|
||
| 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 Useful? React with 👍 / 👎. |
||
|
|
||
| const rounds = current.rounds + 1; | ||
| return { | ||
| version: 1, | ||
| rounds, | ||
| lastCountedHeadSha: headSha, | ||
| counted: true, | ||
| limitReached: rounds >= MAX_SUBSTANTIAL_REVIEW_ROUNDS, | ||
| }; | ||
| } | ||
|
|
||
| module.exports = { | ||
| CLOSURE_LABELS, | ||
| MAX_SUBSTANTIAL_REVIEW_ROUNDS, | ||
| applyReviewEvent, | ||
| isSubstantialReview, | ||
| normalizeState, | ||
| }; | ||
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,96 @@ | ||
| "use strict"; | ||
|
|
||
| const { describe, it } = require("node:test"); | ||
| const assert = require("node:assert/strict"); | ||
| const { | ||
| CLOSURE_LABELS, | ||
| MAX_SUBSTANTIAL_REVIEW_ROUNDS, | ||
| applyReviewEvent, | ||
| isSubstantialReview, | ||
| } = require("./pr-review-lifecycle.cjs"); | ||
|
|
||
| const body = "The implementation still violates the routing boundary and needs a focused regression test."; | ||
|
|
||
| describe("review round accounting", () => { | ||
| it("counts a substantial maintainer change request", () => { | ||
| const result = applyReviewEvent({ | ||
| reviewState: "changes_requested", | ||
| reviewBody: body, | ||
| reviewerHasPushPermission: true, | ||
| headSha: "aaa", | ||
| }); | ||
| assert.equal(result.rounds, 1); | ||
| assert.equal(result.counted, true); | ||
| assert.equal(result.limitReached, false); | ||
| }); | ||
|
|
||
| it("counts at most once per reviewed head SHA", () => { | ||
| const result = applyReviewEvent({ | ||
| state: { rounds: 1, lastCountedHeadSha: "aaa" }, | ||
| reviewState: "changes_requested", | ||
| reviewBody: body, | ||
| reviewerHasPushPermission: true, | ||
| headSha: "aaa", | ||
| }); | ||
| assert.equal(result.rounds, 1); | ||
| assert.equal(result.counted, false); | ||
| }); | ||
|
|
||
| it("does not count non-maintainer or thin reviews", () => { | ||
| assert.equal(applyReviewEvent({ | ||
| reviewState: "changes_requested", | ||
| reviewBody: body, | ||
| reviewerHasPushPermission: false, | ||
| headSha: "aaa", | ||
| }).rounds, 0); | ||
| assert.equal(applyReviewEvent({ | ||
| reviewState: "changes_requested", | ||
| reviewBody: "fix this", | ||
| reviewerHasPushPermission: true, | ||
| headSha: "aaa", | ||
| }).rounds, 0); | ||
| }); | ||
|
|
||
| it("flags the limit after two distinct reviewed revisions", () => { | ||
| const result = applyReviewEvent({ | ||
| state: { rounds: 1, lastCountedHeadSha: "aaa" }, | ||
| reviewState: "changes_requested", | ||
| reviewBody: body, | ||
| reviewerHasPushPermission: true, | ||
| headSha: "bbb", | ||
| }); | ||
| assert.equal(MAX_SUBSTANTIAL_REVIEW_ROUNDS, 2); | ||
| assert.equal(result.rounds, 2); | ||
| assert.equal(result.limitReached, true); | ||
| }); | ||
|
|
||
| it("ignores approvals and comments", () => { | ||
| for (const reviewState of ["approved", "commented", "dismissed"]) { | ||
| assert.equal(applyReviewEvent({ | ||
| reviewState, | ||
| reviewBody: body, | ||
| reviewerHasPushPermission: true, | ||
| headSha: "aaa", | ||
| }).rounds, 0); | ||
| } | ||
| }); | ||
| }); | ||
|
|
||
| describe("policy constants", () => { | ||
| it("recognizes substantive review text", () => { | ||
| assert.equal(isSubstantialReview(body), true); | ||
| assert.equal(isSubstantialReview("too short"), false); | ||
| }); | ||
|
|
||
| it("exports the complete closure taxonomy", () => { | ||
| assert.deepEqual(CLOSURE_LABELS, [ | ||
| "close: no-approved-issue", | ||
| "close: not-review-ready", | ||
| "close: abandoned", | ||
| "close: excessive-review-churn", | ||
| "close: scope-too-large", | ||
| "close: wrong-direction", | ||
| "close: insufficient-tests", | ||
| ]); | ||
| }); | ||
| }); |
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
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.
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.
The proposed ruleset globally requires the cross-platform CI jobs, while
.github/workflows/ci.ymlruns on pull requests only when one of itspathsentries 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 👍 / 👎.