From 264d84a6556a3170cddc9c60cc4aadee92c98baf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Davor=20Raci=C4=87?= Date: Mon, 10 Aug 2026 10:04:22 +0200 Subject: [PATCH] fix(adapters,engine): tell a lost mux session apart from an exited CLI MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 #489 --- CHANGELOG.md | 8 ++ docs/FEATURES.md | 3 +- docs/tui-guide.md | 3 +- src/bmad_loop/adapters/base.py | 8 ++ src/bmad_loop/adapters/generic.py | 64 +++++++++++- src/bmad_loop/adapters/multiplexer.py | 11 ++- src/bmad_loop/adapters/tmux_base.py | 10 ++ src/bmad_loop/engine.py | 16 ++- src/bmad_loop/escalation.py | 43 +++++++- src/bmad_loop/sweep.py | 6 +- tests/test_engine.py | 35 +++++++ tests/test_escalation.py | 42 ++++++++ tests/test_generic_tmux.py | 135 +++++++++++++++++++++++++- tests/test_plugin_workflows.py | 20 ++++ tests/test_sweep.py | 36 +++++++ 15 files changed, 422 insertions(+), 18 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ff817c6f..3b52fa99 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -160,6 +160,14 @@ whose seams had diverged enough that several ports needed a different fix, and t ### Fixed +- **Diagnose a lost multiplexer session on the crash path (#489).** A dead window and a session + destroyed under the run (a reaper, this tool's own prune/stop, an operator `kill-session`, a + server crash) both scored `crashed` and read as an agent fault. Probe `has_session` on a crash + verdict and carry the answer in the failure reason, on every role's `session-end` journal entry + (`session_vanished`), and as a `session-vanished` lifecycle breadcrumb, composed with an + environment-fault pause. Diagnosis only: routing is unchanged and a retry re-creates the + session. + - **A native-Windows install driven from a WSL shell now says so (#332).** WSL appends the Windows `PATH` to its own, so a bash prompt can reach a Windows-installed `bmad-loop`: that interpreter reports `win32`, takes the psmux platform default, and never sees the distro's tmux — while diff --git a/docs/FEATURES.md b/docs/FEATURES.md index 2ee41089..a55f9d43 100644 --- a/docs/FEATURES.md +++ b/docs/FEATURES.md @@ -68,6 +68,7 @@ See [README.md](../README.md) for the narrative overview and [setup-guide.md](se - Dispatched sessions are told the sprint board is orchestrator-owned (#437) — the sibling of the park contract above, injected into the prompt the same way. The board advances as soon as dev verifies, but the story's single commit lands only after the review loop, so a session dispatched in between opens on an uncommitted, unattributed change to `sprint-status.yaml` with nothing in the repo naming its author (one read it as a spec violation, reverted it, and tripped the sign-off-regression gate on a story both sessions agreed was finished). Story dev prompts and the review prompts of sprint and sweep runs carry the same prohibition: never write the board, never revert it, and a row at `done` or `awaiting-operator` is the orchestrator's own bookkeeping — not a defect to fix, and not proof that the work is verified, deliberately, since the row is written _before_ the deterministic dev verification runs and a repair session opens on a red tree under a `done` row. Only the **review** prompt adds where to go instead: a story that cannot be finished without a human decision is finalized to `status: blocked` with a reason — the one hand-back that both withholds the commit and reaches a human, where any other non-terminal status just burns the review budget onto a defer that rolls the work back. A dev prompt gets no such invitation, because `blocked` halts the whole run — the exact failure park exists to avoid — and a dev session that cannot finish already has park. A deferred-work bundle's dev prompt carries nothing (a bundle has no board row) while a bundle's _review_ prompt does, since a sweep runs inside a project whose board exists and is just as revertible; every injected plugin-workflow session carries the prohibition too — `post_dev_phase`, `post_review_result` and `pre_commit_gate` all fire inside that same window — as its own `## Sprint board` section appended _after_ the session-gate hooks, so a plugin prompt rewrite cannot strip it, and without the `blocked` redirect for the same reason a dev prompt has none; stories mode carries none of it, having no board at all. - Typed escalations: `CRITICAL` pauses the run + notifies (desktop + `ATTENTION` file); `PREFERENCE` is journaled and continues. - Environment faults pause without burning budget (#194): a session whose coding CLI never reached the API — a verify command whose _environment_ is broken (`sh` reports rc `126`/`127`; on Windows a missing tool is caught by its `is not recognized` message or by resolving the command's leading token, and a command naming a file `cmd` cannot execute — a `.sh`, or any extension outside `PATHEXT`, which cmd hands to the file association and which exits `0` without running anything — is a fault rather than a silent rc `0` pass, #302) **or** a session whose log matches the profile's `env_fault_patterns` (an `API Error … Connection refused`-class transport failure, or a provider quota/usage-limit refusal, that idled out the session clock) — pauses the run with the matched evidence instead of charging the attempt and deferring the story as if its code were broken. Re-arm restores the budget. Patterns are per-profile: `claude` seeds the connection signature, `opencode` a provider quota/rate-limit and connection pair (#323), and the other four profiles ship none. Each adapter matches them against the log named by its `ENV_FAULT_LOG_SUFFIX` — the tmux pane capture `logs/.log`, or `.server.out` (the `opencode serve` process's own stdout) for `opencode-http`, never that adapter's model-written transcript. A pattern is only sound against a log the model cannot write to, which is why the pane-capture profiles anchor an error token plus a cause on the same line and seed no bare quota patterns. Extend or disable them in a project profile overlay. +- A session the multiplexer lost says so (#489). Sessions complete on a hook `Stop` or on window death, and a window is gone whether the CLI exited or something destroyed the whole mux session out from under the run — an external reaper, a concurrent prune or `bmad-loop stop`, an operator `kill-session`, a server crash, the host sleeping. Both are `crashed`, so the retry/defer reason an operator reads said only `dev session crashed` — pointing at the agent when the host was at fault. The crash verdict now asks whether the _session_ still exists and, when it does not, says so in the reason (`… session crashed: the multiplexer no longer reports the session, so the window's disappearance is not evidence the CLI exited`), as `session_vanished` on every role's `session-end` journal entry (and on `dev-decision`, beside the routing it fed), and as a `session-vanished` breadcrumb in `session-lifecycle.jsonl`. The wording states what the evidence _withdraws_, not what it proves: `has_session` maps every nonzero backend result to False, so a negative lookup is "the backend did not confirm it" rather than proof the session is gone — enough to stop an operator reading window death as a CLI exit, not enough to name a destroyer. It composes with an environment-fault pause instead of being swallowed by it. A session reaped _after_ flushing its result still scores `completed` and is not diagnosed — it produced something. Diagnosis only — the routing is unchanged, and a retry re-creates the session. - CRITICAL resolution: `bmad-loop resolve ` opens an interactive resolve agent seeded with the escalation + frozen spec; you disambiguate, it re-arms the story (`escalated → pending`, spec reset to `ready-for-dev`) and resumes. `--no-interactive` skips to re-arm if you fixed the spec yourself. - Intent-gap patch-restore (BMAD-METHOD#2564): when review halts on an `intent gap`, the dev primitive saves the attempted change as a patch file (referenced from the halt output) before reverting the tree. If that reading turns out to be correct, the resolve agent adds `"restore_patch": ""` to its `resolution.json`; the orchestrator re-arms the spec to `in-review` (not `ready-for-dev`) and re-applies the patch after every reset, so the re-driven session resumes _review_ on the restored diff instead of re-implementing. `bmad-loop resolve --no-interactive --restore-patch ` does the same by hand. A patch that fails to apply escalates rather than dispatching onto a half-restored tree. Sweep bundles get the same recovery. @@ -101,7 +102,7 @@ See [README.md](../README.md) for the narrative overview and [setup-guide.md](se - Every run is a resumable on-disk state machine: `bmad-loop resume ` continues from a gate, escalation, or interruption. - A graceful stop (`stop --graceful` / TUI `S`) is resumable too: unlike a hard stop killed mid-item, it lets the in-flight item finish through commit and finalizes cleanly, ending as a `stopped` run that `resume` picks up at the next item. -- All run state in `.bmad-loop/runs//` (gitignored): `state.json`; `journal.jsonl` (every decision, including the `session-synthesized-from-frontmatter` catch and its `spec-marker-repaired` repair, #276); `events/` (hook signals); `tasks//` (per-session prompt + `result.json` + breadcrumbs — `session-lifecycle.jsonl` records timeout fires, budget-guard trips (`budget-tripped` / `over-budget-fired`), transport-failure classification (`env-fault-classified`, #194) and the #276 forensics (`spec-status-transition-observed`, `frontmatter-unmodified-refused`, `contract-nudge-sent`); `heartbeat.json` is the wait loop's proof-of-life; `resultless-stops.jsonl` records give-up Stops with a verdict — `no-artifact`, `ambiguous-frontmatter`, `unmodified-since-launch`, `terminal-frontmatter-pending`); `logs/`; `deferred/`; `resolve/`; `ATTENTION`. +- All run state in `.bmad-loop/runs//` (gitignored): `state.json`; `journal.jsonl` (every decision, including the `session-synthesized-from-frontmatter` catch and its `spec-marker-repaired` repair, #276); `events/` (hook signals); `tasks//` (per-session prompt + `result.json` + breadcrumbs — `session-lifecycle.jsonl` records timeout fires, budget-guard trips (`budget-tripped` / `over-budget-fired`), transport-failure classification (`env-fault-classified`, #194), a mux session lost under the run (`session-vanished`, #489) and the #276 forensics (`spec-status-transition-observed`, `frontmatter-unmodified-refused`, `contract-nudge-sent`); `heartbeat.json` is the wait loop's proof-of-life; `resultless-stops.jsonl` records give-up Stops with a verdict — `no-artifact`, `ambiguous-frontmatter`, `unmodified-since-launch`, `terminal-frontmatter-pending`); `logs/`; `deferred/`; `resolve/`; `ATTENTION`. - `journal.jsonl` records `session-end` for every session unconditionally — even a teardown that throws still lands one (status `aborted` when the outcome is unknowable). A timed-out session's entry carries `fired_at` (wall time the deadline was declared), `teardown_s` (wall seconds from that fire to this entry — the teardown gap), and `expired_clock` (`monotonic` / `wall` / `both` — `wall` alone fingerprints a host suspend that froze the monotonic clock). Every entry whose usage was read carries `tokens` (raw) and `tokens_weighted` (cache reads at `limits.cache_read_weight`), keeping per-session spend reconstructible; both are `null` when the usage read failed, and both are absent on an `aborted` end. `tokens_weighted` is the end-of-session total — distinct from a tripped session's `budget_weighted`, the guard's mid-session sample at trip time. ### Hook-based transport (no pane-scraping) diff --git a/docs/tui-guide.md b/docs/tui-guide.md index 953a4716..3fbfd8af 100644 --- a/docs/tui-guide.md +++ b/docs/tui-guide.md @@ -220,7 +220,8 @@ One row per story (or sweep bundle/triage task) in the selected run: (the guard's mid-session sample at trip time). The matching `tasks//` dir holds the forensic breadcrumbs the adapter wrote while the session ran: `session-lifecycle.jsonl` (timeout-fire, - budget-guard `budget-tripped` / `over-budget-fired`, kill-escalation, and the + budget-guard `budget-tripped` / `over-budget-fired`, kill-escalation, + `session-vanished` (the mux no longer reported the session during the run, #489), and the #276 missing-marker forensics `spec-status-transition-observed` / `frontmatter-unmodified-refused` / `contract-nudge-sent`), `heartbeat.json` (the wait loop's proof-of-life — diff --git a/src/bmad_loop/adapters/base.py b/src/bmad_loop/adapters/base.py index e636aa95..eafa9294 100644 --- a/src/bmad_loop/adapters/base.py +++ b/src/bmad_loop/adapters/base.py @@ -145,6 +145,14 @@ class SessionResult: # and both fire on a CLI that launched and wedged without doing anything. Stop # is the only canonical event that means a turn actually ended. stop_seen: bool = False + # Set on a `crashed` verdict when the mux no longer reports the SESSION, not + # just its window (#489) — see `GenericAdapter._session_vanished` for why the + # two are otherwise indistinguishable. Diagnostic label only: it changes the + # reason text, never the routing. Deliberately NOT carried by + # `_post_kill_reconcile`'s hand-built result — that path gates on + # stalled/timeout/over_budget, which this flag can never accompany; add it + # there if `crashed` ever joins that rescue set. + session_vanished: bool = False class CodingCLIAdapter(ABC): diff --git a/src/bmad_loop/adapters/generic.py b/src/bmad_loop/adapters/generic.py index 9f0d1c89..94a82deb 100644 --- a/src/bmad_loop/adapters/generic.py +++ b/src/bmad_loop/adapters/generic.py @@ -233,6 +233,19 @@ def _log_evidence(self, handle: SessionHandle) -> bool | None: tees a pane log.""" return None + def _session_vanished(self) -> bool: + """Whether the whole multiplexer session is gone, asked only once a + crash verdict has already been reached (#489). Base: False — an adapter + with no session to lose (opencode-http) never vanishes. Overridden by + `GenericAdapter`. + + Same failure convention as `_window_alive`: `MultiplexerError` is the + seam's declared "couldn't ask" and the override swallows it to False. + Anything else propagates, exactly as it does from the liveness probe — + this is a label on a verdict already made, so it degrades rather than + second-guessing the verdict, but it does not swallow unknown faults.""" + return False + def _final( self, handle: SessionHandle, @@ -274,6 +287,29 @@ def _final( ) result_json = None status = "completed" if result_json is not None else fallback + # Diagnose the crash verdict only (#489) — see `_session_vanished`. A + # read-back upgrade to `completed` is deliberately not diagnosed: a + # session reaped AFTER flushing its result did produce something, and the + # verdict it earned is the honest one. `crashed` also covers the + # `SessionEnd` arm of `GenericAdapter.run()`, where the CLI announced + # 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() + if vanished: + # Evidence rides along like every neighbouring crumb: which session + # went missing (several runs share a host) and what verdict it lands. + # getattr because the mixin does not declare `session_name` (opencode- + # http has none) and only a mux-backed adapter can reach this branch + # (the base `_session_vanished` is a constant False). No default — an + # override on an adapter without a session name must fail loud here, + # not write evidence-free crumbs. + self._note_lifecycle( + handle.task_id, + "session-vanished", + session=getattr(self, "session_name"), + status=status, + ) return SessionResult( status=status, result_json=result_json, @@ -281,6 +317,7 @@ def _final( transcript_path=transcript, budget_weighted=budget_weighted, stop_seen=stop_seen, + session_vanished=vanished, ) def _result_path(self, task_id: str) -> Path: @@ -914,6 +951,29 @@ def _log_activity_key(self, task_id: str) -> tuple[int, int] | None: def _window_alive(self, handle: SessionHandle) -> bool: return handle.native_id in self.mux.list_window_ids(self.session_name) + def _session_vanished(self) -> bool: + # The disambiguating probe (#489): `list_window_ids` returns [] for a + # dead window AND for a session that no longer exists, so a plain + # window-death verdict cannot tell an exited CLI from a session destroyed + # under the run. Only `has_session` separates them. + # + # The destroyer is NOT necessarily foreign. Candidates: an external + # reaper (psmux/psmux#546), a concurrent + # `runs.kill_session` from this tool's own prune/stop/crash paths or the + # TUI, an operator `kill-session`, a mux server crash, the host sleeping. + # The reason text stays neutral about which, because this probe cannot + # tell them apart — it reports that the mux no longer answers for the + # session, nothing more. + # + # Safe to ask this late: `run()`'s teardown kills the WINDOW, never the + # session, so our own kill cannot fake a vanishing, and a session once + # gone stays gone. + try: + return not self.mux.has_session(self.session_name) + except MultiplexerError: + # Unknown is not vanished — the same rule the liveness probe follows. + return False + def send_text(self, handle: SessionHandle, text: str) -> None: self.mux.send_text(handle.native_id, text) @@ -1771,8 +1831,8 @@ def _post_kill_reconcile( # this rescue exists for a session that finished but lost its Stop, not for # one that never ran. A session that ended no turn and whose pane log never # grew produced nothing, so a qualifying artifact is not its output — keep - # the stall/timeout verdict. This is the call path the issue's second - # occurrence (story i-11) took. + # the stall/timeout verdict. This is the call path the incident's second + # occurrence took. if not self._produced_work(handle, result.stop_seen): self._note_lifecycle( handle.task_id, diff --git a/src/bmad_loop/adapters/multiplexer.py b/src/bmad_loop/adapters/multiplexer.py index fe8c0286..4c3ed98e 100644 --- a/src/bmad_loop/adapters/multiplexer.py +++ b/src/bmad_loop/adapters/multiplexer.py @@ -92,7 +92,16 @@ def target(self, session: str, window: str | None = None) -> str: @abstractmethod def has_session(self, name: str) -> bool: - """True iff a session named exactly ``name`` exists.""" + """True iff a session named exactly ``name`` exists. + + Weak False (#489): a False means the backend did not *confirm* the + session, not that it provably no longer exists — implementations map + any failed lookup ("no such session", "no server running", a target + the grammar could not parse) to False alike. A transport failure + (the backend could not be asked at all) raises ``MultiplexerError`` + rather than returning False. Callers that surface a False as + evidence must word it as what the negative withdraws, not what it + proves — see ``escalation.session_failure_reason``.""" @abstractmethod def new_session( diff --git a/src/bmad_loop/adapters/tmux_base.py b/src/bmad_loop/adapters/tmux_base.py index 399e2516..8db38f53 100644 --- a/src/bmad_loop/adapters/tmux_base.py +++ b/src/bmad_loop/adapters/tmux_base.py @@ -116,6 +116,16 @@ def has_session(self, name: str) -> bool: # error), so this can't use check=True. But a timeout or a missing binary # is a real backend failure: raise the seam type so callers catch it via # MultiplexerError instead of a raw subprocess error escaping. + # + # Strength of a False: EVERY nonzero exit maps to it — "no such session", + # "no server running", and a target the grammar could not parse alike. That + # is exactly right for the create-if-missing callers this predicate was + # written for, where a wrong False self-corrects on the next line. It is + # weaker than it looks for a caller that reports the answer as evidence + # (#489), which is why that one words its output as what the negative + # withdraws rather than what it proves. Deliberately NOT tightened here: + # `list_window_ids` raises on transport failure because it backs a liveness + # probe, and this predicate has no such duty to its existing callers. try: probe = self._run(["has-session", "-t", f"={name}"], check=False) except (subprocess.TimeoutExpired, OSError) as exc: diff --git a/src/bmad_loop/engine.py b/src/bmad_loop/engine.py index a3726720..c03f42d3 100644 --- a/src/bmad_loop/engine.py +++ b/src/bmad_loop/engine.py @@ -35,6 +35,7 @@ preference_escalations, review_exhausted, review_retry_or_exhaust, + session_failure_reason, ) from .install import dev_primitive_or_default from .journal import Journal, save_state @@ -1397,7 +1398,7 @@ def _run_workflows(self, stage: str, task: StoryTask, seq: int) -> bool: ) self._defer( task, - f"blocking workflow {wf.name!r} ({lp.name}) did not complete: {result.status}", + session_failure_reason(f"blocking workflow {wf.name!r} ({lp.name})", result), ) return True return False @@ -1618,6 +1619,10 @@ def _dev_phase(self, task: StoryTask, resume_result: SessionResult | None = None # session-transport classification (#194); decide_dev PAUSEs on # the latter, so the fall-through below preserves the worktree. env_fault=bool((outcome is not None and outcome.env_fault) or result.env_fault), + # The all-roles greppable record rides session-end via + # `_session_end_extras` (#489); here the flag pairs the + # diagnosis with the decision it fed. + session_vanished=result.session_vanished, ) if decision.action == Action.PROCEED: # DEV_VERIFY + spec_file is not itself proof of acceptance: this @@ -3871,6 +3876,15 @@ def _session_end_extras(self, result: SessionResult) -> dict: extras["env_fault"] = True if result.env_fault_evidence: extras["env_fault_evidence"] = result.env_fault_evidence + # lost-session diagnosis (#489): rides the same chokepoint so EVERY + # role — dev, review, fix, migration, triage, injected workflows — + # leaves the greppable record, not only the dev decision. The boolean + # inherits the probe's weak False (`TerminalMultiplexer.has_session`): + # a lookup the backend failed for any reason counts as vanished, + # accepted because the window-death verdict proved the transport + # healthy moments before the probe asked. + if result.session_vanished: + extras["session_vanished"] = True return extras @staticmethod diff --git a/src/bmad_loop/escalation.py b/src/bmad_loop/escalation.py index a1a9afdd..5baaeb9b 100644 --- a/src/bmad_loop/escalation.py +++ b/src/bmad_loop/escalation.py @@ -77,8 +77,43 @@ def env_fault_pause_reason(role: str, result: SessionResult) -> str: """The uniform pause/escalation reason for a transport-failed session (#194). `role` is the descriptor placed before "session" — e.g. "dev", "review", "fix", "migration", "triage", or a richer "blocking workflow 'x' (y)". Keeps - the wording identical across escalation/engine/sweep (see env_fault_detail).""" - return f"environment fault: {role} session {result.status} ({env_fault_detail(result)})" + the wording identical across escalation/engine/sweep (see env_fault_detail). + + Composed over `session_failure_reason` so the two diagnoses cannot cancel + each other out (#489): `crashed` is in `ENV_FAULT_STATUSES`, and both deciders + test `env_fault` FIRST, so a session destroyed under the run whose pane-log + tail also matches a transport pattern would otherwise pause blaming only the + provider. Both facts hold and the operator needs both — a lost session is not + evidence about the API, and a log pattern is not evidence the session + survived.""" + return f"environment fault: {session_failure_reason(role, result)} ({env_fault_detail(result)})" + + +def session_failure_reason(role: str, result: SessionResult) -> str: + """The reason text for a non-completed session: `` session ``, + plus the lost-session diagnosis (#489). + + Without the suffix a session destroyed under the run reads exactly like a CLI + that ran and produced nothing, and the operator debugs the agent instead of + the host. Routing is unchanged either way — the verdict was already correct, + only its explanation was missing. + + Only a ``crashed`` verdict can ever carry the suffix (``session_vanished`` is + stamped nowhere else), so on the timeout/stall paths the suffix never appears. + + The wording states what the evidence *withdraws*, not what it proves. All the + probe establishes is that a session lookup came back negative — see + ``TerminalMultiplexer.has_session``, whose False is "the backend did not + confirm it", not "the session provably no longer exists". That is enough to + stop an operator reading window death as a CLI exit, and not enough to assert + the session was destroyed.""" + reason = f"{role} session {result.status}" + if result.session_vanished: + return ( + f"{reason}: the multiplexer no longer reports the session, so the window's " + "disappearance is not evidence the CLI exited" + ) + return reason def decide_dev( @@ -107,7 +142,7 @@ def decide_dev( Action.PAUSE, env_fault_pause_reason("dev", result), ) - reason = f"dev session {result.status}" + reason = session_failure_reason("dev", result) if budget_left: return Decision(Action.RETRY, reason) return Decision(exhausted, _exhaust_reason(task, reason)) @@ -137,7 +172,7 @@ def decide_review_session(task: StoryTask, result: SessionResult, policy: Policy Action.PAUSE, env_fault_pause_reason("review", result), ) - reason = f"review session {result.status}" + reason = session_failure_reason("review", result) if result.status in REVIEW_TIMEOUT_STATUSES: mode = policy.review.on_timeout if mode == "defer": diff --git a/src/bmad_loop/sweep.py b/src/bmad_loop/sweep.py index 922f8234..0fd04f66 100644 --- a/src/bmad_loop/sweep.py +++ b/src/bmad_loop/sweep.py @@ -19,7 +19,7 @@ from . import deferredwork, gates, verify from .engine import Engine -from .escalation import critical_escalations, env_fault_pause_reason +from .escalation import critical_escalations, env_fault_pause_reason, session_failure_reason from .model import Phase, StoryTask from .platform_util import atomic_replace from .statemachine import advance @@ -749,7 +749,7 @@ def _ensure_migration(self, text: str) -> None: self._escalate(task, f"CRITICAL escalation from migration session: {details}") new_text = ledger.read_text(encoding="utf-8") if ledger.is_file() else "" if result.status != "completed": - errors = [f"migration session {result.status}"] + errors = [session_failure_reason("migration", result)] else: errors = validate_migration(result.result_json, manifest, pre_canonical, new_text) self.journal.append( @@ -863,7 +863,7 @@ def _ensure_triage(self, open_now: set[str], cycle: int = 1) -> TriagePlan: details = "; ".join(str(e.get("detail", e.get("type", "?"))) for e in crits) self._escalate(task, f"CRITICAL escalation from triage session: {details}") if result.status != "completed": - plan, errors = None, [f"triage session {result.status}"] + plan, errors = None, [session_failure_reason("triage", result)] else: plan, errors = validate_triage(result.result_json, open_now) self.journal.append( diff --git a/tests/test_engine.py b/tests/test_engine.py index 790a0d43..775aa54c 100644 --- a/tests/test_engine.py +++ b/tests/test_engine.py @@ -6773,6 +6773,41 @@ def env_breaking_fix(spec): # budget. Guard pins confirm plain (non-classified) failures still retry/defer. +def test_lost_session_is_journaled_structurally_on_dev_decision(project): + """#489: the diagnosis has to be greppable, not only readable. The flag + rides every role's `session-end` entry via `_session_end_extras` (so "how + often did this host destroy my sessions" is a query over the journal rather + than a reading exercise over reason strings) and `dev-decision` pairs it + with the routing it fed. Routing stays the ordinary retry.""" + write_sprint(project, {"1-1-a": "ready-for-dev"}) + engine, adapter = make_engine( + project, + [ + SessionResult(status="crashed", session_vanished=True), + SessionResult(status="crashed", session_vanished=True), + ], + ) + engine.run() + + decisions = [e for e in engine.journal.entries() if e["kind"] == "dev-decision"] + assert decisions and all(d["session_vanished"] is True for d in decisions) + assert decisions[0]["action"] == "retry" # diagnosis, not routing + assert "multiplexer no longer reports the session" in decisions[0]["reason"] + ends = [e for e in engine.journal.entries() if e["kind"] == "session-end"] + assert ends and all(e["session_vanished"] is True for e in ends) + # guard pin: an ordinary crash records the field as False rather than omitting + # it on the decision, and omits it on session-end (the env_fault convention + # there). The journal is per-project and this second run appends to the same + # file, so only its own last entries may be inspected. + engine2, _ = make_engine(project, [SessionResult(status="crashed")]) + engine2.run() + plain = [e for e in engine2.journal.entries() if e["kind"] == "dev-decision"][-1] + assert plain["session_vanished"] is False + assert "multiplexer" not in plain["reason"] + plain_end = [e for e in engine2.journal.entries() if e["kind"] == "session-end"][-1] + assert "session_vanished" not in plain_end + + def test_session_env_fault_pauses_dev_without_burning_budget(project): """A dev session classified an environment fault (#194) pauses the run at the first story rather than charging the attempt; the decision + session-end carry diff --git a/tests/test_escalation.py b/tests/test_escalation.py index 2c6ee8d5..bb62cc7b 100644 --- a/tests/test_escalation.py +++ b/tests/test_escalation.py @@ -95,6 +95,48 @@ def test_dev_plain_noncompleted_still_retries_with_budget(): assert "environment fault" not in decision.reason +def test_vanished_session_says_so_without_changing_the_routing(): + """#489: a session the multiplexer destroyed and a CLI that exited both land + `crashed`. The routing is the same (a retry re-creates the session), but the + reason must not read as "the agent ran and produced nothing".""" + task = _task(attempt=1) + vanished = SessionResult(status="crashed", session_vanished=True) + decision = decide_dev(task, vanished, None, POLICY) + assert decision.action == Action.RETRY # unchanged: diagnosis, not routing + assert "multiplexer no longer reports the session" in decision.reason + # ablation pin: the same verdict WITHOUT the flag stays bare, or the suffix + # would be decoration rather than a discriminator. Pinned on a word the suffix + # actually contains — an assertion keyed to wording the text no longer uses + # passes for the wrong reason and stops guarding anything. + plain = decide_dev(task, SessionResult(status="crashed"), None, POLICY) + assert plain.reason == "dev session crashed" + # and the review side reads the same signal + review = decide_review_session(task, vanished, POLICY) + assert "multiplexer no longer reports the session" in review.reason + + +def test_env_fault_and_lost_session_compose_instead_of_cancelling(): + """#489 x #194: `crashed` is in ENV_FAULT_STATUSES and both deciders test + env_fault FIRST, so a session destroyed under the run whose log tail also + matches a transport pattern would otherwise pause blaming only the provider. + Both facts hold; the operator needs both.""" + task = _task(attempt=1) + both = SessionResult( + status="crashed", + env_fault=True, + env_fault_evidence="API Error: Connection refused", + session_vanished=True, + ) + decision = decide_dev(task, both, None, POLICY) + assert decision.action == Action.PAUSE # env-fault routing is untouched + assert "environment fault: dev session crashed" in decision.reason + assert "multiplexer no longer reports the session" in decision.reason + assert "API Error: Connection refused" in decision.reason + # ablation pin: an env fault WITHOUT a lost session stays exactly as before + plain = SessionResult(status="crashed", env_fault=True) + assert "multiplexer" not in decide_dev(task, plain, None, POLICY).reason + + def test_review_env_fault_session_pauses(): """The same classification pauses a review session (evidence in the reason), where a plain non-completed review would RETRY/DEFER.""" diff --git a/tests/test_generic_tmux.py b/tests/test_generic_tmux.py index 14acdeb7..703948e3 100644 --- a/tests/test_generic_tmux.py +++ b/tests/test_generic_tmux.py @@ -478,7 +478,31 @@ def test_kill_grace_zero_is_the_legacy_single_strike(tmp_path): # Stop event, via devcontract. These exercise that override in isolation. -def make_dev_adapter(tmp_path, profile_name="claude", policy=None): +class _UnitMux: + """Mux stand-in for the unit tests: the session is alive, nothing else exists. + + These tests stub `_window_alive`, so before #489 they never touched `self.mux` + at all and the module docstring's "unit tests need no tmux" held by accident. + The crash path now asks `has_session` for its diagnosis, which without this + would reach the HOST multiplexer — a real subprocess (~130ms) answering False + for a tmp_path session that never existed, scoring every crash test + `session_vanished` and writing a breadcrumb. Answering True keeps each test on + the side of the distinction it was written for: the CLI exited. + """ + + def __init__(self): + self.sent: list[tuple[str, str]] = [] + + def has_session(self, name): + return True + + def send_text(self, window_id, text): + # The contract/stall nudges reach the mux too; recording them keeps that + # off the host binary as well, which is the same promise as has_session. + self.sent.append((window_id, text)) + + +def make_dev_adapter(tmp_path, profile_name="claude", policy=None, mux=None): impl = tmp_path / "impl" impl.mkdir() # project root == tmp_path so rebased(spec.cwd=tmp_path) is a no-op: these @@ -493,6 +517,7 @@ def make_dev_adapter(tmp_path, profile_name="claude", policy=None): policy=policy or Policy(limits=LimitsPolicy()), profile=get_profile(profile_name), paths=paths, + mux=mux or _UnitMux(), ) return adapter, impl @@ -2157,6 +2182,11 @@ def test_post_kill_reconcile_leaves_other_statuses_alone(tmp_path): assert ( adapter._post_kill_reconcile(_dev_handle(), _dev_spec(tmp_path), original) is original ) + # pins the base.py cross-claim that `session_vanished` rides only verdicts + # this hook never rebuilds: if `crashed` ever joins the rescue set, this + # rescuable artifact would produce a rebuilt result without the flag. + vanished = _unvouched("crashed", session_vanished=True) + assert adapter._post_kill_reconcile(_dev_handle(), _dev_spec(tmp_path), vanished) is vanished def test_post_kill_reconcile_keeps_stall_when_window_alive_after_kill(tmp_path): @@ -2613,6 +2643,11 @@ def new_window(self, session_name, window_name, cwd, env, cmd): def pipe_pane(self, window_id, log_file): self.piped.append((window_id, Path(log_file))) + def has_session(self, name): + # The crash-path diagnosis probe (#489) asks this; these tests are about a + # window that died under a session that is still very much there. + return True + def test_start_session_resets_reused_task_log(tmp_path): """A re-armed run reuses task_ids and both mux backends APPEND to @@ -2764,6 +2799,93 @@ def test_wait_for_completion_genuine_window_death_still_crashes(tmp_path, monkey assert result.status == "crashed" +class _SessionProbeMux: + """Mux stand-in exposing only what `_session_vanished` asks: has_session.""" + + def __init__(self, answer): + self._answer = answer + self.calls: list[str] = [] + + def has_session(self, name): + self.calls.append(name) + if isinstance(self._answer, Exception): + raise self._answer + return self._answer + + +@pytest.mark.parametrize( + ("has_session", "expect_vanished"), + [ + (False, True), # the session itself is gone: something destroyed it + (True, False), # session alive, window gone: the CLI exited + (MultiplexerError("server wedged"), False), # unknown is not vanished + ], +) +def test_window_death_distinguishes_a_destroyed_session_from_an_exited_cli( + tmp_path, monkeypatch, has_session, expect_vanished +): + """#489: `list_window_ids` answers [] for a dead window AND for a session that + no longer exists, so the crash verdict alone cannot say which happened. The + verdict is `crashed` either way — only the diagnosis differs.""" + monkeypatch.setattr(generic, "RESULT_GRACE_S", 0.0) + monkeypatch.setattr(generic, "RESULT_POLL_S", 0.0) + adapter, _ = make_dev_adapter(tmp_path) + adapter._window_alive = lambda handle: False + adapter.mux = _SessionProbeMux(has_session) + + adapter.watcher = _ScriptedWatcher([]) + result = adapter.wait_for_completion(_dev_handle(), _dev_spec(tmp_path)) + + assert result.status == "crashed" + assert result.session_vanished is expect_vanished + assert adapter.mux.calls == [adapter.session_name] + # The durable half of the diagnosis: CHANGELOG and FEATURES both promise this + # crumb, and without an assertion deleting the write keeps the suite green. + crumbs = _lifecycle_events(adapter, "session-vanished") + assert len(crumbs) == (1 if expect_vanished else 0) + if expect_vanished: + assert crumbs[0]["session"] == adapter.session_name + assert crumbs[0]["status"] == "crashed" + + +def test_session_probe_is_skipped_for_non_crash_verdicts(tmp_path, monkeypatch): + """The probe answers "why did the window die" — a stall/timeout reached under a + LIVE window never asks it, or an unrelated mux outage would be misattributed to + a session the mux never touched.""" + adapter, _ = make_dev_adapter(tmp_path) + adapter.mux = _SessionProbeMux(False) # would say "vanished" if asked + + res = adapter._final(_dev_handle(), _dev_spec(tmp_path), "timeout", None, None) + + assert res.status == "timeout" + assert res.session_vanished is False + assert adapter.mux.calls == [] + + +def test_read_back_upgrade_is_not_diagnosed_even_if_the_session_is_gone(tmp_path): + """The deliberately-dropped case: a session reaped AFTER flushing its result + still earns `completed`, and a completed session gets no vanished diagnosis — + it produced something. Pinned separately because the skip test above only + exercises a `timeout` fallback, so this branch could invert unnoticed.""" + adapter = make_adapter(tmp_path) + adapter.mux = _SessionProbeMux(False) # the session really is gone + task_dir = adapter.tasks_dir / "1-1-a-dev-1" + task_dir.mkdir(parents=True) + (task_dir / "result.json").write_text('{"status": "done", "workflow": "dev"}') + + res = adapter._final( + SessionHandle(task_id="1-1-a-dev-1", native_id="@1"), + make_spec(tmp_path), + "crashed", + None, + None, + ) + + assert res.status == "completed" + assert res.session_vanished is False + assert adapter.mux.calls == [] # never even asked + + def _usage_adapter(tmp_path, profile_name, **kw) -> GenericTmuxAdapter: return GenericTmuxAdapter( run_dir=tmp_path / "run", @@ -4302,8 +4424,11 @@ def test_proof_of_work_leaves_task_scoped_result_json_alone(tmp_path): large (#298 review). A base adapter's `tasks//result.json` is unique to this task and unlinked at launch, so its presence already proves THIS session wrote it — no foreign writer can reach it. Gating it could only ever discard an - authoritative completion, so `_ResultFileMixin` declines the gate outright.""" - adapter = make_adapter(tmp_path) + authoritative completion, so `_ResultFileMixin` declines the gate outright. + `_UnitMux` keeps the crash-verdict `_final` call off the host multiplexer — + the read-back upgrade skips the probe today, but that must not be what this + test leans on.""" + adapter = make_adapter(tmp_path, mux=_UnitMux()) task_dir = adapter.tasks_dir / "1-1-a-dev-1" task_dir.mkdir(parents=True) (task_dir / "result.json").write_text('{"status": "done", "workflow": "dev"}') @@ -4316,8 +4441,8 @@ def test_proof_of_work_leaves_task_scoped_result_json_alone(tmp_path): def test_proof_of_work_gates_post_kill_reconcile(tmp_path, monkeypatch): - """The i-11 call path: the rescue is for a session that finished and lost its - Stop, not for one that never ran.""" + """The post-kill call path: the rescue is for a session that finished and + lost its Stop, not for one that never ran.""" adapter, impl = make_dev_adapter(tmp_path) monkeypatch.setattr(generic, "RESULT_GRACE_S", 0.0) adapter._window_alive = lambda handle: False diff --git a/tests/test_plugin_workflows.py b/tests/test_plugin_workflows.py index 499c1f8c..cf0c3478 100644 --- a/tests/test_plugin_workflows.py +++ b/tests/test_plugin_workflows.py @@ -294,6 +294,26 @@ def test_blocking_workflow_env_fault_escalates_instead_of_deferring(project): assert "story-deferred" not in kinds # escalated, not deferred +def test_blocking_workflow_lost_session_says_so_in_the_defer_reason(project): + """#489 on the one path that DEFERS rather than retries: a blocking workflow + whose mux session was destroyed must not be filed as "the workflow ran and + failed". The defer reason an operator reads carries both which workflow died + and that the session went missing under it.""" + setup_story(project) + reg = PluginRegistry([LoadedPlugin(manifest=wf_manifest("wf", blocking=True))]) + script = [ + dev_effect(project, "1-1-a"), + SessionResult(status="crashed", session_vanished=True), + ] + engine, _ = make_engine(project, script, reg) + summary = engine.run() + + assert summary.deferred == 1 + deferred = [e for e in engine.journal.entries() if e["kind"] == "story-deferred"][-1] + assert "blocking workflow 'doc' (wf)" in deferred["reason"] + assert "multiplexer no longer reports the session" in deferred["reason"] + + def test_nonblocking_workflow_failure_is_advisory(project): captured: list = [] setup_story(project) diff --git a/tests/test_sweep.py b/tests/test_sweep.py index f3020cff..a04e2897 100644 --- a/tests/test_sweep.py +++ b/tests/test_sweep.py @@ -1594,6 +1594,42 @@ def test_triage_plain_timeout_still_retries_to_cap(project): assert all(d["env_fault"] is False for d in dec) +def test_triage_lost_session_names_the_mux_in_its_errors(project): + """#489 on the sweep triage path: a destroyed session must not be filed as a + triage that ran and failed. The adopted reason builder is only verified on the + dev/review paths otherwise, so reverting this site would go unnoticed.""" + write_ledger(project, {"DW-1": "open"}) + engine, adapter = make_sweep( + project, + [ + SessionResult(status="crashed", session_vanished=True), + SessionResult(status="crashed", session_vanished=True), + ], + ) + engine.run() + + dec = [e for e in engine.journal.entries() if e["kind"] == "triage-decision"][-1] + assert any("triage session crashed" in e for e in dec["errors"]) + assert any("multiplexer no longer reports the session" in e for e in dec["errors"]) + + +def test_migration_lost_session_names_the_mux_in_its_errors(project): + """The migration twin of the triage case above — same builder, same blind spot.""" + write_legacy_ledger(project, LEGACY_LEDGER) + engine, adapter = make_sweep( + project, + [ + SessionResult(status="crashed", session_vanished=True), + SessionResult(status="crashed", session_vanished=True), + ], + ) + engine.run() + + dec = [e for e in engine.journal.entries() if e["kind"] == "migrate-decision"][-1] + assert any("migration session crashed" in e for e in dec["errors"]) + assert any("multiplexer no longer reports the session" in e for e in dec["errors"]) + + def test_migration_session_env_fault_escalates_without_consuming_attempts(project): """A migration session whose CLI lost its API connection (#194) escalates on the first attempt instead of charging a migration retry; migrate-decision carries