From c794a48462c6c5471c4f9e4b3147762485b84808 Mon Sep 17 00:00:00 2001 From: Timothy Date: Mon, 27 Jul 2026 18:30:44 +0200 Subject: [PATCH] docs(649): narrow the base-ref headline to what the checkout actually binds MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The record's bold sentence said the base-ref checkout means the workflow "cannot be rewritten by that same PR to weaken its own judgment". That is true of the SCRIPTS the job executes and false of the workflow itself: Gitea resolves a `pull_request` workflow definition from the PR's own head, so a PR editing `review-verdict.yml` runs its rewritten copy — which can delete the checkout outright, or just post `review-verdict/h10=success` and stop. Branch protection requires the context, not an author, and carries `required_approvals: 0` (#672). The scoping already existed further down, under "What is deliberately NOT claimed". That is not good enough for this particular sentence: it is bold, it is the paragraph a reader resolving this record from the catalog lands on, and someone who stops there leaves with the opposite of the truth. A caveat only works if it is reached. So the headline now says what the checkout binds (the scripts, from the already-reviewed base) and a following paragraph states the head-resolution hole directly, with the superseded claim quoted so a reader who remembers it can see it was retracted rather than wonder whether two records disagree. The later paragraph loses its duplicated opener and points at it instead. Docs only; no behaviour change. Same failure class the rest of #649 kept turning up — a claim stronger than the code — reached this time through prose rather than a test. Refs #649, #672 Decisions-Edit: yes --- .../records/ci/shared-pr-file-enumeration.md | 22 ++++++++++++++----- 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/docs/decisions/records/ci/shared-pr-file-enumeration.md b/docs/decisions/records/ci/shared-pr-file-enumeration.md index 0903fbc22..196627dcc 100644 --- a/docs/decisions/records/ci/shared-pr-file-enumeration.md +++ b/docs/decisions/records/ci/shared-pr-file-enumeration.md @@ -74,13 +74,24 @@ in each caller: none) or a local diff, not a tighter check here; the guarantee is stated narrowly rather than left to read as complete. -**Base-ref checkout — now a property of the tree.** `review-verdict.yml` checks out the PR's BASE -ref (`ref: ${{ github.event.pull_request.base.sha }}`, `persist-credentials: false`), never the -head, so the workflow that judges a PR cannot be rewritten by that same PR to weaken its own -judgment. The checkout and its `ref` are the security-relevant parts — a bare `run:` calling the +**Base-ref checkout — binds the SCRIPTS to the base, not the workflow itself.** `review-verdict.yml` +checks out the PR's BASE ref (`ref: ${{ github.event.pull_request.base.sha }}`, +`persist-credentials: false`), never the head, so the *scripts the job executes* — above all +`scripts/pr-changed-files.sh` — come from the already-reviewed base rather than from the PR under +judgment. The checkout and its `ref` are the security-relevant parts: a bare `run:` calling the script would not have been sufficient, since the script would then have come from wherever the runner happened to be. +**It does NOT mean a PR cannot rewrite the gate that judges it (#672).** Gitea resolves a +`pull_request` workflow *definition* from the PR's own head, so a PR editing `review-verdict.yml` +runs its own rewritten copy — which can delete this checkout, or simply post +`review-verdict/h10=success` for its head sha and stop. Branch protection does not close that: it +requires the *context*, not an author, and carries `required_approvals: 0`. An earlier revision of +this paragraph said the workflow "cannot be rewritten by that same PR to weaken its own judgment", +which is true of the scripts and false of the workflow — and stated in the one sentence a reader +resolving this record from the catalog is most likely to stop at. Contained today only by push +access being required; tracked in #672. + An earlier revision of this record stated the requirement in the future tense, because the wiring was staged over two PRs: the workflow runs the BASE version of the gate, and until the shared script existed on `main` a wired workflow would have exited 127 on its own PR and blocked the merge gate @@ -89,8 +100,7 @@ through the combined status. That staging is complete. Both halves are asserted matching it — `head.sha` for `base.sha` is a nine-character diff, and a text-level check would still pass if a second checkout step took the head afterwards and won. -**What is deliberately NOT claimed.** The base-ref checkout closes the self-rewrite route through -the *scripts*; it does not make the workflow immune to everything a PR controls. The `PROTECTED` +**What is deliberately NOT claimed.** Beyond the head-resolution hole above, the `PROTECTED` path list remains the guard that stops a bot-authored PR from editing the gate and exempting itself, and mutation testing was what established that `PROTECTED` is load-bearing only on the BOT path — it and `DOCS_ONLY` are disjoint patterns, so on the docs-only path that clause can never fire. A -- 2.47.3