fix(cleanup,mux): honest kill outcomes (#435) and the dropped version diagnostic (#428) - #524
Conversation
…ed version diagnostic Two operator-visible reports were optimistic to the point of being wrong. `cleanup` counted every ctl window a kill was *attempted* on as removed — in the text summary, the TUI toast and `--json` — because `kill_window` is best-effort and reports nothing. The prune now takes one liveness listing after its kills and partitions the result into removed / survived / unverifiable, the shape `prune_sessions` already returns for sessions. The verdict is taken in the prune rather than pushed into the seam: a backend cannot verify a kill from `kill_window(target)` alone (a bare `@N` carries no session, and every liveness listing is session-scoped), and the prune is the one caller that both needs the answer and already holds the session. `ctl_windows.removed` now means verifiably gone, so CLEANUP_SCHEMA_VERSION moves to 2. `version()` collapsed a crashing probe — corrupt install, AV-blocked exe, hung server — to the same None a binary reporting no version returns, and the probe's stderr was gone. It keeps that contract; a new non-abstract `version_error()` carries the dropped diagnostic, and `bmad-loop mux` prints it beside the `-` in the VERSION column. UnicodeError joins the catch: strict POSIX decoding makes an undecodable `-V` raise outside the SubprocessError family, which is exactly the corrupt-binary case the diagnostic exists for. Closes bmad-code-org#435 Closes bmad-code-org#428
…e raises `cmd_cleanup` kills the agent sessions first, then prunes ctl windows — and the second half is raiser-side (its candidate scan probes `has_session`). An unguarded raise reached `main()`'s backstop, which prints an error and returns 1 with stdout empty. Under `--json` that destroys the record of the kills that already happened: a consumer cannot tell "killed nothing" from "killed three and lost the receipt". The repair had already succeeded; only the observation failed, and observation degrades. Guarded like the TUI worker has been all along, naming the failure on stderr and reporting three empty window lists — the honest answer, since the raise comes from the candidate scan and no window was killed or even chosen. Pre-existing, surfaced reviewing bmad-code-org#435: that change made the window half of the document worth trusting, which is what makes losing the whole document matter.
|
Warning Review limit reached
Next review available in: 51 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. WalkthroughChangesCleanup and multiplexer reporting
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant CleanupCLI
participant prune_ctl_windows
participant Multiplexer
participant cleanup_document
CleanupCLI->>prune_ctl_windows: prune candidate control windows
prune_ctl_windows->>Multiplexer: kill candidate windows
prune_ctl_windows->>Multiplexer: list window IDs once
Multiplexer-->>prune_ctl_windows: remaining IDs or verification failure
prune_ctl_windows-->>CleanupCLI: removed, survived, unverifiable
CleanupCLI->>cleanup_document: build schema version 2 document
cleanup_document-->>CleanupCLI: structured cleanup JSON
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 |
![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
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_launch.py (1)
373-445: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winUse the shared
projectsandbox fixture.Lines 390-395 create
.bmad-loopstate directly undertmp_path. Change_ctl_prune_fakeand its callers to use theprojectfixture andproject.project. This keeps the tests aligned with the repository sandbox contract.As per coding guidelines, “Use the
projectconftest sandbox fixture rather than hand-rolled temporary repositories or the template repository.”🤖 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_launch.py` around lines 373 - 445, Update _ctl_prune_fake and every caller to accept and use the shared project sandbox fixture instead of tmp_path. Use project.project for the project path, and create the run state beneath that path while preserving the existing fake tmux behavior and assertions.Source: Coding guidelines
🧹 Nitpick comments (1)
CHANGELOG.md (1)
163-177: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReduce these entries to terse imperative changelog items.
These bullets contain detailed implementation rationale and schema semantics. Keep a short imperative summary in
CHANGELOG.md. Keep the full contract details indocs/FEATURES.md.As per coding guidelines, “Write CHANGELOG entries under
Unreleased; keep them terse, scannable, and imperative.”🤖 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 `@CHANGELOG.md` around lines 163 - 177, Replace the two detailed Unreleased changelog bullets with terse, scannable imperative summaries covering the cleanup reporting fix and version-probe error distinction. Remove implementation rationale and schema/contract details from CHANGELOG.md, while preserving those details in docs/FEATURES.md.Source: Coding guidelines
🤖 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/tui/launch.py`:
- Around line 345-351: Update BaseTmuxBackend.list_window_ids to raise
MultiplexerError for nonzero list-windows results unless it verifies that the
target session is gone, rather than returning an empty set. In the cleanup flow
around mux.list_window_ids(CTL_SESSION), classify this typed failure as
unverifiable instead of marking all candidates removed. Add a regression test
covering a nonzero listing failure while the session remains live.
---
Outside diff comments:
In `@tests/test_tui_launch.py`:
- Around line 373-445: Update _ctl_prune_fake and every caller to accept and use
the shared project sandbox fixture instead of tmp_path. Use project.project for
the project path, and create the run state beneath that path while preserving
the existing fake tmux behavior and assertions.
---
Nitpick comments:
In `@CHANGELOG.md`:
- Around line 163-177: Replace the two detailed Unreleased changelog bullets
with terse, scannable imperative summaries covering the cleanup reporting fix
and version-probe error distinction. Remove implementation rationale and
schema/contract details from CHANGELOG.md, while preserving those details in
docs/FEATURES.md.
🪄 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: aa62f217-5010-45e7-aab3-def929fee172
📒 Files selected for processing (15)
CHANGELOG.mddocs/FEATURES.mdsrc/bmad_loop/adapters/multiplexer.pysrc/bmad_loop/adapters/tmux_base.pysrc/bmad_loop/cli.pysrc/bmad_loop/documents.pysrc/bmad_loop/tui/app.pysrc/bmad_loop/tui/launch.pytests/test_backend_registry.pytests/test_cli.pytests/test_multiplexer.pytests/test_psmux_backend.pytests/test_psmux_live.pytests/test_tui_app.pytests/test_tui_launch.py
| live = set(mux.list_window_ids(CTL_SESSION)) | ||
| except MultiplexerError: | ||
| # The kills may well have landed; nothing here can say so. Claiming the | ||
| # optimistic half is exactly the bug — the next cleanup pass retries. | ||
| return [], [], [name for _win_id, name in candidates] | ||
| removed = [name for win_id, name in candidates if win_id not in live] | ||
| survived = [name for win_id, name in candidates if win_id in live] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Do not treat every nonzero liveness listing as a verified removal.
Line 345 can receive [] when BaseTmuxBackend.list_window_ids() gets any nonzero list-windows exit. A server-side failure can leave CTL_SESSION and its windows alive. Lines 350-351 then classify every candidate as removed.
Make the multiplexer seam raise MultiplexerError unless it can confirm that the session is gone. Classify that typed failure as unverifiable. Add a regression test for a nonzero listing while the session remains live.
As per coding guidelines, “At I/O boundaries, fail loudly: use typed escalation rather than bare except; observation may degrade, but repair writes must raise.”
🤖 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/tui/launch.py` around lines 345 - 351, Update
BaseTmuxBackend.list_window_ids to raise MultiplexerError for nonzero
list-windows results unless it verifies that the target session is gone, rather
than returning an empty set. In the cleanup flow around
mux.list_window_ids(CTL_SESSION), classify this typed failure as unverifiable
instead of marking all candidates removed. Add a regression test covering a
nonzero listing failure while the session remains live.
Source: Coding guidelines
There was a problem hiding this comment.
Correct diagnosis, and it is the one thing in this area I deliberately did not fix here. Tracked separately rather than declined.
BaseTmuxBackend.list_window_ids folds every nonzero exit to [], so yes — a server-side failure with the windows still alive reports them removed. The PR description calls this out as a known ceiling and the prune_ctl_windows docstring names it at the call site.
Two reasons it is not in this PR:
It is not a regression. Before this change the prune reported every attempted kill as removed, unconditionally. The rc≠0 path is now the only remaining way to get an over-optimistic answer, where previously it was the guaranteed answer. This PR strictly narrows the hole; it does not open it.
The suggested fix is not local to cleanup. list_window_ids backs window_alive, which is the engine's liveness probe. Today a vanished session answers [] → "dead", and the engine's crash/recovery paths are built on that. Making nonzero exits raise flips those to "unknowable" repo-wide, which is a behavior change to run supervision reached through a cleanup-reporting fix. It also needs a decision this PR has no basis to make: which stderr signatures count as proved gone versus a real fault. tmux's "session not found" and "no server running" are both nonzero and both genuinely mean gone.
So it needs its own change with its own blast-radius pass over the engine, not a rider here. Filing it as an issue with the ledger entry's reasoning.
One correction to the suggested classification: routing this to unverifiable is right for the fault case, but a session that really did die with its last window must stay removed. Pessimism there invents a phantom survivor that every subsequent cleanup re-reports and never clears — which is the same class of dishonest report from the other side. That distinction is exactly what the seam cannot currently make, and why the fix is a seam change rather than a caller change.
There was a problem hiding this comment.
@dracic, agreed. The limitation is real, but it predates this PR and requires a seam-level behavior decision with engine-wide review.
prune_ctl_windows() correctly improves the report for failures that raise MultiplexerError. It cannot distinguish a confirmed empty session from a failed list_window_ids() call that returns []. Treating all empty results as unverifiable would also misreport successful removal of the final window.
I am withdrawing this finding for this PR. A follow-up issue should define confirmed-session-absence handling and evaluate the window_alive and recovery call paths before changing BaseTmuxBackend.list_window_ids().
🐇🔍
✏️ Learnings added
Learnt from: dracic
Repo: bmad-code-org/bmad-loop PR: 524
File: src/bmad_loop/tui/launch.py:345-351
Timestamp: 2026-08-10T18:00:51.731Z
Learning: In `src/bmad_loop/adapters/tmux_base.py`, `BaseTmuxBackend.list_window_ids()` currently maps every nonzero `list-windows` exit to `[]`. This behavior is also used by `window_alive` and engine crash/recovery logic. A future change must distinguish a confirmed absent tmux session, such as “session not found” or “no server running”, from a transport or server fault. Review all engine liveness consumers before changing this seam.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
You are interacting with an AI system.
|
|
||
| def test_prune_ctl_windows(monkeypatch, tmp_path: Path): | ||
| def _ctl_prune_fake( | ||
| monkeypatch, tmp_path: Path, *, kill: str = "lands" |
There was a problem hiding this comment.
Replying here to CodeRabbit's outside-diff finding on this helper ("use the shared project sandbox fixture instead of tmp_path"). Declining, with the evidence:
The flagged setup is not new code. git diff main...HEAD shows no +/- lines for the .bmad-loop/runs/... / write_pid / project_tag block — git matched it as unchanged context. It was already in test_prune_ctl_windows; this PR extracted the surrounding test into a helper so four verdict cases could share it, and carried that block along verbatim.
tmp_path is this file's convention, not an oversight. tests/test_tui_launch.py references tmp_path 63 times and the project fixture 0 times. Converting only the five prune tests would leave one file with two sandbox conventions and no way to tell from a test which one applies.
The fixture is also the wrong tool for these tests. project copies a session-scoped template repo per test; what the prune needs is one state.json and a pid, so that a single window reads live and the rest do not. Everything else these tests touch is a faked tmux transport. Paying a repo copy per case buys no additional coverage.
The AGENTS.md rule the guideline comes from is aimed at hand-rolled repos — tests that need real git state and would otherwise reach for the template. These are transport tests with a run directory. If the file should move to project wholesale, that is a worthwhile separate cleanup of all 63 sites; doing five of them inside a cleanup-reporting fix would make the inconsistency permanent rather than fix it.
| - **`cleanup` no longer reports a surviving ctl window as removed (#435).** Killing a window is | ||
| best-effort and reports nothing, so the prune counted every _attempted_ kill as a removal. It now | ||
| verifies with one liveness listing and partitions into removed / survived / unverifiable. | ||
| `ctl_windows.removed` means _verifiably gone_ and gains `survived` / `unverifiable` siblings, so |
There was a problem hiding this comment.
Applied, partly — the #435 entry was 9 lines against a median of 6 across the 113 entries under Unreleased; trimmed to 6 in 8e824c0 by cutting the prune_sessions comparison and the --dry-run clause, both of which docs/FEATURES.md and the cleanup_document docstring already carry.
Kept deliberately, because a changelog reader who does not open FEATURES.md still needs them:
CLEANUP_SCHEMA_VERSIONis 2 — a consumer pinning the version has to see the bump here.- still at exit 0 — the count on stdout got smaller and the difference goes to stderr; a wrapper checking
$?sees no change and would otherwise have no signal that anything moved. sessions.removedis untouched and still an attempted kill — without it, "cleanup now verifies removals" reads as covering both halves of the document when it covers one.
The #428 entry is 6 lines, exactly the file's median, so it is left as is.
1 participant
Two operator-visible reports that were optimistic to the point of being wrong. Paired because both are "adapter seam + operator-visible output" — one pass, one review.
#435 —
cleanupreported a surviving ctl window as removedkill_windowis best-effort and reports nothing, so the prune counted every attempted kill as a removal: in the text summary, the TUI toast, andcleanup --json.The prune now kills every candidate, takes one
list_window_ids(CTL_SESSION), and partitions by membership intoremoved/survived/unverifiable.Deviation from the issue's sketch, deliberate. #435 proposes threading a
bool | tri-statethroughkill_windowacross every backend. That cannot work: a backend cannot verify a kill fromkill_window(target)alone — a bare@Ncarries no session, and every liveness listing is session-scoped — so all backends except psmux (which already does this internally for its own key cleanup) would be forced to answer "unverifiable". The ctl prune is the one caller that both needs the answer and already holds the session, and one post-kill listing there covers every backend uniformly at one extra round trip per prune instead of one per window. No seam signature changes.ctl_windows.removednow means verifiably gone, which is a meaning change, soCLEANUP_SCHEMA_VERSIONis 2. Text mode marks the stdout count and names the two non-removed arms on stderr, still at exit 0. Survivors are retried by the nextcleanup, as before.sessions.removedis untouched and still an attempted kill —kill_sessionis silent in exactly the waykill_windowis. The docstring now says so rather than implying the sessions half was already trustworthy.#428 — a crashed version probe was indistinguishable from "reports no version"
version()keeps itsNonecontract (observation may degrade). A new non-abstractversion_error()carries the diagnostic it dropped, andbmad-loop muxprints it as a whitespace-collapsedwarning:on stderr below the table.UnicodeErrorjoins the catch:_rundecodes with the locale codec and the strict handler on POSIX, so an undecodable-Vraised aValueErroroutside theSubprocessError/OSErrorfamily and escaped as a raw crash — a corrupt install, exactly what the diagnostic exists for.The read is deliberately not gated on
available(): psmux's availability probe calls its ownversion(), so an AV-blocked binary readsavailable=False— the row that most needs an explanation is the one where availability already failed.Second commit
cmd_cleanuphad noMultiplexerErrorguard while the TUI twin has had one all along. The sessions are already killed by the time the raiser-side ctl half runs, so a raise reachedmain()'s backstop and left stdout empty — under--jsonthat destroys the record of those kills. Pre-existing; surfaced by review, and it matters more now that the window half of the document is worth trusting.Notes for the reviewer
Two ceilings are documented rather than fixed:
BaseTmuxBackend.list_window_idsfolds every nonzero exit to[], not just a vanished session, so a server that errors while its windows live still reports them removed. Strictly less optimistic than before this change (which reported everything removed unconditionally), and narrowing that sentinel changeswindow_alive— the engine's liveness probe — repo-wide. Tracked separately.list_windows'window_idcolumn againstlist_window_ids— a third symmetry the seam did not state. It now does, in thelist_windowsdocstring, because a backend qualifying one side and not the other reports every killed window as gone, silently and optimistically.Verification
Full suite,
pyright, andtrunk checkclean against the local baseline. The new negative assertions were ablated: deleting the candidate guard, the survivor partition, the availability-independent diagnostic read, the session argument of the liveness probe, and theMultiplexerErrorguard each makes the owning test fail.tests/test_psmux_live.pygains a suppressed-kill survivor leg — the half that actually pins the qualified-id symmetry against a real server — and a portable twin lands intests/test_psmux_backend.pyso CI holds it too. Zero LLM tokens consumed by any test.Closes #435
Closes #428
Summary by CodeRabbit
New Features
bmad-loop muxdisplays warnings when backend version checks fail.Bug Fixes