fix(tui): report the parked-window return honestly (#227)#316
Conversation
|
Warning Review limit reached
Next review available in: 34 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 (3)
WalkthroughThe parked-window return flow now reports actual detach and switch effects, distinguishes typed return outcomes, clears ChangesParked-window return flow
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant SweepEngine
participant ReturnFlow
participant Multiplexer
participant Backend
SweepEngine->>ReturnFlow: request return after decisions
ReturnFlow->>Multiplexer: detach or switch client
Multiplexer->>Backend: execute client verb
Backend-->>Multiplexer: report observed effect
Multiplexer-->>ReturnFlow: return outcome
ReturnFlow-->>SweepEngine: RETURNED, ATTENDED, or UNREACHABLE
Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Greptile SummaryThis PR fixes
Confidence Score: 5/5Safe to merge — the fix is correctly scoped, all three outcome branches are covered by new tests with ablation checks, and the psmux measurement strategy degrades to False rather than a vacuous True when the count is unobservable. The root bug (unconditional True return) is squarely addressed. The three-way ReturnOutcome correctly models the two distinct failure modes; RETURN_OPTION is preserved on failure and cleared only on a verified return. The tmux path reads exit codes honestly; the psmux measurement approach is sound (delta-based, not absolute-count-based). Test coverage is thorough: every new branch has a dedicated test and both new negative tests were ablation-checked by the author. Files Needing Attention: The _attached_clients helper in psmux_backend.py uses str.isdigit() before int(), which could raise ValueError for Unicode superscript digits — a minor robustness gap with no practical impact on psmux's ASCII output.
|
| Filename | Overview |
|---|---|
| src/bmad_loop/tui/launch.py | Core fix: return_attached_client now returns a three-way ReturnOutcome enum; RETURN_OPTION is cleared only on RETURNED, and the two failure modes (failed switch → ATTENDED; failed detach → UNREACHABLE) are correctly distinguished. |
| src/bmad_loop/adapters/psmux_backend.py | New _attached_clients/_client_left helpers measure verb effect via client-count delta; detach_client and switch_client overrides use these to avoid trusting psmux's exit-0-always behavior. Minor robustness nit: isdigit() should be isdecimal() before int(). |
| src/bmad_loop/adapters/tmux_base.py | detach_client widened from -> None to -> bool, returning the exit code; transport failures correctly return False. |
| src/bmad_loop/adapters/multiplexer.py | ABC signature for detach_client widened to bool; docstring updated to document the effect-not-dispatch contract for all backends. |
| src/bmad_loop/sweep.py | _return_after_decisions correctly branches on all three ReturnOutcome values: ATTENDED returns early (keeps prompting), RETURNED clears prompting and journals the success, UNREACHABLE clears prompting silently and journals sweep-return-no-client. |
| tests/test_tui_launch.py | New tests cover: switch-fails-stays-ATTENDED, detach-fails-is-UNREACHABLE, fallback-switch path; existing tests updated to assert ReturnOutcome enum values instead of bare booleans. |
| tests/test_psmux_backend.py | New psmux-specific tests exercise: observed drop (True), zero clients before detach (False), unobservable format field (False), outside-pane guard (no verb issued), switch primary/fallback paths, and the inert-switch-leg case. |
| tests/test_sweep.py | New test_interactive_decisions_unreachable_goes_unattended validates that UNREACHABLE silences prompting and journals sweep-return-no-client without the success message; helper factoring is clean. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[return_attached_client] --> B{mux_usable?\nwin present?\nRETURN_OPTION set?}
B -- "No" --> C[ATTENDED\nkeep prompting]
B -- "Yes, option=detach" --> D[mux.detach_client]
B -- "Yes, option=pane target" --> E[mux.switch_client\nlast_fallback=True]
D -- "True\nclient detached" --> F[RETURNED\nclear RETURN_OPTION]
D -- "False\nno client / unobservable" --> G[UNREACHABLE\noption preserved]
E -- "True\nclient switched" --> F
E -- "False\nclient still here" --> C
F --> H[sweep: prompting=False\njournal sweep-returned-after-decisions\nprint success]
G --> I[sweep: prompting=False\njournal sweep-return-no-client\nprint nothing]
C --> J[sweep: return early\nprompting stays True]
Reviews (4): Last reviewed commit: "docs(adapters): scope the client-verb ef..." | Re-trigger Greptile
![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
🤖 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 `@docs/adapter-authoring-guide.md`:
- Around line 98-101: Update the authoring guide’s detach_client/switch_client
boolean contract to allow either backend-reported command success or observed
client movement, rather than requiring actual movement and forbidding vacuous
True values. Explicitly document the psmux limitation that a successful command
may return True even when zero clients are attached, keeping the guidance
consistent with multiplexer.py and psmux_backend.py.
🪄 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: 622ef2ba-591b-4eaa-932b-91838edd6d7f
📒 Files selected for processing (8)
CHANGELOG.mddocs/adapter-authoring-guide.mdsrc/bmad_loop/adapters/multiplexer.pysrc/bmad_loop/adapters/psmux_backend.pysrc/bmad_loop/adapters/tmux_base.pysrc/bmad_loop/tui/launch.pytests/test_multiplexer.pytests/test_tui_launch.py
return_attached_client() discarded switch_client's result and answered True unconditionally, so a failed switch plus a failed -l fallback still journaled the return and sent the sweep unattended while the human sat in the sweep window with their terminal never handed back. Thread the result out and clear RETURN_OPTION only on a real return, so a failed one is left for the parked window's trailer. Widen TerminalMultiplexer.detach_client from None to bool for the same reason: the returncode was in hand and dropped, so the detach branch carried the identical false positive. Closes bmad-code-org#227
…e-org#227 review) The psmux ledger entry said only `switch-client -t` carries a server reply. That describes psmux main, not the v3.3.7 the version gate admits: there the whole CLI arm is `send_control(cmd)?; return Ok(())`, so no form carries a reply and `-t` is exit-0-regardless too. The response path landed after the tag and only for `-t`, leaving `-l` unconditional either way. Understated the ceiling in the direction that matters, so state it at both revisions and name it as the rc-0 no-op family (bmad-code-org#228). Also resolve the contradiction the same round introduced: the authoring guide promised the seam never answers a vacuous True while the ABC and the psmux ledger both documented psmux doing exactly that. Keep the requirement — report effect, not dispatch — and name psmux as the worked deviation, rather than relaxing the contract to "command succeeded", which is the failure bmad-code-org#227 exists to remove.
…d-code-org#227 review) Threading the boolean through fixes the switch branch and breaks the detach one. `sweep._return_after_decisions` consumes it to answer "can a human still answer here?", and the two failures answer that in opposite directions: a failed switch leaves the client in this window with someone in front of it, while a failed detach means there was no client to detach at all. Reporting both as a plain False keeps a --repeat sweep prompting into a window nobody is viewing, on input() that never returns - the livelock the function's own docstring exists to prevent, and the documented flow walks straight into it (tui-guide step 3 tells tmux users to detach right after answering; on herdr, whose detach is a no-op the widened seam now requires to answer False, it is not a race but the only outcome). So return_attached_client answers a ReturnOutcome instead: RETURNED, ATTENDED (keep talking to the terminal - nothing was recorded to return to, the backend is unusable, or the switch left the client here), UNREACHABLE (a detach found nothing attached, or the backend has no detach verb). RETURN_OPTION still clears only on RETURNED. The sweep goes unattended on anything but ATTENDED, and announces only a real return - UNREACHABLE journals sweep-return-no-client and prints nothing, because there is no one to read it.
…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.
The case scripted no attached-client counts, so deleting the guard failed the test on the fixture running dry rather than on the behavior. Script counts a probe could read: the ablation now lands on `assert ... is False`.
c262708 to
04a65ea
Compare
|
Reviewed, rebased onto 1. The detach branch inverted the safe degradation
With So 2. "Moot today" stopped being true four hours after this opened#315 merged at 23:16Z and revived psmux's per-window option channel, so both the description and the ledger paragraph were stale on arrival — the condition #317 predicted verbatim ("the moment #310 / #315 revives that channel, this becomes reachable") had been met. On psmux Rather than defer it, psmux now measures: count the session's attached clients before and after the verb, answer on the drop. Never on an absolute count — a Your v3.3.7 source reading is preserved verbatim in the new 3. Doc surfaces
Verification
Still open before merge: One housekeeping note: the description says the |
Windows VM runbook — confirm
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@docs/adapter-authoring-guide.md`:
- Around line 111-117: Revise the guidance around the failed detach case to say
that False indicates no verified return effect, not proof that no client exists.
Clarify that UNREACHABLE is the caller’s conservative policy for this uncertain
outcome, while preserving the distinction from a failed switch_client and the
existing ATTENDED behavior.
- Around line 103-106: Clarify the psmux example in the adapter-authoring
guidance: do not use a session-wide attached-client count to validate
switch-client. Document the window/current-target observation that verifies
detach-client and switch-client, or specify a separate probe for the switch
operation, while preserving the existing measurement guidance for cases where
exit statuses are unreliable.
🪄 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: 4a19429c-61cc-46cc-8a47-af87f1be9d2c
📒 Files selected for processing (12)
CHANGELOG.mddocs/adapter-authoring-guide.mddocs/tui-guide.mdsrc/bmad_loop/adapters/multiplexer.pysrc/bmad_loop/adapters/psmux_backend.pysrc/bmad_loop/adapters/tmux_base.pysrc/bmad_loop/sweep.pysrc/bmad_loop/tui/launch.pytests/test_multiplexer.pytests/test_psmux_backend.pytests/test_sweep.pytests/test_tui_launch.py
🚧 Files skipped from review as they are similar to previous changes (5)
- src/bmad_loop/adapters/tmux_base.py
- tests/test_multiplexer.py
- CHANGELOG.md
- src/bmad_loop/adapters/multiplexer.py
- tests/test_tui_launch.py
Windows probe result:
|
| step | command | result |
|---|---|---|
| 1 | display-message -p -t probe '#{session_attached}', nothing attached |
0 |
| 2 | same, one client attached | 1 |
| 2 | detach-client; $LASTEXITCODE |
0 |
| 2 | count after the detach | 0 — the drop we measure |
| 3 | detach-client with nothing attached; $LASTEXITCODE |
0 |
| 3 | count after | 0, unchanged |
Step 3 is #317 in two lines. psmux exits 0 for a detach that did nothing, with the count sitting still — the vacuous True the seam used to hand return_attached_client. Step 2 exits 0 for a detach that worked. Identical exit codes, opposite effects: the count is the only thing that separates them, which is what _attached_clients now reads.
So the psmux half of this PR is verified at the transport, not just in the mocked unit tests. #317 can close with the merge.
Two things this did not prove, stated so the record is accurate:
- The end-to-end sweep (step 4) was not run — no pending decision available on that host. The seam is measured; the full attach → answer → hand-back path on Windows is still only covered by the mocked tests.
- The probes ran from the
bmad-loop-315checkout, not this branch. Harmless here, since steps 1–3 are purepsmuxCLI calls that never read the repo — but it is the same wrong-checkout trap that cost four probe rounds during fix(adapters): give psmux a per-window option channel #315, and it would have invalidated step 4. Flagging rather than quietly counting it.
Residue unchanged and documented in the backend's degradation ledger: a switch whose target pane lives in the same session moves a client between windows without changing the session's attached count, so it reads as no effect and the caller keeps prompting — the safe direction.
…review) Two places where the prose claimed more than the seam delivers — the same defect class already fixed once in c262708. The psmux worked example described the attached-client count as if it proved any switch. It proves a detach, and the switch the return path normally performs (the recorded target is a pane in the session the client came from, so a real switch leaves this session) — but not a switch within this session, which moves the client between windows without moving the count. psmux_backend.py already carried that under `Residue:`; the guide did not. Failing that way is conformant, so the rule is unchanged: the caller reads no effect and keeps prompting. "A failed detach_client means there was no client there at all" is true on tmux and nowhere else. The same section already mandates False for an unverifiable effect, and the guide's own herdr paragraph describes a no-op detach whose client only a manual chord releases. False means "no verified hand-back"; UNREACHABLE is the caller's policy for that uncertainty, justified by the asymmetric cost — prompting into an unwatched window blocks a --repeat cycle on input() forever, while going unattended in front of a human only defers decisions to `bmad-loop decisions`. launch.py and sweep.py repeated the same overstatement, so they move with the guide rather than leaving the inconsistency relocated. No behavior change; the no-drop-reads-False shape is already covered by tests/test_psmux_backend.py.
2 participants
return_attached_client()discardedswitch_client's result and answeredTrueunconditionally, so a failed switch plus a failed-lfallback still journaledsweep-returned-after-decisions, printed "✓ decisions recorded" and sent the sweep unattended — while the human sat in the sweep window with their terminal never handed back, every later decision silently routed to the unattended path.The change
switch_clientresult is now the return value, andRETURN_OPTIONis cleared only on a real return, so a failed one is left for the parked window's trailer.TerminalMultiplexer.detach_clientwidensNone→boolfor the same reason: the returncode was in hand and dropped, so the detach branch (the "TUI runs outside the multiplexer" attach) carried the identical false positive. Escalated into scope by review, maintainer-approved.sweep.pyis untouched — it already branches on the boolean correctly.What the honesty guarantee actually covers
tmux: fully. Smoke-measured on live tmux 3.4 (isolated
-Lsocket):detach-clientwith no client attached exits 1 withno current client; with one attached it exits 0 andlist-clientsis empty afterwards.psmux: not at all, and the docstrings now say so. Verified in the Rust source at the version our gate admits (v3.3.7): both the
detach-clientandswitch-clientCLI arms end insend_control(cmd)?; return Ok(()), so the only nonzero exit is an unreachable session server. A detach therefore succeeds with zero clients attached — and a flag-less detach is promoted server-side to detach-all — while no form ofswitch-client,-tincluded, carries a server reply. Later builds narrow this but do not close it: a response path landed after the tag and only for-t, leaving the-lfallback exit-0 regardless. Both seam booleans can therefore be vacuouslyTrueon psmux — the same ceiling as the rc-0 no-op (#228), reached through the booleans instead of a shell fallback.Corrected after review: an earlier revision of this description credited
switch-client -twith a reply at v3.3.7. That describes psmuxmain, not the gated tag, and understated the ceiling.This is moot today — psmux's per-window option channel is inert (#310), so the parked-return path reads an empty option and answers
Falsebefore reaching either verb. It becomes live the moment #310 / #315 revives that channel, which is why it is recorded in the psmux backend's degradation ledger rather than left implicit.herdr (out-of-tree): unchanged in practice, but this is a contract change. The reference third-party backend (
pbean/bmad-loop-adapter-herdr) declaresdetach_client() -> None, and its detach is a documented no-op — herdr has no client to detach. At runtime the widening therefore improves it:Noneis falsy, so the parked-return path reads "nothing was detached", keepsRETURN_OPTION, and reports no return — which is the truth for a no-op, and strictly better than the unconditionalTrueit effectively got before. Nothing degrades.What does change is the type: a
-> Noneoverride no longer satisfies the widened ABC under static checking. The fix is two lines in its own repo, so it is not filed here; both places indocs/adapter-authoring-guide.mdthat described the void seam now state the requirement — a backend with no real detach returnsFalse, neverNoneand never a vacuousTrue.Evidence levels differ, deliberately: tmux is measured against a live server, psmux is verified against the Rust source at the gated tag, herdr is inferred from this repo's own documentation of it — I have no local checkout and did not verify its source. Treat the herdr paragraph as a contract statement, not a smoke result.
Worth a reviewer's eye: this is the first mux-seam change that is not additive-only. Every prior addition (
current_return_target,window_pane_pids,version()) shipped as a concrete default precisely so out-of-tree backends inherited it untouched, and there is no protocol-version or capability-negotiation mechanism for this seam. That policy gap is the real follow-up; it is recorded in the deferred-work ledger rather than smuggled into this PR.Verification
sleepdependency).pyright@1.1.411,ruff,prettier@3.8.4clean.Closes #227
Summary by CodeRabbit
Summary by CodeRabbit
Bug Fixes
ReturnOutcomeresult.Documentation
Tests