fix(psmux): refuse an attached-client count that names another session

CHANGELOG.md

@@ -52,6 +52,12 @@ breaking changes may land in a minor release.
   already taken a read-only target's READONLY bit when the replace was denied, and Windows
   refuses to delete a READONLY file, so the cleanup was denied too and the temp survived.
   It now clears the bit and retries.
+- Refuse a psmux attached-client count whose answer names a different session, so a duplicated
+  registry entry can no longer vouch for a `switch-client` that moved nobody — which
+  `return_attached_client` reported as `RETURNED`, clearing the return option for a human still
+  sitting there. Also refuse an empty or `:`-bearing session name before the probe spawns. A
+  same-named session on a foreign server, and one differing only by whitespace the seam
+  normalizes away, stay #531's subject (#671)
 
 ### Security
 

src/bmad_loop/adapters/psmux_backend.py

@@ -44,9 +44,14 @@
 see ``_LAST_UNSUPPORTED`` for what the floor buys and why it moves.
 ``has_session``
 is inherited unchanged, but one server per session gives it a residual the
-tmux path does not have: a ``-t`` read naming a session whose own server is
-gone can be answered by a different server, so a wrong ``True`` is reachable
-when a same-named session exists on a foreign one. For the lost-session probe
+tmux path does not have: a ``-t`` read resolves through the named port and key
+files, so a wrong ``True`` is reachable whenever that PAIR addresses some OTHER
+live server. Both halves are needed — the server rejects a key mismatch before
+running anything — so a merely recycled port does not reach it; a duplicated
+registry entry or a same-named session on a foreign server does. A server that
+is simply *gone* fails closed: measured on 3.3.8, both a removed port file and
+a stale one whose process was killed answer rc 1, ``no server running on
+session``. For the lost-session probe
 (#489) that is the safe direction — a wrong ``True`` drops the diagnosis
 rather than inventing one — and the collision it needs is an independently
 created session sharing a ``bmad-loop-<run-id>`` name: an operator's, or
@@ -842,13 +847,16 @@ def current_return_target(self) -> str | None:
     #
     # Two costs are taken deliberately here, since both are the kind that go
     # unnoticed. The gate IS an absolute count, which the delta rule below
-    # forbids for good reason: the same server-fallback that makes "zero
-    # attached" meaningless (#315) can hand back a FOREIGN session's nonzero
-    # count, and a switch that exits 0 for its own reasons would then read as
-    # vouched-for. It is admitted only because the alternative — the delta — is
-    # blind to the same-session move this verb's main caller performs, and the
-    # damage directions are not equal: a wrong True here costs one unverified
-    # hand-back claim, where the delta cost a relocated human. And the rc rule
+    # forbids for good reason: a misrouted read (#315) can hand back a FOREIGN
+    # session's nonzero count, and a switch that exits 0 for its own reasons
+    # would then read as vouched-for. _attached_clients now refuses the answer
+    # that does not NAME the session it was asked about, so what is left is a
+    # foreign server whose session carries the same name — the #531 collision,
+    # which no read of a name can separate. It is admitted only because the
+    # alternative — the delta — is blind to the same-session move this verb's
+    # main caller performs, and the damage directions are not equal: a wrong
+    # True here costs one unverified hand-back claim, where the delta cost a
+    # relocated human. And the rc rule
     # assumes the admitted floor: psmux/psmux#483 lands in 3.3.8, so on a build
     # forced past ``available()`` (an explicit backend override) a ``-t`` that
     # moves nobody still exits 0 and is answered True.
@@ -858,10 +866,14 @@ def current_return_target(self) -> str | None:
     # Neither can use rc — ``-l`` has no target form and carries no server reply
     # at all, and ``detach-client`` exits 0 with zero clients attached (a
     # flag-less detach is promoted server-side to detach-all). Never an absolute
-    # count: a read against a session whose server is gone answers from whichever
-    # server the fallback picks, so "zero attached" on its own proves nothing
-    # (#315). A drop cannot be manufactured that way — it needs two successful
-    # reads of the same session, the first of them nonzero.
+    # count: a misrouted read answers for a session nobody asked about, so "zero
+    # attached" on its own proves nothing (#315) — and the identity compare in
+    # _attached_clients narrows that to a same-name foreign server rather than
+    # closing it. Against a DIFFERENTLY-named server the delta is additionally
+    # safe: manufacturing a drop needs two successful reads, and the second one
+    # is refused by name. Against a same-NAME one it is not — that server's own
+    # client detaching between the two reads is a real drop, of a real client,
+    # in the wrong session. #531 is the only thing that closes it.
     #
     # The delta is why ``-t`` could not stay on it (#659): a switch whose target
     # lives in THIS session moves the client between windows without changing the
@@ -879,20 +891,71 @@ def current_return_target(self) -> str | None:
     def _attached_clients(self, session: str) -> int | None:
         """Clients attached to ``session``, or None when psmux cannot say.
 
-        Self-detecting on purpose: an unsupported format field cannot answer a
-        plausible integer, so a psmux build that does not carry
-        ``#{session_attached}`` degrades to None instead of a wrong count.
+        Self-detecting on purpose, in two directions. An unsupported format
+        field cannot answer a plausible integer, so a build that does not carry
+        ``#{session_attached}`` degrades to None instead of a wrong count. And
+        the read reports the session it answered FOR: ``-t`` resolves through
+        the named port AND key files, so when that pair addresses some OTHER
+        live server the answer is that server's — rc 0, plausible count, wrong
+        session (measured on 3.3.8: port and key copied from a live session
+        answered ``0|alive`` for ``-t forged``, naming itself). Both files are
+        required — a key mismatch is rejected before the command runs — so this
+        is a duplicated registry entry, not a merely recycled port. Comparing
+        the answered name against the requested one is what refuses it. The seam's
+        ``={session}`` token cannot: psmux strips the ``=`` before it routes
+        anything (``parse_target`` → ``strip_exact_match_prefix``), so both
+        spellings were byte-identical in every probed state. What survives the
+        compare is a genuine same-NAME session on a foreign server, which no
+        name can separate — see the module docstring and #531.
+
+        The count comes FIRST in the format so the split is unambiguous: it is
+        all digits and so cannot contain the separator, which leaves the name
+        as the whole remainder even when the name contains one.
         """
+        # The #221 rule the sibling call sites already apply (see
+        # current_return_target): `a:b` parses as session `a`, window `b`, so a
+        # `:`-bearing name addresses something else entirely. Refuse before
+        # spawning rather than trusting what comes back.
+        if not session or ":" in session:
+            return None
         try:
             proc = self._run(
-                ["display-message", "-p", "-t", session, "#{session_attached}"], check=False
+                ["display-message", "-p", "-t", session, "#{session_attached}|#{session_name}"],
+                check=False,
             )
         except (subprocess.SubprocessError, OSError):
             return None
         if proc.returncode != 0:
             return None
-        text = proc.stdout.strip()
-        return int(text) if text.isdigit() else None
+        # `session` reaches here from `current_session`, which is
+        # `_display_message` and therefore ALREADY `.strip()`ped, so the name we
+        # ask with is the normalized one. That bounds what this compare can and
+        # cannot do, in both directions:
+        #
+        #   - A session whose REAL name carries surrounding whitespace is
+        #     unreachable through the normalized name anyway — the registry is
+        #     one file per name, so `-t ctl` finds no `ctl.port` for a session
+        #     named `" ctl"` and fails closed at rc 1. There is no own-count case
+        #     to protect here, which is why the answer is stripped the same way
+        #     rather than preserved.
+        #   - The converse is the hazard: with a real `"ctl"` also present,
+        #     `-t ctl` reaches THAT session and its count passes the compare.
+        #     Measured on 3.3.8 — `" ctl"` and `"ctl"` coexist as ` ctl.port` and
+        #     `ctl.port`, and the read answers `0|ctl`. A name cannot separate
+        #     names the seam has already merged; that is the #531 family, the
+        #     same ceiling the same-name foreign server sits under.
+        #
+        # Control characters need no handling: psmux escapes them in format
+        # output, so a session whose name ends in a real carriage return answers
+        # with the literal characters `\` and `r` and fails the compare like any
+        # other mismatch. Measured on 3.3.8 through the hardest route available —
+        # duplicate a live session's port and key under a second name, then
+        # rename the original to a CR-bearing name so the surviving alias reaches
+        # a server whose in-memory name differs only by the control character.
+        count, _, answered = proc.stdout.strip().partition("|")
+        if answered != session or not count.isdigit():
+            return None
+        return int(count)
 
     def _client_left(self, verb: list[str]) -> bool | None:
         """Run a client verb and answer whether a client left this session —

tests/test_psmux_backend.py

@@ -1643,14 +1643,35 @@ def fake(argv, **kwargs):
 # ---------------------------------------- client verbs: observed effect (#317)
 
 
-def _client_fake(monkeypatch, *, attached, session="ctl", verb_rc=0, drains_on_switch=False):
+#: The one format the attached-count probe asks for. The session NAME rides
+#: along with the count so the answer identifies which session it is about —
+#: a `-t` read can be served by another server (gh-671).
+ATTACHED_FMT = "#{session_attached}|#{session_name}"
+
+
+def _client_fake(
+    monkeypatch,
+    *,
+    attached,
+    session="ctl",
+    verb_rc=0,
+    drains_on_switch=False,
+    answered_session=None,
+):
     """Script the two probes the client verbs measure with.
 
-    ``attached`` is the successive answers to ``#{session_attached}``, consumed
-    in call order. The delta verbs (``-l``, ``detach-client``) take two each — a
-    detach that worked is ["1", "0"] and psmux's rc-0 no-op is ["1", "1"] — while
-    ``switch-client -t`` takes ONE, the gate count read before it. Every verb
-    exits ``verb_rc`` (0 by default), which only the ``-t`` leg reads.
+    ``attached`` is the successive COUNTS the probe answers, consumed in call
+    order; the harness pairs each with the answering session name, exactly as
+    ``ATTACHED_FMT`` makes psmux do. The delta verbs (``-l``, ``detach-client``)
+    take two each — a detach that worked is ["1", "0"] and psmux's rc-0 no-op is
+    ["1", "1"] — while ``switch-client -t`` takes ONE, the gate count read before
+    it. Every verb exits ``verb_rc`` (0 by default), which only the ``-t`` leg
+    reads.
+
+    ``answered_session`` is the name the probe REPORTS, defaulting to the
+    session we are in. Setting it apart from ``session`` is the misrouted read
+    (gh-671): another server answering at rc 0 with a plausible count of its
+    own.
 
     ``drains_on_switch`` makes the count answer ``"0"`` from the moment a
     ``switch-client`` has been seen, i.e. the session empties BECAUSE the verb
@@ -1661,16 +1682,17 @@ def _client_fake(monkeypatch, *, attached, session="ctl", verb_rc=0, drains_on_s
     monkeypatch.setenv("TMUX", "/tmp/psmux-1000/default,123,0")  # inside a pane
     monkeypatch.setenv("TMUX_PANE", "%9")  # the pane the probes pin to (gh-669)
     counts = list(attached)
+    answered = session if answered_session is None else answered_session
     calls: list[list] = []
 
     def fake(argv, **kwargs):
         calls.append(list(argv))
         if argv[1] == "display-message" and argv[-1] == "#{session_name}":
             return subprocess.CompletedProcess(argv, 0, stdout=f"{session}\n", stderr="")
-        if argv[1] == "display-message" and argv[-1] == "#{session_attached}":
+        if argv[1] == "display-message" and argv[-1] == ATTACHED_FMT:
             drained = drains_on_switch and any(c[1] == "switch-client" for c in calls[:-1])
             answer = "0" if drained else counts.pop(0)
-            return subprocess.CompletedProcess(argv, 0, stdout=f"{answer}\n", stderr="")
+            return subprocess.CompletedProcess(argv, 0, stdout=f"{answer}|{answered}\n", stderr="")
         return subprocess.CompletedProcess(argv, verb_rc, stdout="", stderr="")
 
     monkeypatch.setattr(tmux_base.subprocess, "run", fake)
@@ -1682,7 +1704,7 @@ def test_psmux_detach_reports_the_observed_drop(monkeypatch):
     assert PsmuxMultiplexer().detach_client() is True
     assert ["psmux", "detach-client"] in calls
     # both probes routed to the session, never left to the most-recent fallback
-    assert calls.count(["psmux", "display-message", "-p", "-t", "ctl", "#{session_attached}"]) == 2
+    assert calls.count(["psmux", "display-message", "-p", "-t", "ctl", ATTACHED_FMT]) == 2
 
 
 def test_psmux_detach_with_nothing_attached_is_false(monkeypatch):
@@ -1743,7 +1765,7 @@ def test_psmux_switch_reports_a_cross_session_move(monkeypatch):
     assert not any(c[1:3] == ["switch-client", "-l"] for c in calls)
     # One gate read, routed at this session — the #315-safe shape the detach
     # tests pin for the delta legs, and the arity a re-added `after` read breaks.
-    assert calls.count(["psmux", "display-message", "-p", "-t", "ctl", "#{session_attached}"]) == 1
+    assert calls.count(["psmux", "display-message", "-p", "-t", "ctl", ATTACHED_FMT]) == 1
 
 
 def test_psmux_switch_with_nothing_attached_cannot_vouch(monkeypatch):
@@ -2025,3 +2047,106 @@ def boom(argv, **kwargs):
     monkeypatch.setattr(tmux_base.subprocess, "run", boom)
     assert PsmuxMultiplexer().switch_client("ctl:%9", last_fallback=True) is None
     assert dispatched == [["psmux", "switch-client", "-l"]]
+
+
+# ------------------------------- attached-count identity (gh-671)
+
+
+def _count_fake(monkeypatch, *, rc=0, stdout=""):
+    """Answer whatever the attached-count probe asks with one canned reply, and
+    record every spawn — including the spawns that never happen, which is what
+    the pre-flight refusals are asserted on."""
+    calls: list[list] = []
+
+    def fake(argv, **kwargs):
+        calls.append(list(argv))
+        return subprocess.CompletedProcess(argv, rc, stdout=stdout, stderr="")
+
+    monkeypatch.setattr(tmux_base.subprocess, "run", fake)
+    return calls
+
+
+def test_attached_count_reads_its_own_session(monkeypatch):
+    calls = _count_fake(monkeypatch, stdout="2|ctl\n")
+    assert PsmuxMultiplexer()._attached_clients("ctl") == 2
+    assert calls == [["psmux", "display-message", "-p", "-t", "ctl", ATTACHED_FMT]]
+
+
+def test_attached_count_refuses_a_differently_named_sessions_answer(monkeypatch):
+    """A `-t` read resolves through the named port AND key files, so when that
+    pair addresses another LIVE server the answer is that server's: rc 0, a
+    plausible count, and a session nobody asked about. Measured on 3.3.8; both
+    files are needed, since a key mismatch is rejected before the command runs.
+    The name riding along with the count is the only thing that separates such
+    an answer from our own."""
+    _count_fake(monkeypatch, stdout="1|alive\n")
+    assert PsmuxMultiplexer()._attached_clients("ctl") is None
+
+
+def test_attached_count_refuses_an_unnamed_answer(monkeypatch):
+    """An empty name field is an absent identity, not a matching one."""
+    _count_fake(monkeypatch, stdout="2|\n")
+    assert PsmuxMultiplexer()._attached_clients("ctl") is None
+
+
+def test_attached_count_refuses_a_dead_server(monkeypatch):
+    """Both a removed port file and a stale one whose process was killed answer
+    `no server running on session` at rc 1 — measured, and the direction that
+    was already safe. Pinned so the identity compare cannot mask a regression
+    into rc 0 here.
+
+    The stdout is a WELL-FORMED matching answer on purpose: with an empty one
+    the parser refuses for its own reasons, so deleting the rc guard would leave
+    this test green and pin nothing."""
+    _count_fake(monkeypatch, rc=1, stdout="2|ctl\n")
+    assert PsmuxMultiplexer()._attached_clients("ctl") is None
+
+
+def test_attached_count_survives_a_separator_inside_the_session_name(monkeypatch):
+    """The count is all digits and so cannot contain the separator: splitting
+    once leaves the name whole, however many separators it carries."""
+    _count_fake(monkeypatch, stdout="2|we|ird\n")
+    assert PsmuxMultiplexer()._attached_clients("we|ird") == 2
+
+
+def test_attached_count_refuses_a_colon_bearing_session_without_spawning(monkeypatch):
+    """`a:b` parses as session `a`, window `b` (#221), so the read would address
+    something else entirely — the rule current_return_target already applies."""
+    calls = _count_fake(monkeypatch, stdout="2|foo\n")
+    assert PsmuxMultiplexer()._attached_clients("foo:bar") is None
+    assert calls == []
+
+
+def test_attached_count_refuses_an_empty_session_without_spawning(monkeypatch):
+    calls = _count_fake(monkeypatch, stdout="2|\n")
+    assert PsmuxMultiplexer()._attached_clients("") is None
+    assert calls == []
+
+
+def test_switch_client_will_not_vouch_with_another_sessions_count(monkeypatch):
+    """The gh-671 hazard at the verdict layer. Since #670 the gate count is read
+    in the NONZERO direction, so a borrowed nonzero count would vouch for an
+    rc-0 switch that moved nobody — a vacuous True that
+    tui.launch.return_attached_client turns into RETURNED and a cleared return
+    option. Unvouched must answer None, never True."""
+    _client_fake(monkeypatch, attached=["1"], answered_session="alive")
+    assert PsmuxMultiplexer().switch_client("ctl:%9") is None
+
+
+def test_detach_verdict_refuses_a_borrowed_delta(monkeypatch):
+    """The same substitution one layer down: a drop read off another session's
+    counts is not our client leaving."""
+    _client_fake(monkeypatch, attached=["1", "0"], answered_session="alive")
+    assert PsmuxMultiplexer().detach_client() is False
+
+
+def test_attached_count_refuses_a_name_differing_only_by_whitespace(monkeypatch):
+    """`" ctl"` is not `"ctl"`: different registry entries, different servers.
+    The requested name arrives pre-`.strip()`ped (`current_session` is
+    `_display_message`), so this is the shape a whitespace-bearing session takes
+    when some OTHER server answers for the normalized name — refuse it, exactly
+    as any other differing name. The reverse direction needs no guard: a session
+    really named `" ctl"` has no `ctl.port`, so the read fails closed at rc 1
+    before any compare."""
+    _count_fake(monkeypatch, stdout="1| ctl\n")
+    assert PsmuxMultiplexer()._attached_clients("ctl") is None