fix(remote): keep curl's stderr on the GET path too (#850)

scripts/remote.sh

@@ -379,10 +379,19 @@ _remote_http_post_json() {
 # Nothing else is sent, because there is nothing else to send: this protocol
 # carries no credential at all (see cmd_connect).
 _remote_http_get_json() {
-  local url="$1" team_id="$2" out_file="$3" cfg curl_output curl_status=0
-  cfg="$(mktemp "${TMPDIR:-/tmp}/agmsg-curl-cfg.XXXXXX")"
+  local url="$1" team_id="$2" out_file="$3" cfg curl_output curl_status=0 \
+    curl_err work_dir
+  # One allocation, then the trap, then everything else inside it — the same
+  # shape as the POST helper, and for the same two reasons. A trap set inside a
+  # function cannot expand that function's locals when it fires, so the paths
+  # are baked in with printf %q; and anything created before the trap is armed
+  # is unprotected, so only one thing is.
+  work_dir="$(mktemp -d "${TMPDIR:-/tmp}/agmsg-curl.XXXXXX")"
+  trap "rm -rf $(printf '%q' "$work_dir")" EXIT INT TERM
+  cfg="$work_dir/config"
+  curl_err="$work_dir/stderr"
+  : > "$cfg"
   chmod 600 "$cfg"
-  trap 'rm -f "$cfg"' EXIT INT TERM
   {
     printf 'url = "%s"\n' "$url"
     printf 'request = "GET"\n'
@@ -392,13 +401,20 @@ _remote_http_get_json() {
     printf 'max-time = "15"\n'
     printf 'max-filesize = "2097152"\n'
   } > "$cfg"
-  if curl_output=$(curl -sS -o "$out_file" -w '%{http_code}' -K "$cfg" 2>/dev/null); then
+  # Same reason as the POST helper: discarding stderr leaves "000" as the only
+  # thing anyone sees, and "000" is what this reports for every failure alike.
+  # `pull` goes through here, so a failure on this path was as undiagnosable as
+  # the connect one that cost a Windows run its afternoon.
+  if curl_output=$(curl -sS -o "$out_file" -w '%{http_code}' -K "$cfg" 2>"$curl_err"); then
     :
   else
     curl_status=$?
   fi
+  [ "$curl_status" -ne 0 ] && [ -s "$curl_err" ] && cat "$curl_err" >&2
   [ "$curl_status" -eq 0 ] || curl_output="000"
-  rm -f "$cfg"
+  # The config and the error file are both inside it, so the normal path
+  # removes exactly what the trap would have.
+  rm -rf "$work_dir"
   trap - EXIT INT TERM
   printf '%s' "$curl_output"
 }

tests/test_remote_curl_stderr_get.bats

@@ -0,0 +1,199 @@
+#!/usr/bin/env bats
+# THE SAME LOSS, ON THE OTHER HELPER (#850).
+#
+# `_remote_http_get_json` discarded curl's stderr exactly as the POST helper
+# did, and reports the same `000` for every failure alike. `pull` goes through
+# here, so a failure on this path was as undiagnosable as the connect one.
+#
+# SEPARATE TESTS, NOT A SHARED ONE. This is a different function with a
+# different shape -- no fifo, no copier, its own trap and its own cleanup --
+# and the two were fixed in two pull requests so that a mutation in one cannot
+# be covered by the other's tests. A parameterised file over both helpers would
+# put that back: reverting the GET change would redden a case whose name says
+# POST, and the attribution is the thing being bought.
+#
+# Stderr is captured to a FILE rather than read from bats's `$output`, which
+# merges the streams. The http code goes to stdout and the diagnosis has to go
+# to stderr; a test that cannot tell them apart cannot say that.
+
+load test_helper
+
+SANDBOX_TOOLS=(bash dirname mktemp mkfifo chmod rm rmdir sed cp cat grep python3 uname)
+
+setup() {
+  setup_test_env
+
+  STUB_SRC="$BATS_TEST_TMPDIR/stubs"
+  mkdir -p "$STUB_SRC"
+
+  cat > "$STUB_SRC/curl" <<'STUB'
+#!/usr/bin/env bash
+set -u
+cfg=""; out=""; prev=""
+for arg in "$@"; do
+  case "$prev" in
+    -K) cfg="$arg" ;;
+    -o) out="$arg" ;;
+  esac
+  prev="$arg"
+done
+[ -z "${STUB_CURL_STDERR:-}" ] || printf '%s\n' "$STUB_CURL_STDERR" >&2
+if [ "${STUB_CURL_MODE:-ok}" = "fail" ]; then
+  exit 7
+fi
+# The GET helper sends the team in a header and expects a body back; nothing
+# here dumps headers, which is one of the shape differences from the POST side.
+grep -q '^header = "Agmsg-Team-ID: ' "$cfg" || { echo "STUB_CURL: no team header" >&2; exit 2; }
+[ -z "$out" ] || printf '{"teams":[]}' > "$out"
+printf '200'
+STUB
+  chmod +x "$STUB_SRC/curl"
+}
+
+teardown() { teardown_test_env; }
+
+sandbox_path() {
+  local dir tool src
+  dir="$(mktemp -d "$BATS_TEST_TMPDIR/sandbox.XXXXXX")"
+  for tool in "${SANDBOX_TOOLS[@]}"; do
+    src="$(command -v "$tool")" || { echo "host lacks $tool" >&2; return 1; }
+    ln -s "$src" "$dir/$tool"
+  done
+  ln -s "$STUB_SRC/curl" "$dir/curl"
+  printf '%s' "$dir"
+}
+
+get_with_curl() {
+  local mode="$1" stderr_text="$2"
+  RUN_TMPDIR="$(mktemp -d "$BATS_TEST_TMPDIR/run.XXXXXX")"
+  ERR_FILE="$BATS_TEST_TMPDIR/helper-stderr"
+  local bin; bin="$(sandbox_path)"
+
+  run env PATH="$bin" TMPDIR="$RUN_TMPDIR" STUB_CURL_MODE="$mode" \
+    STUB_CURL_STDERR="$stderr_text" bash -c '
+    set -uo pipefail
+    . '"$SCRIPTS"'/remote.sh 2>/dev/null
+    _remote_http_get_json "https://example.invalid/v1/teams" "team-abc" \
+      "'"$RUN_TMPDIR"'/out-body" 2>"'"$ERR_FILE"'"
+  '
+}
+
+@test "GET: a failing curl's diagnosis reaches the caller's stderr (#850)" {
+  # `pull` is the command a person runs here, and before this it could only say
+  # 000 -- the same 000 as a refused connection, a timeout, or an unopenable
+  # path.
+  get_with_curl fail "curl: (7) Failed to connect to example.invalid port 443"
+  [ "$status" -eq 0 ]
+  [ "$output" = "000" ]
+
+  grep -q 'Failed to connect' "$ERR_FILE"
+}
+
+@test "GET: a successful curl's stderr is NOT shown, even when it wrote something (#850)" {
+  # Tells "shown only when curl failed" apart from "the stream was empty". The
+  # stub deliberately writes on the success path; real curl -sS would not, and
+  # a quiet stub would pass against an unconditional dump.
+  get_with_curl ok "a progress line nobody asked for"
+  [ "$status" -eq 0 ]
+  [ "$output" = "200" ]
+
+  [ ! -s "$ERR_FILE" ]
+}
+
+@test "GET: the http code is unchanged on both paths (#850)" {
+  get_with_curl ok ""
+  [ "$output" = "200" ]
+
+  get_with_curl fail "curl: (28) Operation timed out"
+  [ "$output" = "000" ]
+}
+
+@test "GET: no scratch file is left behind, on either path (#850)" {
+  # This helper writes a config and now an error file, and removes both. Each
+  # run gets a private TMPDIR so the question is about this call only.
+  # THE NAME MATTERS. An earlier version globbed agmsg-curl-cfg.* and
+  # agmsg-curl-err.*, which this layout never creates -- the check would pass on
+  # any behaviour at all, including a leaked directory. An absence assertion
+  # aimed at a name nothing uses is indistinguishable from a clean run.
+  get_with_curl ok ""
+  [ "$output" = "200" ]
+  refute ls -d "$RUN_TMPDIR"/agmsg-curl.* 2>/dev/null
+
+  get_with_curl fail "curl: (28) Operation timed out"
+  [ "$output" = "000" ]
+  refute ls -d "$RUN_TMPDIR"/agmsg-curl.* 2>/dev/null
+}
+
+@test "GET: an early exit between the mktemp and the cleanup still sweeps both files (#850)" {
+  # The same hole as the POST side, and the same bounded way in: `cat` is the
+  # last command of the chain that shows the diagnosis, so a failing cat under
+  # `set -e` leaves the function after the mktemp and before the rm.
+  #
+  # This helper has no fifo and no copier, so nothing here can strand a reader
+  # -- which makes it the cheaper of the two to drive, and it is still the same
+  # property: whatever survives an early exit came from the trap.
+  RUN_TMPDIR="$(mktemp -d "$BATS_TEST_TMPDIR/run.XXXXXX")"
+  local bin; bin="$(sandbox_path)"
+
+  # The symlink is removed before writing: `>` follows a symlink to its target,
+  # which here would be the system's own /bin/cat.
+  rm -f "$bin/cat"
+  printf '#!/usr/bin/env bash\nexit 1\n' > "$bin/cat"
+  chmod +x "$bin/cat"
+
+  run env PATH="$bin" TMPDIR="$RUN_TMPDIR" STUB_CURL_MODE=fail \
+    STUB_CURL_STDERR="curl: (7) Failed to connect" bash -c '
+    set -euo pipefail
+    . '"$SCRIPTS"'/remote.sh 2>/dev/null
+    _remote_http_get_json "https://example.invalid/v1/teams" "team-abc" \
+      "'"$RUN_TMPDIR"'/out-body"
+  '
+  [ "$status" -ne 0 ]
+
+  refute ls -d "$RUN_TMPDIR"/agmsg-curl.* 2>/dev/null
+}
+
+@test "GET: a failure while setting up leaves nothing behind either (#850)" {
+  # The window review found: whatever exists before the trap is armed is
+  # unprotected, and there is always a first allocation. There is now exactly
+  # one, and everything else is made inside it.
+  #
+  # Driven by making `chmod` fail, which happens after the directory exists and
+  # after the config has been created inside it.
+  RUN_TMPDIR="$(mktemp -d "$BATS_TEST_TMPDIR/run.XXXXXX")"
+  local bin; bin="$(sandbox_path)"
+
+  rm -f "$bin/chmod"
+  printf '#!/usr/bin/env bash\nexit 1\n' > "$bin/chmod"
+  chmod +x "$bin/chmod"
+
+  run env PATH="$bin" TMPDIR="$RUN_TMPDIR" bash -c '
+    set -euo pipefail
+    . '"$SCRIPTS"'/remote.sh 2>/dev/null
+    _remote_http_get_json "https://example.invalid/v1/teams" "team-abc" \
+      "'"$RUN_TMPDIR"'/out-body"
+  '
+  [ "$status" -ne 0 ]
+
+  refute ls -d "$RUN_TMPDIR"/agmsg-curl.* 2>/dev/null
+}
+
+@test "GET: the leftover check can see a leftover when there is one (#850)" {
+  # Control on the absence above: a glob matching nothing looks the same as a
+  # glob aimed at the wrong directory.
+  # Planted under the name the helper really mints, so this controls the glob
+  # the absence assertions actually run.
+  get_with_curl ok ""
+  mkdir -p "$RUN_TMPDIR/agmsg-curl.planted"
+  run ls -d "$RUN_TMPDIR"/agmsg-curl.*
+  [ "$status" -eq 0 ]
+}
+
+@test "GET: the request still carries the team header (#850)" {
+  # The contract the stub leans on, asserted rather than assumed. Without this
+  # the stub's own guard could be satisfied by a config that changed shape, and
+  # every case above would be reporting on something other than a GET.
+  get_with_curl ok ""
+  [ "$output" = "200" ]
+  [ -s "$RUN_TMPDIR/out-body" ]
+}