feat(send): --stdin and --body-file so bodies bypass the sender shell and argv (#378) - #507
feat(send): --stdin and --body-file so bodies bypass the sender shell and argv (#378)#507Masashi-Ono0611 wants to merge 4 commits into
Conversation
… and argv (fujibee#378) Message bodies passed positionally go through the caller's shell first, so an agent composing the command string can execute $(...) or backticks by accident. Separately, MSYS2 silently truncates a native caller's argv at 8186 chars. Neither is interceptable inside send.sh, because both happen before it runs — but an input path that never touches argv sidesteps both. --stdin / --body-file read the body verbatim. The positional form is unchanged, and -- was added so a body that IS the literal string --stdin/--body-file/--force keeps working. Ambiguous invocations are rejected rather than silently resolved. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Windows / MSYS2 verification — all clear, merge recommendedTL;DR — Verified Environment
This is the same machine and msys2-runtime the #378 truncation evidence was gathered on; the base tree is v1.1.11 minus its last six commits, none of which touch the send/storage path. The positive control below confirms the truncation still reproduces on this exact tree before the new flags are exercised. One version-specificity note: the 8,186-char threshold itself is a property of this msys2-runtime build's bats regression
Positive controlSending an 8,187-char body via the positional argument from a native caller (Node.js Matrix results (summary)Six body variants (at/above the 8,186-char boundary, multibyte, trailing newline, shell metacharacters) tested across Node.js, PowerShell, and Bash callers:
Two Windows caveats we hit while testing, both caller-side and worth a line in the docs:
With those two caller-side details handled, (If you inspect our raw logs: a few Field-incident replayWe replayed the exact body from the original truncation incident (8,324 chars / 12,234 bytes):
The real body content is non-public; only lengths, byte counts, and Conclusion
Test harness and raw logs are available on request. |
|
This is fantastic — thank you so much! A byte-exact replay of the original 8,324-char incident body plus the full bats suite on real MSYS2 is exactly the verification this PR needed, and it's coverage I couldn't have produced myself without a Windows environment. Really grateful you took the time to run it end to end. |
|
Thanks for this. The direction is right: reading the body from a file descriptor instead of argv is the correct first step, and we intend to take it as an additive mitigation. The test pass is unusually thorough — checking that the trailing-newline test actually fails when the verbatim read is replaced with a line-oriented one, rather than trusting the green, is the kind of evidence that makes a review short. One scoping note before the requested changes. This PR will not close #378. #378 is about the positional body being a hazard at all, and that is only resolved when the positional form is retired; adding a safe alternative alongside it narrows exposure without removing it. We will keep #378 open and link this PR as the first stage. Three changes we would like. B1 — flip the canonical form in the docs, not just add to it. Right now B2 — a NUL byte is silently truncating, so "verbatim" does not hold yet. B3 — the backward-compatibility claim needs correcting. The PR body says the positional form is unchanged and that no caller has to migrate. That is not quite the case: a caller that was passing a body which is literally Two non-blocking notes. (1) The test named " (2) A few places describe the change in terms that blend "the new path avoids the hazard" with "the hazard is gone". Since the positional form remains available and remains the shape most existing callers use, please keep those statements to what the safe path does, and leave the removal claim for the stage that actually removes it. On the shape of the interface itself: two separate flags is fine and we are not asking you to unify them into a |
…ating (fujibee#378) Addresses the review on fujibee#507. B1 — make --stdin/--body-file the canonical spelling rather than an extra option alongside the positional form. Every first-party example now uses it and marks the positional form deprecated in the same place: README.md, README.ja.md (which the first commit left out of sync), SKILL.md, all nine driver templates, the grok-build delivery rule, the codex bridge's reply prompt, both session-start.sh monitor directives, and docs/building-on-agmsg.md. Each agent-facing surface states that a body an agent composes MUST NOT use the positional form. This is stage (1) only: no runtime deprecation warning, nothing removed. B2 — a NUL byte was silently truncating the body and still exiting 0, so "verbatim" only held for NUL-free input. `read -d ''` exits 0 only when it found its delimiter and non-zero at EOF, so that status now distinguishes a NUL-bearing input from a complete read, and such input is refused with nothing stored. The check runs before the empty-input check because a leading NUL yields an empty value AND a zero exit. Also: the "-- keeps a body that is literally --force working, and --force after it still applies" test never passed a trailing --force. It is split into a narrowed test plus a pair that sends to an unregistered recipient (refused unless force mode is on) with a control asserting the same call without the trailing flag fails on the roster check specifically. Examples use the delimiter AGMSG_BODY, not EOF: a body containing a bare EOF line would end the heredoc early and run the rest as shell commands. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Thanks — the scoping note is the useful part, and I've taken it as written: this PR is stage (1) only, B1 — canonical form flipped, not appended to. Two judgment calls in there worth your eye:
B2 — NUL now fails closed. B3 — compatibility claim corrected. You're right, and the PR body no longer says otherwise. It now states that a caller whose body is literally (1) The test now exercises the half of its name it was skipping: it sends One thing my own review pass caught while making the heredoc form canonical, which I fixed rather than shipped: the examples were delimited by (2) Reworded. The script header, the README/JA paragraphs, the test-file comment and the PR body now say what the safe path does — a body sent that way meets neither hazard — and state plainly that the hazard is not gone, because the positional form still exists and still carries both. The removal claim is left for stage (3). One thing I found while sweeping and deliberately did not change, since it is code rather than an example and so outside stage (1) as you scoped it: two first-party callers still pass a body positionally — On the interface shape: understood, keeping the two flags as they are. |
…our bodies Second review pass (agmsg peer + self-audit) found three description/reality gaps of the same kind B3 was about. - The MUST NOT was missing from the codex bridge's runtime prompt — the one text an agent reads before composing a reply — where it existed only as a source comment. Emit it there. Also add the "deprecated" marking to docs/building-on-agmsg.md and both session-start.sh directives, which had the canonical form but not the wording B1 asks for in the same places. - The compat break is four literal bodies, not three: `--` itself was an ordinary positional body before (only argument 5 was inspected) and is now consumed as the terminator. Migration form is `-- --`. Pinned with a test. - send.sh's header claimed the positional form "keeps working for now so no existing caller breaks", contradicting the break documented alongside it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Corrections to my previous comment — a second review pass found three things I got wrong, all of the same "description does not match what happens" kind B3 was about. The break is four literal bodies, not three. I missed I named the wrong callers. B1 was not actually complete where it matters most. I had claimed the MUST NOT was on every agent-facing surface. It was not on the codex bridge's runtime prompt — the one text an agent actually reads before composing a reply — where it existed only as a source comment. It is now emitted in the prompt. I also added the deprecation marking to I also dropped a line in Nothing changed in B2, the |
…tions Follow-through on the fourth affected literal body. The compat escape hatch is described in the README (both languages), SKILL.md and all nine driver templates, and each still listed only `--stdin` / `--body-file` / `--force` — the three cases known before `--` itself turned out to be affected. Those are exactly the places a caller looks when their body is one of these, so they now list all four and name `-- --` as the form for a body of `--`. Docs only; no behaviour change. The `--` case itself is already implemented and pinned by a test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
3 participants
Part of #378.
Adds
--stdinand--body-file <path>tosend.sh, so a message body can be read verbatim instead of arriving through the sender's shell, and makes that the canonical way to send in every first-party example.Why this can't be fixed inside
send.shwithout a new input pathBoth hazards in the issue happen before
send.shever runs:send ... "x $(cmd)"executescmdin the caller's shell.send.shreceives the result, not the source, so it has nothing to intercept. (Note the precise mechanism: a positional argument is not re-evaluated; a body already containing literal backticks is safe. The problem is agents composing the command string.)build_argv→globify→glob.ccMAXPATHLEN8192 path, silently cutting bodies at 8,186 characters with no error. Their repro harness and control experiments are what make this diagnosable rather than folklore — thank you for that work.An input path that never touches argv sidesteps both.
Interface
Design decisions worth flagging:
--stdin/--body-fileis now the spelling inREADME.md,README.ja.md,SKILL.md, all nine driver templates, the grok-build delivery rule file, the codex bridge's reply prompt, thesession-start.shmonitor directives, anddocs/building-on-agmsg.md. The positional form is marked deprecated in the same places, and each agent-facing surface states that a body an agent composes MUST NOT use it. This is stage (1) of the three-stage migration: (1) flip the docs and examples, (2) emit a deprecation warning on positional use, (3) remove it in the next breaking change. Only stage (1) is in this PR — there is no runtime warning here and nothing is removed.--is its migration syntax, not a reason it doesn't exist. Four literal bodies are affected.--itself was a valid body before (only argument 5 was inspected) and is now consumed as the terminator; its migration form is-- --.--stdinand--body-filenow have that argument read as a mode selector.--forceis now rejected outright — previously it arrived as the body, because only argument 5 was checked for the flag (send.sh t a b --forcesetBODY=--force,FORCE=0; verified against the pre-PR script). All four must be rewritten assend.sh <team> <from> <to> -- <body>to get the old behaviour. That is a real break for those callers — narrow, but real. Every other positional body is unaffected and keeps working unchanged.--stdin,--stdinplus--body-file,--body-filewith no path or with a flag-shaped path, empty stdin, an empty or unreadable file — each errors with a message naming the problem, rather than sending a silently-empty message.IFS= read -r -d ''plus a sentinel around the SQL-escape command substitution (command substitution otherwise eats trailing newlines). This is deliberate: a heredoc contributes the newline immediately before its closing delimiter.read -d ''exits 0 only when it found its delimiter — here NUL — and non-zero at EOF, which is the normal complete read. That status is now checked instead of discarded, so NUL-bearing input is rejected with a clear error and nothing is stored. Without it the shortened body was saved and delivered with a success exit, which meant "verbatim" only held for NUL-free input. The check runs before the empty-input check, because a leading NUL yields an empty value and a zero exit and would otherwise be misreported as empty stdin.Scope limits I'd rather state than have you find
<message>cannot reach this case at all, since argv strings cannot carry NUL either.send.sh's argv — and the SQL itself already went tosqlite3over stdin, so it doesn't cross a second argv boundary either. But I have no Windows machine, so I'm not claiming the field incident is fixed, only that the mechanism it was attributed to is bypassed.scripts/windows/dispatch.sh:176andapp/src-tauri/src/agmsg.rs:613. Both hand the body to a process spawn rather than a shell, so the evaluation hazard does not apply to them; the argv one still does, anddispatch.shis the Windows path send.sh: add a stdin/file input mode so message bodies survive shell quoting #378 describes. (scripts/despawn.sh:101also sends positionally but with the fixed literalctrl:despawn, so it is not exposed.) They are code, not examples, so they are outside stage (1) as scoped — but after this PR the docs tell agents an agent-composed body must not use the positional form while agmsg's own Windows dispatcher and desktop app still do. Happy to migrate them here if you'd rather not carry that gap, or to file it as a follow-up.Verification
bats tests/→ all green.tests/test_messaging.batsis 50 ok, 0 not ok, exit 0 (was 44): 21 tests for this feature, including byte-identical bodies containing backticks and$(...)via both input modes, a >8192-char body, explicit trailing-newline preservation,--forcecomposition, the--compat cases, three NUL-rejection paths, and the remaining rejection paths.IFS= read … || truemakes exactly the three NUL tests fail (exit 1). Moving the NUL check after the empty check makes only the leading-NUL test fail — which is what pins the ordering. Replacing the verbatim read with a line-orientedreadmakes the trailing-newline test fail; neutralising the--case arm makes the compat tests fail. All restored and re-confirmed green.--+--forcetest now exercises its own name. It sends to an unregistered team/recipient, which the roster check refuses unless force mode is on, so the send succeeding is what proves the trailing--forcewas still parsed after--consumed the body. A control test asserts the same call without the trailing--forceis refused — otherwise the success could just mean the roster check never ran.codex exec(gpt-5.6-sol, high). It flagged the--compat hole (fixed) and the NUL caveat (now a rejection). It also flagged a possible second argv boundary at thesqlite3call and aseqportability problem — I checked both: the SQL is already piped via stdin, andseqis present at/usr/bin/seqon macOS 26.5. Neither needed a change.