diff --git a/.claude/skills/ship/SKILL.md b/.claude/skills/ship/SKILL.md index a07fa6f4f0..1464c791c6 100644 --- a/.claude/skills/ship/SKILL.md +++ b/.claude/skills/ship/SKILL.md @@ -14,7 +14,7 @@ Run this flow only for a pull request that targets `internal/main`. 4. Post the review to GitHub. Include the reviewed head SHA, verdict, high-risk findings, and advisory findings. 5. Stop if the review finds a high-risk issue. Request changes with path and line evidence. 6. For a clean direct import, dispatch **Upstream import merge** after the review posts. -7. For a reconciled import, review only the conflict-resolution diff. Submit the required human approval, then dispatch **Upstream import merge**. +7. For a reconciled import, review only the conflict-resolution diff, then dispatch **Upstream import merge**. 8. Confirm the pull request merged with a merge commit. Do not merge a pull request with failed CI, failed provenance, a stale upstream head, or an unresolved high-risk finding. diff --git a/docs/fork-governance.md b/docs/fork-governance.md index 2d69dda5dd..55c0633451 100644 --- a/docs/fork-governance.md +++ b/docs/fork-governance.md @@ -13,17 +13,13 @@ This repository is the public `ZGEnergy/paseo` fork. Keep the fork's `main` bran Internal pull requests targeting `internal/main` use one of three mutually exclusive modes: -- **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 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. +- **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, and compares every upstream and fork pull-request patch ID in order (direct imports), or validates the `[upstreamHead, forkMain]` reconciliation merge commit (reconciled imports, with `Fork main:` and `Reconciliation merge:` metadata). +- **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. +- **Downstream feature**: uses exactly `Downstream feature: true` plus one non-empty `Downstream rationale:` line. It is fork-only 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, 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. -### Approval phases - -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. +None of the three modes requires a pull-request review. This is a single-maintainer fork, and GitHub refuses to let an author approve their own pull request, so any review requirement here would be permanently unsatisfiable. Provenance is established mechanically — from the labeled metadata fields and, for upstream imports, patch-ID or reconciliation-merge verification — not from a second human's sign-off. If an upstream import is stale, the final provenance run fails and the bot does not merge it. Update its metadata or reconcile against the new upstream head, then rerun checks. Never merge an import whose upstream head changed after review. @@ -63,7 +59,7 @@ This exception does not provide a general provenance bypass: a non-governance pa ## Local ship gate -Internal teammates run `/ship` locally for a pull request that targets `internal/main`. The operator uses their own review agent, posts its review to GitHub, and stops on a high-risk finding. The skill requires CI and provenance for the current head before review. A clean direct import may dispatch **Upstream import merge** after the review posts. A reconciled import requires the focused conflict-resolution review and the existing human approval before that dispatch. See `.claude/skills/ship/SKILL.md` for the procedure. +Internal teammates run `/ship` locally for a pull request that targets `internal/main`. The operator uses their own review agent, posts its review to GitHub, and stops on a high-risk finding. The skill requires CI and provenance for the current head before review. A clean direct import may dispatch **Upstream import merge** after the review posts. A reconciled import requires the focused conflict-resolution review before that dispatch. See `.claude/skills/ship/SKILL.md` for the procedure. ## CI and permissions diff --git a/scripts/check-upstream-provenance.mjs b/scripts/check-upstream-provenance.mjs index 78922fe069..3950a210c9 100755 --- a/scripts/check-upstream-provenance.mjs +++ b/scripts/check-upstream-provenance.mjs @@ -178,83 +178,7 @@ function changedFiles(current, repository, pullRequest, mode) { return { currentHead, changedFiles: validateChangedPaths(changed, mode) }; } -function resolveApproval(reviews, currentHead) { - const latestByReviewer = new Map(); - for (const review of reviews) { - const login = review?.user?.login; - if (!login) continue; - const state = review.state?.toUpperCase(); - if (state === "COMMENTED" || state === "PENDING") continue; - const key = login.toLowerCase(); - const previous = latestByReviewer.get(key); - if (!previous || Number(review.id ?? 0) >= Number(previous.id ?? 0)) { - latestByReviewer.set(key, review); - } - } - const approval = [...latestByReviewer.values()].find( - (review) => - review.state?.toUpperCase() === "APPROVED" && - review.user?.type === "User" && - typeof review.commit_id === "string" && - review.commit_id.toLowerCase() === currentHead, - ); - if (!approval) { - throw new Error( - "Downstream feature exception requires a human APPROVED review of the current pull request head", - ); - } - return { - id: approval.id, - authorLogin: approval.user.login, - authorType: approval.user.type, - state: approval.state, - commitOid: approval.commit_id.toLowerCase(), - }; -} - -function mergedApproval(current) { - const state = current.state?.toLowerCase(); - const isClosed = state === "closed"; - const isMerged = current.merged === true; - if (isClosed && !isMerged) { - throw new Error( - "Downstream exception requires a closed and merged pull request for post-merge evidence", - ); - } - if (isMerged && !isClosed) { - throw new Error( - "Downstream exception requires a closed and merged pull request for post-merge evidence", - ); - } - if (!isClosed) return undefined; - - const merger = current.merged_by; - if (!merger?.login || merger.type !== "User") { - throw new Error("Merged downstream exception requires a recorded human merged_by actor"); - } - return { - phase: "post-merge", - evidenceType: "human-merger", - authorLogin: merger.login, - authorType: merger.type, - }; -} - -function effectiveApproval(current, repository, pullRequest) { - const postMergeApproval = mergedApproval(current); - if (postMergeApproval) return postMergeApproval; - const currentHead = shaFrom(current.head?.sha ?? "", "Pull request head"); - return { - phase: "pre-merge", - evidenceType: "exact-head-review", - ...resolveApproval( - paginatedJson(`repos/${repository}/pulls/${pullRequest}/reviews`), - currentHead, - ), - }; -} - -function exceptionEvidence(repository, pullRequest, mode, rationale, scope, approval) { +function exceptionEvidence(repository, pullRequest, mode, rationale, scope) { return { repository, pullRequest, @@ -268,16 +192,13 @@ function exceptionEvidence(repository, pullRequest, mode, rationale, scope, appr changedFiles: scope.changedFiles, }, currentHead: scope.currentHead, - ...(mode === "downstream-feature" ? { approval } : {}), result: mode === "downstream-governance" ? "governance-exception" : "feature-exception", }; } function downstreamExceptionEvidence(current, repository, pullRequest, mode, rationale) { const scope = changedFiles(current, repository, pullRequest, mode); - const approval = - mode === "downstream-feature" ? effectiveApproval(current, repository, pullRequest) : undefined; - return exceptionEvidence(repository, pullRequest, mode, rationale, scope, approval); + return exceptionEvidence(repository, pullRequest, mode, rationale, scope); } function metadataValue(body, label) { @@ -400,23 +321,6 @@ function patchIds(patch, label) { return ids; } -function approvalSummary(approval) { - const lines = [ - `- Approval phase: \`${approval.phase}\``, - `- Approval evidence: \`${approval.evidenceType}\``, - ]; - if (approval.phase === "post-merge") { - lines.push(`- Human merger: \`${approval.authorLogin}\` (${approval.authorType})`); - } else { - lines.push( - `- Effective approval: review #${approval.id} by \`${approval.authorLogin}\` (${approval.authorType})`, - `- Review state: \`${approval.state}\``, - `- Review commit: \`${approval.commitOid}\` (matches current head)`, - ); - } - return lines; -} - function writeEvidence(path, evidence) { if (path) { mkdirSync(dirname(path), { recursive: true }); @@ -430,8 +334,7 @@ function writeEvidence(path, evidence) { `- Rationale: ${evidence.rationale}`, `- Changed files: \`${evidence.scope.changedFiles.join(", ")}\``, `- Current pull request head: \`${evidence.currentHead}\``, - ...approvalSummary(evidence.approval), - "- Result: **downstream feature exception accepted; no upstream patch equivalence asserted**", + "- Result: **downstream feature exception accepted; no upstream patch equivalence asserted, no review required**", "", ]; appendFileSync(process.env.GITHUB_STEP_SUMMARY, `${lines.join("\n")}\n`); @@ -739,10 +642,8 @@ export { DOWNSTREAM_GOVERNANCE_PATHS, downstreamFeatureMarker, downstreamGovernanceMarker, - effectiveApproval, exceptionEvidence, exceptionMode, - resolveApproval, validateChangedFileCount, validateChangedPaths, }; diff --git a/scripts/check-upstream-provenance.test.mjs b/scripts/check-upstream-provenance.test.mjs index 9c5664189e..c22723dd47 100644 --- a/scripts/check-upstream-provenance.test.mjs +++ b/scripts/check-upstream-provenance.test.mjs @@ -3,26 +3,13 @@ import test from "node:test"; import { downstreamFeatureMarker, - effectiveApproval, exceptionEvidence, exceptionMode, - resolveApproval, validateChangedFileCount, validateChangedPaths, } from "./check-upstream-provenance.mjs"; const currentHead = "a".repeat(40); -const otherHead = "b".repeat(40); - -function review(overrides = {}) { - return { - id: 1, - state: "APPROVED", - commit_id: currentHead, - user: { login: "reviewer", type: "User" }, - ...overrides, - }; -} test("accepts exact downstream-feature marker and rationale", () => { assert.deepEqual( @@ -93,83 +80,6 @@ test("forbids governance and workflow paths in feature mode", () => { ); }); -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: "automation", type: "Bot" } }), - review({ state: "DISMISSED" }), - ]) { - 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), - /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), { - id: 1, - authorLogin: "reviewer", - authorType: "User", - state: "APPROVED", - commitOid: currentHead, - }); - } -}); - -test("accepts human merger evidence only for a closed-and-merged pull request", () => { - assert.deepEqual( - effectiveApproval( - { - state: "closed", - merged: true, - merged_by: { login: "merger", type: "User" }, - }, - "fork/project", - 7, - ), - { - phase: "post-merge", - evidenceType: "human-merger", - authorLogin: "merger", - authorType: "User", - }, - ); - for (const current of [ - { state: "closed", merged: false, merged_by: { login: "merger", type: "User" } }, - { state: "closed", merged: true }, - { state: "closed", merged: true, merged_by: { login: "automation", type: "Bot" } }, - ]) { - assert.throws( - () => effectiveApproval(current, "fork/project", 7), - /closed and merged|human merged_by/, - ); - } -}); - test("governance evidence does not require review approval", () => { const evidence = exceptionEvidence("fork/project", 7, "downstream-governance", undefined, { currentHead, @@ -180,16 +90,12 @@ test("governance evidence does not require review approval", () => { assert.equal(evidence.currentHead, currentHead); }); -test("feature evidence retains effective approval", () => { - const approval = resolveApproval([review()], currentHead); - const evidence = exceptionEvidence( - "fork/project", - 7, - "downstream-feature", - "reason", - { currentHead, changedFiles: ["packages/example.ts"] }, - approval, - ); +test("feature evidence does not require or store review approval", () => { + const evidence = exceptionEvidence("fork/project", 7, "downstream-feature", "reason", { + currentHead, + changedFiles: ["packages/example.ts"], + }); assert.equal(evidence.result, "feature-exception"); - assert.deepEqual(evidence.approval, approval); + assert.equal("approval" in evidence, false); + assert.equal(evidence.currentHead, currentHead); }); diff --git a/scripts/ci-workflow.test.mjs b/scripts/ci-workflow.test.mjs index 4c76bbb1f4..7968aa6060 100644 --- a/scripts/ci-workflow.test.mjs +++ b/scripts/ci-workflow.test.mjs @@ -125,25 +125,13 @@ test("fork governance workflows retain their enforcement boundaries", () => { assert.match(relayDeploy, /if: \$\{\{ github\.repository == 'ZGEnergy\/paseo' \}\}/); assert.match(relayDeploy, /npx wrangler deploy/); }); -test("feature exceptions enforce phase-specific human approval evidence", () => { - const provenance = readFileSync(provenanceScriptPath, "utf8"); - - assert.match(provenance, /pulls\/\$\{pullRequest\}\/reviews/); - assert.match(provenance, /phase: "pre-merge"/); - assert.match(provenance, /evidenceType: "exact-head-review"/); - assert.match(provenance, /phase: "post-merge"/); - assert.match(provenance, /evidenceType: "human-merger"/); - assert.match(provenance, /human APPROVED review of the current pull request head/); -}); - -test("governance exceptions do not require human approval", () => { +test("no exception mode fetches or requires a human review approval", () => { const provenance = readFileSync(provenanceScriptPath, "utf8"); assert.match(provenance, /mode === "downstream-feature"/); - assert.doesNotMatch( - provenance, - /Downstream exception requires a non-author human approval of the current pull request head/, - ); + assert.doesNotMatch(provenance, /pulls\/\$\{pullRequest\}\/reviews/); + assert.doesNotMatch(provenance, /APPROVED/); + assert.doesNotMatch(provenance, /merged_by/); }); test("provenance closed events require a merge and CI covers internal release pushes", () => {