diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index bee6264496..7e3dd02a7a 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -21,11 +21,6 @@ cancellation, reduced-motion behavior, and failure fallback. Verification must assert that the target API was actually called; a mock that silently skips it does not count. -For a large PR spanning multiple architectural owners, split independent -changes when practical. If it must remain combined, obtain a non-author -maintainer approval on the current head so the `independent cross-boundary -review` gate can pass. - ## Documentation impact Documentation-impact: TODO diff --git a/.github/scripts/cross-boundary-review.cjs b/.github/scripts/cross-boundary-review.cjs deleted file mode 100644 index 7867b14904..0000000000 --- a/.github/scripts/cross-boundary-review.cjs +++ /dev/null @@ -1,184 +0,0 @@ -"use strict"; - -const STATUS_CONTEXT = "independent cross-boundary review"; -const ALLOWED_PERMISSIONS = new Set(["admin", "maintain", "write"]); - -function pullNumberFromContext(context) { - const direct = context.payload.pull_request?.number; - const fromWorkflowRun = context.payload.workflow_run?.pull_requests?.[0]?.number; - const fromDispatch = Number(context.payload.inputs?.pull_number ?? 0); - return direct ?? fromWorkflowRun ?? (Number.isInteger(fromDispatch) && fromDispatch > 0 ? fromDispatch : undefined); -} - -function changeSummary(files) { - const groups = new Set(); - for (const { filename } of files) { - if (filename.startsWith("desktop/frontend/")) groups.add("frontend"); - else if (filename.startsWith("desktop/")) groups.add("desktop-native"); - else if (/^(?:internal|cmd)\//.test(filename)) groups.add("kernel"); - else if (/^(?:\.github|scripts|tools)\//.test(filename)) groups.add("control-plane"); - else if (/^(?:sdk|npm|site|docs|workers)\//.test(filename)) groups.add("distribution"); - } - const changedLines = files.reduce((sum, file) => sum + file.changes, 0); - return { - changedLines, - groups: [...groups], - required: files.length >= 25 || changedLines >= 1000 || groups.size >= 3, - }; -} - -function currentHeadApprovalCandidates(reviews, pull) { - const latestByReviewer = new Map(); - for (const review of reviews) { - const login = review.user?.login; - if (!login || login === pull.user.login || review.user?.type === "Bot") continue; - // Comments do not revoke an approval; only explicit review decisions do. - if (review.state === "COMMENTED" || review.state === "PENDING") continue; - latestByReviewer.set(login, { state: review.state, commitId: review.commit_id }); - } - return [...latestByReviewer.entries()] - .filter(([, review]) => review.state === "APPROVED" && review.commitId === pull.head.sha) - .map(([login]) => login); -} - -async function publishStatus({ github, context, sha, state, description }) { - await github.rest.repos.createCommitStatus({ - owner: context.repo.owner, - repo: context.repo.repo, - sha, - state, - context: STATUS_CONTEXT, - description: description.slice(0, 140), - target_url: `${context.serverUrl ?? "https://github.com"}/${context.repo.owner}/${context.repo.repo}/actions/runs/${context.runId}`, - }); -} - -async function run({ github, context, core }) { - let pullNumber = pullNumberFromContext(context); - if (!pullNumber && context.payload.workflow_run?.head_sha) { - const associated = await github.rest.repos.listPullRequestsAssociatedWithCommit({ - owner: context.repo.owner, - repo: context.repo.repo, - commit_sha: context.payload.workflow_run.head_sha, - }); - const pull = associated.data.find((candidate) => - candidate.state === "open" && candidate.base?.repo?.full_name === `${context.repo.owner}/${context.repo.repo}`, - ); - pullNumber = pull?.number; - } - if (!pullNumber) { - core.info("No pull request is associated with this event; nothing to reevaluate."); - return; - } - - let pull; - try { - ({ data: pull } = await github.rest.pulls.get({ - owner: context.repo.owner, - repo: context.repo.repo, - pull_number: pullNumber, - })); - if (pull.state !== "open") { - core.info(`Pull request #${pull.number} is not open; nothing to reevaluate.`); - return; - } - if (pull.draft) { - await publishStatus({ - github, - context, - sha: pull.head.sha, - state: "success", - description: "Draft PR; independent review is deferred", - }); - return; - } - - const files = await github.paginate(github.rest.pulls.listFiles, { - owner: context.repo.owner, - repo: context.repo.repo, - pull_number: pull.number, - per_page: 100, - }); - const summary = changeSummary(files); - if (!summary.required) { - await publishStatus({ - github, - context, - sha: pull.head.sha, - state: "success", - description: [ - `${files.length} files`, - `${summary.changedLines} lines`, - `${summary.groups.length} owner groups; review gate not required`, - ].join(", "), - }); - return; - } - - const reviews = await github.paginate(github.rest.pulls.listReviews, { - owner: context.repo.owner, - repo: context.repo.repo, - pull_number: pull.number, - per_page: 100, - }); - const approvers = []; - for (const login of currentHeadApprovalCandidates(reviews, pull)) { - try { - const permission = await github.rest.repos.getCollaboratorPermissionLevel({ - owner: context.repo.owner, - repo: context.repo.repo, - username: login, - }); - if (ALLOWED_PERMISSIONS.has(permission.data.permission)) approvers.push(login); - } catch (error) { - core.info(`Ignoring approval from ${login}: repository permission could not be verified (${error.status ?? "unknown"}).`); - } - } - - if (approvers.length === 0) { - await publishStatus({ - github, - context, - sha: pull.head.sha, - state: "failure", - description: "Large cross-boundary change needs current-head approval from a non-author maintainer", - }); - const areas = summary.groups.join(", ") || "one uncategorized area"; - core.warning( - `PR changes ${files.length} files and ${summary.changedLines} lines across ${areas}; ` + - "split it or obtain current-head approval from a non-author maintainer.", - ); - return; - } - - await publishStatus({ - github, - context, - sha: pull.head.sha, - state: "success", - description: `Current-head independent review approved by ${approvers.join(", ")}`, - }); - core.info(`Independent cross-boundary review approved by ${approvers.join(", ")}.`); - } catch (error) { - if (pull?.head?.sha) { - try { - await publishStatus({ - github, - context, - sha: pull.head.sha, - state: "error", - description: "Independent review policy could not be evaluated", - }); - } catch (statusError) { - core.info(`Could not publish evaluation error status: ${statusError.message}`); - } - } - throw error; - } -} - -module.exports = run; -module.exports.changeSummary = changeSummary; -module.exports.currentHeadApprovalCandidates = currentHeadApprovalCandidates; -module.exports.pullNumberFromContext = pullNumberFromContext; -module.exports.STATUS_CONTEXT = STATUS_CONTEXT; diff --git a/.github/scripts/cross-boundary-review.test.cjs b/.github/scripts/cross-boundary-review.test.cjs deleted file mode 100644 index bd819080d3..0000000000 --- a/.github/scripts/cross-boundary-review.test.cjs +++ /dev/null @@ -1,106 +0,0 @@ -"use strict"; - -const test = require("node:test"); -const assert = require("node:assert/strict"); -const gate = require("./cross-boundary-review.cjs"); -const { - STATUS_CONTEXT, - changeSummary, - currentHeadApprovalCandidates, - pullNumberFromContext, -} = require("./cross-boundary-review.cjs"); - -function gateHarness(reviews) { - const listFiles = () => {}; - const listReviews = () => {}; - const statuses = []; - const github = { - rest: { - pulls: { - get: async () => ({ data: { number: 7, state: "open", draft: false, user: { login: "author" }, head: { sha: "current" } } }), - listFiles, - listReviews, - }, - repos: { - createCommitStatus: async (status) => statuses.push(status), - getCollaboratorPermissionLevel: async () => ({ data: { permission: "write" } }), - }, - }, - paginate: async (endpoint) => endpoint === listFiles - ? [ - { filename: "desktop/frontend/src/App.tsx", changes: 10 }, - { filename: "desktop/main.go", changes: 10 }, - { filename: ".github/workflows/ci.yml", changes: 10 }, - ] - : reviews, - }; - const context = { - payload: { pull_request: { number: 7 } }, - repo: { owner: "example", repo: "reasonix" }, - runId: 99, - serverUrl: "https://github.example", - }; - const core = { info() {}, warning() {} }; - return { github, context, core, statuses }; -} - -test("large multi-owner changes require independent review", () => { - const files = [ - { filename: "desktop/frontend/src/App.tsx", changes: 10 }, - { filename: "desktop/main.go", changes: 10 }, - { filename: ".github/workflows/ci.yml", changes: 10 }, - ]; - assert.deepEqual(changeSummary(files), { - changedLines: 30, - groups: ["frontend", "desktop-native", "control-plane"], - required: true, - }); -}); - -test("approval must target the current pull request head", () => { - const pull = { user: { login: "author" }, head: { sha: "current" } }; - const reviews = [ - { user: { login: "stale", type: "User" }, state: "APPROVED", commit_id: "previous" }, - { user: { login: "current", type: "User" }, state: "APPROVED", commit_id: "current" }, - { user: { login: "author", type: "User" }, state: "APPROVED", commit_id: "current" }, - { user: { login: "automation", type: "Bot" }, state: "APPROVED", commit_id: "current" }, - ]; - assert.deepEqual(currentHeadApprovalCandidates(reviews, pull), ["current"]); -}); - -test("later explicit review decisions replace approval without comments revoking it", () => { - const pull = { user: { login: "author" }, head: { sha: "current" } }; - const reviews = [ - { user: { login: "maintainer", type: "User" }, state: "APPROVED", commit_id: "current" }, - { user: { login: "maintainer", type: "User" }, state: "COMMENTED", commit_id: "current" }, - { user: { login: "maintainer", type: "User" }, state: "CHANGES_REQUESTED", commit_id: "current" }, - ]; - assert.deepEqual(currentHeadApprovalCandidates(reviews, pull), []); -}); - -test("protected events resolve the intended pull request", () => { - assert.equal(pullNumberFromContext({ payload: { pull_request: { number: 12 } } }), 12); - assert.equal(pullNumberFromContext({ payload: { workflow_run: { pull_requests: [{ number: 34 }] } } }), 34); - assert.equal(pullNumberFromContext({ payload: { inputs: { pull_number: "56" } } }), 56); - assert.equal(STATUS_CONTEXT, "independent cross-boundary review"); -}); - -test("stale approval publishes a failing current-head status", async () => { - const harness = gateHarness([ - { user: { login: "maintainer", type: "User" }, state: "APPROVED", commit_id: "previous" }, - ]); - await gate(harness); - assert.equal(harness.statuses.length, 1); - assert.equal(harness.statuses[0].sha, "current"); - assert.equal(harness.statuses[0].state, "failure"); - assert.equal(harness.statuses[0].context, STATUS_CONTEXT); -}); - -test("current-head maintainer approval publishes success", async () => { - const harness = gateHarness([ - { user: { login: "maintainer", type: "User" }, state: "APPROVED", commit_id: "current" }, - ]); - await gate(harness); - assert.equal(harness.statuses.length, 1); - assert.equal(harness.statuses[0].state, "success"); -}); diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 90039e01ce..b8c109cc8d 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -578,11 +578,8 @@ jobs: .github/workflows/release-desktop.yml \ .github/workflows/release-verify-issues.yml \ .github/workflows/docs-impact.yml \ - .github/workflows/ci.yml \ - .github/workflows/cross-boundary-review.yml \ - .github/workflows/cross-boundary-review-signal.yml + .github/workflows/ci.yml bash scripts/release-workflows.test.sh - node --test .github/scripts/cross-boundary-review.test.cjs node scripts/check-single-release-public-contract.mjs # The site/ auth client has security-sensitive redirect-validation logic diff --git a/.github/workflows/cross-boundary-review-signal.yml b/.github/workflows/cross-boundary-review-signal.yml deleted file mode 100644 index 7cf4d6a0e6..0000000000 --- a/.github/workflows/cross-boundary-review-signal.yml +++ /dev/null @@ -1,18 +0,0 @@ -name: Cross-boundary review signal - -# This candidate-context workflow carries no write permission and executes no -# repository code. Its completion only asks the protected workflow_run policy -# to fetch current reviews and recompute the real status. -on: - pull_request_review: - types: [submitted, edited, dismissed] - -permissions: - contents: read - -jobs: - signal: - name: signal cross-boundary review reevaluation - runs-on: ubuntu-latest - steps: - - run: echo "Review state changed; protected policy will reevaluate the pull request." diff --git a/.github/workflows/cross-boundary-review.yml b/.github/workflows/cross-boundary-review.yml deleted file mode 100644 index a03936be6b..0000000000 --- a/.github/workflows/cross-boundary-review.yml +++ /dev/null @@ -1,42 +0,0 @@ -name: Cross-boundary review gate - -# Large changes or changes spanning multiple architectural owners are easy to -# validate by aggregate CI while missing a small contract migration. Evaluate -# them only from the protected default-branch control plane. Review workflows -# on the candidate ref may request reevaluation, but they cannot decide or -# publish the result. -on: - pull_request_target: - types: [opened, synchronize, reopened, ready_for_review, converted_to_draft] - workflow_run: - workflows: [Cross-boundary review signal] - types: [completed] - workflow_dispatch: - inputs: - pull_number: - description: Pull request number to reevaluate - required: true - type: number - -permissions: - contents: read - pull-requests: read - statuses: write - -jobs: - independent-review: - name: publish independent cross-boundary review status - runs-on: ubuntu-latest - steps: - - name: Checkout protected review policy - uses: actions/checkout@v7 - with: - ref: ${{ github.workflow_sha }} - path: review-control - persist-credentials: false - - - uses: actions/github-script@v9 - with: - script: | - const gate = require(`${process.env.GITHUB_WORKSPACE}/review-control/.github/scripts/cross-boundary-review.cjs`); - await gate({ github, context, core }); diff --git a/scripts/release-workflows.test.sh b/scripts/release-workflows.test.sh index 3824782bb2..06e7f3b030 100755 --- a/scripts/release-workflows.test.sh +++ b/scripts/release-workflows.test.sh @@ -117,19 +117,18 @@ grep -Fq 'name: Smoke-test Wails approval in WebView2' "$repo_root/.github/workf grep -Fq '../scripts/test-webview2-approval-smoke.ps1' "$repo_root/.github/workflows/ci.yml" grep -Fq 'wails build -clean -s -skipbindings -nopackage -platform windows/amd64 -webview2 embed' \ "$repo_root/.github/workflows/ci.yml" -review_gate="$repo_root/.github/workflows/cross-boundary-review.yml" -review_signal="$repo_root/.github/workflows/cross-boundary-review-signal.yml" -grep -Eq '^ pull_request_target:$' "$review_gate" -grep -Eq '^ workflow_run:$' "$review_gate" -grep -Eq '^ workflow_dispatch:$' "$review_gate" -! grep -Eq '^ pull_request_review:$' "$review_gate" -grep -Fq 'ref: ${{ github.workflow_sha }}' "$review_gate" -grep -Fq 'persist-credentials: false' "$review_gate" -grep -Fq 'review-control/.github/scripts/cross-boundary-review.cjs' "$review_gate" -grep -Eq '^ pull_request_review:$' "$review_signal" -! grep -Eq 'statuses: write|pull-requests: write|contents: write' "$review_signal" -grep -Fq 'review.commit_id' "$repo_root/.github/scripts/cross-boundary-review.cjs" -grep -Fq 'pull.head.sha' "$repo_root/.github/scripts/cross-boundary-review.cjs" +for retired_review_gate in \ + "$repo_root/.github/workflows/cross-boundary-review.yml" \ + "$repo_root/.github/workflows/cross-boundary-review-signal.yml" \ + "$repo_root/.github/scripts/cross-boundary-review.cjs" \ + "$repo_root/.github/scripts/cross-boundary-review.test.cjs"; do + if [ -e "$retired_review_gate" ]; then + echo "Retired cross-boundary review gate still exists: $retired_review_gate" >&2 + exit 1 + fi +done +! grep -Fq 'cross-boundary-review' "$repo_root/.github/workflows/ci.yml" +! grep -Fq 'independent cross-boundary review' "$repo_root/.github/pull_request_template.md" desktop_build_line="$(grep -n -m1 'name: Build and package' "$repo_root/.github/workflows/release-desktop.yml" | cut -d: -f1)" webview2_smoke_line="$(grep -n -m1 'name: Smoke-test Wails approval in WebView2' "$repo_root/.github/workflows/release-desktop.yml" | cut -d: -f1)" signpath_upload_line="$(grep -n -m1 'name: Upload unsigned Windows payload for SignPath' "$repo_root/.github/workflows/release-desktop.yml" | cut -d: -f1)"