fix(#777): the remaining argv-to-stdin sites, plus a Windows CRLF row-separator bug found verifying them by joelmitz · Pull Request #991 · fujibee/agmsg

joelmitz · 2026-08-25T15:30:47Z

Fixes the remaining call sites from #777 (inbox.sh, check-inbox.sh, watch.sh,
drivers/types/codex/watch-once.sh, and the "Not measured" storage_read_cursor_consume
site) by passing the SQL on stdin instead of as one argv element — the same shape #899
already applied to history.sh. Also fixes a second, independent bug found while verifying
this on real Windows hardware: sqlite3.exe terminates multi-row output with \r\n, which
silently corrupts mark-as-read.

Part 1: the remaining #777 sites

Same construction, same fix as #899: _arr interpolated into json_each('<...>') and passed
to sqlite3 as one argv element. Fixed the same way — write the statement to a mktemp file
via printf, feed it to agmsg_sqlite on stdin.

scripts/inbox.sh (was :51-56)
scripts/check-inbox.sh (was :258-263)
scripts/watch.sh (was :701-711) — the monitor delivery path
scripts/drivers/types/codex/watch-once.sh (was :130-131)
scripts/drivers/storage/sqlite.sh's storage_read_cursor_consume — the site the issue's
"Not measured" section named but didn't confirm. Builds one BEGIN IMMEDIATE; ...; COMMIT;
block per delivered id and concatenates them into one argv element. Reproduced directly:
a 400-id batch (~160,000 bytes) returns runtime_error and the cursor stays unmoved
unpatched; ok and the cursor advances to 400 patched.

Cleanup style differs per site because agmsg_sqlite/watch.sh traps don't stack:
inbox.sh/check-inbox.sh use trap ... EXIT HUP INT TERM; watch.sh and
storage_read_cursor_consume (a shared function called every poll from watch.sh's
long-lived loop, which already owns a permanent trap cleanup EXIT) use an explicit rm -f
on both paths instead, to avoid clobbering that trap.

Investigated, not changed:

remote.sh — read every agmsg_sqlite_mem call site; each operates on a small,
roster/config-bounded document (agents map, previous_bindings, a pull-bootstrap
response), none scale with message/unread count. The issue's remote.sh:786 reference no
longer matches a call site in the current file.
drivers/storage/sqlite-sync.sh's storage_sync_apply_pull outcome-report query (the
outcome_ids site the issue's remote pull: 79 minutes of silence, then 'Argument list too long' in the apply phase (Windows native) — same cause as #777 #882-linked comment flagged as the most severe — a
Windows-native pull that commits then crashes) — already goes through
_sqlite_data_stdin under remote pull: 79 minutes of silence, then 'Argument list too long' in the apply phase (Windows native) — same cause as #777 #882. No change needed here.

Part 2: a second bug, found verifying Part 1 on real Windows hardware

Independent of argv length — reproduces on unpatched main at message counts far under any
ceiling, on this same Windows machine.

Symptom: inbox.sh on a 100-message backlog marked only 1 message read per run (100
runs to clear it). Traced to events rows with message_read.msg_id ending in a stray \r.

Cause (confirmed on the wire, mechanism is a hypothesis): sqlite3.exe here (measured
3.53.4) ends each row of a multi-row result with \r\n, not \n:

$ agmsg_sqlite ":memory:" "SELECT 1; SELECT 2; SELECT 3;" | od -c
0000000   1  \r  \n   2  \r  \n   3

ROWS=$(agmsg_sqlite ...) strips only the trailing newline of the whole captured output,
so every row but the last keeps a \r glued to its final field. IFS=$'\x1f' read doesn't
split on \r, so it rides into the field value — typically an id, since every row-building
SELECT in this codebase puts id/cursor/at last and body earlier. This is a different
mechanism from #102/#143 (sqlite3 ≥ 3.50's own caret-notation escaping, already handled by
the existing -escape off probe) — that one reproduces on Linux too; this CRLF ending does
not, on either machine tested.

Fix: agmsg_sqlite() now pipes through sed $'s/\r$//' — normalizing only a \r
immediately before the line-ending \n, not every \r in the stream. Deliberately not
tr -d '\r' (already used by _sqlite_data/_sqlite_data_stdin, which wrap this function):
that would also delete a \r that is a message body's own content (char(13) isn't replaced
the way char(10) already is in every row-building SELECT here).

