fix(git): pr-review.sh — surface the provider's stated reason, drop the hardcoded #865 attribution (#1006)

Co-authored-by: mos-dt-0 <[email protected]>
2026-07-31 10:52:58 +00:00
parent a4280b9c98
commit 826a8b3b26
2 changed files with 128 additions and 7 deletions
@@ -58,6 +58,9 @@ CREDENTIALS_FILE="$WORK_DIR/credentials.json"
 # A dedicated scratch dir the wrapper is pointed at via TMPDIR, so the leak
 # check can assert every POST/GET body + metadata temp file is cleaned up.
 TMP_SCRATCH="$WORK_DIR/scratch"
+# Sandboxed HOME so nothing under the real $HOME (notably the per-slot Gitea token
+# store at ~/.config/mosaic/secrets/gitea-tokens/) is reachable from the wrapper.
+HOME_DIR="$WORK_DIR/home"

 cleanup() {
    rm -rf "$WORK_DIR"
@@ -78,9 +81,18 @@ OVERRIDE_TOKEN="override-token-placeholder"
 CROSS_HOST_LOGIN="foreign-host-reviewer"
 CROSS_HOST_TOKEN="cross-host-token-placeholder"

-mkdir -p "$REPO_DIR" "$BIN_DIR" "$XDG_DIR" "$STATE_DIR" "$TMP_SCRATCH"
+mkdir -p "$REPO_DIR" "$BIN_DIR" "$XDG_DIR" "$STATE_DIR" "$TMP_SCRATCH" "$HOME_DIR"
 git -C "$REPO_DIR" init -q
 git -C "$REPO_DIR" remote add origin https://git.mosaicstack.dev/mosaicstack/stack.git
+# HERMETICITY: get_gitea_token() step 0 resolves a per-agent identity from
+# `git config --get mosaic.gitIdentity`, which on a provisioned agent seat is set
+# GLOBALLY and therefore leaks into this fresh repo. It then reads a REAL per-slot
+# token from $HOME and returns it WITHOUT ever consulting MOSAIC_CREDENTIALS_FILE,
+# so the fixture credentials below are silently ignored and the suite runs against
+# production credentials. An empty repo-local value shadows the global one and reads
+# back as empty at rc=0, restoring the shared-credential path this suite intends to
+# exercise. Paired with the sandboxed HOME in run_review().
+git -C "$REPO_DIR" config mosaic.gitIdentity ""

 # tea config: the override login carries its own token here. The default login
 # name ("mosaicstack") is deliberately absent, so the no-override default path
@@ -266,6 +278,24 @@ submitted = json.loads(os.environ["PR_REVIEW_PAYLOAD"])
 with open(state_path, encoding="utf-8") as handle:
    reviews = json.load(handle)

+# review-refused-422 (#1004): the server REFUSES the submit outright with a
+# definite, correct, machine-readable reason in the body — the shape Gitea
+# returns when the acting credential authored the PR. Nothing is created. The
+# wrapper must surface what the server said and must NOT relabel this as the
+# #865 silent-no-op defect class, which is precisely what it is not.
+if mode == "review-refused-422":
+    print("422")
+    print(json.dumps({"message": "Cannot approve your own pull request"}))
+    raise SystemExit(0)
+
+# review-refused-html (#1004): a non-JSON error body, as a fronting proxy or
+# gateway emits. The detail extraction must degrade to the first raw line rather
+# than silently dropping the only explanation available.
+if mode == "review-refused-html":
+    print("502")
+    print("<html><head><title>502 Bad Gateway</title></head>\n<body>nginx</body></html>")
+    raise SystemExit(0)
+
 # no-op-concurrent-review: the wrapper's own submit is SUPPRESSED (200, no
 # created object) even though a concurrent same-identity, same-state review at
 # the same head already exists. Nothing is persisted; no created id to verify.
@@ -517,6 +547,8 @@ run_review() {
        cd "$REPO_DIR"
        PATH="$BIN_DIR:$PATH" \
        TMPDIR="$TMP_SCRATCH" \
+        HOME="$HOME_DIR" \
+        MOSAIC_GIT_IDENTITY="" \
        XDG_CONFIG_HOME="$XDG_DIR" \
        MOSAIC_CREDENTIALS_FILE="$CREDENTIALS_FILE" \
        PR_REVIEW_TEA_LOG="$TEA_LOG" \
@@ -940,4 +972,44 @@ if grep -q 'Approved and verified' "$OUTPUT_FILE"; then
 fi
 assert_no_temp_leak "review-body-null"

+# Case 19 (#1004): an outright server REFUSAL must report the provider's own
+# reason and must NOT be relabelled as the #865 silent-no-op defect class. The
+# old arm hardcoded "(#865: no durable review created)" for EVERY non-2xx, so a
+# 422/403/404 — all of them definite, correct refusals the server explained in
+# the discarded body — arrived at the caller wearing the name of the one defect
+# they are not. That misdirection is what makes an operator re-issue the request
+# by hand against the live object to find out what actually happened.
+if run_review review-refused-422 approve; then
+    echo "FAIL: approve reported success when the server refused the submit" >&2
+    cat "$OUTPUT_FILE" >&2
+    exit 1
+fi
+grep -q 'HTTP 422' "$OUTPUT_FILE"
+if ! grep -q 'Cannot approve your own pull request' "$OUTPUT_FILE"; then
+    echo "FAIL: the provider's stated reason was discarded (#1004)" >&2
+    cat "$OUTPUT_FILE" >&2
+    exit 1
+fi
+if grep -q '#865: no durable review created' "$OUTPUT_FILE"; then
+    echo "FAIL: a server refusal was misattributed to the #865 defect class (#1004)" >&2
+    cat "$OUTPUT_FILE" >&2
+    exit 1
+fi
+assert_no_temp_leak "review-refused-422"
+
+# Case 20 (#1004): a non-JSON error body (a fronting proxy's HTML page) must
+# still yield something the caller can act on, rather than a bare status code.
+if run_review review-refused-html approve; then
+    echo "FAIL: approve reported success on a 502" >&2
+    cat "$OUTPUT_FILE" >&2
+    exit 1
+fi
+grep -q 'HTTP 502' "$OUTPUT_FILE"
+if ! grep -q '502 Bad Gateway' "$OUTPUT_FILE"; then
+    echo "FAIL: a non-JSON error body was dropped instead of degrading to its first line (#1004)" >&2
+    cat "$OUTPUT_FILE" >&2
+    exit 1
+fi
+assert_no_temp_leak "review-refused-html"
+
 echo "pr-review.sh REST review + comment create/read-back regression passed"