From 706bb281da07ff9dcd3eb47de46aed4050bd6b1a Mon Sep 17 00:00:00 2001 From: fujibee Date: Fri, 11 Sep 2026 18:38:43 -0700 Subject: [PATCH 1/2] feat(team): join placement records on canonical (kind, instance, pane), not raw ref (#1144) Record-only layer of the #1144 detector: two seats' records collide only when both resolve to the same canonical locator. A ref with no instance component to resolve (every herdr ref today; a legacy bare tmux %N/@N) is reported as unscoped_record instead of being joined by raw string equality, since record-only evidence cannot tell such refs apart across terminal instances (measured for herdr in #1155: two live instances answered the same bare pane id). Every empty-return path in the walk now says why it is empty: a team whose agents could not be enumerated, a path-resolution failure, an unreadable placement record, and an empty ref field each surface as a named coverage diagnostic with count and target, rather than a silent continue. collisions: none means the walk completed and found nothing; collisions: none_observed plus coverage: partial means something could not be read; collisions: not_attempted means there was no teams/ directory to walk. Each failure branch has a negative-control test that fails when its guard is removed. Drops the old resident_agent: absent check (terminal_pane_state / terminal_team_input_ready): that touches a live pane, which belongs to the actual-location layer, not this one. scripts/lib/placement-actual-location.sh adds that layer's classifier against a census snapshot, matching #1155's own row shapes, but it is not wired here yet -- it needs a real census (#1155 is still landing) and must not be exercised end-to-end against a stub. --- README.md | 2 +- scripts/lib/placement-actual-location.sh | 53 ++++ scripts/placement-collisions.sh | 297 +++++++++++++++------- tests/test_placement_actual_location.bats | 70 +++++ tests/test_placement_collisions.bats | 223 +++++++++++----- 5 files changed, 489 insertions(+), 156 deletions(-) create mode 100644 scripts/lib/placement-actual-location.sh create mode 100644 tests/test_placement_actual_location.bats diff --git a/README.md b/README.md index 2dfc17ca8..aed9141d1 100644 --- a/README.md +++ b/README.md @@ -344,7 +344,7 @@ See [docs/opencode.md](docs/opencode.md) for full setup instructions. `team.sh` combines the roster with terminal placement, activity, delivery mode, and identity consistency. Verified identity is collapsed to `identity=ok`; mismatches and values that could not be observed are expanded with their evidence. `--json` emits every field for every registration. The repair flags report each action per identity cell as `changed`, `skipped`, `failed`, or (for a session name that could not be read back) `poked_unverified`. They are two different kinds of act: `--fix-pane-names` repairs the pane label and agent key through the terminal's own API and never types into a session; `--rename-sessions` repairs the CLI session name by typing the type's rename command (`/rename -` for Claude Code, whatever the type's manifest declares otherwise) into the pane, and only after positively identifying a ready process there. `--fix` does both, unconditionally, including the keystroke. Pane liveness will join this view when the pane-state contract lands; until then the unavailable column is omitted rather than filled with `unknown`. -`placement-collisions.sh` is a separate, read-only installation-wide report. It lists refs claimed by different agent names and reports `resident_agent: absent` only when the terminal proves that the pane exists but has no resident agent. It never repairs or removes a record; keeping this fleet observation outside `team.sh` prevents an operator-level scan from becoming part of a seat's repair path. +`placement-collisions.sh` is a separate, read-only installation-wide report. It never repairs or removes a record; keeping this fleet observation outside `team.sh` prevents an operator-level scan from becoming part of a seat's repair path. Today it reports only the record-only layer: two DIFFERENT seats' records resolved to the same canonical (kind, instance, pane) locator, entirely from records on disk — no terminal is ever asked anything. A ref with no instance component to resolve (every herdr ref today; a legacy bare tmux `%N`/`@N`) is not joined by raw string equality; it is listed under `unscoped_records` instead, since record-only evidence cannot tell such refs apart across terminal instances. `collisions: none` means the walk completed and found nothing; `collisions: none_observed` paired with `coverage: partial` means something along the way (a team config, a placement record, an empty ref) could not be read, so the empty answer is not a proven one; `collisions: not_attempted` means there was no `teams/` directory to walk at all. An actual-location layer — matching a seat's own record against where a live census actually observes it — is designed but not yet wired here; see the script's header. Terminal identity has a different number of observable names on each backend. Herdr exposes three independent values: the visible pane label, its internal agent key, and the CLI session name. tmux exposes two: the `@agmsg_agent` pane option is the internal key, while the CLI owns `pane_title`, so there is no independent pane-label field after the CLI starts. `team.sh` reports that tmux field as `n/a` rather than treating an unavailable concept as a mismatch. diff --git a/scripts/lib/placement-actual-location.sh b/scripts/lib/placement-actual-location.sh new file mode 100644 index 000000000..3c591280b --- /dev/null +++ b/scripts/lib/placement-actual-location.sh @@ -0,0 +1,53 @@ +# Actual-location layer of the #1144 detector (interface only). +# +# NOT WIRED from placement-collisions.sh yet, and deliberately so: this layer +# needs a live census (#1155's agmsg_terminal_enumerate, still landing) and +# this function performs no enumeration of its own -- it only classifies a +# SEAT's own claimed locator against a census snapshot the CALLER already +# collected. Wiring it means the caller takes one `agmsg_terminal_enumerate` +# snapshot, then classifies every seat's own record against that SAME +# snapshot -- not a fresh call per seat, which would let the fleet change +# mid-report and turn the answer into a mixed-time snapshot (the failure mode +# #1144's design note names for folding this into a repair sweep). +# +# Census rows are agmsg_terminal_enumerate's own TSV lines, unmodified: +# \t\t a pane was observed there +# !\t\t that instance could not be read +# !!\t that terminal's instance list could not be read +# ?\t that terminal cannot enumerate at all +# +# _agmsg_actual_location_classify +# +# Prints exactly one of: +# +# matched the locator was positively observed in the census +# stale_or_missing_target the census reached this exact instance (or found +# it to hold no panes at all) and did NOT observe +# this pane there -- a positive absence, not a guess +# unknown the census never reached this locator at all: +# its terminal kind could not enumerate (`?`), that +# kind's instance list could not be read (`!!`), or +# this specific instance could not be read (`!`). +# Never say "stale" for this case -- "could not +# look" must not become "not there". +# +# occupant_mismatch is deliberately NOT produced here. That needs occupant +# IDENTITY -- who is actually sitting in the pane -- and #1155's census +# proves only that a pane EXISTS, not who holds it (verified by reading its +# shipped implementation, not assumed from its PR description). A caller +# must not treat "matched" as "this seat is really there" without a separate +# identity check; enumeration success, agent-likeness, and identity are three +# different facts and this function establishes only the first. +_agmsg_actual_location_classify() { # + local census="$1" kind="$2" inst="$3" pane="$4" line + local t; t="$(printf '\t')" + while IFS= read -r line; do + case "$line" in + "?${t}${kind}") printf 'unknown\n'; return 0 ;; + "!!${t}${kind}") printf 'unknown\n'; return 0 ;; + "!${t}${kind}${t}${inst}") printf 'unknown\n'; return 0 ;; + "${kind}${t}${inst}${t}${pane}") printf 'matched\n'; return 0 ;; + esac + done <<< "$census" + printf 'stale_or_missing_target\n' +} diff --git a/scripts/placement-collisions.sh b/scripts/placement-collisions.sh index d33fdab0e..aa47a5913 100755 --- a/scripts/placement-collisions.sh +++ b/scripts/placement-collisions.sh @@ -5,6 +5,36 @@ set -euo pipefail # Deliberately separate from team.sh: this command observes the fleet so an # operator can decide which seat to contact; it never participates in a seat's # self-repair path and never changes a placement record. +# +# TWO LAYERS, kept apart on purpose (see the #1144 design note reached from +# the issue). Folding them into one pass makes "we could not look" collapse +# into "there is nothing there" -- the exact failure this report exists to +# prevent. +# +# record-only layer (this file, ACTIVE today) +# Duplicate consistency: do two DIFFERENT seats' records resolve to the +# same canonical (kind, instance, pane) locator? Answered ENTIRELY from +# records on disk. No terminal is ever asked anything -- no +# terminal_pane_state, no agent_list, nothing that touches a live pane. +# A ref that cannot be resolved to a locator carrying an instance +# component (every herdr ref today; a legacy bare tmux %N/@N) is not +# silently joined by raw string equality -- it is reported as +# unscoped_record and excluded from both the collision count and a +# "collisions: none" verdict, because record-only evidence genuinely +# cannot tell such refs apart across terminal instances (measured for +# herdr in #1155: two live instances answered the same bare pane id). +# +# actual-location layer (interface only, NOT wired here) +# Does an individual seat's own claimed locator match where a census +# (#1155's agmsg_terminal_enumerate) actually observed it? This needs a +# live enumeration and an occupant-identity resolution this script does +# not perform, so it stays a documented interface +# (scripts/lib/placement-actual-location.sh) until #1155 lands and is +# wired from here. Do not make that layer report "matched" or +# "stale_or_missing_target" against a stubbed or partial census -- an +# interface with no real observation behind it must not be exercised +# end-to-end, since a caller cannot tell "checked, fine" from "not +# really checked" once it prints a verdict. SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)" SKILL_DIR="$(cd "$SCRIPT_DIR/.." && pwd)" @@ -21,94 +51,187 @@ if [ "$#" -ne 0 ]; then exit 2 fi -# Print rows for refs claimed by two DIFFERENT agent names. Claims are found -# through team registries and agmsg_spawn_path rather than by splitting the flat -# spawn filename: both team and agent names may legally contain `__`, so that -# filename is not reversible. The same agent registered in multiple teams is -# one seat for this report and is not a collision by itself. -_placement_collision_rows() { - local cfg team escaped agent rec ref _project type tab - [ -d "$SKILL_DIR/teams" ] || return 0 - tab="$(printf '\t')" - { - for cfg in "$SKILL_DIR"/teams/*/config.json; do - [ -f "$cfg" ] || continue - team="${cfg%/config.json}"; team="${team##*/}" - escaped="$(sed "s/'/''/g" "$cfg")" - while IFS= read -r agent; do - [ -n "$agent" ] || continue - rec="$(agmsg_spawn_path "$team" "$agent" 2>/dev/null)" || continue - [ -f "$rec" ] || continue - IFS="$tab" read -r ref _project type _fence < "$rec" 2>/dev/null || continue - [ -n "$ref" ] || continue - printf '%s\t%s\t%s\t%s\n' "$ref" "$agent" "$team" "$type" - done < <(sqlite3 -noheader :memory: \ - "SELECT key FROM json_each(json_extract('$escaped', '\$.agents')) ORDER BY key;" 2>/dev/null) - done - } | LC_ALL=C sort -t "$tab" -k1,1 -k2,2 -k3,3 | awk -F '\t' ' - function flush() { if (distinct > 1) printf "%s", rows } - $1 != ref { - flush() - ref = $1; last_agent = ""; distinct = 0; rows = "" - } - { - if ($2 != last_agent) { distinct++; last_agent = $2 } - rows = rows $1 "\t" $3 "\t" $2 "\t" $4 "\n" - } - END { flush() } - ' -} - -# Print `absent` only when the terminal establishes BOTH facts: the pane exists, -# and no agent resides there. rc=1 is the readiness contract's not_ready branch; -# rc=2 is unknown even if a driver's body happens to resemble a known reason. -_collision_resident() { # [ ...] - local ref="$1" terminal pane state result rc type cli saw_type=0 - shift - terminal="$(agmsg_terminal_ref_terminal "$ref" 2>/dev/null)" || return 1 - pane="$(agmsg_terminal_ref_id "$ref" 2>/dev/null)" || return 1 - agmsg_terminal_load "$terminal" >/dev/null 2>&1 || return 1 - state="$(terminal_pane_state "$pane" 2>/dev/null)" || return 1 - [ "$state" = present ] || return 1 - declare -F terminal_team_input_ready >/dev/null 2>&1 || return 1 - for type in "$@"; do - [ -n "$type" ] || return 1 - cli="$(agmsg_type_get "$type" cli 2>/dev/null)" || return 1 - [ -n "$cli" ] || return 1 - saw_type=1 - if result="$(terminal_team_input_ready "$pane" "$cli" 2>/dev/null)"; then - rc=0 - else - rc=$? +# Placement records now carry a fourth TAB field (fence=:, +# #1152/#1157). Not read here: this walk only ever takes the FIRST tab-delimited +# field (the ref) via parameter expansion below, never a fixed-arity `read`, so +# an extra trailing field is already inert to it -- nothing to update for it. +TAB="$(printf '\t')" + +# `teams/` itself missing is a THIRD value, distinct from "walked everything +# and found nothing" and from "walked, but something along the way could not +# be read" -- there was no walk to attempt at all. +if [ ! -d "$SKILL_DIR/teams" ]; then + printf 'collisions: not_attempted\n' + printf 'reason: no teams directory\n' + exit 0 +fi + +COV_DIR="$(mktemp -d "${TMPDIR:-/tmp}/agmsg-placement-collisions.XXXXXX")" +trap 'rm -rf "$COV_DIR"' EXIT +: > "$COV_DIR/agent_enumeration_failed" +: > "$COV_DIR/path_resolution_failed" +: > "$COV_DIR/record_unreadable" +: > "$COV_DIR/empty_ref" +: > "$COV_DIR/rows" +: > "$COV_DIR/unscoped" + +shopt -s nullglob +cfgs=("$SKILL_DIR"/teams/*/config.json) +shopt -u nullglob + +for cfg in "${cfgs[@]}"; do + [ -f "$cfg" ] || continue + team="${cfg%/config.json}"; team="${team##*/}" + escaped="$(sed "s/'/''/g" "$cfg")" + # CAPTURED, not looped-over-directly: a query that fails outright must not + # read as "this team has zero agents" (the same shape #1155 was BLOCKed on + # in review -- an extraction failure silently becoming an empty result). + if agents="$(sqlite3 -noheader :memory: \ + "SELECT key FROM json_each(json_extract('$escaped', '\$.agents')) ORDER BY key;" 2>/dev/null)"; then + : + else + printf '%s\n' "$team" >> "$COV_DIR/agent_enumeration_failed" + continue + fi + [ -n "$agents" ] || continue # zero registered agents is a real, observed state + while IFS= read -r agent; do + [ -n "$agent" ] || continue + if ! rec="$(agmsg_spawn_path "$team" "$agent" 2>/dev/null)"; then + printf '%s/%s\n' "$team" "$agent" >> "$COV_DIR/path_resolution_failed" + continue fi - [ "$rc" -eq 1 ] && [ "$result" = not_ready:agent_not_found ] || return 1 - done - [ "$saw_type" -eq 1 ] || return 1 - printf 'absent\n' -} - -rows="$(_placement_collision_rows)" -[ -n "$rows" ] || exit 0 - -printf 'Placement collisions:\n' -current="" types="" -while IFS="$(printf '\t')" read -r ref team agent type; do - [ -n "$ref" ] || continue - if [ "$ref" != "$current" ]; then - if [ -n "$current" ]; then - resident="$(_collision_resident "$current" $types 2>/dev/null)" || resident="" - [ "$resident" = absent ] && printf ' resident_agent: absent\n' + # No placement record at all is the ordinary state for a registered agent + # that has never named a pane -- not a read failure, so not a coverage gap. + [ -f "$rec" ] || continue + # NOT `IFS="$TAB" read -r ref _project _type < "$rec"`: tab is an "IFS + # whitespace" character to bash's read/word-splitting, so a LEADING tab + # (an empty ref field) is silently swallowed rather than producing an + # empty first field -- measured, `printf '\t/tmp/proj\tx\n'` read back + # ref="/tmp/proj". Reading the whole line and slicing it with parameter + # expansion does not collapse anything. + if ! IFS= read -r rec_line < "$rec" 2>/dev/null; then + printf '%s/%s\n' "$team" "$agent" >> "$COV_DIR/record_unreadable" + continue fi - current="$ref"; types="" - printf ' ref: %s\n' "$ref" - fi - printf ' - %s/%s\n' "$team" "$agent" - case " $types " in *" $type "*) ;; *) types="${types:+$types }$type" ;; esac -done </dev/null)" || resident="" - [ "$resident" = absent ] && printf ' resident_agent: absent\n' + case "$rec_line" in + *"$TAB"*) ref="${rec_line%%"$TAB"*}" ;; + *) ref="$rec_line" ;; + esac + if [ -z "$ref" ]; then + printf '%s/%s\n' "$team" "$agent" >> "$COV_DIR/empty_ref" + continue + fi + if _agmsg_placement_split "$ref" 2>/dev/null; then + case "$_AGMSG_PS_TERM" in + # plain's ref is always the "no addressable pane" sentinel (see + # driver-interface.md): there is no shared resource two records could + # be claiming, so it is neither a collision candidate nor an + # unscoped_record -- reporting it as either would imply doubt where + # none exists. + plain) : ;; + tmux) + if [ -n "$_AGMSG_PS_SOCK" ]; then + printf 'tmux\t%s\t%s\t%s\t%s\n' \ + "$_AGMSG_PS_SOCK" "$_AGMSG_PS_ID" "$team" "$agent" >> "$COV_DIR/rows" + else + printf '%s\t%s\t%s\n' "$ref" "$team" "$agent" >> "$COV_DIR/unscoped" + fi + ;; + *) + # herdr today, and any future scheme with no instance component in + # the ref: the record alone cannot name which live instance it + # belongs to, so it cannot be safely joined against another record. + printf '%s\t%s\t%s\n' "$ref" "$team" "$agent" >> "$COV_DIR/unscoped" + ;; + esac + else + printf '%s\t%s\t%s\n' "$ref" "$team" "$agent" >> "$COV_DIR/unscoped" + fi + done <<< "$agents" +done + +# --- record-only collisions: group by canonical (kind, instance, pane) ----- +# Same exclusion as the original #1144 report: one agent name registered in +# more than one team is one seat, not a collision, by itself. Rows in +# $COV_DIR/rows are "\t\t\t\t"; only tmux +# rows with a resolved instance (socket) ever reach this file (see the main +# walk above), so kind is always "tmux" today, but the join is written on the +# three-field locator rather than assuming that. +# +# awk prints two tagged line shapes so the shell loop below never has to +# guess which fields a line carries: "GROUP\t\t\t" once +# per colliding locator, followed by one "ROW\t\t" per claimant. +collision_groups="$(LC_ALL=C sort -t "$TAB" -k1,1 -k2,2 -k3,3 -k5,5 "$COV_DIR/rows" 2>/dev/null | awk -F'\t' ' + function flush() { + if (distinct > 1) { printf "GROUP\t%s\t%s\t%s\n%s", kind, inst, pane, rows } + } + ($1 SUBSEP $2 SUBSEP $3) != key { + flush() + key = $1 SUBSEP $2 SUBSEP $3; kind = $1; inst = $2; pane = $3 + last_agent = ""; distinct = 0; rows = "" + } + { + if ($5 != last_agent) { distinct++; last_agent = $5 } + rows = rows "ROW\t" $4 "\t" $5 "\n" + } + END { flush() } +')" + +n_collisions=0 +collision_report="" +if [ -n "$collision_groups" ]; then + while IFS="$TAB" read -r tag a b c; do + case "$tag" in + GROUP) + n_collisions=$((n_collisions + 1)) + collision_report="${collision_report} ref: ${a}:${b}:${c}"$'\n' + ;; + ROW) + collision_report="${collision_report} - ${a}/${b}"$'\n' + ;; + esac + done <<< "$collision_groups" +fi + +n_unscoped="$(wc -l < "$COV_DIR/unscoped" | tr -d ' ')" + +# --- coverage: never let a silent skip read as a clean answer --------------- +n_agent_enum="$(wc -l < "$COV_DIR/agent_enumeration_failed" | tr -d ' ')" +n_path="$(wc -l < "$COV_DIR/path_resolution_failed" | tr -d ' ')" +n_record="$(wc -l < "$COV_DIR/record_unreadable" | tr -d ' ')" +n_empty_ref="$(wc -l < "$COV_DIR/empty_ref" | tr -d ' ')" +total_failures=$((n_agent_enum + n_path + n_record + n_empty_ref)) + +printf 'Placement collisions (record-only):\n' +if [ "$n_collisions" -gt 0 ]; then + printf '%s' "$collision_report" + printf 'collisions: %s\n' "$n_collisions" +elif [ "$total_failures" -gt 0 ]; then + printf 'collisions: none_observed\n' +else + printf 'collisions: none\n' fi + +printf 'unscoped_records: %s\n' "$n_unscoped" +if [ "$n_unscoped" -gt 0 ]; then + LC_ALL=C sort "$COV_DIR/unscoped" | awk -F'\t' ' + $1 != ref { ref = $1; printf " ref: %s\n", ref } + { printf " - %s/%s\n", $2, $3 } + ' +fi + +if [ "$total_failures" -gt 0 ]; then + printf 'coverage: partial\n' + [ "$n_agent_enum" -gt 0 ] && printf ' agent_enumeration_failed: %s (%s)\n' \ + "$n_agent_enum" "$(paste -sd, "$COV_DIR/agent_enumeration_failed")" + [ "$n_path" -gt 0 ] && printf ' path_resolution_failed: %s (%s)\n' \ + "$n_path" "$(paste -sd, "$COV_DIR/path_resolution_failed")" + [ "$n_record" -gt 0 ] && printf ' record_unreadable: %s (%s)\n' \ + "$n_record" "$(paste -sd, "$COV_DIR/record_unreadable")" + [ "$n_empty_ref" -gt 0 ] && printf ' empty_ref: %s (%s)\n' \ + "$n_empty_ref" "$(paste -sd, "$COV_DIR/empty_ref")" +else + printf 'coverage: complete\n' +fi + exit 0 diff --git a/tests/test_placement_actual_location.bats b/tests/test_placement_actual_location.bats new file mode 100644 index 000000000..592a4aa76 --- /dev/null +++ b/tests/test_placement_actual_location.bats @@ -0,0 +1,70 @@ +#!/usr/bin/env bats +# Actual-location layer interface (#1144). NOT wired from +# placement-collisions.sh yet -- see that script's header and +# scripts/lib/placement-actual-location.sh. These tests feed the classifier a +# hand-built census blob shaped exactly like agmsg_terminal_enumerate's own +# TSV output; none of them call the real primitive or touch a terminal. + +load test_helper + +setup() { + setup_test_env + # shellcheck disable=SC1090 + . "$SCRIPTS/lib/placement-actual-location.sh" +} +teardown() { teardown_test_env; } + +_census() { printf '%s\n' "$@"; } + +@test "a locator the census actually observed is matched (#1144)" { + census="$(_census $'herdr\tsockA\tw1:p9' $'tmux\tsockB\t%3')" + run _agmsg_actual_location_classify "$census" herdr sockA w1:p9 + [ "$status" -eq 0 ] + [ "$output" = matched ] +} + +@test "an instance the census reached, with no matching pane, is stale_or_missing_target (#1144)" { + # sockA WAS reached (it has an observed row for a different pane), and the + # target pane just is not among them -- a positive absence. + census="$(_census $'herdr\tsockA\tw1:p1')" + run _agmsg_actual_location_classify "$census" herdr sockA w1:p9 + [ "$status" -eq 0 ] + [ "$output" = stale_or_missing_target ] +} + +@test "an instance never mentioned at all, with an empty census, is stale_or_missing_target (#1144)" { + # An empty census for a kind that enumerated cleanly (no ?/!!/! rows at + # all) means every instance of that kind was reached and found empty. + census="" + run _agmsg_actual_location_classify "$census" herdr sockA w1:p9 + [ "$status" -eq 0 ] + [ "$output" = stale_or_missing_target ] +} + +@test "an instance the census could not read is unknown, never stale (#1144)" { + census="$(_census $'!\therdr\tsockA')" + run _agmsg_actual_location_classify "$census" herdr sockA w1:p9 + [ "$status" -eq 0 ] + [ "$output" = unknown ] +} + +@test "a kind whose instance list could not be read is unknown (#1144)" { + census="$(_census $'!!\therdr')" + run _agmsg_actual_location_classify "$census" herdr sockA w1:p9 + [ "$status" -eq 0 ] + [ "$output" = unknown ] +} + +@test "a kind that cannot enumerate at all is unknown (#1144)" { + census="$(_census $'?\tplain')" + run _agmsg_actual_location_classify "$census" plain '-' anything + [ "$status" -eq 0 ] + [ "$output" = unknown ] +} + +@test "an unreadable OTHER instance of the same kind does not poison this one (#1144)" { + census="$(_census $'!\therdr\tsockZ' $'herdr\tsockA\tw1:p9')" + run _agmsg_actual_location_classify "$census" herdr sockA w1:p9 + [ "$status" -eq 0 ] + [ "$output" = matched ] +} diff --git a/tests/test_placement_collisions.bats b/tests/test_placement_collisions.bats index c19281e82..761f25e61 100644 --- a/tests/test_placement_collisions.bats +++ b/tests/test_placement_collisions.bats @@ -1,107 +1,194 @@ #!/usr/bin/env bats +# Record-only layer of the #1144 placement collision report. See the design +# note reached from the issue and the header of scripts/placement-collisions.sh +# for the two-layer contract this file exercises only the first half of. load test_helper setup() { setup_test_env; } teardown() { teardown_test_env; } -_install_collision_fixture() { - local bin="$BATS_TEST_TMPDIR/collision-bin" - mkdir -p "$bin" - cat > "$bin/herdr" <<'STUB' -#!/usr/bin/env bash -case "$1/$2" in - agent/list) - printf '%s\n' '{"result":{"agents":[{"agent":"","pane_id":"w1:p9","terminal_id":"tm1","tab_id":"t1","workspace_id":"ws1"}]}}' - ;; - agent/get) - if [ "${COLLISION_OCCUPIED:-0}" -eq 1 ]; then - printf '%s\n' '{"result":{"agent":{"agent":"claude","agent_status":"idle"}}}' - else - printf '%s\n' '{"error":{"code":"agent_not_found"}}' - exit 1 - fi - ;; - pane/get) - printf '%s\n' '{"result":{"pane":{"pane_id":"w1:p9","agent_status":"idle","label":"","terminal_title":""}}}' - ;; - *) exit 1 ;; -esac -STUB - chmod +x "$bin/herdr" - export PATH="$bin:$PATH" -} - -_join_with_claim() { # [ref] - local team="$1" agent="$2" ref="${3:-herdr:w1:p9}" +# +_place() { + local team="$1" agent="$2" ref="$3" bash "$SCRIPTS/join.sh" "$team" "$agent" claude-code /tmp/proj >/dev/null mkdir -p "$TEST_SKILL_DIR/run" printf '%s\t/tmp/proj\tclaude-code\n' "$ref" > "$TEST_SKILL_DIR/run/spawn.${team}__${agent}" } -@test "placement collisions reports different agents across teams and a proven empty pane (#1144)" { - _install_collision_fixture - _join_with_claim alpha alice - _join_with_claim beta alice - _join_with_claim gamma bob +@test "reports a canonical tmux collision between two distinct seats (#1144)" { + _place alpha alice tmux:/tmp/sockA:%3 + _place beta bob tmux:/tmp/sockA:%3 run bash "$SCRIPTS/placement-collisions.sh" [ "$status" -eq 0 ] - grep -Fq "Placement collisions:" <<< "$output" + grep -Fq "Placement collisions (record-only):" <<< "$output" + grep -Fq "ref: tmux:/tmp/sockA:%3" <<< "$output" + grep -Fq -- "- alpha/alice" <<< "$output" + grep -Fq -- "- beta/bob" <<< "$output" + grep -Fq "collisions: 1" <<< "$output" + grep -Fq "unscoped_records: 0" <<< "$output" + grep -Fq "coverage: complete" <<< "$output" +} + +@test "two different tmux instances sharing a bare pane number are not joined (#1144)" { + _place alpha alice tmux:/tmp/sockA:%3 + _place beta bob tmux:/tmp/sockB:%3 + + run bash "$SCRIPTS/placement-collisions.sh" + [ "$status" -eq 0 ] + refute grep -Fq "collisions: 1" <<< "$output" + grep -Fq "collisions: none" <<< "$output" +} + +@test "herdr refs are never joined as a collision, even when the raw id repeats (#1144)" { + _place alpha alice herdr:w1:p9 + _place beta bob herdr:w1:p9 + + run bash "$SCRIPTS/placement-collisions.sh" + [ "$status" -eq 0 ] + # The whole point: a bare herdr id is not an address (#1155). Record-only + # evidence cannot tell two live instances apart, so this must never read as + # a proven collision. + refute grep -Fq "collisions: 1" <<< "$output" + grep -Fq "collisions: none" <<< "$output" + grep -Fq "unscoped_records: 2" <<< "$output" grep -Fq "ref: herdr:w1:p9" <<< "$output" grep -Fq -- "- alpha/alice" <<< "$output" - grep -Fq -- "- beta/alice" <<< "$output" - grep -Fq -- "- gamma/bob" <<< "$output" - grep -Fq "resident_agent: absent" <<< "$output" - [ "$(cut -f1 "$TEST_SKILL_DIR/run/spawn.alpha__alice")" = herdr:w1:p9 ] - [ "$(cut -f1 "$TEST_SKILL_DIR/run/spawn.beta__alice")" = herdr:w1:p9 ] - [ "$(cut -f1 "$TEST_SKILL_DIR/run/spawn.gamma__bob")" = herdr:w1:p9 ] + grep -Fq -- "- beta/bob" <<< "$output" } -@test "placement collisions keeps rc2 unknown even when its body says agent_not_found (#1144)" { - _install_collision_fixture - _join_with_claim alpha alice - _join_with_claim beta bob - cat >> "$TEST_SKILL_DIR/scripts/drivers/terminals/herdr/ops.sh" <<'OPS' -terminal_team_input_ready() { - printf 'not_ready:agent_not_found\n' - return 2 +@test "a legacy bare tmux ref with no socket is unscoped, not joined (#1144)" { + _place alpha alice %3 + _place beta bob %3 + + run bash "$SCRIPTS/placement-collisions.sh" + [ "$status" -eq 0 ] + grep -Fq "collisions: none" <<< "$output" + grep -Fq "unscoped_records: 2" <<< "$output" } -OPS + +@test "the same agent name registered in two teams is one seat, not a collision (#1144)" { + _place alpha alice tmux:/tmp/sockA:%3 + _place beta alice tmux:/tmp/sockA:%3 run bash "$SCRIPTS/placement-collisions.sh" [ "$status" -eq 0 ] - grep -Fq "Placement collisions:" <<< "$output" - refute grep -Fq "resident_agent: absent" <<< "$output" + grep -Fq "collisions: none" <<< "$output" + grep -Fq "unscoped_records: 0" <<< "$output" } -@test "placement collisions excludes one agent registered in two teams (#1144)" { - _install_collision_fixture - _join_with_claim alpha alice - _join_with_claim beta alice +@test "a plain ref (no addressable pane) is neither a collision nor unscoped (#1144)" { + _place alpha alice plain:- + _place beta bob plain:- run bash "$SCRIPTS/placement-collisions.sh" [ "$status" -eq 0 ] - [ -z "$output" ] + grep -Fq "collisions: none" <<< "$output" + grep -Fq "unscoped_records: 0" <<< "$output" + grep -Fq "coverage: complete" <<< "$output" } -@test "placement collisions never guesses that an occupied pane is empty (#1144)" { - _install_collision_fixture - _join_with_claim alpha alice herdr:w1:pA - _join_with_claim beta bob - _join_with_claim gamma carol - export COLLISION_OCCUPIED=1 +@test "no teams directory at all is not_attempted, not none (#1144)" { + rm -rf "$TEST_SKILL_DIR/teams" run bash "$SCRIPTS/placement-collisions.sh" [ "$status" -eq 0 ] - grep -Fq -- "- beta/bob" <<< "$output" - grep -Fq -- "- gamma/carol" <<< "$output" - refute grep -Fq -- "- alpha/alice" <<< "$output" - refute grep -Fq "resident_agent: absent" <<< "$output" + grep -Fq "collisions: not_attempted" <<< "$output" + grep -Fq "reason: no teams directory" <<< "$output" + refute grep -Fq "collisions: none" <<< "$output" } -@test "placement collisions rejects arguments rather than implying a repair scope (#1144)" { +@test "an empty teams directory is a fully observed empty answer (#1144)" { + run bash "$SCRIPTS/placement-collisions.sh" + [ "$status" -eq 0 ] + grep -Fq "collisions: none" <<< "$output" + grep -Fq "coverage: complete" <<< "$output" +} + +@test "rejects arguments rather than implying a repair scope (#1144)" { run bash "$SCRIPTS/placement-collisions.sh" alpha [ "$status" -eq 2 ] grep -Fq "Usage: placement-collisions.sh" <<< "$output" } + +# --- negative controls: an unreadable path must never read as "none" ------- +# +# Each of these forces one of the walk's failure branches and asserts the +# output says WHY it is empty, distinctly from a genuinely observed empty +# answer. Removing the corresponding counting in placement-collisions.sh +# (reverting to a bare `continue`) makes each of these red: the coverage +# line and its named category would silently disappear and "collisions: +# none" would come back instead of "none_observed". + +@test "control: a team config whose agents cannot be enumerated is coverage:partial, not none (#1144)" { + _place alpha alice tmux:/tmp/sockA:%3 + # Malformed JSON: the sqlite3 json_each extraction genuinely errors, rather + # than the file merely being absent. + mkdir -p "$TEST_SKILL_DIR/teams/broken" + printf '{ this is not json' > "$TEST_SKILL_DIR/teams/broken/config.json" + + run bash "$SCRIPTS/placement-collisions.sh" + [ "$status" -eq 0 ] + refute grep -Fq "collisions: none$" <<< "$output" + grep -Fq "collisions: none_observed" <<< "$output" + grep -Fq "coverage: partial" <<< "$output" + grep -Fq "agent_enumeration_failed: 1 (broken)" <<< "$output" +} + +@test "control: a path-resolution failure for one agent is coverage:partial, not none (#1144)" { + bash "$SCRIPTS/join.sh" alpha alice claude-code /tmp/proj >/dev/null + # Force agmsg_spawn_path to fail for exactly this agent, the way a future + # caller's own bug would -- proving the walk surfaces it instead of quietly + # treating alice as "never placed". + cat >> "$TEST_SKILL_DIR/scripts/lib/actas-lock.sh" <<'OVERRIDE' +agmsg_spawn_path() { + [ "$2" = alice ] && return 1 + printf '%s/run/spawn.%s__%s' "$SKILL_DIR" "$1" "$2" +} +OVERRIDE + + run bash "$SCRIPTS/placement-collisions.sh" + [ "$status" -eq 0 ] + grep -Fq "collisions: none_observed" <<< "$output" + grep -Fq "coverage: partial" <<< "$output" + grep -Fq "path_resolution_failed: 1 (alpha/alice)" <<< "$output" +} + +@test "control: an unreadable placement record is coverage:partial, not none (#1144)" { + bash "$SCRIPTS/join.sh" alpha alice claude-code /tmp/proj >/dev/null + mkdir -p "$TEST_SKILL_DIR/run" + : > "$TEST_SKILL_DIR/run/spawn.alpha__alice" # present, but empty: read fails + + run bash "$SCRIPTS/placement-collisions.sh" + [ "$status" -eq 0 ] + grep -Fq "collisions: none_observed" <<< "$output" + grep -Fq "coverage: partial" <<< "$output" + grep -Fq "record_unreadable: 1 (alpha/alice)" <<< "$output" +} + +@test "control: a record with an empty ref field is coverage:partial, not none (#1144)" { + bash "$SCRIPTS/join.sh" alpha alice claude-code /tmp/proj >/dev/null + mkdir -p "$TEST_SKILL_DIR/run" + printf '\t/tmp/proj\tclaude-code\n' > "$TEST_SKILL_DIR/run/spawn.alpha__alice" + + run bash "$SCRIPTS/placement-collisions.sh" + [ "$status" -eq 0 ] + grep -Fq "collisions: none_observed" <<< "$output" + grep -Fq "coverage: partial" <<< "$output" + grep -Fq "empty_ref: 1 (alpha/alice)" <<< "$output" +} + +@test "a coverage gap does not suppress a collision found alongside it (#1144)" { + _place alpha alice tmux:/tmp/sockA:%3 + _place beta bob tmux:/tmp/sockA:%3 + mkdir -p "$TEST_SKILL_DIR/teams/broken" + printf '{ this is not json' > "$TEST_SKILL_DIR/teams/broken/config.json" + + run bash "$SCRIPTS/placement-collisions.sh" + [ "$status" -eq 0 ] + grep -Fq "collisions: 1" <<< "$output" + grep -Fq "ref: tmux:/tmp/sockA:%3" <<< "$output" + grep -Fq "coverage: partial" <<< "$output" + grep -Fq "agent_enumeration_failed: 1 (broken)" <<< "$output" +} From 6fc5fda96c4fcc8e44795561d3b4e83fdadd2489 Mon Sep 17 00:00:00 2001 From: fujibee Date: Fri, 11 Sep 2026 21:21:56 -0700 Subject: [PATCH 2/2] fix(team): drop nullglob+array for the teams walk (#1144) CI log showed the real cause instead of a guess: 'cfgs[@]: unbound variable' at scripts/placement-collisions.sh:147, macos-latest only. macOS's default /bin/bash is 3.2, where referencing "${cfgs[@]}" on a ZERO-element array under set -u raises unbound variable (fixed upstream well after 3.2; ubuntu-latest's modern bash does not reproduce it). Reproduced directly with /bin/bash 3.2.57 locally, confirmed the fix against the same interpreter, and reran the full suite. --- scripts/placement-collisions.sh | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/scripts/placement-collisions.sh b/scripts/placement-collisions.sh index aa47a5913..7ef7c63e7 100755 --- a/scripts/placement-collisions.sh +++ b/scripts/placement-collisions.sh @@ -75,11 +75,14 @@ trap 'rm -rf "$COV_DIR"' EXIT : > "$COV_DIR/rows" : > "$COV_DIR/unscoped" -shopt -s nullglob -cfgs=("$SKILL_DIR"/teams/*/config.json) -shopt -u nullglob - -for cfg in "${cfgs[@]}"; do +# NOT `shopt -s nullglob; cfgs=(...); for cfg in "${cfgs[@]}"`: macOS's +# default /bin/bash is 3.2, where referencing "${cfgs[@]}" on a +# ZERO-element array under `set -u` raises "unbound variable" (fixed +# upstream well after 3.2; ubuntu-latest's modern bash does not reproduce +# this, which is exactly why this broke on macOS only -- measured on CI). +# A plain glob with no nullglob leaves the literal pattern as one word when +# nothing matches, and `[ -f "$cfg" ]` already filters that out. +for cfg in "$SKILL_DIR"/teams/*/config.json; do [ -f "$cfg" ] || continue team="${cfg%/config.json}"; team="${team##*/}" escaped="$(sed "s/'/''/g" "$cfg")"