fix(adapters): seam-owned psmux option-key namespace, kill-order containment - #434
fix(adapters): seam-owned psmux option-key namespace, kill-order containment#434dracic wants to merge 2 commits into
Conversation
…ainment (bmad-code-org#313) Key grammar moves to a seam-owned marker (@opt__blw@N) so both destructive sweeps match only keys the channel itself minted; kill_window now kills, verifies the window is gone, then frees keys, retaining them on any ambiguity with the orphan sweep as backstop; ctl option calls target by window id instead of first-match names; a Windows-local zero-token live gate proves cross-project prune isolation on real psmux 3.3.7. Every silent key-cleanup failure now warns: both sweeps route unsets through one _free_scoped_key helper (nonzero rc warns), the orphan sweep warns when the option listing fails, composition tests pin the failure sequences (ownership readable after a failed kill; stranded keys reclaimed by the sweep), and the live gate's teardown verifies the session died.
|
Warning Review limit reached
Next review available in: 37 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 (2)
WalkthroughThe psmux backend now uses seam-owned Changespsmux safety and stable window targeting
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant TUI
participant PsmuxMultiplexer
participant psmux
participant ScopedOptions
TUI->>PsmuxMultiplexer: resolve control window ID
PsmuxMultiplexer->>psmux: kill window by ID
PsmuxMultiplexer->>psmux: verify window is gone
PsmuxMultiplexer->>ScopedOptions: identify __blw@ keys
PsmuxMultiplexer->>psmux: unset verified keys
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/test_tui_app.py (1)
2139-2157: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd
force_tmux_backendto these two attach tests.Both tests assert literal
tmuxargv (["tmux", "switch-client", "-t", "=bmad-loop-ctl"]) through the multiplexer seam, but neither pins the backend. The neighbouringtest_attach_prefers_agent_session_without_decisionat Line 2183 carries@pytest.mark.usefixtures("force_tmux_backend")with the comment "pin tmux against win32-matching externals". Without the fixture, platform selection can pick psmux on a win32 host and these assertions fail on backend name alone.As per coding guidelines: "Use the mock adapter for engine tests; use
force_tmux_backendwhen asserting tmux argv through the multiplexer seam."💚 Proposed fix
+@pytest.mark.usefixtures("force_tmux_backend") # pin tmux against win32-matching externals async def test_attach_targets_ctl_window_when_decision_pending(project, monkeypatch):+@pytest.mark.usefixtures("force_tmux_backend") # pin tmux against win32-matching externals async def test_attach_falls_back_to_ctl_window(project, monkeypatch):Also applies to: 2202-2218
🤖 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_tui_app.py` around lines 2139 - 2157, Add the force_tmux_backend pytest fixture marker to both attach tests, including test_attach_targets_ctl_window_when_decision_pending and the test covering lines 2202-2218. Keep their existing tmux argv assertions unchanged, matching the neighboring test_attach_prefers_agent_session_without_decision pattern.Source: Coding guidelines
🧹 Nitpick comments (1)
src/bmad_loop/adapters/psmux_backend.py (1)
580-589: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider reusing
_write_scopedor catching transport errors per key.
_free_scoped_keyissues the sameset-option -uverb as_write_scoped, but it does not catchsubprocess.SubprocessError/OSError. Both callers wrap the loop in a guardedtry, so a transport failure on one key aborts the remaining keys of that sweep instead of freeing them. The two unset-and-warn paths also produce different warning text for the same failure.Two options: delegate to
_write_scoped(adds the transport catch and one message shape), or keep the distinct message and add the same catch inside_free_scoped_keyso the loop continues.♻️ Option: catch transport errors per key
def _free_scoped_key(self, session: str, name: str) -> None: # The one unset path both sweeps share. rc-0 is not proof the key is # gone on psmux's write side, but a nonzero rc IS proof it is not — # silence here would leak a key for the server's life with no signal. - proc = self._run(["set-option", "-u", "-t", session, name], check=False) - if proc.returncode != 0: - print( - f"warning: could not free option {name} on {session}: {proc.stderr.strip()}", - file=sys.stderr, - ) + try: + proc = self._run(["set-option", "-u", "-t", session, name], check=False) + except (subprocess.SubprocessError, OSError) as exc: + # Per key, so one dead round-trip does not abandon the rest of the sweep. + print(f"warning: could not free option {name} on {session}: {exc}", file=sys.stderr) + return + if proc.returncode != 0: + print( + f"warning: could not free option {name} on {session}: {proc.stderr.strip()}", + file=sys.stderr, + )🤖 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 `@src/bmad_loop/adapters/psmux_backend.py` around lines 580 - 589, Update _free_scoped_key to catch subprocess.SubprocessError and OSError around the unset operation, warning with the transport error and returning so one failed key does not abort the sweep. Preserve its existing nonzero-return warning behavior and align transport-failure handling with _write_scoped.
🤖 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 `@tests/test_tui_launch.py`:
- Around line 259-309: Update all four
tests—test_ctl_window_id_matches_run_id_suffix,
test_ctl_window_id_skips_empty_id_rows,
test_kill_ctl_window_kills_by_id_not_first_name_match, and
test_ctl_window_id_no_session_or_tmux—to use the existing force_tmux_backend
fixture, ensuring backend selection is pinned and the multiplexer cache is
cleared. Preserve the local shutil.which override to None in
test_ctl_window_id_no_session_or_tmux after applying the fixture.
---
Outside diff comments:
In `@tests/test_tui_app.py`:
- Around line 2139-2157: Add the force_tmux_backend pytest fixture marker to
both attach tests, including
test_attach_targets_ctl_window_when_decision_pending and the test covering lines
2202-2218. Keep their existing tmux argv assertions unchanged, matching the
neighboring test_attach_prefers_agent_session_without_decision pattern.
---
Nitpick comments:
In `@src/bmad_loop/adapters/psmux_backend.py`:
- Around line 580-589: Update _free_scoped_key to catch
subprocess.SubprocessError and OSError around the unset operation, warning with
the transport error and returning so one failed key does not abort the sweep.
Preserve its existing nonzero-return warning behavior and align
transport-failure handling with _write_scoped.
🪄 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: b42c54a2-aa8d-4c10-b9a4-defbfd42fb0f
📒 Files selected for processing (10)
AGENTS.mdCHANGELOG.mddocs/multiplexer-backends.mdsrc/bmad_loop/adapters/psmux_backend.pysrc/bmad_loop/tui/app.pysrc/bmad_loop/tui/launch.pytests/test_psmux_backend.pytests/test_psmux_live.pytests/test_tui_app.pytests/test_tui_launch.py
Greptile SummaryThis PR hardens three safety gaps in the psmux option channel: a seam-owned key namespace marker (
Confidence Score: 5/5Safe to merge. All three hardening goals are implemented correctly and consistently, and no new defects were introduced. The key-namespace change, kill-order flip, and id-resolution refactor are each mechanically sound. _SCOPE_MARKER is derived in one place and all consumers stay in sync with it. Kill-then-verify handles all ambiguous states conservatively. Per-key exception isolation is correct via _write_scoped delegation. Test coverage is thorough with 13 new backend tests plus a live gate. Files Needing Attention: No files require special attention.
|
| Filename | Overview |
|---|---|
| src/bmad_loop/adapters/psmux_backend.py | Core backend changes: _SCOPE_MARKER, _KEY_SUFFIX, _free_scoped_key, _sweep_orphan_keys, and kill_window all updated consistently; kill-then-verify logic correctly ordered with conservative fallback on ambiguous liveness probes |
| src/bmad_loop/tui/launch.py | ctl_window renamed to ctl_window_id returning stable window id; empty-id guard prevents blank -t from resolving to current window |
| src/bmad_loop/tui/app.py | action_attach updated to use ctl_window_id / select_ctl_window_id; return_window now passes stable id directly to set_return_pane |
| tests/test_psmux_backend.py | New tests cover kill-then-verify ordering, failed kill key retention, unverifiable liveness fallback, batch isolation in _free_scoped_key, sweep warnings, and old-convention user options surviving sweeps |
| tests/test_psmux_live.py | New Windows-local live gate; skip-guarded; asserts cross-project prune isolation including survival of old-convention user option |
| tests/test_tui_app.py | Monkeypatches updated to ctl_window_id/select_ctl_window_id; stamp assertions updated to bare @n ids |
| tests/test_tui_launch.py | Tests updated for ctl_window_id rename, empty-id skip, duplicate-name kill correctness, and attach_plan return_window as stable id |
Sequence Diagram
sequenceDiagram
participant Caller
participant kill_window
participant BaseMux
participant sweep as _sweep_orphan_keys
Caller->>kill_window: "kill_window(ctl:@3)"
kill_window->>kill_window: "_option_scope -> (ctl, 3)"
kill_window->>BaseMux: super().kill_window
BaseMux-->>kill_window: kill sent
kill_window->>BaseMux: list_window_ids(ctl)
BaseMux-->>kill_window: "[@1,@13] no @3 confirmed dead"
kill_window->>kill_window: "free keys ending in __blw@3"
Note over kill_window: _free_scoped_key via _write_scoped per-key warn never raise
Note over kill_window,sweep: If @3 still in live list
kill_window-->>Caller: return keys retained for live window
Note over kill_window,sweep: If liveness list is empty
kill_window-->>Caller: return keys retained orphan sweep will reclaim
Caller->>sweep: _sweep_orphan_keys at next parked-window launch
sweep->>BaseMux: _scoped_options
sweep->>BaseMux: list_window_ids
sweep->>sweep: "free __blw@N keys with no live window"
Reviews (2): Last reviewed commit: "fix(adapters): contain a dead option-fre..." | Re-trigger Greptile
| scope = self._option_scope(target) | ||
| if scope is not None: | ||
| session, digits = scope | ||
| suffix = f"_@{digits}" | ||
| try: | ||
| for name in self._scoped_options(session) or (): | ||
| if name.endswith(suffix): | ||
| self._run(["set-option", "-u", "-t", session, name], check=False) | ||
| except (subprocess.SubprocessError, OSError): | ||
| pass | ||
| super().kill_window(target) |
There was a problem hiding this comment.
_option_scope is called before super().kill_window, so an exception from _window_id_for_name → super().list_windows (e.g., TmuxError, subprocess.SubprocessError) prevents the kill entirely. In the old code, any cleanup exception left the kill intact; here the kill is never sent. The outer try/except at line 670 only guards the cleanup block, not the pre-kill scope lookup. Since every current caller passes a stable id (not a name token), this path is dormant for normal operation — but a future name-target caller or a transient psmux hiccup during name resolution would silently skip the kill.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/bmad_loop/adapters/psmux_backend.py
Line: 647-648
Comment:
**Name-target kill skipped when scope resolution raises**
`_option_scope` is called before `super().kill_window`, so an exception from `_window_id_for_name → super().list_windows` (e.g., `TmuxError`, `subprocess.SubprocessError`) prevents the kill entirely. In the old code, any cleanup exception left the kill intact; here the kill is never sent. The outer `try/except` at line 670 only guards the cleanup block, not the pre-kill scope lookup. Since every current caller passes a stable id (not a name token), this path is dormant for normal operation — but a future name-target caller or a transient psmux hiccup during name resolution would silently skip the kill.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
Not valid — the exception this depends on cannot happen. _option_scope reaches the listing through _window_id_for_name, which calls super().list_windows, and the base swallows the whole transport class itself (adapters/tmux_base.py:354-361):
try:
probe = self._run(["list-windows", "-t", f"={session}", "-F", fmt], check=False)
except (subprocess.SubprocessError, OSError):
return []
if probe.returncode != 0:
return []So a dead or failing name-resolve returns [], _window_id_for_name answers None, digits is empty, and _option_scope returns None. The kill still goes out — unscoped, with the keys left to the launch-time orphan sweep, which is the same degradation the ambiguous-liveness branch below already takes. Nothing propagates, so TmuxError never enters this path either (the base only raises it from _tmux/has_session, neither of which is on this leg).
I did write the guard you're describing first, then ablated it: with a TimeoutExpired injected into list-windows, kill-window was sent either way — the guard was unreachable, so it came back out rather than landing as dead code.
The reading is fair, though: the scope-before-kill ordering makes it look plausible on sight, and nothing in the file pinned the opposite. 3daade2 adds test_kill_window_sends_the_kill_when_the_scope_lookup_dies to characterize it, so a future change that lets _window_id_for_name propagate turns your hypothetical into a red test.
_free_scoped_key issued set-option -u without the transport catch every other write in this channel has, so a dead round-trip on key N escaped to the sweep's outer guard and left keys N+1..M stranded until the next launch. Route it through _write_scoped instead: one warn-never-raise body, one message shape, and the batch continues past a failed key. Also pin the pre-kill leg of kill_window: the scope lookup runs before the kill, which reads like a transport failure there could cost the kill, but the base list_windows swallows to [] so it degrades to an unscoped kill.
|
Review pass addressed in 3daade2. Inline replies on the three anchored threads; the outside-diff one has no thread to reply to, so it is here.
The fix. On the outside-diff finding. All five attach tests in Gates on Windows: 389 passed / 7 skipped across |
Hardens the psmux option channel's safety core — the three ceilings accepted when the channel landed (#310):
__blw@marker (@bmad_project__blw@3, was@bmad_project_@3), so the generic cleanup sweeps can no longer delete a hand-written user option like@theme_@3when window@3dies. Colliding now requires deliberately imitating the seam's marker.kill_windowkills first and frees the window's keys only after a liveness listing proves the kill landed. A failed kill leaves the live window its project tag and return key; keys stranded by a cleanup failure are reclaimed by the launch-time orphan sweep. Every silent key-cleanup failure now warns.ctl_window/select_ctl_windowfold intoctl_window_id/select_ctl_window_id).Cross-project prune isolation is proven live: a Windows-local zero-token gate (
tests/test_psmux_live.py) runs two projects' parked windows on one real psmux server and asserts a prune in one leaves the other's window, tag, and foreign options untouched.Note for dev-build users: keys minted by the unreleased pre-marker build read as foreign and are never swept — restart the ctl psmux server after upgrading.
Closes #313
Summary by CodeRabbit
Bug Fixes
Documentation