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

[fujibee]

Follow-up to #853, and based on it rather than on main: both touch the same shape in the same file, so stacking keeps the diff honest instead of producing a conflict for whoever lands second.

What is wrong

#853 stopped the POST helper discarding curl's stderr. _remote_http_get_json 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. The Windows fix did not reach it, because that work was scoped to the helper the failure happened to land in.

What does NOT apply here, said explicitly

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 #851's defect never reached this function — only the diagnosability one did.

Worth stating because "the Windows fix missed a spot" would be the wrong reading. Two different problems shared one helper; only one of them was in both.

Scope

With this, no curl call in the file discards its stderr — verified by grep, not by eye:

curl calls still discarding stderr: 0

What was verified

tests/test_remote.bats — 122 ok / 0 not ok, on this branch.

Green was not evidence anything was diagnosed. When this was written, no test asserted on curl's stderr — that is no longer true, and the tests added since are described at the end of this body. What green establishes is that capturing it, and emitting it only on failure, leaves every existing caller's behaviour and every reported HTTP code unchanged.

This one is not gated on a platform: it changes the same way everywhere, which is also why the suite is a more meaningful check here than on #851 or #852.

Not measured

A team with an existing store has not been migrated on Windows, on this path or any other. The verified Windows run was a fresh team.

---

Tests, added at c7ddd16d7d4b

tests/test_remote_curl_stderr_get.bats — 6 ok / 0 not ok, driving the production _remote_http_get_json. Production untouched; the diff is this one file.

A separate file from #853's, 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 split into two pull requests precisely so a mutation in one cannot be covered by the other's tests. Sharing a file puts that back: reverting the GET change would redden a case whose name says POST.

case
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
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

mutation	red
M0 no mutation	0
M1 2>/dev/null restored	the diagnosis reaches stderr
M2 stderr shown on success too	success stays quiet
M3 the error file never removed	no scratch left behind
M4 failure reports curl's exit code	the code contract, and the two cases that read it

Every mutation is 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 here.

The first run of that matrix was wrong

Worth recording rather than quietly re-running. 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 "assign an empty code", neither of which is the mutation the label names.

Both still produced red, and the red looked plausible. What gave it away was M2 reddening the failure case instead of the success case. A mutation that does something other than what its label says is a false row in the table, not a stricter one.

Not covered

The signal path, exactly as in #853: the scratch file is removed on both normal paths, the trap does not name it, and what I have is a reading of the trap's text rather than a measurement — the probe hung on this host. Adding curl_err to the trap is one line and it changes the fix, so it is the author's call.

Windows is unverified by me.

---

Rebased, tested, and given the same trap fix — at fa3ccdfa8060

New coordinates, since this PR is stacked:

base   d59284c11e38c244615b9324347bed955f4c8236   (#853's head: the POST fix + its tests)
head   fa3ccdfa80606d9a01d71309503697b6d29cdff8

Two commits were replayed onto that base. The old base 1c833bb is still the second commit of this branch — what moved is where it sits, not what it says.

The GET helper's trap had the same hole, and the same cause

Review asked for one line: add curl_err to the existing trap. That line does not work. An EXIT trap set inside a function runs after the 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.

bash 3.2.57   TRAP SEES: [EMPTY]
bash 5.3.15   TRAP SEES: [EMPTY]

This helper's trap held only $cfg, and it has never removed it on an early exit either. Both the config and the error file are now baked in with printf %q at set time, and curl_err is created beside the config so it exists before the trap that must sweep it. The measured run behind this is in #853, where all three of that helper's files survived.

tests/test_remote_curl_stderr_get.bats — 7 ok / 0 not ok

case
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.

mutation	red
M0 no mutation	0
M1 2>/dev/null restored	the diagnosis reaches stderr
M2 stderr shown on success too	success stays quiet
M3 the error file never removed	no scratch left behind
M4 failure reports curl's exit code	the code contract, and the two cases that read it

Every mutation is confined to the GET function's line range, re-derived each round: the POST helper on this branch carries the same lines verbatim, so a file-wide substitution would have mutated #853's fix and reported the result here.

The first run of that matrix was wrong. The perl replacements had $curl_err and $curl_status unescaped, so perl expanded them as its own variables — to nothing. M2 became "test an empty string" and M4 "assign an empty code". Both still went red, plausibly; what gave it away was M2 reddening the failure case instead of the success case. A mutation that does something other than what its label says is a false row, not a stricter one.

On this head, the POST tests and the existing remote suite run together: 129 ok / 0 not ok.

Windows is unverified by me.

---

Update at 9acc615a4839 — one directory here too

New coordinates, since this PR is stacked:

base   a38d6f8e1430325f1bbf5537348fbd9edeffb6d3   (#853's head)
head   9acc615a4839f9b8c0b5a2a247afcd0c7d652779

The window review found applies to this helper as well: it 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. Same shape as the POST side now — acquire one directory, arm the trap on it, make everything inside.

New case: make chmod fail, 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.

The absence assertions were aimed at names this layout never creates

They 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.

tests/test_remote_curl_stderr_get.bats     8 ok / 0 not ok
POST tests + tests/test_remote.bats      130 ok / 0 not ok   (on this head)

The orphaned-copier behaviour on the POST side is filed as #864 rather than folded into either PR.