Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/risk-tiers.yml
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ high:
# Applied by the Contributor Reputation Check writer.
- "needs-review:HIGH"
approvals:
required: 2
required: 1
require_core: true

medium:
Expand Down
8 changes: 5 additions & 3 deletions docs/maintainers/submission-gate.md
Original file line number Diff line number Diff line change
Expand Up @@ -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:

Expand All @@ -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.

Expand Down Expand Up @@ -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.
Expand Down
18 changes: 17 additions & 1 deletion eng/submission-gate.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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 });
Expand Down
21 changes: 15 additions & 6 deletions eng/submission-gate.test.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import {
classifyRisk,
computeState,
evaluateApprovals,
gateConclusion,
evaluateCheck,
evaluateSubmission,
globToRegExp,
Expand Down Expand Up @@ -274,17 +275,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 -------------------------------------------------------------------
Expand Down Expand Up @@ -904,7 +913,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)));
});

Expand Down
Loading