Skip to content

Commit b771962

Browse files
myasnikovdaniilmyasnikovdaniil
committed
ci(api-review-gate): publish the gate as a commit status
The gate's merge context was its job name, which inherits two GitHub behaviours that together make it both unenforceable and self-poisoning. A required *job* that skips counts as PASSED by branch protection, so any path that skips the evaluating job hands out a free pass — the hole that let fork PRs merge with e2e never run (#3257). And check runs are not deduplicated per name: every workflow run gets its own check suite, so `GET /commits/{sha}/check-runs?filter=latest` returns the same set as `filter=all`, and the rollup is worst-of. Two runs on one head SHA therefore leave two verdicts standing for one context name, and a cancelled run poisons the context until the next push. Commit statuses are deduplicated per context, newest wins. Rename the evaluating job off the gate context and publish "API Review Gate" as a commit status: `api-gate-report` for same-repo PRs, and a privileged workflow_run for forks, whose GITHUB_TOKEN is read-only on both the pull_request and the pull_request_review trigger and so can never post a status itself. Skipped and cancelled runs post nothing, so declining to post can only ever preserve a verdict some real run reached — it can never invent one. That invariant is what makes it safe to stop re-evaluating on the plain `commented` reviews the review bots post: such a review cannot move a verdict that counts only APPROVED reviews, and the run that skips now leaves the previous status untouched instead of overwriting it. Also split the concurrency group so a run that computes no verdict cannot cancel one that does. A bot commenting during the ~30s evaluation used to cancel it and then contribute nothing, leaving a cancelled run as that SHA's only api-gate verdict (41d1153, 4b1966c). For the gate to actually block a merge, main branch protection must require the "API Review Gate" status context; until then it is advisory, as the job name was. Assisted-by: LLM Signed-off-by: Myasnikov Daniil <[email protected]>
1 parent e1e925a commit b771962

2 files changed

Lines changed: 261 additions & 10 deletions

File tree

‎.github/workflows/api-review-gate-fork.yaml‎

Lines changed: 102 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,102 @@
1+
name: API Review Gate (fork)
2+
3+
# Privileged publisher for the "API Review Gate" merge gate on FORK pull
4+
# requests, mirroring the split e2e-fork.yaml uses for "E2E Tests"
5+
# (docs/agents/e2e-testing.md §10).
6+
#
7+
# A fork PR cannot publish its own merge gate: the GITHUB_TOKEN is read-only for
8+
# pull requests from a fork on BOTH the `pull_request` and the
9+
# `pull_request_review` trigger, so no run of api-review-gate.yaml can call
10+
# createCommitStatus for a fork — the read-only restriction is not specific to
11+
# `pull_request`. This workflow runs from the DEFAULT branch after that run
12+
# completes, so it holds a write-capable token, and concludes the status on the
13+
# PR head SHA.
14+
#
15+
# Nothing untrusted runs here, and nothing needs to: the evaluation already
16+
# happened in the unprivileged run. The only inputs are that run's conclusion and
17+
# its head SHA — base-repo payload data a fork cannot forge — so this workflow
18+
# checks nothing out, builds nothing, and executes no fork-authored code. That is
19+
# the whole reason the gate is not a `pull_request_target` run, which would hand
20+
# fork code a privileged token to reach the same result.
21+
#
22+
# Same-repo PRs are published by api-review-gate.yaml's `api-gate-report` job;
23+
# this workflow no-ops for them (see the job guard), so the two publishers are
24+
# mutually exclusive and can never clobber each other.
25+
26+
on:
27+
workflow_run:
28+
workflows: ["API Review Gate"]
29+
types: [completed]
30+
31+
# Supersede a stale publish when the fork pushes again. Keyed by fork repo plus
32+
# branch because the PR number is not available at workflow level. Safe to cancel:
33+
# statuses are per-SHA, and a newer gate run publishes its own.
34+
concurrency:
35+
group: api-review-gate-fork-${{ github.event.workflow_run.head_repository.full_name }}-${{ github.event.workflow_run.head_branch }}
36+
cancel-in-progress: true
37+
38+
permissions:
39+
contents: read
40+
41+
jobs:
42+
report:
43+
name: Report fork API review gate status
44+
# Fork PRs only: the head repository differs from the base repository.
45+
if: >-
46+
${{ github.event.workflow_run.head_repository.full_name != github.event.workflow_run.repository.full_name }}
47+
runs-on: ubuntu-latest
48+
permissions:
49+
contents: read
50+
statuses: write
51+
steps:
52+
- name: Report "API Review Gate" commit status
53+
uses: actions/github-script@f28e40c7f34bde8b3046d885e986cb6290c5673b # v7
54+
env:
55+
HEAD_SHA: ${{ github.event.workflow_run.head_sha }}
56+
CONCLUSION: ${{ github.event.workflow_run.conclusion }}
57+
GATE_RUN_URL: ${{ github.event.workflow_run.html_url }}
58+
with:
59+
script: |
60+
const { HEAD_SHA, CONCLUSION, GATE_RUN_URL } = process.env;
61+
const { owner, repo } = context.repo;
62+
63+
// Validate before the SHA flows into a status target.
64+
if (!/^[0-9a-f]{40}$/.test(HEAD_SHA)) {
65+
core.setFailed(`unexpected head_sha '${HEAD_SHA}'`);
66+
return;
67+
}
68+
69+
// Publish only a real verdict. A `commented` review skips every job in
70+
// the gate run (conclusion `skipped`); a run cancelled by the
71+
// concurrency group never evaluated; a fork run still awaiting
72+
// maintainer approval concludes `action_required`. In every one of those
73+
// cases the previous status on this SHA is still the correct one, and
74+
// writing over it would either invent a pass or clobber a green.
75+
// Declining to post is also fail-closed: on a SHA that never got a
76+
// status the required context stays "Expected" and blocks the merge.
77+
if (CONCLUSION !== 'success' && CONCLUSION !== 'failure') {
78+
core.info(`gate run concluded '${CONCLUSION}' — posting nothing`);
79+
return;
80+
}
81+
82+
// A workflow_run payload carries no job outputs, so unlike the
83+
// same-repo `api-gate-report` this cannot separate "needs an API owner
84+
// approval" from "the evaluation broke". target_url points at the gate
85+
// run, whose job summary spells the verdict out.
86+
const state = CONCLUSION === 'success' ? 'success' : 'failure';
87+
const description = state === 'success'
88+
? 'API review gate satisfied'
89+
: 'API review gate not satisfied — see the gate run';
90+
91+
try {
92+
await github.rest.repos.createCommitStatus({
93+
owner, repo, sha: HEAD_SHA, state,
94+
context: 'API Review Gate', description, target_url: GATE_RUN_URL,
95+
});
96+
} catch (err) {
97+
// Fail loudly and fail-closed: an unposted required status stays
98+
// "Expected", which blocks the merge.
99+
core.setFailed(`Failed to report API Review Gate status: ${err}`);
100+
return;
101+
}
102+
core.info(`API Review Gate → ${state}: ${description}`);

