fix(tui): record session-qualified parked-window return target

[dracic]

Summary

An interactive attach recorded the client's origin as a bare pane id (%N) and replayed it as switch-client -t %N from inside the control session. Sound under tmux's one-server model, but on psmux (one server per session, upstream-final per psmux/psmux#483) a bare id is session-local: at best unresolvable, at worst colliding with a real control-session pane and landing the client on the wrong one with exit 0 — past the switch-client -l fallback.

This is candidate B from the issue thread: the recording seam (tui/launch.py) now emits =session:%N when both probes succeed, retaining bare %N only if the session-name probe fails (a resolvable own pane means the client IS inside the multiplexer, so recording detach would strand it). An rc-0-but-empty pane probe is rejected before composing. The replay sides — POSIX and pwsh trailers, switch_client, return_attached_client — treat the value as an opaque target and are unchanged beyond comment accuracy. RETURN_DETACH stays collision-free.

Verification

• tmux 3.4 (WSL): =sess:%N parses and resolves to the exact pane; switch-client accepts the form. tmux resolves the qualified form to the same pane a bare id always reached.
• psmux side is source-verified against psmux main (d56d777 + 6c76ff9, see the issue thread); live confirmation stays on the Live re-verification of switch-client paths on the next psmux release (post psmux#483 fix) #222 checklist — psmux 3.3.7 (the only release passing the version gate) no-ops all window/pane targets by design, so nothing regresses there.
• Windows targeted suite (launch/cli/tui-app/multiplexer/psmux): 457 passed, 1 known baseline failure (skill-sync drift). Full WSL suite: failures identical to a clean checkout of main (stash-compared), none introduced.
• Recording is covered by probe-composition tests (qualified, outside-mux, transport failure, empty pane, failed/empty session name); the replay fixture now round-trips the qualified value.

Closes #221

Summary by CodeRabbit

• Bug Fixes
  • Fixed return-pane selection after interactive attaches to use session-qualified return targets when available.
  • Avoided cross-session misrouting when pane identifiers overlap between sessions.
  • Kept safe fallback behavior when session information can’t be determined reliably, ensuring correct tmux/psmux return from parked windows.
• Documentation
  • Updated the adapter authoring guide with clarified parked-window return target and pane identifier formatting rules.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 9cb77c53-ed1a-42e9-892f-39b550a4fbb3

📥 Commits

Reviewing files that changed from the base of the PR and between a0ec7a1 and 0a427e5.

📒 Files selected for processing (8)

• CHANGELOG.md
• docs/adapter-authoring-guide.md
• src/bmad_loop/adapters/multiplexer.py
• src/bmad_loop/adapters/psmux_backend.py
• src/bmad_loop/adapters/tmux_base.py
• src/bmad_loop/tui/launch.py
• tests/test_psmux_backend.py
• tests/test_tui_launch.py

🚧 Files skipped from review as they are similar to previous changes (4)

• CHANGELOG.md
• src/bmad_loop/tui/launch.py
• docs/adapter-authoring-guide.md
• src/bmad_loop/adapters/tmux_base.py

---

Walkthrough

Attach return handling now uses backend-composed targets, including session-qualified psmux targets such as =session:%N, with bare-pane fallback when qualification is unavailable. CLI and TUI flows, replay behavior, documentation, and tests were updated.

Changes

Session-qualified return targets

Layer / File(s)	Summary
Return-target contract and resolution src/bmad_loop/adapters/multiplexer.py, src/bmad_loop/adapters/psmux_backend.py, src/bmad_loop/tui/launch.py	Backends provide opaque return targets; psmux combines valid session and pane probes, while launch stores the result or detach sentinel.
Attach recording and parked-window replay src/bmad_loop/cli.py, src/bmad_loop/tui/app.py, src/bmad_loop/adapters/tmux_base.py, docs/adapter-authoring-guide.md, CHANGELOG.md	Attach paths record resolved targets, and documentation describes backend-specific target composition and replay fallback behavior.
Return-target and replay tests tests/test_cli.py, tests/test_tui_app.py, tests/test_tui_launch.py, tests/test_psmux_backend.py	Tests cover qualified targets, bare fallbacks, probe failures, attach stamps, and client switching.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant AttachFlow
  participant Launch
  participant ControlWindow
  participant MultiplexerClient
  AttachFlow->>Launch: current_return_target()
  Launch-->>AttachFlow: qualified or bare target
  AttachFlow->>ControlWindow: set_return_pane(target)
  ControlWindow->>MultiplexerClient: switch-client -t target
  MultiplexerClient-->>ControlWindow: switch result or -l fallback

Loading

Possibly related issues

• Live re-verification of switch-client paths on the next psmux release (post psmux#483 fix) #222 — Requests live verification of the session-qualified targets and fallback paths introduced here.

Possibly related PRs

• bmad-code-org/bmad-loop#47 — Refactors the parked-window trailer handling that replays the recorded return target.
• bmad-code-org/bmad-loop#181 — Introduces the psmux backend semantics extended by this return-target change.

Suggested reviewers: pbean

Poem

A rabbit stamps a target bright,
Session-bound to guide it right.
Bare panes wait when probes fail,
The parked window hops the trail.
Tmux smiles beneath moonlight.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 19.44% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly describes the main change: recording a session-qualified parked-window return target.
Linked Issues check	✅ Passed	The PR updates attach and replay paths to use backend-composed return targets, with psmux session-qualified fallback behavior and RETURN_DETACH preserved.
Out of Scope Changes check	✅ Passed	The changes are limited to the return-target fix, with docs, tests, and comments supporting the same behavior.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[dracic]

Ran a multi-aspect review of this branch (general correctness, test coverage, comment accuracy, silent-failure audit). Verdict: the core change is sound — all degradation branches of current_return_target() are covered by tests at the subprocess.run seam, the replay sides genuinely treat the recorded value as opaque, and no caller of the old launch.current_pane_id() was missed. The = prefix is also a quiet correctness upgrade: it forces exact session match where tmux would otherwise pattern-match.

One finding was addressed in a0ec7a1:

• Seam violation: current_return_target() hand-assembled =session:%N with an f-string, contradicting the adapter guide's contract that core never formats the target grammar itself, and bypassing backend target() overrides. Now routed through mux.target(session, pane) (byte-identical output for the tmux family), with a guide note that this caller carries a pane id in the window slot. A few comment-precision fixes rode along (the RETURN_OPTION block now names the bare-%N degradation; the parked-trailer comment distinguishes the option name from its recorded value).

Verified safe (no action): session names cannot contain :/. under tmux so the qualified form cannot misparse; a session literally named detach yields =detach:%N, which the trailer's equality test cannot match; the rc-0-empty-stdout probe answers degrade identically to rc!=0 and both are pinned by tests.

Two pre-existing adjacent issues surfaced by the audit are filed separately as follow-ups (ignored switch_client result in return_attached_client, and the pre-#483-fix psmux rc-0 no-op having no version gate) — they are out of this PR's scope.

ruff check clean; affected test files pass on Windows (434 passed, 1 known skill-sync-drift baseline failure unrelated to this diff).

[pbean]

@CodeRabbit review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[pbean]

Ran this through a full review with live verification: scratch tmux server with a real pty-attached client, tmux source at 3.4 and master, psmux cli.rs at master, and the herdr adapter source. Verdict: the recording-side structure is right, but =session:%N regresses the tmux path this PR promises to leave intact, and breaks the herdr adapter's return hop. Since maintainer edits are enabled I'll push a fix commit onto this branch (backend-owned composition — sketch at the end) rather than bounce it; review of that commit welcome.

1. tmux rejects =session:%N at the replay verb

$ tmux -V
tmux 3.7b
# scratch -L server: sessions ctl + main, real client attached to ctl via a pty
$ tmux -L S switch-client -t "=main:%1"
can't find window: %1        # rc=1, client does not move
$ tmux -L S switch-client -t "%1"
                             # rc=0, client lands on main:%1 (the pre-PR form)

Source-confirmed in 3.4 and master (cmd-find.c): with a colon and no period, cmd_find_target puts the post-colon part in the window slot unconditionally — the find type never matters there — and cmd_find_get_window_with_session has no % handling at all: it tries @ ids, +/- offsets, !/^/$, numeric index, then exact/prefix/fnmatch name matching, then returns -1 → no_window: → can't find window. The pane-id checks exist only in the pane helpers (reached via a .-separated pane part or the no-separator branch).

Consequence: every parked-window return on tmux now exits 1 and rides the switch-client -l fallback — origin session usually recovered, exact-pane return gone — and the client hard-strands in the control session whenever -l has nothing to fall back to (e.g. the client was attached directly into the ctl session, so it has no last session).

Why the PR's tmux 3.4 verification passed anyway — two independent false positives:

• display-message -p -t '=main:%N' '#{pane_id}' answers the right pane, but not because the target resolves. Its target is declared { 't', CMD_FIND_PANE, CMD_FIND_CANFAIL }, and cmd_find_target's error: path under CANFAIL return (0) with the partially-filled state: the session half already resolved, and cmd_find_get_window_with_session seeds fs->wl = fs->s->curw before failing. So the answer is "the session's current pane" — indistinguishable from correct resolution precisely when the probed pane is that session's active pane, i.e. exactly the by-hand test from your own pane. switch-client (flags = 0) takes the strict return (-1) and errors. Reproduced both against the same server and target string.
• End-to-end, the -l fallback usually lands near the right place (the origin session's curw is where the user left), masking the rc=1.

CI stayed green because every touched test scripts subprocess.run with rc=0 — nothing exercises real target resolution.

2. herdr's return hop breaks

herdr's current_pane_id() is a native id (w1:p1). This branch records target(label, "w1:p1") = =label:w1:p1; herdr switch_client → _parse_target → multiplexer.parse_target yields ("label", "w1:p1") and then looks for a tab labeled w1:p1 → None → silent False. Today's bare native id passes parse_target untouched (no leading =) and works. The guide note added here ("a backend … must tolerate a pane id in the window slot") is a contract change the shipped herdr adapter does not satisfy.

3. No uniform composition exists

• =sess:%N — precise on psmux (parse_target: post-colon %N sets pane_is_id), hard error on tmux (above).
• =sess:.%N — precise on tmux (verified live: rc=0, exact pane — the empty window slot routes %N through cmd_find_get_pane_with_session, which does handle ids), but psmux silently drops the pane: .%N goes down the dot-split path and "%N".parse::<usize>() fails, leaving session-only.
• bare %N — precise on tmux and herdr, broken on psmux (#221 itself).

So the issue's constraint — "a single seam-level recording change beats per-backend branching if the form parses on both" — fails its own premise. The composition has to be backend-owned.

Fix being pushed

The recorded value is written to and replayed from the same backend's server, so the backend composes it: TerminalMultiplexer.current_return_target() gains a concrete default (self.current_pane_id() or None) — tmux and herdr byte-identical to pre-PR behavior with zero changes on their side — and PsmuxMultiplexer overrides it to emit =sess:%N (the form your psmux source read verified), degrading to bare %N when the session probe fails or answers with a : in the name (unchanged rationale: a resolvable own pane means we're inside the mux, so None→detach would strand the client). launch.current_return_target() becomes a thin delegate and the replay sides stay fully opaque — which preserves everything this PR got right: the rename, the empty-probe degradation guards and their tests, and the RETURN_DETACH non-collision reasoning.

[pbean]

Pushed 0a427e5 implementing the backend-owned composition from the review, on top of your two commits.

What it does:

• TerminalMultiplexer.current_return_target() — new concrete seam method, default self.current_pane_id() or None. tmux and native-id backends (herdr) record exactly what they recorded before fix(tui): record session-qualified parked-window return target #225: the bare native pane id, which their own switch-client resolves precisely. The or None keeps your rc-0-empty-pane guard at the seam.
• PsmuxMultiplexer.current_return_target() — the qualified composition moved here: =session:%N via target(), degrading to the bare id when the session probe fails, answers empty, or answers a name containing : (which parse_target's first-colon split couldn't carry). Your degradation rationale and comments are preserved verbatim where they still apply.
• launch.current_return_target() is now a thin delegate; cli.py / tui/app.py call sites and your rename are untouched. Replay sides (_parked_trailer sh + pwsh, switch_client, return_attached_client) stay fully opaque — no code changes, comments reworded from "targets are =session:%N" to "backend-composed".
• Adapter guide: the "pane id rides in the window slot" tolerance note is replaced by documenting the optional current_return_target override — the seam grammar never has to carry a pane id now, so out-of-tree backends need no changes.
• Tests: your probe-composition/degradation tests moved to test_psmux_backend.py (plus a session-name-with-colon case); the launch-level test pins the tmux default back to bare %N and asserts the session probe is never issued there; your qualified-value round-trip through _return_fake is kept — the replay must handle any recorded form.

Verification: full suite 2681 passed / 1 known skip; trunk check --no-fix clean on all 12 touched files. Live on tmux 3.7b (scratch -L server): launch.current_return_target() from inside a real pane answers %0, and switch-client -t %0 with a pty-attached client returns it to the exact pane, rc 0 — the pre-#225 behavior, restored. The psmux side keeps your source-verified =session:%N form; live confirmation stays on the #222 checklist as before.

[pbean]

@CodeRabbit review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.