diff --git a/scripts/doctor.sh b/scripts/doctor.sh index abf3ce9f4..ff5fe422c 100755 --- a/scripts/doctor.sh +++ b/scripts/doctor.sh @@ -1,9 +1,25 @@ #!/usr/bin/env bash set -euo pipefail -# doctor.sh — "who holds what" for a (project, type) in one screen. #267/#605. +# doctor.sh — "who holds what" in one screen. #267/#605. # -# Usage: doctor.sh [--redacted] +# Usage: doctor.sh [--project ] [--type ] [--team ] [--redacted] +# doctor.sh --help +# +# Default (no filters): the whole installation -- every team, every project, +# every type. --project / --type / --team narrow it and combine freely. This +# matches how claude/codex/brew/flutter doctor all behave (no scope argument, +# default to everything) rather than requiring a cross-section up front -- +# koit's call, made explicit because the earlier -required +# form had it backwards: a reporter who doesn't already know which project/type +# to name can't use a doctor that demands one. Positional is +# not kept for compatibility -- koit judged it not worth carrying (see PR/report +# history for round 2), and a stale positional form alongside flags that mean +# something different by default would be its own source of confusion. +# +# Argument parsing is kept separate from scope-building (the SCOPE loop +# below) so that a future change to what the flags are doesn't have to touch +# how a scope, once decided, gets turned into (project, type) pairs. # # Read-only: never claims, releases, or removes a lock, pidfile, or # registration. A stale lock or dead pidfile is reported, not cleaned up -- @@ -13,9 +29,10 @@ set -euo pipefail # Data sources are the existing helpers this project already has for each # fact -- identities.sh for registrations, actas-lock.sh/instance-id.sh for # lock ownership and liveness, delivery.sh for mode and watcher/bridge -# status. Nothing here recomputes a verdict those already reach; #605's -# diagnostic duplicated agmsg_instance_alive once and that duplication was -# exactly what review pushed back on. +# status, agmsg_registered_projects for cross-project/cross-type discovery. +# Nothing here recomputes a verdict those already reach; #605's diagnostic +# duplicated agmsg_instance_alive once and that duplication was exactly +# what review pushed back on. # # Exit codes: # 0 no warnings @@ -23,13 +40,12 @@ set -euo pipefail # 2 usage or resolution error _usage() { - echo "Usage: doctor.sh [--redacted]" >&2 + echo "Usage: doctor.sh [--project ] [--type ] [--team ] [--redacted]" >&2 + echo " doctor.sh --help" >&2 } -# Scanned for --help before the positional args are required below: with -# ${1:?...} doing that job instead, `doctor.sh --help` alone reads --help as -# PROJECT, then dies on the missing TYPE with "Missing agent_type" (bash's own -# nounset message, exit 1) and never reaches a help branch at all. +# Scanned for --help before anything else is parsed, same reasoning as +# before: a validation error on some other flag must never suppress --help. for _arg in "${@:-}"; do case "$_arg" in -h|--help) _usage; exit 0 ;; @@ -37,20 +53,27 @@ for _arg in "${@:-}"; do done unset _arg -if [ "$#" -lt 2 ]; then - _usage - exit 2 -fi - -PROJECT="$1" -TYPE="$2" -shift 2 - +# --- argument parsing: produces FILTER_PROJECT / FILTER_TYPE / FILTER_TEAM / +# REDACTED only. Deliberately does not decide what a scope IS -- that is +# entirely the SCOPE-building block below, so a future flag change stays +# a parsing-only change. No positional arguments are accepted -- any +# bare token is a usage error. ----------------------------------------- REDACTED=0 +FILTER_PROJECT="" FILTER_TYPE="" FILTER_TEAM="" while [ "$#" -gt 0 ]; do case "$1" in + --project) + case "${2:-}" in ''|-*) echo "doctor: --project requires a value" >&2; exit 2 ;; esac + FILTER_PROJECT="$2"; shift 2 ;; + --type) + case "${2:-}" in ''|-*) echo "doctor: --type requires a value" >&2; exit 2 ;; esac + FILTER_TYPE="$2"; shift 2 ;; + --team) + case "${2:-}" in ''|-*) echo "doctor: --team requires a value" >&2; exit 2 ;; esac + FILTER_TEAM="$2"; shift 2 ;; --redacted) REDACTED=1; shift ;; - *) echo "doctor: unknown option: $1" >&2; exit 2 ;; + -*) echo "doctor: unknown option: $1" >&2; exit 2 ;; + *) echo "doctor: unexpected argument: '$1' (doctor takes flags only -- see --help)" >&2; exit 2 ;; esac done @@ -63,30 +86,129 @@ RUN_DIR="$SKILL_DIR/run" . "$SCRIPT_DIR/lib/actas-lock.sh" # shellcheck disable=SC1091 . "$SCRIPT_DIR/lib/type-registry.sh" +# shellcheck disable=SC1091 +. "$SCRIPT_DIR/lib/validate.sh" -# Rejected here, not left to delivery.sh/identities.sh to fail into: those -# don't error loudly on an unknown type, they just return empty/quiet, which -# is exactly the "warning text on screen, exit 0 underneath" shape that made -# this a blocking finding in review. -if ! agmsg_is_known_type "$TYPE"; then - echo "doctor: unknown agent type: '$TYPE' (supported: $(agmsg_known_types | sort -u | paste -sd, - | sed 's/,/, /g'))" >&2 +# --project / --type / --team all validated here, before any scope work: +# an unknown --type or --team is a usage error (exit 2), not left to fail +# quietly into an empty scope -- same "no silent empty-looks-clean report" +# reasoning as the original form's type check. An unknown +# --project has no fixed enum to check against; it falls through to the +# generic "no registrations match this scope" exit 2 below instead, which +# reaches the same exit code by the same means every other empty scope does. +if [ -n "$FILTER_TYPE" ] && ! agmsg_is_known_type "$FILTER_TYPE"; then + echo "doctor: unknown agent type: '$FILTER_TYPE' (supported: $(agmsg_known_types | sort -u | paste -sd, - | sed 's/,/, /g'))" >&2 exit 2 fi +if [ -n "$FILTER_TEAM" ]; then + # --team becomes a path segment below (teams/$FILTER_TEAM/config.json, + # both here and inside agmsg_registered_projects) whether or not it turns + # out to name a real team -- validate.sh's header names every entry point + # that does this ("join.sh, leave.sh, team.sh, rename.sh, rename-team.sh") + # for exactly this reason: an unvalidated value containing "..", "/", or + # similar can resolve to a config-shaped file outside teams/ entirely. This + # is doctor.sh's first --team-shaped entry point, so it needs the same + # validator every other one already runs through -- not a new check, just + # this file failing to call the existing one. Runs BEFORE the existence + # check below: a value validate.sh rejects should never even reach a + # filesystem lookup. + agmsg_validate_team_name "$FILTER_TEAM" || exit 2 + if [ ! -f "$SKILL_DIR/teams/$FILTER_TEAM/config.json" ]; then + echo "doctor: unknown team: '$FILTER_TEAM'" >&2 + exit 2 + fi +fi -PROJECT="$(agmsg_resolve_project "$PROJECT" "$TYPE")" - -# Whether this type already reports its own per-role runtime status (codex's -# _delivery.sh does, via the embedded delivery-status block above -- one -# "Codex bridge: team/agent ..." line per role). Everything else (currently -# claude-code, opencode) falls through to the default runtime status, which -# is a single project-wide count with no per-role breakdown, so those types -# get the watcher= field built below instead. Detected structurally (does -# the type's plug override the function) rather than hardcoding "codex", so -# a future type with its own per-role reporting is picked up automatically. -TYPE_HAS_ROLE_RUNTIME=0 -TYPE_DELIVERY_PLUG="$SKILL_DIR/scripts/drivers/types/$TYPE/_delivery.sh" -if [ -f "$TYPE_DELIVERY_PLUG" ] && grep -q '^agmsg_delivery_runtime_status()' "$TYPE_DELIVERY_PLUG" 2>/dev/null; then - TYPE_HAS_ROLE_RUNTIME=1 +# Keep only the "\t" lines belonging to FILTER_TEAM. A no-op +# (prints input unchanged) when no --team was given. Needed in two places: +# narrowing a (project, type) pair's own registration rows to just the +# requested team, and (identically) deciding whether that team has ANY +# registration at a candidate pair while building SCOPE below. +_team_filter_lines() { + local input="$1" team="$2" t a + [ -n "$team" ] || { printf '%s' "$input"; return 0; } + while IFS=$'\t' read -r t a; do + [ -z "$t" ] && continue + [ "$t" = "$team" ] && printf '%s\t%s\n' "$t" "$a" + done <<< "$input" + # A while loop's own exit status is whatever its LAST executed command + # left behind -- here, "[ "$t" = "$team" ] && printf ...", whose short- + # circuiting means a NON-matching final input line leaves exit 1, even + # though filtering out a non-match is completely normal. Every caller of + # this function assigns its output via a bare command substitution (no + # `|| true`), so under set -e that leaked 1 aborted the whole script -- + # silently, with no output at all, whenever --team's target sorted before + # some other team on the LAST row of a pair's registrations (identities.sh + # orders by team name, so this depended on which teams happened to share a + # project/type and how their names compared). Found by running --team + # against the real installation, where this ordering wasn't in this + # branch's favor. return 0 makes "ran to completion, matched or not" the + # function's actual contract, matching what every caller already assumes. + return 0 +} + +# --- scope: build the (project, type) pairs to scan -------------------- +# +# One output format regardless of which filters were given: newline-separated +# "projecttype". Everything downstream just iterates this list -- the +# per-pair judgment logic (_doctor_scan_pair) is identical regardless of how +# the list was built. +SCOPE="" + +while IFS= read -r _type; do + [ -n "$_type" ] || continue + if [ -n "$FILTER_PROJECT" ]; then + # --project given: no single resolve call applies without a type, so this + # loops it across every type being scanned (all known types, or just + # FILTER_TYPE), exactly as identities.sh is then used to confirm a real + # registration exists there. (An earlier version tried to match the input + # against agmsg_registered_projects by canonicalizing both sides -- wrong + # on macOS, where agmsg_canonical_path resolves the /var -> /private/var + # symlink but the registry stores whatever raw path join.sh was given, so + # a real registration never matched. agmsg_resolve_project already gets + # this right per type; reusing it here sidesteps reinventing that + # matching a second time.) FILTER_TEAM, if given, scopes both the + # resolution's registry fallback AND which registration counts as a hit. + _resolved="$(agmsg_resolve_project "$FILTER_PROJECT" "$_type" "$FILTER_TEAM")" + _hit="$("$SCRIPT_DIR/identities.sh" "$_resolved" "$_type")" + _hit="$(_team_filter_lines "$_hit" "$FILTER_TEAM")" + if [ -n "$_hit" ]; then + SCOPE="${SCOPE}${_resolved}"$'\t'"${_type}"$'\n' + fi + else + # No --project: every project registered for this type (agmsg_registered_projects's + # own team param does the --team narrowing here, at the source, rather than + # listing everyone's projects and filtering after). sort -u because + # agmsg_registered_projects dedups WITHIN one team's config.json via SQL + # DISTINCT, but concatenates every scanned team's config.json results with + # no cross-file dedup -- a project registered under two different teams (a + # real shape: two teams sharing one workspace) comes back twice when no + # --team narrows it to one file, and without sort -u here that turns into + # the same (project, type) pair scanned and reported twice, with summary + # counts inflated to match. Confirmed by direct inspection of its raw + # output before this fix landed. + while IFS= read -r _proj; do + [ -n "$_proj" ] || continue + SCOPE="${SCOPE}${_proj}"$'\t'"${_type}"$'\n' + done <<< "$(agmsg_registered_projects "$_type" "$FILTER_TEAM" | sort -u)" + fi +done <<< "$(if [ -n "$FILTER_TYPE" ]; then printf '%s\n' "$FILTER_TYPE"; else agmsg_known_types | sort -u; fi)" +unset _type _proj _resolved _hit + +# An empty SCOPE means two different things depending on whether a filter +# narrowed it: an EXPLICIT --project/--type/--team that matched nothing is +# almost certainly a mistake (a typo'd project path, a team that doesn't +# exist) -- exit 2, as before. But no filters at all, on an installation +# that genuinely has zero registrations anywhere, is a VALID whole-install +# scan whose answer happens to be empty -- that is not a usage error, and +# treating it as one meant "diagnose an empty installation" itself failed +# doctor's own exit-code contract. Falls through to the normal report path +# below, which -- with SCOPE empty -- naturally produces a clean "0 team(s), +# 0 registration(s), 0 warning(s)" / "no warnings." / exit 0 with no special +# casing needed there. +if [ -z "$SCOPE" ] && { [ -n "$FILTER_PROJECT" ] || [ -n "$FILTER_TYPE" ] || [ -n "$FILTER_TEAM" ]; }; then + echo "doctor: no registrations match this scope" >&2 + exit 2 fi WARNINGS="" @@ -94,6 +216,11 @@ _warn() { WARNINGS="${WARNINGS}$1"$'\n'; } # --- redaction: consistent pseudonyms, not one-off masking ----------------- # +# One pseudonym table for the WHOLE run, not per (project, type) pair: with +# --all spanning multiple projects, the same team/agent appearing under two +# different projects has to read as the same pseudonym both times, or the +# output stops being cross-referenceable against itself. +# # A fixed team1/agent1 substitution (not a hash) so the same name reads the # same way everywhere it appears in one run -- the #605 reporter hand-redacted # their own report exactly this way (generic team/agent names, home-relative @@ -124,6 +251,22 @@ _redact_agent() { _R_AGENT_K[$n]="$1"; _R_AGENT_V[$n]="agent$((n + 1))" _REDACT_OUT="${_R_AGENT_V[$n]}" } +# Same idea for project paths as team/agent above: one table for the whole +# run, so the same project reads as the same pseudonym in every (project, +# type) block it appears in under --all, not a fresh placeholder each time. +_R_PROJ_K=(); _R_PROJ_V=() +_redact_project() { + [ "$REDACTED" = 1 ] || { _REDACT_OUT="$1"; return 0; } + case "$1" in + "$HOME"*) _REDACT_OUT="~${1#"$HOME"}"; return 0 ;; + esac + local i n=${#_R_PROJ_K[@]} + for ((i = 0; i < n; i++)); do + if [ "${_R_PROJ_K[$i]}" = "$1" ]; then _REDACT_OUT="${_R_PROJ_V[$i]}"; return 0; fi + done + _R_PROJ_K[$n]="$1"; _R_PROJ_V[$n]="" + _REDACT_OUT="${_R_PROJ_V[$n]}" +} # Plain output shows the owner token IN FULL -- #605 was actually resolved by # matching this exact value against a "codex-bridge: resumed thread " # line in a bridge log, and a shortened token can't be matched that way. This @@ -143,18 +286,6 @@ _redact_owner() { printf '...%s' "${1: -6}" fi } -# A path outside $HOME (a shared worktree, /Volumes/..., /Users/Shared/...) -# has no HOME-relative form to fall back to, and showing it raw would be the -# exact leak --redacted exists to prevent -- so that case gets a bare -# placeholder instead of the path. The HOME case keeps the more readable -# "~/..." form. -_redact_project() { - [ "$REDACTED" = 1 ] || { printf '%s' "$1"; return 0; } - case "$1" in - "$HOME"*) printf '~%s' "${1#"$HOME"}" ;; - *) printf '' ;; - esac -} # Literal (not glob, not regex) substring replace. A quoted portion of a # case/parameter-expansion pattern matches literally regardless of what it # contains, so this needs no escaping for team/agent names or paths that @@ -178,7 +309,9 @@ _replace_literal() { # script did not format itself (delivery.sh's own output) -- --redacted's # only promise is "safe to paste", so text quoted wholesale from elsewhere # has to go through the same pseudonym table and $HOME masking as everything -# doctor.sh builds by hand, not get echoed as-is. +# doctor.sh builds by hand, not get echoed as-is. Takes the CURRENT pair's +# resolved project explicitly (not a global) -- under --all this runs once +# per (project, type) block, each with a different project. # # No word boundaries: a team/agent name that also occurs as a substring # elsewhere in the text (e.g. a team named "agmsg" inside a path like @@ -186,15 +319,15 @@ _replace_literal() { # fixed -- the failure direction is over-redaction, not a leak, which is the # side --redacted is supposed to fail on. _redact_text() { - local text="$1" i n + local text="$1" project="$2" i n [ "$REDACTED" = 1 ] || { printf '%s' "$text"; return 0; } text="$(_replace_literal "$text" "$HOME" "~")" # A project outside $HOME survives the substitution above untouched (no # $HOME prefix to catch), and delivery.sh's own output names it directly # (its settings-hooks-file path is under it) -- so the exact resolved path - # is masked here too, the same placeholder _redact_project uses for the - # non-HOME case, not just doctor's own "project:" line. - text="$(_replace_literal "$text" "$PROJECT" "")" + # is masked here too, the same pseudonym _redact_project produces for it. + _redact_project "$project" + text="$(_replace_literal "$text" "$project" "$_REDACT_OUT")" n=${#_R_TEAM_K[@]} for ((i = 0; i < n; i++)); do text="$(_replace_literal "$text" "${_R_TEAM_K[$i]}" "${_R_TEAM_V[$i]}")" @@ -206,142 +339,296 @@ _redact_text() { printf '%s' "$text" } -# --- gather everything before printing anything ---------------------------- -# -# Order matters here for one reason: redacting the embedded delivery-status -# block (below) needs the team/agent pseudonym table already built, and that -# table is only built by walking registrations. So registrations are walked -# first (silently, into REG_LINES) and the mode line is read before anything -# is echoed, even though "registrations" prints after "delivery status" on -# screen. +# --- scan one (project, type) pair, buffer its block ------------------------ # -# Shelled out to the real CLI (not sourced): delivery.sh dispatches on argv at -# file scope, so sourcing it would run that dispatch. Reused verbatim (through -# _redact_text below) -- the type-specific per-role bridge liveness this -# project already has (codex's _delivery.sh) is not worth a second -# implementation here. Trade-off: MODE and the stale-pidfile warnings below -# are parsed out of this human-readable text, so if delivery.sh's wording -# changes, both go silent (no warning, not a wrong one) rather than erroring -# -- a duplicated implementation would drift instead of going quiet, which is -# worse. Flagged here so whoever next changes delivery.sh's status wording -# knows to check. -DELIVERY_STATUS=0 -DELIVERY_OUTPUT="$(bash "$SCRIPT_DIR/delivery.sh" status "$TYPE" "$PROJECT" 2>&1)" || DELIVERY_STATUS=$? -MODE_LINE="$(printf '%s\n' "$DELIVERY_OUTPUT" | head -1)" -MODE="${MODE_LINE#mode: }" - -PAIRS="$("$SCRIPT_DIR/identities.sh" "$PROJECT" "$TYPE")" -PAIR_COUNT="$(printf '%s\n' "$PAIRS" | grep -c . || true)" - -REG_LINES="" -FIRST_TEAM="" FIRST_AGENT="" -if [ "$PAIR_COUNT" -gt 0 ]; then - while IFS=$'\t' read -r team agent; do - [ -z "$team" ] && continue - [ -n "$FIRST_TEAM" ] || { FIRST_TEAM="$team"; FIRST_AGENT="$agent"; } - - _redact_team "$team"; dteam="$_REDACT_OUT" - _redact_agent "$agent"; dagent="$_REDACT_OUT" - owner="$(actas_lock_owner "$team" "$agent")" - - if [ -z "$owner" ]; then - REG_LINES="${REG_LINES}$(printf ' %-22s lock=none' "$dteam/$dagent")"$'\n' - continue - fi +# Buffered into REPORT_BLOCKS rather than printed inline: the summary line +# koit asked for has to come FIRST on screen ("撃った人が最初に見るのはそ +# こ"), but its counts (teams/registrations/warnings) aren't known until +# every pair in the scope has been scanned. Nothing here is large enough for +# buffering to matter -- even the whole install across every team is a +# handful of KB. +REPORT_BLOCKS="" +TOTAL_PAIR_COUNT=0 +# The "watch processes: N alive, M stale pidfiles" line the default runtime +# status emits scans the WHOLE run/ directory, not any one (project, type)'s +# own state -- an installation-wide fact, not a per-pair one. Captured ONCE +# here, independent of the scope being scanned, via `delivery.sh status` with +# no / -- do_status's own comment documents this as its +# no-args path: it skips the project-scoped mode line and just reports the +# global watcher state. This independence matters: an earlier version +# captured it opportunistically from whichever pair's own delivery.sh call +# happened to emit it first, which meant it silently never appeared at all +# on an installation whose registrations are ALL a no-delivery type (skips +# the call) and/or codex (overrides runtime status with its own per-role +# bridge lines instead of this one) -- an install like that would lose +# run/watch.*.pid stale-watcher detection entirely, not just deduplicate it. +# Caught in review; the fix is scanning run/ once, unconditionally, not +# deduplicating a per-pair emission that may never happen. +GLOBAL_WATCH_LINE="$(bash "$SCRIPT_DIR/delivery.sh" status 2>&1 | grep '^watch processes: ' | head -1 || true)" +_doctor_scan_pair() { + local project="$1" type="$2" - if agmsg_instance_alive "$owner"; then - alive_word="alive" - else - alive_word="STALE" - _warn "stale lock: $dteam/$dagent (owner=$(_redact_owner "$owner"))" - fi + # Whether this type already reports its own per-role runtime status + # (codex's _delivery.sh does, via the embedded delivery-status block -- + # one "Codex bridge: team/agent ..." line per role). Everything else + # (currently claude-code, opencode) falls through to the default runtime + # status, which is a single project-wide count with no per-role + # breakdown, so those types get the watcher= field built below instead. + # Detected structurally (does the type's plug override the function) + # rather than hardcoding "codex", so a future type with its own per-role + # reporting is picked up automatically. + local type_has_role_runtime=0 type_plug="$SKILL_DIR/scripts/drivers/types/$type/_delivery.sh" + if [ -f "$type_plug" ] && grep -q '^agmsg_delivery_runtime_status()' "$type_plug" 2>/dev/null; then + type_has_role_runtime=1 + fi - cc_note="" - if agmsg_instance_is_composite "$owner"; then - pid="${owner##*.}" - if [ -f "$RUN_DIR/cc-instance.$pid" ]; then cc_note=" cc-instance=present"; else cc_note=" cc-instance=absent"; fi + # Whether this type has ANY real delivery to ask about. delivery_modes= in + # the type's manifest lists every mode the type can be SET to; a type whose + # list is nothing but "off" (agmsg-app, hermes) has no agmsg-side delivery + # at all -- agmsg-app is the desktop app's own identity, which owns its + # own send/receive UI. Querying delivery.sh status for such a type exits 1 + # by design (there's nothing to report), and this doctor was turning that + # into a WARNING on an otherwise completely healthy installation -- a real + # installation, run once, came back "9 team(s), 56 registration(s), 5 + # warning(s)" purely from this, violating the exit-code contract this + # command promised on day one (0 = nothing to report). Checked via the + # manifest (agmsg_type_get, already used by PR #631 for the same key) so a + # future no-delivery type is picked up the same way automatically, rather + # than by name. + local type_has_delivery=0 _dm_tok + for _dm_tok in $(agmsg_type_get "$type" delivery_modes); do + if [ "$_dm_tok" != "off" ]; then + type_has_delivery=1 + break fi + done + unset _dm_tok - # Per-role watcher liveness -- only for types whose runtime status doesn't - # already break this down per role (see TYPE_HAS_ROLE_RUNTIME above). - # The pidfile watch.sh's SessionStart directive writes is keyed on the - # SAME normalized instance id actas-claim.sh records as the lock owner - # (both go through agmsg_normalize_instance_id on the same session id), - # so the owner token IS the watcher's pidfile name -- no separate lookup - # or correlation needed, and no liveness logic of its own: reuses - # _agmsg_pid_alive_local, the same helper delivery.sh's own default - # runtime status calls. - watcher_note="" - if [ "$TYPE_HAS_ROLE_RUNTIME" -eq 0 ]; then - wpidfile="$RUN_DIR/watch.$owner.pid" - if [ -f "$wpidfile" ]; then - wpid="$(cat "$wpidfile" 2>/dev/null || true)" - if [ -n "$wpid" ] && _agmsg_pid_alive_local "$wpid" 2>/dev/null; then - watcher_note=" watcher=running" - else - watcher_note=" watcher=stale-pidfile" - fi + # Shelled out to the real CLI (not sourced): delivery.sh dispatches on argv + # at file scope, so sourcing it would run that dispatch. Reused verbatim + # (through _redact_text) -- the type-specific per-role bridge liveness + # this project already has (codex's _delivery.sh) is not worth a second + # implementation here. Trade-off: MODE and the stale-pidfile warnings + # below are parsed out of this human-readable text, so if delivery.sh's + # wording changes, both go silent (no warning, not a wrong one) rather + # than erroring -- a duplicated implementation would drift instead of + # going quiet, which is worse. Flagged here so whoever next changes + # delivery.sh's status wording knows to check. + local delivery_status=0 delivery_output="" mode_line="" mode="off" + if [ "$type_has_delivery" -eq 1 ]; then + delivery_output="$(bash "$SCRIPT_DIR/delivery.sh" status "$type" "$project" 2>&1)" || delivery_status=$? + mode_line="$(printf '%s\n' "$delivery_output" | head -1)" + mode="${mode_line#mode: }" + + # This pair's own delivery.sh call may ALSO emit the same global line + # (default runtime status, when this type doesn't override it) -- always + # captured independently above now, so here it's only ever stripped out + # of this pair's own text, never (re-)captured from it. delivery.sh + # always emits "mode: ..." before this line when given a type/project + # (do_status runs agmsg_delivery_status first, unconditionally), so this + # grep -v never filters every line away in practice -- guarded with + # || true anyway rather than leaning on that ordering under set -e. + delivery_output="$(printf '%s\n' "$delivery_output" | grep -v '^watch processes: ' || true)" + fi + + # Tracks whether this pair turned out to have anything worth a full block: + # a warning count taken before/after (any _warn call below flips this, + # without needing every call site to also set a flag), plus "any lock held + # at all" and "delivery has more than a bare idle mode line" below. All + # three false is exactly the shape a healthy, unconfigured project/type has + # -- 27 such groups on a real install were each 6 lines to say nothing, + # which is what made the report unreadable at real scale. + local _warn_count_before + _warn_count_before="$(printf '%s\n' "$WARNINGS" | grep -c . || true)" + + local pairs pair_count reg_lines="" first_team="" first_agent="" _any_owner=0 + pairs="$("$SCRIPT_DIR/identities.sh" "$project" "$type")" + # FILTER_TEAM, when set, narrows the report to that team's own rows -- a + # pair SCOPE already guaranteed has at least one registration for that + # team (see the --project branch above / agmsg_registered_projects's team + # param), so this never empties a pair SCOPE included. + pairs="$(_team_filter_lines "$pairs" "$FILTER_TEAM")" + pair_count="$(printf '%s\n' "$pairs" | grep -c . || true)" + TOTAL_PAIR_COUNT=$((TOTAL_PAIR_COUNT + pair_count)) + + local team agent dteam dagent owner alive_word cc_note pid + local wpidfile wpid watcher_note first_dteam first_dagent + if [ "$pair_count" -gt 0 ]; then + while IFS=$'\t' read -r team agent; do + [ -z "$team" ] && continue + [ -n "$first_team" ] || { first_team="$team"; first_agent="$agent"; } + + _redact_team "$team"; dteam="$_REDACT_OUT" + _redact_agent "$agent"; dagent="$_REDACT_OUT" + owner="$(actas_lock_owner "$team" "$agent")" + + if [ -z "$owner" ]; then + reg_lines="${reg_lines}$(printf ' %-22s lock=none' "$dteam/$dagent")"$'\n' + continue + fi + _any_owner=1 + + if agmsg_instance_alive "$owner"; then + alive_word="alive" else - watcher_note=" watcher=none" - # Only when the lock itself is legitimately live: a stale lock having - # no watcher is unremarkable (already covered by the warning above), - # but an alive lock with no watcher means the role claims exclusivity - # and isn't receiving -- the shape #605 and Alice/Bob both were. - if [ "$alive_word" = "alive" ]; then - _warn "actas lock held but no watcher: $dteam/$dagent (owner=$(_redact_owner "$owner"))" + alive_word="STALE" + _redact_project "$project" + _warn "[$_REDACT_OUT] stale lock: $dteam/$dagent (owner=$(_redact_owner "$owner"))" + fi + + cc_note="" + if agmsg_instance_is_composite "$owner"; then + pid="${owner##*.}" + if [ -f "$RUN_DIR/cc-instance.$pid" ]; then cc_note=" cc-instance=present"; else cc_note=" cc-instance=absent"; fi + fi + + # Per-role watcher liveness -- only for types whose runtime status + # doesn't already break this down per role (see type_has_role_runtime + # above). The pidfile watch.sh's SessionStart directive writes is + # keyed on the SAME normalized instance id actas-claim.sh records as + # the lock owner (both go through agmsg_normalize_instance_id on the + # same session id), so the owner token IS the watcher's pidfile name + # -- no separate lookup or correlation needed, and no liveness logic + # of its own: reuses _agmsg_pid_alive_local, the same helper + # delivery.sh's own default runtime status calls. + watcher_note="" + if [ "$type_has_role_runtime" -eq 0 ]; then + wpidfile="$RUN_DIR/watch.$owner.pid" + if [ -f "$wpidfile" ]; then + wpid="$(cat "$wpidfile" 2>/dev/null || true)" + if [ -n "$wpid" ] && _agmsg_pid_alive_local "$wpid" 2>/dev/null; then + watcher_note=" watcher=running" + else + watcher_note=" watcher=stale-pidfile" + fi + else + watcher_note=" watcher=none" + # Only when the lock itself is legitimately live: a stale lock + # having no watcher is unremarkable (already covered above), but + # an alive lock with no watcher means the role claims exclusivity + # and isn't receiving -- the shape #605 and koit's own example + # both were. + if [ "$alive_word" = "alive" ]; then + _redact_project "$project" + _warn "[$_REDACT_OUT] actas lock held but no watcher: $dteam/$dagent (owner=$(_redact_owner "$owner"))" + fi fi fi - fi - REG_LINES="${REG_LINES}$(printf ' %-22s lock=owner(%s)=%s%s%s' "$dteam/$dagent" "$alive_word" "$(_redact_owner "$owner")" "$cc_note" "$watcher_note")"$'\n' - done <<< "$PAIRS" + reg_lines="${reg_lines}$(printf ' %-22s lock=owner(%s)=%s%s%s' "$dteam/$dagent" "$alive_word" "$(_redact_owner "$owner")" "$cc_note" "$watcher_note")"$'\n' + done <<< "$pairs" - if [ "$PAIR_COUNT" -gt 1 ] && { [ "$MODE" = "turn" ] || [ "$MODE" = "both" ]; }; then - _redact_team "$FIRST_TEAM"; first_dteam="$_REDACT_OUT" - _redact_agent "$FIRST_AGENT"; first_dagent="$_REDACT_OUT" - _warn "$PAIR_COUNT registrations for this (project, type) under turn-mode delivery -- only the first registered ($first_dteam/$first_dagent) receives Stop-hook delivery; the rest are silent under turn" + if [ "$pair_count" -gt 1 ] && { [ "$mode" = "turn" ] || [ "$mode" = "both" ]; }; then + _redact_team "$first_team"; first_dteam="$_REDACT_OUT" + _redact_agent "$first_agent"; first_dagent="$_REDACT_OUT" + _redact_project "$project" + _warn "[$_REDACT_OUT] $pair_count registrations for this (project, type) under turn-mode delivery -- only the first registered ($first_dteam/$first_dagent) receives Stop-hook delivery; the rest are silent under turn" + fi fi -fi -# codex's per-role lines always carry a parenthetical reason (e.g. "stale -# pidfile (pid 123 not running)"); grepping for the "(" excludes the -# claude-code default's aggregate "N stale pidfiles" line, which is handled -# by the numeric check below instead and would otherwise false-positive here -# on its own plural (a literal prefix match of "stale pidfile"). -if printf '%s\n' "$DELIVERY_OUTPUT" | grep -q "stale pidfile ("; then - _warn "watcher/bridge pidfile present but process not running (see delivery status above)" -fi -STALE_COUNT="$(printf '%s\n' "$DELIVERY_OUTPUT" | sed -n 's/.*, \([0-9]*\) stale pidfiles*$/\1/p' | head -1)" -case "$STALE_COUNT" in ''|*[!0-9]*) STALE_COUNT=0 ;; esac -if [ "$STALE_COUNT" -gt 0 ]; then - _warn "watcher/bridge pidfile present but process not running (see delivery status above)" -fi + # codex's per-role lines always carry a parenthetical reason (e.g. "stale + # pidfile (pid 123 not running)") -- a genuine per-(project, type) fact, + # unlike the installation-wide "N stale pidfiles" default-runtime-status + # line (captured independently into GLOBAL_WATCH_LINE above and checked + # once, globally, after the whole scope has been scanned -- see below the + # scan loop). + if printf '%s\n' "$delivery_output" | grep -q "stale pidfile ("; then + _redact_project "$project" + _warn "[$_REDACT_OUT] watcher/bridge pidfile present but process not running (see delivery status above)" + fi -# TYPE is already validated above, so this is not the "unknown type" case -- -# some other failure inside delivery.sh status itself. Surfaced as a warning -# rather than swallowed: showing error text on screen while still reporting -# "no warnings." / exit 0 underneath would be a doctor that lies about its -# own read. -if [ "$DELIVERY_STATUS" -ne 0 ]; then - _warn "delivery.sh status exited $DELIVERY_STATUS (see delivery status above)" -fi + # type is already validated (or came from the registry) before this is + # ever called, so this is not the "unknown type" case -- some other + # failure inside delivery.sh status itself. Surfaced as a warning rather + # than swallowed: showing error text on screen while still reporting + # "no warnings." / exit 0 underneath would be a doctor that lies about + # its own read. type_has_delivery gates this call happening at all now, so + # this can only fire for a type that DOES have delivery to query. + if [ "$delivery_status" -ne 0 ]; then + _redact_project "$project" + _warn "[$_REDACT_OUT] delivery.sh status exited $delivery_status (see delivery status above)" + fi -# --- now print, in screen order ------------------------------------------- -DISPLAY_PROJECT="$(_redact_project "$PROJECT")" -echo "project: $DISPLAY_PROJECT" -echo "type: $TYPE" -echo + # "Nothing to report": no lock held (by anyone), no warning raised while + # scanning this pair, and delivery has nothing beyond a bare idle mode + # line (no type_has_delivery at all, or mode=off with delivery_output -- + # after the global watch-processes line above was stripped out of it -- + # amounting to just that one "mode: off" line, no hooks/bridge detail + # worth a look). On a real installation this was 27 of the report's + # groups, each spending 6 lines to say "nothing here" -- unreadable at + # real scale even once the exit-code bug above stops making them warnings. + local _warn_count_after + _warn_count_after="$(printf '%s\n' "$WARNINGS" | grep -c . || true)" + local _delivery_line_count + _delivery_line_count="$(printf '%s\n' "$delivery_output" | grep -c . || true)" + local _boring=0 + if [ "$_any_owner" -eq 0 ] \ + && [ "$_warn_count_after" -eq "$_warn_count_before" ] \ + && [ "$_delivery_line_count" -le 1 ] && [ "$mode" = "off" ]; then + _boring=1 + fi -echo "$(_redact_text "$DELIVERY_OUTPUT")" -echo + _redact_project "$project" + if [ "$_boring" -eq 1 ]; then + local _noun="registrations" + [ "$pair_count" -eq 1 ] && _noun="registration" + REPORT_BLOCKS="${REPORT_BLOCKS}$_REDACT_OUT [$type] $pair_count $_noun, nothing to report"$'\n' + else + REPORT_BLOCKS="${REPORT_BLOCKS}project: $_REDACT_OUT"$'\n' + REPORT_BLOCKS="${REPORT_BLOCKS}type: $type"$'\n\n' + REPORT_BLOCKS="${REPORT_BLOCKS}$(_redact_text "$delivery_output" "$project")"$'\n\n' + REPORT_BLOCKS="${REPORT_BLOCKS}registrations ($pair_count):"$'\n' + if [ "$pair_count" -eq 0 ]; then + REPORT_BLOCKS="${REPORT_BLOCKS} (none for this project/type)"$'\n' + else + REPORT_BLOCKS="${REPORT_BLOCKS}${reg_lines}" + fi + REPORT_BLOCKS="${REPORT_BLOCKS}"$'\n' + fi +} -echo "registrations ($PAIR_COUNT):" -if [ "$PAIR_COUNT" -eq 0 ]; then - echo " (none for this project/type)" -else - printf '%s' "$REG_LINES" +# --- run the whole scope, then print summary -> blocks -> warnings -------- +# +# Team count for the summary line is built alongside the scan (every +# distinct team name seen across the scope's registrations, not the count +# of (project, type) pairs) rather than a second pass over SCOPE -- reuses +# the exact identities.sh call _doctor_scan_pair already makes for the same +# pair, instead of querying it twice. +DISTINCT_TEAMS="" +while IFS=$'\t' read -r _proj _type; do + [ -z "$_proj" ] && continue + _doctor_scan_pair "$_proj" "$_type" + while IFS=$'\t' read -r _team _agent; do + [ -z "$_team" ] && continue + case $'\n'"$DISTINCT_TEAMS"$'\n' in + *$'\n'"$_team"$'\n'*) ;; + *) DISTINCT_TEAMS="${DISTINCT_TEAMS}${_team}"$'\n' ;; + esac + done <<< "$(_team_filter_lines "$("$SCRIPT_DIR/identities.sh" "$_proj" "$_type")" "$FILTER_TEAM")" +done <<< "$SCOPE" +TEAM_COUNT="$(printf '%s\n' "$DISTINCT_TEAMS" | grep -c . || true)" + +# GLOBAL_WATCH_LINE's own stale-pidfile count is an installation-wide fact +# (see where it's captured in _doctor_scan_pair) -- checked here, ONCE, for +# the whole run, rather than once per pair scanned. Reuses the exact same +# text the per-pair codex check above parses a different (per-role) line +# from; this just applies that same parsing to the one line that is global. +if [ -n "$GLOBAL_WATCH_LINE" ]; then + GLOBAL_STALE_COUNT="$(printf '%s\n' "$GLOBAL_WATCH_LINE" | sed -n 's/.*, \([0-9]*\) stale pidfiles*$/\1/p')" + case "$GLOBAL_STALE_COUNT" in ''|*[!0-9]*) GLOBAL_STALE_COUNT=0 ;; esac + if [ "$GLOBAL_STALE_COUNT" -gt 0 ]; then + _warn "watcher pidfile present but process not running, installation-wide (see the 'watch processes' line above)" + fi fi +WARN_COUNT="$(printf '%s\n' "$WARNINGS" | grep -c . || true)" + +echo "$TEAM_COUNT team(s), $TOTAL_PAIR_COUNT registration(s), $WARN_COUNT warning(s)" echo +if [ -n "$GLOBAL_WATCH_LINE" ]; then + echo "$GLOBAL_WATCH_LINE" + echo +fi +printf '%s' "$REPORT_BLOCKS" if [ -n "$WARNINGS" ]; then echo "warnings:" diff --git a/scripts/lib/validate.sh b/scripts/lib/validate.sh index 8085fdb92..5c0157296 100644 --- a/scripts/lib/validate.sh +++ b/scripts/lib/validate.sh @@ -5,7 +5,7 @@ # (teams//config.json). A name containing "/", "\", or equal to "." / ".." # can escape teams/ and create/read/move/delete files outside the agmsg state # tree (#140). Validate at every entry point that turns a team name into a path: -# join.sh, leave.sh, team.sh, rename.sh, rename-team.sh. +# join.sh, leave.sh, team.sh, rename.sh, rename-team.sh, doctor.sh (--team). # # Team names are intentionally allowed to be arbitrary UTF-8 (e.g. Japanese team # names like "testチーム" exist in the wild), so this is a deny-list of diff --git a/tests/test_doctor.bats b/tests/test_doctor.bats index 03cedb583..8464f541e 100644 --- a/tests/test_doctor.bats +++ b/tests/test_doctor.bats @@ -14,58 +14,240 @@ teardown() { rm -rf "$PROJ" } -# --- usage-error contract (exit 2), separate from the "found a problem" -# contract (exit 1) above -- these used to collapse into bash's own -# ${1:?...} exit 1 on a missing argument, which is indistinguishable -# from a warning to anything scripting against the exit code. ---------- +# --- default scope: no filters = the whole installation ------------------- +# (koit's round-2 call: other doctor-style commands -- claude/codex/brew/ +# flutter -- all default to "everything", so this one now does too. The +# old -required positional form is dropped, not kept for +# compatibility -- see doctor.sh's header comment.) ---------------------- + +@test "doctor: no arguments defaults to the whole installation, not a usage error" { + local other_proj="$(mktemp -d)" + bash "$SCRIPTS/join.sh" other bob claude-code "$other_proj" >/dev/null + + run bash "$SCRIPTS/doctor.sh" + [ "$status" -eq 0 ] + # Both projects are represented (as their raw paths -- both pairs here have + # nothing held and collapse to one line each, which shows the project path + # but not the team/agent names; see the "nothing to report" tests below). + [[ "$output" == *"$PROJ"* ]] + [[ "$output" == *"$other_proj"* ]] + + rm -rf "$other_proj" +} + +@test "doctor: a genuinely empty installation (no filters, zero registrations anywhere) is a clean 0/0/0, not a usage error" { + # setup() always creates one registration; leave it so this test starts + # from a truly empty installation (leave.sh also removes the now-empty + # team). An explicit --project/--type/--team matching nothing is still a + # usage error (exit 2, unchanged) -- only the no-filter default-scope case + # changes, since a whole-install scan that finds nothing is a VALID result, + # not a mistake the way a typo'd explicit filter would be. + bash "$SCRIPTS/leave.sh" team alice >/dev/null -@test "doctor: no arguments is a usage error (exit 2), not bash's own exit 1" { run bash "$SCRIPTS/doctor.sh" + [ "$status" -eq 0 ] + [[ "$output" == "0 team(s), 0 registration(s), 0 warning(s)"* ]] + [[ "$output" == *"no warnings."* ]] +} + +@test "doctor: an explicit --project matching nothing is still a usage error (exit 2), unlike the no-filter empty case" { + run bash "$SCRIPTS/doctor.sh" --project "$(mktemp -d)" [ "$status" -eq 2 ] - [[ "$output" == *"Usage: doctor.sh"* ]] + [[ "$output" == *"no registrations match this scope"* ]] } -@test "doctor: a missing type argument is a usage error (exit 2)" { - run bash "$SCRIPTS/doctor.sh" "$PROJ" +@test "doctor: a positional argument (the dropped form) is a usage error, not silently accepted" { + run bash "$SCRIPTS/doctor.sh" "$PROJ" claude-code [ "$status" -eq 2 ] - [[ "$output" == *"Usage: doctor.sh"* ]] + [[ "$output" == *"unexpected argument"* ]] +} + +@test "doctor: --project alone shows every type registered for that project" { + bash "$SCRIPTS/join.sh" team alice codex "$PROJ" >/dev/null + + run bash "$SCRIPTS/doctor.sh" --project "$PROJ" + [ "$status" -eq 0 ] + # Format-agnostic (full block or the "nothing to report" one-liner -- + # claude-code has nothing held so collapses, codex's own bridge status + # line means it never does) -- both types are represented either way. + [[ "$output" == *"claude-code"* ]] + [[ "$output" == *"codex"* ]] } @test "doctor: bare --help exits 0 and prints usage, even with no other arguments" { - # ${1:?...} alone would consume "--help" as PROJECT, then die on the - # missing TYPE with status 1 and never reach a help branch. run bash "$SCRIPTS/doctor.sh" --help [ "$status" -eq 0 ] [[ "$output" == *"Usage: doctor.sh"* ]] } @test "doctor: an unknown option is a usage error (exit 2)" { - run bash "$SCRIPTS/doctor.sh" "$PROJ" claude-code --nonsense + run bash "$SCRIPTS/doctor.sh" --project "$PROJ" --type claude-code --nonsense [ "$status" -eq 2 ] [[ "$output" == *"unknown option"* ]] } -@test "doctor: an unknown agent type is a usage error (exit 2), not a silent clean report" { - run bash "$SCRIPTS/doctor.sh" "$PROJ" not-a-real-type +@test "doctor: an unknown --type is a usage error (exit 2), not a silent clean report" { + run bash "$SCRIPTS/doctor.sh" --project "$PROJ" --type not-a-real-type [ "$status" -eq 2 ] [[ "$output" == *"unknown agent type"* ]] # Must not fall through to delivery.sh/identities.sh and report clean. [[ "$output" != *"no warnings."* ]] } +@test "doctor: an unknown --team is a usage error (exit 2)" { + run bash "$SCRIPTS/doctor.sh" --team not-a-real-team + [ "$status" -eq 2 ] + [[ "$output" == *"unknown team"* ]] +} + +@test "doctor: --team is rejected as a path-traversal attempt even when it resolves to a real config-shaped file outside teams/" { + # --team becomes a path segment (teams/$FILTER_TEAM/config.json). Without + # running it through the existing agmsg_validate_team_name first, a value + # containing ".." could resolve OUTSIDE teams/ entirely -- and if a + # config-shaped file happens to exist there, the plain existence check + # this file used before would have accepted it. Proves the rejection is + # the validator firing, not an incidental "file doesn't exist": the + # traversal target is made to exist first. + mkdir -p "$TEST_SKILL_DIR/evil" + printf '{"name": "evil", "agents": {}}' > "$TEST_SKILL_DIR/evil/config.json" + + run bash "$SCRIPTS/doctor.sh" --team "../evil" + [ "$status" -eq 2 ] + [[ "$output" == *"path traversal"* ]] +} + +# --- BLOCKING fix: a type with no real delivery (delivery_modes is nothing +# but "off" -- agmsg-app is the desktop app's own identity, which owns +# its own send/receive UI) must never be queried against delivery.sh, and +# must never turn into a warning. Running the pre-fix version against a +# healthy real installation returned "9 team(s), 56 registration(s), 5 +# warning(s)" purely from this -- an exit-code-contract violation caught +# by koit running doctor against real data, not by any of these fixtures, +# which is exactly why real-data verification was asked for. ----------- + +@test "doctor: a type with delivery_modes=off only is never queried against delivery.sh and never warns" { + bash "$SCRIPTS/join.sh" team carol agmsg-app "$PROJ" >/dev/null + + run bash "$SCRIPTS/doctor.sh" --project "$PROJ" --type agmsg-app + [ "$status" -eq 0 ] + [[ "$output" == *"no warnings."* ]] + [[ "$output" != *"delivery.sh status exited"* ]] +} + +@test "doctor: an installation with only no-delivery-type registrations is entirely clean (exit 0, 0 warnings)" { + local app_proj="$(mktemp -d)" + bash "$SCRIPTS/join.sh" team you agmsg-app "$app_proj" >/dev/null + + run bash "$SCRIPTS/doctor.sh" --project "$app_proj" + [ "$status" -eq 0 ] + [[ "$output" == "1 team(s), 1 registration(s), 0 warning(s)"* ]] + [[ "$output" == *"no warnings."* ]] + + rm -rf "$app_proj" +} + +# --- the "watch processes: N alive, M stale pidfiles" line default runtime +# status emits scans the WHOLE run/ directory -- an installation-wide +# fact, not a (project, type) fact. Printing it inside every group that +# uses default runtime status repeats the identical line once per group; +# tl2 flagged this as duplication on the same real-installation run. --- + +@test "doctor: the install-wide 'watch processes' line appears once, not once per group" { + # The line only appears when run/ exists (default runtime status guards + # on it); both pairs below use claude-code, which has real delivery, so + # each independently calls delivery.sh status and would each emit its own + # copy of this line pre-fix. + mkdir -p "$TEST_SKILL_DIR/run" + local other_proj="$(mktemp -d)" + bash "$SCRIPTS/join.sh" other bob claude-code "$other_proj" >/dev/null + + run bash "$SCRIPTS/doctor.sh" + [ "$status" -eq 0 ] + local line_count + line_count="$(printf '%s\n' "$output" | grep -c '^watch processes: ')" + [ "$line_count" -eq 1 ] + + rm -rf "$other_proj" +} + +@test "doctor: a stale watch pidfile is still detected when every registration is codex or a no-delivery type" { + # codex overrides runtime status with its own per-role bridge lines instead + # of the default "watch processes: ..." line; a no-delivery type skips + # calling delivery.sh at all. An earlier version captured the global line + # opportunistically from whichever pair's own delivery.sh call happened to + # emit it -- on an installation whose registrations are ALL one of these + # two shapes, that line (and the stale-pidfile warning derived from it) + # never appeared AT ALL, not just once instead of per-group. Regression for + # the fix: capture it independently, once, regardless of what's in scope. + bash "$SCRIPTS/leave.sh" team alice >/dev/null + bash "$SCRIPTS/join.sh" team alice codex "$PROJ" >/dev/null + mkdir -p "$TEST_SKILL_DIR/run" + ( exit 0 ) & local deadpid=$! + wait "$deadpid" 2>/dev/null || true + printf '%s\n' "$deadpid" > "$TEST_SKILL_DIR/run/watch.faketoken.pid" + + run bash "$SCRIPTS/doctor.sh" + [ "$status" -eq 1 ] + [[ "$output" == *"watch processes: 0 alive, 1 stale pidfiles"* ]] + [[ "$output" == *"watcher pidfile present but process not running, installation-wide"* ]] +} + @test "doctor: exits 0 and reports no warnings when nothing is registered as locked" { - run bash "$SCRIPTS/doctor.sh" "$PROJ" claude-code + run bash "$SCRIPTS/doctor.sh" --project "$PROJ" --type claude-code [ "$status" -eq 0 ] [[ "$output" == *"no warnings."* ]] - [[ "$output" == *"team/alice"* ]] - [[ "$output" == *"lock=none"* ]] + # Nothing held, nothing configured -- collapses to the one-line "nothing + # to report" form rather than a 6-line block naming lock=none per row. + [[ "$output" == *"$PROJ [claude-code] 1 registration, nothing to report"* ]] +} + +# --- readability: a (project, type) pair with nothing held, no warnings, and +# no delivery worth mentioning collapses to one line instead of a full +# block. A real installation had 27 such groups, each spending 6 lines to +# say "nothing here" -- unreadable at real scale even before the +# BLOCKING exit-code fix below stopped some of them being warnings. ------ + +@test "doctor: a boring (project, type) pair -- no lock, no warning, mode off -- collapses to one line" { + run bash "$SCRIPTS/doctor.sh" --project "$PROJ" --type claude-code + [ "$status" -eq 0 ] + [[ "$output" == *"nothing to report"* ]] + [[ "$output" != *"lock=none"* ]] + [[ "$output" != *"project: $PROJ"* ]] +} + +@test "doctor: a boring pair with more than one registration still collapses, with the plural noun and correct count" { + bash "$SCRIPTS/join.sh" team bob claude-code "$PROJ" >/dev/null + + run bash "$SCRIPTS/doctor.sh" --project "$PROJ" --type claude-code + [ "$status" -eq 0 ] + [[ "$output" == *"$PROJ [claude-code] 2 registrations, nothing to report"* ]] +} + +@test "doctor: the collapsed one-line form is still redacted under --redacted" { + case "$PROJ" in + "$HOME"*) skip "fixture \$PROJ landed under \$HOME this run; this test needs it outside" ;; + esac + + run bash "$SCRIPTS/doctor.sh" --project "$PROJ" --type claude-code --redacted + [ "$status" -eq 0 ] + [[ "$output" == *" [claude-code] 1 registration, nothing to report"* ]] + [[ "$output" != *"$PROJ"* ]] +} + +@test "doctor: an explicitly configured mode (not off) is never collapsed, even with nothing held" { + bash "$SCRIPTS/delivery.sh" set monitor claude-code "$PROJ" >/dev/null + + run bash "$SCRIPTS/doctor.sh" --project "$PROJ" --type claude-code + [ "$status" -eq 0 ] + [[ "$output" != *"nothing to report"* ]] + [[ "$output" == *"mode: monitor"* ]] } @test "doctor: exits non-zero and names a stale lock (composite owner, dead pid, no cc-instance)" { mkdir -p "$TEST_SKILL_DIR/run" printf '%s\n' "deadtoken.999999" > "$TEST_SKILL_DIR/run/actas.team__alice.session" - run bash "$SCRIPTS/doctor.sh" "$PROJ" claude-code + run bash "$SCRIPTS/doctor.sh" --project "$PROJ" --type claude-code [ "$status" -eq 1 ] [[ "$output" == *"stale lock: team/alice"* ]] [[ "$output" == *"cc-instance=absent"* ]] @@ -80,7 +262,7 @@ teardown() { # the lock owner -- see doctor.sh's comment on TYPE_HAS_ROLE_RUNTIME. printf '%s\n' "$$" > "$TEST_SKILL_DIR/run/watch.$owner.pid" - run bash "$SCRIPTS/doctor.sh" "$PROJ" claude-code + run bash "$SCRIPTS/doctor.sh" --project "$PROJ" --type claude-code [ "$status" -eq 0 ] # Plain output shows the owner token in FULL -- shortening only applies # under --redacted (see doctor.sh's comment on _redact_owner: #605 was @@ -98,7 +280,7 @@ teardown() { # watching for it -- exclusivity claimed, nothing receiving. #605's report # was exactly this shape. - run bash "$SCRIPTS/doctor.sh" "$PROJ" claude-code + run bash "$SCRIPTS/doctor.sh" --project "$PROJ" --type claude-code [ "$status" -eq 1 ] [[ "$output" == *"lock=owner(alive)=$owner cc-instance=present watcher=none"* ]] [[ "$output" == *"actas lock held but no watcher: team/alice"* ]] @@ -108,7 +290,7 @@ teardown() { mkdir -p "$TEST_SKILL_DIR/run" printf '%s\n' "deadtoken.999999" > "$TEST_SKILL_DIR/run/actas.team__alice.session" - run bash "$SCRIPTS/doctor.sh" "$PROJ" claude-code + run bash "$SCRIPTS/doctor.sh" --project "$PROJ" --type claude-code [ "$status" -eq 1 ] [[ "$output" == *"stale lock: team/alice"* ]] [[ "$output" != *"actas lock held but no watcher"* ]] @@ -119,7 +301,7 @@ teardown() { mkdir -p "$TEST_SKILL_DIR/run" printf '%s\n' "livetoken.$$" > "$TEST_SKILL_DIR/run/actas.team__bob.session" - run bash "$SCRIPTS/doctor.sh" "$PROJ" codex + run bash "$SCRIPTS/doctor.sh" --project "$PROJ" --type codex [[ "$output" != *"watcher="* ]] } @@ -130,7 +312,7 @@ teardown() { mkdir -p "$TEST_SKILL_DIR/run" printf '%s\n' "deadtoken.999999" > "$TEST_SKILL_DIR/run/actas.team__alice.session" - run bash "$SCRIPTS/doctor.sh" "$home_proj" claude-code --redacted + run bash "$SCRIPTS/doctor.sh" --project "$home_proj" --type claude-code --redacted [ "$status" -eq 1 ] [[ "$output" == *"project: ~/redact-me"* ]] [[ "$output" != *"team/alice"* ]] @@ -150,9 +332,18 @@ teardown() { case "$PROJ" in "$HOME"*) skip "fixture \$PROJ landed under \$HOME this run; this test needs it outside" ;; esac + # A stale lock keeps this pair out of the "nothing to report" collapse (see + # the boring-pair tests below), so the FULL block -- and its "project: " + # line -- is what's actually exercised here. + mkdir -p "$TEST_SKILL_DIR/run" + printf '%s\n' "deadtoken.999999" > "$TEST_SKILL_DIR/run/actas.team__alice.session" - run bash "$SCRIPTS/doctor.sh" "$PROJ" claude-code --redacted - [[ "$output" == *"project: "* ]] + run bash "$SCRIPTS/doctor.sh" --project "$PROJ" --type claude-code --redacted + # Numbered (, not a fixed ) since the default whole- + # install scope needs distinct projects to read as distinct pseudonyms + # within one run; a single-scope invocation just always lands on the + # first number. + [[ "$output" == *"project: "* ]] [[ "$output" != *"$PROJ"* ]] } @@ -173,19 +364,113 @@ teardown() { local owner="459d8198-3fcf-4c9e-a4ff-5f8fbd18c802.$deadpid" printf '%s\n' "$owner" > "$TEST_SKILL_DIR/run/actas.team__alice.session" - run bash "$SCRIPTS/doctor.sh" "$PROJ" claude-code + run bash "$SCRIPTS/doctor.sh" --project "$PROJ" --type claude-code [[ "$output" == *"lock=owner(STALE)=$owner"* ]] - run bash "$SCRIPTS/doctor.sh" "$PROJ" claude-code --redacted + run bash "$SCRIPTS/doctor.sh" --project "$PROJ" --type claude-code --redacted [[ "$output" == *"lock=owner(STALE)=...$deadpid "* ]] [[ "$output" != *"$owner"* ]] } +# --- scope selection: --project / --team narrow the default whole-install +# scope; the underlying judgment logic is unchanged either way --------- + +@test "doctor: --project narrows the report to just that project, even though others are registered" { + local other_proj="$(mktemp -d)" + bash "$SCRIPTS/join.sh" other bob claude-code "$other_proj" >/dev/null + + run bash "$SCRIPTS/doctor.sh" --project "$PROJ" + [ "$status" -eq 0 ] + [[ "$output" == *"$PROJ"* ]] + [[ "$output" != *"$other_proj"* ]] + + rm -rf "$other_proj" +} + +@test "doctor: --team narrows the report to just that team's own rows, even at a project/type another team also shares" { + bash "$SCRIPTS/join.sh" other bob claude-code "$PROJ" >/dev/null + # A live, healthy lock on alice's role (no warning) keeps this pair out of + # the "nothing to report" collapse, so the per-registration rows -- what + # --team is actually narrowing -- are visible to assert on. + mkdir -p "$TEST_SKILL_DIR/run" + local owner="livetoken.$$" + printf '%s\n' "$owner" > "$TEST_SKILL_DIR/run/actas.team__alice.session" + printf '%s\n' "$owner" > "$TEST_SKILL_DIR/run/cc-instance.$$" + printf '%s\n' "$$" > "$TEST_SKILL_DIR/run/watch.$owner.pid" + + run bash "$SCRIPTS/doctor.sh" --team team + [ "$status" -eq 0 ] + [[ "$output" == "1 team(s),"* ]] + [[ "$output" == *"team/alice"* ]] + [[ "$output" != *"other/bob"* ]] +} + +@test "doctor: --team still works when the LAST identities.sh row for a pair belongs to a DIFFERENT team" { + # identities.sh orders rows by team name; "other" sorts before "team", so + # filtering --team other means the last row _team_filter_lines reads for + # this pair is "team"'s -- a non-match. Its filtering loop's own exit + # status is that non-match's (short-circuited "&&") failure, which, with + # no caller guarding the assignment, aborted the whole script under set -e + # -- silently, with zero output. Found by running --team against the real + # installation, where alphabetical luck went the other way from the test + # above. This is the mirror-image fixture: same two registrations, the + # OTHER of the two teams filtered for. + bash "$SCRIPTS/join.sh" other bob claude-code "$PROJ" >/dev/null + + run bash "$SCRIPTS/doctor.sh" --team other + [ "$status" -eq 0 ] + # Confirms both "didn't crash" (the actual bug: silent exit 1, zero output) + # and "actually filtered" (1 registration -- bob's -- not the 2 that exist + # for this pair total). + [[ "$output" == "1 team(s), 1 registration(s), 0 warning(s)"* ]] +} + +@test "doctor: the default whole-install scope does not double-count a (project, type) registered under two different teams" { + # agmsg_registered_projects dedups within one team's config.json but + # concatenates every team's file with no cross-file dedup -- a second team + # registered in the SAME project/type this test's $PROJ already has (team/ + # alice, from setup()) is exactly the shape that came back twice before + # scope-building added its own sort -u. + bash "$SCRIPTS/join.sh" other bob claude-code "$PROJ" >/dev/null + + run bash "$SCRIPTS/doctor.sh" + [ "$status" -eq 0 ] + [[ "$output" == "2 team(s), 2 registration(s), 0 warning(s)"* ]] + # Exactly one project/type entry, not two -- both registrations are + # lock=none so this collapses to the one-line "nothing to report" form, + # which itself carries the correct (deduped) count: "2 registrations", + # not two separate "1 registration" lines. + [[ "$output" == *"$PROJ [claude-code] 2 registrations, nothing to report"* ]] + local line_count + line_count="$(printf '%s\n' "$output" | grep -c 'nothing to report')" + [ "$line_count" -eq 1 ] +} + +@test "doctor: the default whole-install scope with --redacted masks paths from multiple projects with consistent, distinct pseudonyms" { + local other_proj="$(mktemp -d)" + bash "$SCRIPTS/join.sh" other bob claude-code "$other_proj" >/dev/null + + run bash "$SCRIPTS/doctor.sh" --redacted + [ "$status" -eq 0 ] + # Both real paths are gone... + [[ "$output" != *"$PROJ"* ]] + [[ "$output" != *"$other_proj"* ]] + # ...replaced by two DISTINCT numbered placeholders, not one shared one -- + # a single fixed would make two different projects indistinguishable. + [[ "$output" == *""* ]] + [[ "$output" == *""* ]] + # Real team/agent names are also gone. + [[ "$output" != *"team/alice"* ]] + [[ "$output" != *"other/bob"* ]] + + rm -rf "$other_proj" +} + @test "doctor: warns when more than one registration exists under turn-mode delivery" { bash "$SCRIPTS/join.sh" team bob claude-code "$PROJ" >/dev/null bash "$SCRIPTS/delivery.sh" set turn claude-code "$PROJ" >/dev/null - run bash "$SCRIPTS/doctor.sh" "$PROJ" claude-code + run bash "$SCRIPTS/doctor.sh" --project "$PROJ" --type claude-code [ "$status" -eq 1 ] [[ "$output" == *"registrations for this (project, type) under turn-mode delivery"* ]] } @@ -194,7 +479,7 @@ teardown() { bash "$SCRIPTS/join.sh" team bob claude-code "$PROJ" >/dev/null bash "$SCRIPTS/delivery.sh" set monitor claude-code "$PROJ" >/dev/null - run bash "$SCRIPTS/doctor.sh" "$PROJ" claude-code + run bash "$SCRIPTS/doctor.sh" --project "$PROJ" --type claude-code [ "$status" -eq 0 ] [[ "$output" != *"under turn-mode delivery"* ]] } @@ -218,7 +503,7 @@ teardown() { echo "type=codex" } > "$TEST_SKILL_DIR/run/codex-bridge.agmsg.advisor.meta" - run bash "$SCRIPTS/doctor.sh" "$home_proj" codex --redacted + run bash "$SCRIPTS/doctor.sh" --project "$home_proj" --type codex --redacted [ "$status" -eq 0 ] [[ "$output" == *"Codex bridge: team1/agent1 alive"* ]] [[ "$output" != *"agmsg/advisor"* ]]