‎.github/workflows/api-review-gate.yaml‎

Lines changed: 159 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -5,35 +5,90 @@ name: API Review Gate
55
# API group, a new resource, or a breaking change to an existing resource's
66
# schema. Detection is deterministic (cmd/api-gate); no LLM or network calls.
77
#
8-
# The job runs on every PR (no path filter) so it can always report a status
9-
# and therefore be marked as a required check in branch protection without
10-
# leaving PRs that touch no API stuck in "pending". When nothing sizeable
11-
# changed it passes immediately.
8+
# The job runs on every PR (no path filter) so it can always reach a verdict
9+
# without leaving PRs that touch no API stuck in "pending". When nothing
10+
# sizeable changed it passes immediately.
11+
#
12+
# The merge gate is the COMMIT STATUS named "API Review Gate", posted by the
13+
# `api-gate-report` job for same-repo PRs and by api-review-gate-fork.yaml for
14+
# fork PRs — NOT by the evaluating job below. Same reasoning as the "E2E Tests"
15+
# gate (docs/agents/e2e-testing.md §10), and it is what makes the
16+
# `commented`-review skip below safe:
17+
# - A required *job* that skips counts as PASSED by branch protection, so any
18+
# path that skips the evaluating job would hand out a free pass (#3257). A
19+
# status is "success" only when a report step posts it, and a never-posted
20+
# required status stays "Expected", which blocks the merge.
21+
# - Check runs are NOT deduplicated per name: every workflow run gets its own
22+
# check suite, so `GET /commits/{sha}/check-runs?filter=latest` returns the
23+
# same set as `filter=all` and the rollup is worst-of. Two runs on one head
24+
# SHA leave two verdicts standing for the same name, and a cancelled run
25+
# then poisons the context for good. Commit statuses ARE deduplicated per
26+
# context, newest wins.
27+
#
28+
# Consequence, and the invariant every change here must preserve: declining to
29+
# post can only preserve a verdict some real run reached, never invent one. So
30+
# skipped and cancelled runs post nothing.
31+
#
32+
# For the gate to actually block a merge, main branch protection must require
33+
# the "API Review Gate" status CONTEXT; without that it is advisory. Never wire
34+
# branch protection to a job name.
1235

