From d20082a2d2147e6cfac17aacf0fffe37261df05f Mon Sep 17 00:00:00 2001 From: fujibee Date: Fri, 11 Sep 2026 18:49:17 -0700 Subject: [PATCH] feat(terminal): a herdr id may carry its socket, and one locator grammar for every kind (#1055) A herdr pane id is unique inside one running session and nowhere else; with two sessions both owning a w1:p2, every command that took a pane id reached whichever session the ambient environment named, and a repair resolved in one session landed in another's live pane. tmux has carried the socket inside the id since #1051; herdr now does the same: :wN:pX is accepted everywhere a herdr id is, a bare id keeps meaning the ambient instance, and a colon or control character in the socket is refused by name (the round-trip encoding is #1166). Every call about a pane goes through _herdr_cli , which sets HERDR_SOCKET_PATH from the id and hands the CLI the bare pane; arrange refuses a source and target in different instances. The registry gains the one locator grammar, ::, with agmsg_locator_compose and agmsg_locator_split; each driver splits its own id (terminal_id_split), because where an instance ends inside an id is the driver's grammar. Refusals are one named reason on stderr. A call that bypasses _herdr_cli is silent and lands on the wrong instance, so .github/scripts/check-herdr-cli-routing.sh counts direct herdr calls per function by call position and compares them with a named allowlist in both directions; it runs from the bats suite as well. --- .github/herdr-cli-routing-allowlist | 19 +++ .github/scripts/check-herdr-cli-routing.sh | 90 ++++++++++++ scripts/drivers/terminals/herdr/ops.sh | 98 ++++++++++--- scripts/drivers/terminals/plain/ops.sh | 6 + scripts/drivers/terminals/tmux/ops.sh | 9 ++ scripts/lib/terminal-registry.sh | 92 ++++++++++++ tests/test_herdr_cli_routing.bats | 106 ++++++++++++++ tests/test_locator.bats | 156 +++++++++++++++++++++ 8 files changed, 554 insertions(+), 22 deletions(-) create mode 100644 .github/herdr-cli-routing-allowlist create mode 100755 .github/scripts/check-herdr-cli-routing.sh create mode 100644 tests/test_herdr_cli_routing.bats create mode 100644 tests/test_locator.bats diff --git a/.github/herdr-cli-routing-allowlist b/.github/herdr-cli-routing-allowlist new file mode 100644 index 000000000..6505d0113 --- /dev/null +++ b/.github/herdr-cli-routing-allowlist @@ -0,0 +1,19 @@ +# Functions in scripts/drivers/terminals/herdr/ops.sh that call the `herdr` CLI +# directly, with the exact number of such calls. Checked by +# .github/scripts/check-herdr-cli-routing.sh in both directions. +# +# Every function here is instance-wide or deliberately ambient: it lists or +# creates in the instance the environment names, and takes no pane id whose +# socket could name a different one. A function that operates ON a pane id is +# not allowed here -- it routes through `_herdr_cli ...`, which sets +# HERDR_SOCKET_PATH from a socket-qualified id. (`_herdr_cli` itself calls +# `herdr "$@"` with no literal subcommand word, so the position rule never +# counts it: it is the one routed call by construction.) +# +# +_herdr_pane_for_session 1 +terminal_describe 2 +terminal_detect 1 +terminal_find_by_label 1 +terminal_pane_process_observe 1 +terminal_spawn 5 diff --git a/.github/scripts/check-herdr-cli-routing.sh b/.github/scripts/check-herdr-cli-routing.sh new file mode 100755 index 000000000..09c5512b9 --- /dev/null +++ b/.github/scripts/check-herdr-cli-routing.sh @@ -0,0 +1,90 @@ +#!/usr/bin/env bash +# +# Fail when scripts/drivers/terminals/herdr/ops.sh calls the `herdr` CLI directly +# from a function that is not on the named allowlist, or when an allowlisted +# function's count of direct calls moves in EITHER direction. +# +# WHY. A herdr pane id may be qualified by the socket of the instance that owns +# it (`:wN:pX`, #1055). Every call ABOUT that pane must reach that +# socket, which `_herdr_cli ...` does by setting HERDR_SOCKET_PATH from the +# id. A call that bypasses `_herdr_cli` raises no error: it goes to whatever +# instance the ambient environment names, and a pane id is only unique inside +# one instance -- so the call lands on a different seat's live pane (measured +# 2026-09-11, the accident #1055 is about). One missed site is silent, and a +# review that reads twenty-one one-line edits will miss one. So this is +# counted by machine. +# +# HOW IT COUNTS. By CALL POSITION, not by text match: a line is a direct call +# only when `herdr ` stands where a command stands -- at the start +# of a statement, or right after `$(`, `if`, `!`, `&&`, `||`, `|`, `then`, +# `else`, `do`. Comment lines are skipped before matching, and `herdr:` inside a +# message string never has a subcommand word after it. (An earlier count of +# these calls by plain grep answered 55 where the true number is 34: comments +# and prose matched. That count is what this script must not repeat.) +# +# THE ALLOWLIST is `.github/herdr-cli-routing-allowlist`: one ` ` +# per line, naming the functions whose direct calls are instance-wide or +# deliberately ambient (listing every pane of an instance, spawning into the +# caller's own instance, describing the backend). A function not listed with a +# direct call fails; a listed function whose count went UP fails; a count that +# went DOWN fails too, and says to lower the entry -- an allowlist that is +# stale in the low direction lets the next addition hide inside the old number. +# `_herdr_cli` itself is the one function that must call `herdr` directly. + +set -u +root="$(cd "$(dirname "$0")/../.." && pwd)" +ops="$root/scripts/drivers/terminals/herdr/ops.sh" +allow="$root/.github/herdr-cli-routing-allowlist" +[ -f "$ops" ] || { echo "check-herdr-cli-routing: $ops not found" >&2; exit 2; } +[ -f "$allow" ] || { echo "check-herdr-cli-routing: $allow not found" >&2; exit 2; } + +# function -> number of direct `herdr ` calls at command position +counts="$(awk ' + /^[A-Za-z_][A-Za-z0-9_]*\(\)/ { fn=$0; sub(/\(\).*/,"",fn); next } + /^[[:space:]]*#/ { next } + fn != "" { + line=$0 + # strip a trailing comment that begins after whitespace + sub(/[[:space:]]#.*$/,"",line) + n=0 + # a command boundary, then optional spaces, then `herdr ` + while (match(line, /(^|\$\(|[;|&(]|[[:space:]](if|then|else|do|!)[[:space:]])[[:space:]]*herdr[[:space:]]+[a-z][a-z-]*/)) { + n++ + line=substr(line, RSTART+RLENGTH) + } + if (n) c[fn]+=n + } + END { for (f in c) printf "%s %d\n", f, c[f] }' "$ops" | sort)" + +status=0 +# every function with direct calls must be listed with exactly that count +while read -r fn n; do + [ -n "$fn" ] || continue + want="$(awk -v f="$fn" '$1==f {print $2}' "$allow")" + if [ -z "$want" ]; then + printf ' %s calls herdr directly %s time(s) and is not on the allowlist: route it through _herdr_cli , or add "%s %s" to %s with a reason in the commit\n' "$fn" "$n" "$fn" "$n" "${allow#"$root/"}" + status=1 + elif [ "$n" -gt "$want" ]; then + printf ' %s: %s direct herdr calls, allowlist says %s -- ABOVE: a new direct call; route it through _herdr_cli or raise the entry deliberately\n' "$fn" "$n" "$want" + status=1 + elif [ "$n" -lt "$want" ]; then + printf ' %s: %s direct herdr calls, allowlist says %s -- BELOW: lower the entry, or the next addition hides inside the old number\n' "$fn" "$n" "$want" + status=1 + fi +done <<< "$counts" +# every listed function must still exist with a nonzero count +while read -r fn want; do + case "$fn" in ''|'#'*) continue ;; esac + if ! printf '%s\n' "$counts" | grep -q "^$fn "; then + printf ' %s is on the allowlist with %s but makes no direct herdr call now -- remove the entry\n' "$fn" "$want" + status=1 + fi +done < "$allow" + +total="$(printf '%s\n' "$counts" | awk '{s+=$2} END {print s+0}')" +if [ "$status" -eq 0 ]; then + echo "check-herdr-cli-routing: $total direct herdr calls, all on the allowlist at their listed counts." +else + echo "check-herdr-cli-routing: FAILED (direct herdr calls outside _herdr_cli must be listed by name and count)." +fi +exit "$status" diff --git a/scripts/drivers/terminals/herdr/ops.sh b/scripts/drivers/terminals/herdr/ops.sh index 36578cd4f..ccac4214d 100644 --- a/scripts/drivers/terminals/herdr/ops.sh +++ b/scripts/drivers/terminals/herdr/ops.sh @@ -303,7 +303,26 @@ _herdr_pane_id_ok() { # registry (`_agmsg_terminal_id_ok herdr `) for every row the label # resolver reads and every ref it validates; a malformed row must answer no. # w:p, alphanumerics only, exactly one colon. -terminal_id_ok() { # +# A herdr id is `wN:pX`, optionally qualified by the socket of the instance +# that owns it: `:wN:pX` (#1055; the shape tmux took in #1051). +# Pane ids repeat across running herdr sessions, so a bare id names a pane only +# in whatever instance the ambient HERDR_SOCKET_PATH points at; a qualified id +# names ONE pane, and every call about it goes to that socket (_herdr_cli). A +# socket path may contain spaces; a colon or a control character in it is +# refused rather than mis-split (the round-trippable form is #1166). +_herdr_sock_of() { # -> socket path, or "" for a bare id + case "$1" in + *:w*:p*) printf '%s' "${1%:*:*}" ;; + *) printf '' ;; + esac +} +_herdr_bare_of() { # -> wN:pX + case "$1" in + *:w*:p*) printf '%s' "${1#"${1%:*:*}":}" ;; + *) printf '%s' "$1" ;; + esac +} +_herdr_bare_ok() { # case "$1" in w[0-9A-Za-z]*:p[0-9A-Za-z]*) : ;; *) return 1 ;; @@ -312,6 +331,35 @@ terminal_id_ok() { # case "$1" in *[!0-9A-Za-z:]*) return 1 ;; esac return 0 } +terminal_id_ok() { # + local sock bare + # Control characters are checked on the WHOLE id first: `$( )` would strip a + # trailing newline from a split half and let it pass. + case "$1" in *[[:cntrl:]]*) return 1 ;; esac + sock="$(_herdr_sock_of "$1")"; bare="$(_herdr_bare_of "$1")" + if [ -n "$sock" ]; then + case "$sock" in *:*) return 1 ;; esac + fi + case "$1" in :*) return 1 ;; esac + _herdr_bare_ok "$bare" +} +# The id's two halves, as the locator grammar wants them: "\t". +# A bare id has no instance and is refused here -- a locator must name one. +terminal_id_split() { # + local sock + terminal_id_ok "$1" || return 1 + sock="$(_herdr_sock_of "$1")" + [ -n "$sock" ] || return 1 + printf '%s\t%s\n' "$sock" "$(_herdr_bare_of "$1")" +} +# Run one herdr CLI call ABOUT : a qualified id reaches its own instance +# through HERDR_SOCKET_PATH; a bare id keeps the ambient one. The bare pane id +# is what the CLI is given (via the caller's arguments), never the qualified one. +_herdr_cli() { # + local id="$1"; shift + local sock; sock="$(_herdr_sock_of "$id")" + if [ -n "$sock" ]; then HERDR_SOCKET_PATH="$sock" herdr "$@"; else herdr "$@"; fi +} _herdr_new_pane_id() { local json="$1" q pane esc @@ -359,7 +407,7 @@ _herdr_new_pane_id() { # they are not split — both are UNKNOWN. _herdr_pane_input_ready() { local pane="$1" info rc=0 sp fg jesc - info="$(herdr pane process-info --pane "$pane" 2>/dev/null)" || rc=$? + info="$(_herdr_cli "$pane" pane process-info --pane "$(_herdr_bare_of "$pane")" 2>/dev/null)" || rc=$? [ "$rc" -eq 0 ] || return 2 jesc="$(printf '%s' "$info" | sed "s/'/''/g")" # Require the JSON TYPE to be integer, in the SAME payload, BEFORE reading the value: @@ -473,7 +521,7 @@ terminal_spawn() { terminal_pane_state() { local id="$1" json rc=0 command -v herdr >/dev/null 2>&1 || { echo unknown; return 10; } - json="$(herdr agent list 2>/dev/null)" || rc=$? + json="$(_herdr_cli "$id" agent list 2>/dev/null)" || rc=$? [ "$rc" -eq 0 ] || { echo unknown; return 10; } [ -n "$json" ] || { echo unknown; return 10; } @@ -550,7 +598,7 @@ terminal_pane_state() { terminal_despawn() { local id="$1" - herdr pane close "$id" >/dev/null 2>&1 || { echo runtime_error; return 13; } + _herdr_cli "$id" pane close "$(_herdr_bare_of "$id")" >/dev/null 2>&1 || { echo runtime_error; return 13; } echo ok return 0 } @@ -562,7 +610,7 @@ terminal_where() { local id="$1" json rc=0 esc container present command -v herdr >/dev/null 2>&1 || { echo unknown; return 10; } _herdr_pane_id_ok "$id" || { echo unsupported; return 13; } - json="$(herdr pane layout --pane "$id" 2>/dev/null)" || rc=$? + json="$(_herdr_cli "$id" pane layout --pane "$(_herdr_bare_of "$id")" 2>/dev/null)" || rc=$? [ "$rc" -eq 0 ] && [ -n "$json" ] || { echo unknown; return 10; } esc="$(printf '%s' "$json" | sed "s/'/''/g")" present="$(sqlite3 :memory: "SELECT count(*) FROM json_each('$esc','\$.result.layout.panes') WHERE json_extract(value,'\$.pane_id') = '$(printf '%s' "$id" | sed "s/'/''/g")'" 2>/dev/null)" \ @@ -657,6 +705,12 @@ _herdr_swap_changed() { terminal_arrange() { local source="$1" intent="$2" target="$3" layout source_layout state rc=0 tab first second temporary_tab swap_result + # Two panes are only arrangeable inside ONE instance: a qualified source and + # target naming different sockets is refused here, before any call is made. + if [ "$(_herdr_sock_of "$source")" != "$(_herdr_sock_of "$target")" ]; then + echo "herdr: cannot arrange across instances ('$source' vs '$target')" >&2 + echo runtime_error; return 10 + fi command -v herdr >/dev/null 2>&1 || { echo runtime_error; return 10; } _herdr_pane_id_ok "$source" && _herdr_pane_id_ok "$target" || { echo unsupported; return 13; } case "$intent" in place_below|place_right|swap) : ;; *) echo unsupported; return 13 ;; esac @@ -665,13 +719,13 @@ terminal_arrange() { # Swap keeps both panes occupied, but the native command still needs a # positive existence observation for each id. A layout response for one # pane cannot establish the other when they live in different tabs. - layout="$(herdr pane layout --pane "$target" 2>/dev/null)" || { echo runtime_error; return 10; } + layout="$(_herdr_cli "$target" pane layout --pane "$(_herdr_bare_of "$target")" 2>/dev/null)" || { echo runtime_error; return 10; } [ -n "$layout" ] && _herdr_layout_has_pane "$layout" "$target" \ || { echo unknown; return 10; } - source_layout="$(herdr pane layout --pane "$source" 2>/dev/null)" || { echo runtime_error; return 10; } + source_layout="$(_herdr_cli "$source" pane layout --pane "$(_herdr_bare_of "$source")" 2>/dev/null)" || { echo runtime_error; return 10; } [ -n "$source_layout" ] && _herdr_layout_has_pane "$source_layout" "$source" \ || { echo unknown; return 10; } - swap_result="$(herdr pane swap --source-pane "$source" --target-pane "$target" 2>/dev/null)" \ + swap_result="$(_herdr_cli "$source" pane swap --source-pane "$(_herdr_bare_of "$source")" --target-pane "$(_herdr_bare_of "$target")" 2>/dev/null)" \ || { echo runtime_error; return 12; } [ -n "$swap_result" ] || { echo runtime_error; return 12; } rc=0 @@ -682,7 +736,7 @@ terminal_arrange() { *) echo runtime_error; return 12 ;; esac fi - layout="$(herdr pane layout --pane "$target" 2>/dev/null)" || rc=$? + layout="$(_herdr_cli "$target" pane layout --pane "$(_herdr_bare_of "$target")" 2>/dev/null)" || rc=$? [ "$rc" -eq 0 ] && [ -n "$layout" ] || { echo runtime_error; return 10; } rc=0 state="$(_herdr_arrange_state "$layout" "$source" "$intent" "$target")" || rc=$? @@ -696,7 +750,7 @@ terminal_arrange() { # valid move candidate. Ask the source itself before treating absence as # "different"; an unanswered or malformed lookup remains unknown. rc=0 - source_layout="$(herdr pane layout --pane "$source" 2>/dev/null)" || rc=$? + source_layout="$(_herdr_cli "$source" pane layout --pane "$(_herdr_bare_of "$source")" 2>/dev/null)" || rc=$? [ "$rc" -eq 0 ] && [ -n "$source_layout" ] && _herdr_layout_has_pane "$source_layout" "$source" \ || { echo unknown; return 10; } ;; @@ -706,13 +760,13 @@ terminal_arrange() { tab="$(sqlite3 :memory: "SELECT json_extract('$(printf '%s' "$layout" | sed "s/'/''/g")','\$.result.layout.tab_id')" 2>/dev/null)" \ || { echo runtime_error; return 10; } [ -n "$tab" ] || { echo runtime_error; return 10; } - first="$(herdr pane move "$source" --new-tab --no-focus 2>/dev/null)" || { echo runtime_error; echo "herdr: failed before moving '$source' to a temporary tab" >&2; return 12; } + first="$(_herdr_cli "$source" pane move "$(_herdr_bare_of "$source")" --new-tab --no-focus 2>/dev/null)" || { echo runtime_error; echo "herdr: failed before moving '$source' to a temporary tab" >&2; return 12; } _herdr_move_changed "$first" || { echo runtime_error; echo "herdr: the temporary-tab move for '$source' did not report changed=true" >&2; return 12; } temporary_tab="$(_herdr_move_created_tab "$first")" || temporary_tab="" [ -n "$temporary_tab" ] || temporary_tab='' case "$intent" in - place_below) second="$(herdr pane move "$source" --tab "$tab" --split down --target-pane "$target" --no-focus 2>/dev/null)" ;; - place_right) second="$(herdr pane move "$source" --tab "$tab" --split right --target-pane "$target" --no-focus 2>/dev/null)" ;; + place_below) second="$(_herdr_cli "$source" pane move "$(_herdr_bare_of "$source")" --tab "$tab" --split down --target-pane "$(_herdr_bare_of "$target")" --no-focus 2>/dev/null)" ;; + place_right) second="$(_herdr_cli "$source" pane move "$(_herdr_bare_of "$source")" --tab "$tab" --split right --target-pane "$(_herdr_bare_of "$target")" --no-focus 2>/dev/null)" ;; esac || { echo runtime_error echo "herdr: '$source' is left in temporary tab '$temporary_tab': placing it back in tab '$tab' relative to '$target' failed" >&2 @@ -761,9 +815,9 @@ terminal_peek() { local tmp rc=0 tmp="$(mktemp)" || { echo "herdr: could not allocate a temp file to peek pane '$id'" >&2; return 12; } if [ -n "$lines" ]; then - herdr pane read "$id" --source "$src" --lines "$lines" >"$tmp" 2>/dev/null || rc=$? + _herdr_cli "$id" pane read "$(_herdr_bare_of "$id")" --source "$src" --lines "$lines" >"$tmp" 2>/dev/null || rc=$? else - herdr pane read "$id" --source "$src" >"$tmp" 2>/dev/null || rc=$? + _herdr_cli "$id" pane read "$(_herdr_bare_of "$id")" --source "$src" >"$tmp" 2>/dev/null || rc=$? fi if [ "$rc" -ne 0 ]; then [ -s "$tmp" ] && cat "$tmp" >&2 # the error body is a diagnostic, not content @@ -782,8 +836,8 @@ terminal_team_observe() { local id="$1" pane_json agents_json pesc aesc activity label key title command -v herdr >/dev/null 2>&1 || return 10 _herdr_pane_id_ok "$id" || return 13 - pane_json="$(herdr pane get "$id" 2>/dev/null)" || return 10 - agents_json="$(herdr agent list 2>/dev/null)" || return 10 + pane_json="$(_herdr_cli "$id" pane get "$(_herdr_bare_of "$id")" 2>/dev/null)" || return 10 + agents_json="$(_herdr_cli "$id" agent list 2>/dev/null)" || return 10 pesc="$(printf '%s' "$pane_json" | sed "s/'/''/g")" aesc="$(printf '%s' "$agents_json" | sed "s/'/''/g")" activity="$(sqlite3 :memory: "SELECT COALESCE(json_extract('$pesc','\$.result.pane.agent_status'),'unknown:activity_missing')" 2>/dev/null)" || return 10 @@ -825,7 +879,7 @@ terminal_team_input_ready() { local id="$1" expected="$2" raw escaped kind status rc=0 command -v herdr >/dev/null 2>&1 || { printf 'unknown:terminal_unreachable\n'; return 2; } _herdr_pane_id_ok "$id" || { printf 'unknown:invalid_pane_id\n'; return 2; } - raw="$(herdr agent get "$id" 2>/dev/null)" || rc=$? + raw="$(_herdr_cli "$id" agent get "$(_herdr_bare_of "$id")" 2>/dev/null)" || rc=$? if [ "$rc" -ne 0 ]; then case "$raw" in *agent_not_found*) printf 'not_ready:agent_not_found\n'; return 1 ;; @@ -861,7 +915,7 @@ terminal_poke() { # "no one to receive" failure that peek does not. command -v herdr >/dev/null 2>&1 \ || { echo runtime_error; echo "herdr: not on PATH — cannot reach the terminal to poke pane '$id'" >&2; return 10; } - herdr agent prompt "$id" "$text" >/dev/null 2>&1 \ + _herdr_cli "$id" agent prompt "$(_herdr_bare_of "$id")" "$text" >/dev/null 2>&1 \ || { echo runtime_error; echo "herdr: could not deliver to pane '$id' — it may be gone, or have no live agent to receive (poke needs a running agent; peek does not)" >&2; return 12; } echo ok return 0 @@ -991,7 +1045,7 @@ terminal_label_of() { # [ -n "$id" ] || return 13 command -v herdr >/dev/null 2>&1 || return 10 _herdr_pane_id_ok "$id" || return 13 - pane_json="$(herdr pane get "$id" 2>/dev/null)" || return 10 + pane_json="$(_herdr_cli "$id" pane get "$(_herdr_bare_of "$id")" 2>/dev/null)" || return 10 esc="$(printf '%s' "$pane_json" | sed "s/'/''/g")" # NULLIF: json_extract returns SQL NULL for a missing key and '' for a key set # to the empty string, and both mean "this pane carries no label" -- neither is @@ -1038,7 +1092,7 @@ terminal_name() { return 13 } local _err _rc=0 - _err="$(herdr agent rename "$id" "$key" 2>&1 >/dev/null)" || _rc=$? + _err="$(_herdr_cli "$id" agent rename "$(_herdr_bare_of "$id")" "$key" 2>&1 >/dev/null)" || _rc=$? if [ "$_rc" -ne 0 ]; then echo runtime_error echo "herdr: 'agent rename' for '$team/$name' on pane '$id' failed (rc=$_rc)${_err:+: $_err}" >&2 @@ -1051,7 +1105,7 @@ terminal_name() { # non-zero here would therefore throw away the very thing the reordering above # exists to protect, for a decoration. if [ "$mode" != key ]; then - herdr pane rename "$id" "$label" >/dev/null 2>&1 || true + _herdr_cli "$id" pane rename "$(_herdr_bare_of "$id")" "$label" >/dev/null 2>&1 || true fi echo ok return 0 diff --git a/scripts/drivers/terminals/plain/ops.sh b/scripts/drivers/terminals/plain/ops.sh index b6eca4c45..7cc486c22 100644 --- a/scripts/drivers/terminals/plain/ops.sh +++ b/scripts/drivers/terminals/plain/ops.sh @@ -31,6 +31,12 @@ terminal_id_ok() { [ "$1" = '-' ] && return 0 _plain_parse_id "$1" } +# The id's two halves for the locator grammar: "\t". The legacy +# '-' sentinel names no place and is refused -- a locator must carry one. +terminal_id_split() { # + _plain_parse_id "$1" || return 1 + printf '%s\t%s\n' "$_PLAIN_EMULATOR" "$_PLAIN_TTY" +} terminal_describe() { printf 'name=plain\n' diff --git a/scripts/drivers/terminals/tmux/ops.sh b/scripts/drivers/terminals/tmux/ops.sh index 97bb3d430..b63e7c5c3 100644 --- a/scripts/drivers/terminals/tmux/ops.sh +++ b/scripts/drivers/terminals/tmux/ops.sh @@ -187,6 +187,15 @@ terminal_id_ok() { # # Run tmux against the server that owns . With no socket in the id this is # plain `tmux`, which is what a legacy record gets and what the ambient # environment decides — the honest behaviour for a ref that does not say. +# The id's two halves for the locator grammar: "\t<%N|@N>". A bare +# legacy id names no server and is refused -- a locator must carry one. +terminal_id_split() { # + local sock + terminal_id_ok "$1" || return 1 + sock="$(_tmux_sock_of "$1")" + [ -n "$sock" ] || return 1 + printf '%s\t%s\n' "$sock" "$(_tmux_bare_of "$1")" +} _tmux_do() { # local id="$1"; shift local sock; sock="$(_tmux_sock_of "$id")" diff --git a/scripts/lib/terminal-registry.sh b/scripts/lib/terminal-registry.sh index dad4f3266..dd57cdc65 100644 --- a/scripts/lib/terminal-registry.sh +++ b/scripts/lib/terminal-registry.sh @@ -1147,3 +1147,95 @@ agmsg_terminal_name_self_safe() { [ "$_restore_e" = 1 ] && set -e return "$_rc" } + +# --------------------------------------------------------------------------- +# Locators: the one grammar for "a pane, in an instance, of a kind" (#1055, #1152). +# +# :, where every driver's id is itself :: +# herdr:/run/jugemu.sock:w1:p7 instance = the server's socket path +# tmux:/tmp/tmux-501/default:%4 instance = the socket path +# plain:iterm:/dev/ttys040 instance = the emulator adapter name +# +# Pane ids repeat across instances (two herdr sessions both own a w1:p2; tmux +# has one id space per socket), so a pane id alone can name a live pane in +# another instance -- measured 2026-09-11 when a repair resolved in one session +# landed in another's pane. A locator carries the instance, and every reader +# of one goes through THIS split: four seats read locators today, and four +# parsers would disagree only after the fact. +# +# The registry owns the outer shape (kind + id) and asks the KIND's driver for +# the boundary inside its id (`terminal_id_split`): where a herdr id ends in +# two colon fields and a tmux or plain id in one is the driver's grammar, held +# once, in the driver. An instance may contain spaces; it may NOT contain a +# colon or a control character -- a socket path with a colon is legal on POSIX +# and is refused BY NAME (instance_malformed) rather than mis-split; the +# round-trippable encoding is deferred (#1166), and refusing is what keeps a +# mis-read from becoming a write. +# +# agmsg_locator_compose -> "::" rc 0 +# agmsg_locator_split -> "\t\t" rc 0 +# rc 2, nothing on stdout, one named reason on stderr: +# unknown_kind | instance_malformed | pane_malformed | id_malformed | locator_malformed +# (id_malformed: the kind's driver refused ":" as one id -- +# a colon inside a socket path, a bare pane with no instance, a pane outside +# the grammar; the registry does not guess which half) +# Neither function replaces the caller's loaded driver: the kind's driver is +# consulted through _agmsg_terminal_id_ok / _agmsg_terminal_id_split, which +# load it in a subshell when it is not the loaded one. + +_agmsg_locator_kind_ok() { # + case "$1" in ''|*[!A-Za-z0-9_-]*) return 1 ;; esac + [ -d "$(agmsg_terminal_dir "$1" 2>/dev/null)" ] +} + +_agmsg_locator_instance_ok() { # + case "$1" in ''|*:*|*[[:cntrl:]]*) return 1 ;; esac + return 0 +} + +# The kind's own split of its id: "\t", or rc 1 when the id is +# not a qualified one. Direct when that driver is loaded; a subshell load +# otherwise (the same posture as _agmsg_terminal_id_ok). +_agmsg_terminal_id_split() { # + local kind="$1" id="$2" + _agmsg_locator_kind_ok "$kind" || return 1 + if [ "$kind" = "$_AGMSG_TERMINAL_LOADED" ]; then + declare -F terminal_id_split >/dev/null 2>&1 || return 1 + terminal_id_split "$id" + else + ( agmsg_terminal_load "$kind" >/dev/null 2>&1 || exit 1 + declare -F terminal_id_split >/dev/null 2>&1 || exit 1 + terminal_id_split "$id" ) + fi +} + +agmsg_locator_compose() { # + local kind="$1" instance="$2" pane="$3" id + _agmsg_locator_kind_ok "$kind" || { echo "agmsg: locator: unknown_kind" >&2; return 2; } + _agmsg_locator_instance_ok "$instance" || { echo "agmsg: locator: instance_malformed" >&2; return 2; } + case "$pane" in ''|*[[:cntrl:]]*) echo "agmsg: locator: pane_malformed" >&2; return 2 ;; esac + id="$instance:$pane" + _agmsg_terminal_id_ok "$kind" "$id" || { echo "agmsg: locator: pane_malformed" >&2; return 2; } + # The driver must split it back into the same halves, or the grammar and the + # composition disagree about where the instance ends. + [ "$(_agmsg_terminal_id_split "$kind" "$id")" = "$(printf '%s\t%s' "$instance" "$pane")" ] \ + || { echo "agmsg: locator: pane_malformed" >&2; return 2; } + printf '%s:%s:%s\n' "$kind" "$instance" "$pane" +} + +agmsg_locator_split() { # + local loc="$1" kind id halves instance pane + case "$loc" in *[[:cntrl:]]*|'') echo "agmsg: locator: locator_malformed" >&2; return 2 ;; esac + case "$loc" in *:*) ;; *) echo "agmsg: locator: locator_malformed" >&2; return 2 ;; esac + kind="${loc%%:*}"; id="${loc#*:}" + _agmsg_locator_kind_ok "$kind" || { echo "agmsg: locator: unknown_kind" >&2; return 2; } + [ -n "$id" ] || { echo "agmsg: locator: locator_malformed" >&2; return 2; } + # The driver is the only judge of its own id. When it refuses, the registry + # cannot honestly say WHICH half is wrong (a plain instance is an enumerated + # adapter name, a herdr one a path), so the reason names the whole id. + _agmsg_terminal_id_ok "$kind" "$id" || { echo "agmsg: locator: id_malformed" >&2; return 2; } + halves="$(_agmsg_terminal_id_split "$kind" "$id")" || { echo "agmsg: locator: instance_malformed" >&2; return 2; } + instance="${halves%%$'\t'*}"; pane="${halves#*$'\t'}" + _agmsg_locator_instance_ok "$instance" || { echo "agmsg: locator: instance_malformed" >&2; return 2; } + printf '%s\t%s\t%s\n' "$kind" "$instance" "$pane" +} diff --git a/tests/test_herdr_cli_routing.bats b/tests/test_herdr_cli_routing.bats new file mode 100644 index 000000000..1a740ed7d --- /dev/null +++ b/tests/test_herdr_cli_routing.bats @@ -0,0 +1,106 @@ +#!/usr/bin/env bats +# .github/scripts/check-herdr-cli-routing.sh: every direct `herdr` call in the +# herdr driver is either routed through _herdr_cli or listed by name and +# count. Counted by call position, not by text. Runs in the bats suite as well +# as in CI so that a shard sees it (the same posture as test_unguarded_env_reads). + +load test_helper + +setup() { + setup_test_env + export SKILL_DIR="$TEST_SKILL_DIR" + CHECK="$SKILL_DIR/.github/scripts/check-herdr-cli-routing.sh" + OPS="$SKILL_DIR/scripts/drivers/terminals/herdr/ops.sh" + ALLOW="$SKILL_DIR/.github/herdr-cli-routing-allowlist" + # setup_test_env copies scripts/ but not .github/: bring the checker and its + # allowlist into the copy, so the mutations below touch nothing real + mkdir -p "$SKILL_DIR/.github/scripts" + cp "$BATS_TEST_DIRNAME/../.github/scripts/check-herdr-cli-routing.sh" "$CHECK" + cp "$BATS_TEST_DIRNAME/../.github/herdr-cli-routing-allowlist" "$ALLOW" +} +teardown() { teardown_test_env; } + +@test "routing: the real driver sits at its allowlist" { + run bash "$CHECK" + [ "$status" -eq 0 ] || { echo "$output" >&2; return 1; } +} + +@test "routing: a new direct call in a routed function is red, and names the function" { + printf '\nterminal_peek_extra() {\n herdr pane read "$1" >/dev/null\n}\n' >> "$OPS" + run bash "$CHECK" + [ "$status" -eq 1 ] + printf '%s\n' "$output" | grep -q 'terminal_peek_extra calls herdr directly 1 time(s) and is not on the allowlist' +} + +@test "routing: an extra direct call inside an allowlisted function is red (ABOVE)" { + # append a second raw call to the first allowlisted function's body + local fn; fn="$(awk '!/^#/ && NF {print $1; exit}' "$ALLOW")" + python3 - "$OPS" "$fn" <<'PY' +import sys,re +p,fn=sys.argv[1],sys.argv[2]; s=open(p).read() +i=s.index(fn+"()"); j=s.index("\n}\n",i) +s=s[:j]+'\n herdr pane list >/dev/null 2>&1 || true'+s[j:] +open(p,"w").write(s) +PY + run bash "$CHECK" + [ "$status" -eq 1 ] + printf '%s\n' "$output" | grep -q -- "$fn: .* -- ABOVE" +} + +@test "routing: a direct call that disappeared is red too (BELOW), and says to lower the entry" { + local fn; fn="$(awk '!/^#/ && NF {print $1; exit}' "$ALLOW")" + python3 - "$ALLOW" "$fn" <<'PY' +import sys +p,fn=sys.argv[1],sys.argv[2] +lines=open(p).read().splitlines(True) +out=[] +for l in lines: + if l.startswith(fn+" "): + n=int(l.split()[1]); l=f"{fn} {n+1}\n" # the list claims one MORE than exists == a call vanished + out.append(l) +open(p,"w").write("".join(out)) +PY + run bash "$CHECK" + [ "$status" -eq 1 ] + printf '%s\n' "$output" | grep -q -- "$fn: .* -- BELOW: lower the entry" +} + +@test "routing: a `herdr:` inside a message string and a `herdr` in a comment are NOT calls" { + printf '\n_herdr_noise() {\n # herdr pane get is mentioned here only\n echo "herdr: could not read pane" >&2\n printf "%%s\\n" "see: herdr pane list"\n}\n' >> "$OPS" + run bash "$CHECK" + [ "$status" -eq 0 ] || { echo "$output" >&2; return 1; } +} + +@test "routing: the routed driver reaches the id's socket for every pane-addressed op (argv+env logged)" { + export FAKEBIN="$SKILL_DIR/fakebin"; mkdir -p "$FAKEBIN" + export ARGV_LOG="$SKILL_DIR/argv.log"; : > "$ARGV_LOG" + export PATH="$FAKEBIN:$PATH" + cat > "$FAKEBIN/herdr" <<'FAKE' +#!/usr/bin/env bash +{ printf 'sock=%s |' "${HERDR_SOCKET_PATH:-}"; for a in "$@"; do printf ' [%s]' "$a"; done; printf '\n'; } >> "$ARGV_LOG" +case "$1 $2" in + "pane get") printf '{"result":{"pane":{"agent_status":"idle","label":"","terminal_title":"t","terminal_id":"term_X"}}}\n' ;; + "agent list") printf '{"id":"1","result":{"type":"list","agents":[]}}\n' ;; + "agent get") printf '{"result":{"agent":{"agent":"claude","agent_status":"idle"}}}\n' ;; + "pane layout") printf '{"result":{"layout":{}}}\n' ;; + *) : ;; +esac +exit 0 +FAKE + chmod +x "$FAKEBIN/herdr" + # shellcheck disable=SC1090 + source "$SKILL_DIR/scripts/lib/terminal-registry.sh"; agmsg_terminal_load herdr + local id="/run/other.sock:w1:p7" + HERDR_SOCKET_PATH=/run/ambient.sock terminal_peek "$id" >/dev/null 2>&1 || true + HERDR_SOCKET_PATH=/run/ambient.sock terminal_team_observe "$id" >/dev/null 2>&1 || true + HERDR_SOCKET_PATH=/run/ambient.sock terminal_team_input_ready "$id" claude >/dev/null 2>&1 || true + HERDR_SOCKET_PATH=/run/ambient.sock terminal_poke "$id" hello >/dev/null 2>&1 || true + HERDR_SOCKET_PATH=/run/ambient.sock terminal_label_of "$id" >/dev/null 2>&1 || true + HERDR_SOCKET_PATH=/run/ambient.sock terminal_where "$id" >/dev/null 2>&1 || true + HERDR_SOCKET_PATH=/run/ambient.sock terminal_name "$id" T alice >/dev/null 2>&1 || true + # every logged call went to the id's socket, with the BARE pane, never to the ambient one + [ "$(grep -c 'sock=/run/other.sock |' "$ARGV_LOG")" -ge 7 ] + refute grep -q 'sock=/run/ambient.sock' "$ARGV_LOG" + refute grep -q '/run/other.sock:w1:p7' "$ARGV_LOG" + grep -q 'sock=/run/other.sock | \[pane\] \[read\] \[w1:p7\]' "$ARGV_LOG" +} diff --git a/tests/test_locator.bats b/tests/test_locator.bats new file mode 100644 index 000000000..2fff8145d --- /dev/null +++ b/tests/test_locator.bats @@ -0,0 +1,156 @@ +#!/usr/bin/env bats +# The one locator grammar: :: (scripts/lib/terminal-registry.sh), +# and the herdr driver's socket-qualified id it rests on (#1055, #1152). + +load test_helper + +setup() { + setup_test_env + export SKILL_DIR="$TEST_SKILL_DIR" + # shellcheck disable=SC1090 + source "$SKILL_DIR/scripts/lib/terminal-registry.sh" + export FAKEBIN="$SKILL_DIR/fakebin"; mkdir -p "$FAKEBIN" + export ARGV_LOG="$SKILL_DIR/argv.log"; : > "$ARGV_LOG" + export PATH="$FAKEBIN:$PATH" + unset TMUX TMUX_PANE HERDR_SOCKET_PATH +} +teardown() { teardown_test_env; } + +# --- compose --------------------------------------------------------------------- + +@test "compose: herdr, tmux and plain locators, including a socket path with spaces" { + run agmsg_locator_compose herdr /run/herdr-a.sock w1:p7 + [ "$status" -eq 0 ]; [ "$output" = "herdr:/run/herdr-a.sock:w1:p7" ] + run agmsg_locator_compose tmux "/tmp/server with space" %4 + [ "$status" -eq 0 ]; [ "$output" = "tmux:/tmp/server with space:%4" ] + run agmsg_locator_compose plain iterm /dev/ttys040 + [ "$status" -eq 0 ]; [ "$output" = "plain:iterm:/dev/ttys040" ] +} + +@test "compose: refuses by NAME -- an instance with a colon, an unknown kind, a pane outside the grammar, an empty instance" { + # `run` folds stderr into $output, so the reason IS the output and stdout is empty + run agmsg_locator_compose herdr "/run/a:b.sock" w1:p7 + [ "$status" -eq 2 ]; [ "$output" = "agmsg: locator: instance_malformed" ] + run agmsg_locator_compose screen /tmp/x w1:p7 + [ "$status" -eq 2 ]; [ "$output" = "agmsg: locator: unknown_kind" ] + run agmsg_locator_compose herdr /run/herdr-a.sock "w1" + [ "$status" -eq 2 ]; [ "$output" = "agmsg: locator: pane_malformed" ] + run agmsg_locator_compose herdr "" w1:p7 + [ "$status" -eq 2 ]; [ "$output" = "agmsg: locator: instance_malformed" ] + [ -z "$(agmsg_locator_compose herdr "" w1:p7 2>/dev/null)" ] +} + +@test "compose: the reason lands on stderr, one word, and stdout stays empty" { + local err; err="$(agmsg_locator_compose herdr "/run/a:b.sock" w1:p7 2>&1 >/dev/null)" || true + [ "$err" = "agmsg: locator: instance_malformed" ] + err="$(agmsg_locator_compose nosuch /x w1:p7 2>&1 >/dev/null)" || true + [ "$err" = "agmsg: locator: unknown_kind" ] + err="$(agmsg_locator_compose tmux /x "w1:p7" 2>&1 >/dev/null)" || true + [ "$err" = "agmsg: locator: pane_malformed" ] +} + +# --- split ----------------------------------------------------------------------- + +@test "split: every composed locator round-trips, and the herdr pane keeps its own colon" { + local loc + for loc in "herdr:/run/herdr-a.sock:w1:p7" "tmux:/tmp/server with space:%4" "plain:iterm:/dev/ttys040" "tmux:/tmp/tmux-a:@12"; do + run agmsg_locator_split "$loc" + [ "$status" -eq 0 ] + case "$loc" in + herdr:*) [ "$output" = "$(printf 'herdr\t/run/herdr-a.sock\tw1:p7')" ] ;; + tmux:*space*) [ "$output" = "$(printf 'tmux\t/tmp/server with space\t%%4')" ] ;; + tmux:*) [ "$output" = "$(printf 'tmux\t/tmp/tmux-a\t@12')" ] ;; + plain:*) [ "$output" = "$(printf 'plain\titerm\t/dev/ttys040')" ] ;; + esac + # and composing the split gives the locator back + local k i p; k="${output%%$'\t'*}"; i="${output#*$'\t'}"; p="${i#*$'\t'}"; i="${i%%$'\t'*}" + [ "$(agmsg_locator_compose "$k" "$i" "$p")" = "$loc" ] + done +} + +@test "split: an instance with a colon is refused by name, never mis-split into a different pane" { + # /run/a:b.sock:w1:p7 -- a naive right split would read instance=/run/a, pane=b.sock:w1:p7 or worse + run agmsg_locator_split "herdr:/run/a:b.sock:w1:p7" + [ "$status" -eq 2 ]; [ "$output" = "agmsg: locator: id_malformed" ] + [ -z "$(agmsg_locator_split "herdr:/run/a:b.sock:w1:p7" 2>/dev/null)" ] +} + +@test "split: unknown kind, no instance, bare pane and control characters are refused by name" { + local err + err="$(agmsg_locator_split "screen:/x:w1:p7" 2>&1 >/dev/null)" || true; [ "$err" = "agmsg: locator: unknown_kind" ] + # a bare herdr pane is a VALID driver id with no instance: the registry can tell, and says so + err="$(agmsg_locator_split "herdr:w1:p7" 2>&1 >/dev/null)" || true; [ "$err" = "agmsg: locator: instance_malformed" ] + err="$(agmsg_locator_split "plain:/dev/ttys040" 2>&1 >/dev/null)" || true; [ "$err" = "agmsg: locator: id_malformed" ] + # the legacy plain sentinel is a valid driver id that names no instance: the registry can tell + err="$(agmsg_locator_split "plain:-" 2>&1 >/dev/null)" || true; [ "$err" = "agmsg: locator: instance_malformed" ] + err="$(agmsg_locator_split "herdr:/x:w1" 2>&1 >/dev/null)" || true; [ "$err" = "agmsg: locator: id_malformed" ] + err="$(agmsg_locator_split "$(printf 'herdr:/x\n:w1:p7')" 2>&1 >/dev/null)" || true; [ "$err" = "agmsg: locator: locator_malformed" ] + err="$(agmsg_locator_split "" 2>&1 >/dev/null)" || true; [ "$err" = "agmsg: locator: locator_malformed" ] +} + +@test "split: the tmux legacy pane id inside a locator is still a pane, and the loaded driver is not replaced" { + agmsg_terminal_load tmux + run agmsg_locator_split "herdr:/run/herdr-a.sock:w1:p7" + [ "$status" -eq 0 ] + [ "$_AGMSG_TERMINAL_LOADED" = tmux ] + declare -F terminal_id_ok >/dev/null + terminal_id_ok "%4" # still tmux's grammar +} + +@test "split: the herdr split relies on the pane grammar having exactly ONE colon -- a three-field pane is refused, never split elsewhere" { + # `_herdr_sock_of` takes the trailing two colon fields as the pane. That is + # only right while `w…:p…` has exactly one colon, which terminal_id_ok + # enforces. If the pane grammar ever grows a field, this is the test that + # notices: the value must be REFUSED, not silently split one field to the left. + run agmsg_locator_split "herdr:/run/herdr-a.sock:w1:p7:x9" + [ "$status" -eq 2 ] + [ "$output" = "agmsg: locator: id_malformed" ] + agmsg_terminal_load herdr + refute terminal_id_ok "/run/herdr-a.sock:w1:p7:x9" + refute terminal_id_split "/run/herdr-a.sock:w1:p7:x9" +} + +# --- the herdr id: bare or socket-qualified ------------------------------------------- + +_fake_herdr_env_logger() { + cat > "$FAKEBIN/herdr" <<'FAKE' +#!/usr/bin/env bash +{ printf 'sock=%s |' "${HERDR_SOCKET_PATH:-}"; for a in "$@"; do printf ' [%s]' "$a"; done; printf '\n'; } >> "$ARGV_LOG" +printf '{"result":{"pane":{"agent_status":"idle","label":"","terminal_title":"t","terminal_id":"term_X"}}}\n' +FAKE + chmod +x "$FAKEBIN/herdr" +} + +@test "herdr id grammar: bare and socket-qualified ids are accepted; a colon or control char in the socket is refused" { + agmsg_terminal_load herdr + terminal_id_ok "w1:p7" + terminal_id_ok "/run/herdr-a.sock:w1:p7" + terminal_id_ok "/tmp/server with space:w1:pB" + refute terminal_id_ok "/run/a:b.sock:w1:p7" + refute terminal_id_ok "$(printf '/run/x\n:w1:p7')" + refute terminal_id_ok ":w1:p7" + refute terminal_id_ok "w1" + [ "$(_herdr_sock_of "/run/herdr-a.sock:w1:p7")" = "/run/herdr-a.sock" ] + [ "$(_herdr_bare_of "/run/herdr-a.sock:w1:p7")" = "w1:p7" ] + [ "$(_herdr_sock_of "w1:p7")" = "" ] + [ "$(_herdr_bare_of "w1:p7")" = "w1:p7" ] +} + +@test "herdr _herdr_cli: a qualified id reaches ITS socket and the CLI is given the bare pane; a bare id keeps the ambient socket" { + _fake_herdr_env_logger + agmsg_terminal_load herdr + _herdr_cli "/run/herdr-a.sock:w1:p7" pane get w1:p7 >/dev/null + grep -Fqx 'sock=/run/herdr-a.sock | [pane] [get] [w1:p7]' "$ARGV_LOG" + : > "$ARGV_LOG" + HERDR_SOCKET_PATH=/run/ambient.sock _herdr_cli "w1:p7" pane get w1:p7 >/dev/null + grep -Fqx 'sock=/run/ambient.sock | [pane] [get] [w1:p7]' "$ARGV_LOG" + : > "$ARGV_LOG" + HERDR_SOCKET_PATH=/run/ambient.sock _herdr_cli "/run/other.sock:w1:p7" pane get w1:p7 >/dev/null + grep -Fqx 'sock=/run/other.sock | [pane] [get] [w1:p7]' "$ARGV_LOG" # the id wins over the environment +} + +@test "registry: _agmsg_terminal_id_ok for herdr follows the qualified grammar" { + _agmsg_terminal_id_ok herdr "/run/herdr-a.sock:w1:p7" + refute _agmsg_terminal_id_ok herdr "/run/a:b.sock:w1:p7" + _agmsg_terminal_id_ok herdr "w1:p7" +}