diff --git a/docs/fork-governance.md b/docs/fork-governance.md index a42bfa2d41..2d69dda5dd 100644 --- a/docs/fork-governance.md +++ b/docs/fork-governance.md @@ -15,13 +15,13 @@ Internal pull requests targeting `internal/main` use one of three mutually exclu - **Upstream import** (default): identifies its upstream issue, pull request, head repository, and upstream pull-request head SHA. The **Upstream provenance** check verifies that metadata against the public upstream API, confirms the base repository, actual head repository, and SHA, compares every upstream and fork pull-request patch ID in order, and uploads reconciliation evidence. Direct imports need no human approval; reconciled imports (with `Fork main:` and `Reconciliation merge:` metadata) retain one human approval because conflict resolution requires review. - **Downstream governance**: uses the exact body marker `Downstream governance: true` and is restricted to the governance allowlist below. It does not assert upstream patch equivalence and does not require a pull-request review. -- **Downstream feature**: uses exactly `Downstream feature: true` plus one non-empty `Downstream rationale:` line. It is fork-only, requires one non-author human `APPROVED` review of the current pull-request head, and cannot change governance-allowlisted files or any `.github/workflows/` file. +- **Downstream feature**: uses exactly `Downstream feature: true` plus one non-empty `Downstream rationale:` line. It is fork-only, requires one human `APPROVED` review of the current pull-request head (self-approval counts — this is a single-maintainer fork), and cannot change governance-allowlisted files or any `.github/workflows/` file. -Feature and governance markers cannot coexist with each other or with upstream-provenance metadata. For a downstream feature, a review on an older head, a dismissed review, an author review, or a bot review does not satisfy the exception. Any new commit invalidates prior feature approval until a reviewer approves the new head. +Feature and governance markers cannot coexist with each other or with upstream-provenance metadata. For a downstream feature, a review on an older head, a dismissed review, or a bot review does not satisfy the exception. Any new commit invalidates prior feature approval until a reviewer approves the new head. ### Approval phases -Downstream feature exceptions have two approval phases. Before merge, the **pre-merge** phase requires one non-author human `APPROVED` review whose commit SHA exactly matches the pull-request head. After merge, the **post-merge** phase accepts the human actor recorded in `merged_by` as approval evidence only when the pull request is both `closed` and `merged`. Missing or bot merger data, and a closed-but-unmerged pull request, fail provenance; closure alone never bypasses review. Downstream governance exceptions do not require approval in either phase. +Downstream feature exceptions have two approval phases. Before merge, the **pre-merge** phase requires one human `APPROVED` review whose commit SHA exactly matches the pull-request head. After merge, the **post-merge** phase accepts the human actor recorded in `merged_by` as approval evidence only when the pull request is both `closed` and `merged`. Missing or bot merger data, and a closed-but-unmerged pull request, fail provenance; closure alone never bypasses review. Downstream governance exceptions do not require approval in either phase. The provenance evidence for feature exceptions records the phase and evidence type that qualified: an exact-head review before merge or a human merger after merge. Manual provenance dispatch selects the same phase from current pull-request state. Governance exceptions record their downstream-governance outcome without approval evidence. diff --git a/scripts/check-upstream-provenance.mjs b/scripts/check-upstream-provenance.mjs index 39f90898c3..78922fe069 100755 --- a/scripts/check-upstream-provenance.mjs +++ b/scripts/check-upstream-provenance.mjs @@ -178,7 +178,7 @@ function changedFiles(current, repository, pullRequest, mode) { return { currentHead, changedFiles: validateChangedPaths(changed, mode) }; } -function resolveApproval(reviews, currentHead, authorLogin) { +function resolveApproval(reviews, currentHead) { const latestByReviewer = new Map(); for (const review of reviews) { const login = review?.user?.login; @@ -195,14 +195,12 @@ function resolveApproval(reviews, currentHead, authorLogin) { (review) => review.state?.toUpperCase() === "APPROVED" && review.user?.type === "User" && - authorLogin && - review.user.login.toLowerCase() !== authorLogin.toLowerCase() && typeof review.commit_id === "string" && review.commit_id.toLowerCase() === currentHead, ); if (!approval) { throw new Error( - "Downstream feature exception requires a non-author human approval of the current pull request head", + "Downstream feature exception requires a human APPROVED review of the current pull request head", ); } return { @@ -252,7 +250,6 @@ function effectiveApproval(current, repository, pullRequest) { ...resolveApproval( paginatedJson(`repos/${repository}/pulls/${pullRequest}/reviews`), currentHead, - current.user?.login ?? "", ), }; } diff --git a/scripts/check-upstream-provenance.test.mjs b/scripts/check-upstream-provenance.test.mjs index 3e74de6409..9c5664189e 100644 --- a/scripts/check-upstream-provenance.test.mjs +++ b/scripts/check-upstream-provenance.test.mjs @@ -93,51 +93,50 @@ test("forbids governance and workflow paths in feature mode", () => { ); }); -test("requires a current-head approval from a non-author human", () => { - assert.deepEqual(resolveApproval([review()], currentHead, "author"), { +test("requires a current-head APPROVED human review, author or not", () => { + assert.deepEqual(resolveApproval([review()], currentHead), { id: 1, authorLogin: "reviewer", authorType: "User", state: "APPROVED", commitOid: currentHead, }); + // A solo maintainer is their own only reviewer, so self-approval counts. + assert.deepEqual( + resolveApproval([review({ user: { login: "author", type: "User" } })], currentHead), + { + id: 1, + authorLogin: "author", + authorType: "User", + state: "APPROVED", + commitOid: currentHead, + }, + ); for (const candidate of [ review({ commit_id: otherHead }), - review({ user: { login: "author", type: "User" } }), review({ user: { login: "automation", type: "Bot" } }), review({ state: "DISMISSED" }), ]) { - assert.throws( - () => resolveApproval([candidate], currentHead, "author"), - /current pull request head/, - ); + assert.throws(() => resolveApproval([candidate], currentHead), /current pull request head/); } }); test("latest review state replaces an older approval", () => { assert.throws( - () => - resolveApproval( - [review({ id: 1 }), review({ id: 2, state: "DISMISSED" })], - currentHead, - "author", - ), + () => resolveApproval([review({ id: 1 }), review({ id: 2, state: "DISMISSED" })], currentHead), /current pull request head/, ); }); test("preserves current-head approval across later comment-only reviews", () => { for (const state of ["COMMENTED", "PENDING"]) { - assert.deepEqual( - resolveApproval([review({ id: 1 }), review({ id: 2, state })], currentHead, "author"), - { - id: 1, - authorLogin: "reviewer", - authorType: "User", - state: "APPROVED", - commitOid: currentHead, - }, - ); + assert.deepEqual(resolveApproval([review({ id: 1 }), review({ id: 2, state })], currentHead), { + id: 1, + authorLogin: "reviewer", + authorType: "User", + state: "APPROVED", + commitOid: currentHead, + }); } }); @@ -182,7 +181,7 @@ test("governance evidence does not require review approval", () => { }); test("feature evidence retains effective approval", () => { - const approval = resolveApproval([review()], currentHead, "author"); + const approval = resolveApproval([review()], currentHead); const evidence = exceptionEvidence( "fork/project", 7, diff --git a/scripts/ci-workflow.test.mjs b/scripts/ci-workflow.test.mjs index 4a91229f36..4c76bbb1f4 100644 --- a/scripts/ci-workflow.test.mjs +++ b/scripts/ci-workflow.test.mjs @@ -133,7 +133,7 @@ test("feature exceptions enforce phase-specific human approval evidence", () => assert.match(provenance, /evidenceType: "exact-head-review"/); assert.match(provenance, /phase: "post-merge"/); assert.match(provenance, /evidenceType: "human-merger"/); - assert.match(provenance, /non-author human approval of the current pull request head/); + assert.match(provenance, /human APPROVED review of the current pull request head/); }); test("governance exceptions do not require human approval", () => {