diff --git a/terminal/launchers/gitpr-merge-safe.sh b/terminal/launchers/gitpr-merge-safe.sh index 4f8cb4d..90f5419 100755 --- a/terminal/launchers/gitpr-merge-safe.sh +++ b/terminal/launchers/gitpr-merge-safe.sh @@ -31,6 +31,21 @@ require_cmd() { require_cmd git require_cmd gh +# ui_spinner, so the gh round trips below read as work rather than as a hang. +# This script runs as its own process (the git menu launches it, never sources +# it), so it has to load the UI library itself — and degrade to a no-op if the +# library is not there, because a missing spinner must not cost you the merge +# tool. MQ_UI_LIB is an override for tests. +_mq_ui_lib="${MQ_UI_LIB:-$(cd "$(dirname "${BASH_SOURCE[0]}")/../../ui/terminal-ui" 2>/dev/null && pwd)/mq-ui.sh}" +if [[ -f "$_mq_ui_lib" ]]; then + # shellcheck source=../../ui/terminal-ui/mq-ui.sh + source "$_mq_ui_lib" +fi +if ! declare -f ui_spinner >/dev/null; then + ui_spinner() { local _label="$1"; shift; "$@"; } +fi +unset _mq_ui_lib + git rev-parse --is-inside-work-tree >/dev/null 2>&1 || die "Not inside a Git repository." # Class C interactive action: refuse before any network operation if there is no @@ -57,16 +72,21 @@ pr_number="${1:-}" if [[ -z "$pr_number" ]]; then # Try the PR whose head is the current branch. - pr_number="$(gh pr view --json number --jq '.number' 2>/dev/null || true)" + pr_number="$(ui_spinner "Looking for a PR on $current_branch" \ + gh pr view --json number --jq '.number' 2>/dev/null || true)" fi if [[ -z "$pr_number" ]]; then yellow "No PR argument and no PR for the current branch. Open pull requests:" echo - gh pr list --state open --limit 30 \ - --json number,title,headRefName,baseRefName \ - --template '{{range .}}{{printf " %v) #%v %s (%s -> %s)\n" .number .number .title .headRefName .baseRefName}}{{end}}' \ + # Captured rather than streamed, so the spinner has somewhere to sit while + # the list is fetched and the list still prints in one piece afterwards. + open_prs="$(ui_spinner "Listing open pull requests" \ + gh pr list --state open --limit 30 \ + --json number,title,headRefName,baseRefName \ + --template '{{range .}}{{printf " %v) #%v %s (%s -> %s)\n" .number .number .title .headRefName .baseRefName}}{{end}}')" \ || die "Could not list pull requests." + printf '%s\n' "$open_prs" echo read -r -p "PR number to merge (or q to quit): " pr_number [[ "${pr_number:-}" =~ ^[Qq]$ ]] && exit 0 @@ -77,23 +97,28 @@ fi # ------------------------ # SHOW MERGE PLAN # ------------------------ -gh pr view "$pr_number" --json number >/dev/null 2>&1 || die "Could not read PR #$pr_number." - -# Read one field at a time via gh's built-in --jq, so we do not depend on a -# standalone jq binary being installed. -pr_field() { - gh pr view "$pr_number" --json "$1" --jq ".$1" 2>/dev/null || true -} - -title="$(pr_field title)" -state="$(pr_field state)" -is_draft="$(pr_field isDraft)" -base_ref="$(pr_field baseRefName)" -head_ref="$(pr_field headRefName)" -mergeable="$(pr_field mergeable)" -merge_state="$(pr_field mergeStateStatus)" -review_decision="$(pr_field reviewDecision)" -pr_url="$(pr_field url)" +# One request for the whole plan, still through gh's built-in --jq so there is +# no dependency on a standalone jq binary. This used to be nine `gh pr view` +# calls, one per field, which measured 3.44s of silence against 0.47s for the +# batched read. The separate existence check was a tenth call saying what this +# one already says by failing. +# +# Fields are joined on U+001F, not on a tab: tab counts as IFS whitespace, so +# `read` collapses a run of them and an empty field shifts every later field +# left. reviewDecision is empty on any PR nobody has reviewed — which is most +# of them — so a tab-separated read would have silently swapped the review +# decision for the URL. Nulls are mapped to "" for the same reason: jq's join +# refuses to join null. +pr_plan="$(ui_spinner "Reading PR #$pr_number" \ + gh pr view "$pr_number" \ + --json title,state,isDraft,baseRefName,headRefName,mergeable,mergeStateStatus,reviewDecision,url \ + --jq '[.title,.state,.isDraft,.baseRefName,.headRefName,.mergeable,.mergeStateStatus,.reviewDecision,.url] + | map(if . == null then "" else tostring end) | join("\u001f")' \ + 2>/dev/null)" || die "Could not read PR #$pr_number." +[[ -n "$pr_plan" ]] || die "Could not read PR #$pr_number." + +IFS=$'\037' read -r title state is_draft base_ref head_ref \ + mergeable merge_state review_decision pr_url <<<"$pr_plan" echo blue "Merge plan" @@ -113,9 +138,18 @@ if [[ "$is_draft" == "true" ]]; then die "PR #$pr_number is a draft. Mark it ready for review first." fi -# CI / check summary. +# CI / check summary. Fetched once and reused below: this used to run twice, +# once to print and once to decide, paying for the same round trip either way. +checks_rc=0 +checks_output="$(ui_spinner "Reading checks for #$pr_number" \ + gh pr checks "$pr_number" 2>/dev/null)" || checks_rc=$? + yellow "Checks:" -gh pr checks "$pr_number" 2>/dev/null || yellow " (no checks reported)" +if [[ -n "$checks_output" ]]; then + printf '%s\n' "$checks_output" +else + yellow " (no checks reported)" +fi echo # ------------------------ @@ -134,7 +168,7 @@ if [[ "$review_decision" == "CHANGES_REQUESTED" ]]; then yellow "Review decision is CHANGES_REQUESTED." risky=1 fi -if ! gh pr checks "$pr_number" >/dev/null 2>&1; then +if [[ "$checks_rc" -ne 0 ]]; then yellow "One or more checks are failing or pending." risky=1 fi diff --git a/tests/gitpr-merge-safe-smoke.sh b/tests/gitpr-merge-safe-smoke.sh new file mode 100755 index 0000000..6e0fbd2 --- /dev/null +++ b/tests/gitpr-merge-safe-smoke.sh @@ -0,0 +1,131 @@ +#!/usr/bin/env bash +# gitpr-merge-safe.sh closes a remote PR. Until now it had no coverage at all — +# gitmerge-safe-smoke.sh tests the local-merge sibling, not this one. +# +# Two things are pinned here. The guardrails, because this is the one script in +# the repo that can change a remote branch: no TTY means no merge, and the +# final confirmation must be answerable with "no". And the shape of the plan +# fetch, because the reason for touching this file was a wait: reading nine PR +# fields took nine `gh pr view` calls, 3.44s measured, all of it silent. One +# batched call measured 0.47s. A spinner makes a wait legible; not making the +# wait is better, so the batching is the fix and the spinner covers what is +# left. +# +# The fake gh records every invocation, so "one call, not nine" is asserted +# against behaviour rather than against how the source happens to read. +set -euo pipefail + +ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" +SCRIPT="$ROOT/terminal/launchers/gitpr-merge-safe.sh" +TMP="$(mktemp -d)" +trap 'rm -rf "$TMP"' EXIT + +echo "SMOKE: gitpr-merge-safe" + +echo "[1/6] the script exists and parses" +test -x "$SCRIPT" +bash -n "$SCRIPT" + +echo "[2/6] it refuses to merge without a TTY" +out="$(cd "$ROOT" && "$SCRIPT" 999 squash &1 || true)" +grep -q "interactive terminal" <<<"$out" + +# A throwaway repo and a fake gh, so nothing here can reach GitHub. +REPO="$TMP/repo" +mkdir -p "$REPO" "$TMP/bin" +git -C "$REPO" init -q +git -C "$REPO" -c user.email=t@t -c user.name=t commit -q --allow-empty -m init +git -C "$REPO" branch -M feature-x + +export GH_CALLS="$TMP/gh-calls" +: >"$GH_CALLS" + +cat >"$TMP/bin/gh" <<'STUB' +#!/usr/bin/env bash +printf '%s\n' "$*" >>"$GH_CALLS" +case "$1 $2" in + "auth status") exit 0 ;; + "pr checks") echo "build pass 10s"; exit 0 ;; + "pr view") + # One batched read: every field the merge plan needs, joined on U+001F. + # reviewDecision is deliberately empty — that is the ordinary case (nobody + # reviewed it) and the one a tab-separated read would silently shift. + printf 'Add a thing\037OPEN\037false\037main\037feature-x\037MERGEABLE\037CLEAN\037\037https://example/pr/42\n' + exit 0 + ;; +esac +exit 0 +STUB +chmod +x "$TMP/bin/gh" + +# Answers the one confirmation with "n", so no merge is ever attempted. +run_plan() { + PATH="$TMP/bin:$PATH" GH_CALLS="$GH_CALLS" python3 - "$SCRIPT" "$REPO" <<'PY' +import os, pty, re, select, sys + +# pty.spawn's stdin_read callback fires whenever the master is writable, which +# floods the script with answers. Drive it manually and reply only to an actual +# prompt, so the transcript is the script's output and nothing else. +script, repo = sys.argv[1], sys.argv[2] +os.chdir(repo) +PROMPT = re.compile(rb"\[y/N\]:\s*$", re.IGNORECASE) + +pid, fd = pty.fork() +if pid == 0: + os.execv("/bin/bash", ["bash", script, "42", "squash"]) + +seen, pending = bytearray(), bytearray() +while True: + if not select.select([fd], [], [], 30)[0]: + break + try: + chunk = os.read(fd, 1024) + except OSError: + break + if not chunk: + break + seen.extend(chunk) + pending.extend(chunk) + if PROMPT.search(bytes(pending).rstrip()): + os.write(fd, b"n\n") + pending.clear() + +os.waitpid(pid, 0) +sys.stdout.write(seen.decode(errors="replace")) +PY +} + +echo "[3/6] the merge plan renders and the confirmation can be declined" +: >"$GH_CALLS" +out="$(run_plan 2>&1)" +grep -q "Merge plan" <<<"$out" +grep -q "Add a thing" <<<"$out" +grep -q "feature-x -> main" <<<"$out" +grep -q "Merge cancelled" <<<"$out" +# An empty reviewDecision must not drag the URL into its place. +grep -q "Review: none" <<<"$out" +grep -q "URL: https://example/pr/42" <<<"$out" + +echo "[4/6] declining means no merge was ever attempted" +! grep -q "pr merge" "$GH_CALLS" + +echo "[5/6] the plan costs one gh pr view, not one per field" +# Nine fields used to mean nine round trips. Anything above two here means the +# per-field pattern has come back. +views="$(grep -c "^pr view" "$GH_CALLS" || true)" +test "$views" -le 2 || { + echo "FAIL: $views 'gh pr view' calls for one merge plan" >&2 + cat "$GH_CALLS" >&2 + exit 1 +} + +echo "[6/6] it still runs when the UI library is unreachable" +# The script is launched as its own process from the git menu, so it sources +# mq-ui.sh itself. A missing library must degrade to no spinner, not to a +# broken merge tool. +: >"$GH_CALLS" +out="$(MQ_UI_LIB="$TMP/does-not-exist.sh" run_plan 2>&1)" +grep -q "Merge plan" <<<"$out" +grep -q "Merge cancelled" <<<"$out" + +echo "OK: gitpr-merge-safe smoke passed" diff --git a/tests/manifest.tsv b/tests/manifest.tsv index aa967ef..42d24f9 100644 --- a/tests/manifest.tsv +++ b/tests/manifest.tsv @@ -97,3 +97,4 @@ menu-exit-contract-smoke.sh active - theme-manager-path-smoke.sh active - ui-spinner-smoke.sh active - mqobsidian-manifest-shell-parity-smoke.sh active - +gitpr-merge-safe-smoke.sh active - diff --git a/tools/scripts/test-all.sh b/tools/scripts/test-all.sh index 9708594..eec4640 100755 --- a/tools/scripts/test-all.sh +++ b/tools/scripts/test-all.sh @@ -27,6 +27,7 @@ echo "== Running mqlaunch headless checks ==" "$PROJECT_ROOT/tests/mq-stack-contract-smoke.sh" "$PROJECT_ROOT/tests/gitlaunch-menu-surface-smoke.sh" "$PROJECT_ROOT/tests/gitmerge-safe-smoke.sh" +"$PROJECT_ROOT/tests/gitpr-merge-safe-smoke.sh" "$PROJECT_ROOT/tests/git-menu-surface-smoke.sh" "$PROJECT_ROOT/tests/git-restore-to-base-smoke.sh" "$PROJECT_ROOT/tests/dashboard-header-cache-smoke.sh"