mirror of
https://github.com/cozystack/cozystack.git
synced 2026-11-12 23:54:01 +09:00
Myasnikov Daniil
878f569e61
A commit status belongs to the commit, so two open PRs that carry the same head share one "API Review Gate" status. The fork workflow attributed that status to a fork PR by head SHA alone. A fork PR opened at a commit of this repository could therefore overwrite the verdict of the same-repo PR that owns it: resolve wrote a failure after api-gate-report had posted success, and the sweep applied the fork PR's reviews to a status the other PR had earned. A fork can reference any commit of the repository's network, so anyone able to open a PR could hold another PR's gate red. The fork workflow now leaves a head that an open same-repo PR carries to api-gate-report, and the sweep skips any head carried by more than one open PR. Two fork PRs at one head still fail closed in resolve, since their bases, and so their verdicts, can differ. The header of the same-repo workflow said skipped and cancelled runs post nothing. That stopped being true once a review posts pending ahead of its evaluation, so it now says they post no verdict. Assisted-by: LLM Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
352 lines
17 KiB
YAML
352 lines
17 KiB
YAML
name: API Review Gate
|
|
|
|
# Requires a review from a designated API owner (see API_OWNERS below) when a
|
|
# pull request makes a "sizeable" change to the Cozystack API surface: a new
|
|
# API group, a new resource, or a breaking change to an existing resource's
|
|
# schema. Detection is deterministic (cmd/api-gate); no LLM or network calls.
|
|
#
|
|
# The job runs on every PR (no path filter) so it can always reach a verdict
|
|
# without leaving PRs that touch no API stuck in "pending". When nothing
|
|
# sizeable changed it passes immediately.
|
|
#
|
|
# The merge gate is the COMMIT STATUS named "API Review Gate", posted by the
|
|
# `api-gate-report` job for same-repo PRs and by api-review-gate-fork.yaml for
|
|
# fork PRs — NOT by the evaluating job below. Same reasoning as the "E2E Tests"
|
|
# gate (docs/agents/e2e-testing.md §10), and it is what makes the
|
|
# `commented`-review skip below safe:
|
|
# - A required *job* that skips counts as PASSED by branch protection, so any
|
|
# path that skips the evaluating job would hand out a free pass (#3257). A
|
|
# status is "success" only when a report step posts it, and a never-posted
|
|
# required status stays "Expected", which blocks the merge.
|
|
# - 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 leave two verdicts standing for the same name, and a cancelled run
|
|
# then poisons the context for good. Commit statuses ARE deduplicated per
|
|
# context, newest wins.
|
|
#
|
|
# Consequence, and the invariant every change here must preserve: declining to
|
|
# post can only preserve a verdict some real run reached, never invent one. So
|
|
# skipped and cancelled runs post no verdict. The one thing a run posts before it
|
|
# has one is `pending`, which is not a pass and blocks the merge until a real
|
|
# verdict replaces it.
|
|
#
|
|
# For the gate to actually block a merge, main branch protection must require
|
|
# the "API Review Gate" status CONTEXT; without that it is advisory. Never wire
|
|
# branch protection to a job name.
|
|
|
|
on:
|
|
pull_request:
|
|
types: [opened, synchronize, reopened]
|
|
# Re-evaluate when a review is submitted or dismissed so an approval by an
|
|
# API owner flips the status from failing to passing without a new push. The
|
|
# job-level `if` below drops the review-bot noise (plain `commented` reviews);
|
|
# only reviews that can move the gate — approved, changes_requested,
|
|
# dismissed — actually re-run it.
|
|
pull_request_review:
|
|
types: [submitted, dismissed]
|
|
|
|
# A run that computes a verdict must never be cancelled by a run that does not.
|
|
# A plain `commented` review skips the evaluation (see the job `if`), so it gets
|
|
# its own group and can only cancel other no-op runs, while pushes and
|
|
# actionable reviews share the `eval` group and legitimately supersede each
|
|
# other. Under a single shared group a review bot commenting during the ~30s
|
|
# evaluation cancelled it and then contributed nothing — observed on real PRs
|
|
# (41d1153c, 4b1966c5), leaving a cancelled run as that SHA's only verdict.
|
|
concurrency:
|
|
group: >-
|
|
api-review-gate-${{ github.event.pull_request.number }}-${{
|
|
github.event_name == 'pull_request_review'
|
|
&& github.event.review.state == 'commented' && 'noop' || 'eval' }}
|
|
cancel-in-progress: true
|
|
|
|
permissions:
|
|
contents: read
|
|
pull-requests: read
|
|
|
|
env:
|
|
# Space-separated GitHub logins allowed to satisfy the gate. Edit here to
|
|
# change who can approve sizeable API changes. Workflow-level so the
|
|
# evaluating job and the published status description cannot drift apart.
|
|
API_OWNERS: "lllamnyp kvaps"
|
|
|
|
jobs:
|
|
api-review-gate:
|
|
# Deliberately NOT named after the gate: the merge gate is the "API Review
|
|
# Gate" commit status, and a required context that is also a job name
|
|
# inherits the skip-to-green and no-dedup problems in the header.
|
|
name: API review gate (evaluate)
|
|
# A push always evaluates the gate. A review event re-runs it only when the
|
|
# review can move the verdict: an approval can flip fail->pass, a dismissal
|
|
# or changes_requested can flip pass->fail. Plain `commented` reviews — all
|
|
# that the review bots (CodeRabbit, dosubot) ever post — cannot, so they skip
|
|
# here instead of rebuilding the detector on every bot comment. Safe because
|
|
# `api-gate-report` skips in lockstep and posts nothing, which leaves the
|
|
# last real verdict standing.
|
|
if: >-
|
|
github.event_name != 'pull_request_review' ||
|
|
github.event.review.state != 'commented'
|
|
runs-on: ubuntu-22.04
|
|
outputs:
|
|
sizeable: ${{ steps.detect.outputs.sizeable }}
|
|
verdict: ${{ steps.approve.outputs.verdict }}
|
|
env:
|
|
BASE_REF: ${{ github.event.pull_request.base.ref }}
|
|
HEAD_SHA: ${{ github.event.pull_request.head.sha }}
|
|
PR_NUMBER: ${{ github.event.pull_request.number }}
|
|
# The pipe falls back to direct on any proxy error (hack/common-envs.mk).
|
|
GOPROXY: "https://proxy.golang.org|direct"
|
|
steps:
|
|
- name: Checkout head
|
|
uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262 # v4
|
|
with:
|
|
ref: ${{ github.event.pull_request.head.sha }}
|
|
fetch-depth: 0
|
|
# Do not leave a usable token in the repo's git config: the PR-authored
|
|
# build and api-gate steps below must not have access to it. The token
|
|
# is scoped to the approval step alone, which is the only step that
|
|
# calls the GitHub API.
|
|
persist-credentials: false
|
|
|
|
- name: Set up Go
|
|
uses: actions/setup-go@40f1582b2485089dde7abd97c1529aa768e1baff # v5
|
|
with:
|
|
go-version-file: go.mod
|
|
cache: true
|
|
|
|
- name: Materialize base checkout at the merge base
|
|
run: |
|
|
set -euo pipefail
|
|
# Diff against the PR's actual merge base, not the base branch tip:
|
|
# otherwise unrelated commits landing on the base branch while the PR
|
|
# is open would invent or hide API findings.
|
|
git fetch --no-tags origin "${BASE_REF}"
|
|
merge_base="$(git merge-base "${HEAD_SHA}" "origin/${BASE_REF}")"
|
|
echo "Merge base: ${merge_base}"
|
|
git worktree add --detach "${RUNNER_TEMP}/base" "${merge_base}"
|
|
|
|
- name: Build api-gate
|
|
run: |
|
|
set -euo pipefail
|
|
# Build the detector from a trusted checkout so a PR cannot neuter the
|
|
# gate by editing its own detector code — the tool analyzes the head as
|
|
# *data*, not as code. This selection stays inline in the workflow (an
|
|
# API-owner-protected file that is always present) rather than in a
|
|
# helper script: a helper would be read from the untrusted PR head,
|
|
# which both breaks PRs whose head predates the helper and lets a PR
|
|
# steer the selection.
|
|
#
|
|
# Precedence: prefer the merge-base checkout; when the merge base
|
|
# predates the commit that introduced the gate (cmd/api-gate absent
|
|
# there and in the equally old head) fall back to the base branch tip,
|
|
# which is equally trusted (protected branch, not PR-authored) and
|
|
# current — this unblocks PRs whose merge base is older than the gate
|
|
# without forcing a rebase. Bootstrap from head only while the tool
|
|
# exists nowhere on the base branch yet; in that window CODEOWNERS on
|
|
# cmd/api-gate, internal/apigate, and this workflow keeps tampering
|
|
# behind an API-owner review.
|
|
if [ -d "${RUNNER_TEMP}/base/cmd/api-gate" ]; then
|
|
build_dir="${RUNNER_TEMP}/base"
|
|
echo "Building api-gate from the trusted merge-base checkout."
|
|
elif git cat-file -e "origin/${BASE_REF}:cmd/api-gate" 2>/dev/null; then
|
|
git worktree add --detach "${RUNNER_TEMP}/base-tip" "origin/${BASE_REF}"
|
|
build_dir="${RUNNER_TEMP}/base-tip"
|
|
echo "Building api-gate from the ${BASE_REF} tip; the merge base predates the gate."
|
|
else
|
|
build_dir="${GITHUB_WORKSPACE}"
|
|
echo "::warning::api-gate not present on the base branch yet; building from head (bootstrap). CODEOWNERS enforces API-owner review on the gate's own files."
|
|
fi
|
|
( cd "${build_dir}" && go build -o "${RUNNER_TEMP}/api-gate" ./cmd/api-gate )
|
|
|
|
- name: Detect sizeable API changes
|
|
id: detect
|
|
run: |
|
|
set -uo pipefail
|
|
set +e
|
|
"${RUNNER_TEMP}/api-gate" \
|
|
--base "${RUNNER_TEMP}/base" \
|
|
--head "${GITHUB_WORKSPACE}" \
|
|
--format markdown > "${RUNNER_TEMP}/report.md"
|
|
code=$?
|
|
set -e
|
|
echo "code=${code}" >> "$GITHUB_OUTPUT"
|
|
cat "${RUNNER_TEMP}/report.md"
|
|
case "${code}" in
|
|
0) echo "sizeable=false" >> "$GITHUB_OUTPUT" ;;
|
|
2) echo "sizeable=true" >> "$GITHUB_OUTPUT" ;;
|
|
*) echo "::error::api-gate failed with exit code ${code}"; exit "${code}" ;;
|
|
esac
|
|
|
|
- name: Check for an API owner approval
|
|
id: approve
|
|
if: steps.detect.outputs.sizeable == 'true'
|
|
env:
|
|
GH_TOKEN: ${{ github.token }}
|
|
run: |
|
|
set -euo pipefail
|
|
{
|
|
echo "## API Review Gate"
|
|
echo
|
|
cat "${RUNNER_TEMP}/report.md"
|
|
} >> "$GITHUB_STEP_SUMMARY"
|
|
|
|
# An APPROVED review by any API owner satisfies the gate, but only
|
|
# each reviewer's *latest* review *on the current head commit* counts:
|
|
# this drops approvals that a later commit invalidated, and approvals
|
|
# an owner subsequently superseded with CHANGES_REQUESTED or that was
|
|
# dismissed. A COMMENTED review is dropped before picking the latest:
|
|
# GitHub does not let a comment withdraw an approval, and the review
|
|
# trigger skips comments, so counting one here would make the verdict
|
|
# depend on which event happened to re-run the gate. GitHub forbids
|
|
# self-approval, so a sizeable PR authored by an owner still needs the
|
|
# other owner. api-review-gate-fork.yaml applies the same rule.
|
|
approvals="$(
|
|
gh api "repos/${GITHUB_REPOSITORY}/pulls/${PR_NUMBER}/reviews" --paginate |
|
|
jq -rs --arg head "${HEAD_SHA}" '
|
|
add
|
|
| map(select(.commit_id == $head and .state != "COMMENTED"))
|
|
| sort_by(.submitted_at // "")
|
|
| group_by(.user.login)
|
|
| map(last)
|
|
| .[]
|
|
| select(.state == "APPROVED")
|
|
| .user.login
|
|
'
|
|
)"
|
|
|
|
# The verdict is recorded as a step output so `api-gate-report` can tell
|
|
# "sizeable change, no owner approval" apart from "the evaluation itself
|
|
# broke" — both of which fail this job. Written before the exit in each
|
|
# branch, since a step output set after exiting is never emitted.
|
|
for owner in ${API_OWNERS}; do
|
|
if grep -qxi "${owner}" <<<"${approvals}"; then
|
|
echo "Sizeable API change approved by @${owner}."
|
|
echo "verdict=pass-approved" >> "$GITHUB_OUTPUT"
|
|
exit 0
|
|
fi
|
|
done
|
|
|
|
echo "verdict=fail-needs-approval" >> "$GITHUB_OUTPUT"
|
|
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."
|
|
exit 1
|
|
|
|
# A dismissal or a request for changes can turn a green head red, and until
|
|
# the evaluation reports, about a minute later, the old `success` would still
|
|
# let the PR merge. Same opener as the "E2E Tests" lane. Pushes need none: a
|
|
# new head SHA starts with no status. Losing it to a transient API error only
|
|
# reopens that window, so it must not fail the run.
|
|
api-gate-pending:
|
|
name: Mark API review gate as re-evaluating
|
|
if: >-
|
|
${{ github.event_name == 'pull_request_review'
|
|
&& github.event.review.state != 'commented'
|
|
&& !github.event.pull_request.head.repo.fork }}
|
|
runs-on: ubuntu-latest
|
|
continue-on-error: true
|
|
permissions:
|
|
statuses: write
|
|
steps:
|
|
- name: Report pending "API Review Gate" commit status
|
|
uses: actions/github-script@f28e40c7f34bde8b3046d885e986cb6290c5673b # v7
|
|
env:
|
|
HEAD_SHA: ${{ github.event.pull_request.head.sha }}
|
|
RUN_URL: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}
|
|
with:
|
|
script: |
|
|
const { HEAD_SHA, RUN_URL } = process.env;
|
|
const { owner, repo } = context.repo;
|
|
await github.rest.repos.createCommitStatus({
|
|
owner, repo, sha: HEAD_SHA, state: 'pending',
|
|
context: 'API Review Gate', description: 're-evaluating after a review',
|
|
target_url: RUN_URL,
|
|
});
|
|
|
|
# Publish the required "API Review Gate" commit status. Separate from the
|
|
# evaluating job for the reasons in the header: the gate must not be a job
|
|
# name, and a run that reached no verdict must post none.
|
|
#
|
|
# The `if` guard stays in LOCKSTEP with the evaluating job's — this job runs if
|
|
# and only if that one does. A `commented` review skips both, so the previous
|
|
# real status stands untouched. `!cancelled()` rather than `always()`: a run
|
|
# killed by the concurrency group has no verdict, and posting one anyway would
|
|
# overwrite a green status with red — the exact failure #3262 hit twice with
|
|
# "E2E Tests" (see its `e2e-report`).
|
|
#
|
|
# Fork PRs skip this job. Their GITHUB_TOKEN is read-only on BOTH the
|
|
# `pull_request` and `pull_request_review` triggers, so no run of this workflow
|
|
# can post a status for a fork, and its verdict could not be relayed anyway: a
|
|
# fork's run executes the fork's copy of this file. api-review-gate-fork.yaml
|
|
# evaluates fork PRs again from the default branch. Until it does, the required
|
|
# status is unposted and the fork PR is not mergeable — fail-closed by
|
|
# construction.
|
|
api-gate-report:
|
|
name: Report API review gate status
|
|
needs: [api-gate-pending, api-review-gate]
|
|
if: >-
|
|
${{ !cancelled()
|
|
&& !github.event.pull_request.head.repo.fork
|
|
&& (github.event_name != 'pull_request_review'
|
|
|| github.event.review.state != 'commented') }}
|
|
runs-on: ubuntu-latest
|
|
permissions:
|
|
contents: read
|
|
statuses: write
|
|
steps:
|
|
- name: Report "API Review Gate" commit status
|
|
uses: actions/github-script@f28e40c7f34bde8b3046d885e986cb6290c5673b # v7
|
|
env:
|
|
GATE_RESULT: ${{ needs.api-review-gate.result }}
|
|
SIZEABLE: ${{ needs.api-review-gate.outputs.sizeable }}
|
|
VERDICT: ${{ needs.api-review-gate.outputs.verdict }}
|
|
# Inherited from the workflow-level env; passed explicitly so the
|
|
# script's use of it is visible at the call site.
|
|
API_OWNERS: ${{ env.API_OWNERS }}
|
|
HEAD_SHA: ${{ github.event.pull_request.head.sha }}
|
|
RUN_URL: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}
|
|
with:
|
|
script: |
|
|
const { GATE_RESULT, SIZEABLE, VERDICT, API_OWNERS, HEAD_SHA, RUN_URL } = process.env;
|
|
const { owner, repo } = context.repo;
|
|
|
|
// No verdict to publish — say nothing and let the last real status
|
|
// stand. Reachable when the evaluating job was skipped despite the
|
|
// lockstep guard (a `needs` short-circuit), and defensive against a
|
|
// future guard drift.
|
|
if (GATE_RESULT !== 'success' && GATE_RESULT !== 'failure') {
|
|
core.info(`evaluation result '${GATE_RESULT}' — posting nothing, previous status stands`);
|
|
return;
|
|
}
|
|
|
|
let state, description;
|
|
if (GATE_RESULT === 'success') {
|
|
state = 'success';
|
|
description = SIZEABLE === 'true'
|
|
? 'sizeable API change approved by an API owner'
|
|
: 'no sizeable API change';
|
|
} else if (VERDICT === 'fail-needs-approval') {
|
|
state = 'failure';
|
|
description = `sizeable API change needs an approving review from: ${API_OWNERS}`;
|
|
} else {
|
|
// Either the job failed before reaching a verdict (checkout, build or
|
|
// the detector), or it reached `fail-needs-approval` but the output did
|
|
// not survive the failing job — whether a failed job propagates its
|
|
// outputs is not something the docs pin down. Only the description
|
|
// degrades: the state is `failure` down both paths, so the gate itself
|
|
// is unaffected. Fail-closed either way — an unverifiable gate is red.
|
|
state = 'failure';
|
|
description = 'gate evaluation did not complete — see the run for details';
|
|
}
|
|
|
|
try {
|
|
await github.rest.repos.createCommitStatus({
|
|
owner, repo, sha: HEAD_SHA, state,
|
|
context: 'API Review Gate', description, target_url: RUN_URL,
|
|
});
|
|
} catch (err) {
|
|
// Fail loudly and fail-closed: setFailed keeps this job red and the
|
|
// required status unposted ("Expected"), so the PR stays unmergeable.
|
|
core.setFailed(`Failed to report API Review Gate status: ${err}`);
|
|
return;
|
|
}
|
|
core.info(`API Review Gate → ${state}: ${description}`);
|