feat(cli): validate --json emits the check findings as a document (#205)#209
Conversation
`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".
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
Walkthrough
ChangesValidate JSON findings
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@CodeRabbit review |
✅ Action performedReview finished.
|
Closes #205. Last of the three sub-issues under #196 — closes that tracking issue too.
bmad-loop validateaccumulatednotesandproblemsas prose strings and printed them asok: .../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, andVALIDATE_CHECKS— the frozenset of every id, whichValidationReport.addasserts 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:
installmust return Findings andcliimportsinstall— putting the type incliis a cycle.machineis transport for the--jsoncontract with zero domain knowledge, and severity is a concept no other--jsoncommand has.The refactor
_platform_preflightreturnslist[Finding],install.missing_base_skills/missing_stories_supportreturnlist[Finding],_validate_stories_queueappends to the report, and the notes/problems pair collapses into oneValidationReport.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.pyback 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: " + msginto the same list theok:printer walked. Worth flagging why it needed a new test —_validate_outputlowercases and substring-matches, so a "tidy" passes every text test silently. I checked: with the space tidied, all 17 substring tests stay green and onlytest_validation_report_renders_each_severity_verbatimfails.The document
VALIDATE_SCHEMA_VERSION = 1;{schema_version, ok, mode, spec_folder, counts, findings}.okis true when no finding has severityproblem— warnings do not clear it, so it mirrors the exit code exactly.spec_folderis""in sprint mode, not null (the""-for-inapplicable precedent from_list_document'spaused_stage).findingsstays flat and in emission order; grouping by severity would destroy the cross-severity ordering and make "every non-ok finding" a two-array concat.detailkeeps what each check knew before it flattened itself into a sentence. The important one is mux: the text emitstmux*, psmux (unavailable), whose trailing*a consumer must parse to learn which backend is selected —mux.backends-detectedkeeps theMuxBackendInforows verbatim (all six fields).mux.selectionkeeps the rawreasonenum rather than_mux_reason_label's prose, andskills.base-incompletekeepsmissing_markersas a list rather than the", "-join the message renders.Three things the docstring states explicitly, because each is a way to misread the document:
messageis not contracted (several problems are a barestr(e)whose wording moves with the config/policy/profile/sprint-status modules —checkis the only matchable identity); absence is not a pass (the gates are chained, so a policy that fails to load leavesprofilesempty and the binary/hook/skill gates contributing no finding at all); andmux.backends-detectedis gated on more than one registered backend, so a lone-tmux host carries no inventory.The contract amendment
validate --jsonemits a full document while exiting 1. That does not violate the letter —machine.pysays 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 ownok, which unlike rc separates "the checks failed" from "the command broke". Every gate incmd_validateis inside atry, so the command has no error path of its own: rc ∈ {0, 1} is purely the verdict. Mirrored indocs/FEATURES.md.Rejected alternatives, for the record. Always exit 0 with an
okboolean: validate's own help says "exit non-zero on failure",add_json_flagpromises only "instead of text", it silently disarms everybmad-loop validate || exit 1in CI, and the TUI surfaces rc in its modal. A distinct rc 2: breaks the existing exit contract for a distinctionokalready makes.Traps worth knowing about
getattr(args, "json", False), notargs.json. Ten tests callcmd_validatedirectly with a hand-built Namespace that has nojsonattribute.cmd_statusgets away withargs.jsonbecause its tests go throughcli.main(). In-file precedent:_stories_mode'sgetattr(args, "spec", None). Regression test added — I confirmed that withargs.jsonit fails, and takes 10 pre-existing tests down with it.install.py's signature change reaches run start, not just validate:_require_base_skillsis the other caller, and itsFAIL: {problem}+run \bmad-loop validate` for details` had to stay byte-identical.mux.external-backendstays severityokeven though it reads like a warning — promoting it would insert thewarning:infix, i.e. a text change. Follow-up, not this PR.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.pystubsrun_capturedreturning(1, "FAIL: no policy\n"). Switching it to--jsonand 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
bmad-loop validate --json.Documentation
okvalue.