diff --git a/scripts/actas-claim.sh b/scripts/actas-claim.sh index f66200a62..d4684a9c9 100755 --- a/scripts/actas-claim.sh +++ b/scripts/actas-claim.sh @@ -97,6 +97,40 @@ while IFS= read -r team; do agmsg_role_session_record "$team" "$NAME" "$BARE_SID" "$PROJECT_PHYS" "$TYPE" || true done <<< "$TEAMS" +# Start the engine for each claimed team, if one is not already up (#774). +# +# The second of the two trigger points. `actas` is where a session takes on a +# role and therefore a team, and a session that arrives this way never passes +# through session-start's block with that team in hand — a spawn's boot prompt +# is `actas`, so on a rebooted machine this is the first moment the team is +# known. +# +# AFTER the claim and BEFORE the status line: the claim is the thing the caller +# is waiting on, and nothing about starting an engine may delay or fail it. +# +# DELAY IS THE HALF THAT NEEDED WORK. Returning 0 is not enough — a synchronous +# `sync start` holds `status=ok` back for as long as the engine takes to become +# ready, which is up to ~16s per team before the command even gives up. The +# helper bounds the WAIT (`AGMSG_SYNC_AUTOSTART_TIMEOUT_S`, 5s for the whole +# call) and leaves a slow start running rather than killing it. `|| true` says +# the exit-status half a second time. +# +# Whether an engine is already running is not asked here — `sync start` answers +# it under the per-team lock, and the concurrent case (several sessions claiming +# roles at once) is exactly the one a second answer gets wrong. See +# scripts/lib/sync-autostart.sh. +if [ -x "$SKILL_DIR/scripts/remote.sh" ] && [ -r "$SKILL_DIR/scripts/lib/sync-autostart.sh" ]; then + # shellcheck source=scripts/lib/sync-autostart.sh + . "$SKILL_DIR/scripts/lib/sync-autostart.sh" + _autostart_teams=() + while IFS= read -r _t; do + [ -n "$_t" ] && _autostart_teams+=("$_t") + done <<< "$TEAMS" + if [ ${#_autostart_teams[@]} -gt 0 ]; then + agmsg_sync_autostart "$SKILL_DIR/scripts/remote.sh" "${_autostart_teams[@]}" || true + fi +fi + # Print a line describing each claimed team. One team per most projects but # the underlying model allows multi-team same-name registrations. printf 'status=ok' diff --git a/scripts/lib/sync-autostart.sh b/scripts/lib/sync-autostart.sh new file mode 100644 index 000000000..96c761da3 --- /dev/null +++ b/scripts/lib/sync-autostart.sh @@ -0,0 +1,211 @@ +# Start a connected team's sync engine when an agent turns up (#774). +# +# A machine restart leaves every sync engine dead and nothing restarts one. The +# agent keeps working, `send` keeps committing locally, and nothing reaches the +# other machines until a person happens to type `remote sync start`. #765 made +# that visible; a warning still asks a person to do what the machine can do. +# +# Sourced by the two places an agent establishes what it is: +# scripts/session-start.sh — where the monitor is started +# scripts/actas-claim.sh — where a session takes on a role, and a team +# +# ONE ENGINE PER (MACHINE, TEAM) IS NOT ENFORCED HERE, AND MUST NOT BE. +# +# `cmd_sync_start` already takes `agmsg_lock_acquire "$TEAMS_DIR/"`, and +# under that lock it answers `Sync engine already running (pid N).` and returns +# 0. The pidfile is per team. So the invariant holds by construction in the +# command, and this calls the command. +# +# The alternative — checking the pidfile here and starting only when it looks +# dead — puts a SECOND answer to "is it running?" in the tree, outside the lock +# that makes the first one true. Two answers to that question diverge exactly +# when several sessions open at once, which is the case this exists for: they +# race for the lock, one starts the engine, the rest are told `already running` +# and carry on. That behaviour is the command's, and it is inherited rather than +# reproduced. +# +# THE BINDING CHECK IS INHERITED TOO. `cmd_sync_start` refuses a team with no +# active binding and a disconnected team, by name, before it starts anything. +# Filtering on the binding here would be the same duplication one level up. +# +# NOTHING HERE MAY FAIL A SESSION, AND FAILING INCLUDES BEING SLOW. +# +# Returning 0 on every path is only half of it — the first version did that and +# still ran `sync start` synchronously, which means `actas` did not print +# `status=ok` and session start did not emit the Monitor directive until the +# engine was ready. `cmd_sync_start` waits for a readiness nonce (~16s of its +# own before it gives up), takes a per-team lock others may be holding, and can +# be stuck for as long as its child is. Multiplied by the number of connected +# teams, in the critical path of an agent opening. A release-blocker fix that +# can stop a session from starting is not a fix (raised in review). +# +# So each start runs in the BACKGROUND and this waits, at most, for a whole-call +# budget shared by every team. When the budget runs out the child is LEFT +# RUNNING rather than killed: it may be seconds from having started the engine, +# and killing it could leave a half-made pidfile behind. What stops is the +# WAITING. The session goes on and the line says a start is still in flight. + +# Usage: agmsg_sync_autostart ... +# +# Prints, at most, one block: the teams whose engines this call started, and the +# teams it could not start. A team whose engine was already running produces no +# output at all — starting is a side effect the person did not ask for in this +# moment, and "nothing changed" is not news. +agmsg_sync_autostart() { + local remote_sh="$1"; shift + # Sourced here rather than at file scope: this file is sourced by two hooks, + # and pulling in a second file at their top level is a cost they pay whether + # or not anything is started. + if ! declare -F agmsg_close_inherited_fds >/dev/null 2>&1; then + local _lib_dir="${BASH_SOURCE[0]%/*}" + # shellcheck source=scripts/lib/close-fds.sh + [ -r "$_lib_dir/close-fds.sh" ] && . "$_lib_dir/close-fds.sh" + fi + [ -x "$remote_sh" ] || return 0 + [ $# -gt 0 ] || return 0 + + # Seconds, for the whole call. Overridable so a test can drive the deadline + # without waiting for it, and so an operator on a slow machine can raise it. + local budget="${AGMSG_SYNC_AUTOSTART_TIMEOUT_S:-5}" + local elapsed_start=$SECONDS + + local team out rc tmp started="" failed="" slow="" + for team in "$@"; do + [ -n "$team" ] || continue + # WHY THERE IS NO REFUSAL CHECK HERE, having had one (#773). + # + # The version of this that read `remote.sh status` before each start + # existed to stop a restart loop: the engine used to EXIT when the server + # refused, so auto-start would raise it again on the next session and it + # would exit again. + # + # #792 removed that. The engine now records the refusal, backs off to its + # longest interval and keeps the loop — `sleepCall(MAX_BACKOFF_MS); + # continue;` — so starting a refused team costs one quiet process that + # reports the reason through `status`, and there is no loop to prevent. + # + # The check was not free. It put a second command in the path where a + # session prints its Monitor directive, and bounding it correctly took a + # background wrapper, a watchdog, a shared deadline, a grace period and a + # reaping rule — seven review rounds of machinery to make a lookup safe + # that the thing it protected against no longer needs. Reading the engine + # is what settled it, not the review count. + tmp="$(mktemp 2>/dev/null)" || tmp="" + [ -n "$tmp" ] || return 0 + # stderr folded in: `cmd_sync_start` says why it refused on stderr, and that + # sentence is the useful half of a failure. Swallowing it would leave this + # printing "could not start" with the reason on the floor. + # THE QUESTION IS "HAS IT FINISHED?", NOT "IS IT ALIVE?". + # + # The child writes its exit status to a sentinel as its last act, and this + # polls for the sentinel. No pid is examined, so no liveness check is made + # — which is what `scripts/lib/instance-id.sh`'s `_agmsg_pid_alive` exists + # to own, and what a bare `kill -0` here would have duplicated badly (a + # repo-wide check catches that; mine reached CI before I did). + # + # It is also the more exact question. `kill -0` succeeds for a child that + # has exited and not been reaped, so polling liveness would have waited + # past the moment the answer was available. + # DETACHED FROM THIS CALLER'S STREAMS, and that is not tidiness. + # + # The child is deliberately allowed to outlive this function. If it still + # holds the caller's stdout, anything that CAPTURES that output — `run` in + # a test, `$(...)`, a hook whose output is piped — waits for EOF, and a + # start that hangs then hangs the session. That is the requirement this + # whole budget exists for, broken in a way no exit code and no timeout + # here could see: I measured it as a suite that stopped finishing. + # + # stdin too: a child left on a terminal can stop for input. + ( + # EVERY INHERITED DESCRIPTOR, not just 0/1/2. + # + # Detaching stdin/stdout/stderr was necessary and not sufficient: bats + # hands a harness pipe down on fd 3 and 4, and a child that keeps them + # open holds the shard after every case has passed. `scripts/lib/close-fds.sh` + # exists because that exact leak hung a shard once already, from a + # different spawn path — and a repo-wide check found mine. + # + # Called INSIDE the subshell so it closes the child's copies and leaves + # this shell's own descriptors alone, which is the pattern that file's + # own comment prescribes. + agmsg_close_inherited_fds + "$remote_sh" sync start "$team" >"$tmp" 2>&1 + printf '%s\n' "$?" > "$tmp.rc" + # The literal `3>&- 4>&-` as well as the call inside, because the repo-wide + # check reads the spawn LINE (tests/test_spawn_fd_guard.bats). Belt and + # braces is the right answer here anyway: the call closes whatever the + # runtime handed down, and the redirections say so where a reader — and + # that check — can see it without following a function. + ) /dev/null 2>&1 3>&- 4>&- & + while [ ! -f "$tmp.rc" ] && [ $((SECONDS - elapsed_start)) -lt "$budget" ]; do + sleep 0.1 + done + if [ ! -f "$tmp.rc" ]; then + # Budget spent. The child is NOT killed — see the header — so its two + # temp files are left for it to finish writing into. They are in the + # system temp directory, and that is the price of not truncating a start + # that may be about to succeed. + slow="$slow$team"$'\n' + continue + fi + rc="$(cat "$tmp.rc" 2>/dev/null || printf '1')" + out="$(cat "$tmp" 2>/dev/null)" + rm -f "$tmp" "$tmp.rc" + if [ "$rc" -eq 0 ] && printf '%s' "$out" | grep -q 'already running'; then + continue + fi + if [ "$rc" -eq 0 ]; then + started="$started$team"$'\n' + continue + fi + # The team name AND what the command said. A bare "could not start " + # sends the reader to a log to find a sentence this already had. + failed="$failed$team $(printf '%s' "$out" | tr '\n' ' ')"$'\n' + done + + if [ -n "$started" ]; then + # Written as a loop rather than a joined string: a team name may contain + # characters that make a one-line join ambiguous, and one line per team is + # what the #765 block already established as this hook's voice. + printf '%s\n' 'AGMSG: no sync engine was running; started one for:' + printf '%s' "$started" | while IFS= read -r t; do + [ -n "$t" ] || continue + printf ' %s\n' "$t" + done + printf '\n' + fi + + if [ -n "$slow" ]; then + printf '%s\n' "AGMSG: a sync engine start is still in flight after ${budget}s; not waiting for it:" + printf '%s' "$slow" | while IFS= read -r t; do + [ -n "$t" ] || continue + printf ' %s\n' "$t" + done + printf '%s\n' 'The session continues. Check it with:' '' + printf '%s' "$slow" | while IFS= read -r t; do + [ -n "$t" ] || continue + printf ' bash %q status %q\n' "$remote_sh" "$t" + done + printf '\n' + fi + + if [ -n "$failed" ]; then + printf '%s\n' 'AGMSG: connected, but not syncing.' '' + printf '%s\n' \ + 'No sync engine is running for the team(s) below and starting one failed,' \ + 'so messages from other machines are not arriving. The session continues.' '' + printf '%s' "$failed" | while IFS=$'\t' read -r t reason; do + [ -n "$t" ] || continue + printf ' %s: %s\n' "$t" "$reason" + # The runnable line, UNCHANGED FROM #765 — including its two-space + # indent. That prefix is part of the contract: `test_delivery.bats` + # extracts the command with `sed -n 's/^ bash //p'` and runs it, so a + # deeper indent leaves the operator's remedy unrunnable by the check that + # proves it is runnable (measured: it failed on exactly that). + printf ' bash %q sync start %q\n' "$remote_sh" "$t" + done + printf '\n' + fi + + return 0 +} diff --git a/scripts/session-start.sh b/scripts/session-start.sh index f66f345d3..e9130d116 100755 --- a/scripts/session-start.sh +++ b/scripts/session-start.sh @@ -245,50 +245,51 @@ if [ -n "$CC_PID" ]; then printf '%s\n' "$INSTANCE_ID" > "$STATE" fi -# --- Say when a connected team has no engine (#761). --- +# --- Start the engine for a connected team that has none (#761, #774). --- # A reboot leaves every sync engine dead and nothing restarts one: the five # commands that start it are all operator actions. `connected` keeps printing, # `send` keeps succeeding locally, and the only symptom is messages not arriving # — which reads as "nobody wrote anything". This is the first moment after a -# reboot when anything of ours runs, so it is where the absence gets said. +# reboot when anything of ours runs, so it is where it gets started. +# +# #765 made the absence VISIBLE here, in this block, and that was the right +# first step and not enough: the warning appeared only when someone opened a +# session, and it asked the person for something the machine can do. Starting it +# is the same trigger doing the whole job. # # BEFORE the three directive blocks below rather than inside them: there are # three ways out of this script (a watcher already streaming, the actas variant, # and the default), and a line added to one of them is missing from the other # two. # -# Best-effort, and bounded. `status` is a subprocess and needs python3; if it -# cannot run, this says nothing rather than guessing. That is a real gap and not -# a silent one: the check reports what it saw, and what it could not see is the -# absence of a line, which is why the line names the teams rather than a count. +# WHICH TEAMS. `status` is asked for CONNECTED teams — the binding, not the +# engine. Whether an engine is running is `sync start`'s question, under the +# lock that makes the answer true; asking it here as well is the second answer +# that diverges (see scripts/lib/sync-autostart.sh). +# +# Best-effort, and bounded IN TIME as well as in outcome. `status` is a +# subprocess and needs python3; if it cannot run, this does nothing rather than +# guessing. A start that fails never fails the session, and a start that is SLOW +# never delays the Monitor directive below: the helper waits for a whole-call +# budget (`AGMSG_SYNC_AUTOSTART_TIMEOUT_S`, 5s) and then leaves the start +# running and moves on. An agent that will not open because a sync engine was +# thinking is worse than a sync engine that is down. # # Reaches `monitor` and `both` only — this hook is not installed for `turn` or # `off`, so those modes still get the absence only from `status`. -if [ -x "$SKILL_DIR/scripts/remote.sh" ]; then - _stale_teams="$("$SKILL_DIR/scripts/remote.sh" status 2>/dev/null \ - | awk -F'\t' '/engine (stopped|stale)/ {print $1}' || true)" - if [ -n "$_stale_teams" ]; then - printf '%s\n' "AGMSG: connected, but not syncing." '' - printf '%s\n' \ - 'No sync engine is running for the team(s) below, so messages from other' \ - 'machines are not arriving. Nothing restarts one after a reboot; run:' '' - # ONE RUNNABLE LINE PER TEAM, with no placeholder in it. - # - # The first version printed `sync start ` once. A reader has to - # replace `` before that works, and an unreplaced `` is a valid - # team name as far as validation is concerned — angle brackets are not among - # the characters it rejects. The names are already in hand here, so there is - # nothing to leave unfilled (the same reasoning #339 applied to the - # onboarding prompt). - # - # `printf %q` for both the path and the name: an install directory with a - # space in it, or a team name that needs quoting, otherwise produces a line - # that reads as runnable and is not. - printf '%s\n' "$_stale_teams" | while IFS= read -r _t; do - [ -n "$_t" ] || continue - printf ' bash %q sync start %q\n' "$SKILL_DIR/scripts/remote.sh" "$_t" - done - printf '\n' +if [ -x "$SKILL_DIR/scripts/remote.sh" ] && [ -r "$SKILL_DIR/scripts/lib/sync-autostart.sh" ]; then + # shellcheck source=scripts/lib/sync-autostart.sh + . "$SKILL_DIR/scripts/lib/sync-autostart.sh" + _connected_teams="$("$SKILL_DIR/scripts/remote.sh" status 2>/dev/null \ + | awk -F'\t' '$2 ~ /^connected/ {print $1}' || true)" + if [ -n "$_connected_teams" ]; then + # Word splitting on newlines only: a team name may contain a space, and + # `$(...)` unquoted would split it into two names that start nothing. + _old_ifs="$IFS"; IFS=$'\n' + # shellcheck disable=SC2086 + set -- $_connected_teams + IFS="$_old_ifs" + agmsg_sync_autostart "$SKILL_DIR/scripts/remote.sh" "$@" || true fi fi diff --git a/tests/test_delivery.bats b/tests/test_delivery.bats index 1f1b76e86..d99f562d3 100644 --- a/tests/test_delivery.bats +++ b/tests/test_delivery.bats @@ -491,11 +491,48 @@ eperm_pid() { [ "$3" = "$sp" ] } -@test "session-start: a connected team with no engine is said, and a silent one is not (#761)" { +@test "session-start: a connected team with no engine is started, and said when that fails (#761, #774)" { # A reboot kills every sync engine and nothing restarts one. `connected` keeps # printing and `send` keeps succeeding locally, so the only symptom is silence # — which reads as "nobody wrote anything". This hook is the first thing of # ours that runs afterwards. + # + # A RULING WAS REVERSED HERE, and this test is where it is recorded. + # + # #761/#765 decided: do NOT start anything, make the absence VISIBLE. That + # decision is what this test was written to hold. #774 reverses it — an agent + # arriving at a connected team now STARTS the engine — on the grounds that + # visibility asks a person for something the machine can do. + # + # What #765 built is not discarded: its warning, its wording and its runnable + # remedy are exactly what remains when the start FAILS, and that is the case + # driven below. The assertions about what the operator is told are therefore + # unchanged; only the reason they are reachable is new. This is a reversal of + # a decision, not a test edited to fit new output (raised in review). + # + # THE FAILURE IS FORCED BY THE COMMAND ITSELF, not by its environment. + # + # It used to be forced with an unusable interpreter, on the reasoning that + # `sync start` would then "fail immediately and for a named reason". That is + # a claim about a machine, and it was false on one: on a macOS CI runner the + # command had not returned after SIXTY seconds, so the hook printed "a start + # is still in flight" — a different fact, tested elsewhere — and this case + # failed for a reason that had nothing to do with what it asserts. + # + # So `remote.sh` is replaced, for this half of the test, by one that answers + # `status` with a connected team and refuses `sync start` at once. The real + # one is restored before the section below, which needs it to SUCCEED. + # + # It used to be inherited instead — the fixture simply had no engine to start + # — and that held only while this file ran alone: the case passed under + # `--filter` and failed in the full file, because what a start does depends on + # what other tests left behind. That is the same cross-test coupling this PR + # fixes in its own suite, arriving from the other direction. The condition is + # stated here so nothing about the surrounding file can decide it. + # + # The budget is raised as well: under the 5s default a slow failure is + # reported as "still in flight", which is a different fact and is tested + # separately in tests/test_sync_autostart.bats. env AGMSG_RESOLVE_PROJECT=0 bash "$SCRIPTS/join.sh" team alice claude-code "$TEST_PROJECT" >/dev/null # NEGATIVE FIRST, on the state every ordinary machine is in: no connected @@ -521,6 +558,23 @@ eperm_pid() { ));")" printf '%s\n' "$updated" > "$cfg" + # The stub goes in HERE, after the negative half has run against the real + # command. Installed any earlier it would report a connected team before one + # exists, and the negative assertion — the one that catches a line printed + # unconditionally — would be testing the stub instead of the hook. + cp "$SCRIPTS/remote.sh" "$TEST_SKILL_DIR/remote.real.sh" + { + printf '%s\n' '#!/usr/bin/env bash' + printf '%s\n' 'if [ "${1:-}" = "status" ] && [ -z "${2:-}" ]; then' + printf '%s\n' ' printf "team\tconnected since 2026-08-12\n"; exit 0' + printf '%s\n' 'fi' + printf '%s\n' 'if [ "${1:-}" = "sync" ]; then' + printf '%s\n' ' echo "agmsg: cannot start the sync engine for '"'"'$3'"'"': no runtime" >&2; exit 1' + printf '%s\n' 'fi' + printf '%s\n' 'exit 0' + } > "$SCRIPTS/remote.sh" + chmod +x "$SCRIPTS/remote.sh" + run env AGMSG_RESOLVE_PROJECT=0 bash "$SCRIPTS/session-start.sh" claude-code "$TEST_PROJECT" "$cfg" + mkdir -p "$TEST_SKILL_DIR/run" + ENGINE_PIDS="" +} + +teardown() { + local pid + for pid in $ENGINE_PIDS; do + kill "$pid" 2>/dev/null || true + wait "$pid" 2>/dev/null || true + done + # The "does not wait" cases leave a `sync start` child running ON PURPOSE — + # that is the behaviour under test. It must not outlive the test: a CI shard + # runs many files in one process tree, and a fake that loops forever would + # then be somebody else's flake (raised in review). + # BY THE PATH THEY ACTUALLY RUN UNDER. The hanging fake is COPIED over + # `$SCRIPTS/remote.sh`, so matching the name it was written as reaps nothing + # and the child outlives the whole file — which is how this suite stopped + # exiting even with every case green. Both paths are inside the test's own + # skill dir, so the pattern cannot reach anything else. + pkill -f "$TEST_SKILL_DIR/" 2>/dev/null || true + teardown_test_env +} + +# A node that becomes READY and then stays up. +# +# `cmd_sync_start` does not return when the process exists — it waits for the +# engine's `startup_nonce` to appear in the logfile, so a fake that only sleeps +# makes the command spin until its own timeout. Same shape as +# test_remote_status_liveness.bats's fake node, which is where this came from. +write_fake_node() { + local fake_node="$TEST_SKILL_DIR/fake-node" + printf '%s\n' '#!/usr/bin/env bash' \ + 'if [ "${1:-}" = "--version" ]; then' \ + ' echo v23.0.0' \ + ' exit 0' \ + 'fi' \ + 'echo "{\"event\":\"capabilities\",\"startup_nonce\":\"${AGMSG_SYNC_START_NONCE:-}\"}"' \ + 'trap "exit 0" TERM INT' \ + 'while :; do sleep 1; done' > "$fake_node" + chmod +x "$fake_node" + printf '%s\n' "$fake_node" +} + +# A node that fails to start at all. +write_failing_node() { + local fake_node="$TEST_SKILL_DIR/fake-node-bad" + printf '%s\n' '#!/usr/bin/env bash' \ + 'if [ "${1:-}" = "--version" ]; then echo v23.0.0; exit 0; fi' \ + 'echo "engine exploded" >&2' \ + 'exit 1' > "$fake_node" + chmod +x "$fake_node" + printf '%s\n' "$fake_node" +} + +# Register a (team, agent) pair for the test project, as the actas tests do. +fake_register() { + local team="$1" agent="$2" proj="${3:-/tmp/p1}" + bash "$SCRIPTS/join.sh" "$team" "$agent" claude-code "$proj" >/dev/null 2>&1 || true +} + +# Wait briefly for a call to be RECORDED. +# +# The "does not wait" cases give the helper a 1s budget, so it returns while the +# child is still running — and the child records the team name as its first act. +# Grepping immediately is therefore a race with a process the test deliberately +# did not wait for: it passed on an idle machine and went red under load, which +# is a flaky assertion dressed as a strict one. The bound here is generous +# because it is not measuring speed; the SESSION's bound is measured separately, +# from the outside, in the same test. +wait_for_call() { + local file="$1" needle="$2" i=0 + while [ "$i" -lt 100 ]; do + grep -q "^$needle\$" "$file" 2>/dev/null && return 0 + sleep 0.1 + i=$((i + 1)) + done + return 1 +} + +# A `remote.sh` that answers instantly, for the cases where the SUBJECT is what +# the helper does with an answer — not how long the real command takes. +# +# The real command is kept for the race case below, which is about inheriting +# its lock. Everywhere else it only made the suite slow and timing-coupled: +# raising the budget so a case could not be cut short is the same admission, +# with a worse failure mode (a 60s case that goes red when the machine is busy). +write_answering_remote() { + local answer="$1" fake="$TEST_SKILL_DIR/fake-remote-answer.sh" + { + printf '%s\n' '#!/usr/bin/env bash' + printf '%s\n' '[ "${1:-}" = "sync" ] || exit 0' + case "$answer" in + started) printf '%s\n' 'echo "Sync engine started for '"'"'$3'"'"' (pid 4242)."; exit 0' ;; + running) printf '%s\n' 'echo "Sync engine already running (pid 4242)."; exit 0' ;; + refused) printf '%s\n' 'echo "agmsg: team '"'"'$3'"'"' is disconnected; connect or pull it before starting sync" >&2; exit 1' ;; + broken) printf '%s\n' 'echo "engine exploded" >&2; exit 1' ;; + esac + } > "$fake" + chmod +x "$fake" + printf '%s\n' "$fake" +} + +collect_engine_pids() { + local pidfile="$TEST_SKILL_DIR/run/remote-sync.testteam.pid" + [ -f "$pidfile" ] && ENGINE_PIDS="$ENGINE_PIDS $(cat "$pidfile")" + return 0 +} + +@test "starts an engine for a connected team that has none" { + # THE REAL COMMAND, because this case asserts on the artifact it leaves: a + # pidfile naming a live process. The sentence is not the evidence. + export AGMSG_SYNC_AUTOSTART_TIMEOUT_S=60 + export AGMSG_NODE="$(write_fake_node)" + source "$SCRIPTS/lib/sync-autostart.sh" + run agmsg_sync_autostart "$SCRIPTS/remote.sh" testteam + collect_engine_pids + [ "$status" -eq 0 ] + printf '%s' "$output" | grep -q 'started one for' + printf '%s' "$output" | grep -q 'testteam' + [ -f "$TEST_SKILL_DIR/run/remote-sync.testteam.pid" ] + # Liveness through the shipped helper, not a bare kill -0 (a repo-wide check + # forbids the latter, and it caught this branch once already). + run bash -c 'source "'"$SCRIPTS"'/lib/instance-id.sh"; _agmsg_pid_alive "$(cat "'"$TEST_SKILL_DIR"'/run/remote-sync.testteam.pid")"' + [ "$status" -eq 0 ] +} + +@test "says nothing at all when the engine is already running" { + source "$SCRIPTS/lib/sync-autostart.sh" + run agmsg_sync_autostart "$(write_answering_remote running)" testteam + [ "$status" -eq 0 ] + # Starting is a side effect nobody asked for in this moment; "nothing + # changed" is not news, and a line here would appear on every session start + # for the rest of the machine's life. + [ -z "$output" ] +} + +@test "several sessions at once leave exactly one engine, and none of them fails" { + # THE CASE THIS FEATURE IS FOR. Five callers race for the per-team lock. + export AGMSG_NODE="$(write_fake_node)" + source "$SCRIPTS/lib/sync-autostart.sh" + + local i outdir="$TEST_SKILL_DIR/race" + mkdir -p "$outdir" + for i in 1 2 3 4 5; do + ( + agmsg_sync_autostart "$SCRIPTS/remote.sh" testteam > "$outdir/$i.out" 2>&1 + printf '%s\n' "$?" > "$outdir/$i.rc" + ) & + done + wait + collect_engine_pids + + # Every caller succeeded — the losers of the race are not failures. + for i in 1 2 3 4 5; do + [ "$(cat "$outdir/$i.rc")" = "0" ] + done + + # Exactly one of them reports having started it. The rest say nothing, which + # is what `already running` produces. + local started=0 quiet=0 + for i in 1 2 3 4 5; do + if grep -q "started one for" "$outdir/$i.out"; then + started=$((started + 1)) + elif [ ! -s "$outdir/$i.out" ]; then + quiet=$((quiet + 1)) + fi + done + [ "$started" -eq 1 ] + [ "$quiet" -eq 4 ] + + # And one engine exists, not five. Counted from the process table rather than + # from the pidfile: the pidfile can only ever name one, so asking it would be + # asking the wrong witness. + local pidfile="$TEST_SKILL_DIR/run/remote-sync.testteam.pid" + [ -f "$pidfile" ] + kill -0 "$(cat "$pidfile")" + # COUNTED IN THIS TEST'S OWN TREE. `fake-node` as a bare name matches any + # leftover from another run in the same process tree — a CI shard runs many + # files in one — so the count would include somebody else's engine and this + # assertion would fail for their leak rather than a second engine here. + # Measured: with two strays present on the machine it read 3 and went red, + # green on the run before, which is what a global pattern looks like from + # the inside. + local live + live="$(pgrep -f "$TEST_SKILL_DIR/fake-node" 2>/dev/null | wc -l | tr -d ' ')" + [ "$live" = "1" ] +} + +@test "a refusal from the command is repeated, not replaced" { + # THE SUBJECT IS THE HELPER'S HANDLING of a refusal, so the refusal is given + # to it directly. Driving the real command here made the case depend on how + # busy the machine was — it went green alone and red in the full file — and + # raising the budget only made it slow instead of wrong. + # + # The binding check itself belongs to `cmd_sync_start` and is tested where it + # lives; what is asserted here is that its sentence survives. + source "$SCRIPTS/lib/sync-autostart.sh" + run agmsg_sync_autostart "$(write_answering_remote refused)" testteam + [ "$status" -eq 0 ] + printf '%s' "$output" | grep -q 'disconnected' + printf '%s' "$output" | grep -q 'connected, but not syncing' +} + +@test "a start that fails does not fail the caller, and says what the command said" { + # An agent that will not open because a sync engine refused is worse than a + # sync engine that is down. + # + # The budget is raised for this case on purpose. `cmd_sync_start` does not + # notice a dead engine immediately — it waits out its readiness loop — so + # under the default 5s this failure is reported as "still in flight", which + # is TRUE and is a different sentence. The two outcomes are tested + # separately rather than folded together: "it failed" and "it has not + # answered yet" are different facts and the tool says different things. + source "$SCRIPTS/lib/sync-autostart.sh" + run agmsg_sync_autostart "$(write_answering_remote broken)" testteam + [ "$status" -eq 0 ] + printf '%s' "$output" | grep -q 'connected, but not syncing' + printf '%s' "$output" | grep -q 'The session continues.' + # The runnable remedy survives from #765 — the person now also knows it was + # tried. + printf '%s' "$output" | grep -q 'sync start' +} + +@test "no teams, no output, no failure" { + source "$SCRIPTS/lib/sync-autostart.sh" + run agmsg_sync_autostart "$SCRIPTS/remote.sh" + [ "$status" -eq 0 ] + [ -z "$output" ] +} + +# ── the two production triggers, driven for real ───────────────────────────── +# +# Everything above drives `agmsg_sync_autostart` directly, and deleting the +# wiring from either trigger leaves all of it green (raised in review). The +# wiring is the PR's whole point and it is different on each side: +# session-start awks `remote.sh status` for connected teams, actas-claim +# array-ifies `$TEAMS` after the claim. Neither follows from the helper being +# right. + +# A `remote.sh` this test controls, standing in for the real one so a trigger +# can be driven without a server. It records every call it was given. +write_fake_remote() { + local behaviour="$1" fake="$TEST_SKILL_DIR/fake-remote.sh" + { + printf '%s\n' '#!/usr/bin/env bash' + printf '%s\n' 'calls="$AGMSG_FAKE_REMOTE_CALLS"' + printf '%s\n' 'if [ "${1:-}" = "status" ]; then' + printf '%s\n' ' printf "%s\tconnected (engine stopped — run: x) since 2026-07-30T00:00:00Z\n" testteam' + printf '%s\n' ' printf "%s\tdisconnected (was connected until 2026-08-01T00:00:00Z)\n" otherteam' + printf '%s\n' ' exit 0' + printf '%s\n' 'fi' + printf '%s\n' 'if [ "${1:-}" = "sync" ] && [ "${2:-}" = "start" ]; then' + printf '%s\n' ' printf "%s\n" "$3" >> "$calls"' + case "$behaviour" in + starts) printf '%s\n' ' echo "Sync engine started for '"'"'$3'"'"' (pid 4242)."; exit 0' ;; + hangs) printf '%s\n' ' while :; do sleep 1; done' ;; + esac + printf '%s\n' 'fi' + printf '%s\n' 'exit 0' + } > "$fake" + chmod +x "$fake" + printf '%s\n' "$fake" +} + +@test "session-start starts the engine for a connected team, and still emits the directive" { + local fake calls="$TEST_SKILL_DIR/calls.txt" + fake="$(write_fake_remote starts)" + cp "$fake" "$SCRIPTS/remote.sh" + : > "$calls" + fake_register testteam alice + echo "sid-current" > "$RUN_DIR/cc-instance.$$" + + run env AGMSG_FAKE_REMOTE_CALLS="$calls" bash -c \ + 'printf "{\"session_id\":\"sid-current\"}" | bash "$1" claude-code /tmp/p1' _ \ + "$SCRIPTS/session-start.sh" + + # The connected team was started... + grep -q '^testteam$' "$calls" + # ...and the disconnected one was never offered to the command. + # + # `refute`, not `! grep`. A leading `!` does not trip errexit on either + # interpreter, so in a non-last position it reports ok whatever it finds — + # I removed five of those from this file and introduced this one in the same + # head (raised in review). + refute grep -q '^otherteam$' "$calls" + # ...and the thing the session actually needs still came out. + printf '%s' "$output" | grep -q 'AGMSG' + [ "$status" -eq 0 ] +} + +@test "session-start does not wait for a start that hangs" { + local fake calls="$TEST_SKILL_DIR/calls.txt" began ended + fake="$(write_fake_remote hangs)" + cp "$fake" "$SCRIPTS/remote.sh" + : > "$calls" + fake_register testteam alice + echo "sid-current" > "$RUN_DIR/cc-instance.$$" + + began=$SECONDS + run env AGMSG_FAKE_REMOTE_CALLS="$calls" AGMSG_SYNC_AUTOSTART_TIMEOUT_S=1 bash -c \ + 'printf "{\"session_id\":\"sid-current\"}" | bash "$1" claude-code /tmp/p1' _ \ + "$SCRIPTS/session-start.sh" + ended=$SECONDS + + # THAT IT WAS TRIED. Without this the case passes when the invocation is + # DELETED — nothing to wait for is also fast — so it would be measuring the + # absence of the feature and calling it a bound (found by the deletion + # mutation; the actas twin below had the same hole). + wait_for_call "$calls" testteam + # The bound, from the outside: a session that waits on a hung child is the + # release-blocker fix blocking a release. + [ $((ended - began)) -lt 10 ] + # It said a start is in flight rather than pretending nothing happened. + printf '%s' "$output" | grep -q 'still in flight' + [ "$status" -eq 0 ] +} + +@test "actas-claim starts the engine and still prints status=ok" { + local fake calls="$TEST_SKILL_DIR/calls.txt" + fake="$(write_fake_remote starts)" + cp "$fake" "$SCRIPTS/remote.sh" + : > "$calls" + fake_register testteam alice + + run env AGMSG_FAKE_REMOTE_CALLS="$calls" bash "$SCRIPTS/actas-claim.sh" \ + /tmp/project-a claude-code alice sid-actas + # The claim is what the caller is waiting on, and it still arrives. + printf '%s' "$output" | grep -q 'status=ok' + grep -q '^testteam$' "$calls" + [ "$status" -eq 0 ] +} + +@test "actas-claim does not wait for a start that hangs" { + local fake calls="$TEST_SKILL_DIR/calls.txt" began ended + fake="$(write_fake_remote hangs)" + cp "$fake" "$SCRIPTS/remote.sh" + : > "$calls" + fake_register testteam alice + + began=$SECONDS + run env AGMSG_FAKE_REMOTE_CALLS="$calls" AGMSG_SYNC_AUTOSTART_TIMEOUT_S=1 \ + bash "$SCRIPTS/actas-claim.sh" /tmp/project-a claude-code alice sid-actas + ended=$SECONDS + + # THAT IT WAS TRIED — see the session-start twin. Deleting the invocation + # made this case pass, which is the check measuring its own absence. + wait_for_call "$calls" testteam + [ $((ended - began)) -lt 10 ] + printf '%s' "$output" | grep -q 'status=ok' + [ "$status" -eq 0 ] +} + +# --- #773: why there is no refusal check here ------------------------------ +# +# An earlier version of this suite had three cases asserting that a team whose +# server had refused was never offered to `sync start`, plus two more bounding +# and reaping the lookup that made that possible. +# +# They are gone with the mechanism. The refusal check existed to stop a restart +# loop — the engine used to EXIT on a refusal — and #792 ended that: the engine +# records the refusal, backs off to its longest interval and keeps looping. A +# refused team costs one quiet process that reports the reason through +# `status`, so there is nothing here to prevent, and nothing to test. +# +# Recorded rather than silently dropped, because "there used to be a check" +# reads as an oversight to whoever finds this next.