_agmsg_sqlite_recording() — the path taken whenever AGMSG_SQLITE_OUTCOME_FILE is set,
i.e. every call through the sync driver (storage-sync-driver.sh) — bypassed this via an
early return and needed the same fix, restructured because the original >&3 3>&- fd
passthrough can't have sed spliced into it: stderr now goes through a temp file, exit code
read from ${PIPESTATUS[0]}. First attempt at this guarded the pipeline with | sed ... || true, which silently turned every busy/failed result into rc=0 under a caller that already
has set -o pipefail active (which storage-sync-driver.sh does) — caught by this repo's own
existing busy-timeout contract test, not by inspection. Fixed by keeping the original code's
if-guard shape, which reads PIPESTATUS inside each branch before anything else executes.

Confirmed on Linux (this fix's own dev machine's sqlite3 never emits \r) with a
PATH-shimmed sqlite3 wrapper (tests/test_sqlite_crlf.bats, 10 cases) that appends a
synthetic \r before every line of the real binary's output — deterministically reproducing
the Windows row separator. Confirmed as a real regression test by running it against
pre-fix code via git stash.

Verified on three platforms

	Linux (this dev machine)	WSL	Windows (native, Git Bash)
Part 1 (#777 remaining sites)	fixed, tested	fixed, tested	fixed, tested on real hardware where it originally reproduced
Part 2 (CRLF)	n/a — doesn't reproduce here	n/a — doesn't reproduce here	fixed, confirmed on real hardware (`od -c` before/after)

Full test sweep at final state (test_sqlite_crlf.bats, test_inbox.bats, test_watch.bats,
test_watch_once.bats, test_remote_sync.bats, test_sync_cipher.bats,
test_sqlite_sync_jq_binary.bats, test_messaging.bats, test_delivery.bats,
test_storage_contract.bats, test_team.bats) is green on Linux and WSL. On Windows, one
unrelated pre-existing failure in test_remote_sync.bats (a busy-timeout contract test
sensitive to this machine's process-startup latency) reproduces identically on unpatched
main, so it is not a regression from this change; test_watch.bats's #777 case is skipped
on Windows by an existing, unrelated constraint (#182).

Two-machine, network-connected remote pull/push was not exercised end-to-end; the sync
driver's contract tests (apply/reconcile/pull/reprocess/recording, 44 cases) were run in
isolation instead, all against a throwaway team/store — happy to add a live two-machine pass
if useful.

Not covered here

The CRLF fix doesn't have an issue number yet — flagging it here rather than filing separately
first, since it was found while fixing #777 on the same call paths. Happy to split it into
its own issue/PR if you'd rather review them independently.

Disclosure

Investigated and drafted by Claude via Claude Code (multiple sessions: Linux, WSL, and
Windows native), prepared with the user's authorization, following the same convention as
the rest of #777 and #899. Part 1 and Part 2 both went through this project's own commit-review
process (an independent Codex reviewer, separate from the implementer) before being proposed
here, in addition to the cross-platform verification described above.

inbox.sh, check-inbox.sh, watch.sh, and codex/watch-once.sh each embedded a JSON array of unread/undelivered messages into ONE argv element for `sqlite3 ':memory:' "<embedded SQL>"`. That array grows with every message a team/pair accumulates, so it eventually exceeds the OS's per-argument ceiling (Linux MAX_ARG_STRLEN=131,072 bytes; smaller still on Windows/macOS) and sqlite3 fails to exec with "Argument list too long". Where the failure was swallowed by `2>/dev/null || true` (check-inbox.sh, watch.sh, watch-once.sh), the backlog that triggered it never shrinks on its own, so the same statement fails identically on every following poll -- a stall, not a one-off skip. In watch.sh this also meant the read cursor never advanced. Same fix in all four: write the SQL statement to a temp file with printf (a bash builtin, so it never execs) and feed sqlite3 the statement on stdin instead. Mirrors drivers/storage/sqlite-sync.sh's own fujibee#882 fix (`_sqlite_data_stdin`) and scripts/history.sh's existing fujibee#777 fix, which this change was modeled on. Per-file notes: - inbox.sh / check-inbox.sh: straightforward mktemp+trap+stdin swap, matching history.sh's shape. check-inbox.sh's version lives inside a `$( set -euo pipefail; ... )` subshell, so its EXIT trap is scoped to that subshell and never touches the outer script. - watch.sh: no trap added here on purpose -- the script installs `trap cleanup EXIT` and `trap 'exit 0' INT TERM HUP` once near the top, and bash traps do not stack, so a loop-local trap would silently replace those for the rest of the long-lived polling process. The temp file is removed explicitly on every path instead, with a fail-open guard (empty ROWS) if mktemp itself fails. - watch-once.sh: `|| continue` on mktemp failure, matching the existing per-pair `continue` used when a team's storage read fails, so one pair's error doesn't end the whole subscription's poll. Also reviewed scripts/remote.sh (all 30 agmsg_sqlite_mem/agmsg_sqlite call sites) and scripts/drivers/storage/sqlite-sync.sh's storage_sync_apply_pull outcome-report query per the issue's other two leads: - remote.sh: every call operates on a small, roster/config-bounded JSON document (agents map, previous_bindings, members list, or a pull/connect control-plane response) -- none scale with message or unread count. The actual message-sync engine is a separate Node process (internal/remote-sync.mjs) that never shells out to sqlite3 this way. No fix applied here; the issue's "remote.sh:786" line reference no longer corresponds to a matching call site. - sqlite-sync.sh: already fixed under fujibee#882 (`_sqlite_data_stdin`, scripts/drivers/storage/sqlite-sync.sh:1301 / scripts/drivers/storage/sqlite.sh:59). No change needed. Adds a shared bulk_send_direct() test helper (tests/test_helper.bash) and one regression test per script (test_inbox.bats x2, test_watch.bats, test_watch_once.bats): 100 messages of ~2000 bytes each (~200,000 bytes of body alone, past the measured 131,072-byte Linux ceiling), sent via storage_send directly so building the fixture itself never has to exec anything with the whole backlog as one argument. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GUJZpBvin7hQE6QFWvSatA

Follow-up to 483e7fd. The issue's "Not measured" section named this exact function (drivers/storage/sqlite.sh:252-291 at the time) as a suspected but unconfirmed instance of the same defect, and it was left out of the original request's target list by mistake. storage_read_cursor_consume built one INSERT/UPDATE block per delivered id, concatenated them into a single `$sql` variable, and passed the whole "BEGIN IMMEDIATE; ...; COMMIT;" statement to `agmsg_sqlite` as ONE argv element. Confirmed on real Windows hardware: 97 ids built a 38,897-byte statement and CreateProcess refused it outright (Windows' ceiling is 32,767 characters, well under Linux's own MAX_ARG_STRLEN=131,072 bytes) -- and the failure was masked further, surfacing only as this function's ordinary `runtime_error`/13 return, never as a visible "Argument list too long". This is why inbox.sh's display fixed itself (483e7fd) but mark-as-read silently kept failing on Windows: the display query and the mark-as-read query are two different statements with two different sizes, and only the first one was fixed. Also reproduced directly on Linux in this session (not just inferred from the Windows report): with the pre-fix code and a 400-id batch (~160,000 bytes, safely past the 131,072-byte ceiling), the OLD function returned `runtime_error` and left the read cursor at 0; the fixed function returns `ok` and advances the cursor to 400 with the identical input. So the defect and the fix are both confirmed on Linux, not only inferred from the Windows measurement. Same fix as 483e7fd's four scripts and sqlite-sync.sh's own fujibee#882 fix: write the statement to a temp file with printf (a bash builtin, so it never execs) and feed `agmsg_sqlite` the statement on stdin instead. No trap added, on purpose: this is a shared library function called every poll from watch.sh's long-lived loop, which installs its own permanent `trap cleanup EXIT` / `trap 'exit 0' INT TERM HUP` near the top of that process. Bash traps do not stack, so a trap set and cleared in here would replace watch.sh's for the rest of its life the first time this function ran -- the exact mistake 483e7fd's own watch.sh fix identified and avoided one step earlier in the same call chain. The temp file is removed explicitly on every path instead. Verified: watch.sh's read-cursor advancement goes through this same function, so its fujibee#777 regression test (test_watch.bats) continues to pass and now exercises a real fix at that layer too -- previously it only had margin because 100 messages' consume-statement stayed under Linux's ceiling. No new tests added per this task's scope; ran the existing fujibee#777-tagged tests (test_inbox.bats, including the mark-as-read assertions) plus a broad regression sweep (test_watch.bats, test_watch_once.bats, test_delivery.bats, test_messaging.bats, test_storage_contract.bats, test_team.bats, test_remote_sync.bats -- 374 tests total across those runs) with zero failures. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GUJZpBvin7hQE6QFWvSatA

… yet) Independent of fujibee#777 -- found by claude-win while verifying fujibee#777's fix on real Windows hardware, not by argv-length. No issue number assigned; not filed upstream yet (fork-only per explicit instruction). Bug: Windows' sqlite3.exe (measured: 3.53.4) ends each row of a multi-row result with \r\n, not \n -- confirmed with `od -c` on real Windows hardware. `ROWS=$(agmsg_sqlite ...)` only strips the trailing newline of the WHOLE captured output, so every row but the last keeps a \r stuck to its final field (typically an id, since this codebase's row-building SELECTs put id/cursor/at last and body earlier). `IFS=$'\x1f' read` does not split on \r, so it rides along into the field value, and storage_mark_read_batch's ids then match no real msg_id. Measured on Windows: a 100-message backlog lost 99 of 100 mark-as-read updates in one inbox.sh run. Root cause is stated as a hypothesis, not a fact, per this repo's discipline on unverified claims: the CRLF-on-the-wire is confirmed; *why* (suspected: the Windows CRT's stdio text-mode LF->CRLF translation) is not, and the fix does not depend on which mechanism it turns out to be. This is a separate defect from fujibee#102/fujibee#143 (sqlite3 >= 3.50's own caret-notation escaping of control bytes, fixed by the existing `-escape off` probe) -- reproduces on origin/main before fujibee#777 too, at message counts far under any argv ceiling, and is unrelated to argv size entirely. Fix: agmsg_sqlite() now pipes sqlite3's stdout through `sed $'s/\r$//'`, normalizing ONLY a \r immediately before the line-ending \n. Deliberately not `tr -d '\r'` (already used by _sqlite_data/_sqlite_data_stdin in drivers/storage/sqlite.sh, which wrap calls to this same function): that deletes every \r anywhere in the output, including one that could be a message body's own content -- char(13) is not replaced the way char(10) already is in every row-building SELECT in this codebase, so a body ending in a genuine \r is a real, reachable byte sequence this fix must not corrupt. `sed`'s `$` anchor matches only end-of-line, leaving a mid-row \r untouched. Wrapped in a subshell with its own `set -o pipefail` (same shape as _sqlite_data/_sqlite_data_stdin) so the pipeline's exit status is sqlite3's, not sed's, without changing pipefail for the calling script. Verification (this machine's sqlite3 3.45.1 never emits \r on its own -- confirmed directly with `od -c` -- so the bug and the fix both needed a stand-in for Windows' sqlite3.exe to exercise on Linux): - Added tests/test_sqlite_crlf.bats with a PATH-shimmed `sqlite3` wrapper (mirrors test_watch_once.bats's slow-awk shim technique: the real binary's path is resolved before the shim directory is ever on PATH, and baked into the wrapper as a literal exec target) that appends a synthetic \r before every line of the real sqlite3's output, reproducing the reported \r\n row separator deterministically. - Ran the new 4-test file against the pre-fix code (temporarily via `git stash`) to confirm it is a genuine regression test: 3 of 4 fail without the fix (the CRLF-stripping unit test, the mid-body-CR preservation test, and the inbox.sh 20-message full-backlog test), and all 4 pass with it. - Ran the existing fujibee#777 suite (test_inbox.bats, test_watch.bats, test_watch_once.bats -- 45 tests) plus a broad sweep (test_messaging.bats, test_delivery.bats, test_storage_contract.bats, test_team.bats, test_remote_sync.bats -- 363 more tests) with the fix in place: 408 tests total, zero failures, on top of the 4 new ones. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GUJZpBvin7hQE6QFWvSatA

…path Addresses a codex commit-before-review BLOCKER on the prior CRLF fix (1d3fd90): AGMSG_SQLITE_OUTCOME_FILE (set only by the sync driver adapter, scripts/internal/storage-sync-driver.sh) makes agmsg_sqlite() early-return into _agmsg_sqlite_recording() instead of the path that fix touched, so Windows remote pull/push (which always goes through the sync driver adapter) kept the same trailing-CR corruption on every multi-row SELECT the recording path handles. Fix: _agmsg_sqlite_recording() gets its own copy of the same `sed $'s/\r$//'` normalization, applied only to stdout. Its stderr capture had to change shape to make room: the original piped sqlite3's own stdout directly to the real fd 1 via an `>&3 3>&-` trick with no process in between, and inserting `sed` means stdout now goes through an actual pipe, so a temp file replaces the `err=$(...)` command-substitution capture for stderr, with the exit status read from `${PIPESTATUS[0]}` (sqlite3's, not sed's). Per the review's own explicit requirement, this does not change: fd-3-style stdout passthrough semantics (data untouched beyond the CR fix), stderr classification (verbatim re-emission, same ok/busy/failed word written to AGMSG_SQLITE_OUTCOME_FILE), or the exit code contract. Caught a second, more serious bug while implementing this, via codex's own requested test additions (not by inspection): the first attempt guarded the new pipeline with `pipeline | sed ... || true` (mirroring the non-recording path's own guard against `set -e`), but this call site ALSO reads `${PIPESTATUS[0]}` afterward -- and PIPESTATUS is overwritten by the next command the shell runs, of ANY kind. Whenever the caller already has `set -o pipefail` active (storage-sync-driver.sh sets it at its own top, and _agmsg_sqlite_recording is a plain function call that inherits it), the pipeline's own exit status became sqlite3's non-zero one, `|| true` therefore ran `true`, and reading PIPESTATUS immediately after read back `true`'s (0) instead of sqlite3's real one -- turning EVERY busy/failed call into a silently reported "ok". This is not hypothetical: it broke test_remote_sync.bats's real busy-timeout contract test ("a store another writer holds is busy (11), not a failed check (13)"), which exercises the real adapter end to end, while every synthetic PATH-stub test I had written first (run from a plain `bash -c` with no pipefail) stayed green, because none of them replicated a pipefail-active caller. Fixed by going back to the same `if`-wrapped shape the original code already used (a command tested by `if` is exempt from `set -e` regardless of pipefail, and reading PIPESTATUS inside the if/else branches, before anything else runs, keeps it correct either way). Tests added to tests/test_sqlite_crlf.bats (existing PATH-shim techniques, extended with a second, fully synthetic sqlite3 stub for deterministic ok/busy/failed classification without real lock-contention timing): - recording-path CRLF stripping and mid-body-CR preservation (mirrors the two non-recording-path tests) - ok / busy / failed classification, outcome-file content, exit code, and verbatim stderr all unchanged (per the review's explicit ask) - busy classification survives a caller with -e/pipefail already on -- the exact caller shape that broke, confirmed by temporarily reintroducing the `|| true` bug and observing this new test (only this one) fail, then restoring the fix and confirming all pass Verification: reintroduced the `|| true` bug locally and confirmed (a) the real busy-timeout contract test in test_remote_sync.bats fails exactly as codex's report predicted, matching the observed symptom, and (b) the new test 10 in test_sqlite_crlf.bats is the only one of the ten that catches it. Restored the fix and re-ran: tests/test_sqlite_crlf.bats (10), test_inbox.bats/test_watch.bats/test_watch_once.bats (45), test_remote_sync.bats/test_sync_cipher.bats/test_sqlite_sync_jq_binary.bats (59, including the real busy-timeout contract test), and a broad sweep (test_messaging.bats/test_delivery.bats/test_storage_contract.bats, 240) -- 354 tests total, zero failures. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GUJZpBvin7hQE6QFWvSatA

joelmk326 and others added 4 commits August 25, 2026 17:20

Navigation Menu

fix(#777): the remaining argv-to-stdin sites, plus a Windows CRLF row-separator bug found verifying them - #991

fix(#777): the remaining argv-to-stdin sites, plus a Windows CRLF row-separator bug found verifying them#991
joelmitz wants to merge 4 commits into
fujibee:mainfrom
joelmitz:fix/777-argv-payload-stdin

joelmitz commented Aug 25, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

joelmitz commented Aug 25, 2026

Part 1: the remaining #777 sites

Part 2: a second bug, found verifying Part 1 on real Windows hardware

Verified on three platforms

Not covered here

Disclosure

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants