From c4180ca104c02b10898dea14805a0da431e1ca50 Mon Sep 17 00:00:00 2001 From: fujibee Date: Mon, 17 Aug 2026 01:54:00 -0700 Subject: [PATCH 1/4] fix(remote): keep curl's stderr on the GET path too (#850) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The POST helper stopped discarding curl's stderr in the previous commit. The GET helper still did, and the argument is identical: it reports "000" for every kind of failure alike, so without stderr there is nothing anywhere saying why. `pull` goes through this path. A failure there was as undiagnosable as the `connect` one that cost a Windows run its afternoon — and the Windows fix did not reach it, because that work was scoped to the helper the failure happened to land in. Unlike the POST helper, this one embeds no paths in its curl config: the output file arrives via `-o`, which is curl's own argv, and MSYS does translate argv for a native binary. So the path defect never reached here. Only the diagnosability one did. With this, no curl call in the file discards its stderr. --- scripts/remote.sh | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/scripts/remote.sh b/scripts/remote.sh index f094b73c..33ab557c 100644 --- a/scripts/remote.sh +++ b/scripts/remote.sh @@ -392,11 +392,18 @@ _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. + local curl_err; curl_err="$(mktemp "${TMPDIR:-/tmp}/agmsg-curl-err.XXXXXX")" + 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 + rm -f "$curl_err" [ "$curl_status" -eq 0 ] || curl_output="000" rm -f "$cfg" trap - EXIT INT TERM From 71fda7646916a052026b0a13f6799595be132a32 Mon Sep 17 00:00:00 2001 From: fujibee Date: Mon, 17 Aug 2026 16:32:43 -0700 Subject: [PATCH 2/4] test(remote): bind the kept stderr on the GET path (#850) tests/test_remote_curl_stderr_get.bats, 6 ok / 0 not ok, driving the production _remote_http_get_json. Reverting the GET change turns the first test red. A SEPARATE FILE, not a parameterised one over both helpers. These are two functions with two shapes -- the GET side has no fifo, no copier, its own trap and its own cleanup -- and they were fixed in two pull requests precisely so a mutation in one cannot be covered by the other's tests. Sharing a file would put that back: reverting the GET change would redden a case whose name says POST. failing curl the diagnosis reaches the caller's stderr succeeding curl nothing does, and the stub wrote something anyway both the http code is what it always was, 200 and 000 both no scratch file left in the run's own TMPDIR control the leftover check can see a planted leftover shape the request still carries the team header, so the cases above are reporting on a GET and not on something reshaped M0 no mutation 0 red M1 stderr discarded again 1 -- the diagnosis reaches stderr M2 stderr shown on success too 1 -- success stays quiet M3 the error file never removed 1 -- no scratch left behind M4 failure reports curl's exit code 3 -- the code contract and the two cases that read it THE MUTATIONS ARE CONFINED TO THE GET FUNCTION'S LINE RANGE, re-derived from the file each round. On this branch the POST helper carries the same lines verbatim, so a file-wide substitution would have mutated #853's fix and reported the result under #854. And the first run of that matrix was wrong in a way worth recording. The perl replacements contained $curl_err and $curl_status unescaped, so perl expanded them as its own variables -- to nothing. M2 became "test an empty string" and M4 became "assign an empty code", neither of which is the mutation named. Both still produced red, and the red looked plausible: M2 reddened the failure case instead of the success case, which is the only reason I looked. A mutation that does something other than what its label says is a false entry in this table, not a stricter one. NOT COVERED, and named rather than left to the matrix to imply: the scratch file is removed on the two normal paths, and the trap set earlier in the function does not name it. The signal path is unmeasured -- a reading of the trap's text, not a measurement. Adding curl_err to that list is one line, but it changes the fix rather than testing it, so it is the author's call. Production untouched: the diff is this one test file. Windows unverified by me. --- tests/test_remote_curl_stderr_get.bats | 141 +++++++++++++++++++++++++ 1 file changed, 141 insertions(+) create mode 100644 tests/test_remote_curl_stderr_get.bats diff --git a/tests/test_remote_curl_stderr_get.bats b/tests/test_remote_curl_stderr_get.bats new file mode 100644 index 00000000..4fd810bf --- /dev/null +++ b/tests/test_remote_curl_stderr_get.bats @@ -0,0 +1,141 @@ +#!/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. + get_with_curl ok "" + [ "$output" = "200" ] + refute ls "$RUN_TMPDIR"/agmsg-curl-err.* 2>/dev/null + refute ls "$RUN_TMPDIR"/agmsg-curl-cfg.* 2>/dev/null + + get_with_curl fail "curl: (28) Operation timed out" + [ "$output" = "000" ] + refute ls "$RUN_TMPDIR"/agmsg-curl-err.* 2>/dev/null + refute ls "$RUN_TMPDIR"/agmsg-curl-cfg.* 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. + get_with_curl ok "" + : > "$RUN_TMPDIR/agmsg-curl-err.planted" + run ls "$RUN_TMPDIR"/agmsg-curl-err.* + [ "$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" ] +} From bb87f1891f1c0ba9b261388da9bb5de8428267f9 Mon Sep 17 00:00:00 2001 From: fujibee Date: Mon, 17 Aug 2026 17:32:11 -0700 Subject: [PATCH 3/4] fix(remote): the GET helper's trap had the same hole, and the same cause (#850) Rebased onto the corrected POST head, then given the same treatment. WHY THE ONE LINE REVIEW ASKED FOR DOES NOT WORK. An EXIT trap set inside a function runs after that function's frame is gone, so a single-quoted body expands `$cfg` in the caller's scope, where no local of that name exists. It removes the empty string and returns 0. Measured on bash 3.2.57 and 5.3.15, both EMPTY; the POST commit carries the run that left three real files behind. This helper's trap held only `$cfg`, and it has never removed it on an early exit either. Now both the config and the error file are baked into the trap with printf %q at set time, and curl_err is created beside the config so it exists before the trap that must sweep it. TESTS. tests/test_remote_curl_stderr_get.bats, 7 ok / 0 not ok. failing curl the diagnosis reaches the caller's stderr succeeding curl nothing does, and the stub wrote to stderr anyway both the http code is unchanged, 200 and 000 both no scratch file left in the run's own TMPDIR early exit errexit out of the middle sweeps both files control the leftover check fires on a planted leftover shape the request still carries the team header, so the rows above are reporting on a GET The early-exit case is cheaper here than on the POST side: this helper has no fifo and no copier, so nothing can be stranded on open() and the run cannot hang. Same property, fewer moving parts. REBASE. Two commits replayed onto d59284c, which is the POST fix plus its tests. The old base 1c833bb is still the second commit of this branch, so what moved is where it sits, not what it says. Confirmed after replay: the GET change is intact and the POST tests still pass here. tests/test_remote_curl_stderr.bats + tests/test_remote.bats 129 ok / 0 not ok on this head Windows unverified by me. --- scripts/remote.sh | 11 +++++++--- tests/test_remote_curl_stderr_get.bats | 30 ++++++++++++++++++++++++++ 2 files changed, 38 insertions(+), 3 deletions(-) diff --git a/scripts/remote.sh b/scripts/remote.sh index 33ab557c..1c10c14a 100644 --- a/scripts/remote.sh +++ b/scripts/remote.sh @@ -379,10 +379,16 @@ _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 + local url="$1" team_id="$2" out_file="$3" cfg curl_output curl_status=0 curl_err cfg="$(mktemp "${TMPDIR:-/tmp}/agmsg-curl-cfg.XXXXXX")" + # Made with the config, so both exist before the trap that has to remove them. + curl_err="$(mktemp "${TMPDIR:-/tmp}/agmsg-curl-err.XXXXXX")" chmod 600 "$cfg" - trap 'rm -f "$cfg"' EXIT INT TERM + # Baked in with printf %q rather than expanded when the trap fires — the same + # reason as the POST helper: an EXIT trap set inside a function runs after + # that function's frame is gone, so a single-quoted `$cfg` expands to nothing + # in the caller's scope and the cleanup silently removes an empty string. + trap "rm -f $(printf '%q %q' "$cfg" "$curl_err")" EXIT INT TERM { printf 'url = "%s"\n' "$url" printf 'request = "GET"\n' @@ -396,7 +402,6 @@ _remote_http_get_json() { # 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. - local curl_err; curl_err="$(mktemp "${TMPDIR:-/tmp}/agmsg-curl-err.XXXXXX")" if curl_output=$(curl -sS -o "$out_file" -w '%{http_code}' -K "$cfg" 2>"$curl_err"); then : else diff --git a/tests/test_remote_curl_stderr_get.bats b/tests/test_remote_curl_stderr_get.bats index 4fd810bf..70a92d63 100644 --- a/tests/test_remote_curl_stderr_get.bats +++ b/tests/test_remote_curl_stderr_get.bats @@ -122,6 +122,36 @@ get_with_curl() { refute ls "$RUN_TMPDIR"/agmsg-curl-cfg.* 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 "$RUN_TMPDIR"/agmsg-curl-err.* 2>/dev/null + refute ls "$RUN_TMPDIR"/agmsg-curl-cfg.* 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. From 9acc615a4839f9b8c0b5a2a247afcd0c7d652779 Mon Sep 17 00:00:00 2001 From: fujibee Date: Mon, 17 Aug 2026 17:57:22 -0700 Subject: [PATCH 4/4] fix(remote): one directory for the GET helper too (#850) Rebased onto the corrected POST head and given the same shape: acquire one directory, arm the trap on it, make the config and the error file inside. The window review found applies here as well -- this helper made the config, then the error file, and only then armed the trap, so a failure at the second allocation or at the chmod stranded the first. There is now exactly one thing in existence before the trap. New case: make `chmod` fail, which happens after the directory exists and after the config is inside it, and assert nothing survives. Cheaper than the POST side's control, because this helper has no fifo and no copier to strand. AND THE ABSENCE ASSERTIONS WERE AIMED AT NAMES THIS LAYOUT NEVER CREATES. The leftover checks globbed agmsg-curl-cfg.* and agmsg-curl-err.*; both files now live inside agmsg-curl.XXXXXX. Those checks would have passed on any behaviour at all, including a leaked directory. They and their planted-leftover control now use the name the helper really mints. 8 ok / 0 not ok tests/test_remote_curl_stderr_get.bats 130 ok / 0 not ok the POST tests and tests/test_remote.bats on this head Windows unverified by me. --- scripts/remote.sh | 26 ++++++++------- tests/test_remote_curl_stderr_get.bats | 44 +++++++++++++++++++++----- 2 files changed, 51 insertions(+), 19 deletions(-) diff --git a/scripts/remote.sh b/scripts/remote.sh index 1c10c14a..9c7a5607 100644 --- a/scripts/remote.sh +++ b/scripts/remote.sh @@ -379,16 +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 curl_err - cfg="$(mktemp "${TMPDIR:-/tmp}/agmsg-curl-cfg.XXXXXX")" - # Made with the config, so both exist before the trap that has to remove them. - curl_err="$(mktemp "${TMPDIR:-/tmp}/agmsg-curl-err.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" - # Baked in with printf %q rather than expanded when the trap fires — the same - # reason as the POST helper: an EXIT trap set inside a function runs after - # that function's frame is gone, so a single-quoted `$cfg` expands to nothing - # in the caller's scope and the cleanup silently removes an empty string. - trap "rm -f $(printf '%q %q' "$cfg" "$curl_err")" EXIT INT TERM { printf 'url = "%s"\n' "$url" printf 'request = "GET"\n' @@ -408,9 +411,10 @@ _remote_http_get_json() { curl_status=$? fi [ "$curl_status" -ne 0 ] && [ -s "$curl_err" ] && cat "$curl_err" >&2 - rm -f "$curl_err" [ "$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" } diff --git a/tests/test_remote_curl_stderr_get.bats b/tests/test_remote_curl_stderr_get.bats index 70a92d63..371fc037 100644 --- a/tests/test_remote_curl_stderr_get.bats +++ b/tests/test_remote_curl_stderr_get.bats @@ -111,15 +111,17 @@ get_with_curl() { @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 "$RUN_TMPDIR"/agmsg-curl-err.* 2>/dev/null - refute ls "$RUN_TMPDIR"/agmsg-curl-cfg.* 2>/dev/null + refute ls -d "$RUN_TMPDIR"/agmsg-curl.* 2>/dev/null get_with_curl fail "curl: (28) Operation timed out" [ "$output" = "000" ] - refute ls "$RUN_TMPDIR"/agmsg-curl-err.* 2>/dev/null - refute ls "$RUN_TMPDIR"/agmsg-curl-cfg.* 2>/dev/null + 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)" { @@ -148,16 +150,42 @@ get_with_curl() { ' [ "$status" -ne 0 ] - refute ls "$RUN_TMPDIR"/agmsg-curl-err.* 2>/dev/null - refute ls "$RUN_TMPDIR"/agmsg-curl-cfg.* 2>/dev/null + 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 "" - : > "$RUN_TMPDIR/agmsg-curl-err.planted" - run ls "$RUN_TMPDIR"/agmsg-curl-err.* + mkdir -p "$RUN_TMPDIR/agmsg-curl.planted" + run ls -d "$RUN_TMPDIR"/agmsg-curl.* [ "$status" -eq 0 ] }