1336
on:
1437
pull_request:
1538
types: [opened, synchronize, reopened]
1639
# Re-evaluate when a review is submitted or dismissed so an approval by an
17-
# API owner flips the check from failing to passing without a new push.
40+
# API owner flips the status from failing to passing without a new push. The
41+
# job-level `if` below drops the review-bot noise (plain `commented` reviews);
42+
# only reviews that can move the gate — approved, changes_requested,
43+
# dismissed — actually re-run it.
1844
pull_request_review:
1945
types: [submitted, dismissed]
2046

47+
# A run that computes a verdict must never be cancelled by a run that does not.
48+
# A plain `commented` review skips the evaluation (see the job `if`), so it gets
49+
# its own group and can only cancel other no-op runs, while pushes and
50+
# actionable reviews share the `eval` group and legitimately supersede each
51+
# other. Under a single shared group a review bot commenting during the ~30s
52+
# evaluation cancelled it and then contributed nothing — observed on real PRs
53+
# (41d1153c, 4b1966c5), leaving a cancelled run as that SHA's only verdict.
2154
concurrency:
22-
group: api-review-gate-${{ github.event.pull_request.number }}
55+
group: >-
56+
api-review-gate-${{ github.event.pull_request.number }}-${{
57+
github.event_name == 'pull_request_review'
58+
&& github.event.review.state == 'commented' && 'noop' || 'eval' }}
2359
cancel-in-progress: true
2460

2561
permissions:
2662
contents: read
2763
pull-requests: read
2864

