From e18d8b1e7da4120181643e84eb97013eea243563 Mon Sep 17 00:00:00 2001 From: fujibee Date: Tue, 25 Aug 2026 23:14:41 -0700 Subject: [PATCH 1/9] test: count a role's bridges by pair set, and return the value that was waited for (#984) (#999) * test: count a role's bridges by pair set, and return the value that was waited for (#984) The five #937 reap tests each waited on one observation and asserted on a different, later one. The waiter polled until the count matched, then counted again and returned that second reading -- and between the two the reaper kills the orphan and the launcher respawns, so the count passes through 2 and through 0. The number the caller got was one that nothing had ever waited for. Verification measured the same shape in isolation: with a 60ms observation the old form returns 2 and fails, the single-observation form returns 1 and passes. One site had a second defect on top of that. The pair-superset test spawns its own {alice,bob} bridge, and the counter matched the role name anywhere in a process's argv, so it answered 1 for that fixture: the `-ge 1` gate opened on the test's own fixture, before the launcher had spawned anything. Both are replaced together, at all five sites: _count_exact_role_bridges requires a pair set of exactly {name} -- the same set inequality the superset test's `kill -0` already relies on _wait_exact_role_count returns the value that satisfied the wait The argv-match pair had no other users in the tree, so it is removed rather than left standing beside the new one. A leftover second form is how four of the five sites came to be missed in the first place; with it gone, a missed site is a grep. The expected value stays 1, and is now reached for the stated reason: the reaper kills the pidfile-less orphan and the launcher respawns one bridge for the role, so exactly one bridge with pair set {alice} exists once it settles. In the superset test the {alice,bob} fixture is not one of them, which is what the `kill -0` on the next line asserts separately. The new test pins the counter itself. Its positive control comes first, because three zeroes are also what a counter that answers 0 to everything produces -- which is what the first version of this predicate did: `ps` renders a pair's tab separator as the four characters \011, three of them digits, so a single-character [^0-9A-Za-z] separator class matched nothing at all. The test also asks the waiter for a count it will never reach, which is the only assertion in the file that goes red if the waiter is ever rewritten to return its `want` instead of what it saw; every other check here passes under that rewrite. Whether this is what fires in CI is NOT measured. #984 records two failing tests on main and both of them are covered, but the failure does not reproduce on this machine, so a green shard after this lands is a green shard -- not a cause. * test: make the reap gate load-bearing, and prove an exhausted one fails (#984) The gate the five #937 reap tests open before making an orphan was a `for i in {1..80}; do ... && break; done` loop. Running out of tries and reaching the count leave the same state behind, so from the next line the two are indistinguishable. A test whose launcher had not spawned yet went on to `rm -f` pidfiles that did not exist, waited, and was then satisfied by a bridge the launcher started during that wait -- green, having created no orphan and reaped none. The test passes without exercising #937 at all, and nothing says so. All five gates now go through `_require_launcher_bridge`, which fails and names the count it actually reached. The point-in-time count in the first test is dropped: the gate now makes the same statement, earlier. The control test is new, and was fired before this was handed over. With the gate returning 0 on exhaustion -- the behaviour the `for ... break` form had -- it goes red at `[ "$status" -ne 0 ]`; with the gate as written it is green. It costs the waiter's full sweep on purpose: an exhausted gate is the thing being measured. The one other `{1..80}` loop in this file (the #466 mid-loop cache test) was checked and is already load-bearing -- the line after it repeats the `grep -q` bare, so exhausting the loop fails there too. Whole file after the change: 29 passed, 0 failed (1 skipped). --- tests/test_codex_bridge_launcher.bats | 157 ++++++++++++++++++++++---- 1 file changed, 134 insertions(+), 23 deletions(-) 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 From 867f02ac1357dc65a70ebdb6bc6f3ccc9b3be2dd Mon Sep 17 00:00:00 2001 From: fujibee Date: Tue, 25 Aug 2026 23:16:16 -0700 Subject: [PATCH 2/9] fix(sync): stop the apply failure path from discarding the shell's stderr (#974) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `_sqlite_sync_apply_fail` closed fd 3 as `exec 3<&- 2>/dev/null`. With no command word, bash does not scope that redirection to anything — it applies it to the shell, permanently. Every `>&2` after the first apply failure went to /dev/null. It sits on the failure path, so what it silences is the output a failing apply is about to produce. The first thing lost is the message #911 added this morning, naming which check returned 13 — the diagnostic that made today's work possible, disabled a few hundred lines away in the same file. `{ exec 3<&-; } 2>/dev/null` scopes it to the block. Two tests, because either alone passes while the other's failure ships: one lifts the driver's own function body out of the file by line range and calls it (a copy of the shape would keep passing while the driver regressed), one greps the file for a bare `exec` carrying a redirection. Both go red on the old form. Reported by @JoelMitz, who also identified #911 as the first casualty. --- scripts/drivers/storage/sqlite-sync.sh | 2 +- tests/test_apply_fail_stderr.bats | 46 ++++++++++++++++++++++++++ 2 files changed, 47 insertions(+), 1 deletion(-) create mode 100644 tests/test_apply_fail_stderr.bats diff --git a/scripts/drivers/storage/sqlite-sync.sh b/scripts/drivers/storage/sqlite-sync.sh index 68b2bfb55..d48bed8e5 100644 --- a/scripts/drivers/storage/sqlite-sync.sh +++ b/scripts/drivers/storage/sqlite-sync.sh @@ -1043,7 +1043,7 @@ storage_sync_apply_pull() { [ -s "$jq_err" ] && sed 's/^/agmsg: sqlite-sync: /' "$jq_err" >&2 printf 'agmsg: sqlite-sync: %s\n' "$1" >&2 fi - exec 3<&- 2>/dev/null || true + { exec 3<&-; } 2>/dev/null || true rm -f "$sql_file" "$jq_err" _AGMSG_SYNC_SQL_FILE=""; _AGMSG_SYNC_JQ_ERR="" trap - EXIT INT TERM HUP 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 ] +} From 5801f425d766e77e02fc1f6773b180a8d12ac408 Mon Sep 17 00:00:00 2001 From: fujibee Date: Tue, 25 Aug 2026 23:16:50 -0700 Subject: [PATCH 3/9] fix(install): write Codex writable_roots through a symlinked config.toml instead of replacing the link (#747) (#995) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(install): write Codex writable_roots through a symlinked config.toml instead of replacing the link (#747) configure_codex_sandbox() edited ~/.codex/config.toml with `awk ... > tmp && mv tmp config`. `mv` replaces a symlink with the plain temp file, so on a config.toml managed as a symlink (stow/chezmoi/manual dotfiles) the install detached the link and wrote agmsg's paths to a fresh file at the link path — the dotfiles target kept the old contents and Codex, still reading through the (now-gone) link's original location, lost the edit. The install reported success, so nothing flagged it. Deterministic whenever writable_roots or [sandbox_workspace_write] already exists and agmsg's paths do not; the no-section branch already appended with `>>`, which follows the link, so it was unaffected. Both mv-based branches now write through write_through_symlink(), which cats the temp file over the destination — a redirect follows the link and updates its target, preserving the link. Not atomic, which is fine for a few-KB config written once at install time; the third branch is left as-is. Tests reproduce the reporter's shape (a symlinked config.toml into a dotfiles dir) for both mv branches and assert the link survives and the edit lands on the target; reverting the helper to `mv` fails both. * fix(install): keep the atomic mv for a regular config.toml; write through only a symlinked one (#747 review) The first pass routed BOTH branches through an unconditional cat-redirect, which also made the common non-symlink path non-atomic: an interrupted install, a write error, or a full disk could leave an empty or partial ~/.codex/config.toml where mv had left all-old-or-all-new (co1's blocking review). Match the fix's scope to the defect's: move_into_place() now branches on the destination. A symlinked dest is written through (the #747 case); a regular dest keeps the atomic mv. The symlink arm stays non-atomic — there is no atomic write-through-a-link with plain POSIX tools — but that exposure is now confined to symlink users, whose target is typically a version-controlled dotfile, rather than imposed on every install. (Chosen over resolving the link and mv-ing onto the real path: that would need a portable readlink -f the tree deliberately avoids, and same-dir temp placement to stay atomic across filesystems — more surface than this bug warrants.) Adds the reverse control co1 asked for: a regular config.toml must come out with a NEW inode (mv renames; a truncate-in-place cat would keep it), so the atomic mv cannot be dropped again unseen. Reverting either arm now fails its own test. --- install.sh | 24 ++++++++++++++-- tests/test_install.bats | 61 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 83 insertions(+), 2 deletions(-) diff --git a/install.sh b/install.sh index ae1031e8c..3c1c4a1ce 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" diff --git a/tests/test_install.bats b/tests/test_install.bats index 246b05a10..c4ecf2503 100644 --- a/tests/test_install.bats +++ b/tests/test_install.bats @@ -563,6 +563,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) --- From 0d08381b67ec86fa560b4dced3bc50dd12c1e69c Mon Sep 17 00:00:00 2001 From: fujibee Date: Wed, 26 Aug 2026 00:53:30 -0700 Subject: [PATCH 4/9] fix(install): refuse to guess between multiple installs on --update with no --cmd (#659) * fix(install): refuse to guess between multiple installs on --update with no --cmd (#599) install.sh --update with no --cmd stopped at the first .agmsg-marked directory a glob yielded. A glob expands in collation order, not installation order, and nothing records which install came first, so on a machine with a sibling install (agmsg-dfc beside agmsg, where 0x2D sorts before 0x2F) the sibling was updated and the intended install was left untouched, while the command reported success. Update mode now enumerates every marked directory. Zero or one candidate behaves exactly as before; two or more fail closed: the candidates are listed and --cmd is asked for. No name-based exclusion of backup-shaped directories: --cmd has no reserved-name validation, so any pattern that would catch a real backup can match a legitimately chosen name; a leftover backup that still carries the marker is just one more candidate, and ambiguity is what this refuses to guess through. --cmd is unchanged. * test(install): the two-install bare --update case is a refusal now, and its assertions can fail #553's "bare --update does NOT force-steal a Codex shim owned by a different install" pinned, on its own words, the pre-#659 base: two installs present, bare --update proceeds, and the shim must survive. With #599 fixed a bare --update over two installs refuses before it touches anything, so the case now asserts the refusal, the message that names both installs, and the shim's owner line unchanged across it -- the property #553 protects, in its strongest form. The single-install case, where a bare --update does proceed, is unchanged and still covered by the neighbouring #553 test. The eight non-last [[ =~ ]] assertions this branch added could not fail on bash 3.2 (the enforceable-assertions check counted them); they are plain greps and refute now, and the baseline moves down to 636 as the checker asks. --- .github/enforced-assertions-baseline | 2 +- install.sh | 38 +++++++++++--- tests/test_install.bats | 78 ++++++++++++++++++++++------ 3 files changed, 94 insertions(+), 24 deletions(-) diff --git a/.github/enforced-assertions-baseline b/.github/enforced-assertions-baseline index 231a7d578..a8f78cee4 100644 --- a/.github/enforced-assertions-baseline +++ b/.github/enforced-assertions-baseline @@ -1 +1 @@ -638 +636 diff --git a/install.sh b/install.sh index 3c1c4a1ce..4400cb6d6 100755 --- a/install.sh +++ b/install.sh @@ -285,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 @@ -294,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/tests/test_install.bats b/tests/test_install.bats index c4ecf2503..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 @@ -775,14 +821,18 @@ EOF @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" @@ -792,13 +842,9 @@ EOF 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" ] } From de226195a2c664e2c63bdcc5cb11db9b96240417 Mon Sep 17 00:00:00 2001 From: fujibee Date: Thu, 27 Aug 2026 21:24:10 -0700 Subject: [PATCH 5/9] fix(session-start): stand down instead of an unfiltered watcher when a resumed seat is unidentified (#982) (#993) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(session-start): stand down instead of an unfiltered watcher when a resumed seat is unidentified (#982) On a resumed session with no role-session record, the fallback emitted the generic watch.sh directive (no 4th arg). On a project with several registered seats that watcher subscribes to every pair and, as it delivers, stamps read_at and advances each pair's read cursor to the tip -- so it consumes other seats' unread mail, which those seats then never receive. Fail-open is the wrong default here. Fail closed instead: - Narrowing: when no role-session record matches but an actas __.session lock is owned by this session's bare sid, re-seat to that pair and emit the role-filtered directive. The record the branch wanted and the lock that exists carry the same fact; an ambiguous 2+ match is left unidentified rather than guessed. - When the seat still cannot be established: emit a generic watcher only for a single-pair project (nobody else's mail to take); with more than one pair, emit no watcher and say why, naming `/agmsg actas `. A silent no-watcher is indistinguishable from "no messages arriving", so the stand-down is explicit and points at the recovery. This is a delivery miss, not a loss: messages remain in the store and history.sh returns them. Tests (tests/test_resume_seat_guard.bats) run the directive as emitted and observe which pairs' read cursors move -- a string check on the directive text would stay green if the wrong watcher were emitted or watch.sh ignored its 4th arg. A fail-open regression advances a bystander seat's cursor (red); a broken narrowing fails to deliver the seat's own mail (red). * fix(session-start): state the resumed seat's basis honestly — recorded vs inferred from the actas lock (#982) The role-filtered directive is prose the next session reads and acts on: it launches the watcher the text describes. The narrowing path (#982) reused the record path's wording — "this session was recorded as that role's seat" — but it has no record; it inferred the seat from an actas lock this sid still owns. Telling the reader a thing was recorded when it was inferred hands them a claim they cannot check, and this whole fix exists because a directive was trusted and executed as-is. Split the sentence on how the seat was established: - record path: "was recorded as that role's seat" (unchanged). - narrowing path: names the actas lock as the basis and why it stands in for the record, so the reader can weigh "what if that lock is stale?". Tests assert both directions of the distinction, so an inferred seat can never again read as a recorded one. --- scripts/session-start.sh | 105 ++++++++++++++-- tests/test_resume_seat_guard.bats | 194 ++++++++++++++++++++++++++++++ 2 files changed, 289 insertions(+), 10 deletions(-) create mode 100644 tests/test_resume_seat_guard.bats 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_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" ] +} From 8085cd673af8cac081c573b79e9cb228267e50cc Mon Sep 17 00:00:00 2001 From: fujibee Date: Thu, 27 Aug 2026 21:25:54 -0700 Subject: [PATCH 6/9] test(despawn): enforce the ctrl:despawn read-state assertion with refute, not a no-op ! (#715 step 1) (#997) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `! _is_unread_for_alice "ctrl:despawn"` reported ok even when the row was still unread: a bare `! cmd` is exempt from errexit on every bash (#670), so the assertion was written but watched nothing. Switching to `refute` makes it fail when the ctrl:despawn row lingers unread for alice. Verified the enforced form actually fails: with the condition forced true (the row always unread), `refute` turns the test red while the old `!` form stays green — the no-op made visible. The enforceable-assertions baseline drops 638 to 637 (this was its single remaining `!` exception). This does NOT fix the load-dependent flake #715 was filed for (the row sometimes not yet read right after despawn returns, under load). It makes that flake observable for the first time: enforced, the assertion can now go red, whereas before it never could. On this machine a reproduction harness (isolated store, the despawn scenario under eight concurrent writers) saw 0 flakes in 25 iterations, and bats-core 1.13 cannot run under the bash 3.2.57 the report measured on — so the red is not reproduced here, only made possible. If the flake is real, post-merge CI is where it will surface. #715 stays open for that fix. --- .github/enforced-assertions-baseline | 2 +- tests/test_despawn.bats | 20 +++++++------------- 2 files changed, 8 insertions(+), 14 deletions(-) diff --git a/.github/enforced-assertions-baseline b/.github/enforced-assertions-baseline index a8f78cee4..7d71bf4ad 100644 --- a/.github/enforced-assertions-baseline +++ b/.github/enforced-assertions-baseline @@ -1 +1 @@ -636 +635 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 } From ad5c13f88b6a3eb9674209e98e3b220be75c938f Mon Sep 17 00:00:00 2001 From: fujibee Date: Thu, 27 Aug 2026 21:26:31 -0700 Subject: [PATCH 7/9] fix(tests): widen _wait_pidfile's window and name what it saw (#595) (#797) * fix(tests): widen _wait_pidfile's window and name what it saw (#595) * test(watch): keep every distinct pidfile observation, not just the last one * test(watch): name the four states a pidfile read can be in, and print what the successor is doing * test(watch): let the read decide readability, not a test that predicts it --- tests/test_watch.bats | 72 ++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 68 insertions(+), 4 deletions(-) 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 } From 3dcd54b098b3dbaddc3a4bdb8a1842dc6bbf8e51 Mon Sep 17 00:00:00 2001 From: mkmariko Date: Fri, 28 Aug 2026 16:23:30 +0900 Subject: [PATCH 8/9] fix(#894): detect systemd-supervised sync engines --- scripts/remote.sh | 83 ++++++++++++++++++++++++-- tests/test_remote_status_liveness.bats | 73 ++++++++++++++++++++++ 2 files changed, 150 insertions(+), 6 deletions(-) diff --git a/scripts/remote.sh b/scripts/remote.sh index 4e6c2db02..fada0a584 100644 --- a/scripts/remote.sh +++ b/scripts/remote.sh @@ -2041,11 +2041,69 @@ _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="" + IFS=$'\t' read -r systemd_state systemd_pid < <(_remote_systemd_engine_status "$team") + 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' @@ -2489,6 +2547,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 +2882,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 )) 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" +} From a85c740314b1a3189ad64a63904febb2c47a7647 Mon Sep 17 00:00:00 2001 From: mkmariko Date: Fri, 28 Aug 2026 16:43:42 +0900 Subject: [PATCH 9/9] fix(#894): keep pidfile lifecycle checks supervisor-local --- scripts/remote.sh | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/scripts/remote.sh b/scripts/remote.sh index fada0a584..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 @@ -2095,7 +2095,11 @@ _remote_sync_engine_status() { local team="$1" pidfile pid command expected systemd_state systemd_pid REMOTE_SYNC_ENGINE_SUPERVISOR="" REMOTE_SYNC_ENGINE_SUPERVISOR_PID="" - IFS=$'\t' read -r systemd_state systemd_pid < <(_remote_systemd_engine_status "$team") + 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" @@ -2135,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 @@ -2943,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\"" ' @@ -3438,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 @@ -3469,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 --