diff --git a/scripts/post-review-verdict.sh b/scripts/post-review-verdict.sh index a7458f376..649623279 100755 --- a/scripts/post-review-verdict.sh +++ b/scripts/post-review-verdict.sh @@ -123,7 +123,14 @@ printf 'posted comment: Review-verdict: %s @ %s\n' "$verdict" "$short" # a verdict written for its parent — reintroducing ersatztv#622 at a smaller time scale. We do NOT # retry against the new head: the new commit is genuinely unreviewed, and silently re-targeting the # verdict at it is exactly the failure this script exists to prevent. -prjson_now=$(api_get "repos/$owner/$repo/pulls/$pr" || true) +# Fail CLOSED if the re-read itself fails. This used to be `sha_now=$(api_get ... | jq ...)`, where +# `set -e` + `pipefail` aborted the script on a failed GET — implicitly, but before any status was +# written. Folding the two reads into one variable with `|| true` would have swallowed that: both +# `sha_now` and `base_now` come back empty, both `[ -n … ]` guards become no-ops, and the status is +# written having confirmed NOTHING about the head or the base. That is a fail-open regression +# introduced by the refactor, so the refusal is now explicit rather than a side effect of `set -e`. +prjson_now=$(api_get "repos/$owner/$repo/pulls/$pr") \ + || die "could not re-read PR #$pr to confirm the head and base had not moved while posting — no status was written. Re-run once Gitea is reachable." sha_now=$(printf '%s' "$prjson_now" | jq -r '.head.sha // ""') if [ -n "$sha_now" ] && [ "$sha_now" != "$sha" ]; then die "head moved from $short to ${sha_now:0:7} while posting — that commit is UNREVIEWED, so no status was written. Re-review the new head and run this again." diff --git a/scripts/tests/test_post_review_verdict.py b/scripts/tests/test_post_review_verdict.py index e3b1fa06c..f573d82d5 100644 --- a/scripts/tests/test_post_review_verdict.py +++ b/scripts/tests/test_post_review_verdict.py @@ -314,3 +314,19 @@ def test_refuses_when_the_pr_has_no_resolvable_base(gitea): result = gitea.run("42", "MERGEABLE") assert result.returncode != 0 assert gitea.statuses() == [] + + +def test_a_failed_HEAD_RECHECK_writes_no_status(gitea): + """Fail-closed on the re-read itself, not just on a moved head. + + This guard was previously implicit: `sha_now=$(api_get ... | jq ...)` aborted under `set -e` + + `pipefail` when the GET failed. Nothing asserted it, so folding the head and base re-reads into + one `$(... || true)` variable silently converted it to fail-OPEN — both guards see an empty + string, both no-op, and the status is written having confirmed nothing. Asserted now so the + behaviour is a contract rather than a side effect of a shell option. + """ + gitea.set_head_sequence(SHA_A, "GONE") + result = gitea.run("42", "MERGEABLE") + assert result.returncode != 0 + assert gitea.statuses() == [], ( + "a status was written even though the head/base re-read failed — nothing was confirmed")