fix(remote): render embedded curl-config paths for the platform (#850) - #851
Open
fujibee wants to merge 4 commits into
Open
fix(remote): render embedded curl-config paths for the platform (#850)#851fujibee wants to merge 4 commits into
fujibee wants to merge 4 commits into
Conversation
`_remote_http_post_json` writes a curl `-K` config so the request body — which holds the token — never reaches curl's argv. Two of the values it writes are paths: `dump-header` and `data = "@..."`. On Windows/Git Bash those paths are unopenable. MSYS translates POSIX paths to Windows form for a native binary's ARGV, and not for the contents of a file that binary reads. So `/tmp/...` written into the config stays `/tmp/...`, and native curl has no such path. curl fails, and the caller reports the "000" it reports for every failure, with no indication that a path was the problem. `cygpath -m`, not `-w`: -m yields a Windows drive path with FORWARD slashes, and curl's config parser treats a backslash as an escape — so the -w form is re-mangled by curl itself. Measured on the machine, not reasoned about. Gated on `command -v cygpath`, not on an OS name. Where cygpath does not exist the helper returns its argument unchanged, so macOS and Linux take exactly the path they took before — verified directly rather than inferred from a green suite: the helper is a pass-through for /tmp, $TMPDIR and a /private/tmp path on this machine.
…tely (#850) The stop condition was that reverting the production change leaves CI green. It does now: reverting the renderer turns two tests red, and reverting either single field turns exactly its own test red. The test drives the real _remote_http_post_json, sourced from remote.sh, with two stubs on PATH. What makes the stub worth anything is that it does not just record the config -- it OPENS WHAT THE CONFIG NAMES: cygpath -m yields forward slashes, -w yields backslashes. It must really produce both, or the test cannot tell the two apart, and telling them apart is the whole point of the fix naming -m. curl resolves each embedded path the way a native Windows binary resolves a Windows path back to the same file, and EXITS NON-ZERO when it cannot. A stub that accepts any string as a path would pass on a rendering no curl could open -- which is the defect itself. Four cases: no cygpath both fields byte-for-byte, so macOS and Linux are demonstrably unchanged rather than assumed unchanged cygpath, data rendered mixed cygpath, dump-header rendered mixed either way no backslash anywhere in the config The two field cases were one test until the matrix showed why they cannot be. Reverting the header field and reverting the data field both reddened the same combined assertion, which says 'something is untranslated' and points at neither. They are two effects of one line and regress apart: translate the body and not the header and curl opens the body, fails on the header, and the caller reports 000 -- which reads as a header problem. A fifth case is the control on the stub: replace _remote_curl_path after sourcing so it emits a path nothing can open, and the helper must return 000 -- the code the user actually saw. Without it, every assertion above could be passing on a string that no curl would accept. M0 no mutation 0 red M1 renderer back to passthrough 2 -- DATA and DUMP-HEADER M2 cygpath -m becomes -w 3 -- both fields and the slash assertion M3 header field left untranslated 1 -- DUMP-HEADER M4 data field left untranslated 1 -- DATA Production is untouched. Windows: still unverified here -- the cygpath branch is driven by a stub, and a stub cannot tell you what MSYS does.
) Review found two holes, both in my instrument rather than in the fix. P1-a THE NEGATIVE ARM WAS NOT NEGATIVE. 'without cygpath' prepended a directory holding only a curl symlink and left the host PATH behind it, so production's 'command -v cygpath' would still have found a real one. It passed here because macOS has no cygpath -- the test inherited an absence instead of creating it, and on the one machine this fix is for it would have run the wrong arm. Now an allowlist sandbox, the shape test_helper already uses for path_without_python3. A subtraction cannot express this: on Git Bash cygpath lives in the same directory as mktemp. The first test asserts the sandbox produces BOTH answers, so the capability every other arm rests on is measured rather than assumed. P1-b THE STUB ACCEPTED THE BROKEN PATH. Its resolve() took any absolute POSIX path whatever the platform, so it could not fail on the untranslated /tmp/... that broke the user -- while the file header claimed it opened what curl would open and the commit message claimed it failed where curl would fail. M1 went red on string assertions alone. The claim and the stub now say the same thing. The stub is told which consumer it plays. native-windows opens only Windows paths; posix opens only POSIX ones. Neither accepts both, which is the actual shape of the defect: the same bytes are openable or not depending on who reads them. Both directions have a case, so 'refuses a POSIX path' cannot be read as 'refuses whatever the test needs'. The outcome assertion moved out of the two field tests, and this was forced by the matrix rather than chosen. With the stub now failing on either untranslated field, asserting 200 inside both field tests made both go red for either mutation -- destroying the per-field attribution that splitting them was for. The 200 has its own test, whose job is to fail for any of them. M0 no mutation 0 red M1 renderer back to passthrough 3 -- DATA, DUMP-HEADER, completes M2 cygpath -m becomes -w 4 -- those three and the slash assertion M3 header field untranslated 2 -- DUMP-HEADER and completes M4 data field untranslated 2 -- DATA and completes 7 ok / 0 not ok. Production untouched; the diff is this one test file. Windows: still unverified by me. The cygpath arm is driven by stubs, and a stub cannot tell you what MSYS does. What this closes is 'revert production and CI stays green', not 'it works on Windows'.
fujibee
added a commit
that referenced
this pull request
Aug 17, 2026
…probe (#850) Two changes. The tests are the stop condition; the one production line is the ARTIFACT finding from the same review, and it is a comment. TESTS. tests/test_remote_header_sink.bats, 6 ok / 0 not ok, driving the real _remote_http_post_json under an allowlist sandbox PATH. Absences are the whole subject here -- no fifo, no copier -- and an absence is only observable if the instrument can record a presence. mkfifo and python3 are replaced by wrappers that log and then exec the real thing, so nothing about the run changes, and the first test proves the wrappers record at all. A log that was never wired up looks exactly like a call that never happened. no fifo is made marker present no bounded copier is started marker present the caller's header file survives marker present fifo AND copier still used marker absent -- the platforms that are supposed to be unchanged, asserted the fifo is removed, the header file is not marker absent The third is the trap this fix had to avoid: on the marker path header_fifo IS header_file, so a cleanup that removes the fifo deletes the headers the caller asked for, after a request that succeeded. M0 no mutation 0 red M1 marker ignored, always fifo 2 -- no fifo, no copier M2 cleanup removes header_fifo 1 -- header file survives M3 marker branch builds a fifo 1 -- no fifo M4 marker branch starts a copier 1 -- no copier M3 and M4 are why the two absences are two tests. They are separate machinery and are left behind separately: a fifo nobody drains, or a reader blocked on a file that never fills. One test asserting both reddens for either and names neither. COMMENT. The gate said "what decides is whether a real fifo can be made, and that is what the probe asks". It does not ask that. It runs a command -v on cygpath, which is a capability MARKER for an environment where MSYS fifos and a native curl coexist -- nothing here tests whether a real fifo can be made. As written it would have told the next reader that a machine passing the probe had been checked for the property that matters. Behaviour is unchanged: the diff outside tests/ is comment lines. This branch does not carry #851, so paths stay POSIX here and the curl stub is a POSIX consumer. On a real Windows machine this fix alone is not enough -- both are needed, and they compose only on the combined branch. Windows remains unverified by me.
…tream (#850) CI caught this; my machine did not. Two cases went red on macOS CI at the head I had reported as green: not ok 351 an untranslated POSIX path reaching native curl is the reported 000 not ok 352 a Windows path reaching a POSIX curl is equally a 000 Both assert the helper returns "000". Both got it. What they compared it against was bats's $output, which MERGES stdout and stderr, and under load the runner produced: remote.sh: line 318: .../agmsg-header-pipe.JVbrl7/header: Interrupted system call 000 An exact comparison against a two-line string fails. The code was right; the instrument was reading a stream with something else on it. Not a platform difference -- same OS, same code, different timing. The fifo open in the copier was interrupted on a busy runner and bash reported it. My local runs never hit it, so the assertion looked exact when it was only usually-exact. The helper's stderr now goes to a file and $output holds the http code alone, which is the same separation the stderr tests (#853) make deliberately. There the separation IS the subject; here it was an accident I had not noticed depending on. 8 ok / 0 not ok locally, and the matrix is unchanged: M0 no mutation 0 red M1 renderer back to passthrough 3 -- DATA, DUMP-HEADER, completes M2 cygpath -m becomes -w 4 -- those three and the slash assertion M3 header field untranslated 2 -- DUMP-HEADER and completes M4 data field untranslated 2 -- DATA and completes Production untouched. Windows unverified by me.
fujibee
added a commit
that referenced
this pull request
Aug 18, 2026
…ingerprint is real, and doctor names a wedged lock (#868) * ci(windows): take the package feed off the Windows legs entirely (#824) #827 made a chocolatey outage legible -- retry, cache, and a presence check that is the only step allowed to be red about it. This removes the cause. The issue's own Directions ranked the retry last, "the cheapest change and the least durable", and that was right: on a cache miss the leg still needed community.chocolatey.org. What changed since is the weight. `bats` is a required check on `main` with enforce_admins true, so a red Windows leg now blocks every landing in the repository, and nobody can wave it through. sqlite3 now comes from one pinned sqlite.org URL, verified by hash: https://sqlite.org/2026/sqlite-tools-win-x64-3530400.zip sha256 f46ee2475de4cbe287e6e5f7d43c838796b14e7379cd216bdbb28d391429f9fc 6,557,792 bytes, contains sqlite3.exe All four measured by fetching it, not read off a page: the URL answered 200, the digest is of those bytes, and the archive was listed to confirm the binary is in it. What is gone is the FEED, not the third party -- sqlite.org is still somebody else's host. What it is not is a package index: no resolver, no two-phase lookup, and nothing that answers 503 for one concurrent job while serving another, which is the behaviour that made the original failure unreadable. The hash is load-bearing. Without it this would trade a feed that answers errors for a host that could answer anything, and the tests would run against whatever arrived. Kept from #827, deliberately: the presence check. Whatever supplies sqlite3, the step that decides the leg's colour must be the one that says the dependency is missing, not the one that runs the tests. Also corrects a comment #827 left behind, which still said a bump means "one trip to chocolatey, which is the only time the community feed is asked at all". There is no feed to ask. Not measured: whether the GitHub Windows image already ships sqlite3. It is absent from the Windows 2022 and 2025 image manifests, which is the published list rather than a `where sqlite3` on a live runner. * fix(remote): render embedded curl-config paths for the platform (#850) `_remote_http_post_json` writes a curl `-K` config so the request body — which holds the token — never reaches curl's argv. Two of the values it writes are paths: `dump-header` and `data = "@..."`. On Windows/Git Bash those paths are unopenable. MSYS translates POSIX paths to Windows form for a native binary's ARGV, and not for the contents of a file that binary reads. So `/tmp/...` written into the config stays `/tmp/...`, and native curl has no such path. curl fails, and the caller reports the "000" it reports for every failure, with no indication that a path was the problem. `cygpath -m`, not `-w`: -m yields a Windows drive path with FORWARD slashes, and curl's config parser treats a backslash as an escape — so the -w form is re-mangled by curl itself. Measured on the machine, not reasoned about. Gated on `command -v cygpath`, not on an OS name. Where cygpath does not exist the helper returns its argument unchanged, so macOS and Linux take exactly the path they took before — verified directly rather than inferred from a green suite: the helper is a pass-through for /tmp, $TMPDIR and a /private/tmp path on this machine. * fix(remote): dump headers to a file where a real fifo cannot exist (#850) The headers go through a fifo so a broken or hostile server cannot make us buffer an unbounded response: bounded-copy.py enforces the ceiling while the transfer is still running. That mechanism needs a real named pipe. On Windows/Git Bash there is no real named pipe to have. MSYS emulates mkfifo with a .lnk file that only MSYS-aware programs understand, and curl there is a NATIVE binary — it cannot open what mkfifo made. curl fails, and the caller sees the "000" it reports for every failure alike. Where cygpath exists, dump straight to the destination file and skip both the fifo and the copier. Gated on `command -v cygpath` rather than an OS name: what decides is whether a real fifo can be made, and that is what the probe asks. WHAT THIS GIVES UP, on that platform only: streaming enforcement of the header size ceiling. curl's own `max-filesize` still bounds the body; the header dump is what becomes unbounded. Written here rather than left implicit, because it is a real difference between the platforms. Also fixes a deletion the branch form makes dangerous. The cleanup ran `rm -f "$cfg" "$header_fifo"` unconditionally — and on the cygpath path `header_fifo` IS `header_file`, so it deleted the headers this function was asked to produce, before the caller read them. The fifo only exists when a copier was started, so that is what the removal is keyed on now. * fix(hash): SHA-256 without shasum, and no fingerprint it could not compute (#861) connect --e2ee registered the team with the server and then died on the way to the sync engine. The cause is one absent command: shasum is a Perl script that ships on macOS and most Linux and is NOT in Git for Windows' Git Bash. This repository already knew. scripts/lib/hash.sh exists for exactly this reason and its comment describes today's output -- "NOT in Git for Windows Git Bash, where it fails with shasum: command not found, leaving the hash empty". It was written for SHA-1 only, and the four SHA-256 sites never got the same treatment. Derived rather than taken on report: git grep shasum -- scripts/ gives six invocations across five files. Four are SHA-256 in production -- key.sh's two fingerprint helpers, key.sh's previous-snapshot digest, and remote.sh's age-v1 checkpoint -- and all four now go through the new helper. The fifth is hash.sh's own SHA-1 arm, unchanged. The sixth is release/update-cask.sh, which downloads a .dmg and pushes a Homebrew tap; it cannot run on the platform this is about, so it is deliberately left alone. THE LAST RESORT IS DIFFERENT ON PURPOSE. agmsg_sha1 falls through to cksum because its callers name a socket after the digest and need only stability on one machine. These callers need a real SHA-256: one value is the fingerprint two people read to each other over a separate channel, the other decides whether an epoch snapshot is the one it claims to be. A non-cryptographic stand-in there does not weaken the check, it makes it say something untrue. So agmsg_sha256 ends by FAILING, and the comment says why, because the obvious next edit is to make the two consistent. A FINGERPRINT THAT COULD NOT BE COMPUTED IS WORSE THAN NONE. The five print sites read `echo "Recipient fingerprint: $(_key_fingerprint ...)"`. A command substitution that fails inside a simple command's arguments leaves that command's own status alone, so echo succeeded and printed the label with nothing after it -- and the whole point of the string is that two people compare it. Both would have seen the same blank and agreed. Each site now takes the value into a variable of its own first, where a bare assignment's status IS the substitution's and set -e stops. Not `local x="$(...)"`, which puts the status back on the declaration and undoes it. Preflighted at connect, not only in doctor. Every SHA-256 in the e2ee path happens at or after key generation, which is after registration, so the check is asked before either. Reported by doctor as optional, next to age and for the same reason: a team on cipher "none" never computes one. The preflight probes by RUNNING the digest rather than asking command -v. A tool that is installed and broken answers yes to a presence check and no to the question, which is the direction that hurts -- the preflight passes and the digest fails later, which is the shape of this bug again. Left alone deliberately: remote.sh's fail-closed structure. An empty digest made the checkpoint invalid, setup refused, and connect reported "binding recorded, sync engine not started" without falling back to plaintext. That behaviour was correct; it is the digest that was wrong. UNLIKE #850, THIS IS VERIFIED HERE. The behaviour is chosen by command -v, so a PATH holding only the tools we pick reproduces each platform's choice on any platform -- no Windows machine required. Each arm is asserted against a literal digest rather than against the other arms, since three arms can agree on a wrong answer. The suite includes its own negative control (that a restricted PATH really does hide what it claims to) and covers the case that motivated the run-it probe: tools present and failing. * test(hash): the no-tool sandbox was hiding the fallback it exists to forbid The mutation that matters most for this change is "add a cksum arm, to make agmsg_sha256 consistent with agmsg_sha1" -- the exact edit the helper's comment warns the next reader against. Applied, the suite stayed green on the one test named for it. Not because the refusal survived. The sandbox PATH held only `awk`, so the mutant's `cksum` was not found either, the pipeline failed anyway, and the assertion passed for a reason that had nothing to do with the decision under test. It was measuring the box, not the behaviour. cksum is present on every machine this runs on, so that is the shape the test has to run in. With it in the box, the same mutation now turns four cases red, including the one whose name is the claim. Same correction applied to the other sandboxes built for "no SHA-256 tool": the message case, the usable probe, and the require preflight. * test: make the new assertions able to fail (#670 ratchet) CI's enforceable-assertions check counted 642 against a baseline of 638. The four are mine, and they are the shape that check exists for: a non-last `[[ ]]` cannot fail a test on bash 3.2, so it reads as an assertion and behaves as a comment. Three were in the "says which tools were looked for" case, where only the third of three could ever have fired -- the two that name shasum and sha256sum were decoration. The others were the first half of the doctor case and the message half of the connect preflight case, each followed by another assertion. All four are now plain `grep -qF`, which fails the test under bats' errexit. The count is back at the baseline and the suites are green; the mutation rows that rest on these assertions are being re-run, since rewriting an assertion changes the instrument and the earlier matrix was measured with the old one. * docs(key): the no-blank-fingerprint refusal rides on pipefail, so say which line agmsg_sha256 sits in the middle of the fingerprint pipeline and cut and sed are content with the empty input a failed digest leaves them. Without pipefail the pipeline exits 0 with an empty string and the label prints with nothing after it - the exact output the callers were changed to prevent. Measured: replacing set -euo pipefail with set -eu reddens both no-blank -fingerprint cases and nothing else. The comment names that control so the next person shortening line 2 has somewhere to look. * fix(hash): the helper's refusal was the caller's pipefail, and its answer was unchecked Two findings from static review, both one level below where the same shape was already noted. The arms were shasum|awk, so the function's status was awk's, and awk is content with the empty input a failed digest hands it. 'this FAILS' held only because key.sh and remote.sh set pipefail on line 2 - a caller without it got an empty success and carried it into a fingerprint. Each tool now runs as its own substitution with its status checked. And nothing looked at the answer. A tool that exits 0 while printing a warning is not a tool that failed, so 64 lowercase hex is now required, and the usable probe compares against the known digest of its input rather than asking whether anything came back - sixty-four characters that are the wrong sixty-four are exactly what a fingerprint cannot survive. Five cases, and one of them was rewritten after it failed to discriminate: the first no-pipefail case used an empty PATH, where the helper returns 1 outright and pipefail was never involved. A present- but-broken tool is the only path that isolates it. The case pinning the status check separately from the shape check exists because reverting the arms alone changed no result until it was written. * fix(hash): put the known-answer check in the helper, not in a list of entry points Review: the correctness check lived in agmsg_sha256_usable, whose only production callers are remote.sh's doctor and connect preflight. key.sh is its own CLI - generate, show, import and rotate all reach a digest without going near connect - so a tool exiting 0 with a well-formed but wrong digest was accepted by every one of them, and the suite pinned that as expected behaviour. agmsg_sha256 now asks the selected tool a question whose answer is known, once per process, before it will return anything. Keyed on the helper rather than on the callers because the alternative is a list of entry points kept complete by hand, which is what was missed. A key.sh case drives it directly with a lying shim, because a helper -level case cannot fail if a caller stops calling the helper. Also from review: require and doctor said 'none was found' and 'tool on PATH' for a tool that is present and broken. Presence and usability are different questions and these lines answer the second. * fix(hash): a memo that could be preseeded, and that never memoised anything Review, twice over on the same three lines. The self-test's flag was read from the environment, so _AGMSG_SHA256_VERIFIED=1 in an inherited environment meant 'already checked' and skipped it - an undocumented env override of a fail-closed contract, on the hole closed one commit ago. And it saved nothing. Every production call is printf | agmsg_sha256 or x="$(agmsg_sha256 …)", both subshells, so the flag never reached the parent and the self-test ran again on the next digest anyway. Removed rather than made private: a memo whose saving is imaginary is only a surface. The cost is now stated where the code is - one extra digest of a 5-byte input per digest taken, at most three per command. A case pins that no inherited variable can skip it, trying several names, because the property is about the environment and not about one spelling. * docs(hash): the comment kept the contract the code stopped having Three claims left over from the memo, all found in review, none needing code. The block heading still said ONCE PER PROCESS with RUN BEFORE EVERY DIGEST six lines under it - one comment stating two contracts, and a reader has no way to know which one the code has. agmsg_sha256_usable said it costs one subprocess. It calls agmsg_sha256, which self-tests and then digests, so it runs the tool twice. And the worst path was undercounted: key rotate with an accepted rotation to verify takes four digests, not three - the accepted recipient's fingerprint before the three already listed. * test(remote): bind the curl-config path rendering, both fields separately (#850) The stop condition was that reverting the production change leaves CI green. It does now: reverting the renderer turns two tests red, and reverting either single field turns exactly its own test red. The test drives the real _remote_http_post_json, sourced from remote.sh, with two stubs on PATH. What makes the stub worth anything is that it does not just record the config -- it OPENS WHAT THE CONFIG NAMES: cygpath -m yields forward slashes, -w yields backslashes. It must really produce both, or the test cannot tell the two apart, and telling them apart is the whole point of the fix naming -m. curl resolves each embedded path the way a native Windows binary resolves a Windows path back to the same file, and EXITS NON-ZERO when it cannot. A stub that accepts any string as a path would pass on a rendering no curl could open -- which is the defect itself. Four cases: no cygpath both fields byte-for-byte, so macOS and Linux are demonstrably unchanged rather than assumed unchanged cygpath, data rendered mixed cygpath, dump-header rendered mixed either way no backslash anywhere in the config The two field cases were one test until the matrix showed why they cannot be. Reverting the header field and reverting the data field both reddened the same combined assertion, which says 'something is untranslated' and points at neither. They are two effects of one line and regress apart: translate the body and not the header and curl opens the body, fails on the header, and the caller reports 000 -- which reads as a header problem. A fifth case is the control on the stub: replace _remote_curl_path after sourcing so it emits a path nothing can open, and the helper must return 000 -- the code the user actually saw. Without it, every assertion above could be passing on a string that no curl would accept. M0 no mutation 0 red M1 renderer back to passthrough 2 -- DATA and DUMP-HEADER M2 cygpath -m becomes -w 3 -- both fields and the slash assertion M3 header field left untranslated 1 -- DUMP-HEADER M4 data field left untranslated 1 -- DATA Production is untouched. Windows: still unverified here -- the cygpath branch is driven by a stub, and a stub cannot tell you what MSYS does. * test(remote): make the stubs disagree the way the two platforms do (#850) Review found two holes, both in my instrument rather than in the fix. P1-a THE NEGATIVE ARM WAS NOT NEGATIVE. 'without cygpath' prepended a directory holding only a curl symlink and left the host PATH behind it, so production's 'command -v cygpath' would still have found a real one. It passed here because macOS has no cygpath -- the test inherited an absence instead of creating it, and on the one machine this fix is for it would have run the wrong arm. Now an allowlist sandbox, the shape test_helper already uses for path_without_python3. A subtraction cannot express this: on Git Bash cygpath lives in the same directory as mktemp. The first test asserts the sandbox produces BOTH answers, so the capability every other arm rests on is measured rather than assumed. P1-b THE STUB ACCEPTED THE BROKEN PATH. Its resolve() took any absolute POSIX path whatever the platform, so it could not fail on the untranslated /tmp/... that broke the user -- while the file header claimed it opened what curl would open and the commit message claimed it failed where curl would fail. M1 went red on string assertions alone. The claim and the stub now say the same thing. The stub is told which consumer it plays. native-windows opens only Windows paths; posix opens only POSIX ones. Neither accepts both, which is the actual shape of the defect: the same bytes are openable or not depending on who reads them. Both directions have a case, so 'refuses a POSIX path' cannot be read as 'refuses whatever the test needs'. The outcome assertion moved out of the two field tests, and this was forced by the matrix rather than chosen. With the stub now failing on either untranslated field, asserting 200 inside both field tests made both go red for either mutation -- destroying the per-field attribution that splitting them was for. The 200 has its own test, whose job is to fail for any of them. M0 no mutation 0 red M1 renderer back to passthrough 3 -- DATA, DUMP-HEADER, completes M2 cygpath -m becomes -w 4 -- those three and the slash assertion M3 header field untranslated 2 -- DUMP-HEADER and completes M4 data field untranslated 2 -- DATA and completes 7 ok / 0 not ok. Production untouched; the diff is this one test file. Windows: still unverified by me. The cygpath arm is driven by stubs, and a stub cannot tell you what MSYS does. What this closes is 'revert production and CI stays green', not 'it works on Windows'. * test(remote): bind the header sink, and narrow the comment about the probe (#850) Two changes. The tests are the stop condition; the one production line is the ARTIFACT finding from the same review, and it is a comment. TESTS. tests/test_remote_header_sink.bats, 6 ok / 0 not ok, driving the real _remote_http_post_json under an allowlist sandbox PATH. Absences are the whole subject here -- no fifo, no copier -- and an absence is only observable if the instrument can record a presence. mkfifo and python3 are replaced by wrappers that log and then exec the real thing, so nothing about the run changes, and the first test proves the wrappers record at all. A log that was never wired up looks exactly like a call that never happened. no fifo is made marker present no bounded copier is started marker present the caller's header file survives marker present fifo AND copier still used marker absent -- the platforms that are supposed to be unchanged, asserted the fifo is removed, the header file is not marker absent The third is the trap this fix had to avoid: on the marker path header_fifo IS header_file, so a cleanup that removes the fifo deletes the headers the caller asked for, after a request that succeeded. M0 no mutation 0 red M1 marker ignored, always fifo 2 -- no fifo, no copier M2 cleanup removes header_fifo 1 -- header file survives M3 marker branch builds a fifo 1 -- no fifo M4 marker branch starts a copier 1 -- no copier M3 and M4 are why the two absences are two tests. They are separate machinery and are left behind separately: a fifo nobody drains, or a reader blocked on a file that never fills. One test asserting both reddens for either and names neither. COMMENT. The gate said "what decides is whether a real fifo can be made, and that is what the probe asks". It does not ask that. It runs a command -v on cygpath, which is a capability MARKER for an environment where MSYS fifos and a native curl coexist -- nothing here tests whether a real fifo can be made. As written it would have told the next reader that a machine passing the probe had been checked for the property that matters. Behaviour is unchanged: the diff outside tests/ is comment lines. This branch does not carry #851, so paths stay POSIX here and the curl stub is a POSIX consumer. On a real Windows machine this fix alone is not enough -- both are needed, and they compose only on the combined branch. Windows remains unverified by me. * feat(doctor): name the registry locks, and which directory to remove (#865) A lock left behind by a killed process is never broken by anything: nothing expires, nothing sweeps, and acquire waits out its budget and reports 'timed out acquiring registry lock', which describes contention. The operator goes looking for the process holding it; there is none, and no message anywhere names the directory to remove. A machine in that state cannot get itself out. doctor now lists each team's lock with what it records, whether that process is running, and the removal as a line to paste. Three answers, not two. 'held' and 'the holder is gone' are what an operator acts on; 'no holder recorded' is neither - a lock this cannot ask about, written either by a library version that recorded nothing or by a process killed between creating the lock and writing its record. Calling that gone would be a guess, and the guess that costs is the one that says a live lock is dead. It removes nothing, and it does not touch the exit code. A team being locked is not a failed prerequisite - reporting it as one would fail doctor every time somebody is joining - and sweeping automatically is a separate decision with a worse failure mode. Reported for the named team when doctor is given one, matching what its header already says. * fix(doctor): an unusable pid was never asked about, so it is not stale (#865 review) Three findings, all from static review. _agmsg_pid_alive_local returns false for a value it never put to the process table - non-numeric, leading zero, past the POSIX ceiling - and folding that into 'not running' turned 'pid not-a-pid' into a stale verdict with a rm -r beside it. The number is validated with the same ceiling the helper uses before it is asked about, and an unusable one joins 'no holder recorded' under cannot-tell. The sweep used *, which does not match a leading dot, and team names may begin with one - the validator rejects empty, . , .. , a leading -, / , \ and control characters, and nothing else. A named team is now looked at directly, and the sweep adds the two dot patterns. All checks passed. printed under a stale lock and its removal command reads as withdrawing them. Narrowed to All prerequisite checks passed.; the exit code deliberately does not move, so the wording is what carries it. Five existing assertions updated with it. Reporting one lock is its own function now. Both callers share it, so neither has to join paths into a string and read them back - which would mean an unquoted heredoc, running command substitution on a team name, or unquoted word splitting, globbing one. The first attempt at the sweep did build such a string and fed a while-read loop that had no redirection at all, so it read stdin and reported nothing. * fix(doctor): validate the named team before it becomes a path (#865 review) cmd_doctor takes an optional team and, until the direct lookup landed, only ever put it in a header sentence - so nothing had ever validated it. Building TEAMS_DIR/$team/.config.lock from it turned 'doctor ../outside' into a read of a directory outside the store, reported with its records and a rm -r to paste. The sweep it replaced never reached one only because no team was named that. Validated with agmsg_validate_team_name, the same one every other team-taking path uses, and a rejection ends the command rather than falling through to a prerequisites verdict for a question nobody answered. Three cases: a traversal argument reads nothing (with a sentinel .config .lock outside the store to prove the refusal is about reach), a slash is refused, and an ordinary and a dot-leading name still resolve - a refusal that took the legitimate names with it would be the cheapest way to pass the first two. * test(doctor): follow the summary line the lock report narrowed #862 asserted on "All checks passed"; #865's doctor narrowed the summary to "All prerequisite checks passed." so it cannot be read as cancelling a stale lock and the removal command printed above it. The two landed on different lines, so the merge was clean and the assertion was left describing a sentence the code no longer emits. * test(remote): assert the status line, not the stream a killed copier writes into (#850) * test(remote): name the issue the loosened assertion is recording (#869)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part 1 of 3 splitting the Windows connect fix. The three are conceptually separate defects and are split so a mutation in one cannot hide the other. All three together are on
win-connect-fix-850, which is the branch to install for a real walk.Classification: affects existing users — a road that could not be walked becomes walkable. A Windows machine could never be the first to connect a team.
What is wrong
_remote_http_post_jsonwrites a curl-Kconfig so the request body — which holds the token — never reaches curl's argv. Two of the values it writes are paths:dump-headeranddata = "@...".On Windows/Git Bash those paths are unopenable. MSYS translates POSIX paths to Windows form for a native binary's argv, and not for the contents of a file that binary reads. So
/tmp/...written into the config stays/tmp/..., native curl has no such path, and the call collapses to the000this helper reports for every failure alike.Why
cygpath -mand not-w-myields a Windows drive path with forward slashes. curl's config parser treats a backslash as an escape, so the-wform is re-mangled by curl itself. Measured on the machine, not reasoned about.Gating
command -v cygpath, not an OS name. Where cygpath does not exist the helper returns its argument unchanged.What was verified on Windows 11 / Git Bash
Quoted rather than summarised:
What was verified on macOS, and what that is worth
tests/test_remote.bats— 122 ok / 0 not ok, on this branch alone.Green here is not evidence the Windows defect is fixed. The change is capability-gated on a binary this machine does not have, so nothing in this suite takes the new path. What green does establish is the other half: the unchanged platforms are unchanged.
Verified directly rather than inferred from the suite — the helper is a pass-through on this machine:
Not measured
A team with an existing store has not been migrated on Windows. The verified run was a fresh team. What an existing store does on this path is unknown to everyone so far, and this PR does not change that.
Tests, added at
432f2f3b37e4— and what the Windows section above now meansThe Windows verification quoted above was measured on
6e86626c, not on this head. It is inherited, not re-run, and the inheritance is exact rather than argued:Same blob, so the run that produced that quotation still describes the code here. Nothing has been run on Windows since, and the tests below cannot change that.
tests/test_remote_curl_config_paths.bats— 7 ok / 0 not ok. It drives the production_remote_http_post_json, sourced fromremote.sh, under stubs that are capability-paired, because the defect is that the same bytes are openable or not depending on who reads them:native-windowsopens only Windows paths;posixopens only POSIX ones. Neither accepts both, and both directions have a case.-mand backslashes for-w.cygpath -m→-wTwo earlier holes, both in the test rather than the fix, and both found by review: the "without cygpath" arm inherited macOS's absence instead of creating it, and the curl stub accepted the untranslated POSIX path that broke the user — so it could not have failed where curl fails, while the file claimed it did.
This closes "revert production and CI stays green". It does not close "it works on Windows".