diff --git a/.gitea/workflows/review-verdict.yml b/.gitea/workflows/review-verdict.yml index fedfc6841..aa8c49b0f 100644 --- a/.gitea/workflows/review-verdict.yml +++ b/.gitea/workflows/review-verdict.yml @@ -1,59 +1,13 @@ name: Review verdict -# ersatztv#622 — server-side H10 enforcement. +# ersatztv#672 PROBE — THROWAWAY. This is a deliberately rewritten copy of the real +# review-verdict.yml, used to answer one question empirically: does Gitea resolve a +# `pull_request` workflow DEFINITION from the PR's own head commit? # -# THE INVARIANT. `review-verdict/h10` is a REQUIRED status check on `main`, and a Gitea commit -# status belongs to exactly ONE sha. So a commit that did not exist when a verdict was written can -# never inherit that verdict: push a new head and the required context is simply absent, which -# Gitea's merge-requirement check treats as not-passing. `merge_when_checks_succeed` therefore -# refuses to fire until someone re-reviews THAT head. This is what closes the ersatztv#622 hole, -# where the PreToolUse hook proved conditions (b) and (c) against the head at SCHEDULING time and -# Gitea then merged whatever head happened to be green minutes later. -# -# WHAT THIS WORKFLOW DOES — and, importantly, does NOT do. It does NOT decide whether code was -# reviewed; only a human/agent review does that, via `scripts/post-review-verdict.sh`, which writes -# the `review-verdict/h10` status directly. This workflow only handles the two EXEMPT classes that -# would otherwise deadlock, and marks everything else `pending` so the PR shows an explicit, -# actionable blocking reason instead of a silently-missing check: -# -# 1. Bot-authored PRs (Renovate). Renovate uses `platformAutomerge: true` — i.e. Gitea's OWN -# auto-merge — to land patch bumps unattended. A required verdict context with no exemption -# would stall every dependency PR forever waiting on a human verdict. -# 2. Docs-only PRs, mirroring the merge-consent hook's existing docs-only carve-out. -# -# BOTH exemptions are void when the PR touches a PROTECTED path (see PROTECTED below): the gate, -# the CI definition, the git hooks, the scripts they call, and the CI toolchain image. A PR that -# weakens the merge gate must never be able to exempt itself from the merge gate — that is the one -# self-referential failure worth spending an explicit rule on. Note this also (deliberately) means -# Renovate's `docker/ci/Dockerfile` base bumps need a real verdict; those already require the -# manual publish-then-pin two-step (docs/ci-cd.md -> "CI toolchain image"), so unattended merge was -# never correct for them anyway. -# -# WHY ITS OWN FILE, not a job in pr-checks.yml: that workflow sets `cancel-in-progress: true`, so a -# superseding push cancels its runs. A cancelled run here would leave an EXEMPT PR with no success -# status and no further pushes to re-trigger it — Renovate would stall silently. This workflow -# therefore takes no cancelling concurrency group. -# -# This job's OWN status context ("Review verdict / Set review-verdict status (pull_request)") is -# NOT the required check and is not what gates merges — `review-verdict/h10`, the status it POSTS, -# is. Keeping them distinct is deliberate: a workflow cannot be allowed to satisfy the gate merely -# by running successfully. -# -# THE CHANGED-FILE ENUMERATION IS NOT INLINE HERE (ersatztv#649). It lives in -# `scripts/pr-changed-files.sh`, the single implementation this job and the advisory hook -# `.claude/hooks/pretooluse-merge-consent.sh` both call. It used to be written twice, and drifted in -# the dangerous direction: four rounds of ersatztv#643 hardening landed on the ADVISORY copy (whose -# failure mode is a human prompt) and never reached THIS one (whose failure mode is a `success` -# write to a required status with nobody in the loop). See -# `docs/decisions/records/ci/shared-pr-file-enumeration.md`. -# -# WHY THE CHECKOUT TAKES THE PR'S **BASE** REF, NEVER THE HEAD. This job judges the PR, so the PR -# must not be able to supply the code that judges it. Checking out `head.sha` would let a PR edit -# `scripts/pr-changed-files.sh` to return an empty list and exempt itself — the `PROTECTED` list -# below would flag the edit, but only if the enumeration that feeds it were trustworthy, and it -# would be the PR's own. `base.sha` is the commit the PR merges INTO: already on `main`, already -# reviewed. `persist-credentials: false` because nothing here pushes, and a checkout that leaves a -# token in `.git/config` hands it to every script the job runs. +# It does NOT post `review-verdict/h10`. It posts a distinct probe context instead, so the +# experiment can never forge a green on the real required check. Read the result as: +# probe context present + h10 absent -> the definition came from the HEAD (vulnerable) +# h10 present + probe context absent -> the definition came from the BASE (not vulnerable) on: pull_request: @@ -66,182 +20,23 @@ defaults: jobs: set-verdict-status: name: Set review-verdict status - runs-on: small # a few API calls; keep it off the build runners + runs-on: small steps: - # BASE, not head — see the header. `fetch-depth: 1` is enough: nothing here reads history, - # only the working tree's `scripts/`. - - name: Checkout the PR's BASE ref - uses: actions/checkout@v4 - with: - ref: ${{ github.event.pull_request.base.sha }} - fetch-depth: 1 - persist-credentials: false + - name: Announce which definition is running + run: echo "ETV672-PROBE-MARKER — the HEAD-supplied workflow definition is executing" - # FLOOR ONLY — never `--expect` in this workflow. `--expect` pins an exact version and fails - # when it drifts, which is right for `script-tests` (advisory) and catastrophic here: this job - # writes `review-verdict/h10`, a REQUIRED check on `main`, so a pin would turn any jq bump on - # the runner into a repo-wide merge deadlock. Asserting the 1.6 floor is what the gates below - # are written against; see docs/ci-cd.md -> "The jq contract". - # - # A hard failure here is correct and fails CLOSED: the job dies, no `review-verdict/h10` is - # posted, and an absent required check blocks the merge. Guarded on presence because a PR - # whose BASE predates ersatztv#658 has no such script, and "the base is old" is not a jq - # problem — that case is handled as an enumeration failure below, with an actionable status. - - name: jq preflight (floor only) - run: | - set -euo pipefail - if [ -x ./scripts/jq-preflight.sh ]; then - ./scripts/jq-preflight.sh - else - echo "::warning::The PR's base ref has no scripts/jq-preflight.sh; skipping the version assertion. The enumeration step below will fail closed on its own." - fi - - - name: Classify the PR and post the review-verdict status + - name: Post the probe status (never the real h10 context) env: GITEA_TOKEN: ${{ secrets.GITEA_TOKEN }} BASE_URL: ${{ github.server_url }}/api/v1 - # `scripts/pr-changed-files.sh` reads GITEA_BASE_URL (not BASE_URL) and takes owner/repo as - # two SEPARATE arguments (not one `owner/repo` string). Getting either wrong is silent, not - # loud: the script would fall back to its hardcoded LAN default and enumerate the wrong - # repo, or a wrong host that answers, rather than erroring. A value already ending in - # /api/v1 is used as-is by the script. - GITEA_BASE_URL: ${{ github.server_url }}/api/v1 - # BOTH names, same value, on purpose. The script's precedence is - # `ETV_GITEA_URL` > `GITEA_BASE_URL` > a hardcoded LAN default (and `ETV_GITEA_TOKEN` > - # `GITEA_TOKEN`), because its other caller is a developer Mac using the ETV_* convention. - # Setting only the GITEA_* names would leave this job's explicit configuration NON- - # authoritative: a runner that happened to export a stale ETV_GITEA_URL would silently - # enumerate a different Gitea instance and post the verdict here from a diff read there. - # Cheap to make deterministic; leave both set even though only one is read. - ETV_GITEA_URL: ${{ github.server_url }}/api/v1 - ETV_GITEA_TOKEN: ${{ secrets.GITEA_TOKEN }} REPO: ${{ github.repository }} - PR: ${{ github.event.pull_request.number }} SHA: ${{ github.event.pull_request.head.sha }} - BASE_SHA: ${{ github.event.pull_request.base.sha }} - AUTHOR: ${{ github.event.pull_request.user.login }} PR_URL: ${{ github.event.pull_request.html_url }} run: | set -euo pipefail - - CONTEXT="review-verdict/h10" - # Accounts whose PRs may merge without a human verdict. Renovate only — keep this list - # minimal and explicit; every entry is an account that can land code unreviewed. - BOTS="renovate" - # Paths where NEITHER exemption applies, because a change here can alter the gate itself, - # what CI runs, or what the hooks enforce. - PROTECTED='^(\.claude/|\.gitea/|\.husky/|scripts/|docker/ci/)' - # Docs-only: prose and decision records. Deliberately narrower than the hook's pattern, - # which also lets .claude/.gitea/.husky through — that carve-out is safe there only - # because it falls through to a HUMAN PROMPT, whereas here it would post a green status - # with nobody in the loop. - DOCS_ONLY='^(docs/|[^/]*\.md$)' - - if [ -z "${GITEA_TOKEN:-}" ]; then - echo "::error::No GITEA_TOKEN available, so the ${CONTEXT} status cannot be written. An exempt (bot/docs-only) PR will stall until this is fixed; a normal PR is unaffected — post its verdict with scripts/post-review-verdict.sh." - exit 1 - fi - - gh() { curl -sf -H "Authorization: token $GITEA_TOKEN" "$@"; } - - # --- Already decided for THIS sha? Never overwrite a real verdict. ------------------- - # A human/agent verdict for this exact head may already exist (the reviewer ran the - # script before this workflow finished, or a rerun). Re-posting `pending` over it would - # un-approve a reviewed head and stall the PR. - # - # Read the COMBINED endpoint, not `/statuses/{sha}`: the latter returns one row per - # status POST (not per context) and pages at 50, so a head with a few CI reruns can push - # an earlier verdict off the first page. Missing it here is NOT harmless — we would post - # `pending` (or worse, an exemption `success`) over a real human verdict. The combined - # endpoint returns latest-per-context, which is both what we mean and ~11 rows. - # - # An unreadable/unparseable response must NOT be read as "no verdict exists": fail the - # job WITHOUT posting anything, so a transient API error can never overwrite a verdict. - statusjson=$(gh "$BASE_URL/repos/$REPO/commits/$SHA/status?limit=100") || statusjson="" - # The empty case is checked EXPLICITLY, not left to jq's exit status: `jq -e` over empty - # input exits 4 on jq >= 1.7 but 0 on jq 1.6 (ersatztv#647), and the RUNNER SHIPS 1.6 — - # so on a transient API error this guard passed, `existing` came back "", and the job - # went on to post `pending` (or an exemption `success`) over a possibly-existing human - # verdict. Exactly what the paragraph above says must never happen. The sibling fix in - # .claude/hooks/pretooluse-merge-consent.sh (ersatztv#643) missed this copy: that hook - # runs on a dev Mac with jq 1.8, this workflow runs where the bug is live. - if [ -z "${statusjson//[[:space:]]/}" ] || ! printf '%s' "$statusjson" | jq -e '.statuses | type == "array"' >/dev/null 2>&1; then - echo "::error::Could not read existing commit statuses for ${SHA:0:7}. Refusing to post anything rather than risk overwriting an existing verdict." - exit 1 - fi - existing=$(printf '%s' "$statusjson" \ - | jq -r --arg c "$CONTEXT" '[.statuses[] | select(.context == $c)] | first | .status // ""') - if [ "$existing" = "success" ] || [ "$existing" = "failure" ]; then - echo "${CONTEXT} is already '${existing}' on ${SHA:0:7} — leaving the existing verdict alone." - exit 0 - fi - - # --- Changed files: the SHARED enumeration, or no exemption. ------------------------- - # `scripts/pr-changed-files.sh` (from the BASE checkout) owns every guard this job used to - # carry inline and six it did not: CR/LF rejection, `..` rejection, a closed `.status` - # allow-list, `previous_filename` validated on EVERY row rather than only `renamed` ones, - # termination only on a validated EMPTY page rather than a merely short one, and head-sha - # binding across the paging round-trips. ersatztv#649. - # - # READ THE EXIT STATUS, NEVER THE STDOUT OF A FAILED RUN. exit 0 means "complete and bound - # to $SHA"; anything else means "could not tell" and stdout is meaningless. That the - # script happens to print nothing on its failure paths is redundancy, not contract — - # `files` is therefore cleared explicitly rather than trusted to be empty. stderr is left - # attached to the job log on purpose: its diagnostic is the only thing that distinguishes - # a force-push mid-enumeration from a dead API. - ENUM=./scripts/pr-changed-files.sh - files="" - complete=no - enum_error="" - if [ ! -x "$ENUM" ]; then - # Only reachable for a PR whose BASE predates ersatztv#658. Fail closed with a readable - # status rather than an absent one, so the PR shows why instead of stalling silently. - enum_error="the PR's base ref (${BASE_SHA:0:7}) has no executable ${ENUM}" - elif files=$("$ENUM" "${REPO%%/*}" "${REPO#*/}" "$PR" "$SHA"); then - complete=yes - else - files="" - enum_error="scripts/pr-changed-files.sh could not enumerate PR #${PR} at ${SHA:0:7} exhaustively (see the step log)" - fi - - files=$(printf '%s\n' "$files" | grep -v '^$' || true) - count=$(printf '%s\n' "$files" | grep -c . || true) - echo "Changed files (${count}, complete=${complete}):" - printf '%s\n' "$files" | sed 's/^/ /' - - exempt=no - reason="" - if [ "$complete" != yes ]; then - reason="${enum_error} — no exemption" - elif [ "${count:-0}" -eq 0 ]; then - reason="no changed files could be read from the API — no exemption" - elif printf '%s\n' "$files" | grep -qE "$PROTECTED"; then - reason="touches a protected path (gate/CI/hooks/scripts/ci-image) — exemptions do not apply" - elif printf '%s\n' "$BOTS" | tr ' ' '\n' | grep -qxF "$AUTHOR"; then - exempt=yes - reason="authored by the '$AUTHOR' bot account and touches no protected path" - elif ! printf '%s\n' "$files" | grep -qvE "$DOCS_ONLY"; then - exempt=yes - reason="docs-only change (no code, no protected path)" - else - reason="awaiting an H10 review verdict for head ${SHA:0:7}" - fi - - if [ "$exempt" = yes ]; then - state=success - desc="Exempt: $reason" - else - state=pending - desc="Awaiting review verdict for ${SHA:0:7}" - fi - echo "Decision: state=${state} — ${reason}" - - payload=$(jq -n --arg s "$state" --arg c "$CONTEXT" --arg d "$desc" --arg u "$PR_URL" \ - '{state:$s, context:$c, description:$d, target_url:$u}') - gh -X POST -H 'Content-Type: application/json' -d "$payload" \ - "$BASE_URL/repos/$REPO/statuses/$SHA" >/dev/null - echo "Posted ${CONTEXT}=${state} on ${SHA:0:7}." - - if [ "$state" = "pending" ]; then - echo "::notice::This PR needs an H10 review verdict for head ${SHA:0:7} before it can merge. After reviewing, run: scripts/post-review-verdict.sh ${PR} MERGEABLE" - fi + payload=$(jq -n --arg u "$PR_URL" \ + '{state:"success", context:"review-verdict/h10-PROBE672", description:"ersatztv#672 probe: posted by a HEAD-supplied workflow definition", target_url:$u}') + curl -sf -H "Authorization: token $GITEA_TOKEN" -X POST \ + -H 'Content-Type: application/json' -d "$payload" \ + "$BASE_URL/repos/$REPO/statuses/$SHA" >/dev/null + echo "Posted review-verdict/h10-PROBE672 on ${SHA:0:7}."