From 811d02951e76cad30ab08d0192ab608221063c70 Mon Sep 17 00:00:00 2001 From: Jason Woltje Date: Fri, 31 Jul 2026 05:37:10 -0500 Subject: [PATCH] =?UTF-8?q?fix(git):=20pr-review.sh=20=E2=80=94=20surface?= =?UTF-8?q?=20the=20provider's=20stated=20reason,=20drop=20the=20hardcoded?= =?UTF-8?q?=20#865=20attribution=20(closes=20#1004)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All six HTTP-status error arms captured the provider's response body in a mktemp file and then discarded it unread, so a caller got a status code and no cause. The review-submit arm additionally hardcoded "(#865: no durable review created)" onto EVERY non-2xx — including 401/403/404/409/422, where the server had given a definite, correct, machine-readable refusal that is categorically not #865 (the silent-no-op defect). Measured case: `pr-review.sh -n 1001 -a approve` reported Error: Gitea review submit failed with HTTP 422 (#865: no durable review created) when the real cause was that the acting credential authored the PR and Gitea correctly refuses self-approval. Gitea said so plainly in the discarded body. This matters beyond error aesthetics: withholding the cause pushes the caller toward re-issuing the request by hand to see what the server says, and a diagnostic that uses the real verb against the real object is not a diagnostic, it is the operation. Changes: - New gitea_error_detail() renders the provider's own explanation (JSON message/error/errors, else the first body line), truncated to 300 chars. It always returns 0 so it can never abort a caller under `set -e`, and prints nothing when there is nothing to add. - All six arms append it. The review-submit arm no longer cites #865; that reference now appears only on the check that actually diagnoses it — a 2xx yielding no id, immediately below. No control flow changes: every arm still returns 1 and still fails closed. Test hermeticity (required, or the new cases cannot run on any agent seat): get_gitea_token()'s per-agent identity step reads `git config --get mosaic.gitIdentity`, which on a provisioned seat is set GLOBALLY and leaks into the harness's fresh `git init` repo. It then returns a REAL per-slot token from $HOME without ever consulting MOSAIC_CREDENTIALS_FILE, so the fixture placeholder was silently ignored and the suite ran against production credentials, failing 401. The harness now pins an empty repo-local mosaic.gitIdentity (an empty local value shadows the global and reads back empty at rc=0) and runs under a sandboxed HOME. Filed separately: the per-slot token store has no override hook, so no harness can sandbox it. Verification matrix (all three run locally): - base suite + hermeticity fix, unmodified wrapper -> PASS (fix is sufficient and breaks no existing case) - new cases 19/20, unmodified wrapper -> FAIL at case 19, reproducing the exact misattribution above (negative control) - new cases 19/20, fixed wrapper -> PASS Cases 19 (422 JSON refusal) and 20 (502 HTML body) assert the status is present, the provider's reason is present, and "#865: no durable review created" is ABSENT. shellcheck is not installed on this host, so the lint gate is unverified locally. Co-authored-by: mos-dt-0 --- .../mosaic/framework/tools/git/pr-review.sh | 61 +++++++++++++-- .../tools/git/test-pr-review-gitea-comment.sh | 74 ++++++++++++++++++- 2 files changed, 128 insertions(+), 7 deletions(-) diff --git a/packages/mosaic/framework/tools/git/pr-review.sh b/packages/mosaic/framework/tools/git/pr-review.sh index ba296d2e..ce10fe98 100755 --- a/packages/mosaic/framework/tools/git/pr-review.sh +++ b/packages/mosaic/framework/tools/git/pr-review.sh @@ -109,6 +109,55 @@ else detect_platform >/dev/null fi +# Render the provider's own explanation for a failed request, for appending to +# an error message (#1004). Every HTTP arm in this file already has the response +# body on disk; without this it was discarded unread at exactly the moment the +# caller needed it, which pushes an operator toward re-issuing the request by +# hand to find out what the server said. Gitea returns {"message": "..."} on a +# refusal; anything unparseable falls back to a truncated raw first line so a +# proxy's HTML error page still says something. Prints "" when there is nothing +# to add, so callers can interpolate unconditionally. +# +# Args: $1 = path to the response body file. +gitea_error_detail() { + local body_file="$1" + [[ -s "$body_file" ]] || return 0 + python3 - "$body_file" <<'PY' 2>/dev/null || true +import json +import sys + +LIMIT = 300 +try: + with open(sys.argv[1], encoding="utf-8", errors="replace") as response: + raw = response.read().strip() +except OSError: + raise SystemExit(0) +if not raw: + raise SystemExit(0) +detail = "" +try: + parsed = json.loads(raw) + if isinstance(parsed, dict): + for key in ("message", "error", "errors"): + value = parsed.get(key) + if isinstance(value, str) and value.strip(): + detail = value.strip() + break + if isinstance(value, list) and value: + detail = "; ".join(str(item) for item in value).strip() + break +except ValueError: + pass +if not detail: + detail = raw.splitlines()[0].strip() +if not detail: + raise SystemExit(0) +if len(detail) > LIMIT: + detail = detail[:LIMIT] + "..." +print(f" — provider said: {detail}") +PY +} + # Post a comment to a Gitea PR (PR comments ARE issue comments) via the # supported REST API and verify it against a PROVIDER-RETURNED created id. The # write is a direct POST that returns the created comment object, so we learn @@ -150,7 +199,7 @@ print(json.dumps({"body": os.environ["COMMENT_BODY"]})) return 1 fi if [[ "$write_status" != "201" ]]; then - echo "Error: Gitea comment write failed with HTTP $write_status" >&2 + echo "Error: Gitea comment write failed with HTTP $write_status$(gitea_error_detail "$write_file")" >&2 return 1 fi @@ -179,7 +228,7 @@ PY return 1 fi if [[ "$readback_status" != "200" ]]; then - echo "Error: Gitea comment read-back failed with HTTP $readback_status" >&2 + echo "Error: Gitea comment read-back failed with HTTP $readback_status$(gitea_error_detail "$readback_file")" >&2 return 1 fi @@ -370,7 +419,7 @@ gitea_authenticated_login() { return 1 fi if [[ "$status" != "200" ]]; then - echo "Error: Gitea authenticated-identity read failed with HTTP $status" >&2 + echo "Error: Gitea authenticated-identity read failed with HTTP $status$(gitea_error_detail "$response_file")" >&2 return 1 fi @@ -407,7 +456,7 @@ gitea_read_pr_head_into() { return 1 fi if [[ "$status" != "200" ]]; then - echo "Error: Gitea PR head read failed with HTTP $status" >&2 + echo "Error: Gitea PR head read failed with HTTP $status$(gitea_error_detail "$pr_file")" >&2 return 1 fi python3 - "$pr_file" <<'PY' @@ -497,7 +546,7 @@ print(json.dumps({ fi # Gitea returns 200 (occasionally 201) with the created review object. if [[ "$write_status" != "200" && "$write_status" != "201" ]]; then - echo "Error: Gitea review submit failed with HTTP $write_status (#865: no durable review created)" >&2 + echo "Error: Gitea review submit failed with HTTP $write_status$(gitea_error_detail "$write_file")" >&2 return 1 fi @@ -526,7 +575,7 @@ PY return 1 fi if [[ "$readback_status" != "200" ]]; then - echo "Error: Gitea review read-back failed with HTTP $readback_status" >&2 + echo "Error: Gitea review read-back failed with HTTP $readback_status$(gitea_error_detail "$readback_file")" >&2 return 1 fi diff --git a/packages/mosaic/framework/tools/git/test-pr-review-gitea-comment.sh b/packages/mosaic/framework/tools/git/test-pr-review-gitea-comment.sh index cf1de2fb..588ca0bc 100644 --- a/packages/mosaic/framework/tools/git/test-pr-review-gitea-comment.sh +++ b/packages/mosaic/framework/tools/git/test-pr-review-gitea-comment.sh @@ -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("502 Bad Gateway\nnginx") + 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" -- 2.54.0