Skip to content

feat(cli): validate --json emits the check findings as a document (#205)#209

Merged
pbean merged 1 commit into
mainfrom
feat/validate-json-196
Jul 20, 2026
Merged

feat(cli): validate --json emits the check findings as a document (#205)#209
pbean merged 1 commit into
mainfrom
feat/validate-json-196

Conversation

@pbean

@pbean pbean commented Jul 20, 2026

Copy link
Copy Markdown
Collaborator

Closes #205. Last of the three sub-issues under #196 — closes that tracking issue too.

bmad-loop validate accumulated notes and problems as prose strings and printed them as ok: ... / FAIL: .... The exit code told you that it failed; only the text told you what, so CI branching on a specific gate had to match remediation sentences — the part most likely to be reworded.

src/bmad_loop/checks.py (new)

Severity, Finding (check / severity / message / detail), ValidationReport, and VALIDATE_CHECKS — the frozenset of every id, which ValidationReport.add asserts membership in. That assert is what keeps one printed line = exactly one Finding true: a new check site cannot ship without first naming itself in the registry. It only ever fires on a literal an author just wrote, never on runtime data.

Its own module, deliberately:

  • install must return Findings and cli imports install — putting the type in cli is a cycle.
  • machine is transport for the --json contract with zero domain knowledge, and severity is a concept no other --json command has.

The refactor

_platform_preflight returns list[Finding], install.missing_base_skills / missing_stories_support return list[Finding], _validate_stories_queue appends to the report, and the notes/problems pair collapses into one ValidationReport.

The text is byte-for-byte unchanged. The 12 existing text tests needed zero edits, and I verified it independently by swapping HEAD's cli.py/install.py back in and diffing both streams on a fixture that exercises all three severities plus the mux listing — identical on stdout and stderr.

That includes the doubled space in ok: warning: ..., which is shipped output rather than a typo: the warning sites stored " warning: " + msg into the same list the ok: printer walked. Worth flagging why it needed a new test — _validate_output lowercases and substring-matches, so a "tidy" passes every text test silently. I checked: with the space tidied, all 17 substring tests stay green and only test_validation_report_renders_each_severity_verbatim fails.

The document

VALIDATE_SCHEMA_VERSION = 1; {schema_version, ok, mode, spec_folder, counts, findings}. ok is true when no finding has severity problem — warnings do not clear it, so it mirrors the exit code exactly. spec_folder is "" in sprint mode, not null (the ""-for-inapplicable precedent from _list_document's paused_stage). findings stays flat and in emission order; grouping by severity would destroy the cross-severity ordering and make "every non-ok finding" a two-array concat.

detail keeps what each check knew before it flattened itself into a sentence. The important one is mux: the text emits tmux*, psmux (unavailable), whose trailing * a consumer must parse to learn which backend is selected — mux.backends-detected keeps the MuxBackendInfo rows verbatim (all six fields). mux.selection keeps the raw reason enum rather than _mux_reason_label's prose, and skills.base-incomplete keeps missing_markers as a list rather than the ", "-join the message renders.

Three things the docstring states explicitly, because each is a way to misread the document: message is not contracted (several problems are a bare str(e) whose wording moves with the config/policy/profile/sprint-status modules — check is the only matchable identity); absence is not a pass (the gates are chained, so a policy that fails to load leaves profiles empty and the binary/hook/skill gates contributing no finding at all); and mux.backends-detected is gated on more than one registered backend, so a lone-tmux host carries no inventory.

The contract amendment

validate --json emits a full document while exiting 1. That does not violate the letter — machine.py says error ⟹ (empty stdout ∧ nonzero rc), not the converse — but the module steered consumers toward "nonzero rc, don't parse", and nothing in it defined error. Added: an error is a command that could not do its job; a command whose job is a verdict exits nonzero to carry it. The inference is replaced with a positive rule — parse non-empty stdout whatever the exit code, and read the verdict from the document's own ok, which unlike rc separates "the checks failed" from "the command broke". Every gate in cmd_validate is inside a try, so the command has no error path of its own: rc ∈ {0, 1} is purely the verdict. Mirrored in docs/FEATURES.md.

Rejected alternatives, for the record. Always exit 0 with an ok boolean: validate's own help says "exit non-zero on failure", add_json_flag promises only "instead of text", it silently disarms every bmad-loop validate || exit 1 in CI, and the TUI surfaces rc in its modal. A distinct rc 2: breaks the existing exit contract for a distinction ok already makes.

Traps worth knowing about

  • getattr(args, "json", False), not args.json. Ten tests call cmd_validate directly with a hand-built Namespace that has no json attribute. cmd_status gets away with args.json because its tests go through cli.main(). In-file precedent: _stories_mode's getattr(args, "spec", None). Regression test added — I confirmed that with args.json it fails, and takes 10 pre-existing tests down with it.
  • install.py's signature change reaches run start, not just validate: _require_base_skills is the other caller, and its FAIL: {problem} + run \bmad-loop validate` for details` had to stay byte-identical.
  • mux.external-backend stays severity ok even though it reads like a warning — promoting it would insert the warning: infix, i.e. a text change. Follow-up, not this PR.
  • The 4 test_platform_preflight_* tests (7 call sites, not 4) get a helper rebuilding the old (notes, problems) shape; assertion bodies verbatim.
  • machine.add_json_flag(validate_p, "check findings") is registered in a parser block nowhere near the other four.

Not done here

The TUI is deliberately left on the text path — its modal renders text and tests/test_tui_app.py stubs run_captured returning (1, "FAIL: no policy\n"). Switching it to --json and rendering findings structurally is the natural follow-up — filed as #210.

Verification

uv run pytest -q → 2577 passed, 1 skipped. trunk check → no issues. 14 new tests; the two regression tests were each confirmed to fail against the bug they guard.

Summary by CodeRabbit

  • New Features

    • Added machine-readable JSON output for bmad-loop validate --json.
    • Reports include validation status, queue details, severity counts, and ordered findings.
    • Complete JSON results are emitted even when validation checks fail, with exit status preserved for scripting.
  • Documentation

    • Documented the JSON schema, fields, parsing behavior, and recommended use of the result’s ok value.

`validate` accumulated two lists of prose strings and printed them as
`  ok:` / `FAIL:`. That made the outcome addressable (the exit code) but
not the checks: a script wanting to branch on "hooks aren't registered"
had to match remediation sentences, which are the part most likely to be
reworded.

A new `checks.py` holds `Finding` (check id, severity, message, detail),
`ValidationReport`, and `VALIDATE_CHECKS` — the registry `add` asserts
against, so a new check site cannot ship without naming itself. The
module is its own file because `install` must return Findings and `cli`
imports `install` (a cycle if the type lived in `cli`), and because
`machine` is deliberately transport-only with no domain knowledge.

`_platform_preflight` and `install.missing_base_skills` /
`missing_stories_support` now return Findings; the notes/problems pair
collapses into one report. The text output is byte-for-byte unchanged —
verified by diffing both streams against HEAD on a fixture exercising all
three severities, and by the 12 existing text tests passing untouched.

`detail` keeps what each check knew before flattening: mux.backends-detected
keeps the MuxBackendInfo rows rather than the text's `tmux*, psmux
(unavailable)` soup (whose trailing `*` a consumer had to parse to learn
the selection), mux.selection keeps the raw reason enum rather than the
label's prose, and skills.base-incomplete keeps missing_markers as a list.

A failing check emits the whole document and still exits 1. machine.py
gains the clause that makes that well-defined: an error is a command that
could not do its job, a verdict command exits nonzero to carry the answer,
and consumers parse non-empty stdout whatever the exit code and read the
verdict from `ok` — which unlike rc separates "checks failed" from "the
command broke".
@coderabbitai

coderabbitai Bot commented Jul 20, 2026

Copy link
Copy Markdown

Review Change Stack

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: c2dad404-a4c2-407c-a11b-4b5f01c269f7

📥 Commits

Reviewing files that changed from the base of the PR and between 5c15e97 and 08d38b3.

📒 Files selected for processing (9)
  • CHANGELOG.md
  • README.md
  • docs/FEATURES.md
  • src/bmad_loop/checks.py
  • src/bmad_loop/cli.py
  • src/bmad_loop/install.py
  • src/bmad_loop/machine.py
  • tests/test_cli.py
  • tests/test_install.py

Walkthrough

bmad-loop validate now collects registered structured findings, preserves text rendering, and supports a schema-versioned --json document. The document includes verdict, queue metadata, severity counts, ordered findings, and structured details, with tests and documentation covering exit-code semantics.

Changes

Validate JSON findings

Layer / File(s) Summary
Finding and output contracts
src/bmad_loop/checks.py, src/bmad_loop/machine.py, README.md, docs/FEATURES.md, CHANGELOG.md
Adds registered Finding and ValidationReport models, severity counts, text rendering, and documentation for the JSON verdict contract.
Structured preflight sources
src/bmad_loop/cli.py, src/bmad_loop/install.py
Converts platform, stories-support, and base-skill checks into structured findings with stable IDs and details.
Validate command integration
src/bmad_loop/cli.py
Routes validation gates through ValidationReport, builds the JSON document, preserves text output, and enables validate --json.
Behavioral validation coverage
tests/test_cli.py, tests/test_install.py
Tests JSON purity, nonzero verdict documents, counts, IDs, details, rendering, and updated preflight findings.

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

Possibly related issues

  • #210 — The new findings document can provide the structured validation data proposed for later TUI consumption.

Possibly related PRs

Suggested reviewers: dracic

Poem

A rabbit found checks in a neat little row,
With JSON carrots ready to show.
Warnings hop stdout, problems thump stderr,
Stable IDs make each finding clearer.
“Read .ok,” says the bun with delight—
Full documents bloom when checks aren’t right.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 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 summarizes the main change: adding validate --json document output for check findings.
Linked Issues check ✅ Passed The PR implements the structured validate --json document, stable check IDs, schema versioning, purity tests, and the machine-output contract required by #205.
Out of Scope Changes check ✅ Passed The changes stay focused on the validate JSON contract and supporting shared findings plumbing, with docs and tests in scope.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ 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/validate-json-196

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.

@pbean

pbean commented Jul 20, 2026

Copy link
Copy Markdown
Collaborator Author

@CodeRabbit review

@coderabbitai

coderabbitai Bot commented Jul 20, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Adopt --json on validate (pure-document contract)

1 participant