diff --git a/.github/enforced-assertions-baseline b/.github/enforced-assertions-baseline index 231a7d578..7d71bf4ad 100644 --- a/.github/enforced-assertions-baseline +++ b/.github/enforced-assertions-baseline @@ -1 +1 @@ -638 +635 diff --git a/install.sh b/install.sh index ae1031e8c..4400cb6d6 100755 --- a/install.sh +++ b/install.sh @@ -105,6 +105,26 @@ AGENT_TYPE="" # claude-code, codex, gemini, antigravity — passed via --agent- # the SKILL.md the installer itself had written with the wrong flavor. AGMSG_SHARED_SKILL_TPL_TYPES="gemini antigravity opencode hermes cursor grok-build" +# Put at , then remove any leftover . The arm is chosen by +# , so the fix's scope matches the defect's (#747): +# - regular : `mv` — an atomic rename, so an interrupted install leaves +# either the whole old config or the whole new one, never a torn file. This +# is the common path and must stay atomic. +# - symlinked : write THROUGH the link (a redirect follows it) so a +# config.toml managed as a symlink (stow/chezmoi/manual dotfiles) keeps its +# link and its target receives the edit. `mv` would replace the link with a +# plain file and strand the edit on a detached copy — the actual #747 bug. +# This arm is non-atomic (there is no atomic write-through-a-link with plain +# POSIX tools), but the exposure is confined to symlink users, whose target +# is typically a version-controlled dotfile. +move_into_place() { + if [ -L "$2" ]; then + cat "$1" > "$2" && rm -f "$1" + else + mv "$1" "$2" + fi +} + configure_codex_sandbox() { # --- Configure Codex sandbox (if Codex is installed) --- # The Codex bridge writes pidfiles/sockets/request files under the @@ -158,13 +178,13 @@ configure_codex_sandbox() { done=1 } { print } - ' "$code_config" > "$code_config.tmp" && mv "$code_config.tmp" "$code_config" + ' "$code_config" > "$code_config.tmp" && move_into_place "$code_config.tmp" "$code_config" elif grep -q '^\[sandbox_workspace_write\]' "$code_config" 2>/dev/null; then # Section exists but no writable_roots awk -v entries="$entries" ' { print } /^\[sandbox_workspace_write\]/ { print "writable_roots = [" entries "]" } - ' "$code_config" > "$code_config.tmp" && mv "$code_config.tmp" "$code_config" + ' "$code_config" > "$code_config.tmp" && move_into_place "$code_config.tmp" "$code_config" else # No section at all printf '\n[sandbox_workspace_write]\nwritable_roots = [%s]\n' "$entries" >> "$code_config" @@ -265,8 +285,22 @@ if [ "$UPDATE_ONLY" = true ]; then # exactly that, not "we ended up with some skill name one way or another"). CMD_WAS_EXPLICIT=false [ -n "$CMD_NAME" ] && CMD_WAS_EXPLICIT=true - # Find existing install. If --cmd was passed, update exactly that skill; - # otherwise preserve the historical "first installed agmsg skill" behavior. + # Find existing install. If --cmd was passed, update exactly that skill. + # Otherwise, scan for installs and require exactly one: a glob expands in + # collation order, not installation order, and nothing records which + # install came first, so guessing from a list of more than one is a + # silent coin flip on which install (and the shared ~/.agents/bin/codex + # shim it refreshes) gets updated (#599). A single install is unaffected + # -- this is the common case and it still "just works". + # + # No name-based exclusion for backup-shaped directories: --cmd has no + # reserved-name validation, so any pattern that would catch a real backup + # (e.g. "agmsg.bak-20260731") can equally match a legitimately chosen + # install name (e.g. "agmsg.bak-tool") -- there is no substring that is + # guaranteed to mean "not a real install" (co2 review, #659). A leftover + # backup directory that still carries the .agmsg marker is therefore just + # another candidate: it makes the set ambiguous, and ambiguous is exactly + # what this fix already refuses to guess through, below. if [ -n "$CMD_NAME" ]; then SKILL_DIR="$AGENTS_DIR/skills/$CMD_NAME" if [ ! -f "$SKILL_DIR/.agmsg" ]; then @@ -274,13 +308,23 @@ if [ "$UPDATE_ONLY" = true ]; then exit 1 fi else - SKILL_DIR="" + candidates=() for d in "$AGENTS_DIR"/skills/*/; do - if [ -f "${d}.agmsg" ]; then - SKILL_DIR="${d%/}" - break - fi + d="${d%/}" + [ -f "$d/.agmsg" ] && candidates+=("$d") done + case "${#candidates[@]}" in + 0) SKILL_DIR="" ;; + 1) SKILL_DIR="${candidates[0]}" ;; + *) + echo " ! Several agmsg installs found:" >&2 + for d in "${candidates[@]}"; do + echo " $(basename "$d")" >&2 + done + echo " ! --update with no --cmd cannot tell which one you mean. Pass --cmd to pick one." >&2 + exit 1 + ;; + esac fi if [ -z "$SKILL_DIR" ]; then echo " ! Not installed. Run ./install.sh first." >&2 diff --git a/scripts/remote.sh b/scripts/remote.sh index 4e6c2db02..45fb13efb 100644 --- a/scripts/remote.sh +++ b/scripts/remote.sh @@ -1952,7 +1952,7 @@ _remote_sync_engine_start_locked() { # Stop only an engine whose argv proves that it owns this team. A stale # pidfile may point at a recycled, unrelated process and must never authorize # signalling that process. - IFS=$'\t' read -r old_state old_pid < <(_remote_sync_engine_status "$team") + IFS=$'\t' read -r old_state old_pid < <(AGMSG_SKIP_SYSTEMD_PROBE=1 _remote_sync_engine_status "$team") if [ "$old_state" = "running" ]; then kill "$old_pid" 2>/dev/null || true fi @@ -2019,7 +2019,7 @@ _remote_sync_engine_stop() { local team="$1" pidfile pid state pidfile="$(_remote_sync_engine_pidfile "$team")" [ -f "$pidfile" ] || return 0 - IFS=$'\t' read -r state pid < <(_remote_sync_engine_status "$team") + IFS=$'\t' read -r state pid < <(AGMSG_SKIP_SYSTEMD_PROBE=1 _remote_sync_engine_status "$team") if [ "$state" = "running" ]; then if ! _remote_sync_engine_reap_owned "$team" "$pid"; then echo "agmsg: sync engine pid $pid did not stop" >&2 @@ -2041,11 +2041,73 @@ _remote_sync_engine_stop() { rm -f "$(_remote_sync_engine_cycle_stamp "$team")" 2>/dev/null || true } +# Return the systemd user-unit state as "statepid". +# +# A unit that is not installed is not a systemd-managed team here, so callers +# retain the pidfile behavior. An existing unit is different: an active unit +# whose MainPID cannot be authenticated is UNKNOWN and must not be shadowed by +# a new unmanaged engine. The command can be replaced in tests; no host +# systemd query is needed there. +_remote_systemd_engine_status() { + local team="$1" unit show load active sub pid command systemctl_bin + unit="agmsg-remote-sync-$team.service" + systemctl_bin="${AGMSG_SYSTEMCTL:-systemctl}" + command -v "$systemctl_bin" >/dev/null 2>&1 || { printf 'unavailable\t\n'; return; } + show="$($systemctl_bin --user show "$unit" -p LoadState -p ActiveState -p SubState -p MainPID 2>/dev/null)" || { + printf 'absent\t\n' + return + } + load="$(printf '%s\n' "$show" | sed -n 's/^LoadState=//p')" + active="$(printf '%s\n' "$show" | sed -n 's/^ActiveState=//p')" + sub="$(printf '%s\n' "$show" | sed -n 's/^SubState=//p')" + pid="$(printf '%s\n' "$show" | sed -n 's/^MainPID=//p')" + [ "$load" = "not-found" ] && { printf 'absent\t\n'; return; } + case "$active:$sub" in + active:running) + if _agmsg_pid_valid "$pid" && _agmsg_pid_alive_local "$pid"; then + command="$(compat_get_cmdline "$pid" 2>/dev/null || true)" + if agmsg_cmdline_names_path "$command" "$SCRIPT_DIR/internal/remote-sync.mjs" && + case "$command" in *" run --team $team") true ;; *) false ;; esac; then + printf 'running\t%s\n' "$pid" + else + printf 'unknown\t%s\n' "$pid" + fi + else + printf 'unknown\t%s\n' "$pid" + fi + ;; + active:starting|active:reloading|active:auto-restart|activating:*|deactivating:*) + printf 'starting\t%s\n' "$pid" + ;; + inactive:*|failed:*) + printf 'inactive\t%s\n' "$pid" + ;; + *) + printf 'unknown\t%s\n' "$pid" + ;; + esac +} + # Print "\t", where pid is empty when no valid pid is available. # A live PID is not enough: PID reuse can make an unrelated process pass # kill -0, so running requires the exact engine script/team suffix in argv. _remote_sync_engine_status() { - local team="$1" pidfile pid command expected + local team="$1" pidfile pid command expected systemd_state systemd_pid + REMOTE_SYNC_ENGINE_SUPERVISOR="" + REMOTE_SYNC_ENGINE_SUPERVISOR_PID="" + if [ "${AGMSG_SKIP_SYSTEMD_PROBE:-0}" != 1 ]; then + IFS=$'\t' read -r systemd_state systemd_pid < <(_remote_systemd_engine_status "$team") + else + systemd_state=absent + fi + case "$systemd_state" in + running|starting|inactive|unknown) + REMOTE_SYNC_ENGINE_SUPERVISOR="systemd" + REMOTE_SYNC_ENGINE_SUPERVISOR_PID="$systemd_pid" + printf '%s\t%s\n' "$systemd_state" "$systemd_pid" + return + ;; + esac pidfile="$(_remote_sync_engine_pidfile "$team")" if [ ! -f "$pidfile" ]; then printf 'stopped\t\n' @@ -2077,7 +2139,7 @@ _remote_sync_engine_status() { _remote_sync_engine_reap_owned() { local team="$1" owned_pid="$2" state pid signal attempts for signal in TERM KILL; do - IFS=$'\t' read -r state pid < <(_remote_sync_engine_status "$team") + IFS=$'\t' read -r state pid < <(AGMSG_SKIP_SYSTEMD_PROBE=1 _remote_sync_engine_status "$team") if ! _agmsg_pid_alive_local "$owned_pid"; then return 0; fi [ "$state" = "running" ] && [ "$pid" = "$owned_pid" ] || return 1 kill "-$signal" "$owned_pid" 2>/dev/null || true @@ -2489,6 +2551,12 @@ _remote_status_one() { case "$engine_state" in running) echo "$team connected (engine running, pid $engine_pid) since $connected_at" ;; + starting) + echo "$team connected (engine starting under systemd, pid $engine_pid; do not run sync start) since $connected_at" ;; + inactive) + echo "$team connected (engine inactive under systemd; run: systemctl --user restart agmsg-remote-sync-$team.service) since $connected_at" ;; + unknown) + echo "$team connected (engine state unknown under systemd; do not run sync start; inspect systemctl --user status agmsg-remote-sync-$team.service) since $connected_at" ;; stopped) echo "$team connected (engine stopped — run: bash $(agmsg_shq "$SKILL_DIR/scripts/remote.sh") sync start $(agmsg_shq "$team")) since $connected_at" ;; stale) @@ -2818,11 +2886,18 @@ cmd_sync_start() { fi IFS=$'\t' read -r engine_state engine_pid < <(_remote_sync_engine_status "$team") - if [ "$engine_state" = "running" ]; then - echo "Sync engine already running (pid $engine_pid)." - agmsg_lock_release - return - fi + case "$engine_state" in + running) + echo "Sync engine already running (pid $engine_pid)." + agmsg_lock_release + return + ;; + starting|inactive|unknown) + echo "agmsg: systemd owns team '$team' in state '$engine_state'; inspect or restart the user unit instead of sync start" >&2 + agmsg_lock_release + return 1 + ;; + esac logfile="$CONNECTION_ROOT/run/remote-sync.$team.log" [ -f "$logfile" ] && log_offset=$(( $(wc -c < "$logfile" | tr -d ' ') + 1 )) @@ -2872,7 +2947,7 @@ cmd_sync_start() { # not the rest of the machine. agmsg_lock_release while [ "$i" -lt 1600 ]; do - IFS=$'\t' read -r engine_state ready_pid < <(_remote_sync_engine_status "$team") + IFS=$'\t' read -r engine_state ready_pid < <(AGMSG_SKIP_SYSTEMD_PROBE=1 _remote_sync_engine_status "$team") if [ "$engine_state" = "running" ] && [ "$ready_pid" = "$started_pid" ] && tail -c "+$log_offset" "$logfile" 2>/dev/null | awk -v nonce="\"startup_nonce\":\"$startup_nonce\"" ' @@ -3367,7 +3442,7 @@ cmd_set_endpoint() { done fi - IFS=$'\t' read -r engine_state engine_pid < <(_remote_sync_engine_status "$team") + IFS=$'\t' read -r engine_state engine_pid < <(AGMSG_SKIP_SYSTEMD_PROBE=1 _remote_sync_engine_status "$team") [ "$engine_state" = "running" ] && was_running=1 _remote_sync_engine_stop "$team" || { echo "agmsg: the sync engine did not stop; refusing to move the endpoint under it" >&2 @@ -3398,7 +3473,7 @@ cmd_set_endpoint() { # command ran is restarted too (never silently left stopped, and a restart # is what hands it the moved address -- a running engine keeps its old # config in memory). _remote_sync_engine_start kills a live engine first. - IFS=$'\t' read -r end_state end_pid < <(_remote_sync_engine_status "$team") + IFS=$'\t' read -r end_state end_pid < <(AGMSG_SKIP_SYSTEMD_PROBE=1 _remote_sync_engine_status "$team") if [ "$was_running" -eq 1 ] || [ "$end_state" = "running" ]; then # Same rule as cmd_pull and cmd_connect: the move is this command's purpose # and it is done by here, so a start failure reports rather than fails -- diff --git a/scripts/session-start.sh b/scripts/session-start.sh index 51f7ff4e5..7dd0cc6de 100755 --- a/scripts/session-start.sh +++ b/scripts/session-start.sh @@ -316,9 +316,12 @@ fi # ROLE-FILTERED directive instead of the generic unfiltered one: watch.sh with a # 4th arg restricts receive to that role AND re-claims its exclusivity # lock. This covers a manual `claude --resume ` that bypasses spawn's actas -# boot prompt -- the resumed session re-arms as its role automatically. Fail-open: -# no record, no project match, or an unreadable record => generic directive. -ROLE_NAME=""; ROLE_TEAM="" +# boot prompt -- the resumed session re-arms as its role automatically. When no +# record matches, narrowing (#982) tries the actas lock this sid owns; if the +# seat still cannot be established the fallback is fail-CLOSED, not the generic +# unfiltered watcher (which would consume other seats' unread) -- see the two +# blocks below. +ROLE_NAME=""; ROLE_TEAM=""; ROLE_BASIS="" _bare_sid="$(agmsg_instance_bare_sid "$SESSION_ID" 2>/dev/null || printf '%s' "$SESSION_ID")" _rec="$(agmsg_role_session_lookup_by_sid "$_bare_sid" 2>/dev/null || true)" if [ -n "$_rec" ]; then @@ -328,7 +331,38 @@ if [ -n "$_rec" ]; then # (team, agent) is actually one of this project's registered pairs. if [ -n "$_r_agent" ] && [ -n "$_r_team" ] \ && printf '%s\n' "$PAIRS" | grep -Fxq "$(printf '%s\t%s' "$_r_team" "$_r_agent")"; then - ROLE_NAME="$_r_agent"; ROLE_TEAM="$_r_team" + ROLE_NAME="$_r_agent"; ROLE_TEAM="$_r_team"; ROLE_BASIS=record + fi +fi + +# --- Narrowing when the role-session record is missing (#982). --- +# The record above is advisory and can be absent even for a session that IS a +# seat (a resume that bypassed actas-claim, an unreadable record). The same fact +# it would carry may still be on disk: an actas.__.session lock this +# very sid owns. Match the lock owner's BARE sid (stable across resume; the pid +# half changes) against ours, iterating THIS project's registered pairs rather +# than raw lock filenames (those are percent-encoded, and iterating PAIRS keeps +# us to locks that are actually registered here). Exactly one match re-seats us; +# zero leaves ROLE_NAME empty for the fail-closed decision below, and an ambiguous +# 2+ deliberately does the same — an unfiltered watcher is the one thing we must +# not fall back to (it consumes other seats' unread; see the block after the +# role-filtered emit). +if [ -z "$ROLE_NAME" ]; then + _narrow_n=0; _narrow_agent=""; _narrow_team="" + _tab="$(printf '\t')" + while IFS="$_tab" read -r _p_team _p_agent; do + [ -n "$_p_team" ] && [ -n "$_p_agent" ] || continue + _owner="$(actas_lock_owner "$_p_team" "$_p_agent" 2>/dev/null || true)" + [ -n "$_owner" ] || continue + _owner_bare="$(agmsg_instance_bare_sid "$_owner" 2>/dev/null || printf '%s' "$_owner")" + if [ "$_owner_bare" = "$_bare_sid" ]; then + _narrow_n=$((_narrow_n + 1)); _narrow_agent="$_p_agent"; _narrow_team="$_p_team" + fi + done < arg. if [ -n "$ROLE_NAME" ]; then WATCH_COMMAND="$(printf '%q %q %q %q %q' "$WATCH" "$INSTANCE_ID" "$PROJECT" "$TYPE" "$ROLE_NAME")" + # State the seat's basis honestly: the reader launches a watcher on the strength + # of this sentence, so a recorded seat and an inferred one must not read alike + # (#982/#993). The record path has an explicit role-session record; the narrowing + # path has only the actas lock this sid still owns — say which, and why it stands + # in for the record, so the reader can weigh "what if the lock is stale?". + if [ "$ROLE_BASIS" = record ]; then + SEAT_CLAIM="this session was recorded as that role's seat" + else + SEAT_CLAIM="no role record was found for this session, but it still owns that role's actas exclusivity lock — claimed by this seat and carried across the resume — which is taken to stand in for the record (so if that lock were stale, this seating would be too)" + fi cat <` so the seat can be set +# explicitly — which re-fires this hook down the role-filtered path above. +_pair_count="$(printf '%s\n' "$PAIRS" | grep -c '.' || true)" +if [ "${_pair_count:-0}" -le 1 ]; then + WATCH_COMMAND="$(printf '%q %q %q %q' "$WATCH" "$INSTANCE_ID" "$PROJECT" "$TYPE")" + cat <=2 && $2!="" {print " - /agmsg actas "$2}')" +cat < \` +returns them). What is paused is live delivery into THIS session. + +To start receiving as your seat, claim it explicitly — this re-fires the monitor +directive on the role-filtered path: + +$_seat_list + +If you are not any of these seats, no watcher is the correct state. +EOF +exit 0 diff --git a/tests/test_apply_fail_stderr.bats b/tests/test_apply_fail_stderr.bats new file mode 100644 index 000000000..73608d12a --- /dev/null +++ b/tests/test_apply_fail_stderr.bats @@ -0,0 +1,46 @@ +#!/usr/bin/env bats + +# `_sqlite_sync_apply_fail` must not take the shell's stderr with it. +# +# It closes fd 3 and discards whatever closing an already-closed fd would say. +# Written as `exec 3<&- 2>/dev/null`, with no command word, the redirection is +# not scoped to anything -- bash applies it to the shell itself, permanently. +# Every later `>&2` in that process goes to /dev/null. +# +# The path this sits on is the failure path, so the diagnostics it silences are +# the ones a failing apply is about to print. The first casualty is the message +# #911 added, naming which check returned 13. Reported by @JoelMitz. + +setup() { + SCRIPTS="${BATS_TEST_DIRNAME}/../scripts" +} + +@test "apply-fail: closing fd 3 does not redirect the shell's stderr (#911)" { + # Runs the driver's OWN definition, not a copy of it: the function is nested + # inside storage_sync_apply_pull and cannot be sourced, so its body is lifted + # out of the file by line range and defined here. A copy would keep passing + # while the driver regressed, which is the whole failure this test is about. + local first last body + first="$(grep -n '_sqlite_sync_apply_fail() {' "${SCRIPTS}/drivers/storage/sqlite-sync.sh" | head -1 | cut -d: -f1)" + [ -n "$first" ] + last="$(awk -v s="$first" 'NR>s && /^ \}$/ { print NR; exit }' "${SCRIPTS}/drivers/storage/sqlite-sync.sh")" + [ -n "$last" ] + body="$(sed -n "${first},${last}p" "${SCRIPTS}/drivers/storage/sqlite-sync.sh")" + + run bash -c " + jq_err=/dev/null; sql_file=/dev/null + $body + _sqlite_sync_apply_fail + echo 'stderr-after-close' >&2 + " + [ "$status" -eq 0 ] + grep -q "stderr-after-close" <<<"$output" +} + +@test "apply-fail: the driver closes fd 3 in a scoped block, not bare exec" { + # The regression is one character of syntax, so the guard is on the syntax. + # A bare `exec ` with no command word anywhere in this file would + # take the shell's stderr the same way. + run grep -nE '^\s*exec [0-9]*[<>][&-]* +[0-9]*>' "${SCRIPTS}/drivers/storage/sqlite-sync.sh" + [ "$status" -ne 0 ] +} diff --git a/tests/test_codex_bridge_launcher.bats b/tests/test_codex_bridge_launcher.bats index 0e8de0398..8f6b72ec4 100644 --- a/tests/test_codex_bridge_launcher.bats +++ b/tests/test_codex_bridge_launcher.bats @@ -510,22 +510,77 @@ wait_for_child_count() { # --- #937: reap a same-(project,role) orphan via its per-PID identity lease --- -_count_role_bridges() { # - # Match the role name at a word boundary, not as a substring: "alice" must not - # also count "alice2" (the survive tests turn on exactly that distinction), so a - # trailing digit/letter excludes it. Names here are plain [a-z0-9] test tokens. - ps -Ao pid=,args= 2>/dev/null | grep -F "codex-bridge.js" | grep -F -- "--project $1 " | grep -E "$2([^0-9A-Za-z]|\$)" | grep -c . | tr -d ' ' -} -# Poll until this project's role-bridge count settles at , then echo it -- -# a fixed sleep before a point-in-time count races the reap-and-respawn. -_wait_role_count() { # - local i +# Bridges whose pair set is EXACTLY {}: one `--pair`, and that pair naming +# . Distinct from a plain argv match on -- the form these tests used +# until #984 -- which counts any process with anywhere in its argv, +# including a bridge that serves alongside others. +# +# That distinction is the one this file's superset test already turns on: a +# bridge for {alice,bob} is not a bridge for {alice}. An argv match cannot see +# it, so it cannot be used to wait for "the launcher has spawned its alice" in a +# test that has itself just spawned an {alice,bob} bridge -- the gate opens on +# the test's own fixture, before the launcher has done anything (#937). +# +# Counted with `grep`, not by splitting fields: a pair value is `teamname`, +# and awk splits on tabs as well as spaces whatever `-F` says about the input, +# so `$(i+1)` after `--pair` is only `team`. Measured that way first and it +# counted 0 for a bridge that plainly had one alice pair. +_count_exact_role_bridges() { # + ps -Ao pid=,args= 2>/dev/null \ + | grep -F "codex-bridge.js" \ + | grep -F -- "--project $1 " \ + | while IFS= read -r line; do + # Exactly one --pair, and its value's NAME half is . A pair is + # `teamname`, and the separator has two renderings to allow for: + # `ps` writes the tab as the four characters \011 -- three of them + # digits, so a single-character [^0-9A-Za-z] separator class matches + # nothing at all and the counter answers 0 to everything (#984). + [ "$(printf '%s' "$line" | grep -o -- '--pair' | grep -c .)" -eq 1 ] || continue + printf '%s' "$line" | grep -Eq -- "--pair [^ ]*(\\\\011|[^0-9A-Za-z])$2([^0-9A-Za-z]|\$)" || continue + printf '.\n' + done | grep -c . | tr -d ' ' +} + +# Poll until the exact-{} count settles at , then echo THE VALUE THAT +# SATISFIED THE WAIT. +# +# The form these tests used until #984 re-counted after its loop, so what it +# returned was a second, later observation. Between the two, the reaper kills the +# orphan and the launcher respawns, and the count passes through 2 and through 0 +# -- so the caller was handed a number that nothing had ever waited for. That is +# the half of #984 needing no superset fixture, and it is why all five call sites +# could fail, not only the superset one. +_wait_exact_role_count() { # + local i seen for i in {1..100}; do - [ "$(_count_role_bridges "$1" "$2")" -eq "$3" ] && break + seen="$(_count_exact_role_bridges "$1" "$2")" + [ "$seen" = "$3" ] && { printf '%s' "$seen"; return 0; } sleep 0.1 done - _count_role_bridges "$1" "$2" + printf '%s' "$seen" +} + +# The gate the five reap tests open before they make an orphan: the launcher +# must actually have ONE bridge for exactly {} first. +# +# It has to be load-bearing, and a `for ... && break` loop is not. Exhausting +# such a loop and breaking out of it are indistinguishable from the next line, +# so a test whose launcher never spawned would go on to `rm -f` pidfiles that do +# not exist, wait, and then be satisfied by a bridge the launcher started DURING +# that wait -- green, having created no orphan and reaped none. The test would +# pass without exercising #937 at all, and nothing would say so (#984). +# +# Same rule as the team-lock gate in test_remote_engine_start_refusal.bats: when +# a precondition cannot be established, say which count was actually reached and +# fail, rather than continuing into an assertion that no longer means what it +# says. +_require_launcher_bridge() { # + local seen; seen="$(_wait_exact_role_count "$1" "$2" 1)" + [ "$seen" = 1 ] && return 0 + echo "the launcher never reached one {$2} bridge (saw $seen), so this test could not create the orphan it is about" >&2 + return 1 } + # Run the mock bridge directly for a given (project, pairs) so it publishes a # lease of that identity and stays alive. Sets FAKE_PID (NOT via $(...) -- a # background job in command substitution is killed when that subshell exits). @@ -537,15 +592,64 @@ _spawn_fake() { # FAKE_PID=$! } +@test "launcher: the exact-role counter reads a pair set, not an argv substring (#984)" { + export MOCK_BRIDGE_SLEEP=25 + local tab; tab=$(printf '\t') + _spawn_fake "$PROJ" "team${tab}alice" "team${tab}bob"; local both=$FAKE_PID + _spawn_fake "$PROJ" "team${tab}alice2"; local two=$FAKE_PID + _spawn_fake "$TEST_SKILL_DIR/other-proj" "team${tab}alice"; local other=$FAKE_PID + # Positive control FIRST. Without it, the zeroes below are also what a counter + # that answers 0 to everything produces -- including one whose pattern never + # matches the separator `ps` renders between a pair's team and its name (it is + # a tab, and `ps` writes it as the four characters \011, three of them digits). + [ "$(_wait_exact_role_count "$PROJ" alice2 1)" -eq 1 ] + # None of the three is a bridge whose pair set is {alice} in THIS project: + # {alice,bob} is a superset, {alice2} collides only by prefix, and the third + # is another project's. + [ "$(_count_exact_role_bridges "$PROJ" alice)" -eq 0 ] + # ... and {alice,bob} is not {bob} either -- the rule is set equality, not + # "serves this role". Asked through the waiter, with a `want` of 1 it will + # never reach: this is the one assertion here that goes red if the waiter is + # ever rewritten to return its `want` instead of what it saw. Every other + # check in this file passes under that rewrite, which is the shape of the + # defect being fixed (#984). It costs the waiter's full 10s by design. + [ "$(_wait_exact_role_count "$PROJ" bob 1)" -eq 0 ] + # One that IS {alice} counts, with the other three still running. + _spawn_fake "$PROJ" "team${tab}alice"; local solo=$FAKE_PID + [ "$(_wait_exact_role_count "$PROJ" alice 1)" -eq 1 ] + kill "$both" "$two" "$other" "$solo" 2>/dev/null || true + wait "$both" 2>/dev/null || true; wait "$two" 2>/dev/null || true + wait "$other" 2>/dev/null || true; wait "$solo" 2>/dev/null || true +} + +@test "launcher: an unreachable gate fails the test instead of continuing (#984)" { + # No launcher is started here at all, so the gate's condition can never be + # reached. It has to END the test. + # + # This is the control for the five reap tests: each calls the gate bare, so an + # unreachable precondition fails them -- but ONLY if the gate returns non-zero + # on exhaustion. The `for ... && break` gate it replaces returned nothing at + # all: reaching the count and running out of tries left the same state behind, + # and the test carried on to delete pidfiles that did not exist and assert + # against a bridge started during the wait. Green, with #937 never exercised. + # + # Costs the waiter's full sweep by design -- an exhausted gate is what is + # being measured, so it cannot be short-circuited. + run _require_launcher_bridge "$PROJ" alice + [ "$status" -ne 0 ] + # And it must say WHICH count it reached: an exhausted gate that fails with a + # bare non-zero tells the next reader nothing about why. + printf '%s' "$output" | grep -q 'never reached one {alice} bridge (saw 0)' +} + @test "launcher: reaps a same-(project,role) orphan the pidfile lost, converging to one (#937)" { put_record team alice thread-alice "$PROJ" codex export MOCK_BRIDGE_SLEEP=25 sleep 22 3>&- & local parent=$! bash "$LAUNCHER" codex "$PROJ" "ws://127.0.0.1:1" "$parent" >/dev/null 2>&1 3>&- & local disp=$! - local i; for i in {1..80}; do [ "$(_count_role_bridges "$PROJ" alice)" -ge 1 ] && break; sleep 0.1; done - [ "$(_count_role_bridges "$PROJ" alice)" -eq 1 ] + _require_launcher_bridge "$PROJ" alice rm -f "$RUN_DIR"/codex-bridge.*.pid - [ "$(_wait_role_count "$PROJ" alice 1)" -eq 1 ] + [ "$(_wait_exact_role_count "$PROJ" alice 1)" -eq 1 ] kill "$disp" "$parent" 2>/dev/null || true; wait "$disp" 2>/dev/null || true } @@ -556,9 +660,9 @@ _spawn_fake() { # _spawn_fake "$PROJ" "team${tab}bob"; local bob=$FAKE_PID sleep 22 3>&- & local parent=$! bash "$LAUNCHER" codex "$PROJ" "ws://127.0.0.1:1" "$parent" >/dev/null 2>&1 3>&- & local disp=$! - local i; for i in {1..80}; do [ "$(_count_role_bridges "$PROJ" alice)" -ge 1 ] && break; sleep 0.1; done + _require_launcher_bridge "$PROJ" alice rm -f "$RUN_DIR"/codex-bridge.*.pid - [ "$(_wait_role_count "$PROJ" alice 1)" -eq 1 ] + [ "$(_wait_exact_role_count "$PROJ" alice 1)" -eq 1 ] kill -0 "$bob" kill "$bob" "$disp" "$parent" 2>/dev/null || true; wait "$disp" 2>/dev/null || true; wait "$bob" 2>/dev/null || true } @@ -570,9 +674,9 @@ _spawn_fake() { # _spawn_fake "$TEST_SKILL_DIR/other-proj" "team${tab}alice"; local other=$FAKE_PID sleep 22 3>&- & local parent=$! bash "$LAUNCHER" codex "$PROJ" "ws://127.0.0.1:1" "$parent" >/dev/null 2>&1 3>&- & local disp=$! - local i; for i in {1..80}; do [ "$(_count_role_bridges "$PROJ" alice)" -ge 1 ] && break; sleep 0.1; done + _require_launcher_bridge "$PROJ" alice rm -f "$RUN_DIR"/codex-bridge.*.pid - [ "$(_wait_role_count "$PROJ" alice 1)" -eq 1 ] + [ "$(_wait_exact_role_count "$PROJ" alice 1)" -eq 1 ] kill -0 "$other" kill "$other" "$disp" "$parent" 2>/dev/null || true; wait "$disp" 2>/dev/null || true; wait "$other" 2>/dev/null || true } @@ -584,9 +688,9 @@ _spawn_fake() { # _spawn_fake "$PROJ" "team${tab}alice2"; local alice2=$FAKE_PID sleep 22 3>&- & local parent=$! bash "$LAUNCHER" codex "$PROJ" "ws://127.0.0.1:1" "$parent" >/dev/null 2>&1 3>&- & local disp=$! - local i; for i in {1..80}; do [ "$(_count_role_bridges "$PROJ" alice)" -ge 1 ] && break; sleep 0.1; done + _require_launcher_bridge "$PROJ" alice rm -f "$RUN_DIR"/codex-bridge.*.pid - [ "$(_wait_role_count "$PROJ" alice 1)" -eq 1 ] + [ "$(_wait_exact_role_count "$PROJ" alice 1)" -eq 1 ] kill -0 "$alice2" kill "$alice2" "$disp" "$parent" 2>/dev/null || true; wait "$disp" 2>/dev/null || true; wait "$alice2" 2>/dev/null || true } @@ -598,9 +702,16 @@ _spawn_fake() { # _spawn_fake "$PROJ" "team${tab}alice" "team${tab}bob"; local both=$FAKE_PID sleep 22 3>&- & local parent=$! bash "$LAUNCHER" codex "$PROJ" "ws://127.0.0.1:1" "$parent" >/dev/null 2>&1 3>&- & local disp=$! - local i; for i in {1..80}; do [ "$(_count_role_bridges "$PROJ" alice)" -ge 1 ] && break; sleep 0.1; done + # `both` carries `--pair teamalice`, so an argv match answers 1 for it + # before the launcher has spawned anything: this is the one site where the + # gate opened on the test's own fixture. The exact counter requires a pair set + # of {alice}, the same set inequality the `kill -0 "$both"` below relies on. + _require_launcher_bridge "$PROJ" alice rm -f "$RUN_DIR"/codex-bridge.*.pid - [ "$(_wait_role_count "$PROJ" alice 1)" -eq 1 ] + # ONE exact-{alice} bridge: the launcher's. `both` is not counted here -- the + # `kill -0` below is what says it survived. The two assertions carry different + # halves of this test's claim. + [ "$(_wait_exact_role_count "$PROJ" alice 1)" -eq 1 ] # Its pair set is {alice,bob}, not {alice}: set inequality spares it. kill -0 "$both" kill "$both" "$disp" "$parent" 2>/dev/null || true; wait "$disp" 2>/dev/null || true; wait "$both" 2>/dev/null || true diff --git a/tests/test_despawn.bats b/tests/test_despawn.bats index 374e93fba..7e6e8fa90 100644 --- a/tests/test_despawn.bats +++ b/tests/test_despawn.bats @@ -92,19 +92,13 @@ _control_row_exists_for_alice() { # broad (non-actas) watcher that later scans this project's inbox must not # see it resurface as a "new" message (2026-07-19 review finding). _control_row_exists_for_alice - # LEFT AS `!` ON PURPOSE, and it is the one exception in this change. - # - # Converting it to `refute` enforces the assertion -- and enforced, it fails - # under load: green run alone three times, green with this file alone, red in - # a ten-file sweep. So the condition it checks (the ctrl:despawn row is - # already marked read at this point) is not reliably true when the machine is - # busy. That is a timing weakness the silence has been covering, not - # something this change introduced, and fixing it is a different job (#715, - # which carries the reproduction). - # - # Enforcing it here would trade a hidden weakness for an unstable CI, which - # is a worse deal than leaving one assertion visibly listed in the baseline. - ! _is_unread_for_alice "ctrl:despawn" + # `refute`, not a bare `!` (#715). `! cmd` is exempt from errexit on every bash, + # so `! _is_unread_for_alice ...` reported ok even when the row WAS unread — the + # assertion was written but watched nothing (#670). `refute` makes it fail when + # the row lingers unread. The separate, load-dependent flake this then exposes + # (the row not yet read right after despawn returns, under load) is NOT fixed + # here; it stays open as #715. + refute _is_unread_for_alice "ctrl:despawn" kill "$wpid" 2>/dev/null || true; wait "$wpid" 2>/dev/null || true } diff --git a/tests/test_install.bats b/tests/test_install.bats index 246b05a10..e29d4f9e4 100644 --- a/tests/test_install.bats +++ b/tests/test_install.bats @@ -78,13 +78,59 @@ teardown() { run env HOME="$FAKE_HOME" AGMSG_FORCE_WINDOWS=1 bash "$REPO_ROOT/install.sh" --cmd agmsg --update [ "$status" -eq 0 ] - [[ "$output" =~ "Updating agmsg..." ]] - [[ ! "$output" =~ "Updating agmsg.backup-keep" ]] + printf '%s\n' "$output" | grep -Fq "Updating agmsg..." + refute grep -Fq "Updating agmsg.backup-keep" <<<"$output" [ ! -f "$FAKE_HOME/.agents/agmsg.ps1" ] [ ! -f "$FAKE_HOME/.agents/agmsg.backup-keep.ps1" ] grep -q "backup sentinel" "$backup/SKILL.md" } +@test "install: --update with no --cmd refuses to guess between two real installs (#599)" { + HOME="$FAKE_HOME" bash "$REPO_ROOT/install.sh" --cmd agmsg + HOME="$FAKE_HOME" bash "$REPO_ROOT/install.sh" --cmd agmsg-second + # Distinct per-install sentinels, not just each install's VERSION (which is + # the same source-derived string for both and would not distinguish "one of + # them got silently updated" from "neither did" -- co2 review, #659). + echo "agmsg sentinel" > "$FAKE_HOME/.agents/skills/agmsg/SKILL.md" + echo "agmsg-second sentinel" > "$FAKE_HOME/.agents/skills/agmsg-second/SKILL.md" + + run env HOME="$FAKE_HOME" AGMSG_FORCE_WINDOWS=1 bash "$REPO_ROOT/install.sh" --update + [ "$status" -ne 0 ] + printf '%s\n' "$output" | grep -Fq "Several agmsg installs found" + printf '%s\n' "$output" | grep -Fq "agmsg" + printf '%s\n' "$output" | grep -Fq "agmsg-second" + # Neither install was touched -- this is a refusal, not a guess. + grep -q "agmsg sentinel" "$FAKE_HOME/.agents/skills/agmsg/SKILL.md" + grep -q "agmsg-second sentinel" "$FAKE_HOME/.agents/skills/agmsg-second/SKILL.md" +} + +@test "install: --update with no --cmd treats a leftover backup-shaped directory as another candidate, not a silent exclusion (#599)" { + # No code in this repo creates a ".bak-"-named directory -- that name is a + # human backup convention, not something install.sh generates. A pattern + # narrow enough to exclude it is therefore also narrow enough to still + # exclude nothing on a real machine, while remaining broad enough to + # collide with a legitimately chosen --cmd name (--cmd has no reserved-name + # validation: "agmsg.bak-tool" installs today with no error). Two rounds of + # narrowing hit that same collision from co2 review on #659; the fix is to + # not special-case names at all. A directory that still carries the .agmsg + # marker is just another candidate, and more than one candidate is exactly + # the ambiguity this fix already refuses to guess through. + HOME="$FAKE_HOME" bash "$REPO_ROOT/install.sh" --cmd agmsg + local leftover="$FAKE_HOME/.agents/skills/agmsg.bak-20260731" + mkdir -p "$leftover/scripts" "$leftover/templates" "$leftover/db" "$leftover/agents" + touch "$leftover/.agmsg" + echo "leftover sentinel" > "$leftover/SKILL.md" + echo "agmsg sentinel" > "$FAKE_HOME/.agents/skills/agmsg/SKILL.md" + + run env HOME="$FAKE_HOME" AGMSG_FORCE_WINDOWS=1 bash "$REPO_ROOT/install.sh" --update + [ "$status" -ne 0 ] + printf '%s\n' "$output" | grep -Fq "Several agmsg installs found" + printf '%s\n' "$output" | grep -Fq "agmsg" + printf '%s\n' "$output" | grep -Fq "agmsg.bak-20260731" + grep -q "agmsg sentinel" "$FAKE_HOME/.agents/skills/agmsg/SKILL.md" + grep -q "leftover sentinel" "$leftover/SKILL.md" +} + @test "install: Claude Code command file gates actas/drop's fresh Monitor on delivery mode (#280)" { # actas/drop used to invoke a fresh Monitor unconditionally, ignoring # mode=off/turn (#280) — this is prompt-instruction text, not executable @@ -563,6 +609,67 @@ PY fi } +@test "install: a symlinked Codex config.toml keeps its link and the edit lands on the target (#747, writable_roots exists)" { + mkdir -p "$FAKE_HOME/.codex" "$FAKE_HOME/dotfiles" + # The reporter's exact shape: writable_roots already present with an entry, and + # config.toml is a symlink into a dotfiles repo (stow/chezmoi/manual). + cat > "$FAKE_HOME/dotfiles/config.toml" <<'EOF' +[sandbox_workspace_write] +writable_roots = ["/some/existing/path"] +EOF + ln -s "$FAKE_HOME/dotfiles/config.toml" "$FAKE_HOME/.codex/config.toml" + [ -L "$FAKE_HOME/.codex/config.toml" ] || skip "filesystem did not create a real symlink here" + + HOME="$FAKE_HOME" bash "$REPO_ROOT/install.sh" --cmd agmsg + + # The link survives: `mv` would have replaced it with a plain file (#747). + [ -L "$FAKE_HOME/.codex/config.toml" ] + # The edit reached the link's target, not a detached copy at the link path. + grep -q "$SK/db" "$FAKE_HOME/dotfiles/config.toml" + grep -q "$SK/teams" "$FAKE_HOME/dotfiles/config.toml" + grep -q "$SK/run" "$FAKE_HOME/dotfiles/config.toml" + # The pre-existing entry is kept. + grep -q "/some/existing/path" "$FAKE_HOME/dotfiles/config.toml" +} + +@test "install: a symlinked Codex config.toml keeps its link when only the section exists (#747, second branch)" { + mkdir -p "$FAKE_HOME/.codex" "$FAKE_HOME/dotfiles" + # Section present, no writable_roots — the other mv-based branch. + cat > "$FAKE_HOME/dotfiles/config.toml" <<'EOF' +[sandbox_workspace_write] +EOF + ln -s "$FAKE_HOME/dotfiles/config.toml" "$FAKE_HOME/.codex/config.toml" + [ -L "$FAKE_HOME/.codex/config.toml" ] || skip "filesystem did not create a real symlink here" + + HOME="$FAKE_HOME" bash "$REPO_ROOT/install.sh" --cmd agmsg + + [ -L "$FAKE_HOME/.codex/config.toml" ] + grep -q "$SK/db" "$FAKE_HOME/dotfiles/config.toml" + grep -q "$SK/run" "$FAKE_HOME/dotfiles/config.toml" +} + +@test "install: an ordinary Codex config.toml is replaced atomically, not truncated in place (#747 control)" { + mkdir -p "$FAKE_HOME/.codex" + cat > "$FAKE_HOME/.codex/config.toml" <<'EOF' +[sandbox_workspace_write] +writable_roots = ["/some/existing/path"] +EOF + # The reverse of the symlink tests, guarding the ordinary-file arm so the atomic + # mv cannot be dropped again unseen (#747). An atomic `mv` gives the destination + # a NEW inode (the temp file's); a truncate-then-write (`cat >`, the symlink arm) + # keeps the old inode. So an unchanged inode here would mean the ordinary path + # silently became non-atomic. + local ino_before; ino_before="$(ls -i "$FAKE_HOME/.codex/config.toml" | awk '{print $1}')" + + HOME="$FAKE_HOME" bash "$REPO_ROOT/install.sh" --cmd agmsg + + [ ! -L "$FAKE_HOME/.codex/config.toml" ] + grep -q "$SK/db" "$FAKE_HOME/.codex/config.toml" + grep -q "/some/existing/path" "$FAKE_HOME/.codex/config.toml" + local ino_after; ino_after="$(ls -i "$FAKE_HOME/.codex/config.toml" | awk '{print $1}')" + [ "$ino_after" != "$ino_before" ] +} + # --- hermes Agent skill (~/.hermes/skills//SKILL.md) --- @@ -714,14 +821,18 @@ PY @test "install: bare --update (no --cmd) does NOT force-steal a Codex shim owned by a different install (#553)" { # Unlike --update --cmd , a bare --update resolves its target by - # scanning for an existing install rather than the caller naming one --- and - # on this base (#599's fail-closed fix, PR #659, is not yet merged here), - # that resolution does not even fail closed when more than one install is - # present. Forcing the shim reclaim unconditionally for bare --update would - # let whichever install a glob happens to land on steal the shim from - # another one the caller never named at all (review finding). This pins - # that a shim already owned by a DIFFERENT install survives a bare --update - # of the install that does NOT own it. + # scanning for an existing install rather than the caller naming one. + # Forcing the shim reclaim unconditionally for bare --update would let + # whichever install the scan landed on steal the shim from another one the + # caller never named at all (review finding). This pins that a shim already + # owned by a DIFFERENT install survives a bare --update. + # + # Since #599 (PR #659) the scan fails closed when more than one install is + # present, so with two installs a bare --update now refuses before it + # touches anything -- which is the strongest form of "does not steal": the + # refusal is asserted, and the shim's owner line is asserted unchanged + # across it. The single-install case, where a bare --update does proceed, + # is the next test. HOME="$FAKE_HOME" bash "$REPO_ROOT/install.sh" --cmd agmsg --agent-type codex HOME="$FAKE_HOME" bash "$SK/scripts/drivers/types/codex/codex-shim-install.sh" install >/dev/null local shim="$FAKE_HOME/.agents/bin/codex" @@ -731,13 +842,9 @@ PY HOME="$FAKE_HOME" bash "$REPO_ROOT/install.sh" --cmd agmsg-dfr --agent-type codex >/dev/null grep -q "/skills/agmsg/" "$shim" # still the first install's, per the earlier tests - # Bare --update, no --cmd: this base's ambiguous-candidate handling means - # which of the two real installs it lands on isn't the point of this test - # (that's #599 / #659's concern) -- what matters here is that whichever one - # it is, it must not walk away with a shim it was never explicitly told to - # claim. run env HOME="$FAKE_HOME" bash "$REPO_ROOT/install.sh" --update - [ "$status" -eq 0 ] + [ "$status" -ne 0 ] + printf '%s\n' "$output" | grep -Fq "Several agmsg installs found" local after; after="$(grep AGMSG_CODEX_SHIM_SCRIPT_DIR "$shim")" [ "$before" = "$after" ] } diff --git a/tests/test_remote_status_liveness.bats b/tests/test_remote_status_liveness.bats index 59b44f956..0261b93cf 100644 --- a/tests/test_remote_status_liveness.bats +++ b/tests/test_remote_status_liveness.bats @@ -861,3 +861,76 @@ write_unownable_ps_fixture() { run bash -c "grep -v '^[[:space:]]*#' \"\$1\" | sed 's/_agmsg_pid_alive_local//g' | grep -c '_agmsg_pid_alive'" _ "$SCRIPTS/remote.sh" [ "$output" = "0" ] } + + +# systemd-supervised engine detection for #894. The process itself is real; +# only systemctl is replaced, so these tests exercise the same argv/liveness +# checks used on a host while remaining independent of the test runner's user bus. +write_systemd_show_fixture() { + local state="$1" pid="$2" sub=dead fake="$TEST_SKILL_DIR/fake-systemctl" + [ "$state" = active ] && sub=running + printf '%s\n' '#!/usr/bin/env bash' \ + 'if [ "${1:-}" = "--user" ] && [ "${2:-}" = "show" ]; then' \ + " printf '%s\n' 'LoadState=loaded' 'ActiveState=$state' 'SubState=$sub' 'MainPID=$pid'" \ + ' exit 0' \ + 'fi' \ + 'exit 1' > "$fake" + chmod +x "$fake" + printf '%s\n' "$fake" +} + +@test "status: recognizes a verified systemd engine without a pidfile (#894)" { + start_matching_engine + rm -f "$TEST_SKILL_DIR/run/remote-sync.testteam.pid" + local fake_bin fake_systemctl + fake_bin="$(write_matching_ps_fixture)" + fake_systemctl="$(write_systemd_show_fixture active "$ENGINE_PID")" + + run env PATH="$fake_bin:$PATH" AGMSG_SYSTEMCTL="$fake_systemctl" \ + bash "$SCRIPTS/remote.sh" status testteam + [ "$status" -eq 0 ] + printf '%s\n' "$output" | grep -q -F -- "connected (engine running, pid $ENGINE_PID)" + refute grep -qi 'stale\|stopped' <<<"$output" +} + +@test "sync start: does not duplicate an active systemd engine (#894)" { + start_matching_engine + rm -f "$TEST_SKILL_DIR/run/remote-sync.testteam.pid" + local fake_bin fake_systemctl + fake_bin="$(write_matching_ps_fixture)" + fake_systemctl="$(write_systemd_show_fixture active "$ENGINE_PID")" + + run env PATH="$fake_bin:$PATH" AGMSG_SYSTEMCTL="$fake_systemctl" \ + bash "$SCRIPTS/remote.sh" sync start testteam + [ "$status" -eq 0 ] + printf '%s\n' "$output" | grep -q -F -- "already running (pid $ENGINE_PID)" + [ "$(find "$TEST_SKILL_DIR/run" -maxdepth 1 -name 'remote-sync.testteam.pid' | wc -l)" -eq 0 ] +} + +@test "sync start: refuses an active systemd unit with unverified identity (#894)" { + sleep 30 & + local foreign_pid=$! + ENGINE_PIDS="$ENGINE_PIDS $foreign_pid" + local fake_bin fake_systemctl + fake_bin="$(write_matching_ps_fixture)" + fake_systemctl="$(write_systemd_show_fixture active "$foreign_pid")" + + run env PATH="$fake_bin:$PATH" AGMSG_SYSTEMCTL="$fake_systemctl" \ + bash "$SCRIPTS/remote.sh" sync start testteam + [ "$status" -eq 1 ] + printf '%s\n' "$output" | grep -q -F -- "systemd owns team 'testteam'" + refute test -f "$TEST_SKILL_DIR/run/remote-sync.testteam.pid" +} + + +@test "status: reports an inactive systemd unit with restart guidance (#894)" { + start_matching_engine + rm -f "$TEST_SKILL_DIR/run/remote-sync.testteam.pid" + local fake_systemctl + fake_systemctl="$(write_systemd_show_fixture inactive 0)" + + run env AGMSG_SYSTEMCTL="$fake_systemctl" bash "$SCRIPTS/remote.sh" status testteam + [ "$status" -eq 0 ] + printf '%s\n' "$output" | grep -q -F -- "engine inactive under systemd" + printf '%s\n' "$output" | grep -q -F -- "systemctl --user restart agmsg-remote-sync-testteam.service" +} diff --git a/tests/test_resume_seat_guard.bats b/tests/test_resume_seat_guard.bats new file mode 100644 index 000000000..10465b948 --- /dev/null +++ b/tests/test_resume_seat_guard.bats @@ -0,0 +1,194 @@ +#!/usr/bin/env bats + +# #982 — a resumed session that cannot be matched to a seat must NOT get the +# generic, unfiltered watcher on a multi-seat project. +# +# The generic watch.sh (no 4th arg) subscribes to EVERY registered +# (team, agent) pair for the project and, as it delivers, stamps read_at and +# advances each pair's read cursor to the tip. On a project with more than one +# seat that is not "receive a little extra" — it CONSUMES other seats' unread +# mail, and the taken seats never get those messages delivered again. So the +# safe fallback is fail-CLOSED: identify the seat (from an actas lock this sid +# owns) or stand down; never emit the unfiltered watcher where it can eat someone +# else's inbox. +# +# The one "broken but green" these tests are written against: a test that only +# greps the emitted directive text ("standing down", "acting as bob") stays green +# even if watch.sh's delivery is wrong or the wrong watcher is emitted elsewhere +# in the output. So the two load-bearing tests below RUN the directive that was +# actually emitted and observe which pairs' read cursors move — the behaviour, not +# the wording. + +load test_helper + +setup() { + setup_test_env + # Bare-sid keying (#93) so the sid these tests pass is the id the scripts key + # on, deterministic whether the suite runs under an agent process or in CI. + export AGMSG_AGENT_PID="" + export SKILL_DIR="$TEST_SKILL_DIR" + export RUN_DIR="$SKILL_DIR/run" + mkdir -p "$RUN_DIR" + export PROJ="/tmp/agmsg-seat-guard-proj" + # Two registered seats — the shape in which the defect exists. A single-seat + # project is exercised by test_delivery.bats's generic-directive test, where the + # unfiltered watcher cannot consume anyone else's mail and stays the safe default. + bash "$SCRIPTS/join.sh" team alice claude-code "$PROJ" >/dev/null + bash "$SCRIPTS/join.sh" team bob claude-code "$PROJ" >/dev/null +} + +teardown() { teardown_test_env; } + +# Read one pair's store-owned local read frontier (copied from test_watch.bats). +_read_cursor() { + ( # shellcheck disable=SC1090 + source "$SCRIPTS/lib/storage.sh" + agmsg_storage_load + storage_read_cursor_get "$1" "$2" ) +} + +# The `command:` line the directive tells the host to launch, or empty if the +# script emitted no watcher (the stand-down path). +_directive_command() { printf '%s\n' "$1" | sed -n 's/^[[:space:]]*command: //p'; } + +# Mark alive as a bare session id (a live cc-instance. naming it), the +# way actas liveness (agmsg_instance_alive) resolves a bare owner token. +_mark_sid_alive() { echo "$1" > "$RUN_DIR/cc-instance.$$"; } + +# Give ownership of the actas lock for (team, ), as a claim would. +_seed_actas_lock() { + local team="$1" agent="$2" sid="$3" + echo "$sid" > "$RUN_DIR/actas.${team}__${agent}.session" +} + +# Write a role-session record into the isolated skill dir's run/ (as actas-claim +# would), so the record path — not narrowing — seats the session. +_seed_role_record() { + local team="$1" agent="$2" sid="$3" proj="$4" type="${5:-claude-code}" + SKILL_DIR="$TEST_SKILL_DIR" bash -c ' + source "$1/lib/role-session.sh" + agmsg_role_session_record "$2" "$3" "$4" "$5" "$6" + ' _ "$SCRIPTS" "$team" "$agent" "$sid" "$proj" "$type" +} + +_run_session_start() { + env AGMSG_RESOLVE_PROJECT=0 bash "$SCRIPTS/session-start.sh" claude-code "$PROJ" <<< "{\"session_id\":\"$1\"}" +} + +# --- fail-closed stand-down (no seat, several pairs) --- + +@test "resume, unidentified seat, multi-pair: stands down and emits NO watcher" { + run _run_session_start "sid-nobody" + [ "$status" -eq 0 ] + # grep, not `[[ == ]]`: a non-last `[[ ]]` cannot fail the test on bash 3.2 + # (#670), and these must actually be able to fail. + grep -qF "standing down" <<<"$output" + # It must not be silent about why (a missing watcher reads as "no messages"). + grep -qF "/agmsg actas alice" <<<"$output" + grep -qF "/agmsg actas bob" <<<"$output" + grep -qF "history.sh" <<<"$output" + # The load-bearing assertion: no runnable watch command was emitted, so the + # host has nothing to launch and no seat's mail can be consumed. A regression to + # the old fail-open path re-appears here as a non-empty command, not as a + # reworded paragraph. + local cmd; cmd="$(_directive_command "$output")" + [ -z "$cmd" ] +} + +@test "resume, unidentified seat, multi-pair: whatever is emitted consumes no one's mail" { + # Behavioural form of the above: seed unread for both seats, run whatever the + # directive emitted (nothing, when fixed), and assert neither read cursor moved. + # If the script regresses to a generic watcher, this runs it and the cursors + # advance — red. + bash "$SCRIPTS/send.sh" team bob alice "to-alice" >/dev/null + bash "$SCRIPTS/send.sh" team alice bob "to-bob" >/dev/null + local a0 b0; a0="$(_read_cursor team alice)"; b0="$(_read_cursor team bob)" + + run _run_session_start "sid-nobody" + local cmd; cmd="$(_directive_command "$output")" + if [ -n "$cmd" ]; then + eval "set -- $cmd" + AGMSG_WATCH_INTERVAL=1 bash "$@" >/dev/null 2>&1 3>&- 4>&- & + local wpid=$!; sleep 3; kill "$wpid" 2>/dev/null || true; wait "$wpid" 2>/dev/null || true + fi + local a1 b1; a1="$(_read_cursor team alice)"; b1="$(_read_cursor team bob)" + [ "${a1:-0}" = "${a0:-0}" ] + [ "${b1:-0}" = "${b0:-0}" ] +} + +# --- narrowing (no record, but an actas lock this sid owns) --- + +@test "resume, no record, actas lock owned by this sid: re-seats to that pair" { + _mark_sid_alive "sid-bob" + _seed_actas_lock team bob "sid-bob" + + run _run_session_start "sid-bob" + [ "$status" -eq 0 ] + grep -qF "resumed role" <<<"$output" + grep -qF "acting as bob" <<<"$output" + # #993: the narrowing path must state its basis honestly — the actas lock it + # holds, not a record it does not have. The reader launches a watcher on the + # strength of this sentence, so the inferred seat must not read as a recorded one. + grep -qF "actas exclusivity lock" <<<"$output" + refute grep -qF "was recorded as that role's seat" <<<"$output" + local cmd; cmd="$(_directive_command "$output")" + eval "set -- $cmd" + [ "$#" -eq 5 ] + [ "$5" = "bob" ] +} + +@test "resume, role-session record present: says recorded, not the actas-lock basis" { + # The other half of the #993 distinction: a real record must read as recorded, + # so the two bases stay distinguishable to the reader. + _seed_role_record team alice "sid-alice" "$PROJ" claude-code + run _run_session_start "sid-alice" + [ "$status" -eq 0 ] + grep -qF "acting as alice" <<<"$output" + grep -qF "was recorded as that role's seat" <<<"$output" + refute grep -qF "no role record was found" <<<"$output" +} + +@test "resume, narrowed to bob: the emitted watcher consumes bob's mail only, not alice's" { + # The strongest guard (tl's ask): run the directive AS EMITTED and check which + # pairs it actually consumes. A watcher that ignored its 4th arg — or a + # regression that emitted the generic one — would advance alice's cursor too. + _mark_sid_alive "sid-bob" + _seed_actas_lock team bob "sid-bob" + bash "$SCRIPTS/send.sh" team alice bob "to-bob" >/dev/null + bash "$SCRIPTS/send.sh" team bob alice "to-alice" >/dev/null + local a0; a0="$(_read_cursor team alice)" + + run _run_session_start "sid-bob" + local cmd; cmd="$(_directive_command "$output")" + eval "set -- $cmd" + + AGMSG_WATCH_INTERVAL=1 bash "$@" >/dev/null 2>&1 3>&- 4>&- & + local wpid=$! + local i b1 + for i in $(seq 1 100); do + b1="$(_read_cursor team bob)" + [ "${b1:-0}" -gt 0 ] && break + sleep 0.1 + done + kill "$wpid" 2>/dev/null || true; wait "$wpid" 2>/dev/null || true + + # bob's own mail was delivered (cursor advanced past it) ... + [ "${b1:-0}" -gt 0 ] + # ... and alice's was left untouched for alice's own watcher. + local a1; a1="$(_read_cursor team alice)" + [ "${a1:-0}" = "${a0:-0}" ] +} + +# --- an ambiguous multi-claim is treated as unidentified, not guessed --- + +@test "resume, sid owns two actas locks: refuses to guess, stands down" { + _mark_sid_alive "sid-both" + _seed_actas_lock team alice "sid-both" + _seed_actas_lock team bob "sid-both" + + run _run_session_start "sid-both" + [ "$status" -eq 0 ] + grep -qF "standing down" <<<"$output" + local cmd; cmd="$(_directive_command "$output")" + [ -z "$cmd" ] +} diff --git a/tests/test_watch.bats b/tests/test_watch.bats index 262430ea7..76d6bad74 100644 --- a/tests/test_watch.bats +++ b/tests/test_watch.bats @@ -432,13 +432,77 @@ _wait_for_file_contains() { # --- #93: parallel --continue/--resume sessions sharing a session_id --- -# Poll up to ~3s for to record . +# Poll up to ~10s for to record . A watcher relaunch does +# a real fork + lock-acquire + SIGTERM-the-predecessor + self-write before the +# pidfile reflects it, and a loaded CI runner can push that past the 3s this +# used to allow -- the flake #595 caught on a macos-latest shard. On timeout, +# reports what it was waiting for and what it last saw, per #595's ask for a +# failure message that distinguishes "never arrived" from "arrived as +# something else" rather than a bare assertion failure. +# +# `last saw` alone is the LAST poll and nothing else, so it cannot separate +# "the file never appeared" from "it appeared, then went away again" -- and +# those two have different causes. The distinct values are kept instead, with +# the poll each was first seen at. +# +# Four states, not two. `cat` returns the empty string for a path that does +# not exist, a file that exists and is empty, and a file that exists and +# cannot be read; collapsing them into one `` loses the difference +# this trail exists to show (raised in review). They are named apart. +# +# Existence is decided by a test; readability is decided by THE READ. `-r` +# only predicts what a read would do, and a read can still fail after it +# passes -- a permission change, a replacement, a path that is not a regular +# file, an I/O error. Classifying on `-r` and then swallowing the read's +# failure with `|| true` reports ``, merging the two states this +# exists to separate (raised in review; the chmod control drove the `-r` +# branch and never reached the failing read). +_observe_pidfile() { + local pf="$1" v + if [ ! -e "$pf" ]; then printf ''; return 0; fi + if v="$(cat "$pf" 2>/dev/null)"; then + if [ -z "$v" ]; then printf ''; else printf '%s' "$v"; fi + else + printf '' + fi +} + _wait_pidfile() { - local pf="$1" want="$2" i - for i in $(seq 1 30); do - [ -f "$pf" ] && [ "$(cat "$pf" 2>/dev/null)" = "$want" ] && return 0 + # `last` starts at a value no read can produce -- seeded with "" it would + # swallow the first observation in the case that matters most, a file that + # is missing from the very first poll. + local pf="$1" want="$2" i seen last="__no_poll_yet__" trail="" + for i in $(seq 1 100); do + seen="$(_observe_pidfile "$pf")" + [ "$seen" = "$want" ] && return 0 + if [ "$seen" != "$last" ]; then + trail="$trail poll$i='$seen'" + last="$seen" + fi sleep 0.1 done + echo "_wait_pidfile: timed out waiting for '$pf' to record pid $want (last saw: '$seen')" >&2 + echo "_wait_pidfile: distinct observations, first poll each:$trail" >&2 + # What this can say about $want, and no more: signal 0 reaching a pid does + # not establish that the pid is still the process we started -- pids are + # reused (raised in review). So the command line is printed rather than a + # liveness verdict, and the reader decides. + if kill -0 "$want" 2>/dev/null; then + echo "_wait_pidfile: signal 0 reaches pid $want; its command line now is:" >&2 + ps -o pid=,stat=,etime=,command= -p "$want" >&2 2>/dev/null || echo " (ps could not describe it)" >&2 + else + echo "_wait_pidfile: signal 0 does not reach pid $want (exited, or never ours)" >&2 + fi + # The watcher writes its own log beside the pidfile and says there what it + # was doing. A successor that is running and has not yet claimed the slot is + # waiting on something, and this is the only place that says what. + echo "_wait_pidfile: run dir and watcher logs:" >&2 + ls -la "$(dirname "$pf")" >&2 2>/dev/null || true + for _l in "$(dirname "$pf")"/watch.*.log; do + [ -f "$_l" ] || continue + echo "--- $_l" >&2 + tail -20 "$_l" >&2 2>/dev/null || true + done return 1 }