test(adapters): probe psmux workaround premises, not just our reaction - #517
Conversation
Every psmux workaround was pinned only by tests asserting how the backend reacts; nothing observed the upstream behavior each one exists because of, so a fixed divergence changed no test's colour. Four premises had already flipped on psmux main and nothing here noticed. Add eight premise probes to the Windows live gate. Each drives raw psmux argv against a throwaway session and asserts the observed upstream behavior, with a failure message naming the workaround that has become droppable. The failure semantics are inverted on purpose: a red probe is the signal, not a break. Every premise was observed on the installed 3.3.7 build before its assertion was written, and two turned out sharper than assumed. `set-option -w` does not merely lack per-window storage, it silently lands at session scope; and an unresolvable `kill-window` target exits 0 while destroying the ACTIVE window. The `=`-form kill divergence was traced to its cause and filed upstream as psmux/psmux#558. Also ablate the backend's negative assertions — delete the guard, run the test, confirm it fails, revert. Two did not bite: one was vacuous, because `last_fallback` defaults false and left the fallback leg unreachable regardless of the guard it claimed to cover; the other reached its assertion only via an IndexError from a fixture that ran dry. Zero-token contract unchanged, and the live gate stays a manual gate. Closes bmad-code-org#488
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
Warning Review limit reached
Next review available in: 3 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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 change adds isolated live probes for raw psmux behavior, documents the upstream version basis, and updates switch-client tests to exercise fallback handling without vacuous assertions. Changespsmux premise validation
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
🧪 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 |
Maintainer-side review fixes on the bmad-code-org#488 probe block, live gate only. A spaced tmp path now skips the pipe-pane probe alongside `$` and a backtick. psmux's quote re-parse splits a spaced path (a `C:\Users\John Doe` profile is enough), so the sidecar never spawns and the probe would red permanently while its message blamed a transient spawn race. pipe_pane's docstring already lists a spaced path as an untested ceiling; skipping is honest about that. The `psmux_data_root` fixture verifies its own teardown in both the isolated and the default registry view. It is the one session here that can land in the developer's real registry — a build ignoring PSMUX_DATA_DIR is the branch the fixture exists to detect — and its kill was bare best-effort. `switch-client` and its attached-count read run with `$TMUX`/`$TMUX_PANE` scrubbed. The premise is "no client to move", and an inherited `$TMUX` says otherwise when pytest itself runs inside psmux. At 3.3.7 the verb is inert so nothing moves, but on the effect-reporting build this probe exists to catch it would drag the developer's client into the throwaway session and stay green through the very premise flip. The `probe` teardown also catches TimeoutExpired: `_plain_has_session` goes through raw `_run`, which propagates a timeout even under `check=False`, so a hung psmux escaped the teardown instead of reporting the leak. Setup and scaffolding assertions carry a `probe setup: ` prefix so an instrument failure is never read as a premise flip; the premise assertions, whose messages name a droppable workaround, keep their bare form. `_mint_probe_window` gains the qualified-id guard the prune test applies to its own mints, and the kill probe's ordering comment now states the real safety property — every probe owns its session — rather than a file order xdist does not preserve.
|
Reviewed and pushed maintainer-side fixes as Changes
Non-blocking, accepted as-is
Two small notes on the brief, for your call
Verification
I did ablate the new leak guard rather than trust it: with a fake AskCould you re-run the live gate on your psmux 3.3.7 rig? Specifically: confirm the env-scrubbed |
![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 `@tests/test_psmux_live.py`:
- Around line 207-217: Move the teardown kill command into the existing try
blocks in both fixtures, so subprocess.TimeoutExpired from the kill is caught by
the same exception handler as the leak probes. Preserve the current behavior of
setting leaked to True for OSError, TmuxError, or timeout failures and asserting
the session was removed.
🪄 Autofix
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: b2704561-efcb-4563-bdfc-57c0af03b185
📒 Files selected for processing (1)
tests/test_psmux_live.py
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
CodeRabbit review of 4045e0e: in `psmux_data_root` the teardown kill ran outside the try, so a `subprocess.TimeoutExpired` from the kill itself left the fixture before the new leak check could run. Valid, and it is the same defect the leak guard was added to close, one line earlier. That fixture cannot use `kill_session` because it needs `env=` for `PSMUX_DATA_DIR`, and raw `_run` propagates a timeout even under `check=False` — so the kill moves inside the try. Observed both ways against a fake psmux whose `kill-session` outlasts the 30s `_run` timeout: before, the fixture died with a bare `TimeoutExpired` naming no session; after, it reports `probe setup: data-probe session … survived teardown; kill it manually`. A kill that hung is exactly when the session is most likely still standing. The same review flagged the `probe` fixture's `mux.kill_session(session)` as needing the same move. It does not: `PsmuxMultiplexer.kill_session` already swallows `subprocess.SubprocessError`, and `TimeoutExpired` is a subclass, so nothing escapes. Left alone, with the asymmetry recorded next to the catch it would otherwise be re-raised against.
Bot round on
|
|
To use Codex here, create an environment for this repo. |
|
@codex review |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Resolves the CHANGELOG conflict that left the PR unmergeable, which is why no `pull_request` CI run existed for the branch head: GitHub could not build refs/pull/517/merge, so the workflow was never created. Both sides had prepended a new entry to the same `### Added` position — bmad-code-org#488's psmux premise probes here, bmad-code-org#502's `gate:` deferred-work entry on main. Keep both, newest first, which is the convention each side independently followed. No other file conflicted: the merged tree differs from main only in this PR's own four files.
Conflict resolved — CI is green on the real head
Correcting my previous comment. I said CI might be gated on fork-workflow approval. That was wrong. All 10 checks SUCCESS on Read as a trial merge, the merged tree differs from Bot state
Unchanged@dracic, the ask still stands: re-run the live gate on your psmux 3.3.7 rig and confirm the env-scrubbed |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@pbean live gate re-run complete on the Windows psmux rig.
One wrapper-only false alarm was diagnosed before reporting: I initially used unquoted No code changes were needed. |
2 participants
Closes #488
What
Eight premise probes in the Windows live gate. Each drives raw
psmuxargv against a throwawaysession and asserts the observed upstream behavior that a workaround in
psmux_backendexistsbecause of — never the backend verb the workaround implements.
The failure semantics are inverted on purpose: a red probe is the intended signal, and its
message names the workaround that has become droppable. That is the manual step from #222's
admission checklist turned into an assertion.
Plus the ablation half: every negative assertion the issue listed was ablated for real — guard
deleted, test run, result recorded, guard restored.
Why it was needed, concretely
Four of the eight premises had already flipped on psmux
main(d56d777,6c76ff9,33ea109,8edd1cb/194fac0) and nothing in this repo changed colour.Every premise was observed before it was asserted
All eight were driven against the installed 3.3.7 build first. Two are sharper than the issue
described:
set-option -wdoes not merely lack per-window storage — it silently lands at session scope(
show-options -wqvreads empty while the same key at session scope reads the value back).kill-windowon an unresolvable target exits 0 and destroys the ACTIVE window. The probe pinsexactly that: the session survives and the vanished window is precisely the one that was active.
Upstream
Tracing the
=-form kill divergence to its cause produced psmux/psmux#558:parse_targetstrips the
=for routing butPSMUX_TARGET_FULLkeeps the raw argument, so the server'skill-sessionfallback compares"name" == "=name"and never kills. Verified identical atv3.3.7and
main.A second candidate (CLI value corruption for
'and non-ASCII whitespace) was traced anddeliberately not filed —
e41886falready fixed it onmainvia a Unicode-awarequote_arg_if_needed. Consequence worth flagging for the 3.3.8 admission: those two_transportablebranches become droppable, and the round-trip probe here will go red and say so.
Ablation results
:276,:404,:957==/raisesfires first:96,:114,:824,:1618:1625—last_fallbackdefaulted false, so the-lleg was unreachable regardless of the early-return guard it claimed to cover:1647— reached its assertion only viaIndexErrorfrom a fixture that ran dryThe issue's list also mislabels
:414: it is a positive assertion. The negative one in that testis
:404.Five further weakly-discriminating negatives outside the issue's scope (they ablate into a
TypeErrorrather than reaching their assertion) were recorded in the deferred-work ledger ratherthan pulled into this change.
Invariants held
pwsh -NoProfile -Command exit 0; no coding CLI is launched.verified teardown.
PSMUX_DATA_DIRisolation is applied only when the installed builddemonstrably honors it. Bare
switch-client -lis not probed at all — it has no target form, sounder
$TMUXit would move the developer's own client._LAST_UNSUPPORTEDbump. This builds the instrument; psmux 3.3.8: adopt as installed floor, re-verify the switch-client/select-window paths, and drop the psmux#497 and other workarounds #222 owns theadmission.
Verification
tests/test_psmux_live.py+tests/test_psmux_backend.py: 154 passed on Windows with psmux 3.3.7trunk check: no issuesas first written it was vacuous, since
available()applies the same regex and skips the teston the very failure the probe exists to catch
Summary by CodeRabbit
Bug Fixes
Tests
Documentation
psmuxbehavior and recorded updated validation details in the changelog.