diff --git a/.github/risk-tiers.yml b/.github/risk-tiers.yml index 6de076219..ae5e35e5c 100644 --- a/.github/risk-tiers.yml +++ b/.github/risk-tiers.yml @@ -87,7 +87,7 @@ high: # Applied by the Contributor Reputation Check writer. - "needs-review:HIGH" approvals: - required: 2 + required: 1 require_core: true medium: diff --git a/docs/maintainers/submission-gate.md b/docs/maintainers/submission-gate.md index f899f1a8d..8832c8d41 100644 --- a/docs/maintainers/submission-gate.md +++ b/docs/maintainers/submission-gate.md @@ -131,7 +131,7 @@ The status comment includes a collapsed "Why this tier" list with the reasons th |---|---| | `merge-risk:low` | 1 approval from an owner of the changed resource: its domain pool, or the core pools for files outside every domain | | `merge-risk:medium` | 1 approval from a domain reviewer for the area touched | -| `merge-risk:high` | 2 approvals, including a core maintainer | +| `merge-risk:high` | 1 approval from a core maintainer | How approvals are counted: @@ -153,7 +153,9 @@ The core pool is `core-maintainers`. Its members also satisfy the domain and res If routing isn't staffed yet, the gate falls back instead of blocking: - **`.github/review-routing.yml` missing, or the matching domain pools empty:** any approver with write access satisfies the domain or resource-owner requirement. If the file exists but can't be parsed, or `pools` isn't a mapping, the gate fails instead of falling back, so a broken edit can't relax approvals. -- **`core-maintainers` pool empty:** a high-risk PR needs one of its two approvals from a user with `admin` or `maintain` permission. +- **`core-maintainers` pool empty:** a high-risk PR needs its approval from a user with `admin` or `maintain` permission. + +The `submission-gate` check reports `action_required` ("Action required") rather than `failure` when the only thing blocking a PR is a missing approval, so it reads as "waiting for a reviewer" instead of a broken workflow. The check summary lists exactly what is missing. It still blocks merging like any non-passing required check. Failed checks and requested changes still report `failure`. The status comment shows a note whenever a fallback is in effect. @@ -223,7 +225,7 @@ Commands use the same reader/writer split as the gate, so they work on PRs from - **Only trusted code runs.** No workflow here executes PR code with a write token. The gate loads its logic from the base commit, except in the bootstrap case described above, which still has only a read-only token. The writer and command workflows check out the default branch and install dependencies with `npm ci --ignore-scripts`. - **Contributor reputation artifact.** It is used only as a raise-only signal. It must match schema `contributor-check-result/v1` and the PR head SHA. If the `pr-check` job succeeded but the artifact is missing or unreadable, the gate records an infrastructure failure and blocks until a later sweep reads it, so a lost `HIGH` signal can't lower the tier. - **Untrusted text.** Comment bodies are never interpolated into scripts: the reader passes them through environment variables, and the writer re-reads them from the API. Reviewer logins, check names, and details are sanitized before they go into markdown. -- **Tampering with other checks.** A PR can also edit the validation workflows the gate waits for. Any such edit touches `.github/**`, which makes the PR `merge-risk:high`: 2 approvals, including a core maintainer. The writer computes that tier from default-branch code. +- **Tampering with other checks.** A PR can also edit the validation workflows the gate waits for. Any such edit touches `.github/**`, which makes the PR `merge-risk:high`: 1 approval from a core maintainer. The writer computes that tier from default-branch code. - **Check source.** Check runs created with `GITHUB_TOKEN` belong to the GitHub Actions app, the same app as `pull_request` jobs. Requiring `submission-gate` from GitHub Actions in the ruleset blocks other apps and commit statuses; impostor detection covers jobs in the same app. - **Events.** Check runs created with `GITHUB_TOKEN` don't trigger `check_run` workflows. Automation that reacts to the gate should listen for `workflow_run` on **Submission Gate Writer**. - **Rate limits.** The hourly writer sweep refreshes at most 60 open PRs updated in the last 30 days, so a large backlog stays within API rate limits. diff --git a/eng/submission-gate.mjs b/eng/submission-gate.mjs index f5cce8839..328809af4 100644 --- a/eng/submission-gate.mjs +++ b/eng/submission-gate.mjs @@ -1097,6 +1097,22 @@ export async function findImpostorGateChecks(github, { owner, repo, headSha }) { return runs.filter((run) => run.external_id !== GATE_CHECK_EXTERNAL_ID); } +/** + * A PR that is only waiting for a review is not broken: report `action_required`, which + * still blocks the required check but reads as "needs a reviewer" rather than a failure. + */ +export function gateConclusion(evaluation) { + if (evaluation.passed) return "success"; + const { automation, approvals } = evaluation; + const onlyAwaitingReview = + automation.contributionFailures.length === 0 && + automation.infrastructureFailures.length === 0 && + automation.pending.length === 0 && + (approvals.changesRequestedBy ?? []).length === 0 && + !approvals.satisfied; + return onlyAwaitingReview ? "action_required" : "failure"; +} + /** * Publish the required `submission-gate` check on the PR head commit. Only the trusted * writer calls this, so the check can't be satisfied by editing a PR-controlled workflow. @@ -1114,7 +1130,7 @@ export async function publishGateCheck(github, { owner, repo, evaluation, detail }; const fields = pending ? { status: "in_progress", output } - : { status: "completed", conclusion: evaluation.passed ? "success" : "failure", completed_at: new Date().toISOString(), output }; + : { status: "completed", conclusion: gateConclusion(evaluation), completed_at: new Date().toISOString(), output }; if (detailsUrl) fields.details_url = detailsUrl; const runs = await listGateCheckRuns(github, { owner, repo, headSha: evaluation.headSha }); diff --git a/eng/submission-gate.test.mjs b/eng/submission-gate.test.mjs index 58073622f..b0c891d37 100644 --- a/eng/submission-gate.test.mjs +++ b/eng/submission-gate.test.mjs @@ -11,6 +11,7 @@ import { classifyRisk, computeState, evaluateApprovals, + gateConclusion, evaluateCheck, evaluateSubmission, globToRegExp, @@ -286,17 +287,25 @@ test("medium tier requires a domain reviewer when the pool is staffed", () => { assert.equal(noRouting.satisfied, true); }); -test("high tier requires two approvals including a core maintainer", () => { +test("high tier requires one approval from a core maintainer", () => { const permissions = new Map([["alice", "write"], ["bob", "write"], ["corea", "write"], ["admin1", "admin"]]); const files = [file(".github/workflows/x.yml")]; const base = { tier: "high", tiers, author: "author", permissions, routing, files }; - assert.equal(evaluateApprovals({ ...base, reviews: [review("corea", "APPROVED")] }).satisfied, false); + assert.equal(evaluateApprovals({ ...base, reviews: [review("corea", "APPROVED")] }).satisfied, true); assert.equal(evaluateApprovals({ ...base, reviews: [review("alice", "APPROVED"), review("bob", "APPROVED")] }).satisfied, false); - assert.equal(evaluateApprovals({ ...base, reviews: [review("alice", "APPROVED"), review("corea", "APPROVED")] }).satisfied, true); const fallback = { ...base, routing: null }; - assert.equal(evaluateApprovals({ ...fallback, reviews: [review("alice", "APPROVED"), review("bob", "APPROVED")] }).satisfied, false); - assert.equal(evaluateApprovals({ ...fallback, reviews: [review("alice", "APPROVED"), review("admin1", "APPROVED")] }).satisfied, true); + assert.equal(evaluateApprovals({ ...fallback, reviews: [review("alice", "APPROVED")] }).satisfied, false); + assert.equal(evaluateApprovals({ ...fallback, reviews: [review("admin1", "APPROVED")] }).satisfied, true); +}); + +test("gateConclusion reports a PR that only awaits review as action_required", () => { + const clean = { contributionFailures: [], infrastructureFailures: [], pending: [] }; + const waiting = { satisfied: false, changesRequestedBy: [] }; + assert.equal(gateConclusion({ passed: true, automation: clean, approvals: { satisfied: true, changesRequestedBy: [] } }), "success"); + assert.equal(gateConclusion({ passed: false, automation: clean, approvals: waiting }), "action_required"); + assert.equal(gateConclusion({ passed: false, automation: clean, approvals: { ...waiting, changesRequestedBy: ["bob"] } }), "failure"); + assert.equal(gateConclusion({ passed: false, automation: { ...clean, contributionFailures: [{}] }, approvals: waiting }), "failure"); }); // --- checks ------------------------------------------------------------------- @@ -916,7 +925,7 @@ test("publishGateCheck corrects copies of the writer's check that claim success await publishGateCheck(github, { owner: "github", repo: "awesome-copilot", evaluation, log: (line) => logs.push(line) }); const updates = github.calls.filter((call) => call.name === "checks.update"); assert.deepEqual(updates.map((call) => call.params.check_run_id), [11]); - assert.equal(updates[0].params.conclusion, "failure"); + assert.equal(updates[0].params.conclusion, "action_required"); assert.ok(logs.some((line) => /Correcting 1/.test(line))); });