65+
env:
66+
# Space-separated GitHub logins allowed to satisfy the gate. Edit here to
67+
# change who can approve sizeable API changes. Workflow-level so the
68+
# evaluating job and the published status description cannot drift apart.
69+
API_OWNERS: "lllamnyp kvaps"
70+
2971
jobs:
3072
api-review-gate:
31-
name: Require API owner review for sizeable API changes
73+
# Deliberately NOT named after the gate: the merge gate is the "API Review
74+
# Gate" commit status, and a required context that is also a job name
75+
# inherits the skip-to-green and no-dedup problems in the header.
76+
name: API review gate (evaluate)
77+
# A push always evaluates the gate. A review event re-runs it only when the
78+
# review can move the verdict: an approval can flip fail->pass, a dismissal
79+
# or changes_requested can flip pass->fail. Plain `commented` reviews — all
80+
# that the review bots (CodeRabbit, dosubot) ever post — cannot, so they skip
81+
# here instead of rebuilding the detector on every bot comment. Safe because
82+
# `api-gate-report` skips in lockstep and posts nothing, which leaves the
83+
# last real verdict standing.
84+
if: >-
85+
github.event_name != 'pull_request_review' ||
86+
github.event.review.state != 'commented'
3287
runs-on: ubuntu-22.04
88+
outputs:
89+
sizeable: ${{ steps.detect.outputs.sizeable }}
90+
verdict: ${{ steps.approve.outputs.verdict }}
3391
env:
34-
# Space-separated GitHub logins allowed to satisfy the gate. Edit here to
35-
# change who can approve sizeable API changes.
36-
API_OWNERS: "lllamnyp kvaps"
3792
BASE_REF: ${{ github.event.pull_request.base.ref }}
3893
HEAD_SHA: ${{ github.event.pull_request.head.sha }}
3994
PR_NUMBER: ${{ github.event.pull_request.number }}
@@ -121,6 +176,7 @@ jobs:
121176
esac
122177
123178
- name: Check for an API owner approval
179+
id: approve
124180
if: steps.detect.outputs.sizeable == 'true'
125181
env:
126182
GH_TOKEN: ${{ github.token }}
@@ -152,12 +208,105 @@ jobs:
152208
'
153209
)"
154210
211+
# The verdict is recorded as a step output so `api-gate-report` can tell
212+
# "sizeable change, no owner approval" apart from "the evaluation itself
213+
# broke" — both of which fail this job. Written before the exit in each
214+
# branch, since a step output set after exiting is never emitted.
155215
for owner in ${API_OWNERS}; do
156216
if grep -qxi "${owner}" <<<"${approvals}"; then
157217
echo "Sizeable API change approved by @${owner}."
218+
echo "verdict=pass-approved" >> "$GITHUB_OUTPUT"
158219
exit 0
159220
fi
160221
done
161222
223+
echo "verdict=fail-needs-approval" >> "$GITHUB_OUTPUT"
162224
echo "::error::This PR makes a sizeable API change (new group, new resource, or breaking change) and needs an approving review from one of: ${API_OWNERS}. See the job summary for details."
163225
exit 1
226+
227+
# Publish the required "API Review Gate" commit status. Separate from the
228+
# evaluating job for the reasons in the header: the gate must not be a job
229+
# name, and a run that reached no verdict must post nothing.
230+
#
231+
# The `if` guard stays in LOCKSTEP with the evaluating job's — this job runs if
232+
# and only if that one does. A `commented` review skips both, so the previous
233+
# real status stands untouched. `!cancelled()` rather than `always()`: a run
234+
# killed by the concurrency group has no verdict, and posting one anyway would
235+
# overwrite a green status with red — the exact failure #3262 hit twice with
236+
# "E2E Tests" (see its `e2e-report`).
237+
#
238+
# Fork PRs skip this job. Their GITHUB_TOKEN is read-only on BOTH the
239+
# `pull_request` and `pull_request_review` triggers, so no run of this workflow
240+
# can post a status for a fork; api-review-gate-fork.yaml concludes the context
241+
# for them from a privileged workflow_run. Until it does, the required status is
242+
# unposted and the fork PR is not mergeable — fail-closed by construction.
243+
api-gate-report:
244+
name: Report API review gate status
245+
needs: [api-review-gate]
246+
if: >-
247+
${{ !cancelled()
248+
&& !github.event.pull_request.head.repo.fork
249+
&& (github.event_name != 'pull_request_review'
250+
|| github.event.review.state != 'commented') }}
251+
runs-on: ubuntu-latest
252+
permissions:
253+
contents: read
254+
statuses: write
255+
steps:
256+
- name: Report "API Review Gate" commit status
257+
uses: actions/github-script@f28e40c7f34bde8b3046d885e986cb6290c5673b # v7
258+
env:
259+
GATE_RESULT: ${{ needs.api-review-gate.result }}
260+
SIZEABLE: ${{ needs.api-review-gate.outputs.sizeable }}
261+
VERDICT: ${{ needs.api-review-gate.outputs.verdict }}
262+
# Inherited from the workflow-level env; passed explicitly so the
263+
# script's use of it is visible at the call site.
264+
API_OWNERS: ${{ env.API_OWNERS }}
265+
HEAD_SHA: ${{ github.event.pull_request.head.sha }}
266+
RUN_URL: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}
267+
with:
268+
script: |
269+
const { GATE_RESULT, SIZEABLE, VERDICT, API_OWNERS, HEAD_SHA, RUN_URL } = process.env;
270+
const { owner, repo } = context.repo;
271+
272+
// No verdict to publish — say nothing and let the last real status
273+
// stand. Reachable when the evaluating job was skipped despite the
274+
// lockstep guard (a `needs` short-circuit), and defensive against a
275+
// future guard drift.
276+
if (GATE_RESULT !== 'success' && GATE_RESULT !== 'failure') {
277+
core.info(`evaluation result '${GATE_RESULT}' — posting nothing, previous status stands`);
278+
return;
279+
}
280+
281+
let state, description;
282+
if (GATE_RESULT === 'success') {
283+
state = 'success';
284+
description = SIZEABLE === 'true'
285+
? 'sizeable API change approved by an API owner'
286+
: 'no sizeable API change';
287+
} else if (VERDICT === 'fail-needs-approval') {
288+
state = 'failure';
289+
description = `sizeable API change needs an approving review from: ${API_OWNERS}`;
290+
} else {
291+
// Either the job failed before reaching a verdict (checkout, build or
292+
// the detector), or it reached `fail-needs-approval` but the output did
293+
// not survive the failing job — whether a failed job propagates its
294+
// outputs is not something the docs pin down. Only the description
295+
// degrades: the state is `failure` down both paths, so the gate itself
296+
// is unaffected. Fail-closed either way — an unverifiable gate is red.
297+
state = 'failure';
298+
description = 'gate evaluation did not complete — see the run for details';
299+
}
300+
301+
try {
302+
await github.rest.repos.createCommitStatus({
303+
owner, repo, sha: HEAD_SHA, state,
304+
context: 'API Review Gate', description, target_url: RUN_URL,
305+
});
306+
} catch (err) {
307+
// Fail loudly and fail-closed: setFailed keeps this job red and the
308+
// required status unposted ("Expected"), so the PR stays unmergeable.
309+
core.setFailed(`Failed to report API Review Gate status: ${err}`);
310+
return;
311+
}
312+
core.info(`API Review Gate → ${state}: ${description}`);

0 commit comments

Comments
 (0)