diff --git a/packages/mosaic/framework/tools/git/pr-review.sh b/packages/mosaic/framework/tools/git/pr-review.sh index d3f2d582..ba296d2e 100755 --- a/packages/mosaic/framework/tools/git/pr-review.sh +++ b/packages/mosaic/framework/tools/git/pr-review.sh @@ -629,7 +629,14 @@ if [[ "$PLATFORM" == "github" ]]; then elif [[ "$PLATFORM" == "gitea" ]]; then case $ACTION in approve) - host=$(get_remote_host) + # Best-effort host for the tea-login GUESS only (gitea_resolve_api_for_login + # below re-derives the real host from HOST_OVERRIDE/remote independently and + # is authoritative). Prefer an explicit -H/--host; otherwise best-effort + # git-remote inference, tolerating its ABSENCE (a bare `get_remote_host` here + # under `set -e`, with no origin and no -H, previously killed the script + # SILENTLY — exit 1, zero output — even though -r/-H are exactly the flags + # that support running with no usable origin at all). + host="${HOST_OVERRIDE:-$(get_remote_host 2>/dev/null || true)}" # A --login override always wins. Otherwise name this host's login # only as a best effort: the login name merely selects a per-login # token, and gitea_resolve_api_for_login falls back to the host @@ -659,7 +666,14 @@ elif [[ "$PLATFORM" == "gitea" ]]; then echo "Error: Comment required for request-changes" exit 1 fi - host=$(get_remote_host) + # Best-effort host for the tea-login GUESS only (gitea_resolve_api_for_login + # below re-derives the real host from HOST_OVERRIDE/remote independently and + # is authoritative). Prefer an explicit -H/--host; otherwise best-effort + # git-remote inference, tolerating its ABSENCE (a bare `get_remote_host` here + # under `set -e`, with no origin and no -H, previously killed the script + # SILENTLY — exit 1, zero output — even though -r/-H are exactly the flags + # that support running with no usable origin at all). + host="${HOST_OVERRIDE:-$(get_remote_host 2>/dev/null || true)}" # A --login override always wins. Otherwise name this host's login # only as a best effort: the login name merely selects a per-login # token, and gitea_resolve_api_for_login falls back to the host @@ -683,7 +697,14 @@ elif [[ "$PLATFORM" == "gitea" ]]; then echo "Error: Comment required" exit 1 fi - host=$(get_remote_host) + # Best-effort host for the tea-login GUESS only (gitea_resolve_api_for_login + # below re-derives the real host from HOST_OVERRIDE/remote independently and + # is authoritative). Prefer an explicit -H/--host; otherwise best-effort + # git-remote inference, tolerating its ABSENCE (a bare `get_remote_host` here + # under `set -e`, with no origin and no -H, previously killed the script + # SILENTLY — exit 1, zero output — even though -r/-H are exactly the flags + # that support running with no usable origin at all). + host="${HOST_OVERRIDE:-$(get_remote_host 2>/dev/null || true)}" # A --login override always wins. Otherwise name this host's login # only as a best effort: the login name merely selects a per-login # token, and gitea_resolve_api_for_login falls back to the host diff --git a/packages/mosaic/framework/tools/git/test-pr-review-repo-host-override.sh b/packages/mosaic/framework/tools/git/test-pr-review-repo-host-override.sh index b6165d07..c4c69437 100755 --- a/packages/mosaic/framework/tools/git/test-pr-review-repo-host-override.sh +++ b/packages/mosaic/framework/tools/git/test-pr-review-repo-host-override.sh @@ -9,9 +9,23 @@ # #867). When -r is given, the resolved repo is preflighted (GET # .../repos/) BEFORE any write, so a wrong-host cross-wire surfaces as a # clear preflight error instead of an opaque write-404. Every Gitea curl call -# (preflight, write, read-back, /user) must carry a +# (preflight, write, read-back, /user, PR-head) must carry a # `User-Agent: mosaic-pr-review` header, since some Cloudflare-fronted Gitea # hosts intermittently reject curl's default User-Agent. +# +# Round 2 (post-review): the approve/request-changes/comment dispatch bodies +# each independently called a BARE `host=$(get_remote_host)` (only for a +# best-effort tea-login guess) that ignored -H/--host entirely and, under +# `set -e`, DIED SILENTLY (exit 1, zero stdout/stderr) whenever no git origin +# existed — exactly the case -r/-H exist to support. The "no git origin at +# all" fixture below must be a directory TRULY outside any .git ancestry (a +# subdirectory with no .git of its own still resolves `git remote get-url +# origin` UPWARD to an enclosing repo's origin — a real flaw in the first +# version of this fixture, since the test tree itself sits inside the +# mosaicstack/stack checkout). It is created via `mktemp -d` under the SYSTEM +# /tmp and additionally bounded with GIT_CEILING_DIRECTORIES, so `git rev-parse +# --show-toplevel`/`git remote get-url origin` cannot walk past it under any +# circumstance. set -euo pipefail @@ -24,9 +38,14 @@ UA_LOG="$WORK_DIR/ua.log" OUTPUT_FILE="$WORK_DIR/output.log" TMP_SCRATCH="$WORK_DIR/scratch" CREDENTIALS_FILE="$WORK_DIR/credentials.json" +# A TRUE no-git-origin fixture: created under the system /tmp (NOT under +# $WORK_DIR, which sits inside this very checkout's .git ancestry), so that +# `git remote get-url origin` run from inside it cannot resolve upward to the +# mosaicstack/stack checkout's own real origin. +NO_GIT_DIR="$(mktemp -d "${TMPDIR:-/tmp}/pr-review-no-origin.XXXXXX")" cleanup() { - rm -rf "$WORK_DIR" + rm -rf "$WORK_DIR" "$NO_GIT_DIR" } trap cleanup EXIT @@ -34,16 +53,17 @@ HOST_OVERRIDE_VAL="pr-review-override.test" REPO_OVERRIDE_VAL="acme/widgets" TOKEN_VAL="override-token-xyz" ACTING_LOGIN="override-bot" +HEAD_SHA_VAL="feedfacecafebabe0000000000000000000000" mkdir -p "$BIN_DIR" "$STATE_DIR" "$TMP_SCRATCH" -# tea stub: -r/--repo + -H/--host must never need tea at all (no per-host -# login list lookup is required to resolve these overrides). Any invocation is -# a hard failure so a regression that starts shelling out to tea is caught. +# tea stub: deterministic tea-absent behavior regardless of whether the host +# actually has a real `tea` binary on PATH. -r/--repo + -H/--host must not +# NEED a resolvable tea login to work — the wrapper falls back to the +# host-scoped GITEA_TOKEN credential when no tea login resolves. cat > "$BIN_DIR/tea" <<'SH' #!/usr/bin/env bash -echo "FAIL: pr-review.sh invoked tea ($*) while using -r/--repo + -H/--host overrides" >&2 -exit 91 +exit 1 SH chmod +x "$BIN_DIR/tea" @@ -56,6 +76,7 @@ set -euo pipefail output_file="" method="GET" +payload="" url="" config_file="" ua_seen="0" @@ -68,7 +89,7 @@ while [[ \$# -gt 0 ]]; do -K|--config) config_file="\$2"; shift 2 ;; -w) shift 2 ;; -X) shift 2 ;; - -d|--data) shift 2 ;; + -d|--data) payload="\$2"; shift 2 ;; -s|-S|-sS) shift ;; http://*|https://*) url="\$1"; shift ;; *) shift ;; @@ -100,6 +121,29 @@ case "\$url" in "https://$HOST_OVERRIDE_VAL/api/v1/user") write_response 200 "{\\"login\\":\\"$ACTING_LOGIN\\"}" ;; + "https://$HOST_OVERRIDE_VAL/api/v1/repos/$REPO_OVERRIDE_VAL/pulls/123") + write_response 200 "{\\"head\\":{\\"sha\\":\\"$HEAD_SHA_VAL\\"}}" + ;; + "https://$HOST_OVERRIDE_VAL/api/v1/repos/$REPO_OVERRIDE_VAL/pulls/123/reviews") + PR_REVIEW_PAYLOAD="\$payload" PR_REVIEW_ACTING="$ACTING_LOGIN" \ + python3 -c ' +import json, os +p = json.loads(os.environ["PR_REVIEW_PAYLOAD"]) +record = { + "id": 501, + "state": p.get("event"), + "commit_id": p.get("commit_id"), + "body": p.get("body"), + "user": {"login": os.environ["PR_REVIEW_ACTING"]}, +} +with open("$STATE_DIR/review.json", "w", encoding="utf-8") as fh: + json.dump(record, fh) +' + write_response 201 "\$(cat "$STATE_DIR/review.json")" + ;; + "https://$HOST_OVERRIDE_VAL/api/v1/repos/$REPO_OVERRIDE_VAL/pulls/123/reviews/501") + write_response 200 "\$(cat "$STATE_DIR/review.json")" + ;; "https://$HOST_OVERRIDE_VAL/api/v1/repos/$REPO_OVERRIDE_VAL/issues/123/comments") write_response 201 "{\\"id\\":456,\\"body\\":\\"durable-body\\",\\"user\\":{\\"login\\":\\"$ACTING_LOGIN\\"},\\"issue_url\\":\\"\\",\\"pull_request_url\\":\\"\${web_base}/\${repo}/pulls/123\\"}" ;; @@ -126,6 +170,7 @@ run() { MOSAIC_CREDENTIALS_FILE="$CREDENTIALS_FILE" \ GITEA_TOKEN="$TOKEN_VAL" \ GITEA_URL="https://$HOST_OVERRIDE_VAL" \ + GIT_CEILING_DIRECTORIES="$(dirname "$NO_GIT_DIR")" \ "$SCRIPT_DIR/pr-review.sh" "$@" ) > "$OUTPUT_FILE" 2>&1 } @@ -134,13 +179,34 @@ set_mode() { printf '%s' "$1" > "$STATE_DIR/mode" } +# A run must not silently die: exit 1 with a COMPLETELY EMPTY combined +# stdout+stderr is precisely the pre-fix signature of the dispatch-level bare +# `host=$(get_remote_host)` call dying under `set -e` when there was no git +# origin at all. Whatever the outcome, the wrapper must always emit SOME +# diagnostic (success line, or an "Error: ..." line). +assert_not_silently_dead() { + local context="$1" exit_code="$2" + if [[ "$exit_code" -ne 0 && ! -s "$OUTPUT_FILE" ]]; then + echo "FAIL: pr-review.sh died SILENTLY (exit $exit_code, zero output) in a true no-origin dir ($context)" >&2 + exit 1 + fi +} + +# Sanity-check the fixture itself BEFORE relying on it: from inside NO_GIT_DIR, +# `git remote get-url origin` must fail outright — proving this directory is +# truly outside any .git ancestry (the flaw this harness had before: a bare +# subdirectory of the checkout resolves `origin` UPWARD to the checkout's own +# real remote instead of failing). +if (cd "$NO_GIT_DIR" && GIT_CEILING_DIRECTORIES="$(dirname "$NO_GIT_DIR")" git remote get-url origin) 2>/dev/null; then + echo "FAIL: NO_GIT_DIR fixture is not actually outside any git ancestry — 'git remote get-url origin' resolved upward" >&2 + exit 1 +fi + # --- Case 1: -r/--repo and -H/--host are recognized flags (regression lock on # the previous "Unknown option: -r" / "Unknown option: -H" failure). Use a # bogus ACTION so the run fails cheaply, WITHOUT any network I/O, at the # platform-dispatch unknown-action branch rather than at argument parsing — # proving both flags were consumed as options, not rejected. -NO_GIT_DIR="$WORK_DIR/no-git-dir" -mkdir -p "$NO_GIT_DIR" set_mode "repo-ok" if run "$NO_GIT_DIR" -n 123 -a bogus-action -r "$REPO_OVERRIDE_VAL" -H "$HOST_OVERRIDE_VAL"; then echo "FAIL: bogus action unexpectedly succeeded" >&2 @@ -159,11 +225,12 @@ HELP_TEXT="$("$SCRIPT_DIR/pr-review.sh" -h)" echo "$HELP_TEXT" | grep -q -- '-r, --repo' echo "$HELP_TEXT" | grep -q -- '-H, --host' -# --- Case 3: platform detection is tolerated with NO git repository at all -# when -r/--repo is given (reviewer worktree with a missing/nonstandard -# origin) — must NOT fail with "not a git repository or no origin remote". +# --- Case 3 (comment): a TRUE no-git-origin dir + -r/-H must not silently die +# and must not fail with "not a git repository or no origin remote" either. set_mode "repo-ok" -run "$NO_GIT_DIR" -n 123 -a comment -c durable-body -r "$REPO_OVERRIDE_VAL" -H "$HOST_OVERRIDE_VAL" +comment_rc=0 +run "$NO_GIT_DIR" -n 123 -a comment -c durable-body -r "$REPO_OVERRIDE_VAL" -H "$HOST_OVERRIDE_VAL" || comment_rc=$? +assert_not_silently_dead "comment" "$comment_rc" if grep -qi 'not a git repository or no origin remote' "$OUTPUT_FILE"; then echo "FAIL: -r/--repo did not tolerate a missing git origin" >&2 cat "$OUTPUT_FILE" >&2 @@ -171,6 +238,34 @@ if grep -qi 'not a git repository or no origin remote' "$OUTPUT_FILE"; then fi grep -q 'Added and verified comment on Gitea PR #123' "$OUTPUT_FILE" +# --- Case 3b (approve): same true no-origin dir, the `approve` dispatch body +# (its OWN, previously-unguarded `host=$(get_remote_host)` call) must not +# silently die either. +set_mode "repo-ok" +approve_rc=0 +run "$NO_GIT_DIR" -n 123 -a approve -r "$REPO_OVERRIDE_VAL" -H "$HOST_OVERRIDE_VAL" || approve_rc=$? +assert_not_silently_dead "approve" "$approve_rc" +if grep -qi 'not a git repository or no origin remote' "$OUTPUT_FILE"; then + echo "FAIL: approve's -r/--repo did not tolerate a missing git origin" >&2 + cat "$OUTPUT_FILE" >&2 + exit 1 +fi +grep -q 'Approved and verified Gitea PR #123 (review ID 501)' "$OUTPUT_FILE" + +# --- Case 3c (request-changes): same true no-origin dir, the +# `request-changes` dispatch body's OWN previously-unguarded +# `host=$(get_remote_host)` call must not silently die either. +set_mode "repo-ok" +request_changes_rc=0 +run "$NO_GIT_DIR" -n 123 -a request-changes -c please-fix -r "$REPO_OVERRIDE_VAL" -H "$HOST_OVERRIDE_VAL" || request_changes_rc=$? +assert_not_silently_dead "request-changes" "$request_changes_rc" +if grep -qi 'not a git repository or no origin remote' "$OUTPUT_FILE"; then + echo "FAIL: request-changes's -r/--repo did not tolerate a missing git origin" >&2 + cat "$OUTPUT_FILE" >&2 + exit 1 +fi +grep -q 'Requested changes and verified on Gitea PR #123 (review ID 501)' "$OUTPUT_FILE" + # --- Case 4: -r/--repo and -H/--host WIN over git-remote inference, not just # tolerate its absence — set up a real git repo whose origin points at a # COMPLETELY DIFFERENT host/repo and confirm the override, not the remote, is