From e32fdb0e41bcdb69e89c6ffda3dfd4d691589c1f Mon Sep 17 00:00:00 2001 From: Wibias <37517432+Wibias@users.noreply.github.com> Date: Sun, 2 Aug 2026 20:27:41 +0200 Subject: [PATCH 1/2] chore: define review lifecycle and governance rollout --- .github/CODEOWNERS | 6 + .github/CONTRIBUTION_FIREWALL_ROLLOUT.md | 67 ++++++++ .github/scripts/pr-review-lifecycle.cjs | 63 ++++++++ .github/scripts/pr-review-lifecycle.test.cjs | 96 +++++++++++ .github/workflows/issue-quality-tests.yml | 56 +++---- .github/workflows/pr-review-lifecycle.yml | 152 ++++++++++++++++++ CONTRIBUTING.md | 15 ++ MAINTAINERS.md | 53 +++--- .../content/docs/contributing/pr-quality.md | 26 +++ .../2026-08-02-pr-review-lifecycle-design.md | 14 ++ 10 files changed, 497 insertions(+), 51 deletions(-) create mode 100644 .github/CONTRIBUTION_FIREWALL_ROLLOUT.md create mode 100644 .github/scripts/pr-review-lifecycle.cjs create mode 100644 .github/scripts/pr-review-lifecycle.test.cjs create mode 100644 .github/workflows/pr-review-lifecycle.yml create mode 100644 docs-site/src/content/docs/contributing/pr-quality.md create mode 100644 docs/superpowers/specs/2026-08-02-pr-review-lifecycle-design.md diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index 297343edf2..d4e18ec48e 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -1,6 +1,12 @@ # Default reviewers * @lidge-jun @Ingwannu @Wibias +# High-impact runtime behavior +/src/adapters/ @lidge-jun @Ingwannu @Wibias +/src/providers/ @lidge-jun @Ingwannu @Wibias +/src/codex/ @lidge-jun @Ingwannu @Wibias +/src/server/ @lidge-jun @Ingwannu @Wibias + # Repository automation and release security /.github/ @lidge-jun @Ingwannu /scripts/release.ts @lidge-jun @Ingwannu diff --git a/.github/CONTRIBUTION_FIREWALL_ROLLOUT.md b/.github/CONTRIBUTION_FIREWALL_ROLLOUT.md new file mode 100644 index 0000000000..97cd2d14d9 --- /dev/null +++ b/.github/CONTRIBUTION_FIREWALL_ROLLOUT.md @@ -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. + +## 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. diff --git a/.github/scripts/pr-review-lifecycle.cjs b/.github/scripts/pr-review-lifecycle.cjs new file mode 100644 index 0000000000..390364bac7 --- /dev/null +++ b/.github/scripts/pr-review-lifecycle.cjs @@ -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; + + 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, +}; diff --git a/.github/scripts/pr-review-lifecycle.test.cjs b/.github/scripts/pr-review-lifecycle.test.cjs new file mode 100644 index 0000000000..378f84125c --- /dev/null +++ b/.github/scripts/pr-review-lifecycle.test.cjs @@ -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", + ]); + }); +}); diff --git a/.github/workflows/issue-quality-tests.yml b/.github/workflows/issue-quality-tests.yml index 8c6d0da143..aa3e193caf 100644 --- a/.github/workflows/issue-quality-tests.yml +++ b/.github/workflows/issue-quality-tests.yml @@ -1,48 +1,42 @@ -name: Issue quality tests +name: Issue and PR policy tests on: pull_request: paths: - ".github/ISSUE_TEMPLATE/**" - - ".github/scripts/issue-quality.cjs" - - ".github/scripts/issue-quality.test.cjs" - - ".github/scripts/pr-quality.cjs" - - ".github/scripts/pr-quality.test.cjs" - - ".github/scripts/pr-labeler.cjs" - - ".github/scripts/pr-labeler.test.cjs" - - ".github/scripts/enforce-pr-target.test.cjs" - - ".github/scripts/issue-translation.cjs" - - ".github/scripts/issue-translation.test.cjs" - - ".github/scripts/issue-triage.cjs" - - ".github/scripts/issue-triage.test.cjs" - - ".github/scripts/parse-issue-translation-response.cjs" - - ".github/scripts/parse-issue-translation-response.test.cjs" + - ".github/PULL_REQUEST_TEMPLATE.md" + - ".github/scripts/*.cjs" - ".github/workflows/enforce-issue-quality.yml" - ".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/pr-labeler.yml" - ".github/workflows/issue-triage.yml" - ".github/workflows/issue-quality-tests.yml" + - ".coderabbit.yaml" + - "CONTRIBUTING.md" + - "MAINTAINERS.md" push: paths: - ".github/ISSUE_TEMPLATE/**" - - ".github/scripts/issue-quality.cjs" - - ".github/scripts/issue-quality.test.cjs" - - ".github/scripts/pr-quality.cjs" - - ".github/scripts/pr-quality.test.cjs" - - ".github/scripts/pr-labeler.cjs" - - ".github/scripts/pr-labeler.test.cjs" - - ".github/scripts/enforce-pr-target.test.cjs" - - ".github/scripts/issue-translation.cjs" - - ".github/scripts/issue-translation.test.cjs" - - ".github/scripts/issue-triage.cjs" - - ".github/scripts/issue-triage.test.cjs" - - ".github/scripts/parse-issue-translation-response.cjs" - - ".github/scripts/parse-issue-translation-response.test.cjs" + - ".github/PULL_REQUEST_TEMPLATE.md" + - ".github/scripts/*.cjs" - ".github/workflows/enforce-issue-quality.yml" - ".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/pr-labeler.yml" - ".github/workflows/issue-triage.yml" - ".github/workflows/issue-quality-tests.yml" + - ".coderabbit.yaml" + - "CONTRIBUTING.md" + - "MAINTAINERS.md" permissions: contents: read @@ -61,6 +55,11 @@ jobs: run: | node --test .github/scripts/issue-quality.test.cjs node --test .github/scripts/pr-quality.test.cjs + node --test .github/scripts/pr-admission.test.cjs + node --test .github/scripts/pr-readiness.test.cjs + node --test .github/scripts/pr-trust-lane.test.cjs + node --test .github/scripts/pr-hygiene.test.cjs + node --test .github/scripts/pr-review-lifecycle.test.cjs node --test .github/scripts/pr-labeler.test.cjs node --test .github/scripts/enforce-pr-target.test.cjs node --test .github/scripts/issue-translation.test.cjs @@ -78,16 +77,13 @@ jobs: let ok = true; for (const file of files) { const raw = fs.readFileSync(path.join(dir, file), 'utf8'); - // Minimal YAML validation: check for required keys and valid types. if (!raw.includes('name:')) { console.error(file + ': missing name'); ok = false; } if (!raw.includes('description:')) { console.error(file + ': missing description'); ok = false; } if (!raw.includes('body:')) { console.error(file + ': missing body'); ok = false; } - // Check element types. const types = [...raw.matchAll(/type:\s*(\w+)/g)].map(m => m[1]); for (const t of types) { if (!VALID_TYPES.has(t)) { console.error(file + ': unsupported element type: ' + t); ok = false; } } - // Check unique IDs. const ids = [...raw.matchAll(/id:\s*([\w-]+)/g)].map(m => m[1]); const dupes = ids.filter((id, i) => ids.indexOf(id) !== i); if (dupes.length) { console.error(file + ': duplicate IDs: ' + dupes.join(', ')); ok = false; } diff --git a/.github/workflows/pr-review-lifecycle.yml b/.github/workflows/pr-review-lifecycle.yml new file mode 100644 index 0000000000..4ff7390b2f --- /dev/null +++ b/.github/workflows/pr-review-lifecycle.yml @@ -0,0 +1,152 @@ +name: PR review lifecycle + +on: + pull_request_review: + types: [submitted] + +# The workflow and script are loaded from the repository default branch. It +# reads review/PR metadata only and never checks out the pull request head. +permissions: + contents: read + issues: write + pull-requests: read + +concurrency: + group: pr-review-lifecycle-${{ github.event.pull_request.number }} + cancel-in-progress: false + +jobs: + lifecycle: + runs-on: ubuntu-latest + steps: + - name: Checkout trusted lifecycle script + uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 + with: + ref: ${{ github.event.repository.default_branch }} + persist-credentials: false + sparse-checkout: .github/scripts + + - name: Track substantial review rounds + uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9 + with: + script: | + const path = require("node:path"); + const { + CLOSURE_LABELS, + MAX_SUBSTANTIAL_REVIEW_ROUNDS, + applyReviewEvent, + } = require( + path.join(process.cwd(), ".github", "scripts", "pr-review-lifecycle.cjs"), + ); + + const { owner, repo } = context.repo; + const pr = context.payload.pull_request; + const review = context.payload.review; + const pull_number = pr.number; + const marker = ""; + const statePattern = //; + + let reviewerPermission = "read"; + try { + const response = await github.rest.repos.getCollaboratorPermissionLevel({ + owner, + repo, + username: review.user.login, + }); + reviewerPermission = response.data.permission; + } catch (error) { + core.warning(`Reviewer permission lookup failed: ${error.message}`); + } + const reviewerHasPushPermission = ["admin", "maintain", "write"].includes(reviewerPermission); + + const comments = await github.paginate(github.rest.issues.listComments, { + owner, repo, issue_number: pull_number, per_page: 100, + }); + const existing = comments.find( + (comment) => comment.user?.login === "github-actions[bot]" && comment.body?.includes(marker), + ); + let storedState = null; + const match = existing?.body?.match(statePattern); + if (match) { + try { storedState = JSON.parse(match[1]); } + catch (error) { core.warning(`Could not parse lifecycle state: ${error.message}`); } + } + + const result = applyReviewEvent({ + state: storedState, + reviewState: review.state, + reviewBody: review.body, + reviewerHasPushPermission, + headSha: pr.head.sha, + }); + if (!result.counted) { + core.info("Review did not start a new substantial maintainer review round."); + return; + } + + const labels = { + "review: round-1": ["bfdadc", "One substantial maintainer review round completed"], + "review: round-2": ["d4c5f9", "Two substantial maintainer review rounds completed"], + "review: limit-reached": ["b60205", "Review churn threshold reached; maintainer disposition required"], + "awaiting-author": ["d93f0b", "Author action is required before maintainer review"], + ...Object.fromEntries(CLOSURE_LABELS.map((name) => [name, ["5319e7", "Standard pull request closure reason"]])), + }; + + async function ensureLabel(name) { + try { + await github.rest.issues.getLabel({ owner, repo, name }); + } catch (error) { + if (error.status !== 404) throw error; + const [color, description] = labels[name]; + try { + await github.rest.issues.createLabel({ owner, repo, name, color, description }); + } catch (createError) { + if (createError.status !== 422) throw createError; + } + } + } + for (const name of Object.keys(labels)) await ensureLabel(name); + + const current = new Set(pr.labels.map((label) => label.name)); + async function add(name) { + if (current.has(name)) return; + await github.rest.issues.addLabels({ owner, repo, issue_number: pull_number, labels: [name] }); + current.add(name); + } + async function remove(name) { + if (!current.has(name)) return; + await github.rest.issues.removeLabel({ owner, repo, issue_number: pull_number, name }); + current.delete(name); + } + + await remove("awaiting-maintainer"); + await add("awaiting-author"); + await remove("review: round-1"); + await remove("review: round-2"); + await add(`review: round-${Math.min(result.rounds, MAX_SUBSTANTIAL_REVIEW_ROUNDS)}`); + if (result.limitReached) await add("review: limit-reached"); + + const stateMarker = ``; + const limitText = result.limitReached + ? [ + "", + "The two-round review threshold is reached. If the implementation still needs architectural repair, repeatedly reintroduces defects, or cannot be explained by the author, a maintainer may close it with `close: excessive-review-churn`. The author may return with a clean replacement PR.", + ] + : ["", "Address the requested changes on the branch. Maintainers are not expected to implement or debug the fixes for the author."]; + const body = [ + marker, + stateMarker, + "", + `๐Ÿ” **Substantial maintainer review round ${result.rounds} recorded.**`, + ...limitText, + ].join("\n"); + + if (existing) { + await github.rest.issues.updateComment({ owner, repo, comment_id: existing.id, body }); + } else { + await github.rest.issues.createComment({ owner, repo, issue_number: pull_number, body }); + } diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index eb3fdab11a..b8a7fdc4a9 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -3,6 +3,7 @@ Thanks for helping with opencodex. - Start with the canonical guide: [Contributing](https://opencodex.me/contributing/) +- Pull-request quality contract: [Review readiness and author responsibility](https://opencodex.me/contributing/pr-quality/) - Public user docs live in [`docs-site/`](./docs-site) - Current maintainer invariants live in [`structure/`](./structure) - Maintainer roles and merge policy live in [`MAINTAINERS.md`](./MAINTAINERS.md) @@ -31,6 +32,20 @@ Source development requires the `bun` CLI on your `PATH`. The published npm pack Bun runtime for end users, but contributor commands such as `bun install`, `bun run test`, and `bun run prepush` run from your local Bun installation. +## Pull request contract + +A ready-for-review PR is the author's claim that the change is complete, understood, tested, and suitable for merging. Opening a PR does not transfer responsibility for the branch to maintainers. + +- External implementation PRs reference an issue labeled `approved-for-work` before implementation begins. +- Human review starts only after automated intake, trust-lane, hygiene, CI, and CodeRabbit gates pass and the PR is labeled `awaiting-maintainer`. +- Authors own CI failures, missing tests, merge conflicts, and review fixes. Maintainers identify problems; they are not required to implement or debug the fixes for contributors. +- Behavior changes include focused regression tests. Claims such as โ€œtestedโ€ or โ€œCIโ€ without named commands and results are not evidence. +- First-time contributors may have one active implementation PR, are limited to 500 changed lines unless the linked issue has `large-change-approved`, and need `maintainer-sponsored` for authentication, workflow, release, or dependency surfaces. +- A substantial review round is a maintainer change request on a distinct head revision. After two unsuccessful rounds, maintainers may close a PR that still needs architectural repair, repeatedly reintroduces defects, or shows that the author cannot own the implementation. +- PRs labeled `awaiting-author` warn after three inactive days and close after two more. PRs labeled `awaiting-maintainer` are not stale-closed. + +Standard closure labels are `close: no-approved-issue`, `close: not-review-ready`, `close: abandoned`, `close: excessive-review-churn`, `close: scope-too-large`, `close: wrong-direction`, and `close: insufficient-tests`. Reopening requires resolving the stated reason; otherwise submit a clean replacement PR. + ## Pre-push hook After cloning, run once to install a local pre-push hook that runs the typecheck, diff --git a/MAINTAINERS.md b/MAINTAINERS.md index 867e7d6aa3..b6d4539b89 100644 --- a/MAINTAINERS.md +++ b/MAINTAINERS.md @@ -24,12 +24,12 @@ see [The retired `dev2-go` line](#the-retired-dev2-go-line). - The **`enforce-target`** CI check rejects pull requests whose head ancestry sits on the **`main`** tip while far behind **`dev`**, and rejects empty, thin, or malformed descriptions; authors with repository push - permission skip the ancestry heuristic only. As with the approval requirement - above, this is enforced by convention until branch protection is configured - (see the note under the change log). -- A pull request requires approval from at least one maintainer and successful required CI checks - before merge. + permission skip the ancestry heuristic only. +- Human code review begins only when the PR carries `awaiting-maintainer`. Red, pending, or intake-blocked PRs remain the author's responsibility and do not consume maintainer code-review time. +- A pull request requires approval from at least one maintainer and successful required CI checks before merge. - Authors do not approve their own pull requests. +- Maintainers identify defects and architectural problems; they are not expected to repair contributor branches, write missing tests, resolve routine CI failures, or translate every automated finding into step-by-step patches. +- A substantial review round is one actionable `CHANGES_REQUESTED` review by a maintainer on a distinct head SHA. Multiple reviews of one revision count once. After two unsuccessful rounds, a maintainer may close the PR when architectural correction remains necessary, defects recur, or the author cannot explain and maintain the implementation. - Authentication, credential handling, GitHub Actions, release automation, dependency installation, and other security-boundary changes require explicit security review. - A new or promoted provider preset is a credential-destination change. Before merge it needs the @@ -41,12 +41,28 @@ see [The retired `dev2-go` line](#the-retired-dev2-go-line). with the service is disclosed, not disqualifying, and it does not lower the evidence bar. When the evidence is incomplete, prefer an inert `src/providers/free-directory.ts` reference row over a canonical registry entry. -- Security-sensitive and release-related changes should be reviewed by both maintainers when - practical. -- Direct pushes are reserved for maintainer-owned integration work, urgent repairs, or incident - recovery. The same CI and documentation requirements still apply. +- Security-sensitive and release-related changes should be reviewed by both maintainers when practical. +- Direct pushes are reserved for maintainer-owned integration work, urgent repairs, or incident recovery. The same CI and documentation requirements still apply. - Promotion from `dev` to `main` and npm releases is maintainer-controlled. +## Pull request disposition + +Use one standardized closure label and a concise comment naming the reopening condition: + +| Label | Use when | +| --- | --- | +| `close: no-approved-issue` | External implementation began without agreed scope | +| `close: not-review-ready` | The PR was presented as complete but basic readiness requirements remain unmet | +| `close: abandoned` | Author action remained outstanding beyond the inactivity window | +| `close: excessive-review-churn` | Two substantial rounds did not produce a reviewable implementation | +| `close: scope-too-large` | The change cannot be reviewed safely as one PR | +| `close: wrong-direction` | The implementation conflicts with project architecture or direction | +| `close: insufficient-tests` | Changed behavior lacks credible regression coverage | + +Do not stale-close `awaiting-maintainer` PRs. Do not keep a fundamentally broken PR open merely because maintainers already invested time in it; that is sunk-cost reasoning. The author may return with a clean replacement after resolving the disposition. + +Repository rulesets and merge-queue settings are configured by an owner or administrator after the synthetic rollout in [`.github/CONTRIBUTION_FIREWALL_ROLLOUT.md`](./.github/CONTRIBUTION_FIREWALL_ROLLOUT.md). + ## The retired `dev2-go` line `dev2-go` was a parallel integration line that rebuilt the runtime as a Go @@ -93,18 +109,13 @@ Adding or removing a maintainer requires: should go through a reviewed pull request. Scope covers issue and pull-request triage, `dev` integration, and - provider/CI maintenance. (This entry originally also described carrying - merged `dev` work onto `dev2-go`; that duty ended when the line was retired - on 2026-07-30.) Security-boundary ownership in `.github/CODEOWNERS` is - deliberately unchanged: authentication, credential handling, GitHub Actions, - and release automation keep the two owners already listed for those paths, so - this addition does not widen the review surface for them. - - CODEOWNERS requests reviews rather than enforcing them โ€” no branch protection - rule is configured on this repository, so code-owner approval is a convention - here, not a gate. The same is true of the approval requirement in the review - and merge policy above. Widening the security boundary, or enforcing either - of these through branch protection, is a separate decision. + provider/CI maintenance. Security-boundary ownership in `.github/CODEOWNERS` + remains stricter for authentication, credential handling, GitHub Actions, and + release automation. + + CODEOWNERS requests reviews rather than enforcing them until the project owner + enables the documented ruleset. Widening the security boundary or changing + bypass permissions remains a separate owner decision. ## Security reports diff --git a/docs-site/src/content/docs/contributing/pr-quality.md b/docs-site/src/content/docs/contributing/pr-quality.md new file mode 100644 index 0000000000..8075bb38a5 --- /dev/null +++ b/docs-site/src/content/docs/contributing/pr-quality.md @@ -0,0 +1,26 @@ +--- +title: Pull request quality contract +description: Review readiness, contributor responsibility, trust lanes, and closure policy for OpenCodex pull requests. +--- + +## Earn maintainer review + +Opening a pull request does not transfer responsibility for the branch to the maintainers. Human review begins after the automated intake, trust-lane, hygiene, CI, and CodeRabbit gates pass and the PR is labeled `awaiting-maintainer`. + +Authors must understand every changed line, provide exact validation evidence, add focused regression coverage for behavior changes, and remain available to resolve CI and review feedback. Maintainers identify problems; they are not expected to repair contributor branches, write missing tests, or repeatedly translate automated findings into patches. + +## Approved scope + +External implementation work starts from an issue labeled `approved-for-work`. Documentation-only changes and maintainer-owned integration work are exempt. First-time contributors may have one active implementation PR, are limited to 500 changed lines unless the issue has `large-change-approved`, and need `maintainer-sponsored` for authentication, workflow, release, or dependency surfaces. + +## Review rounds + +A substantial review round is a maintainer change request on a distinct revision with actionable explanation. Multiple reviews of the same commit count once. After two unsuccessful rounds, maintainers may close a PR that still needs architectural repair, repeatedly reintroduces defects, or shows that the author cannot own the implementation. The author may return with a clean replacement PR. + +## Author inactivity + +PRs labeled `awaiting-author` receive a warning after three inactive days and close after two more. Updates reset the timer. PRs labeled `awaiting-maintainer` are never closed for contributor inactivity. + +## Closure reasons + +Maintainers use standardized labels: `close: no-approved-issue`, `close: not-review-ready`, `close: abandoned`, `close: excessive-review-churn`, `close: scope-too-large`, `close: wrong-direction`, and `close: insufficient-tests`. diff --git a/docs/superpowers/specs/2026-08-02-pr-review-lifecycle-design.md b/docs/superpowers/specs/2026-08-02-pr-review-lifecycle-design.md new file mode 100644 index 0000000000..62bfdf6ada --- /dev/null +++ b/docs/superpowers/specs/2026-08-02-pr-review-lifecycle-design.md @@ -0,0 +1,14 @@ +# Review lifecycle and governance rollout โ€” Design + +**Stack:** 5/5, based on `agent/pr-hygiene-gate` + +This layer closes the policy gaps that automation alone cannot solve. + +- A substantial review round is one maintainer `CHANGES_REQUESTED` review with at least 40 characters of actionable text on a distinct head SHA. +- Multiple reviews of the same revision count as one round. +- After two rounds, automation applies `review: limit-reached`; it does not close automatically. +- Maintainers may close when architectural repair is still required, defects recur, or the author cannot own the implementation. +- Standard closure labels make dispositions consistent and measurable. +- Contributor policy explicitly assigns branch repair, CI failures, and review fixes to the author. +- CODEOWNERS expands review routing for adapters, providers, Codex integration, and server behavior while keeping authentication and automation under the stricter existing owners. +- Repository rulesets and merge queue remain an owner/admin action documented in an exact rollout checklist. From d3f2e3b0437ed0052fc6a3969af638d65216074a Mon Sep 17 00:00:00 2001 From: Wibias <37517432+Wibias@users.noreply.github.com> Date: Mon, 3 Aug 2026 04:49:25 +0200 Subject: [PATCH 2/2] fix(ci): scope lifecycle permissions, drop awaiting-label fights, clear rounds on approval --- .github/workflows/pr-review-lifecycle.yml | 39 +++++++++++++++---- .../2026-08-02-pr-review-lifecycle-design.md | 2 + 2 files changed, 33 insertions(+), 8 deletions(-) diff --git a/.github/workflows/pr-review-lifecycle.yml b/.github/workflows/pr-review-lifecycle.yml index 4ff7390b2f..a0a3b77316 100644 --- a/.github/workflows/pr-review-lifecycle.yml +++ b/.github/workflows/pr-review-lifecycle.yml @@ -6,10 +6,8 @@ on: # The workflow and script are loaded from the repository default branch. It # reads review/PR metadata only and never checks out the pull request head. -permissions: - contents: read - issues: write - pull-requests: read +# Least privilege: no default permissions; the lifecycle job grants only what it needs. +permissions: {} concurrency: group: pr-review-lifecycle-${{ github.event.pull_request.number }} @@ -18,6 +16,12 @@ concurrency: jobs: lifecycle: runs-on: ubuntu-latest + # issues/pull-requests write maintain the review-round and closure labels; + # PR label writes require pull-requests: write (see pr-labeler.yml). + permissions: + contents: read + issues: write + pull-requests: write steps: - name: Checkout trusted lifecycle script uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 @@ -38,6 +42,9 @@ jobs: } = require( path.join(process.cwd(), ".github", "scripts", "pr-review-lifecycle.cjs"), ); + const { authorHasPushPermission } = require( + path.join(process.cwd(), ".github", "scripts", "pr-quality.cjs"), + ); const { owner, repo } = context.repo; const pr = context.payload.pull_request; @@ -57,7 +64,7 @@ jobs: } catch (error) { core.warning(`Reviewer permission lookup failed: ${error.message}`); } - const reviewerHasPushPermission = ["admin", "maintain", "write"].includes(reviewerPermission); + const reviewerHasPushPermission = authorHasPushPermission(reviewerPermission); const comments = await github.paginate(github.rest.issues.listComments, { owner, repo, issue_number: pull_number, per_page: 100, @@ -79,6 +86,25 @@ jobs: reviewerHasPushPermission, headSha: pr.head.sha, }); + // 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") { + const currentLabels = new Set(pr.labels.map((label) => label.name)); + for (const name of [ + "review: round-1", + "review: round-2", + "review: limit-reached", + ]) { + if (currentLabels.has(name)) { + await github.rest.issues.removeLabel({ + owner, repo, issue_number: pull_number, name, + }); + } + } + core.info("Approval clears review-round labels."); + return; + } + if (!result.counted) { core.info("Review did not start a new substantial maintainer review round."); return; @@ -88,7 +114,6 @@ jobs: "review: round-1": ["bfdadc", "One substantial maintainer review round completed"], "review: round-2": ["d4c5f9", "Two substantial maintainer review rounds completed"], "review: limit-reached": ["b60205", "Review churn threshold reached; maintainer disposition required"], - "awaiting-author": ["d93f0b", "Author action is required before maintainer review"], ...Object.fromEntries(CLOSURE_LABELS.map((name) => [name, ["5319e7", "Standard pull request closure reason"]])), }; @@ -119,8 +144,6 @@ jobs: current.delete(name); } - await remove("awaiting-maintainer"); - await add("awaiting-author"); await remove("review: round-1"); await remove("review: round-2"); await add(`review: round-${Math.min(result.rounds, MAX_SUBSTANTIAL_REVIEW_ROUNDS)}`); diff --git a/docs/superpowers/specs/2026-08-02-pr-review-lifecycle-design.md b/docs/superpowers/specs/2026-08-02-pr-review-lifecycle-design.md index 62bfdf6ada..fcdb23a5be 100644 --- a/docs/superpowers/specs/2026-08-02-pr-review-lifecycle-design.md +++ b/docs/superpowers/specs/2026-08-02-pr-review-lifecycle-design.md @@ -12,3 +12,5 @@ This layer closes the policy gaps that automation alone cannot solve. - Contributor policy explicitly assigns branch repair, CI failures, and review fixes to the author. - CODEOWNERS expands review routing for adapters, providers, Codex integration, and server behavior while keeping authentication and automation under the stricter existing owners. - Repository rulesets and merge queue remain an owner/admin action documented in an exact rollout checklist. + +The lifecycle workflow does not manage `awaiting-author` / `awaiting-maintainer`; those labels remain gate-owned by the readiness layer so the two workflows cannot fight over the same state. Approvals clear the `review: round-*` and `review: limit-reached` labels.