From 135a8836027cda0d7536920a82940111edfda281 Mon Sep 17 00:00:00 2001 From: mkmariko Date: Tue, 25 Aug 2026 17:20:20 +0900 Subject: [PATCH 1/6] fix(#777): pass growing unread/backlog SQL via stdin, not argv 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:' ""`. 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 #882 fix (`_sqlite_data_stdin`) and scripts/history.sh's existing #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 #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 Claude-Session: https://claude.ai/code/session_01GUJZpBvin7hQE6QFWvSatA --- scripts/check-inbox.sh | 31 ++++++++++--- scripts/drivers/types/codex/watch-once.sh | 23 ++++++++-- scripts/inbox.sh | 30 ++++++++++--- scripts/watch.sh | 55 ++++++++++++++++++----- tests/test_helper.bash | 28 ++++++++++++ tests/test_inbox.bats | 39 ++++++++++++++++ tests/test_watch.bats | 47 +++++++++++++++++++ tests/test_watch_once.bats | 21 +++++++++ 8 files changed, 246 insertions(+), 28 deletions(-) diff --git a/scripts/check-inbox.sh b/scripts/check-inbox.sh index b14939137..64795abe3 100755 --- a/scripts/check-inbox.sh +++ b/scripts/check-inbox.sh @@ -283,13 +283,30 @@ for team in "${TEAM_LIST[@]}"; do # _sqlite_sync_lit_into in sqlite-sync.sh, which documents the same hazard. _AGMSG_SQ="'" _arr="[$(printf '%s' "$UNREAD_JSONL" | paste -sd, -)]" - agmsg_sqlite ':memory:' " - SELECT json_extract(value,'\$.from') || char(31) || - replace(replace(json_extract(value,'\$.body'), char(10), '\n'), char(9), '\t') || char(31) || - json_extract(value,'\$.at') || char(31) || - json_extract(value,'\$.id') - FROM json_each('${_arr//$_AGMSG_SQ/$_AGMSG_SQ$_AGMSG_SQ}'); - " + # #777: this team's unread backlog grows with every message sent to it, so + # interpolating it into ONE argv element eventually exceeds the OS's + # per-argument ceiling (Linux MAX_ARG_STRLEN=131,072 bytes; smaller still + # on Windows/macOS) and `agmsg_sqlite` fails with "Argument list too + # long" -- every single poll, since the backlog that triggered it never + # shrinks on its own. Pass the statement on stdin instead, mirroring + # drivers/storage/sqlite-sync.sh:1301 (`_sqlite_data_stdin`, #882) and + # history.sh/inbox.sh: printf is a bash builtin, so writing a large value + # to a temp file never execs and can hit neither that ceiling nor argv's + # at all. The temp file is scoped to THIS subshell -- its EXIT trap fires + # when the subshell itself exits (success, `exit 98`/`exit 13` above, or a + # signal), never touching the outer script's own traps. + _agmsg_ci_sql=$(mktemp "${TMPDIR:-/tmp}/agmsg-checkinbox-rows.XXXXXX") || exit 13 + trap 'rm -f "$_agmsg_ci_sql"' EXIT HUP INT TERM + { + printf "%s\n" "SELECT json_extract(value,'\$.from') || char(31) ||" + printf "%s\n" " replace(replace(json_extract(value,'\$.body'), char(10), '\n'), char(9), '\t') || char(31) ||" + printf "%s\n" " json_extract(value,'\$.at') || char(31) ||" + printf "%s\n" " json_extract(value,'\$.id')" + printf "FROM json_each('" + printf '%s' "${_arr//$_AGMSG_SQ/$_AGMSG_SQ$_AGMSG_SQ}" + printf "');\n" + } > "$_agmsg_ci_sql" + agmsg_sqlite ':memory:' < "$_agmsg_ci_sql" ) _rc=$? set -e diff --git a/scripts/drivers/types/codex/watch-once.sh b/scripts/drivers/types/codex/watch-once.sh index 415c2b86b..1c23f9d06 100755 --- a/scripts/drivers/types/codex/watch-once.sh +++ b/scripts/drivers/types/codex/watch-once.sh @@ -127,9 +127,26 @@ while true; do u="$(storage_list_unread "$_team" "$_agent" 2>/dev/null || true)" [ -n "$u" ] || continue uarr="[$(printf '%s' "$u" | paste -sd, -)]" - ids="$(agmsg_sqlite ':memory:' " - SELECT json_extract(value,'\$.id') FROM json_each('$(printf '%s' "$uarr" | sed "s/'/''/g")'); - " 2>/dev/null || true)" + # #777: this pair's unread backlog grows with every message sent to it, + # so interpolating it into ONE argv element eventually exceeds the OS's + # per-argument ceiling (Linux MAX_ARG_STRLEN=131,072 bytes; smaller + # still on Windows/macOS) and `agmsg_sqlite` fails with "Argument list + # too long" -- every single poll, since the backlog that triggered it + # never shrinks on its own (this script never marks anything read; see + # the file header). Pass the statement on stdin instead, mirroring + # drivers/storage/sqlite-sync.sh:1301 (`_sqlite_data_stdin`, #882) and + # history.sh/inbox.sh: printf is a bash builtin, so writing a large + # value to a temp file never execs and can hit neither that ceiling nor + # argv's at all. `|| continue` on mktemp failure matches the existing + # per-pair `continue` a few lines above: one pair's storage error must + # not end the whole subscription's poll. + _agmsg_wo_sql=$(mktemp "${TMPDIR:-/tmp}/agmsg-watchonce-ids.XXXXXX" 2>/dev/null) || continue + trap 'rm -f "$_agmsg_wo_sql"' EXIT HUP INT TERM + printf "%s\n" "SELECT json_extract(value,'\$.id') FROM json_each('$(printf '%s' "$uarr" | sed "s/'/''/g")');" \ + > "$_agmsg_wo_sql" + ids="$(agmsg_sqlite ':memory:' < "$_agmsg_wo_sql" 2>/dev/null || true)" + rm -f "$_agmsg_wo_sql" + trap - EXIT HUP INT TERM [ -n "$ids" ] || continue count=$(( count + $(printf '%s\n' "$ids" | grep -c .) )) all_ids="$all_ids$ids"$'\n' diff --git a/scripts/inbox.sh b/scripts/inbox.sh index c44a12259..8a887bcff 100755 --- a/scripts/inbox.sh +++ b/scripts/inbox.sh @@ -48,13 +48,29 @@ fi # _sqlite_sync_lit_into in sqlite-sync.sh, which documents the same hazard. _AGMSG_SQ="'" _arr="[$(printf '%s' "$UNREAD_JSONL" | paste -sd, -)]" -ROWS=$(agmsg_sqlite ':memory:' " - SELECT json_extract(value,'\$.from') || char(31) || - replace(replace(json_extract(value,'\$.body'), char(10), '\n'), char(9), '\t') || char(31) || - json_extract(value,'\$.at') || char(31) || - json_extract(value,'\$.id') - FROM json_each('${_arr//$_AGMSG_SQ/$_AGMSG_SQ$_AGMSG_SQ}'); -") +# #777: an agent's unread backlog grows with every message sent to it, so +# interpolating it into ONE argv element eventually exceeds the OS's +# per-argument ceiling (Linux MAX_ARG_STRLEN=131,072 bytes; smaller still on +# Windows/macOS) and `agmsg_sqlite` fails with "Argument list too long" -- +# every single call, since the backlog that triggered it never shrinks on +# its own. Pass the statement on stdin instead, mirroring +# drivers/storage/sqlite-sync.sh:1301 (`_sqlite_data_stdin`, #882) and +# history.sh: printf is a bash builtin, so writing a large value to a temp +# file never execs and can hit neither that ceiling nor argv's at all. +_agmsg_inbox_sql=$(mktemp "${TMPDIR:-/tmp}/agmsg-inbox-rows.XXXXXX") || exit 13 +trap 'rm -f "$_agmsg_inbox_sql"' EXIT HUP INT TERM +{ + printf "%s\n" "SELECT json_extract(value,'\$.from') || char(31) ||" + printf "%s\n" " replace(replace(json_extract(value,'\$.body'), char(10), '\n'), char(9), '\t') || char(31) ||" + printf "%s\n" " json_extract(value,'\$.at') || char(31) ||" + printf "%s\n" " json_extract(value,'\$.id')" + printf "FROM json_each('" + printf '%s' "${_arr//$_AGMSG_SQ/$_AGMSG_SQ$_AGMSG_SQ}" + printf "');\n" +} > "$_agmsg_inbox_sql" +ROWS=$(agmsg_sqlite ':memory:' < "$_agmsg_inbox_sql") +rm -f "$_agmsg_inbox_sql" +trap - EXIT HUP INT TERM COUNT=$(printf '%s\n' "$ROWS" | wc -l | tr -d ' ') echo "$COUNT new message(s):" diff --git a/scripts/watch.sh b/scripts/watch.sh index a3c59f484..00d633e15 100755 --- a/scripts/watch.sh +++ b/scripts/watch.sh @@ -698,17 +698,50 @@ while true; do # _sqlite_sync_lit_into in sqlite-sync.sh, which documents the same hazard. _AGMSG_SQ="'" _arr="[$(printf '%s' "$OUT" | paste -sd, -)]" - ROWS="$(agmsg_sqlite ':memory:' " - SELECT COALESCE(json_extract(value,'\$.type'),'') || char(31) || - COALESCE(json_extract(value,'\$.id'),'') || char(31) || - COALESCE(json_extract(value,'\$.at'),'') || char(31) || - COALESCE(json_extract(value,'\$.team'),'') || char(31) || - COALESCE(json_extract(value,'\$.from'),'') || char(31) || - COALESCE(json_extract(value,'\$.to'),'') || char(31) || - replace(replace(replace(COALESCE(json_extract(value,'\$.body'),''), char(13), ''), char(10), '\\n'), char(9), '\t') || char(31) || - COALESCE(json_extract(value,'\$.cursor'),'') - FROM json_each('${_arr//$_AGMSG_SQ/$_AGMSG_SQ$_AGMSG_SQ}'); - " 2>/dev/null || true)" + # #777: this pair's undelivered backlog grows independently of anything + # this loop bounds, so interpolating it into ONE argv element eventually + # exceeds the OS's per-argument ceiling (Linux MAX_ARG_STRLEN=131,072 + # bytes; smaller still on Windows/macOS) and `agmsg_sqlite` fails with + # "Argument list too long" -- every single poll, because the failure + # below was already swallowed by `|| true` and the read cursor is only + # advanced from FINAL_CURSOR/DELIVERED_IDS further down, so a silently + # empty ROWS here left the cursor stuck forever, repeating the same + # failure on every future poll. Pass the statement on stdin instead, + # mirroring drivers/storage/sqlite-sync.sh:1301 (`_sqlite_data_stdin`, + # #882) and history.sh/inbox.sh: printf is a bash builtin, so writing a + # large value to a temp file never execs and can hit neither that ceiling + # nor argv's at all. + # + # No trap here: this script installs `trap cleanup EXIT` and + # `trap 'exit 0' INT TERM HUP` once, near the top (bash traps do not + # stack -- the last one set wins), and this runs inside that same + # process's long-lived polling loop, once per pair per interval. Adding a + # loop-local trap here would silently replace those for the rest of the + # process's life. The temp file is removed explicitly on every path + # instead; the one path that leaks it (a signal landing between mktemp + # and the following rm) is caught by the pre-existing INT/TERM/HUP + # handler tearing down the whole process, same as any other in-flight + # work here. + _agmsg_watch_sql="$(mktemp "${TMPDIR:-/tmp}/agmsg-watch-rows.XXXXXX" 2>/dev/null || true)" + if [ -n "$_agmsg_watch_sql" ]; then + { + printf "%s\n" "SELECT COALESCE(json_extract(value,'\$.type'),'') || char(31) ||" + printf "%s\n" " COALESCE(json_extract(value,'\$.id'),'') || char(31) ||" + printf "%s\n" " COALESCE(json_extract(value,'\$.at'),'') || char(31) ||" + printf "%s\n" " COALESCE(json_extract(value,'\$.team'),'') || char(31) ||" + printf "%s\n" " COALESCE(json_extract(value,'\$.from'),'') || char(31) ||" + printf "%s\n" " COALESCE(json_extract(value,'\$.to'),'') || char(31) ||" + printf "%s\n" " replace(replace(replace(COALESCE(json_extract(value,'\$.body'),''), char(13), ''), char(10), '\\n'), char(9), '\t') || char(31) ||" + printf "%s\n" " COALESCE(json_extract(value,'\$.cursor'),'')" + printf "FROM json_each('" + printf '%s' "${_arr//$_AGMSG_SQ/$_AGMSG_SQ$_AGMSG_SQ}" + printf "');\n" + } > "$_agmsg_watch_sql" + ROWS="$(agmsg_sqlite ':memory:' < "$_agmsg_watch_sql" 2>/dev/null || true)" + rm -f "$_agmsg_watch_sql" + else + ROWS="" + fi FINAL_CURSOR="" DELIVERED_IDS=() diff --git a/tests/test_helper.bash b/tests/test_helper.bash index a9c481ab6..38916b858 100644 --- a/tests/test_helper.bash +++ b/tests/test_helper.bash @@ -405,3 +405,31 @@ spawn_decoy_with_cmdline() { bash "$decoy" "$path" 3>&- & DECOY_PID=$! } + +# Sends messages of ~ bytes each from to on +# , via storage_send directly rather than send.sh's own CLI (#777 +# argv-length regressions in inbox.sh/check-inbox.sh/watch.sh/watch-once.sh). +# +# A plain bash FUNCTION CALL, not a subprocess: `storage_send "$team" ... +# "$body"` hands the body to sqlite3 through the same escaped-argv path +# production code uses for a single INSERT (which is not itself in scope -- +# no test here builds a body anywhere near that ceiling), but building the +# backlog this way never has to exec anything with the WHOLE backlog as one +# argument, which is exactly the shape production code used to get wrong +# on read. Bodies are tagged "$label-$i-" so a caller can assert both +# ends of the run (index 0 and count-1) are actually present in what the +# script under test displayed, not just that its exit status was 0. +bulk_send_direct() { + local team="$1" from="$2" to="$3" count="$4" bodylen="$5" label="$6" \ + i=0 pad + pad="$(head -c "$bodylen" /dev/zero | tr '\0' 'x')" + ( + # shellcheck disable=SC1090 + source "$SCRIPTS/lib/storage.sh" + agmsg_storage_load + while [ "$i" -lt "$count" ]; do + storage_send "$team" "$from" "$to" "${label}-${i}-${pad}" >/dev/null + i=$((i + 1)) + done + ) +} diff --git a/tests/test_inbox.bats b/tests/test_inbox.bats index ede115c6d..1a9fce0dc 100644 --- a/tests/test_inbox.bats +++ b/tests/test_inbox.bats @@ -409,3 +409,42 @@ _codex_proj() { grep -q 'additive' <<<"$output" [ "$(pair_unread_count ctm alice)" -eq 0 ] } + +# --- argv-length regression (#777) --------------------------------------- +# +# Both inbox.sh and check-inbox.sh used to embed the whole unread backlog into +# ONE argv element for `sqlite3 ':memory:' ""`. 100 messages of +# ~2000 bytes each is about 200,000 bytes of body alone, well past Linux's +# MAX_ARG_STRLEN (131,072 bytes -- measured directly in this same suite's +# environment, and documented in scripts/history.sh; the ceiling is smaller +# still on Windows: 32,767 characters). Before the fix this failed every +# single run with "Argument list too long" -- the backlog that triggered it +# never shrinks on its own, so it never recovered. + +@test "inbox: a backlog large enough to exceed the OS argv ceiling still displays and marks read (#777)" { + bulk_send_direct testteam bob alice 100 2000 BIG + + run bash "$SCRIPTS/inbox.sh" testteam alice + [ "$status" -eq 0 ] + [[ "$output" == *"100 new message(s):"* ]] + [[ "$output" == *"BIG-0-"* ]] + [[ "$output" == *"BIG-99-"* ]] + [ "$(unread_count alice)" -eq 0 ] +} + +@test "check-inbox: a backlog large enough to exceed the OS argv ceiling still delivers and marks read (#777)" { + bulk_send_direct testteam bob alice 100 2000 CIBIG + + # Not delivered_to_operator() here: that helper embeds the WHOLE payload + # into its own single-shot json_valid('$esc') probe (an sqlite3 argv + # element again, just on the test side), so a body this size would trip + # the identical #777 ceiling one layer up and fail for a reason that has + # nothing to do with check-inbox.sh. Reading raw stdout directly, the way + # "multiple identities poll only the first agent's exact team rows" above + # already does, keeps this test pinned on the script under test. + run bash -c "echo '{}' | bash '$SCRIPTS/check-inbox.sh' claude-code /tmp/project-a" + [ "$status" -eq 0 ] + [[ "$output" == *"CIBIG-0-"* ]] + [[ "$output" == *"CIBIG-99-"* ]] + [ "$(unread_count alice)" -eq 0 ] +} diff --git a/tests/test_watch.bats b/tests/test_watch.bats index b50f830ab..b4168caa2 100644 --- a/tests/test_watch.bats +++ b/tests/test_watch.bats @@ -944,6 +944,53 @@ _record_handover_events() { done } +# --- argv-length regression (#777) -------------------------------------- +# +# watch.sh used to embed the whole page of `storage_watch_after` rows into +# ONE argv element for `sqlite3 ':memory:' ""`, and its failure +# was swallowed by a trailing `2>/dev/null || true` -- so ROWS silently +# became empty, FINAL_CURSOR never got set, and the read cursor never +# advanced. The same backlog would then fail identically on every following +# poll: not a one-off skip, a stall. +# +# 100 messages of ~2000 bytes each is about 200,000 bytes of body alone, +# well past Linux's MAX_ARG_STRLEN (131,072 bytes; smaller still on +# Windows/macOS). Sent BEFORE the watcher starts, so its very first poll has +# to scan and embed the entire backlog in one statement -- the shape the bug +# needed, rather than many small pages that would each stay under the +# ceiling on their own. +@test "watch: a backlog large enough to exceed the OS argv ceiling still delivers and advances the cursor (#777)" { + skip_on_windows "watcher background launch under Git Bash (#182)" + local sid="sess-argv-backlog" + local out="$TEST_SKILL_DIR/argv-backlog.log" + + bulk_send_direct team bob alice 100 2000 WBIG + + AGMSG_WATCH_INTERVAL=1 bash "$SCRIPTS/watch.sh" "$sid" "$PROJ" claude-code >"$out" 2>/dev/null 3>&- 4>&- & + local w=$! + _wait_for_file_contains "$out" "WBIG-99-" || { kill "$w" 2>/dev/null || true; false; } + + # Cursor advancement is a SEPARATE step that runs after every row in this + # poll has already been printed (storage_read_cursor_consume, embedding all + # 100 delivered ids in its own statement) -- killing the watcher the instant + # the last line lands, the way the plain burst test (#245) does, races that + # step under this much data. Poll for it instead, same as "watch: restart + # delivers messages that arrived while the watcher was down" above. + local i cursor + for i in $(seq 1 100); do + cursor=$(_read_cursor team alice 2>/dev/null || echo 0) + [ "${cursor:-0}" -gt 0 ] && break + sleep 0.1 + done + kill "$w" 2>/dev/null || true + wait "$w" 2>/dev/null || true + + grep -q "WBIG-0-" "$out" + grep -q "WBIG-99-" "$out" + # Not stuck: the store-owned cursor moved past where it started (0). + [ "${cursor:-0}" -gt 0 ] +} + @test "watch: empty session_id gets a generated fallback instead of a Usage error (#236)" { local out="$BATS_TEST_TMPDIR/empty-sid.out" AGMSG_WATCH_INTERVAL=1 bash "$SCRIPTS/watch.sh" "" "$PROJ" claude-code alice >"$out" 2>&1 3>&- 4>&- & diff --git a/tests/test_watch_once.bats b/tests/test_watch_once.bats index 2ce8a9532..9485c3f16 100644 --- a/tests/test_watch_once.bats +++ b/tests/test_watch_once.bats @@ -105,6 +105,27 @@ _assert_startup_was_delayed() { [[ "$output" =~ "hello pending" ]] } +# --- argv-length regression (#777) -------------------------------------- +# +# This pair's unread ids used to be embedded into ONE argv element for +# `sqlite3 ':memory:' ""`, with the failure swallowed by a +# trailing `2>/dev/null || true` -- so `ids` silently became empty and the +# `[ -n "$ids" ] || continue` a few lines later skipped the whole team every +# single poll, never marking anything read (this script never does) and +# never reporting it pending either. +# +# 100 messages of ~2000 bytes each is about 200,000 bytes of body alone, +# well past Linux's MAX_ARG_STRLEN (131,072 bytes; smaller still on +# Windows/macOS). +@test "watch-once: a backlog large enough to exceed the OS argv ceiling still reports pending (#777)" { + bulk_send_direct team bob alice 100 2000 WOBIG + + run bash "$TYPES/codex/watch-once.sh" "$PROJ" codex --name alice --team team --timeout 2 --interval 1 + [ "$status" -eq 0 ] + [[ "$output" =~ "status=pending" ]] + [[ "$output" =~ "count=100" ]] +} + @test "watch-once: ignores messages already read by inbox.sh" { bash "$SCRIPTS/send.sh" team bob alice "read already" >/dev/null bash "$SCRIPTS/inbox.sh" team alice >/dev/null From 85b3e063c5457256a73fe3141906d37b51c5065e Mon Sep 17 00:00:00 2001 From: mkmariko Date: Tue, 25 Aug 2026 19:25:24 +0900 Subject: [PATCH 2/6] =?UTF-8?q?fix(#777):=20storage=5Fread=5Fcursor=5Fcons?= =?UTF-8?q?ume=20=E2=80=94=20same=20argv-to-stdin=20fix?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 #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 #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 #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 Claude-Session: https://claude.ai/code/session_01GUJZpBvin7hQE6QFWvSatA --- scripts/drivers/storage/sqlite.sh | 43 +++++++++++++++++++++++++++---- 1 file changed, 38 insertions(+), 5 deletions(-) diff --git a/scripts/drivers/storage/sqlite.sh b/scripts/drivers/storage/sqlite.sh index 47bda31e1..763ee0083 100755 --- a/scripts/drivers/storage/sqlite.sh +++ b/scripts/drivers/storage/sqlite.sh @@ -378,9 +378,36 @@ storage_read_cursor_consume() { WHERE e.type='message_sent' AND e.team='$tl' AND e.id='$(_sqlite_lit "$id")' AND e.legacy_id IS NOT NULL);" done - agmsg_sqlite "$db" "BEGIN IMMEDIATE; - $sql - INSERT OR IGNORE INTO read_cursors(team,agent,local_position) + # #777 ("Not measured" section): $sql gains one INSERT/UPDATE block per + # delivered id, and the whole "BEGIN IMMEDIATE; ...; COMMIT;" statement + # used to be handed to `agmsg_sqlite` as ONE argv element. Measured on + # Windows: 97 ids built a 38,897-byte statement and CreateProcess refused + # it outright (that 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". Same fix as history.sh / + # inbox.sh / check-inbox.sh / watch.sh / watch-once.sh, and + # drivers/storage/sqlite-sync.sh's own #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 here, 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` once near the top of + # that process. A trap set and then cleared in here (bash traps do not + # stack) would replace watch.sh's for the rest of its life the first time + # this function ever ran -- the exact mistake this same #777 pass caught + # and avoided in watch.sh's own ROWS-fetch fix a few lines above this one + # in the call chain. The temp file is removed explicitly on every path + # instead; the one path that leaks it (a signal landing mid-call) is left + # for the OS's own temp-directory cleanup, same trade-off already accepted + # there. + local sql_file + sql_file=$(mktemp "${TMPDIR:-/tmp}/agmsg-cursor-consume.XXXXXX" 2>/dev/null) || { echo runtime_error; return 13; } + { + printf '%s\n' "BEGIN IMMEDIATE;" + printf '%s\n' "$sql" + printf '%s\n' " INSERT OR IGNORE INTO read_cursors(team,agent,local_position) VALUES('$tl','$al',0); UPDATE read_cursors SET local_position=MAX(local_position,COALESCE(( SELECT MIN(e.seq)-1 FROM events e @@ -390,8 +417,14 @@ storage_read_cursor_consume() { AND NOT EXISTS(SELECT 1 FROM events r WHERE r.type='message_read' AND r.team=e.team AND r.agent='$al' AND r.msg_id=e.id) ),MIN($target,$(_sqlite_highwater)))) - WHERE team='$tl' AND agent='$al'; - COMMIT;" >/dev/null 2>&1 || { echo runtime_error; return 13; } + WHERE team='$tl' AND agent='$al';" + printf '%s\n' "COMMIT;" + } > "$sql_file" + if ! agmsg_sqlite "$db" < "$sql_file" >/dev/null 2>&1; then + rm -f "$sql_file" + echo runtime_error; return 13 + fi + rm -f "$sql_file" echo ok } From 54c130399bb22f2759542f8ce595c558260b76f5 Mon Sep 17 00:00:00 2001 From: mkmariko Date: Tue, 25 Aug 2026 22:23:19 +0900 Subject: [PATCH 3/6] fix: normalize agmsg_sqlite's CRLF row separator on Windows (no issue yet) Independent of #777 -- found by claude-win while verifying #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 #102/#143 (sqlite3 >= 3.50's own caret-notation escaping of control bytes, fixed by the existing `-escape off` probe) -- reproduces on origin/main before #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 #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 Claude-Session: https://claude.ai/code/session_01GUJZpBvin7hQE6QFWvSatA --- scripts/lib/storage.sh | 44 ++++++++++++- tests/test_sqlite_crlf.bats | 122 ++++++++++++++++++++++++++++++++++++ 2 files changed, 164 insertions(+), 2 deletions(-) create mode 100644 tests/test_sqlite_crlf.bats diff --git a/scripts/lib/storage.sh b/scripts/lib/storage.sh index 6283f22ed..696627603 100644 --- a/scripts/lib/storage.sh +++ b/scripts/lib/storage.sh @@ -253,9 +253,49 @@ agmsg_sqlite() { _agmsg_sqlite_recording "$@" return fi - # shellcheck disable=SC2086 # intentional split: "-escape off" → two args, or none + # Windows' sqlite3.exe (measured: 3.53.4) ends each row of a multi-row + # result with \r\n, not \n -- confirmed by piping a three-row SELECT + # through `od -c` on real Windows hardware. This is independent of the + # `-escape` probe above (#102/#143: that is sqlite3 >= 3.50's own caret- + # notation rendering, fixed by `-escape off`, and reproduces on Linux too + # -- this CRLF ending does not reproduce here). HYPOTHESIS (unverified): + # the Windows C runtime's stdio text-mode translation rewrites sqlite3's + # own LF terminators to CRLF on the way out; what is actually confirmed is + # only the \r\n on the wire, not this mechanism. + # + # `ROWS=$(agmsg_sqlite ...)` strips only the trailing newline of the WHOLE + # captured output (bash command substitution), so every row but the last + # keeps a \r stuck to its final field -- typically an id, since every + # multi-field row built by this codebase's callers puts id/cursor/at last + # and body earlier (never in scope for this fix, but worth naming: it is + # why this hazard has not already shown up as corrupted message bodies). + # `IFS=$'\x1f' read` does not split on \r, so that \r rides along into + # the field value. Reported and measured on real Windows hardware: a + # 100-message backlog lost 99 of 100 mark-as-read updates in one + # inbox.sh run, because storage_mark_read_batch's ids no longer matched + # any real msg_id. + # + # The fix normalizes ONLY a \r immediately before the line-ending \n -- + # not every \r in the stream. `tr -d '\r'` (used by _sqlite_data / + # _sqlite_data_stdin in drivers/storage/sqlite.sh, wrapping calls to THIS + # function) would also be correct for THIS symptom, but it deletes every + # \r anywhere in the output, including one that is 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 it is not used here. `sed`'s + # `$` anchor matches only end-of-line, so a \r elsewhere in a row + # (mid-body) is left untouched. + # + # Wrapped in a subshell with its own `set -o pipefail` so the pipeline's + # status is sqlite3's, not sed's, without changing pipefail for the + # calling script (same shape as _sqlite_data / _sqlite_data_stdin in + # drivers/storage/sqlite.sh). local _agmsg_sqlite_rc=0 - sqlite3 $_AGMSG_ESCAPE_FLAG -cmd ".timeout ${AGMSG_BUSY_TIMEOUT:-5000}" "$@" || _agmsg_sqlite_rc=$? + ( + set -o pipefail + # shellcheck disable=SC2086 # intentional split: "-escape off" → two args, or none + sqlite3 $_AGMSG_ESCAPE_FLAG -cmd ".timeout ${AGMSG_BUSY_TIMEOUT:-5000}" "$@" | sed $'s/\r$//' + ) + _agmsg_sqlite_rc=$? # SQLITE_BUSY after the full timeout used to pass in silence: the caller saw # a non-zero it often swallowed, and the operator saw a command that hung # for the timeout and said nothing (#1001 -- two people diagnosed two diff --git a/tests/test_sqlite_crlf.bats b/tests/test_sqlite_crlf.bats new file mode 100644 index 000000000..b1584cb48 --- /dev/null +++ b/tests/test_sqlite_crlf.bats @@ -0,0 +1,122 @@ +#!/usr/bin/env bats + +# Regression coverage for a Windows-only sqlite3.exe behavior that this +# machine (Linux) cannot reproduce on its own: multi-row SELECT output +# separates rows with \r\n, not \n. Measured on real Windows hardware +# (sqlite3.exe 3.53.4) and reported against inbox.sh's mark-as-read step -- +# see scripts/lib/storage.sh's agmsg_sqlite() for the full writeup. This +# file exercises the fix with a PATH-shimmed sqlite3 stub that reproduces +# the \r\n row separator deterministically on Linux, the same "wrapper +# script ahead of the real binary on PATH" technique test_watch_once.bats +# already uses for its slow-awk shim. +# +# No issue number yet -- this was found independently of #777 while +# verifying #777's own fix on Windows hardware, and has not been filed +# upstream. + +load test_helper + +setup() { + setup_test_env +} + +teardown() { + teardown_test_env +} + +# A wrapper named `sqlite3`, placed ahead of the real one on PATH, that +# behaves exactly like the real binary except every line of its stdout +# gets a synthetic \r appended right before the \n -- simulating the \r\n +# Windows' sqlite3.exe emits at each row boundary. +# +# The real binary's path is resolved HERE, before this directory is ever +# prepended to PATH, and baked into the wrapper as a literal exec target. +# Mirrors test_watch_once.bats's `_slow_startup_path` awk shim exactly for +# this reason: a lookup done INSIDE the wrapper, after PATH already +# includes this directory, would resolve back to the wrapper itself. +_stub_sqlite3_crlf() { + local dir="$BATS_TEST_TMPDIR/crlfbin" real + real="$(command -v sqlite3)" + mkdir -p "$dir" + cat > "$dir/sqlite3" </dev/null + bash "$SCRIPTS/join.sh" crlfteam bob claude-code /tmp/project-crlf >/dev/null + local n + for n in $(seq 1 20); do + bash "$SCRIPTS/send.sh" crlfteam bob alice "CRLF-$n" >/dev/null + done + + PATH="$stub:$PATH" + run bash "$SCRIPTS/inbox.sh" crlfteam alice + [ "$status" -eq 0 ] + [[ "$output" == *"20 new message(s):"* ]] + for n in $(seq 1 20); do + [[ "$output" == *"CRLF-$n"* ]] + done + + # The real symptom: with the pre-fix agmsg_sqlite, only the LAST id in a + # multi-row unread scan kept a clean (unmangled) trailing field, so all + # but one mark-as-read update silently matched no real msg_id and every + # other message stayed unread. Fixed, the whole backlog clears. + local left + # `grep -c .` exits 1 when the count is 0 (no matching lines) -- correct + # and expected here, but bats runs test bodies under `set -e`, so without + # `|| true` that exit status would abort the test right at this + # assignment before the assertion below ever ran. + left="$(bash -c ' + source "'"$SCRIPTS"'/lib/storage.sh" + agmsg_storage_load + storage_list_unread crlfteam alice + ' | grep -c . || true)" + [ "$left" -eq 0 ] +} From 5f684b94572ef65fc3f5e0047924d4ef3368c67f Mon Sep 17 00:00:00 2001 From: mkmariko Date: Tue, 25 Aug 2026 22:52:56 +0900 Subject: [PATCH 4/6] fix: apply the CRLF row-separator fix to the recording (sync-driver) 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 Claude-Session: https://claude.ai/code/session_01GUJZpBvin7hQE6QFWvSatA --- scripts/lib/storage.sh | 64 +++++++++++++++-- tests/test_sqlite_crlf.bats | 136 ++++++++++++++++++++++++++++++++++++ 2 files changed, 193 insertions(+), 7 deletions(-) diff --git a/scripts/lib/storage.sh b/scripts/lib/storage.sh index 696627603..622e3f6c9 100644 --- a/scripts/lib/storage.sh +++ b/scripts/lib/storage.sh @@ -325,16 +325,66 @@ agmsg_sqlite() { # failed, so "the operation failed and the last statement was busy" names it. # # stderr is captured to classify it and re-emitted unchanged, so a caller that -# reads or silences it sees what it saw before; stdout is the data stream and -# is not touched; the exit status is passed through. Written as an `if` so a -# caller running under `set -e` is not exited by the assignment itself. +# reads or silences it sees what it saw before; stdout is the data stream, now +# passed through the same trailing-CR normalization as agmsg_sqlite()'s own +# non-recording path above (Windows' sqlite3.exe row-separator \r\n; see that +# comment for the full writeup -- this path bypasses it entirely via the early +# `return` above, so it needs its own copy of the fix, not a call into it: this +# function's stdout/stderr routing exists for a different purpose, classifying +# ok/busy/failed for the sync driver adapter, and folding the two together +# would tangle two independent concerns). The exit status is still passed +# through, unaffected either way. +# +# The original fd-3 passthrough trick (sqlite3's own fd 1 repointed at +# whatever fd 1 was outside this function, with no process in between) cannot +# survive inserting `sed`: stdout now goes through an actual pipe, so a temp +# file replaces the `err=$(...)` capture for stderr, and the exit status comes +# from `${PIPESTATUS[0]}` (sqlite3's, not sed's) rather than the substitution's +# own `$?`. Stderr is still read back whole and re-emitted verbatim afterward, +# so a caller that reads or silences it sees the same bytes as before. +# +# The pipeline is wrapped in an `if`, same as the original, and for the same +# reason: this is a plain function call, not a subshell, so it runs in the +# CALLING script's own shell -- and several callers set both `-e` and +# `-o pipefail`. A command tested by `if` is exempt from `set -e` on a +# non-zero exit (POSIX), so the pipeline cannot abort the caller here +# regardless of its pipefail setting. +# +# `${PIPESTATUS[0]}` (sqlite3's exit status, not sed's) is read in BOTH +# branches, not once after the `if` -- and specifically not guarded with +# `|| true` the way the CRLF fix above is, because `|| true` is not safe +# here. `PIPESTATUS` is overwritten by the NEXT command this shell +# executes, of any kind, including a trivial one: `pipeline || true` runs +# `true` whenever the pipeline's own exit status is non-zero, and reading +# `${PIPESTATUS[0]}` after that reads back `true`'s status (0), not +# sqlite3's. The CRLF fix's own `|| true` above is fine BECAUSE that call +# site never reads PIPESTATUS at all. This one silently turned every +# failure here into rc=0 whenever pipefail was already active in the +# caller -- and only there: storage-sync-driver.sh sets `-o pipefail` +# itself, so a plain `bash -c` probe without it stayed green while the +# real busy-timeout contract test (test_remote_sync.bats, "a store +# another writer holds is busy") got 0 where it expected 11. Reading +# PIPESTATUS inside the `if`'s own branches, before anything else runs, +# is what keeps it correct either way. _agmsg_sqlite_recording() { - local err rc + local err rc errfile + # A mktemp failure degrades stderr capture to /dev/null rather than failing + # the operation outright: worse diagnostics (an unclassifiable error reads + # as "failed", never as "busy"), not worse correctness, and the same + # "environment problem, not a bad input" class of failure the busy/failed + # distinction exists to tell apart from an ordinary refusal. + errfile=$(mktemp "${TMPDIR:-/tmp}/agmsg-sqlite-recording-err.XXXXXX" 2>/dev/null) || errfile=/dev/null # shellcheck disable=SC2086 # same intentional split as above - if { err=$(sqlite3 $_AGMSG_ESCAPE_FLAG -cmd ".timeout ${AGMSG_BUSY_TIMEOUT:-5000}" "$@" 2>&1 >&3 3>&-); } 3>&1; then - rc=0 + if sqlite3 $_AGMSG_ESCAPE_FLAG -cmd ".timeout ${AGMSG_BUSY_TIMEOUT:-5000}" "$@" 2>"$errfile" | sed $'s/\r$//'; then + rc=${PIPESTATUS[0]} + else + rc=${PIPESTATUS[0]} + fi + if [ "$errfile" = /dev/null ]; then + err="" else - rc=$? + err="$(cat "$errfile" 2>/dev/null)" + rm -f "$errfile" fi [ -z "$err" ] || printf '%s\n' "$err" >&2 if [ "$rc" -eq 0 ]; then diff --git a/tests/test_sqlite_crlf.bats b/tests/test_sqlite_crlf.bats index b1584cb48..105f03676 100644 --- a/tests/test_sqlite_crlf.bats +++ b/tests/test_sqlite_crlf.bats @@ -120,3 +120,139 @@ EOF ' | grep -c . || true)" [ "$left" -eq 0 ] } + +# --- AGMSG_SQLITE_OUTCOME_FILE (recording) path ------------------------- +# +# agmsg_sqlite() takes a completely different branch when +# AGMSG_SQLITE_OUTCOME_FILE is set (_agmsg_sqlite_recording -- only the +# sync driver adapter sets this, scripts/internal/storage-sync-driver.sh), +# so the fix above does not automatically cover it: it needs, and got, its +# own copy of the same trailing-CR normalization. A codex review of the +# first version of this fix caught the gap. Reproduces via the same +# _stub_sqlite3_crlf, plus a second, fully synthetic stub +# (_stub_sqlite3_fixed) for deterministically exercising the ok/busy/failed +# classification without depending on real SQLITE_BUSY lock-contention +# timing. + +@test "agmsg_sqlite (recording path): strips only the trailing CR, same as the non-recording path" { + local stub outfile + stub="$(_stub_sqlite3_crlf)" + outfile="$BATS_TEST_TMPDIR/outcome" + PATH="$stub:$PATH" + run bash -c "source '$SCRIPTS/lib/storage.sh'; AGMSG_SQLITE_OUTCOME_FILE='$outfile' agmsg_sqlite ':memory:' 'SELECT 1; SELECT 2; SELECT 3;'" + [ "$status" -eq 0 ] + [ "$output" = $'1\n2\n3' ] + [ "$(cat "$outfile")" = ok ] +} + +@test "agmsg_sqlite (recording path): preserves a genuine mid-body CR under the same CRLF stub" { + local stub outfile + stub="$(_stub_sqlite3_crlf)" + outfile="$BATS_TEST_TMPDIR/outcome" + PATH="$stub:$PATH" + run bash -c "source '$SCRIPTS/lib/storage.sh'; AGMSG_SQLITE_OUTCOME_FILE='$outfile' agmsg_sqlite ':memory:' \"SELECT 'x' || char(31) || 'a' || char(13) || 'b' || char(31) || 'id123';\"" + [ "$status" -eq 0 ] + [ "$output" = $'x\x1fa\rb\x1fid123' ] + [ "$(cat "$outfile")" = ok ] +} + +# A stub that ignores the real database entirely and just emits FIXED +# stdout/stderr/exit-code content read back from two plain files -- for +# testing _agmsg_sqlite_recording's own ok/busy/failed classification and +# exit-status passthrough in isolation from any real SQL execution or lock +# timing. The desired bytes are written to files by the CALLER (ordinary +# $'...' quoting there, no heredoc-embedding hazards) rather than baked into +# the generated script's own text. +_stub_sqlite3_fixed() { + local dir="$BATS_TEST_TMPDIR/fixedbin" stdout_file="$1" stderr_file="$2" exitcode="$3" + mkdir -p "$dir" + cat > "$dir/sqlite3" <&2 +exit $exitcode +EOF + chmod +x "$dir/sqlite3" + printf '%s' "$dir" +} + +@test "agmsg_sqlite (recording path): ok classification, outcome file, and exit code are unchanged" { + local stub outfile stdout_file stderr_file + stdout_file="$BATS_TEST_TMPDIR/out.txt"; stderr_file="$BATS_TEST_TMPDIR/err.txt" + printf 'row1\nrow2\n' > "$stdout_file" + printf '' > "$stderr_file" + stub="$(_stub_sqlite3_fixed "$stdout_file" "$stderr_file" 0)" + outfile="$BATS_TEST_TMPDIR/outcome" + PATH="$stub:$PATH" + run bash -c "source '$SCRIPTS/lib/storage.sh'; AGMSG_SQLITE_OUTCOME_FILE='$outfile' agmsg_sqlite ':memory:' 'irrelevant, the stub ignores it;'" + [ "$status" -eq 0 ] + [ "$output" = $'row1\nrow2' ] + [ "$(cat "$outfile")" = ok ] +} + +@test "agmsg_sqlite (recording path): busy classification, outcome file, exit code, and verbatim stderr are unchanged" { + local stub outfile stdout_file stderr_file + stdout_file="$BATS_TEST_TMPDIR/out.txt"; stderr_file="$BATS_TEST_TMPDIR/err.txt" + printf '' > "$stdout_file" + printf 'Error: database is locked\n' > "$stderr_file" + stub="$(_stub_sqlite3_fixed "$stdout_file" "$stderr_file" 5)" + outfile="$BATS_TEST_TMPDIR/outcome" + PATH="$stub:$PATH" + # --separate-stderr (bats-core, same idiom test_remote_sync.bats already + # uses) so stdout and stderr can be asserted apart -- the sed fix touches + # stdout only, so the classification text must arrive on $stderr + # byte-for-byte, not just be classified correctly. + run --separate-stderr bash -c "source '$SCRIPTS/lib/storage.sh'; AGMSG_SQLITE_OUTCOME_FILE='$outfile' agmsg_sqlite ':memory:' 'irrelevant, the stub ignores it;'" + [ "$status" -eq 5 ] + [ "$(cat "$outfile")" = busy ] + [[ "$stderr" == *"Error: database is locked"* ]] +} + +@test "agmsg_sqlite (recording path): failed classification, outcome file, exit code, and verbatim stderr are unchanged" { + local stub outfile stdout_file stderr_file + stdout_file="$BATS_TEST_TMPDIR/out.txt"; stderr_file="$BATS_TEST_TMPDIR/err.txt" + printf '' > "$stdout_file" + printf 'Error: near "not": syntax error\n' > "$stderr_file" + stub="$(_stub_sqlite3_fixed "$stdout_file" "$stderr_file" 1)" + outfile="$BATS_TEST_TMPDIR/outcome" + PATH="$stub:$PATH" + run --separate-stderr bash -c "source '$SCRIPTS/lib/storage.sh'; AGMSG_SQLITE_OUTCOME_FILE='$outfile' agmsg_sqlite ':memory:' 'irrelevant, the stub ignores it;'" + [ "$status" -eq 1 ] + [ "$(cat "$outfile")" = failed ] + [[ "$stderr" == *"syntax error"* ]] +} + +@test "agmsg_sqlite (recording path): busy classification survives a caller with -e/pipefail already on (storage-sync-driver.sh's own setting)" { + # _agmsg_sqlite_recording is a plain function call, not a subshell -- it + # runs IN the calling script's own shell, inheriting whatever `set -e` / + # `set -o pipefail` that shell already has. storage-sync-driver.sh, the + # ONLY real caller that ever sets AGMSG_SQLITE_OUTCOME_FILE, has + # `set -euo pipefail` at its own top, so this is the actual condition in + # production, not a hypothetical. + # + # A prior version of this fix guarded the pipeline with `pipeline || + # true` to keep a pipefail-inheriting caller's `set -e` from aborting + # right there. That guard is safe on its own, but this call site ALSO + # reads `${PIPESTATUS[0]}` afterward -- and PIPESTATUS is overwritten by + # the very next command this shell runs, of any kind. With pipefail on, + # the pipeline's own exit status became sqlite3's non-zero one, which is + # exactly when `|| true` runs `true` -- so `${PIPESTATUS[0]}` was read + # back as `true`'s (0), not sqlite3's, and every busy/failed call quietly + # became "ok". Only a caller with pipefail already on triggers `|| true` + # in the first place, which is why the tests above -- run from a plain + # `bash -c` with no pipefail -- stayed green through this: they never + # replicated the one caller shape that actually breaks it. Caught for + # real by test_remote_sync.bats's busy-timeout contract test, which does + # go through the real adapter and hence its `-o pipefail`. + local stub outfile stdout_file stderr_file + stdout_file="$BATS_TEST_TMPDIR/out.txt"; stderr_file="$BATS_TEST_TMPDIR/err.txt" + printf '' > "$stdout_file" + printf 'Error: database is locked\n' > "$stderr_file" + stub="$(_stub_sqlite3_fixed "$stdout_file" "$stderr_file" 5)" + outfile="$BATS_TEST_TMPDIR/outcome" + PATH="$stub:$PATH" + run --separate-stderr bash -c "set -euo pipefail; source '$SCRIPTS/lib/storage.sh'; AGMSG_SQLITE_OUTCOME_FILE='$outfile' agmsg_sqlite ':memory:' 'irrelevant, the stub ignores it;'" + [ "$status" -eq 5 ] + [ "$(cat "$outfile")" = busy ] + [[ "$stderr" == *"Error: database is locked"* ]] +} From 175ee52794c487ea93b699d09a61c8bc00f5740c Mon Sep 17 00:00:00 2001 From: mkmariko Date: Wed, 26 Aug 2026 11:03:54 +0900 Subject: [PATCH 5/6] fix: replace non-enforcing [[ ]]/=~ assertions with grep -qF (#991 CI) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit upstream の check-enforced-assertions.sh(@testボディの非末尾に[[ ]]/(( ))/! を 使うとmacOS標準のbash 3.2ではfalseでも静かにpassしてしまうため、それを数える baseline=638のCIゲート)に、このPRで追加したテストが抵触していた。 claude-winがWindows実機でPR #991のCI失敗を発見し、tests/test_sqlite_crlf.bats:102 の1箇所を特定したが、実際に確認すると他に5箇所(tests/test_inbox.bats 5箇所・ tests/test_watch_once.bats 1箇所)があった(baseline比+6、644件)。 origin/mainの該当ファイルと現ブランチを比較し、新規追加行のうち[[ ]]/(( ))/! を 含む行を機械抽出、各行の位置(@testボディの最後か・for/if内か・||/&&連結か)を 目視確認して特定した。 修正: 該当6箇所のうち5箇所を[[ $output == *"..."* ]]/[[ $output =~ "..." ]] から grep -qF -- "..." <<< "$output" へ変更(部分文字列一致として等価、grep自身の 終了コードでBatsをbash 3.2でも確実に失敗させる)。残る1箇所 (test_watch_once.bats:126の[[ "$output" =~ "count=100" ]])は@testボディの 最後の文でチェッカーの除外対象のため変更していない。 検証: check-enforced-assertions.shが638(baseline)でexit 0。対象4ファイルの bats実行(test_inbox/test_watch_once/test_sqlite_crlf/test_watch、計87件)で not ok 0件。codex commit前レビューPASS。 Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01GUJZpBvin7hQE6QFWvSatA --- tests/test_inbox.bats | 10 +++++----- tests/test_sqlite_crlf.bats | 2 +- tests/test_watch_once.bats | 2 +- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/tests/test_inbox.bats b/tests/test_inbox.bats index 1a9fce0dc..ecf3f7d22 100644 --- a/tests/test_inbox.bats +++ b/tests/test_inbox.bats @@ -426,9 +426,9 @@ _codex_proj() { run bash "$SCRIPTS/inbox.sh" testteam alice [ "$status" -eq 0 ] - [[ "$output" == *"100 new message(s):"* ]] - [[ "$output" == *"BIG-0-"* ]] - [[ "$output" == *"BIG-99-"* ]] + grep -qF -- "100 new message(s):" <<< "$output" + grep -qF -- "BIG-0-" <<< "$output" + grep -qF -- "BIG-99-" <<< "$output" [ "$(unread_count alice)" -eq 0 ] } @@ -444,7 +444,7 @@ _codex_proj() { # already does, keeps this test pinned on the script under test. run bash -c "echo '{}' | bash '$SCRIPTS/check-inbox.sh' claude-code /tmp/project-a" [ "$status" -eq 0 ] - [[ "$output" == *"CIBIG-0-"* ]] - [[ "$output" == *"CIBIG-99-"* ]] + grep -qF -- "CIBIG-0-" <<< "$output" + grep -qF -- "CIBIG-99-" <<< "$output" [ "$(unread_count alice)" -eq 0 ] } diff --git a/tests/test_sqlite_crlf.bats b/tests/test_sqlite_crlf.bats index 105f03676..ca3ec9c39 100644 --- a/tests/test_sqlite_crlf.bats +++ b/tests/test_sqlite_crlf.bats @@ -99,7 +99,7 @@ EOF PATH="$stub:$PATH" run bash "$SCRIPTS/inbox.sh" crlfteam alice [ "$status" -eq 0 ] - [[ "$output" == *"20 new message(s):"* ]] + grep -qF -- "20 new message(s):" <<< "$output" for n in $(seq 1 20); do [[ "$output" == *"CRLF-$n"* ]] done diff --git a/tests/test_watch_once.bats b/tests/test_watch_once.bats index 9485c3f16..bba75cb8f 100644 --- a/tests/test_watch_once.bats +++ b/tests/test_watch_once.bats @@ -122,7 +122,7 @@ _assert_startup_was_delayed() { run bash "$TYPES/codex/watch-once.sh" "$PROJ" codex --name alice --team team --timeout 2 --interval 1 [ "$status" -eq 0 ] - [[ "$output" =~ "status=pending" ]] + grep -qF -- "status=pending" <<< "$output" [[ "$output" =~ "count=100" ]] } From c8434c5d357676431d4675c02e29ce4a9c1d2bbe Mon Sep 17 00:00:00 2001 From: fujibee Date: Sun, 13 Sep 2026 12:02:39 -0700 Subject: [PATCH 6/6] fix: keep the #1001 busy diagnostic under errexit callers The rebase of agmsg_sqlite() onto the #1001 busy diagnostic captured the subshell status on a separate line: ( set -o pipefail; sqlite3 ... | sed ... ) _agmsg_sqlite_rc=$? A caller running under set -e exits at the failing subshell before that line runs, so a SQLITE_BUSY there returns 5 without the diagnostic -- the silent hang #1001 was fixed to end. api.sh, init-db.sh, rename.sh, rename-team.sh and migrate-team-store.sh all call agmsg_sqlite as a bare statement under set -e. Capture the status with || on the subshell itself, the form main already uses. Measured with a sqlite3 stub that exits 5, under set -e: before, rc=5 and no diagnostic; after, rc=5 and the diagnostic is printed. The success path and the non-errexit path are unchanged. --- scripts/lib/storage.sh | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/scripts/lib/storage.sh b/scripts/lib/storage.sh index 622e3f6c9..1c5e35b65 100644 --- a/scripts/lib/storage.sh +++ b/scripts/lib/storage.sh @@ -294,8 +294,7 @@ agmsg_sqlite() { set -o pipefail # shellcheck disable=SC2086 # intentional split: "-escape off" → two args, or none sqlite3 $_AGMSG_ESCAPE_FLAG -cmd ".timeout ${AGMSG_BUSY_TIMEOUT:-5000}" "$@" | sed $'s/\r$//' - ) - _agmsg_sqlite_rc=$? + ) || _agmsg_sqlite_rc=$? # SQLITE_BUSY after the full timeout used to pass in silence: the caller saw # a non-zero it often swallowed, and the operator saw a command that hung # for the timeout and said nothing (#1001 -- two people diagnosed two