fix(#194): pause on coding-CLI transport failures instead of burning story/sweep attempts by pbean · Pull Request #272 · bmad-code-org/bmad-loop

pbean · 2026-07-23T03:54:21Z

Closes #194

Transport failures now pause for a human instead of burning story/sweep attempts. Single branch, all three parts of the plan.

Root cause

A dev session whose coding CLI loses its API connection stays alive but idle, printing API Error: Unable to connect … to the pane while it rides out the session clock. The engine then stamps it status="timeout", indistinguishable from a genuine wall-clock timeout — so each outage charges a dev attempt, and two of them exhaust max_dev_attempts and defer the story with zero real work attempted. Nothing read the pane content: it's tee'd into logs/<task_id>.log but only its (mtime, size) stat was consumed.

Approach

Classify post-mortem, off the log we already tee, and route a classified session down the exact path verify environment faults (rc 126/127) already take: PAUSE (escalate), and re-arm resets the budget. A transport failure costs a human glance with preserved work and evidence, instead of a silent budget burn.

Part 1 — adapter classification plumbing

SessionResult gains env_fault: bool / env_fault_evidence: str | None, appended last so positional constructions stay valid. New base-class identity hook _classify_env_fault, chained in run() after _post_kill_reconcile — a reconcile upgrade to completed is never re-classified; HTTP/mock adapters stay inert.
CLIProfile.env_fault_patterns (Python re), re.compile-validated at parse time (invalid → ProfileError). Seeded only in claude.toml; the pattern requires an API Error line and a connection-level cause on the same line, so ordinary story test output mentioning "API Error" in prose does not trip it. Override/disable via .bmad-loop/profiles/ — no new Policy field, no core.toml ripple.
GenericAdapter._classify_env_fault: for a timeout/stalled/crashed verdict with result_json is None, read the last 64 KiB of the pane log, strip ANSI (self-contained regex — no pyte/TUI machinery), match line-by-line (last match wins, evidence truncated to 240 chars), stamp the result and drop an env-fault-classified breadcrumb. Any OSError → no classification. over_budget excluded (a budget crossing proves real API traffic); completed never reaches the scan.

Part 2 — story-pipeline consumption (dev / review / fix)

escalation.decide_dev / decide_review_session PAUSE first thing in the non-completed block (the CRITICAL crits check still runs first; env-fault results carry result_json=None, so no conflict). Shared env_fault_detail keeps the reason wording uniform: environment fault: <role> session <status> (<evidence>).
engine.py: dev-decision / fix-decision journal env_fault now ORs the session flag with the verify-path outcome flag; _session_end_extras stamps env_fault / env_fault_evidence on both session-end sites; _fix_phase escalates a non-completed env-fault fix session (worktree preserved via the existing PAUSE fall-through).
Re-arm restores the budget: runs.rearm_escalation resets task.attempt = 0 for the spec-less env-fault escalation exactly as it does for any escalation.

Part 3 — workflows + sweep

engine._run_workflows: a blocking workflow's env-fault session escalates instead of deferring the story; workflow-end carries the flag. Non-blocking workflows keep continuing (journaled only) — the next story session classifies and pauses if the outage persists.
sweep.py migration + triage loops escalate a non-completed env-fault session after the decision journal append, before any retry/attempt-cap charge; both journal events gain env_fault. Budget restoration is the sweep's existing ESCALATED-resume task.attempt = 0 (unchanged — documented, not modified).

Tests

test_profile.py — claude ships patterns / others empty; overlay parse; invalid regex → profile error; default-empty.
test_generic_tmux.py — ANSI-colored timeout classified (flag + stripped evidence + breadcrumb); stalled/crashed too; completed + over_budget + result-json-present never; empty-pattern inert; prose "API Error" false-positive control; missing log degrades; last-match-wins + truncation; run() ordering (non-rescued stamped, reconcile→completed not re-classified).
test_escalation.py — dev/review PAUSE with evidence + budget-exhausted still pauses; plain-timeout retry guards.
test_engine.py — dev pause + rearm_escalation → attempt == 0; two-plain-timeouts-still-defer guard; fix-phase escalate.
test_plugin_workflows.py — blocking-workflow env-fault escalates (not defers), workflow-end carries evidence.
test_sweep.py — migration + triage escalate without consuming attempts (triage: resume restores budget), plain-timeout-retries-to-cap guard.

Verification

.venv/bin/python -m pytest — full suite green except 2 pre-existing test_module_skills_sync failures (bmad-loop-setup assets/module.yaml drift), confirmed identical with this branch's changes stashed. Full trunk check clean.

Known accepted edge cases

Error-then-recovery followed by a genuine timeout can still match in the tail → a pause instead of a retry; the failure mode is a human look with evidence + preserved work, strictly safer than a silent budget burn.
A persistent outage pauses again immediately on each resolve/re-arm (correct per the issue; notify fires each time).
SessionResult field-append discipline: the two new fields must stay LAST.

Summary by CodeRabbit

New Features
- Transport and API failures are now detected from session logs and recorded with supporting evidence.
- Environment faults pause development and review sessions without consuming retry attempts.
- Re-arming after an outage restores the available attempt budget.
- Blocking workflows and sweep operations now escalate environment faults immediately instead of deferring work.
Documentation
- Added guidance for configuring environment-fault detection patterns and understanding related session diagnostics.

…3 of #194) A coding CLI that loses its API connection idles out the session clock and is stamped `timeout` (or `stalled`/`crashed`), indistinguishable from a real wall-clock timeout — charging a dev attempt for a session that never reached the API. This adds the plumbing to classify those post-mortem, without yet consuming the signal. - SessionResult gains `env_fault` / `env_fault_evidence` (appended LAST so positional constructions stay valid) plus a base-class identity `_classify_env_fault` template hook, chained in `run()` AFTER `_post_kill_reconcile` so a rescue-to-completed is never re-classified. - CLIProfile gains `env_fault_patterns` (Python `re`), compiled+validated at parse time; seeded only in claude.toml (API Error + connection cause on the same line, so prose "API Error" test output does not trip it). - GenericAdapter overrides the hook: for a timeout/stalled/crashed verdict with result_json None, it reads the last 64 KiB of the tee'd pane log, strips ANSI, matches line-by-line (last match wins, evidence truncated to 240 chars), and stamps the result + an `env-fault-classified` breadcrumb. Any OSError -> no classification (best-effort doctrine). Downstream consumption (escalation/engine/sweep pause-not-charge) lands in parts 2-3. refs #194 — do not close.

coderabbitai · 2026-07-23T03:54:51Z

Warning

Review limit reached

@pbean, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 42 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 1221f6b0-f6c9-4acb-b501-33fa5ea01e1d

📥 Commits

Reviewing files that changed from the base of the PR and between b7b5bd3 and c66428c.

📒 Files selected for processing (3)

src/bmad_loop/engine.py
src/bmad_loop/escalation.py
src/bmad_loop/sweep.py

Walkthrough

This change adds profile-configured, regex-based post-mortem transport-fault classification. Eligible non-completed sessions are checked after reconciliation using ANSI-stripped pane-log tails, and matching results receive environment-fault fields and lifecycle evidence. Workflow and sweep handling now pauses or escalates without consuming retry budgets.

Changes

Environment-fault classification

Layer / File(s)	Summary
Profile pattern contract `src/bmad_loop/adapters/profile.py`, `src/bmad_loop/data/profiles/claude.toml`, `tests/test_profile.py`	Profiles parse and validate `env_fault_patterns`; Claude provides a seed pattern, while defaults and invalid-pattern handling are tested.
Adapter result fields and hook ordering `src/bmad_loop/adapters/base.py`, `docs/adapter-authoring-guide.md`	`SessionResult` gains environment-fault fields, and `run()` invokes classification after post-kill reconciliation.
Generic tmux classification and coverage `src/bmad_loop/adapters/generic.py`, `tests/test_generic_tmux.py`	The adapter scans eligible pane-log tails, strips ANSI controls, applies bounded regex matching, selects the last matching line, records evidence, and resets reused logs.
Workflow decisions and journaling `src/bmad_loop/engine.py`, `src/bmad_loop/escalation.py`, `tests/test_engine.py`, `tests/test_escalation.py`, `tests/test_plugin_workflows.py`	Environment faults pause dev/review sessions, escalate blocking workflows and fix sessions, preserve evidence in journals, and avoid normal budget consumption.
Sweep handling `src/bmad_loop/sweep.py`, `tests/test_sweep.py`	Migration and triage environment faults escalate immediately while preserving retry budgets and ledger state.
Documentation and dependency updates `CHANGELOG.md`, `docs/FEATURES.md`, `docs/adapter-authoring-guide.md`, `pyproject.toml`	Documentation and release notes describe the new classification flow, state breadcrumbs, and timed `regex` matching dependency.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Session
  participant CodingCLIAdapter
  participant GenericAdapter
  participant PaneLog
  participant Engine
  participant Escalation
  Session->>CodingCLIAdapter: run(spec)
  CodingCLIAdapter->>GenericAdapter: reconcile session result
  GenericAdapter->>PaneLog: inspect bounded ANSI-stripped tail
  PaneLog-->>GenericAdapter: matching transport evidence
  GenericAdapter-->>CodingCLIAdapter: classified SessionResult
  CodingCLIAdapter-->>Engine: return result
  Engine->>Escalation: decide workflow action
  Escalation-->>Engine: pause or escalation with evidence

Possibly related PRs

bmad-code-org/bmad-loop#95: Introduces the post-kill adapter hook seam used before classification.
bmad-code-org/bmad-loop#130: Extends environment-fault handling through engine and escalation decisions.
bmad-code-org/bmad-loop#164: Modifies generic adapter post-session reconciliation adjacent to classification.

Suggested reviewers: dracic

Poem

I’m a rabbit guarding the log,
Finding faults through terminal fog.
ANSI fades and clues grow bright,
Budgets rest through outage night.
Reconcile first, then flags take flight—
Evidence tucked away just right.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 58.97% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly describes the main change: pausing on transport failures instead of consuming story/sweep attempts.
Linked Issues check	✅ Passed	The PR implements the env-fault classification and pause/escalation flow required by `#194` across adapters, engine, workflows, and sweep logic.
Out of Scope Changes check	✅ Passed	The changes are tightly scoped to transport-failure classification, related configuration, docs, and tests, with no clear unrelated additions.

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch feat/env-fault-classify-194

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

coderabbitai

Actionable comments posted: 3

🤖 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/adapters/generic.py`:
- Around line 759-763: Update start_session to delete or rotate the existing
task log before launching and piping a new pane when a task ID is reused,
ensuring the subsequent 64 KiB tail contains only the current session’s output.
Add a regression test covering task-ID reuse and confirming stale matching
errors do not classify a later timeout as an environment fault.
- Around line 768-769: Update the regex matching in the post-mortem
classification loop around self._env_fault_patterns to prevent custom patterns
from stalling run() during teardown. Restrict profile-provided regex syntax to a
safe grammar or use a linear-time/timeout-bounded matcher, while preserving
matching for supported environment fault patterns.

In `@src/bmad_loop/adapters/profile.py`:
- Around line 177-182: Update the env_fault_patterns validation around
env_fault_patterns and the profile-loading function to require a TOML array
whose every item is a string; reject scalar values, non-array iterables, and
non-string elements with ProfileError before regex compilation. Preserve the
existing invalid-regex ProfileError behavior for valid string entries, and add
tests covering scalar and non-string-item inputs.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: eddc704b-653e-414f-9544-1a735b308f14

📥 Commits

Reviewing files that changed from the base of the PR and between 946d7f9 and 55fceef.

📒 Files selected for processing (8)

CHANGELOG.md
docs/adapter-authoring-guide.md
src/bmad_loop/adapters/base.py
src/bmad_loop/adapters/generic.py
src/bmad_loop/adapters/profile.py
src/bmad_loop/data/profiles/claude.toml
tests/test_generic_tmux.py
tests/test_profile.py

…ning attempts (parts 2-3 of #194) Consumes the transport-failure classification from part 1 across the whole session pipeline: a session whose coding CLI lost its API connection (result env_fault=True) now PAUSEs for a human — re-arm restores the budget — instead of charging the attempt/cycle and deferring the story as if its code broke. This mirrors how verify environment faults (rc 126/127) already behave. escalation.py: - shared `env_fault_detail(result)` so the pause reason wording stays uniform. - decide_dev / decide_review_session: PAUSE first thing in the non-completed block (CRITICAL crits check still runs first — env-fault results carry result_json=None, so no conflict). engine.py: - dev-decision / fix-decision journal env_fault now ORs the session flag with the verify-path outcome flag. - _session_end_extras stamps env_fault/env_fault_evidence on both session-end emit sites. - _fix_phase escalates a non-completed env-fault fix session (worktree preserved via the existing PAUSE fall-through). - _run_workflows: a blocking workflow's env-fault session escalates instead of deferring the story; workflow-end carries the flag. Non-blocking workflows keep continuing (journaled only). sweep.py: - migration + triage loops escalate a non-completed env-fault session after the decision journal append, before any retry/attempt-cap charge; both journal events gain env_fault. Budget restoration is the sweep's existing ESCALATED-resume attempt reset (unchanged, documented). Tests: escalation dev/review PAUSE + guard pins; engine dev pause + rearm restores attempt, fix-phase escalate, two-plain-timeouts-still-defer guard; blocking-workflow escalate; sweep migration + triage escalate-without-consuming -attempts + plain-timeout guard. Docs: FEATURES.md typed-escalations bullet + env-fault-classified breadcrumb; CHANGELOG consolidated to the full feature. Closes #194.

Address three CodeRabbit findings on the transport-failure classifier: - Reset the reused pane log in start_session. Task-ids are reused across a re-armed run and both mux backends append, so a prior cycle's transport error lingered in the 64 KiB tail and mis-flagged a later unrelated timeout as env_fault. Unlink logs/<task_id>.log before re-piping, mirroring the existing result.json unlink (and matching journal.py's "next session replaces it" assumption). - Bound operator-supplied pattern matching. Match env_fault_patterns with the `regex` module under a per-pattern timeout (ENV_FAULT_MATCH_TIMEOUT_S=2.0s) so a pathological profile regex can't catastrophically backtrack and hang a session teardown; a TimeoutError declines to classify (best-effort, like an unreadable log). Adds `regex` as a core dependency; validation compiles with the same engine. - Validate list-valued profile fields. env_fault_patterns / seed_files / launch_args / bypass_args must be a TOML array of strings: a bare string silently became per-character entries and a scalar leaked a raw TypeError. Both now raise a friendly ProfileError via a shared str_list() guard. Tests: task-id reuse regression, pathological-pattern timeout bound, and scalar/non-string-item rejection across all four list fields. refs #194

coderabbitai

🧹 Nitpick comments (1)

src/bmad_loop/escalation.py (1)
80-89: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Six sites hand-duplicate the same env-fault pause/escalation reason template. Each builds f"environment fault: {role} session {status} ({detail})" inline instead of sharing one formatter next to env_fault_detail.

src/bmad_loop/escalation.py#L80-L89: extract the template (e.g. env_fault_pause_reason("dev", result)) here, next to env_fault_detail.

src/bmad_loop/escalation.py#L114-L120: reuse the same helper for "review".

src/bmad_loop/engine.py#L1049-L1053: reuse the helper for the blocking-workflow escalation message.

src/bmad_loop/engine.py#L2247-L2250: reuse the helper for the fix-session escalation message.

src/bmad_loop/sweep.py#L766-L770: reuse the helper for the migration-session escalation message.

src/bmad_loop/sweep.py#L879-L883: reuse the helper for the triage-session escalation message.
🤖 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/escalation.py` around lines 80 - 89, Extract a shared
env_fault_pause_reason helper next to env_fault_detail in
src/bmad_loop/escalation.py, preserving the existing “environment fault: {role}
session {status} ({detail})” output. Replace the inline templates at
src/bmad_loop/escalation.py:80-89 and 114-120, src/bmad_loop/engine.py:1049-1053
and 2247-2250, and src/bmad_loop/sweep.py:766-770 and 879-883 with calls to this
helper for their respective roles.

🤖 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.

Nitpick comments:
In `@src/bmad_loop/escalation.py`:
- Around line 80-89: Extract a shared env_fault_pause_reason helper next to
env_fault_detail in src/bmad_loop/escalation.py, preserving the existing
“environment fault: {role} session {status} ({detail})” output. Replace the
inline templates at src/bmad_loop/escalation.py:80-89 and 114-120,
src/bmad_loop/engine.py:1049-1053 and 2247-2250, and
src/bmad_loop/sweep.py:766-770 and 879-883 with calls to this helper for their
respective roles.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 1e735a98-b8fe-4320-8ad6-c7ac9bc564d9

📥 Commits

Reviewing files that changed from the base of the PR and between 55fceef and b7b5bd3.

⛔ Files ignored due to path filters (1)

uv.lock is excluded by !**/*.lock

📒 Files selected for processing (15)

CHANGELOG.md
docs/FEATURES.md
docs/adapter-authoring-guide.md
pyproject.toml
src/bmad_loop/adapters/generic.py
src/bmad_loop/adapters/profile.py
src/bmad_loop/engine.py
src/bmad_loop/escalation.py
src/bmad_loop/sweep.py
tests/test_engine.py
tests/test_escalation.py
tests/test_generic_tmux.py
tests/test_plugin_workflows.py
tests/test_profile.py
tests/test_sweep.py

🚧 Files skipped from review as they are similar to previous changes (5)

CHANGELOG.md
tests/test_profile.py
docs/adapter-authoring-guide.md
src/bmad_loop/adapters/profile.py
src/bmad_loop/adapters/generic.py

CodeRabbit nitpick (PR #272): the pause/escalation reason template 'environment fault: {role} session {status} ({detail})' was hand-duplicated at six sites across escalation/engine/sweep. Extract env_fault_pause_reason() next to env_fault_detail and route all six through it; output strings stay byte-identical (existing assertions unchanged). engine/sweep now import the formatter instead of env_fault_detail (their only prior caller).

pbean · 2026-07-23T05:32:29Z

Addressed the remaining nitpick (🧹 Nitpick comments (1) in the latest review) in c66428c.

Extracted a shared env_fault_pause_reason(role, result) next to env_fault_detail in escalation.py and routed all six sites through it — escalation.py (dev/review), engine.py (fix + blocking-workflow, the latter passing a richer "blocking workflow 'x' (y)" role string), and sweep.py (migration/triage). Output strings stay byte-identical, so the existing environment fault: … assertions across test_escalation.py/test_engine.py/test_sweep.py/test_plugin_workflows.py (279 passing) are unchanged. engine.py/sweep.py now import the formatter instead of env_fault_detail (their only prior caller). The wording contract already documented in env_fault_detail's docstring is now enforced in one place.

coderabbitai Bot reviewed Jul 23, 2026

View reviewed changes

Comment thread src/bmad_loop/adapters/generic.py

Comment thread src/bmad_loop/adapters/generic.py Outdated

Comment thread src/bmad_loop/adapters/profile.py Outdated

pbean changed the title ~~feat(adapters): post-mortem transport-failure classification (part 1/3 of #194)~~ fix(#194): pause on coding-CLI transport failures instead of burning story/sweep attempts Jul 23, 2026

pbean marked this pull request as draft July 23, 2026 04:38

pbean marked this pull request as ready for review July 23, 2026 04:39

coderabbitai Bot reviewed Jul 23, 2026

View reviewed changes

pbean merged commit 216c0b8 into main Jul 23, 2026
10 checks passed

pbean deleted the feat/env-fault-classify-194 branch July 24, 2026 01:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(#194): pause on coding-CLI transport failures instead of burning story/sweep attempts#272

fix(#194): pause on coding-CLI transport failures instead of burning story/sweep attempts#272
pbean merged 4 commits into
mainfrom
feat/env-fault-classify-194

pbean commented Jul 23, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Jul 23, 2026 •

edited

Loading

Review limit reached

❌ Failed checks (1 warning)

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Uh oh!

pbean commented Jul 23, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

pbean commented Jul 23, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Closes #194

Root cause

Approach

Part 1 — adapter classification plumbing

Part 2 — story-pipeline consumption (dev / review / fix)

Part 3 — workflows + sweep

Tests

Verification

Known accepted edge cases

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Jul 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review limit reached

Walkthrough

Changes

Sequence Diagram(s)

❌ Failed checks (1 warning)

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

pbean commented Jul 23, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

pbean commented Jul 23, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Jul 23, 2026 •

edited

Loading