fix(adapters): give psmux a per-window option channel#315
Conversation
|
Warning Review limit reached
Next review available in: 5 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
Walkthroughpsmux per-window options now use session-scoped keys qualified by window id. The backend updates option reads, writes, listing, cleanup, parked-window returns, transport validation, documentation, changelog entries, and comprehensive tests. Changespsmux window option channel
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/test_psmux_backend.py (1)
1152-1154: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssertion is tautological in shape.
Filtering on
c[1] == "set-option"and then collectingc[1]compares the verb against itself; it passes for the right reason but reads as a mistake.assert not [c for c in calls if c[1] == "set-option"](or collecting the key, as the sibling sweep test does) states the intent directly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_psmux_backend.py` around lines 1152 - 1154, Update the assertion in the PsmuxMultiplexer().new_parked_window test so it directly verifies that calls with the "set-option" verb are absent, rather than filtering and comparing that same verb to itself; preserve the existing calls capture and test behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/bmad_loop/adapters/psmux_backend.py`:
- Around line 467-473: Update the non-transportable-value branch in the setter
around _transportable to remove or unset the existing option value before
returning, ensuring a rejected write cannot preserve stale state. Keep the
warning behavior and stderr output unchanged, and preserve normal writes for
transportable values.
---
Nitpick comments:
In `@tests/test_psmux_backend.py`:
- Around line 1152-1154: Update the assertion in the
PsmuxMultiplexer().new_parked_window test so it directly verifies that calls
with the "set-option" verb are absent, rather than filtering and comparing that
same verb to itself; preserve the existing calls capture and test behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 73557e5b-3c90-473a-9df1-31549e39a93c
📒 Files selected for processing (5)
CHANGELOG.mddocs/multiplexer-backends.mdsrc/bmad_loop/adapters/multiplexer.pysrc/bmad_loop/adapters/psmux_backend.pytests/test_psmux_backend.py
Greptile SummaryThis PR replaces a broken psmux per-window option path with a working substitute channel. Because psmux has one user-option scope per server and short-circuits every
Confidence Score: 5/5Safe to merge. The change adds an entirely new code path for a psmux-specific workaround; the tmux path is untouched and divergence is pinned by a test. The implementation is carefully guarded at every layer: unroutable targets decline loudly rather than misroute, untransportable values are refused and any stale prior value is freed, key lifecycle is covered by both proactive cleanup (kill_window) and reconciliation (orphan sweep at launch), and the orphan sweep can never fail the mint. All edge cases are exercised by 134 passing tests including ablation checks, degrade gates, and an end-to-end prune discrimination test. The tmux backend is untouched and pinned by a divergence test. No correctness issues were found. Files Needing Attention: No files require special attention.
|
| Filename | Overview |
|---|---|
| src/bmad_loop/adapters/psmux_backend.py | Core of the PR: adds the per-window option channel, key lifecycle management, transport validation, and parked-trailer fix. Logic is carefully guarded, each edge case is commented with psmux source references and live-verification notes. No issues found. |
| tests/test_psmux_backend.py | Adds 464 lines of new tests covering every new method: channel read/write/unset, unroutable/untransportable refusal gates, list_windows column fill, kill_window sweep, orphan-key reconcile, and an end-to-end prune discrimination test. Existing tests updated to account for the orphan sweep's extra subprocess calls. |
| src/bmad_loop/adapters/multiplexer.py | Minor docstring update to set_window_option clarifying that backends may use alternate storage; no logic changes. |
| docs/multiplexer-backends.md | Documents the psmux option channel model and the counter-intuitive transportability rules (apostrophe in spaceless vs. spaced path). |
| CHANGELOG.md | Release note added for the per-window option channel fix. Accurate and concise. |
Sequence Diagram
sequenceDiagram
participant Caller
participant PsmuxMux as PsmuxMultiplexer
participant psmux as psmux binary
participant PsmuxServer as psmux server (session-scoped options)
Note over Caller,PsmuxServer: set_window_option("ctl:@3", "@bmad_project", "C:/p")
Caller->>PsmuxMux: set_window_option
PsmuxMux->>PsmuxMux: _option_scope → ("ctl", "3")
PsmuxMux->>PsmuxMux: _transportable("C:/p") → True
PsmuxMux->>psmux: "set-option -t ctl @bmad_project_@3 C:/p"
psmux->>PsmuxServer: "store @bmad_project_@3"
Note over Caller,PsmuxServer: list_windows("ctl", ["window_id", "@bmad_project"])
Caller->>PsmuxMux: list_windows
PsmuxMux->>psmux: "list-windows -F #{window_id} #{window_id} -t ctl"
psmux-->>PsmuxMux: "@1 @1 / @3 @3"
PsmuxMux->>psmux: show-options -q -t ctl
psmux-->>PsmuxMux: "@bmad_project_@3 "C:/p""
PsmuxMux-->>Caller: "[("ctl:@1",""), ("ctl:@3","C:/p")]"
Note over Caller,PsmuxServer: kill_window("ctl:@3")
Caller->>PsmuxMux: kill_window
PsmuxMux->>psmux: show-options -q -t ctl
psmux-->>PsmuxMux: "@bmad_project_@3 "C:/p""
PsmuxMux->>psmux: "set-option -u -t ctl @bmad_project_@3"
PsmuxMux->>psmux: "kill-window -t ctl:@3"
Note over Caller,PsmuxServer: Parked trailer (in-pane, routes via $TMUX)
PsmuxMux->>psmux: "display-message -p -t $env:TMUX_PANE #{window_id}"
psmux-->>PsmuxMux: "@3"
PsmuxMux->>psmux: "show-options -qv @bmad_return_pane_@3"
psmux-->>PsmuxMux: detach
PsmuxMux->>psmux: detach-client
PsmuxMux->>psmux: "set-option -u @bmad_return_pane_@3"
Reviews (3): Last reviewed commit: "docs(adapters): pin the spaceless-apostr..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
A refused rewrite left the prior value live — a stale parked return target could replay later. The refusal now unsets the key so the option reads as unset, matching the reworded warning (bmad-code-org#315 review). Also: pin (source + round-trip test) that a spaceless UNC path rides the unquoted hop verbatim — the tokenizer's backslash collapse fires only inside double quotes — and de-tautologize the empty-live-list sweep assertion.
|
Heads-up for whoever merges this, from the #227 / PR #316 review — a constraint this PR inherits rather than causes. While the option channel is inert,
PR #316 just made Filed as #317 with the source evidence and three candidate directions, so nothing here needs to change: this is a note that the option channel's first live consumer arrives with a known-untrustworthy signal underneath it. Worth adding to the #313 safety-core checklist rather than treating as a blocker for this merge. |
A refused rewrite left the prior value live — a stale parked return target could replay later. The refusal now unsets the key so the option reads as unset, matching the reworded warning (bmad-code-org#315 review). Also: pin (source + round-trip test) that a spaceless UNC path rides the unquoted hop verbatim — the tokenizer's backslash collapse fires only inside double quotes — and de-tautologize the empty-live-list sweep assertion.
… review) The read-failure poison marker guarded a hazard the caller's untagged fallback already covers: the ctl prune claims an untagged window only when its run dir exists under this project, and run ids are unique by construction, so "" is ownership-safe in both directions. It was also inconsistent with the channel's own documented hole - a psmux server handler can answer the option listing empty-with-success while keys exist, and that path filled "" anyway. Degrade to "unset" everywhere, keep the stderr warning, and drop the cross-backend rule that an option column may carry a backend-private opaque marker.
A refused value re-entered unset_window_option to free the stale key, so an unroutable target warned twice - once about the transport, once about a `set-option -u` the caller never issued. Resolve the scope first (an unroutable target has nothing to write and nothing to free), then gate the value, and free the key through the resolved scope. The two mutating verbs now share one spawn-and-warn body.
The docs cited only a UNC-and-spaced path, but the psmux client quotes a value only when it holds an ASCII space and `'` is literal inside those quotes - so `C:\Users\O'Brien\dev` is refused while its spaced sibling is accepted. Name the inversion and pin it with a parametrize case. Also point the module docstring at the channel block instead of restating the model, and drop a stray word from the kill_window cost note.
2550377 to
99385fe
Compare
|
Reviewed against #310, the design doctrine in AGENTS.md/CONTRIBUTING, and the POSIX path. The approach is the one to take and the scope is right — I pushed four small corrections onto the branch rather than sending it back, and rebased onto What I checked, and what held
What I changed (3 commits)1.
It was also inconsistent with the channel's own documented hole: I also anchored the surviving warning the way #291 taught us: it is visible on the CLI prune (including the 2. 3. Left for you
Local: full suite green apart from two pre-existing |
Live verification — psmux 3.3.7 (05cc5d4), native WindowsRan the whole thing on a Windows VM against The
|
| value | result |
|---|---|
C:\proj\normal |
OK |
C:\Users\Some User\p |
OK |
C:\Users\O'Brien Files\proj |
OK |
a; b |
OK |
two spaces |
OK |
\\srv\share |
OK |
C:\Users\O'Brien\dev |
refused |
a ; b |
refused |
\\srv\share My Proj |
refused |
C:\dir with space\ |
refused |
The accept/refuse split matches your model exactly, including the counter-intuitive pair — spaced O'Brien Files accepted, spaceless O'Brien\dev refused.
The parked return actually returns now
Real new_parked_window, attach, select-window to it, Enter at the park prompt:
window: parkprobe:@2
psmux show-options -qv -t parkprobe "@bmad_return_pane_@2" -> detach
psmux attach -t parkprobe # ... Enter at the banner -> dropped to the host prompt
psmux has-session -t parkprobe; echo "rc=$LASTEXITCODE" -> rc=0
psmux show-options -q -t parkprobe | Select-String bmad -> (nothing)
Client detached, session survived, key freed. The has-session line matters: a read against a dead session doesn't fail on psmux, it falls through the most-recent-session routing and answers from another server — so an option read is only evidence while the session is provably alive.
The switch-client leg is untested and stays inert per psmux/psmux#483, as the PR says.
Cross-project prune — and the "before" is worse than #310 documented
On 99385fe, two projects sharing bmad-loop-ctl:
@bmad_project_@2 "C:\tmp\projA"
@bmad_project_@3 "C:\tmp\projB"
cleanup --dry-run --project C:\tmp\projA -> would close ctl window run-20260101-000000-aaaa
cleanup --dry-run --project C:\tmp\projB -> would close ctl window run-20260102-000000-bbbb
Same scenario on d3c6f5e (has #291, not #310) — the map collapses to one key, last writer wins:
@bmad_project "C:\tmp\projB"
cleanup --dry-run --project C:\tmp\projA -> nothing to clean up
cleanup --dry-run --project C:\tmp\projB -> would close ctl window run-20260101-000000-aaaa
would close ctl window run-20260102-000000-bbbb
Both failure directions, live: a project cannot prune its own window, and the other project would kill it. The issue only documented the second.
The pre-fix return is reproducible too — on d3c6f5e, Enter at a parked window leaves the client attached and drops it to window 0, because the -wqv read yields '' and neither branch fires.
Two assumptions, now probed rather than assumed
Window ids are not recycled — the sweep's correctness comment holds. If they were, a new window would inherit a dead one's stale key:
@1 -> new-window a -> @1 @2 -> kill @2 -> new-window b -> @1 @3
new-session on an existing session errors (duplicate session: dup, rc 1), so new_session's post-create has_session belt is meaningful rather than papering over a silent no-op.
Two cosmetic notes, neither blocking and neither yours
psmux list-windowsrendersrun-20260101-000000-aaaa-with a trailing-— that is psmux's default format appending window flags, not a corrupted name. bmad-loop passes its own-F #{window_name}, andcleanupprinted the clean name, so nothing is affected.bmad-loop muxrenders a broken table on a psmux host:psmux -Vprints two lines andcli.py:407dropsr.versionstraight into aljustcolumn. Thetmuxrow also reports psmux's version, since psmux ships atmux-named binary — harmless for selection today because tmux's platform gate excludes win32. Pre-existing and unrelated to this PR; I'll file it separately.
That closes the verification gap I flagged. From my side this is ready to merge.
…ode (bmad-code-org#317) The parked-return path reads an empty option on psmux only while the per-window option channel is inert. bmad-code-org#310 revived it, so the path now reaches switch_client/detach_client on Windows - and psmux answers both with an exit code that means nothing. At v3.3.7 every arm ends in `send_control(cmd)?; return Ok(())`: the only nonzero exit is an unreachable session server, so a detach succeeds with zero clients attached, and no form of switch-client - `-t` included - carries a server reply. Later builds narrow that only for `-t`. Taking those codes as the seam's booleans is the rc-0 no-op (bmad-code-org#228) reached through Python, and it hands `return_attached_client` a True that clears the return option and reports a hand-back that never happened: bmad-code-org#227's exact symptom, on the platform the psmux backend exists for. So discard the exit code and measure: count the clients attached to this session before and after the verb, answer on the drop. Never on an absolute count - a `-t` read against a session whose server is gone answers from whichever server the fallback picks, so "zero attached" alone proves nothing (bmad-code-org#315); a drop needs two successful reads of the same session, the first nonzero. Unobservable answers False, never a vacuous True: the probe is self-detecting, since a build without #{session_attached} cannot echo a plausible integer. The switch leg reads as no effect today - it is inert at 3.3.7 (psmux/psmux#483) - and starts reporting True on its own when upstream lands the fix, which is why this measures rather than hardcoding. Also state the rule at the seam it belongs to: detach_client/switch_client owe effect, not dispatch, and the authoring guide now shows both ways to pay for it.
2 participants
Closes #310
What
psmux keeps every user option in one per-server map and short-circuits every
-wread of an@-prefixed name to""before that map is consulted. Both mechanisms riding per-window options were therefore broken: the ctl-windowPROJECT_OPTIONprune guard read the same bled value for every row (a prune in one project couldkill-windowanother project's window), and theRETURN_OPTIONparked-return mechanism was fully inert.@bmad_project_@3for window@3), every out-of-pane write and read routed with an explicit-t <session>. Builtin (non-@) options fall through to the base-wpath. No call-site changes.list_windowsdrops@-prefixed fields from the psmux format string (the#{@name}bleed) and fills those columns per row from one batchedshow-options -qlisting. A failed read poisons the column instead of reading as "untagged" —""is the kill-eligible direction._transportable). That includes standalone;/\;tokens: the one-shot CLI path splits them without quote-awareness and hands the remainder to the server as a command — filed upstream as set-option: ";" inside a double-quoted value splits the one-shot command line (value truncated, remainder executed) psmux/psmux#499 with an execution proof.-t $env:TMUX_PANE) and reads/unsets its id-suffixed key. The switch-client leg composes the correct verb but stays inert until switch-client -t: window/pane-scoped targets accepted (exit 0) but silently ignored (3.3.7) psmux/psmux#483 lands upstream; the detach leg works today.kill_windowsweeps that window's_@Nsession keys before the kill, and a launch-time orphan reconcile covers the Enter-dismiss exit thatkill_windownever sees (snapshot-before-listing ordering so a freshly minted window can never read as an orphan).-wargv is byte-identical to before (divergence pinned by test).Every psmux behavior relied on here was read from the psmux source at tag
v3.3.7(the installed build) and the transport edge cases were smoke-tested live against a scratch session on stock 3.3.7.Verification
pytest tests/test_psmux_backend.py tests/test_multiplexer.py -q→ 134 passed (incl. ablation-checked degrade/refusal gates and the tmux divergence pins)trunk checkclean,pyright@1.1.411clean on the touched modulesSummary by CodeRabbit
Bug Fixes
Documentation