feat(tui): render validate --json in a findings modal (#210) by pbean · Pull Request #215 · bmad-code-org/bmad-loop

pbean · 2026-07-20T18:06:42Z

Closes #210.

Pressing v in the TUI ran bmad-loop validate in text mode and dropped the captured
blob into TextOutputModal, with the verdict inferred from the exit code. That meant severity
was a string prefix, the verdict conflated "the checks failed" with "the command broke", and
every check's detail — the data it knew before flattening itself into a sentence — was
already gone by the time the TUI saw it. This moves the modal onto the schema-v1 document from
#205 and renders it structurally.

Three commits, each self-contained:


`609a567`	Phase 1 — the pure seams (no user-visible change)
`55bdd43`	Phase 2 — the modal and the wiring
this	Phase 3 — promote `mux.external-backend` to `warning`

Phase 1 — seams

launch.run_captured_streams(argv_tail) -> (rc, stdout, stderr). run_captured merges the
child's stderr onto stdout after it, so json.loads on the merged string raises
Extra data: — one DeprecationWarning from any dependency would have silently degraded
the feature to the text modal on some machines and not others. run_captured now delegates
to the new function and merges, so there is still exactly one subprocess.run.
encoding="utf-8", errors="replace" rather than text=True: text=True decodes with the
locale encoding at errors='strict', which is the diagnose --json can raise UnicodeEncodeError on a non-UTF-8 console #200 family of failure the JSON path
already fixed CLI-side.
widgets.validate_document() (parse + version pin + shape gate, returns None rather than
raising), validate_findings() (one Table.grid, so columns align across findings) and
validate_header().
widgets._RENDERS_VALIDATE_SCHEMA = 1 is a hand-written literal, deliberately not an
import of documents.VALIDATE_SCHEMA_VERSION — an import would auto-follow a CLI bump and
silently render a v2 document as v1. A tripwire test asserts the two are equal, since
otherwise a mismatch would first surface as a silent degrade in a user's terminal with CI
green.

Phase 2 — modal and wiring

ValidateFindingsModal + a new _show_validate worker. _show_captured is untouched — the
run --dry-run and sweep --dry-run modals still use it.

Verdict from doc["ok"], never rc.
Detail inline for warning/problem, d toggles it for everything. One severity rule, zero
check-id matching (message is not contracted; check is). Rendering all detail inline
would make a passing modal ~3.6× longer than today's while restating messages already on
screen.
Detail rendering is depth-2 with a json.dumps leaf fallback, not recursive: detail is not
flat — the passing path emits {"gates_mode": str, "adapters": {"dev": ...}} as the second
finding of every successful validate, and a scalar-only renderer prints a Python repr. The
bridge test drives a live validate --json and asserts "{'" not in rendered, so it
cannot rot against a fixture.
When any problem exists the header appends a dim footer noting the gates are chained. That
puts documents.py's "absence is not a pass" on screen, and is the one thing the structural
renderer can teach that the text cannot: a short findings list is not a short list of
problems.
The worker body is wrapped in try/except. @work(thread=True) defaults to
exit_on_error=True (Textual 8.2.7), so an unguarded JSONDecodeError/KeyError in the new
parse-and-render body would take down the whole app. Not exit_on_error=False — that turns a
crash into pressing v and nothing happening.
The degrade re-runs in text mode rather than dumping the captured JSON. If stdout was
non-empty but unrenderable, showing the merged blob means a wall of {"schema_version": 2, …}
in a modal titled validate — exit 1, withholding a perfectly good human rendering at the
exact moment the structural one failed. Re-running costs one sub-second subprocess on a path
that should never fire, and makes the claim trivially reviewable: the degrade is
byte-for-byte the pre-TUI: validate modal should consume validate --json and render findings structurally #210 behavior.

Transport is subprocess + --json, which is a knowing exception to documents.py's "a non-CLI
frontend should import the builders directly and never parse CLI stdout" — the TUI is such a
frontend. The justification is in the worker's docstring: cmd_validate loads third-party mux
entry points and probes httpx, so the subprocess quarantines a broken plugin's import side
effects and leaves the TUI's own lru_cached mux selection undisturbed. Extracting an
in-process build_validation_report() is worth filing as a follow-up.

Phase 3 — `mux.external-backend` → `warning`

The finding has always read as a failure — "external mux backend 'x' failed to load: …" — while
carrying severity ok, so it counted as a passing check. It was pinned there only because
promoting it inserts warning: into shipped text the TUI rendered verbatim; the TUI no longer
does. bmad-loop mux has always printed this same condition as warning:, so validate was the
outlier. It stays below problem deliberately: selection degrades past a broken external (a
failed external can never be the selected backend), so the verdict and rc must not flip. The
comment at the site argued the finding was advisory; it now says that.

Two things worth stating rather than leaving implicit:

1. No schema bump — a decision, not an oversight. On an affected host the promotion flips
counts (warning +1, ok −1) but leaves ok and rc alone. machine.py says changing "the
meaning of a value" bumps, and someone could reasonably read a changed count that way. The
counter-argument is that documents.py contracts the check id as the matchable identity,
not any given check's outcome — a consumer branching on mux.external-backend still finds it,
and one reading counts is reading a tally of severities that was always free to move as
checks changed their minds. Under a bump-on-any-count-change reading, every future severity
adjustment would be a breaking change, which is not what the additive-only rule is protecting.
Happy to bump if you disagree — it is cheap now and expensive later.

2. The CLI text still changes, and gets uglier. That line becomes:

  ok:   warning: external mux backend 'x' failed to load: ...

because render() preserves the double prefix by design (the warning sites stored
" warning: " + msg into the same list the ok: printer walked). #210 unblocks the TUI
half of the freeze, not the CLI half. test_validation_report_renders_each_severity_verbatim
is the byte-exact lock and stays green — it builds its own findings and never touches this
check — and the new test asserts the now-shipping line at the real site, so the consequence is
pinned rather than incidental.

Two UI notes

The dashboard's Footer shows through beneath every modal, so while this one is open the
footer still reads d decisions. That is pre-existing for all modals, not new here; the
in-modal d — toggle detail on every finding hint covers discoverability, and the modal's
binding wins since it holds focus.
Detail sub-rows sit in the same column as folded message continuations, distinguished only
by dim styling. Legible in a real terminal, but if you want a visual marker that is a
Phase-1 renderer change, so I left it alone.

Testing

Full suite green, full trunk check (no filter) clean.

Nothing asserted mux.external-backend's severity before this — test_external_backends.py
only covers external_backend_errors() contents — so Phase 3 adds a test pinning the severity,
the detail, and the rendered text.

The degrade path was verified live during Phase 2 — forcing validate_document to None
reproduces the old modal exactly. The rest is covered by the automated cases: the four degrade
shapes, the d toggle, a raising subprocess (the exit_on_error trap), and both verdicts with
rc deliberately disagreeing with what the old code would have inferred from it. Worth a look at
96 columns by a second pair of eyes before merge.

Docs: docs/tui-guide.md described the replaced behavior verbatim in two places.

Summary by CodeRabbit

New Features
- The TUI Validate view now presents structured, per-check findings from validate --json, including verdict-derived summaries, severity counts, check IDs/messages, and an expandable details toggle.
- Validation mode (v) and its on-screen guidance were refreshed to match the new structured renderer.
Bug Fixes
- If the JSON output can’t be rendered, the TUI automatically falls back to the prior text-based validate modal.
- External multiplexer backend load failures are now reported as warnings (and validation counts/output reflect the change).
- Dry-run spawn failures now show an error modal instead of crashing the app.

The pure seams the validate modal will be built from. No user-visible change: nothing here is wired to the app yet. launch.run_captured_streams keeps stdout and stderr apart, because the merged form is unparseable — run_captured appends stderr *after* stdout, so one DeprecationWarning from any dependency turns json.loads into "Extra data" and silently degrades the JSON path to the text one on whichever machines happen to emit it. run_captured now delegates and merges, so there is one subprocess.run. Decoding is pinned to UTF-8 at errors="replace" rather than text=True, which decodes with the locale encoding at errors="strict" (the #200 failure family, already fixed CLI-side). widgets gains validate_document (parse + version pin + shape gate, returning None rather than raising so the caller's degrade is a value check on a worker thread), validate_findings and validate_header. _RENDERS_VALIDATE_SCHEMA is a hand-written literal, deliberately not an import of documents.VALIDATE_SCHEMA_VERSION: an import would auto-follow a CLI bump and render a v2 document through a v1 renderer with the suite green. A tripwire test fails the moment the two diverge. Detail rendering is depth-2 with a json.dumps leaf, not recursive — enough for every shape the check sites actually emit (scalar, list[str], dict[str,str], list[dict]), including the nested adapters dict policy emits on the *passing* path, which a naive renderer prints as a Python repr. Findings render as one grid so check ids align across rows; the grid keeps multi-line messages (PyYAML MarkedYAMLError in a str(e)) in their column, and overflow="fold" keeps long paths from truncating. Rendering is defensive per finding: one malformed entry is a placeholder row, not a dead modal. Tests: a conftest builder producing real documents through ValidationReport -> _validate_document (so its assert rejects invented check ids), the version-pin tripwire, and a bridge test that renders a live `validate --json` on both the rc-0 and rc-1 legs and asserts no Python repr leaked — zero fixture maintenance, so a check site that starts carrying a new detail shape is covered when it ships.

Pressing `v` now renders the document instead of the text stream. The verdict is the document's own `ok` rather than the exit code, which cannot tell "the checks failed" from "the command broke"; severities are styled rather than string prefixes; and each check's `detail` — the data it knew before flattening itself into a sentence — is reachable at all, inline for warnings and problems and one `d` away for the rest. A failure adds the chained-gates footer, so a short findings list after one is not read as a short list of problems. _show_validate is a sibling worker, not a change to _show_captured, which still serves the two dry runs — they have no document to parse. Its body is wrapped because @work(thread=True) defaults to exit_on_error=True: a JSONDecodeError or a KeyError escaping there would take the whole app down rather than this one modal. exit_on_error=False is not the fix — it trades the crash for pressing `v` and nothing happening. The degrade RE-RUNS validate in text mode rather than showing the JSON it captured. Dumping the document would withhold a perfectly good human rendering at the exact moment the structural one failed and hand the reader a wall of {"schema_version": ...} instead; one sub-second subprocess on a path that should never fire buys a fallback that is byte-for-byte the pre-#210 behavior. Verified live on a real project: forcing validate_document to None reproduces the old modal exactly. The modal's __init__ only stores the document — the worker constructs it on a thread, so the grid is built in compose, on the main thread. The header is a Static holding a Text, never an f-string Label: spec_folder is user-controlled and both Static and Label default to markup=True, so a folder named docs/[wip]-epic-3 would be a MarkupError. #dialog pins an explicit height because BaseDialog is height:auto, which would make the 1fr findings body degenerate. Tests: test_validate_shows_output_modal is migrated, not adapted — its run_captured stub goes dead the moment validate calls run_captured_streams, and a dead stub is a real subprocess rather than a failure. The new stub_validate covers both legs for the same reason: stubbing only the JSON one leaves the degrade's run_captured live. Coverage is the four degrade shapes, the toggle, a raising subprocess (the exit_on_error trap), and both verdicts with rc disagreeing with what the old code would have inferred from it.

The finding has always read as a failure — "external mux backend 'x' failed to load: ..." — while carrying severity ok, so a broken third-party adapter counted as a passing check. It was pinned there only because promoting it inserts " warning: " into shipped text the TUI rendered verbatim; after phase 2 the TUI reads the document and styles from the severity field, so the severity is free to say what the message already does. `bmad-loop mux` has always printed this same condition as `warning:` on stderr — validate was the outlier. It stays below problem deliberately: selection degrades past a broken external (a failed external can never be the selected backend), so the preflight outcome above it is authoritative and neither the verdict nor rc may flip. The comment at the site argued the finding was advisory, which was the "ok" justification; it now records what is actually true. No schema bump. On an affected host counts shifts by one (warning +1, ok -1) while ok and rc do not, and machine.py's "meaning of a value" clause could be read to cover that — but documents.py contracts the check id as the matchable identity, not any given check's outcome. Under the other reading every future severity adjustment is a breaking change. The CLI text does change, and gets uglier: render() preserves the double prefix by design, so the line becomes " ok: warning: external mux backend ...". #210 unblocks the TUI half of the freeze, not the CLI half. test_validation_report_renders_each_severity_verbatim stays green (it builds its own findings), so the new test asserts the now-shipping line at the real site — nothing pinned this finding's severity before, since test_external_backends.py only covers external_backend_errors contents.

coderabbitai · 2026-07-20T18:07:12Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 7bee2a04-a600-4f5a-9f09-3aa1096e7589

📥 Commits

Reviewing files that changed from the base of the PR and between 4ddc366 and 3e95f17.

📒 Files selected for processing (3)

CHANGELOG.md
src/bmad_loop/tui/app.py
tests/test_tui_app.py

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

CHANGELOG.md

Walkthrough

The TUI now runs validate --json, parses and renders structured findings with expandable details, and falls back to text output when parsing fails. Validation captures stdout separately from stderr, while external mux backend failures are classified as warnings.

Changes

Validate TUI and reporting

Layer / File(s)	Summary
External mux warning classification `src/bmad_loop/cli.py`, `tests/test_cli.py`, `CHANGELOG.md`	External mux backend load failures now produce warning findings, with updated CLI tests and changelog entries.
Separated validate output capture `src/bmad_loop/tui/launch.py`, `tests/test_tui_launch.py`	Subprocess stdout and stderr are captured independently using UTF-8 replacement decoding; merged display output remains supported.
Validate document and findings renderer `src/bmad_loop/tui/widgets.py`, `tests/conftest.py`, `tests/test_cli.py`, `tests/test_tui_app.py`	Adds strict JSON document parsing, defensive detail formatting, aligned finding rows, verdict headers, and renderer coverage.
JSON-first modal workflow `src/bmad_loop/tui/app.py`, `src/bmad_loop/tui/screens/modals.py`, `tests/test_tui_app.py`, `docs/tui-guide.md`, `CHANGELOG.md`	The `v` action opens structured findings, `d` toggles details, subprocess failures are guarded, and unrenderable JSON falls back to the text modal.

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

Sequence Diagram(s)

sequenceDiagram
  participant BmadLoopApp
  participant run_captured_streams
  participant validate_document
  participant ValidateFindingsModal
  participant TextOutputModal
  BmadLoopApp->>run_captured_streams: run validate --json
  run_captured_streams-->>BmadLoopApp: return code, stdout, stderr
  BmadLoopApp->>validate_document: parse stdout
  validate_document-->>BmadLoopApp: validated document or None
  alt document is valid
    BmadLoopApp->>ValidateFindingsModal: push structured findings
  else document is unrenderable
    BmadLoopApp->>run_captured_streams: rerun validate without --json
    run_captured_streams-->>BmadLoopApp: return fallback output
    BmadLoopApp->>TextOutputModal: push fallback output
  end

Possibly related PRs

bmad-code-org/bmad-loop#209: Introduced the validate --json document contract consumed by this TUI renderer.
bmad-code-org/bmad-loop#118: Related external mux backend warning behavior in CLI preflight.

Poem

I’m a rabbit with findings in rows,
Where each warning and problem now shows.
Press d, details appear,
JSON hops without fear,
And text waits when parsing goes slow.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% 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 is concise and accurately describes the main change: rendering validate JSON in a findings modal.
Linked Issues check	✅ Passed	The changes implement the requested JSON-driven validate modal, schema fallback, detail toggling, verdict handling, tests, and mux severity update.
Out of Scope Changes check	✅ Passed	The added subprocess-stream refactor, guarded error handling, docs, and tests all support the validate-modal work and appear in scope.

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch feat/tui-validate-findings-210

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: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/bmad_loop/tui/app.py`:
- Around line 990-995: Guard the degrade path in the validate worker by wrapping
launch.run_captured(tail) in the same exception handling used for the JSON
execution. If the text rerun raises, handle it without allowing the worker
exception to escape or the app to exit, and preserve the existing
TextOutputModal behavior for successful reruns.

🪄 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

Run ID: b486f2d6-e1b5-400b-b2fe-2eb4787ed04f

📥 Commits

Reviewing files that changed from the base of the PR and between 5ccf814 and 4ddc366.

📒 Files selected for processing (11)

CHANGELOG.md
docs/tui-guide.md
src/bmad_loop/cli.py
src/bmad_loop/tui/app.py
src/bmad_loop/tui/launch.py
src/bmad_loop/tui/screens/modals.py
src/bmad_loop/tui/widgets.py
tests/conftest.py
tests/test_cli.py
tests/test_tui_app.py
tests/test_tui_launch.py

) Review follow-up. _show_validate wrapped its JSON leg but not the degrade's run_captured, and run_captured *calls* run_captured_streams -- so the two legs spawn the same subprocess. A failure to spawn at all (a venv deleted out from under sys.executable, EAGAIN off a loaded process table) is therefore not a JSON-leg failure the text re-run recovers from; it is the same failure twice, and the second one escaped into @work(thread=True)'s exit_on_error=True and took the whole app down -- the exact outcome the first guard exists to prevent. test_validate_worker_survives_a_raising_subprocess did not catch it because it stubbed run_captured_streams to raise while stubbing run_captured to succeed, a split production cannot produce. Stubbing only the streams seam models the real thing, and the raise stands in for the spawn so no subprocess runs either. _show_captured had the same unguarded call, and unlike the validate one that crash ships today: both dry runs die the same way. Rather than a second copy of the try/except, the guard is extracted to _run_captured_guarded, now the only caller of launch.run_captured in src/ -- a chokepoint instead of two blocks that can drift. The reason lands in the modal body rather than a notify() because the modal is already opening, and a blank panel under an "exit 1" header would say only that something went wrong; the header names the command, so the body does not repeat it. Only the dry-run crash gets a CHANGELOG entry. The validate one existed solely on this unreleased branch, so there is nothing shipped to report.

…#212) documents.py is the library-level read-model projection layer, and its docstring says outright that a non-CLI frontend (the planned web backend) imports these builders directly. Every builder nonetheless kept the leading underscore it had as a private function inside cli.py, which says the opposite. The six *_SCHEMA_VERSION constants beside them were already public; the split had no rationale, it was an artifact of the origin. Deferred out of bmad-code-org#211 because that PR's deliverable was a provable verbatim relocation (AST-identical bodies, untouched tests as the proof) — renaming in the same commit would have voided both. run_token_totals goes public too. The issue framed it as an open question, on the grounds that it is a shared helper rather than document API and could stay a private sibling. It cannot: cmd_status imports it across the module boundary and calls it on the text path, and a private name imported by another module is the exact contradiction being removed. The run_ prefix stays — it marks run-level vs per-task aggregation, which is what the function exists to get right. No back-compat cli._x_document aliases: nothing in-tree needs them and they would re-create the private surface this removes (same call the Unreleased notes already record for runs.tmux_sessions -> mux_sessions). No __all__: no leaf module in the package has one, including probe.py and diagnostics.py, which this module is modelled on. No new CHANGELOG entry — nothing user-visible changes — but the Unreleased entry naming _run_token_totals is updated, since it is unshipped notes rather than history. Also adds the library path's first coverage. Every other --json test reaches the builders through cli.main, so nothing pinned the docstring's central promise that the two paths agree. Two parity tests drive one fixture down both and assert the same dict comes back. Equality is against the raw builder return, not a json round-trip: a consumer holds this dict before serializing, so a tuple where a list belongs is a real defect a round-trip would hide — verified by injecting exactly that and watching the test fail. conftest.make_validate_document (new in bmad-code-org#215) was already calling the builder directly, so it needed the rename too. Note make_validate_document contains _validate_document as a substring — the rename is word-boundary anchored so it and its 13 call sites are untouched.

pbean added 3 commits July 20, 2026 08:47

coderabbitai Bot reviewed Jul 20, 2026

View reviewed changes

Comment thread src/bmad_loop/tui/app.py

pbean merged commit 9771650 into main Jul 20, 2026
9 checks passed

pbean mentioned this pull request Jul 20, 2026

refactor(documents): make the document builders public (#212) #216

Merged

pbean deleted the feat/tui-validate-findings-210 branch July 20, 2026 20:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat(tui): render validate --json in a findings modal (#210)#215

feat(tui): render validate --json in a findings modal (#210)#215
pbean merged 4 commits into
mainfrom
feat/tui-validate-findings-210

pbean commented Jul 20, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Jul 20, 2026 •

edited

Loading

❌ Failed checks (1 warning)

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

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

Uh oh!

Phase 1 — seams

Phase 2 — modal and wiring

Phase 3 — mux.external-backend → warning

Two UI notes

Testing

Summary by CodeRabbit

Uh oh!

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

Uh oh!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

pbean commented Jul 20, 2026 •

edited by coderabbitai Bot

Loading

Phase 3 — `mux.external-backend` → `warning`

coderabbitai Bot commented Jul 20, 2026 •

edited

Loading