fix(adapters,engine): tell a lost mux session apart from an exited CLI - #522
fix(adapters,engine): tell a lost mux session apart from an exited CLI#522dracic wants to merge 1 commit into
Conversation
|
Warning Review limit reached
Next review available in: 45 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 (3)
WalkthroughThe change detects vanished multiplexer sessions during crash handling. It records ChangesSession-loss diagnostics
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant GenericAdapter
participant Multiplexer
participant Escalation
participant Journal
GenericAdapter->>Multiplexer: Probe session existence after a crash
Multiplexer-->>GenericAdapter: Return confirmed presence or absence
GenericAdapter->>Escalation: Provide SessionResult.session_vanished
Escalation-->>Journal: Record diagnostic reason without changing routing
GenericAdapter->>Journal: Emit session-vanished lifecycle data
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
🧹 Nitpick comments (2)
src/bmad_loop/adapters/generic.py (1)
298-311: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace
getattrwith direct attribute access.
getattr(self, "session_name")with no default has the same failure behavior asself.session_name. Both raiseAttributeErrorif the attribute is missing, so the "fail loud, no default" intent in the comment holds either way. Use direct attribute access; it is equally safe and more idiomatic.🔧 Proposed fix
self._note_lifecycle( handle.task_id, "session-vanished", - session=getattr(self, "session_name"), + session=self.session_name, status=status, )🤖 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/generic.py` around lines 298 - 311, In the vanished-session branch of the lifecycle handling, replace getattr(self, "session_name") with direct self.session_name access when passing the session value to _note_lifecycle. Preserve the existing fail-loud behavior and all other arguments unchanged.Source: Linters/SAST tools
CHANGELOG.md (1)
163-172: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the
CHANGELOG.mdentry terse and imperative.The entry uses a long narrative and a declarative opening. Replace it with a short imperative summary that names the diagnostic fields and states that routing is unchanged.
Proposed wording
-- **A lost multiplexer session no longer reads as an agent that crashed (`#489`).** A window is - equally gone when the CLI exits and when something destroys the whole session under the run ... +- **Improve crash diagnosis when the multiplexer no longer reports a session (`#489`).** Include + the diagnostic in crash reasons, `session-end`/`dev-decision` journal entries, and + `session-vanished` lifecycle breadcrumbs. Preserve environment-fault composition and retry routing.As per coding guidelines,
CHANGELOG.mdentries must be underUnreleasedand remain 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 - 172, Rewrite the CHANGELOG entry as a terse, imperative summary under the Unreleased section. Name the affected diagnostic fields—crash verdict, operator-facing reason, session_vanished journal entry, and session-vanished lifecycle breadcrumb—and explicitly state that routing is unchanged.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 `@docs/tui-guide.md`:
- Around line 223-224: Update the `session-vanished` diagnostic description in
the event list to say “the mux no longer reported the session during the run”
instead of asserting that the mux lost the session, preserving the wording as an
unconfirmed negative lookup.
---
Nitpick comments:
In `@CHANGELOG.md`:
- Around line 163-172: Rewrite the CHANGELOG entry as a terse, imperative
summary under the Unreleased section. Name the affected diagnostic fields—crash
verdict, operator-facing reason, session_vanished journal entry, and
session-vanished lifecycle breadcrumb—and explicitly state that routing is
unchanged.
In `@src/bmad_loop/adapters/generic.py`:
- Around line 298-311: In the vanished-session branch of the lifecycle handling,
replace getattr(self, "session_name") with direct self.session_name access when
passing the session value to _note_lifecycle. Preserve the existing fail-loud
behavior and all other arguments unchanged.
🪄 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: 4fb9255d-fbe8-4b02-a0b0-0e7bc43550aa
📒 Files selected for processing (15)
CHANGELOG.mddocs/FEATURES.mddocs/tui-guide.mdsrc/bmad_loop/adapters/base.pysrc/bmad_loop/adapters/generic.pysrc/bmad_loop/adapters/multiplexer.pysrc/bmad_loop/adapters/tmux_base.pysrc/bmad_loop/engine.pysrc/bmad_loop/escalation.pysrc/bmad_loop/sweep.pytests/test_engine.pytests/test_escalation.pytests/test_generic_tmux.pytests/test_plugin_workflows.pytests/test_sweep.py
Sessions complete on a hook Stop or on window death, and `list_window_ids` answers [] for both "the CLI exited" and "the whole session is gone" — a missing session exits non-zero and degrades to the same empty list. So a session destroyed under the run (an external reaper, a concurrent prune or stop, an operator kill-session, a server crash, the host sleeping) scored `crashed` exactly like a clean CLI exit, and the retry/defer reason an operator reads said only `dev session crashed`, pointing at the agent when the host was at fault. Ask `has_session` once a crash verdict is already reached, from the single `_final` chokepoint all five crash sites funnel through. Safe to ask that late: run()'s teardown kills the window, never the session. The answer rides `SessionResult.session_vanished` into the reason text, the `dev-decision` journal entry, and a `session-vanished` lifecycle breadcrumb. Worded as the observation, not the conclusion — the probe cannot tell who destroyed the session. Composed into `env_fault_pause_reason` so a lost session whose log also matches a transport pattern keeps both diagnoses instead of the env-fault branch swallowing one. A session reaped after flushing its result still scores `completed` and is not diagnosed: it produced something. Diagnosis only; routing is unchanged and a retry re-creates the session. Unit tests get a mux stand-in: the probe was the first unstubbed `self.mux` access on the crash path, so `make_dev_adapter`'s adapters were reaching the host multiplexer for real — scoring eight existing crash tests `session_vanished` and writing breadcrumbs, against that file's "unit tests need no tmux" contract. Refs bmad-code-org#489
7296287 to
264d84a
Compare
|
Addressed the CodeRabbit findings in 264d84a: the tui-guide breadcrumb description no longer overclaims ("no longer reported the session during the run"), and the CHANGELOG entry is rewritten terse and imperative. The |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 264d84a655
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
| # its own exit rather than the window dying — the label stays truthful | ||
| # there because it reports what the mux answered, not how the window | ||
| # ended. | ||
| vanished = status == "crashed" and self._session_vanished() |
There was a problem hiding this comment.
When the crashed fallback comes from the SessionEnd arm in wait_for_completion, the CLI has explicitly announced its exit; if the mux session is also absent by the time this probe runs, this stamps session_vanished=True and produces a reason claiming that the window disappearance is not evidence the CLI exited. That misdiagnoses exactly the exited-CLI case this change is meant to distinguish. Pass the crash origin into _final, or skip this probe for the SessionEnd path, so only a crash inferred from a failed window-liveness check receives this label.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Verified against the code at 264d84a — declining this one. The conjunction it describes (SessionEnd received AND session absent at probe time) can only arise when a host-level actor destroyed the session, and that is precisely when the suffix must fire:
- A normal CLI exit can never make
_session_vanished()true. Window 0 is a parked shell ("Window 0 is a plain shell so the session survives task windows closing",tmux_base.py), andrun()'s teardown kills only the window, never the session (comment in_session_vanished). So reaching this corner requires an independent destroyer — mux server crash, external reaper, operator/concurrentkill-session, host sleep — the exact candidates enumerated ingeneric.py's probe comment. - In every one of those cases, "the multiplexer no longer reports the session" is a true and operator-relevant fact that an announced CLI exit cannot explain. The likeliest route into the corner is causal, not coincidental: the destroyer HUPs the CLI, whose hook flushes
SessionEndon the way down. Skipping the probe on theSessionEndarm would make exactly that case read as a plain CLI crash — reintroducing the psmux can destroy a live session out from under a run (psmux#546) — decide the exposure and whether the reconcile can tell #489 misdiagnosis this PR fixes. - The suffix withdraws an inference; it does not assert the CLI failed to exit. That wording is deliberate on both sides of the seam:
session_failure_reason's docstring ("states what the evidence withdraws, not what it proves") and_final's comment, which names theSessionEndarm explicitly as considered. Routing is unchanged either way, and theSessionEndreceipt remains on disk in the run'sevents/directory (SignalWatcher never unlinks consumed event files), so no evidence is lost to the operator.
Threading the crash origin into _final to vary one explanation string in a double-fault corner would trade real plumbing for a marginal wording refinement; if anything, the improvement would be phrasing that acknowledges both facts, not narrowing the probe's scope.
2 participants
Addresses Q1 of #489 (the shared-ctl-session/untagged-fallback question, Q2, stays out — it is gated on #419)
Problem
Sessions complete on a hook
Stopevent or on window death — a hard invariant. But_window_aliveis a membership test overlist_window_ids(session), and that list is empty for two different worlds: the window died inside a live session (the CLI exited), and the session itself no longer exists. Both scoredcrashed, so a session destroyed under a run (an external reaper such as psmux/psmux#546, this tool's own prune/stop, an operatorkill-session, a mux server crash, a sleeping host) presented as an ordinary CLI crash — the reason an operator reads said onlydev session crashed, pointing at the agent when the host was at fault.Approach
Once a crash verdict is already reached,
_finalaskshas_session— the only call that separates the two worlds. The answer is a diagnostic label, never a routing input:SessionResult.session_vanished, stamped only when the verdict is alreadycrashed— never to reach a verdict, and never on a read-back upgrade tocompleted(a session reaped after flushing its result did produce something)session_failure_reason(… session crashed: the multiplexer no longer reports the session, so the window's disappearance is not evidence the CLI exited), adopted at the dev/review deciders, the blocking-workflow defer, and the sweep migration/triage sitessession-endentry via the_session_end_extraschokepoint (besideenv_fault), plusdev-decisionsession-vanishedbreadcrumb insession-lifecycle.jsonlcarrying the session name and verdictThe wording states what the evidence withdraws, not what it proves: the weak-False contract (
False= "the backend did not confirm the session"; transport failure raisesMultiplexerError, never returnsFalse) is now declared on theTerminalMultiplexer.has_sessionseam.MultiplexerErrorfrom the probe degrades to "not vanished" — the same "unknown is not dead" rule the liveness probe follows.Routing is deliberately untouched:
_ensure_sessionre-creates the session, so a retry already self-heals. Adapters with no session to lose (opencode-http) are inert via a constant-Falsebase hook.Testing
status == "crashed"gate or the env-fault composition fails the covering tests; the gate test reads the final status, so a regression to gating on the fallback also failssession-end+dev-decisionjournal fields (True on a vanished crash; absent/False on a plain crash), the read-back-upgrade skip, the non-crash skip, the post-kill-reconcile pass-through of a flagged verdict, and the blocking-workflow defer reason end to end_UnitMux);uv run pytestgreen,uv run pyrightclean but for the pre-existingplatform_utilwin32 pair,trunk checkcleanSummary by CodeRabbit
Bug Fixes
Documentation