From 2d80d37f0ea7763f927116ea552ccc2c2c8e69ac Mon Sep 17 00:00:00 2001 From: fujibee Date: Mon, 17 Aug 2026 22:26:10 -0700 Subject: [PATCH 1/3] feat(doctor): name the registry locks, and which directory to remove (#865) A lock left behind by a killed process is never broken by anything: nothing expires, nothing sweeps, and acquire waits out its budget and reports 'timed out acquiring registry lock', which describes contention. The operator goes looking for the process holding it; there is none, and no message anywhere names the directory to remove. A machine in that state cannot get itself out. doctor now lists each team's lock with what it records, whether that process is running, and the removal as a line to paste. Three answers, not two. 'held' and 'the holder is gone' are what an operator acts on; 'no holder recorded' is neither - a lock this cannot ask about, written either by a library version that recorded nothing or by a process killed between creating the lock and writing its record. Calling that gone would be a guess, and the guess that costs is the one that says a live lock is dead. It removes nothing, and it does not touch the exit code. A team being locked is not a failed prerequisite - reporting it as one would fail doctor every time somebody is joining - and sweeping automatically is a separate decision with a worse failure mode. Reported for the named team when doctor is given one, matching what its header already says. --- scripts/remote.sh | 69 ++++++++++++++++++++++++++++++ tests/test_remote.bats | 97 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 166 insertions(+) diff --git a/scripts/remote.sh b/scripts/remote.sh index e17de5846..70ce62f7f 100644 --- a/scripts/remote.sh +++ b/scripts/remote.sh @@ -221,6 +221,74 @@ _remote_prompt_read() { # token, no state change, safe whether or not the team is already connected. # Currently just the age-binary-presence check (§8) — the natural home for # any future preflight check added later. +# WHICH DIRECTORY TO REMOVE — the thing that was missing (#865). +# +# A registry lock left behind by a killed process is never broken by anything: +# nothing expires, nothing sweeps, and acquire waits out its budget and fails +# with `timed out acquiring registry lock`, which describes contention. The +# operator's next move is to look for the process holding it; there is no +# process, and no message anywhere names the directory to remove. A machine in +# that state cannot get itself out. +# +# This REPORTS and removes nothing. The report alone ends "cannot recover": +# it says which team, what the lock records, whether that process is running, +# and prints the removal as a line to paste. Sweeping automatically is a +# separate decision with a worse failure mode — a wrong verdict takes a lock +# away from a process that is using it — and it is not made here. +# +# Not a check, so no `[x]`/`[ ]` and no effect on the exit code: a lock that +# exists is not a failed prerequisite, and a doctor that exits non-zero because +# a team is busy would be wrong every time somebody is joining. +_remote_doctor_locks() { + local only_team="${1:-}" lock team holder pid state shown=0 q + # Every team, or the one named. `for` over a glob rather than `find`, which is + # not on every PATH this tree is required to run under. + for lock in "$TEAMS_DIR"/*/.config.lock; do + [ -d "$lock" ] || continue + team="${lock%/.config.lock}" + team="${team##*/}" + [ -z "$only_team" ] || [ "$team" = "$only_team" ] || continue + if [ "$shown" -eq 0 ]; then + echo "Registry locks:" + shown=1 + fi + holder="$lock.holder" + pid="" + [ -f "$holder" ] && pid="$(sed -n 's/^pid //p' "$holder" 2>/dev/null | head -1)" + # THREE ANSWERS, NOT TWO. "held" and "the holder is gone" are what the + # operator acts on; "no holder recorded" is neither — it is a lock this + # cannot ask about, written either by a version of the library that recorded + # nothing or by a process that was killed between creating the lock and + # writing its record. Reporting it as gone would be a guess, and the guess + # that costs is the one that says a live lock is dead. + if [ -z "$pid" ]; then + state="cannot tell — no holder recorded" + elif _agmsg_pid_alive_local "$pid"; then + state="held — pid $pid is running" + else + state="stale — pid $pid is not running" + fi + echo " $team: $state" + if [ -f "$holder" ]; then + echo " records: $(tr '\n' ' ' < "$holder" 2>/dev/null)" + fi + echo " created: $(ls -ld "$lock" 2>/dev/null || printf '%s (cannot stat)' "$lock")" + # QUOTED, because this is meant to be pasted. The store root and the team + # name can both contain a space, and an unquoted path becomes several + # arguments to `rm -r`. Same scheme as lib/shquote.sh. + q="$(printf "'%s'" "$(printf '%s' "$lock" | sed "s/'/'\\\\''/g")")" + if [ "$state" = "held — pid $pid is running" ]; then + echo " a command is using this team. Nothing to do." + else + echo " if no agmsg command is running for this team, remove it:" + echo " rm -r $q" + [ -f "$holder" ] && echo " rm -f $q.holder" + echo " nothing but the lock lives in there — it holds no team data." + fi + done + [ "$shown" -eq 0 ] || echo +} + cmd_doctor() { local team="${1:-}" echo "Checking prerequisites${team:+ for team '$team'}..." @@ -275,6 +343,7 @@ cmd_doctor() { failed=1 fi echo + _remote_doctor_locks "$team" if [ "$failed" -eq 0 ]; then echo "All checks passed." else diff --git a/tests/test_remote.bats b/tests/test_remote.bats index 6b0665bc0..f3048ba65 100644 --- a/tests/test_remote.bats +++ b/tests/test_remote.bats @@ -2714,3 +2714,100 @@ assert_lookup_rejected() { [[ "$output" != *"No such file or directory"*"remote.sh"* ]] [[ "$output" != *"Usage: remote.sh unlock"* ]] } + +# --- doctor names the wedged lock, and removes nothing (#865) --------------- +# +# A registry lock left by a killed process is never broken: acquire waits out +# its budget and says "timed out acquiring registry lock", which describes +# contention, and no message anywhere names the directory to remove. `doctor` is +# where that becomes findable. + +doctor_lock_dead_pid() { # a pid that is genuinely not running + local p + sleep 0 & + p=$! + wait "$p" 2>/dev/null || true + printf '%s' "$p" +} + +make_lock() { # make_lock [pid] + mkdir -p "$TEST_SKILL_DIR/teams/$1" + mkdir -p "$TEST_SKILL_DIR/teams/$1/.config.lock" + if [ -n "${2:-}" ]; then + printf 'token t\npid %s\ncommand join.sh\nhost h\n' "$2" \ + > "$TEST_SKILL_DIR/teams/$1/.config.lock.holder" + fi +} + +@test "remote doctor: a lock whose holder is gone is named, with the way out (#865)" { + local gone; gone="$(doctor_lock_dead_pid)" + # CONTROL: the pid really is not running, asserted before it is written into + # the holder — a number that merely happened to be free would make this pass + # for a reason that has nothing to do with the report. + run bash -c ". \"$SCRIPTS/lib/instance-id.sh\"; _agmsg_pid_alive_local $gone" + [ "$status" -ne 0 ] + + make_lock wedged "$gone" + run bash "$SCRIPTS/remote.sh" doctor + [ "$status" -eq 0 ] + grep -qF -- "wedged: stale — pid $gone is not running" <<<"$output" + grep -qF -- "rm -r " <<<"$output" + # REPORTS ONLY. The whole point of doing this before an automatic sweep is + # that a wrong verdict must not cost anything. + [ -d "$TEST_SKILL_DIR/teams/wedged/.config.lock" ] +} + +@test "remote doctor: a lock whose holder is alive is not called stale (#865)" { + sleep 30 & + local live=$! + # CONTROL: alive at the moment doctor runs, not merely spawned. + run bash -c ". \"$SCRIPTS/lib/instance-id.sh\"; _agmsg_pid_alive_local $live" + [ "$status" -eq 0 ] + + make_lock busy "$live" + run bash "$SCRIPTS/remote.sh" doctor + [ "$status" -eq 0 ] + grep -qF -- "busy: held — pid $live is running" <<<"$output" + # And it must NOT offer to remove a lock somebody is using. + refute grep -qF -- "if no agmsg command is running for this team" <<<"$output" + kill "$live" 2>/dev/null || true +} + +@test "remote doctor: a lock with no holder record says it cannot tell (#865)" { + # Neither "held" nor "gone": written by a version that recorded nothing, or by + # a process killed between creating the lock and writing its record. Guessing + # here is what would cost a live lock. + make_lock unknown + run bash "$SCRIPTS/remote.sh" doctor + [ "$status" -eq 0 ] + grep -qF -- "unknown: cannot tell — no holder recorded" <<<"$output" + grep -qF -- "rm -r " <<<"$output" +} + +@test "remote doctor: says nothing about locks when there are none (#865)" { + # The negative control for the three above: the section is absent on a healthy + # install, so "Registry locks:" appearing at all is a finding. + run bash "$SCRIPTS/remote.sh" doctor + [ "$status" -eq 0 ] + refute grep -qF -- "Registry locks:" <<<"$output" +} + +@test "remote doctor: a lock does not fail the run (#865)" { + # A team being locked is not a failed prerequisite. If it set the exit code, + # doctor would fail every time somebody is joining. + local gone; gone="$(doctor_lock_dead_pid)" + make_lock wedged "$gone" + run bash "$SCRIPTS/remote.sh" doctor + [ "$status" -eq 0 ] + grep -qF -- "All checks passed." <<<"$output" +} + +@test "remote doctor : reports that team's lock and not another's (#865)" { + local gone; gone="$(doctor_lock_dead_pid)" + make_lock mine "$gone" + make_lock theirs "$gone" + run bash "$SCRIPTS/remote.sh" doctor mine + [ "$status" -eq 0 ] + grep -qF -- "mine: stale" <<<"$output" + refute grep -qF -- "theirs: stale" <<<"$output" +} From 24a2b5a49f406ae8d7872739850cbf1f78215a5d Mon Sep 17 00:00:00 2001 From: fujibee Date: Mon, 17 Aug 2026 22:36:25 -0700 Subject: [PATCH 2/3] fix(doctor): an unusable pid was never asked about, so it is not stale (#865 review) Three findings, all from static review. _agmsg_pid_alive_local returns false for a value it never put to the process table - non-numeric, leading zero, past the POSIX ceiling - and folding that into 'not running' turned 'pid not-a-pid' into a stale verdict with a rm -r beside it. The number is validated with the same ceiling the helper uses before it is asked about, and an unusable one joins 'no holder recorded' under cannot-tell. The sweep used *, which does not match a leading dot, and team names may begin with one - the validator rejects empty, . , .. , a leading -, / , \ and control characters, and nothing else. A named team is now looked at directly, and the sweep adds the two dot patterns. All checks passed. printed under a stale lock and its removal command reads as withdrawing them. Narrowed to All prerequisite checks passed.; the exit code deliberately does not move, so the wording is what carries it. Five existing assertions updated with it. Reporting one lock is its own function now. Both callers share it, so neither has to join paths into a string and read them back - which would mean an unquoted heredoc, running command substitution on a team name, or unquoted word splitting, globbing one. The first attempt at the sweep did build such a string and fed a while-read loop that had no redirection at all, so it read stdin and reported nothing. --- scripts/remote.sh | 126 ++++++++++++++++++++++++++--------------- tests/test_remote.bats | 62 ++++++++++++++++++-- 2 files changed, 137 insertions(+), 51 deletions(-) diff --git a/scripts/remote.sh b/scripts/remote.sh index 70ce62f7f..99baa340f 100644 --- a/scripts/remote.sh +++ b/scripts/remote.sh @@ -239,53 +239,81 @@ _remote_prompt_read() { # Not a check, so no `[x]`/`[ ]` and no effect on the exit code: a lock that # exists is not a failed prerequisite, and a doctor that exits non-zero because # a team is busy would be wrong every time somebody is joining. +# One lock, reported. Split out so the two ways of finding them — a named team, +# or a sweep — share this body and neither has to feed a loop from a string. +# Joining the paths into one variable and reading it back would mean either an +# unquoted heredoc, which runs command substitution on a team name, or unquoted +# word splitting, which globs one. +_remote_doctor_one_lock() { + local lock="$1" team holder pid state q + [ -d "$lock" ] || return 0 + team="${lock%/.config.lock}" + team="${team##*/}" + if [ "$shown" -eq 0 ]; then + echo "Registry locks:" + shown=1 + fi + holder="$lock.holder" + pid="" + [ -f "$holder" ] && pid="$(sed -n 's/^pid //p' "$holder" 2>/dev/null | head -1)" + # THREE ANSWERS, NOT TWO. "held" and "the holder is gone" are what the + # operator acts on; "cannot tell" is neither — a lock this could not ask + # about, and saying it is gone would be a guess. The guess that costs is the + # one that calls a live lock dead and then prints the command to remove it. + # + # WHICH IS WHY THE NUMBER IS VALIDATED BEFORE IT IS ASKED ABOUT. + # `_agmsg_pid_alive_local` returns false for a value it never put to the + # process table at all — non-numeric, leading zero, past the POSIX ceiling — + # and folding that into "not running" turned `pid not-a-pid` into a stale + # verdict with a removal beside it (raised in review). The same ceiling the + # helper uses, for the same reason it uses it. + if [ -z "$pid" ]; then + state="cannot tell — no holder recorded" + elif ! _agmsg_pid_valid "$pid" 2147483647; then + state="cannot tell — the holder record's pid is not a usable number" + elif _agmsg_pid_alive_local "$pid"; then + state="held — pid $pid is running" + else + state="stale — pid $pid is not running" + fi + echo " $team: $state" + if [ -f "$holder" ]; then + echo " records: $(tr '\n' ' ' < "$holder" 2>/dev/null)" + fi + echo " created: $(ls -ld "$lock" 2>/dev/null || printf '%s (cannot stat)' "$lock")" + # QUOTED, because this is meant to be pasted. The store root and the team + # name can both contain a space, and an unquoted path becomes several + # arguments to `rm -r`. Same scheme as lib/shquote.sh. + q="$(printf "'%s'" "$(printf '%s' "$lock" | sed "s/'/'\\\\''/g")")" + if [ "$state" = "held — pid $pid is running" ]; then + echo " a command is using this team. Nothing to do." + else + echo " if no agmsg command is running for this team, remove it:" + echo " rm -r $q" + [ -f "$holder" ] && echo " rm -f $q.holder" + echo " nothing but the lock lives in there — it holds no team data." + fi +} + _remote_doctor_locks() { - local only_team="${1:-}" lock team holder pid state shown=0 q - # Every team, or the one named. `for` over a glob rather than `find`, which is - # not on every PATH this tree is required to run under. - for lock in "$TEAMS_DIR"/*/.config.lock; do - [ -d "$lock" ] || continue - team="${lock%/.config.lock}" - team="${team##*/}" - [ -z "$only_team" ] || [ "$team" = "$only_team" ] || continue - if [ "$shown" -eq 0 ]; then - echo "Registry locks:" - shown=1 - fi - holder="$lock.holder" - pid="" - [ -f "$holder" ] && pid="$(sed -n 's/^pid //p' "$holder" 2>/dev/null | head -1)" - # THREE ANSWERS, NOT TWO. "held" and "the holder is gone" are what the - # operator acts on; "no holder recorded" is neither — it is a lock this - # cannot ask about, written either by a version of the library that recorded - # nothing or by a process that was killed between creating the lock and - # writing its record. Reporting it as gone would be a guess, and the guess - # that costs is the one that says a live lock is dead. - if [ -z "$pid" ]; then - state="cannot tell — no holder recorded" - elif _agmsg_pid_alive_local "$pid"; then - state="held — pid $pid is running" - else - state="stale — pid $pid is not running" - fi - echo " $team: $state" - if [ -f "$holder" ]; then - echo " records: $(tr '\n' ' ' < "$holder" 2>/dev/null)" - fi - echo " created: $(ls -ld "$lock" 2>/dev/null || printf '%s (cannot stat)' "$lock")" - # QUOTED, because this is meant to be pasted. The store root and the team - # name can both contain a space, and an unquoted path becomes several - # arguments to `rm -r`. Same scheme as lib/shquote.sh. - q="$(printf "'%s'" "$(printf '%s' "$lock" | sed "s/'/'\\\\''/g")")" - if [ "$state" = "held — pid $pid is running" ]; then - echo " a command is using this team. Nothing to do." - else - echo " if no agmsg command is running for this team, remove it:" - echo " rm -r $q" - [ -f "$holder" ] && echo " rm -f $q.holder" - echo " nothing but the lock lives in there — it holds no team data." - fi - done + local only_team="${1:-}" lock shown=0 + # A NAMED TEAM IS LOOKED AT DIRECTLY, and the sweep does not stop at `*`. + # + # Team names may begin with a dot — the validator rejects empty, `.`, `..`, a + # leading `-`, `/`, `\` and control characters, and nothing else — and `*` + # does not match a leading dot, so `.foo` was invisible to the sweep (raised + # in review). The two extra patterns cover `.foo` and `..foo`; `.` and `..` + # themselves cannot be team names. + # + # Globs rather than `find`, which is not on every PATH this tree is required + # to run under. Quoted, so a team name containing a space stays one path. + if [ -n "$only_team" ]; then + _remote_doctor_one_lock "$TEAMS_DIR/$only_team/.config.lock" + else + for lock in "$TEAMS_DIR"/*/.config.lock "$TEAMS_DIR"/.[!.]*/.config.lock "$TEAMS_DIR"/..?*/.config.lock; do + _remote_doctor_one_lock "$lock" + done + fi [ "$shown" -eq 0 ] || echo } @@ -345,7 +373,11 @@ cmd_doctor() { echo _remote_doctor_locks "$team" if [ "$failed" -eq 0 ]; then - echo "All checks passed." + # NARROWED, because a lock report can sit above this line. "All checks + # passed." after "stale — pid 4711 is not running" and a `rm -r` reads as + # cancelling it, and the exit code deliberately does not move: a locked team + # is not a failed prerequisite (raised in review). + echo "All prerequisite checks passed." else echo "Some checks failed. See above." exit 1 diff --git a/tests/test_remote.bats b/tests/test_remote.bats index f3048ba65..b2f1643fa 100644 --- a/tests/test_remote.bats +++ b/tests/test_remote.bats @@ -115,7 +115,7 @@ skip_if_no_age() { run bash "$SCRIPTS/remote.sh" doctor [ "$status" -eq 0 ] [[ "$output" == *"age / age-keygen on PATH"* ]] - [[ "$output" == *"All checks passed."* ]] + [[ "$output" == *"All prerequisite checks passed."* ]] } @test "remote doctor: is read-only (no token required, no state touched)" { @@ -1550,7 +1550,7 @@ VALUES ('remote-pending.$key', $owner_pid, strftime('%Y-%m-%dT%H:%M:%SZ','now')) || skip "all doctor prerequisites are not installed" run bash "$SCRIPTS/remote.sh" doctor [ "$status" -eq 0 ] - [[ "$output" == *"All checks passed."* ]] + [[ "$output" == *"All prerequisite checks passed."* ]] } PULL_TEAM_ID=018f3f7e-2222-7000-8000-000000000002 @@ -2582,7 +2582,7 @@ PY local no_age; no_age="$(path_without_age)" run env PATH="$no_age" bash "$SCRIPTS/remote.sh" doctor [ "$status" -eq 0 ] - [[ "$output" == *"All checks passed"* ]] + [[ "$output" == *"All prerequisite checks passed"* ]] [[ "$output" == *"optional"* ]] [[ "$output" != *"is required for end-to-end encryption"* ]] } @@ -2799,7 +2799,7 @@ make_lock() { # make_lock [pid] make_lock wedged "$gone" run bash "$SCRIPTS/remote.sh" doctor [ "$status" -eq 0 ] - grep -qF -- "All checks passed." <<<"$output" + grep -qF -- "All prerequisite checks passed." <<<"$output" } @test "remote doctor : reports that team's lock and not another's (#865)" { @@ -2811,3 +2811,57 @@ make_lock() { # make_lock [pid] grep -qF -- "mine: stale" <<<"$output" refute grep -qF -- "theirs: stale" <<<"$output" } + +@test "remote doctor: a pid that was never asked about is not called stale (#865)" { + # THE VERDICT AND THE REMOVAL RIDE TOGETHER, so "false" from the liveness + # helper is not enough on its own: it is also false for a value the helper + # refused to put to the process table at all. `pid not-a-pid` and a number + # past the POSIX ceiling were being reported as stale, with `rm -r` beside + # them (raised in review). + make_lock badpid + printf 'token t\npid not-a-pid\ncommand join.sh\nhost h\n' \ + > "$TEST_SKILL_DIR/teams/badpid/.config.lock.holder" + run bash "$SCRIPTS/remote.sh" doctor + [ "$status" -eq 0 ] + grep -qF -- "badpid: cannot tell" <<<"$output" + refute grep -qF -- "badpid: stale" <<<"$output" + + # CONTROL: the helper does answer false for it, so this case is measuring the + # validation and not some other difference. + run bash -c ". \"$SCRIPTS/lib/instance-id.sh\"; _agmsg_pid_alive_local not-a-pid" + [ "$status" -ne 0 ] +} + +@test "remote doctor: a pid past the POSIX ceiling is not called stale (#865)" { + make_lock hugepid + printf 'token t\npid 2147483648\ncommand join.sh\nhost h\n' \ + > "$TEST_SKILL_DIR/teams/hugepid/.config.lock.holder" + run bash "$SCRIPTS/remote.sh" doctor + [ "$status" -eq 0 ] + grep -qF -- "hugepid: cannot tell" <<<"$output" + refute grep -qF -- "hugepid: stale" <<<"$output" +} + +@test "remote doctor: a team whose name begins with a dot is swept too (#865)" { + # `*` does not match a leading dot, and the team validator allows one — so the + # sweep walked past `.hidden` entirely (raised in review). The validator + # rejects empty, `.`, `..`, a leading `-`, `/`, `\` and control characters, + # and nothing else. + local gone; gone="$(doctor_lock_dead_pid)" + make_lock .hidden "$gone" + run bash "$SCRIPTS/remote.sh" doctor + [ "$status" -eq 0 ] + grep -qF -- ".hidden: stale" <<<"$output" +} + +@test "remote doctor: the summary does not cancel a lock finding (#865)" { + # "All checks passed." above a stale lock and a removal command reads as + # withdrawing them. The exit code deliberately stays 0 — a locked team is not + # a failed prerequisite — so the wording is what has to carry the distinction. + local gone; gone="$(doctor_lock_dead_pid)" + make_lock wedged "$gone" + run bash "$SCRIPTS/remote.sh" doctor + [ "$status" -eq 0 ] + grep -qF -- "All prerequisite checks passed." <<<"$output" + refute grep -qE '^All checks passed\.$' <<<"$output" +} From 3d3b0b784d49cbc1926bf07c617bc05049fa1318 Mon Sep 17 00:00:00 2001 From: fujibee Date: Mon, 17 Aug 2026 22:42:06 -0700 Subject: [PATCH 3/3] fix(doctor): validate the named team before it becomes a path (#865 review) cmd_doctor takes an optional team and, until the direct lookup landed, only ever put it in a header sentence - so nothing had ever validated it. Building TEAMS_DIR/$team/.config.lock from it turned 'doctor ../outside' into a read of a directory outside the store, reported with its records and a rm -r to paste. The sweep it replaced never reached one only because no team was named that. Validated with agmsg_validate_team_name, the same one every other team-taking path uses, and a rejection ends the command rather than falling through to a prerequisites verdict for a question nobody answered. Three cases: a traversal argument reads nothing (with a sentinel .config .lock outside the store to prove the refusal is about reach), a slash is refused, and an ordinary and a dot-leading name still resolve - a refusal that took the legitimate names with it would be the cheapest way to pass the first two. --- scripts/remote.sh | 16 +++++++++++++++- tests/test_remote.bats | 42 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 57 insertions(+), 1 deletion(-) diff --git a/scripts/remote.sh b/scripts/remote.sh index 99baa340f..ceafe1ad2 100644 --- a/scripts/remote.sh +++ b/scripts/remote.sh @@ -308,6 +308,17 @@ _remote_doctor_locks() { # Globs rather than `find`, which is not on every PATH this tree is required # to run under. Quoted, so a team name containing a space stays one path. if [ -n "$only_team" ]; then + # VALIDATED BEFORE IT BECOMES A PATH. `cmd_doctor` takes its argument and, + # until this line, only ever put it in a header sentence — so nothing had + # ever checked it. Building `$TEAMS_DIR/$only_team/.config.lock` from it + # turned `doctor ../outside` into a read of a directory outside the store, + # reported with its records and a `rm -r` to paste (raised in review). The + # sweep it replaced never reached one only because no team was named that. + # + # The same validator every other team-taking path uses: it rejects empty, + # `.`, `..`, `/`, `\`, a leading `-` and control characters, and allows a + # leading dot and a space, which this reports on and must keep. + agmsg_validate_team_name "$only_team" || return 1 _remote_doctor_one_lock "$TEAMS_DIR/$only_team/.config.lock" else for lock in "$TEAMS_DIR"/*/.config.lock "$TEAMS_DIR"/.[!.]*/.config.lock "$TEAMS_DIR"/..?*/.config.lock; do @@ -371,7 +382,10 @@ cmd_doctor() { failed=1 fi echo - _remote_doctor_locks "$team" + # A REJECTED TEAM NAME ENDS THE COMMAND. The validator prints why; carrying on + # to "All prerequisite checks passed." after refusing to look at what was + # asked about would report success for a question nobody answered. + _remote_doctor_locks "$team" || exit 1 if [ "$failed" -eq 0 ]; then # NARROWED, because a lock report can sit above this line. "All checks # passed." after "stale — pid 4711 is not running" and a `rm -r` reads as diff --git a/tests/test_remote.bats b/tests/test_remote.bats index b2f1643fa..d544c3751 100644 --- a/tests/test_remote.bats +++ b/tests/test_remote.bats @@ -2865,3 +2865,45 @@ make_lock() { # make_lock [pid] grep -qF -- "All prerequisite checks passed." <<<"$output" refute grep -qE '^All checks passed\.$' <<<"$output" } + +@test "remote doctor : a traversal argument is refused and reads nothing (#865)" { + # The named-team lookup builds a path from the argument, and until this was + # added nothing had ever validated it — `cmd_doctor` only ever put the value + # in a header sentence. A sentinel outside the store proves the refusal is + # about reach and not about the string. + local outside="$BATS_TEST_TMPDIR/outside" + mkdir -p "$outside/.config.lock" + printf 'token t\npid 1\ncommand join.sh\nhost h\n' > "$outside/.config.lock.holder" + # CONTROL: the sentinel is real and would be reported if it were reached — + # the same shape, under a team name, IS reported (the case above). + [ -d "$outside/.config.lock" ] + + run bash "$SCRIPTS/remote.sh" doctor "../../$(basename "$BATS_TEST_TMPDIR")/outside" + [ "$status" -ne 0 ] + refute grep -qF -- "Registry locks:" <<<"$output" + refute grep -qF -- "rm -r " <<<"$output" + grep -qF -- "invalid team name" <<<"$output" + # And it must not claim the prerequisites verdict for a question it refused. + refute grep -qF -- "All prerequisite checks passed." <<<"$output" +} + +@test "remote doctor : a slash in the name is refused (#865)" { + run bash "$SCRIPTS/remote.sh" doctor "a/b" + [ "$status" -ne 0 ] + grep -qF -- "invalid team name" <<<"$output" +} + +@test "remote doctor : an ordinary and a dot-leading name still resolve (#865)" { + # The validator allows both, and the direct lookup has to keep working for + # them — a refusal that took the legitimate names with it would be the + # cheapest way to pass the two cases above. + local gone; gone="$(doctor_lock_dead_pid)" + make_lock plain "$gone" + make_lock .dotted "$gone" + run bash "$SCRIPTS/remote.sh" doctor plain + [ "$status" -eq 0 ] + grep -qF -- "plain: stale" <<<"$output" + run bash "$SCRIPTS/remote.sh" doctor .dotted + [ "$status" -eq 0 ] + grep -qF -- ".dotted: stale" <<<"$output" +}