fix(632): fail closed when the head/base re-read itself fails

Self-review of the previous commit. Folding the head and base re-reads into one `prjson_now=$(api_get ... || true)` swallowed a guard that used to be implicit: the old `sha_now=$(api_get ... | jq ...)` aborted under `set -e` + `pipefail` when the GET failed, before any status was written. With `|| true`, both `sha_now` and `base_now` come back empty, both `[ -n ... ]` guards no-op, and the status is written having confirmed nothing about either the head or the base — a fail-open regression introduced by the refactor itself. Confirmed the old behaviour empirically rather than by reading it: a failed piped command substitution under `set -euo pipefail` exits with curl's status. The refusal is now explicit, and pinned by a test — nothing asserted it before, which is exactly why the refactor could drop it silently. Mutation-verified: restoring `|| true` reddens that test alone. Refs #632
2026-07-26 23:16:35 +02:00
parent 00e623c066
commit f0f8708a6e
2 changed files with 24 additions and 1 deletions
@@ -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."
@@ -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")