diff --git a/scripts/check-inbox.sh b/scripts/check-inbox.sh index ec88e351..73086e5f 100755 --- a/scripts/check-inbox.sh +++ b/scripts/check-inbox.sh @@ -141,19 +141,38 @@ for team in "${TEAM_LIST[@]}"; do esac RESULT=$(agmsg_sqlite "$DB" " - SELECT from_agent || char(31) || replace(replace(body, char(10), '\n'), char(9), '\t') || char(31) || created_at + SELECT id || char(31) || from_agent || char(31) || replace(replace(body, char(10), '\n'), char(9), '\t') || char(31) || created_at FROM messages WHERE team='$team_sql' AND to_agent='$AGENT_SQL' AND read_at IS NULL ORDER BY created_at ASC; ") if [ -n "$RESULT" ]; then COUNT=$(echo "$RESULT" | wc -l | tr -d ' ') OUTPUT+="$COUNT new message(s) in $team:"$'\n' - while IFS=$'\x1f' read -r from body ts; do + IDS="" + while IFS=$'\x1f' read -r id from body ts; do OUTPUT+=" [$ts] $from: $body"$'\n' + case "$id" in + ''|*[!0-9]*) ;; # defensive: never splice a non-numeric value into SQL + *) IDS="${IDS:+$IDS,}$id" ;; + esac done <<< "$RESULT" OUTPUT+=$'\n' - # Mark as read - agmsg_sqlite "$DB" "UPDATE messages SET read_at=strftime('%Y-%m-%dT%H:%M:%SZ','now') WHERE team='$team_sql' AND to_agent='$AGENT_SQL' AND read_at IS NULL;" 2>/dev/null || true + # Test seam: a two-file barrier that lets the race regression test land a + # message deterministically between display and mark. No-op unless set. + if [ -n "${AGMSG_TEST_MARK_BARRIER:-}" ]; then + : > "$AGMSG_TEST_MARK_BARRIER.reached" + _agmsg_barrier_waited=0 + while [ ! -e "$AGMSG_TEST_MARK_BARRIER.release" ]; do + sleep 0.05 + _agmsg_barrier_waited=$((_agmsg_barrier_waited + 1)) + [ "$_agmsg_barrier_waited" -ge 200 ] && break # 10s safety cap + done + fi + # Mark as read — only the ids captured above, so a message that arrives + # between the SELECT and this UPDATE is not marked read unseen. + if [ -n "$IDS" ]; then + agmsg_sqlite "$DB" "UPDATE messages SET read_at=strftime('%Y-%m-%dT%H:%M:%SZ','now') WHERE id IN ($IDS);" 2>/dev/null || true + fi fi done diff --git a/scripts/inbox.sh b/scripts/inbox.sh index ad5c21df..68051cac 100755 --- a/scripts/inbox.sh +++ b/scripts/inbox.sh @@ -26,9 +26,10 @@ _agmsg_sqlesc() { printf %s "$1" | sed "s/'/''/g"; } TEAM_SQL="$(_agmsg_sqlesc "$TEAM")" AGENT_SQL="$(_agmsg_sqlesc "$AGENT")" -# Get unread messages — escape newlines/tabs in body to keep one record per line +# Get unread messages — id first so the mark step below targets exactly these +# rows; escape newlines/tabs in body to keep one record per line UNREAD=$(agmsg_sqlite "$DB" " - SELECT from_agent || char(31) || replace(replace(body, char(10), '\n'), char(9), '\t') || char(31) || created_at + SELECT id || char(31) || from_agent || char(31) || replace(replace(body, char(10), '\n'), char(9), '\t') || char(31) || created_at FROM messages WHERE team='$TEAM_SQL' AND to_agent='$AGENT_SQL' AND read_at IS NULL ORDER BY created_at ASC; ") @@ -39,14 +40,36 @@ if [ -z "$UNREAD" ]; then exit 0 fi -# Display +# Display, collecting the ids actually shown COUNT=$(echo "$UNREAD" | wc -l | tr -d ' ') echo "$COUNT new message(s):" echo "" -while IFS=$'\x1f' read -r from body ts; do +IDS="" +while IFS=$'\x1f' read -r id from body ts; do echo " [$ts] $from: $body" + case "$id" in + ''|*[!0-9]*) ;; # defensive: never splice a non-numeric value into SQL + *) IDS="${IDS:+$IDS,}$id" ;; + esac done <<< "$UNREAD" echo "" -# Mark as read (non-fatal — may fail in sandboxed environments) -agmsg_sqlite "$DB" "UPDATE messages SET read_at=strftime('%Y-%m-%dT%H:%M:%SZ','now') WHERE team='$TEAM_SQL' AND to_agent='$AGENT_SQL' AND read_at IS NULL;" 2>/dev/null || true +# Test seam: a two-file barrier that lets the race regression test land a +# message deterministically between display and mark. No-op unless set. +if [ -n "${AGMSG_TEST_MARK_BARRIER:-}" ]; then + : > "$AGMSG_TEST_MARK_BARRIER.reached" + _agmsg_barrier_waited=0 + while [ ! -e "$AGMSG_TEST_MARK_BARRIER.release" ]; do + sleep 0.05 + _agmsg_barrier_waited=$((_agmsg_barrier_waited + 1)) + [ "$_agmsg_barrier_waited" -ge 200 ] && break # 10s safety cap + done +fi + +# Mark as read (non-fatal — may fail in sandboxed environments). +# Only the ids displayed above: a blanket "WHERE read_at IS NULL" would also +# swallow messages that arrived between the SELECT and this UPDATE — they +# would be marked read without ever having been shown. +if [ -n "$IDS" ]; then + agmsg_sqlite "$DB" "UPDATE messages SET read_at=strftime('%Y-%m-%dT%H:%M:%SZ','now') WHERE id IN ($IDS);" 2>/dev/null || true +fi diff --git a/tests/test_inbox.bats b/tests/test_inbox.bats new file mode 100644 index 00000000..50d5fa11 --- /dev/null +++ b/tests/test_inbox.bats @@ -0,0 +1,88 @@ +#!/usr/bin/env bats + +load test_helper + +setup() { + setup_test_env + bash "$SCRIPTS/join.sh" testteam alice claude-code /tmp/project-a + bash "$SCRIPTS/join.sh" testteam bob claude-code /tmp/project-a + DBPATH="$TEST_SKILL_DIR/db/messages.db" + BARRIER="$TEST_SKILL_DIR/mark-barrier" +} + +teardown() { + teardown_test_env +} + +unread_count() { + sqlite3 "$DBPATH" "SELECT COUNT(*) FROM messages WHERE team='testteam' AND to_agent='$1' AND read_at IS NULL;" | tr -d '\r' +} + +# Wait until the script under test has displayed and is paused before its +# mark UPDATE (barrier .reached appears), with a bounded wait. +await_barrier_reached() { + for _ in $(seq 1 100); do + [ -e "$BARRIER.reached" ] && return 0 + sleep 0.05 + done + return 1 +} + +# --- inbox.sh ----------------------------------------------------------- + +@test "inbox: displays unread messages and marks exactly those as read" { + bash "$SCRIPTS/send.sh" testteam bob alice "first" + bash "$SCRIPTS/send.sh" testteam bob alice "second" + run bash "$SCRIPTS/inbox.sh" testteam alice + [ "$status" -eq 0 ] + [[ "$output" == *"2 new message(s):"* ]] + [[ "$output" == *"first"* ]] + [[ "$output" == *"second"* ]] + [ "$(unread_count alice)" -eq 0 ] +} + +@test "inbox: --quiet is silent when there is nothing unread" { + run bash "$SCRIPTS/inbox.sh" testteam alice --quiet + [ "$status" -eq 0 ] + [ -z "$output" ] +} + +@test "inbox: a message arriving between display and mark is NOT marked read unseen" { + bash "$SCRIPTS/send.sh" testteam bob alice "early" + # Pause the run between display and mark, land a message inside the window, + # then release. With the old blanket "WHERE read_at IS NULL" mark, the late + # message was silently marked read without ever having been displayed. + AGMSG_TEST_MARK_BARRIER="$BARRIER" bash "$SCRIPTS/inbox.sh" testteam alice > "$TEST_SKILL_DIR/first-run.out" & + bg_pid=$! + await_barrier_reached + bash "$SCRIPTS/send.sh" testteam bob alice "late" + : > "$BARRIER.release" + wait "$bg_pid" + run cat "$TEST_SKILL_DIR/first-run.out" + [[ "$output" == *"early"* ]] + [[ "$output" != *"late"* ]] + # The late message must still be unread… + [ "$(unread_count alice)" -eq 1 ] + # …and surface on the next check + run bash "$SCRIPTS/inbox.sh" testteam alice + [ "$status" -eq 0 ] + [[ "$output" == *"late"* ]] + [ "$(unread_count alice)" -eq 0 ] +} + +# --- check-inbox.sh ------------------------------------------------------ + +@test "check-inbox: a message arriving between display and mark is NOT marked read unseen" { + bash "$SCRIPTS/send.sh" testteam bob alice "early" + AGMSG_TEST_MARK_BARRIER="$BARRIER" bash "$SCRIPTS/check-inbox.sh" claude-code /tmp/project-a > "$TEST_SKILL_DIR/check-run.out" 2>/dev/null & + bg_pid=$! + await_barrier_reached + bash "$SCRIPTS/send.sh" testteam bob alice "late" + : > "$BARRIER.release" + wait "$bg_pid" || true + run cat "$TEST_SKILL_DIR/check-run.out" + [[ "$output" == *"early"* ]] + [[ "$output" != *"late"* ]] + # The late message was not silently marked read by the first run + [ "$(unread_count alice)" -eq 1 ] +}