From 08d38b3918d3334f4436b508cbdd44f6418a8132 Mon Sep 17 00:00:00 2001 From: pbean Date: Sun, 19 Jul 2026 19:38:18 -0700 Subject: [PATCH] feat(cli): validate --json emits the check findings as a document (#205) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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". --- CHANGELOG.md | 16 +++ README.md | 8 ++ docs/FEATURES.md | 4 +- src/bmad_loop/checks.py | 143 +++++++++++++++++++ src/bmad_loop/cli.py | 292 +++++++++++++++++++++++++++++---------- src/bmad_loop/install.py | 70 +++++++--- src/bmad_loop/machine.py | 13 ++ tests/test_cli.py | 244 ++++++++++++++++++++++++++++++-- tests/test_install.py | 84 +++++++++-- 9 files changed, 763 insertions(+), 111 deletions(-) create mode 100644 src/bmad_loop/checks.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 76f59948..efd7b7be 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,22 @@ breaking changes may land in a minor release. ### Added +- **`bmad-loop validate --json` (#205).** A stable, schema-versioned JSON document of the + preflight: the `ok` verdict, the queue `mode`/`spec_folder`, per-severity `counts`, and every + check as a flat emission-ordered finding. Each finding carries a **stable `check` id** — + `hooks.registered`, `adapter.binary`, `skills.base-incomplete`, … — so CI can branch on a + particular failing gate instead of matching remediation prose, which is the part most likely + to be reworded. `detail` keeps what each check knew before it flattened itself into a + sentence: `mux.backends-detected` keeps every detected backend row rather than the text's + `tmux*, psmux (unavailable)` soup, whose trailing `*` a consumer had to parse to learn which + backend was selected, and `skills.base-incomplete` keeps `missing_markers` as a list rather + than a `", "`-joined string. **A failing check emits the whole document and still exits 1** — + the nonzero code is the verdict being reported, not a failure to produce one, so the existing + `bmad-loop validate || exit 1` in CI is untouched. `machine.py` gains the clause that makes + this well-defined: 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". The text output is byte-for-byte unchanged. + - **`bmad-loop clean --json` / `bmad-loop cleanup --json` (#204).** Stable, schema-versioned JSON documents (one per command — they are separate contracts) reporting what a reclaim removed, or under `--dry-run` would remove: for `clean` the worktree paths, trimmed, diff --git a/README.md b/README.md index 41fd9e31..248dbaa8 100644 --- a/README.md +++ b/README.md @@ -499,6 +499,14 @@ Each run drives its agents inside a dedicated tmux session, `bmad-loop-` `bmad-loop status --json` is the **supported machine-readable surface**: stdout becomes a single JSON document (nothing else is printed), so `bmad-loop status --json | jq '.tasks[].phase'` just works. The document carries `schema_version` (currently 1; changes are additive, anything breaking bumps it), the run's identity and state (`run_id`, `run_type`, `source`, `started_at`, a derived `status` — `finished`/`paused`/`crashed`/`stopped`/`in-progress` — beside the raw flags and `paused_stage`/`paused_reason`/`paused_story_key`), the snapshot's `cache_read_weight`, run-level `tokens` (`raw` + `weighted`), and per-story `tasks` entries: `story_key`, `epic`, `phase`, `attempt`, `review_cycle`, `commit_sha`, `defer_reason`, and a `tokens` object with the four raw counters plus derived `raw` and `weighted`. Everything is computed from the run's `state.json` alone — the weight comes from the persisted policy snapshot, so the numbers match what the run actually enforced. On error (no runs, unknown ref) stdout stays empty and the exit code is 1. The human-readable text output, by contrast, is **best-effort and not a stable interface** — parse the JSON, not the text. +### Scripting `validate` + +`bmad-loop validate --json` turns the preflight into findings a script can act on: stdout becomes a single JSON document carrying `schema_version` (currently 1; additive changes only), the `ok` verdict, the queue `mode` (`sprint`/`stories`) and `spec_folder`, per-severity `counts`, and a flat `findings` array in the order the gates ran — each with a stable `check` id (`hooks.registered`, `adapter.binary`, `queue.sprint-status`, `skills.base-missing`, …), a `severity` of `ok`/`warning`/`problem`, the human `message`, and a `detail` object holding what the check knew before it flattened itself into that sentence (`mux.backends-detected` keeps every detected backend row, `skills.base-incomplete` keeps `missing_markers` as a list). + +**A failing check still emits the whole document, at exit 1.** That is the verdict being reported, not a failure to produce one — so branch on `.ok`, not on the exit status: `bmad-loop validate --json | jq -e '.ok'`, or `jq '.findings[] | select(.severity == "problem") | .check'` to see what to fix. The exit code is unchanged from the text mode, so an existing `bmad-loop validate || exit 1` in CI keeps working. + +Two things to know when matching. `message` is **not** contracted — several problems are the raw text of an underlying config/policy/profile exception, so the wording moves with those modules; `check` is the matchable identity. And a check id's _absence_ is not a pass: the gates are chained, so a policy that fails to load leaves the binary, hook and skill gates with nothing to check, and they contribute no finding at all. Read `ok` for the verdict. + ## Other coding CLIs One generic driver (`adapters/generic.py`) runs any coding CLI that fits the injection + hook-signal transport; everything CLI-specific lives in a declarative **profile** (`adapters/profile.py`), and the terminal transport itself sits behind a pluggable `TerminalMultiplexer` seam (tmux bundled; external backends like the herdr adapter install as packages — see [Terminal multiplexer backends](docs/multiplexer-backends.md)). Built-in profiles ship as TOML in `bmad_loop/data/profiles/`: diff --git a/docs/FEATURES.md b/docs/FEATURES.md index daaf2d62..22a95a1b 100644 --- a/docs/FEATURES.md +++ b/docs/FEATURES.md @@ -177,7 +177,7 @@ See [README.md](../README.md) for the narrative overview and [setup-guide.md](se ### Command reference - `bmad-loop init` — install skills, hooks, policy, gitignore. -- `bmad-loop validate` — preflight all prerequisites. +- `bmad-loop validate` — preflight all prerequisites. `--json` instead emits a stable machine-readable document (schema-versioned; the `ok` verdict, the queue `mode`/`spec_folder`, per-severity `counts`, and every check as a flat emission-ordered finding with a stable `check` id, `severity`, human `message` and structured `detail`) per the [contract below](#machine-readable-output---json); a failing check still emits the whole document, at exit 1 — the nonzero code is the verdict, not a failure to produce one. - `bmad-loop mux` — list registered terminal-multiplexer backends (platform · availability · version · which is selected and why); `mux set ` persists a machine-scoped choice into policy.toml (`--clear` reverts to auto, `--force` allows a name only registered on the target machine). Bundled backend: `tmux`; external backends (e.g. the herdr adapter) register via the `bmad_loop.mux_backends` entry-point group — see [Terminal multiplexer backends](multiplexer-backends.md). - `bmad-loop run` — drive the dev → review → verify → commit loop. - `bmad-loop sweep` — triage + execute open deferred-work entries. @@ -199,4 +199,4 @@ See [README.md](../README.md) for the narrative overview and [setup-guide.md](se ### Machine-readable output (`--json`) -A command's `--json` mode emits exactly one JSON object on stdout and nothing else — no trailers, no fenced blocks. Every document carries an inline integer `schema_version` owned by that command; evolution is additive-only, and anything breaking bumps the version. Errors never produce a partial or error document: the message goes to stderr, stdout stays empty, and the exit code is nonzero — so stdout is always either one complete valid document or empty. The contract is codified in `src/bmad_loop/machine.py` and holds for every command that takes the flag — currently `status`, `list`, `decisions`, `clean`, `cleanup`, `diagnose` and `probe-adapter` — with no exception ([#195](https://github.com/bmad-code-org/bmad-loop/issues/195)). On `diagnose` and `probe-adapter`, whose default output is a human-readable report, `--json` replaces that report rather than appending to it; with `--out FILE` the document goes to the file, stdout stays empty, and the confirmation goes to stderr. +A command's `--json` mode emits exactly one JSON object on stdout and nothing else — no trailers, no fenced blocks. Every document carries an inline integer `schema_version` owned by that command; evolution is additive-only, and anything breaking bumps the version. Errors never produce a partial or error document: the message goes to stderr, stdout stays empty, and the exit code is nonzero — so stdout is always either one complete valid document or empty. An _error_ means the command could not do its job; a command whose job is to report a **verdict** is a different thing, and exits nonzero to carry the answer while still owing you the document (`validate --json` is the case). So the rule is positive rather than inferred from the exit status: **parse non-empty stdout whatever the exit code, and take the verdict from the document's own field** — `ok` on `validate`, which unlike `rc` separates "the checks failed" from "the command broke". The contract is codified in `src/bmad_loop/machine.py` and holds for every command that takes the flag — currently `status`, `list`, `decisions`, `validate`, `clean`, `cleanup`, `diagnose` and `probe-adapter` — with no exception ([#195](https://github.com/bmad-code-org/bmad-loop/issues/195)). On `diagnose` and `probe-adapter`, whose default output is a human-readable report, `--json` replaces that report rather than appending to it; with `--out FILE` the document goes to the file, stdout stays empty, and the confirmation goes to stderr. diff --git a/src/bmad_loop/checks.py b/src/bmad_loop/checks.py new file mode 100644 index 00000000..739841bb --- /dev/null +++ b/src/bmad_loop/checks.py @@ -0,0 +1,143 @@ +"""Findings — the structured form of what ``bmad-loop validate`` reports. + +``validate`` accumulated two lists of prose strings, printed one as `` ok:`` and +the other as ``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 +:class:`Finding` pairs each printed line with a stable ``check`` id, so the id is +the matchable identity and the message stays free to change. + +This lives in its own module rather than in ``cli`` or ``machine``: + +- ``cli`` imports ``install``, and ``install`` must *return* Findings — putting + the type in ``cli`` would make that a cycle. +- ``machine`` is deliberately narrow: transport for the ``--json`` contract, zero + domain knowledge. Severity is a concept no other ``--json`` command has. + +:data:`VALIDATE_CHECKS` is the registry every id must be in, enforced by an +assert in :meth:`ValidationReport.add`. That is what keeps "one printed line = +exactly one Finding" true: a new check site cannot ship without first naming +itself here. The assert only ever fires on a literal an author just wrote, never +on runtime data, so it is a lint with a stack trace rather than a validation. +""" + +from __future__ import annotations + +import sys +from dataclasses import dataclass, field +from typing import Literal + +Severity = Literal["ok", "warning", "problem"] + +# The id names the *gate*, not the outcome — so the same id carries the ok and +# the problem for one check ("adapter.binary" is both "codex found" and "codex +# not found on PATH"). Ids are split only where the two outcomes are genuinely +# different findings with different detail (skills.base vs base-missing vs +# base-incomplete). Scheme: .. +VALIDATE_CHECKS: frozenset[str] = frozenset( + { + "bmad-config", + "policy", + "policy.model-qualified", + "adapter.profile", + "adapter.binary", + "adapter.hookless", + "adapter.httpx", + "queue.sprint-status", + "queue.sprint-status-unknown-keys", + "queue.stories-manifest", + "git.worktree-clean", + "git.probe", + "hooks.config-parse", + "hooks.registered", + "mux.backend", + "mux.preflight", + "mux.backends-detected", + "mux.selection", + "mux.external-backend", + "host.process", + "skills.base", + "skills.base-missing", + "skills.base-incomplete", + "skills.stories-dispatch", + "skills.stories-dispatch-missing", + "skills.stories-dispatch-stale", + } +) + + +@dataclass(frozen=True) +class Finding: + """One check's outcome: a stable ``check`` id, a severity, the human line. + + ``message`` is the exact prose the text mode prints (minus the severity + prefix) and is **not** contracted — several are a bare ``str(e)`` from the + config/policy/profile/sprint-status exceptions, so the wording moves with + those modules. ``detail`` carries what the check knew before it flattened + itself into that sentence, keyed however suits the check. + """ + + check: str + severity: Severity + message: str + detail: dict | None = None + + +@dataclass +class ValidationReport: + """The findings of one ``validate`` pass, in emission order. + + Emission order is the order the gates ran, and it is preserved across + severities — :meth:`render` filters, it never re-sorts, so the text output is + byte-identical to the two-list form it replaced (all stdout lines in append + order, then all stderr lines in append order). + """ + + findings: list[Finding] = field(default_factory=list) + + def add(self, check: str, severity: Severity, message: str, detail: dict | None = None) -> None: + assert check in VALIDATE_CHECKS, f"unregistered check id: {check!r}" + self.findings.append(Finding(check, severity, message, detail)) + + def ok(self, check: str, message: str, detail: dict | None = None) -> None: + self.add(check, "ok", message, detail) + + def warn(self, check: str, message: str, detail: dict | None = None) -> None: + self.add(check, "warning", message, detail) + + def fail(self, check: str, message: str, detail: dict | None = None) -> None: + self.add(check, "problem", message, detail) + + def extend(self, findings: list[Finding]) -> None: + for finding in findings: + self.add(finding.check, finding.severity, finding.message, finding.detail) + + @property + def passed(self) -> bool: + """True when nothing failed. Warnings do not clear it and do not set it — + this is exactly the validate exit code (0 when True, 1 when False).""" + return not any(f.severity == "problem" for f in self.findings) + + def counts(self) -> dict[str, int]: + return { + severity: sum(1 for f in self.findings if f.severity == severity) + for severity in ("ok", "warning", "problem") + } + + def render(self) -> None: + """Print the human-readable form: notes to stdout, then problems to stderr. + + The doubled space in the warning line (`` ok: warning: ...``) is the + shipped output, not a typo to tidy: the warning sites stored + ``" warning: " + msg`` into the same list the `` ok: `` printer walked. + Tidying it would be a silent text change — the validate tests + substring-match on a lowercased stream and would not notice. + """ + for finding in self.findings: + if finding.severity == "ok": + print(f" ok: {finding.message}") + elif finding.severity == "warning": + print(f" ok: warning: {finding.message}") + for finding in self.findings: + if finding.severity == "problem": + print(f"FAIL: {finding.message}", file=sys.stderr) diff --git a/src/bmad_loop/cli.py b/src/bmad_loop/cli.py index cb8fb13b..7ddbb7db 100644 --- a/src/bmad_loop/cli.py +++ b/src/bmad_loop/cli.py @@ -32,6 +32,7 @@ verify, ) from .adapters.base import CodingCLIAdapter +from .checks import Finding, ValidationReport from .engine import Engine from .journal import Journal, load_state, save_state from .model import RunState @@ -192,9 +193,9 @@ def _make_adapters(project: Path, run_dir: Path, policy) -> dict[str, CodingCLIA # ----------------------------------------------------------------- commands -def _platform_preflight() -> tuple[list[str], list[str]]: +def _platform_preflight() -> list[Finding]: """Probe the platform-selected seams — the terminal multiplexer and the process - host — for `cmd_validate`, returning ``(notes, problems)``. + host — for `cmd_validate`, returning the findings in emission order. A backend reports its own readiness through ``available()`` / ``version()``, so a new OS or transport surfaces here by *registering* rather than by adding a @@ -208,26 +209,36 @@ def _platform_preflight() -> tuple[list[str], list[str]]: ) from .process_host import get_process_host - notes: list[str] = [] - problems: list[str] = [] + found: list[Finding] = [] try: backend = get_multiplexer() label = type(backend).__name__ + version = backend.version() if backend.available(): - version = backend.version() - notes.append(f"multiplexer {label} available" + (f" ({version})" if version else "")) + found.append( + Finding( + "mux.backend", + "ok", + f"multiplexer {label} available" + (f" ({version})" if version else ""), + {"backend": label, "available": True, "version": version}, + ) + ) else: - version = backend.version() - problems.append( - f"multiplexer {label} unavailable" - + (f" (reports {version})" if version else "") - + " — its transport binary is missing, the version is unsupported, or a " - "required helper is absent (psmux needs `pwsh` on PATH); " - "see `bmad-loop diagnose`" + found.append( + Finding( + "mux.backend", + "problem", + f"multiplexer {label} unavailable" + + (f" (reports {version})" if version else "") + + " — its transport binary is missing, the version is unsupported, or a " + "required helper is absent (psmux needs `pwsh` on PATH); " + "see `bmad-loop diagnose`", + {"backend": label, "available": False, "version": version}, + ) ) except Exception as e: # noqa: BLE001 — selection or readiness must not abort validate - problems.append(f"multiplexer preflight failed: {e}") + found.append(Finding("mux.preflight", "problem", f"multiplexer preflight failed: {e}")) try: infos = detect_multiplexers() @@ -244,37 +255,131 @@ def _platform_preflight() -> tuple[list[str], list[str]]: ) for i in infos ) - notes.append(f"mux backends: {listed} — `bmad-loop mux` for details") + # The text flattens each row into a suffix soup ("tmux*, psmux + # (unavailable)") whose trailing `*` a consumer would have to parse to + # learn which backend is selected. The detail keeps the rows themselves. + found.append( + Finding( + "mux.backends-detected", + "ok", + f"mux backends: {listed} — `bmad-loop mux` for details", + { + "backends": [ + { + "name": i.name, + "matches_platform": i.matches_platform, + "available": i.available, + "version": i.version, + "selected": i.selected, + "reason": i.reason, + } + for i in infos + ] + }, + ) + ) chosen = next((i for i in infos if i.selected), None) if chosen and chosen.reason in ("env", "policy"): - notes.append(f"multiplexer selection {_mux_reason_label(chosen.reason)}") + # detail keeps the raw enum, not _mux_reason_label's prose: the label is + # wording ("set by [mux] backend in .bmad-loop/policy.toml"), the enum is + # the value MuxBackendInfo.reason actually carries. + found.append( + Finding( + "mux.selection", + "ok", + f"multiplexer selection {_mux_reason_label(chosen.reason)}", + {"backend": chosen.name, "reason": chosen.reason}, + ) + ) # Advisory, not a problem: selection already degraded past the broken # package (a failed external can never be the selected backend), so the # preflight outcome above is authoritative — this line explains the absence. for ep_name, reason in sorted(external_backend_errors().items()): - notes.append(f"external mux backend '{ep_name}' failed to load: {reason}") + found.append( + Finding( + "mux.external-backend", + "ok", + f"external mux backend '{ep_name}' failed to load: {reason}", + {"entry_point": ep_name, "error": reason}, + ) + ) try: - notes.append(f"process host: {type(get_process_host()).__name__}") + host = type(get_process_host()).__name__ + found.append(Finding("host.process", "ok", f"process host: {host}", {"host": host})) except Exception as e: # noqa: BLE001 — a bad BMAD_LOOP_PROCESS_HOST must report, not crash - problems.append(f"process host preflight failed: {e}") + found.append(Finding("host.process", "problem", f"process host preflight failed: {e}")) + + return found + + +VALIDATE_SCHEMA_VERSION = 1 - return notes, problems + +def _validate_document(report: ValidationReport, stories_on: bool, spec_folder: str) -> dict: + """The `validate --json` document: the verdict plus every check that produced it. + + Obeys the pure-document contract in machine.py (additive-only evolution; + anything breaking bumps VALIDATE_SCHEMA_VERSION). ``ok`` is true when no + finding has severity ``problem`` — warnings do not clear it — so it mirrors + the exit code exactly. This is the first ``--json`` command that emits a whole + document at rc 1: the nonzero code is the verdict being reported, not a + failure to produce one (see machine.py on parsing non-empty stdout whatever + the exit code). + + Three things a consumer has to know: + + - **``message`` is not contracted.** Several problems are a bare ``str(e)`` + from the config, policy, profile and sprint-status exceptions, so their + wording moves with those modules. ``check`` is the only matchable identity; + match on it, and read ``message``/``detail`` for humans. + - **Absence is not a pass.** The gates are chained: if policy fails to load, + ``profiles`` is empty and the binary, hook and base-skill gates contribute + no finding at all. A check id missing from ``findings`` means "did not run", + never "passed" — check ``ok`` for the verdict, not the absence of an id. + - **``mux.backends-detected`` is gated on more than one registered backend**, + so a lone-tmux host carries no backend inventory. Same rule as above. + + ``findings`` stays flat and in emission order rather than grouped by severity: + grouping would destroy the cross-severity ordering (the order the gates ran) + and turn "every non-ok finding" into a two-array concatenation. + """ + return { + "schema_version": VALIDATE_SCHEMA_VERSION, + "ok": report.passed, + "mode": "stories" if stories_on else "sprint", + # "" rather than null in sprint mode, where a spec folder is inapplicable — + # the same convention as _list_document's paused_stage. + "spec_folder": spec_folder if stories_on else "", + "counts": report.counts(), + "findings": [ + { + "check": f.check, + "severity": f.severity, + "message": f.message, + "detail": f.detail, + } + for f in report.findings + ], + } def cmd_validate(args: argparse.Namespace) -> int: from .install import relay_registered project = _project(args) - problems: list[str] = [] - notes: list[str] = [] + report = ValidationReport() try: paths = bmadconfig.load_paths(project) - notes.append(f"BMAD config OK: artifacts at {paths.implementation_artifacts}") + report.ok( + "bmad-config", + f"BMAD config OK: artifacts at {paths.implementation_artifacts}", + {"implementation_artifacts": str(paths.implementation_artifacts)}, + ) except bmadconfig.BmadConfigError as e: - problems.append(str(e)) + report.fail("bmad-config", str(e)) paths = None # Policy first — its [stories].source (or a --spec override) selects which @@ -289,10 +394,12 @@ def cmd_validate(args: argparse.Namespace) -> int: try: pol = policy_mod.load(_policy_path(project)) role_names = {role: pol.adapter.resolved(role).name for role in ROLES} - notes.append( + report.ok( + "policy", f"policy OK: gates={pol.gates.mode}, " f"adapter dev={role_names['dev']}, review={role_names['review']}, " - f"triage={role_names['triage']}" + f"triage={role_names['triage']}", + {"gates_mode": pol.gates.mode, "adapters": dict(role_names)}, ) for name in dict.fromkeys(role_names.values()): try: @@ -300,58 +407,73 @@ def cmd_validate(args: argparse.Namespace) -> int: profiles.append(profile) profile_by_name[name] = profile except ProfileError as e: - problems.append(str(e)) + report.fail("adapter.profile", str(e), {"profile": name}) except policy_mod.PolicyError as e: - problems.append(str(e)) + report.fail("policy", str(e)) stories_on, spec_folder = _stories_mode(args, pol) if paths: if stories_on: _validate_stories_queue( - project, paths, spec_folder, [p.skill_tree for p in profiles], notes, problems + project, paths, spec_folder, [p.skill_tree for p in profiles], report ) else: try: ss = sprintstatus.load(paths.sprint_status) actionable = [s for s in ss.stories if s.status in sprintstatus.ACTIONABLE_STATUSES] - notes.append( - f"sprint-status OK: {len(ss.stories)} stories, {len(actionable)} actionable" + report.ok( + "queue.sprint-status", + f"sprint-status OK: {len(ss.stories)} stories, {len(actionable)} actionable", + {"stories": len(ss.stories), "actionable": len(actionable)}, ) if ss.unknown_keys: - notes.append(f" warning: unknown keys ignored: {', '.join(ss.unknown_keys)}") + report.warn( + "queue.sprint-status-unknown-keys", + f"unknown keys ignored: {', '.join(ss.unknown_keys)}", + {"unknown_keys": list(ss.unknown_keys)}, + ) except sprintstatus.SprintStatusError as e: - problems.append(str(e)) + report.fail("queue.sprint-status", str(e)) try: if not verify.worktree_clean(project): - problems.append("git worktree is not clean — commit or stash before running") + report.fail( + "git.worktree-clean", "git worktree is not clean — commit or stash before running" + ) else: - notes.append("git worktree clean") + report.ok("git.worktree-clean", "git worktree clean") except verify.GitError as e: - problems.append(f"git check failed: {e}") + report.fail("git.probe", f"git check failed: {e}") - pf_notes, pf_problems = _platform_preflight() - notes.extend(pf_notes) - problems.extend(pf_problems) + report.extend(_platform_preflight()) for tool in dict.fromkeys(p.binary for p in profiles): if shutil.which(tool): - notes.append(f"{tool} found") + report.ok("adapter.binary", f"{tool} found", {"binary": tool}) else: - problems.append(f"{tool} not found on PATH") + report.fail("adapter.binary", f"{tool} not found on PATH", {"binary": tool}) for profile in profiles: if profile.hookless: - notes.append( - f"{profile.name}: hookless (HTTP/SSE transport) — no hook registration needed" + report.ok( + "adapter.hookless", + f"{profile.name}: hookless (HTTP/SSE transport) — no hook registration needed", + {"profile": profile.name}, ) # The HTTP adapter needs httpx, which ships as an optional extra — # surface a missing install here instead of at run start. if importlib.util.find_spec("httpx") is not None: - notes.append(f"httpx available for {profile.name}") + report.ok( + "adapter.httpx", + f"httpx available for {profile.name}", + {"profile": profile.name}, + ) else: - problems.append( - f"{profile.name}: httpx not installed — run `pip install 'bmad-loop[opencode]'`" + report.fail( + "adapter.httpx", + f"{profile.name}: httpx not installed — " + f"run `pip install 'bmad-loop[opencode]'`", + {"profile": profile.name}, ) continue hook_config = project / profile.hooks.config_path @@ -363,13 +485,23 @@ def cmd_validate(args: argparse.Namespace) -> int: parsed, profile.hooks.dialect, profile.hooks.events ) except json.JSONDecodeError: - problems.append(f"{hook_config} is not valid JSON") + report.fail( + "hooks.config-parse", + f"{hook_config} is not valid JSON", + {"profile": profile.name, "config_path": str(hook_config)}, + ) if hooks_ok: - notes.append(f"bmad-loop hooks registered for {profile.name}") + report.ok( + "hooks.registered", + f"bmad-loop hooks registered for {profile.name}", + {"profile": profile.name, "config_path": str(hook_config)}, + ) else: - problems.append( + report.fail( + "hooks.registered", f"bmad-loop hooks not registered for {profile.name} — " - f"run `bmad-loop init --cli {profile.name}`" + f"run `bmad-loop init --cli {profile.name}`", + {"profile": profile.name, "config_path": str(hook_config)}, ) # opencode config-file model ids are "provider/model" (see the opencode_http docstring); @@ -380,21 +512,29 @@ def cmd_validate(args: argparse.Namespace) -> int: cfg = pol.adapter.resolved(role) prof = profile_by_name.get(cfg.name) if prof is not None and prof.hookless and cfg.model and "/" not in cfg.model: - notes.append( - f" warning: {role} model {cfg.model!r} is not 'provider/model' — " - f"{prof.name} expects e.g. 'anthropic/claude-haiku-4-5'" + report.warn( + "policy.model-qualified", + f"{role} model {cfg.model!r} is not 'provider/model' — " + f"{prof.name} expects e.g. 'anthropic/claude-haiku-4-5'", + {"role": role, "model": cfg.model, "profile": prof.name}, ) base_problems = install.missing_base_skills(project, [p.skill_tree for p in profiles]) if profiles and not base_problems: - notes.append("upstream skills present (bmad-dev-auto + review hunters)") - problems.extend(base_problems) + report.ok( + "skills.base", + "upstream skills present (bmad-dev-auto + review hunters)", + {"trees": list(dict.fromkeys(p.skill_tree for p in profiles))}, + ) + report.extend(base_problems) - for note in notes: - print(f" ok: {note}") - for problem in problems: - print(f"FAIL: {problem}", file=sys.stderr) - return 1 if problems else 0 + if getattr(args, "json", False): + # getattr, not args.json: cmd_validate is called directly by tests (and by + # anything holding a hand-built Namespace) that predate the flag. + machine.emit(_validate_document(report, stories_on, spec_folder)) + else: + report.render() + return 0 if report.passed else 1 def _mux_reason_label(reason: str) -> str: @@ -540,7 +680,7 @@ def _require_base_skills(project: Path, pol, *, require_stories: bool = False) - problems += install.missing_stories_support(project, skill_trees) if problems: for problem in problems: - print(f"FAIL: {problem}", file=sys.stderr) + print(f"FAIL: {problem.message}", file=sys.stderr) print("run `bmad-loop validate` for details", file=sys.stderr) return False return True @@ -594,30 +734,39 @@ def _validate_stories_queue( paths: bmadconfig.ProjectPaths, spec_folder: str, skill_trees: list[str], - notes: list[str], - problems: list[str], + report: ValidationReport, ) -> None: """Stories-mode counterpart of ``cmd_validate``'s sprint-status gate: validate the ``stories.yaml`` manifest + ``SPEC.md`` and confirm the installed ``bmad-dev-auto`` carries the folder+id dispatch flow stories mode needs (an - older skill would HALT at dispatch). Appends notes/problems in place; the - probe carries its own remediation text ("update the bmm module").""" + older skill would HALT at dispatch). Appends findings to ``report`` in place; + the probe carries its own remediation text ("update the bmm module").""" folder = stories_mod.resolve_spec_folder(paths.project, spec_folder) problem = _validate_stories_folder(paths, spec_folder) if problem: - problems.append(problem) + report.fail("queue.stories-manifest", problem, {"spec_folder": str(folder)}) else: try: count = len(stories_mod.load_stories(folder).entries) - notes.append( - f"stories mode OK: {count} stories in {folder}/stories.yaml, SPEC.md present" + report.ok( + "queue.stories-manifest", + f"stories mode OK: {count} stories in {folder}/stories.yaml, SPEC.md present", + {"spec_folder": str(folder), "stories": count}, ) except stories_mod.StoriesError as e: # already validated above — defensive - problems.append(f"stories mode: {e} (spec folder: {folder})") + report.fail( + "queue.stories-manifest", + f"stories mode: {e} (spec folder: {folder})", + {"spec_folder": str(folder)}, + ) stories_probs = install.missing_stories_support(project, skill_trees) if skill_trees and not stories_probs: - notes.append("bmad-dev-auto supports folder+id dispatch (stories mode)") - problems.extend(stories_probs) + report.ok( + "skills.stories-dispatch", + "bmad-dev-auto supports folder+id dispatch (stories mode)", + {"trees": list(dict.fromkeys(skill_trees))}, + ) + report.extend(stories_probs) def _warn_unknown_keys(ss: sprintstatus.SprintStatus) -> None: @@ -2320,6 +2469,7 @@ def add(name: str, func, help: str, *, aliases=()) -> argparse.ArgumentParser: help="validate stories mode against this epic spec folder's stories.yaml " "(overrides [stories].source; skips the sprint-status gate)", ) + machine.add_json_flag(validate_p, "check findings") mux_p = add( "mux", diff --git a/src/bmad_loop/install.py b/src/bmad_loop/install.py index e196295e..ccb5b24c 100644 --- a/src/bmad_loop/install.py +++ b/src/bmad_loop/install.py @@ -24,6 +24,7 @@ from pathlib import Path from .adapters.profile import ALIASES, CLIProfile, ProfileError, load_profiles +from .checks import Finding from .policy import POLICY_TEMPLATE from .process_host import get_process_host @@ -108,18 +109,23 @@ STORIES_PROBE_TEXT = "folder+id dispatch" -def missing_stories_support(project: Path, trees: Sequence[str]) -> list[str]: +def missing_stories_support(project: Path, trees: Sequence[str]) -> list[Finding]: """Problems for stories mode's stricter bmad-dev-auto requirement. Sprint mode drives any bmad-dev-auto; stories mode needs the folder+id dispatch flow, which older skill versions lack. For each active CLI skill tree, confirm ``bmad-dev-auto/step-01-clarify-and-route.md`` exists and - carries the dispatch-protocol marker. Returns one human-readable problem per - tree lacking it (empty = OK). Callers gate this on stories mode only — - sprint-mode runs must not require the newer skill.""" - problems: list[str] = [] + carries the dispatch-protocol marker. Returns one problem :class:`Finding` + per tree lacking it (empty = OK). Callers gate this on stories mode only — + sprint-mode runs must not require the newer skill. + + The two failures are separate check ids because they are separate conditions + with separate remediations: ``-missing`` is a half install (reinstall the + module), ``-stale`` is an install that is simply too old (update it).""" + problems: list[Finding] = [] for tree in dict.fromkeys(trees): probe = project / tree / STORIES_PROBE_SKILL / STORIES_PROBE_FILE + detail = {"tree": tree, "skill": STORIES_PROBE_SKILL, "file": STORIES_PROBE_FILE} try: text = probe.read_text(encoding="utf-8") except (OSError, UnicodeDecodeError): @@ -128,46 +134,70 @@ def missing_stories_support(project: Path, trees: Sequence[str]) -> list[str]: # dispatch-protocol marker can't be confirmed, so report a problem rather # than letting the decode error escape and crash the whole preflight. problems.append( - f"{tree}/{STORIES_PROBE_SKILL}/{STORIES_PROBE_FILE} not found — stories " - f"mode needs folder+id dispatch; update the BMad Method (bmm) module" + Finding( + "skills.stories-dispatch-missing", + "problem", + f"{tree}/{STORIES_PROBE_SKILL}/{STORIES_PROBE_FILE} not found — stories " + f"mode needs folder+id dispatch; update the BMad Method (bmm) module", + detail, + ) ) continue if STORIES_PROBE_TEXT not in text: problems.append( - f"{tree}/{STORIES_PROBE_SKILL} lacks folder+id dispatch (no " - f"{STORIES_PROBE_TEXT!r} in {STORIES_PROBE_FILE}) — stories mode needs a " - f"newer bmad-dev-auto; update the bmm module" + Finding( + "skills.stories-dispatch-stale", + "problem", + f"{tree}/{STORIES_PROBE_SKILL} lacks folder+id dispatch (no " + f"{STORIES_PROBE_TEXT!r} in {STORIES_PROBE_FILE}) — stories mode needs a " + f"newer bmad-dev-auto; update the bmm module", + {**detail, "marker": STORIES_PROBE_TEXT}, + ) ) return problems -def missing_base_skills(project: Path, trees: Sequence[str]) -> list[str]: +def missing_base_skills(project: Path, trees: Sequence[str]) -> list[Finding]: """Problems for the upstream skills the orchestrator drives but doesn't bundle. The dev primitive (bmad-dev-auto) and the three review hunters it invokes inline — adversarial-general, edge-case-hunter, and verification-gap — are installed by the BMad Method module, not by `bmad-loop init`. Each must exist - in every active CLI skill tree and carry its marker files. Returns one - human-readable problem string per missing/incomplete skill; empty list means - OK. Run as a preflight so a missing skill fails loudly with remediation instead - of stalling as an `Unknown command` until the run times out. + in every active CLI skill tree and carry its marker files. Returns one problem + :class:`Finding` per missing/incomplete skill; empty list means OK. Run as a + preflight so a missing skill fails loudly with remediation instead of stalling + as an `Unknown command` until the run times out. + + ``skills.base-incomplete`` carries ``missing_markers`` as a list — the message + joins it with ", " for the human line, which a consumer would otherwise have to + split back apart on a separator the message is free to change. """ required = dict(DEV_BASE_SKILLS) - problems: list[str] = [] + problems: list[Finding] = [] for tree in dict.fromkeys(trees): for skill, markers in required.items(): skill_dir = project / tree / skill if not (skill_dir / "SKILL.md").is_file(): problems.append( - f"{tree}/{skill} not found — install the BMad Method (bmm) module " - f"(the orchestrator drives this upstream skill directly)" + Finding( + "skills.base-missing", + "problem", + f"{tree}/{skill} not found — install the BMad Method (bmm) module " + f"(the orchestrator drives this upstream skill directly)", + {"tree": tree, "skill": skill}, + ) ) continue absent = [m for m in markers if not (skill_dir / m).is_file()] if absent: problems.append( - f"{tree}/{skill} is incomplete (missing {', '.join(absent)}) — " - f"reinstall it from the bmm module" + Finding( + "skills.base-incomplete", + "problem", + f"{tree}/{skill} is incomplete (missing {', '.join(absent)}) — " + f"reinstall it from the bmm module", + {"tree": tree, "skill": skill, "missing_markers": absent}, + ) ) return problems diff --git a/src/bmad_loop/machine.py b/src/bmad_loop/machine.py index b0a2e169..17c165d7 100644 --- a/src/bmad_loop/machine.py +++ b/src/bmad_loop/machine.py @@ -13,6 +13,19 @@ stdout stays empty, and the exit code is nonzero. Consumers may rely on "stdout is either one complete valid document or empty". +An **error** is a command that could not do its job — no runs to dump, an +unresolvable run ref, a policy that will not parse. A command whose job is to +report a *verdict* is a different thing: it did its job, and it exits nonzero to +carry the answer. ``validate --json`` is the case — a failing check is the +finding it was asked for, and the document is still owed. So the rule for +consumers is positive rather than inferred from the exit code: **parse non-empty +stdout whatever the exit code, and take the verdict from the document's own +field** (``ok`` on ``validate``). That field, unlike rc, separates "the checks +failed" from "the command broke": rc 1 is both, ``ok: false`` is only the first, +and a command that broke leaves nothing on stdout to read the field from. Note +that every gate in ``cmd_validate`` runs inside a ``try``, so the command has no +error path of its own — its rc ∈ {0, 1} is purely the verdict. + **Success does not imply a document on stdout.** ``diagnose`` and ``probe-adapter`` also take ``--out FILE``; with both flags the document goes to the file and stdout is legitimately empty at exit 0, with only a confirmation on diff --git a/tests/test_cli.py b/tests/test_cli.py index a42e1859..629a6e8e 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -5,7 +5,14 @@ import pytest import yaml -from conftest import escalated_run, install_bmad_config, machine_json, write_sprint +from conftest import ( + escalated_run, + git, + install_bmad_config, + install_dev_base_skills, + machine_json, + write_sprint, +) from bmad_loop import cli from bmad_loop import policy as policy_mod @@ -2800,11 +2807,21 @@ def _patch_preflight(monkeypatch, backend): monkeypatch.setattr(ph_mod, "get_process_host", lambda: _FakeHost()) +def _preflight_notes_problems(): + """The preflight's pre-#205 (notes, problems) shape, rebuilt from its findings — + these tests assert the *seam probing*, which the Finding refactor did not change.""" + found = cli._platform_preflight() + return ( + [f.message for f in found if f.severity != "problem"], + [f.message for f in found if f.severity == "problem"], + ) + + def test_platform_preflight_reports_available_backend(monkeypatch): # An available backend reports through available()/version() — no sys.platform # branch — and the selected process host is named for visibility. _patch_preflight(monkeypatch, _FakeBackend(ok=True, version="tmux 3.4")) - notes, problems = cli._platform_preflight() + notes, problems = _preflight_notes_problems() assert not problems assert any("_FakeBackend" in n and "tmux 3.4" in n for n in notes) assert any("process host" in n and "_FakeHost" in n for n in notes) @@ -2814,7 +2831,7 @@ def test_platform_preflight_flags_unavailable_backend(monkeypatch): # A backend whose transport binary is absent surfaces here as a problem, so a # new OS registers a backend rather than inlining a win32 block in validate. _patch_preflight(monkeypatch, _FakeBackend(ok=False)) - notes, problems = cli._platform_preflight() + notes, problems = _preflight_notes_problems() assert any("unavailable" in p for p in problems) @@ -2828,7 +2845,7 @@ def _boom(): raise MultiplexerError("BMAD_LOOP_MUX_BACKEND='bogus' matches no registered backend") monkeypatch.setattr(mux_mod, "get_multiplexer", _boom) - notes, problems = cli._platform_preflight() # must not raise + notes, problems = _preflight_notes_problems() # must not raise assert any("bogus" in p for p in problems) @@ -2844,7 +2861,7 @@ def _boom(): raise ProcessHostError("BMAD_LOOP_PROCESS_HOST='bogus' matches no registered host") monkeypatch.setattr(ph_mod, "get_process_host", _boom) - notes, problems = cli._platform_preflight() # must not raise + notes, problems = _preflight_notes_problems() # must not raise assert any("bogus" in p for p in problems) assert any("_FakeBackend" in n for n in notes) # the healthy seam still reported @@ -3037,6 +3054,217 @@ def test_validate_stories_folder_known_selector_ok(project): assert cli._validate_stories_folder(project, STORIES_SPEC_FOLDER, selector="2") is None +# ------------------------- #205: validate --json ------------------------------ + +CLAUDE_ONLY_POLICY = '[adapter]\nname = "claude"\nmodel = "opus"\n' + + +def _make_validate_pass(project, monkeypatch, capsys): + """Set a project up so every validate gate passes, and pin the two gates whose + outcome is a property of the *host* rather than of the project: whether the CLI + binary is on PATH and whether a multiplexer is installed. Without those pins the + rc-0 leg would pass or fail by machine, which is exactly the kind of green that + means nothing.""" + install_bmad_config(project) + _write_policy(project.project, CLAUDE_ONLY_POLICY) + write_sprint(project, {"epic-1": "backlog", "1-1-a": "ready-for-dev"}) + install_dev_base_skills(project.project, folder_id=True) + assert cli.main(["init", "--project", str(project.project)]) == 0 # registers the hooks + git(project.project, "add", "-A") # every file above is a worktree change + git(project.project, "commit", "-q", "-m", "validate fixture") + monkeypatch.setattr(cli.shutil, "which", lambda tool: f"/usr/bin/{tool}") + monkeypatch.setattr( + cli, + "_platform_preflight", + lambda: [cli.Finding("mux.backend", "ok", "multiplexer TmuxBackend available (tmux 3.4)")], + ) + capsys.readouterr() # drop `init`'s chatter — the next read must see only the document + + +def test_validate_json_clean_project_is_a_pure_document_at_rc_0(project, capsys, monkeypatch): + """The happy path: one whole document on stdout, nothing else, ok true.""" + _make_validate_pass(project, monkeypatch, capsys) + + doc = machine_json(["validate", "--project", str(project.project), "--json"], capsys) + assert doc["schema_version"] == cli.VALIDATE_SCHEMA_VERSION == 1 + assert doc["ok"] is True + assert doc["counts"]["problem"] == 0 + assert doc["findings"], "a passing validate still reports what it checked" + assert {f["check"] for f in doc["findings"]} >= {"bmad-config", "policy", "git.worktree-clean"} + + +def test_validate_json_failing_check_emits_whole_document_at_rc_1(project, capsys): + """#205's interesting case: rc 1 is the *verdict*, not a failure to produce a + document. stdout is still one complete document and stderr is still empty — + the `FAIL:` lines became findings, they did not move to the other stream.""" + _write_policy(project.project, CLAUDE_ONLY_POLICY) # no BMAD config -> a real problem + + doc = machine_json(["validate", "--project", str(project.project), "--json"], capsys, rc=1) + assert doc["ok"] is False + assert doc["counts"]["problem"] >= 1 + failed = [f for f in doc["findings"] if f["severity"] == "problem"] + assert "bmad-config" in {f["check"] for f in failed} + + +def test_validate_json_counts_and_ok_agree_with_findings(project, capsys): + """`ok` mirrors the exit code exactly: problems clear it, warnings never do.""" + install_bmad_config(project) + _write_policy(project.project, '[adapter]\nname = "opencode"\nmodel = "haiku"\n') + write_sprint(project, {"epic-1": "backlog"}) + + doc = machine_json(["validate", "--project", str(project.project), "--json"], capsys, rc=1) + findings = doc["findings"] + for severity in ("ok", "warning", "problem"): + assert doc["counts"][severity] == sum(1 for f in findings if f["severity"] == severity) + assert doc["ok"] == (doc["counts"]["problem"] == 0) + # this project warns (bare opencode model) *and* fails — the warning did not + # clear ok, and the warning is not counted among the problems + assert doc["counts"]["warning"] >= 1 and doc["ok"] is False + + +def test_validate_json_warning_message_carries_no_severity_prefix(project, capsys): + """The severity lives in the `severity` field, never doubled into the message. + + The text form prints ` ok: warning: ...` because the note *stored* the + "warning: " prefix; a consumer must not have to strip it back off.""" + install_bmad_config(project) + _write_policy(project.project, '[adapter]\nname = "opencode"\nmodel = "haiku"\n') + write_sprint(project, {"epic-1": "backlog"}) + + doc = machine_json(["validate", "--project", str(project.project), "--json"], capsys, rc=1) + warned = [f for f in doc["findings"] if f["severity"] == "warning"] + assert warned, "the bare opencode model warns" + assert any(f["check"] == "policy.model-qualified" for f in warned) + for finding in warned: + assert "warning:" not in finding["message"] + assert not finding["message"].startswith(" ") + + +@pytest.mark.parametrize( + ("spec", "mode", "folder"), + [(None, "sprint", ""), (STORIES_SPEC_FOLDER, "stories", STORIES_SPEC_FOLDER)], + ids=["sprint", "stories"], +) +def test_validate_json_mode_and_spec_folder_reflect_the_flag(project, capsys, spec, mode, folder): + """`--spec` forces stories mode, and the document says which queue was gated. + + spec_folder is "" — not null — in sprint mode, where it is inapplicable: the + same ""-for-inapplicable convention as _list_document's paused_stage.""" + install_bmad_config(project) + _setup_stories_fixture(project, [_stories_entry("1")]) + _write_policy(project.project, CLAUDE_ONLY_POLICY) # sprint policy either way + write_sprint(project, {"epic-1": "backlog"}) + + argv = ["validate", "--project", str(project.project), "--json"] + if spec: + argv += ["--spec", spec] + doc = machine_json(argv, capsys, rc=1) + assert doc["mode"] == mode + assert doc["spec_folder"] == folder + gate = "queue.stories-manifest" if spec else "queue.sprint-status" + assert gate in {f["check"] for f in doc["findings"]} + + +def test_validate_json_every_emitted_check_is_registered(project, capsys, monkeypatch): + """The `add` assert enforces this per call site; this proves it end-to-end over a + real run, so an id that only ever appears on an unexercised path still has to be + in the registry.""" + from bmad_loop.checks import VALIDATE_CHECKS + + _make_validate_pass(project, monkeypatch, capsys) + passing = machine_json(["validate", "--project", str(project.project), "--json"], capsys) + _write_policy(project.project, "[adapter]\nname = ") # unparseable -> the failure paths + failing = machine_json(["validate", "--project", str(project.project), "--json"], capsys, rc=1) + emitted = {f["check"] for f in (*passing["findings"], *failing["findings"])} + assert emitted, "the run emitted findings" + assert emitted <= VALIDATE_CHECKS + + +def test_validate_json_mux_detail_keeps_the_rows_the_text_flattens(mux_registry, capsys): + """The text line ("alpha*, beta (unavailable)") makes a consumer parse a trailing + `*` to learn which backend is selected. The detail keeps all six MuxBackendInfo + fields verbatim — and the text line itself is unchanged.""" + import sys as _sys + + mux_registry.register_multiplexer( + "alpha", lambda p: p == _sys.platform, lambda: _MuxStub(avail=True, version="alpha 1.2") + ) + mux_registry.register_multiplexer("beta", lambda p: False, lambda: _MuxStub(avail=False)) + + found = cli._platform_preflight() + listing = next(f for f in found if f.check == "mux.backends-detected") + # unchanged text + assert "alpha*" in listing.message and "beta (unavailable)" in listing.message + rows = {r["name"]: r for r in listing.detail["backends"]} + assert set(rows) == {"alpha", "beta"} + assert set(rows["alpha"]) == { + "name", + "matches_platform", + "available", + "version", + "selected", + "reason", + } + assert rows["alpha"]["selected"] is True and rows["beta"]["selected"] is False + assert rows["alpha"]["version"] == "alpha 1.2" and rows["beta"]["version"] is None + assert rows["beta"]["matches_platform"] is False and rows["beta"]["available"] is False + + +def test_platform_preflight_selection_detail_keeps_the_raw_reason(mux_registry, monkeypatch): + """mux.selection's message renders _mux_reason_label's prose; the detail keeps the + enum MuxBackendInfo.reason actually carries, which is the matchable value.""" + mux_registry.register_multiplexer("alpha", lambda p: False, lambda: _MuxStub(avail=True)) + monkeypatch.setenv("BMAD_LOOP_MUX_BACKEND", "alpha") + mux_registry.get_multiplexer.cache_clear() + + selection = next(f for f in cli._platform_preflight() if f.check == "mux.selection") + assert selection.detail == {"backend": "alpha", "reason": "env"} + assert "forced by BMAD_LOOP_MUX_BACKEND" in selection.message # prose stays in the message + + +def test_validate_without_a_json_attribute_still_renders_text(project, capsys): + """cmd_validate reads `getattr(args, "json", False)`, not `args.json`: it is called + directly with hand-built Namespaces that predate the flag (every validate test + above does), and an AttributeError there would be a crash, not a fallback.""" + install_bmad_config(project) + _write_policy(project.project, CLAUDE_ONLY_POLICY) + args = argparse.Namespace(project=str(project.project), spec=None) # no `json` attribute + + cli.cmd_validate(args) # must not raise + out = capsys.readouterr().out + assert " ok: BMAD config OK:" in out # the text form, not a document + assert not out.lstrip().startswith("{") + + +def test_validation_report_renders_each_severity_verbatim(capsys): + """The exact bytes of all three severities. + + The doubled space in the warning line is shipped output, not a typo: the + warning sites stored `" warning: " + msg` into the list the ` ok: ` printer + walked. _validate_output lowercases and substring-matches, so tidying it would + pass every text test silently — this test is the thing that would fail.""" + from bmad_loop.checks import ValidationReport + + report = ValidationReport() + report.ok("git.worktree-clean", "git worktree clean") + report.warn("queue.sprint-status-unknown-keys", "unknown keys ignored: x, y") + report.fail("adapter.binary", "codex not found on PATH") + report.render() + + out, err = capsys.readouterr() + assert out == " ok: git worktree clean\n ok: warning: unknown keys ignored: x, y\n" + assert err == "FAIL: codex not found on PATH\n" + + +def test_validation_report_rejects_an_unregistered_check_id(): + """One printed line = exactly one Finding, and every Finding names a registered + gate. A new check site cannot ship without adding its id to VALIDATE_CHECKS.""" + from bmad_loop.checks import ValidationReport + + with pytest.raises(AssertionError, match="unregistered check id"): + ValidationReport().ok("git.worktee-clean", "typo'd id") # codespell:ignore + + def test_dry_run_stories_shows_plan_halt_markers(project, capsys): """Item 10: dry-run mirrors the real dispatch's leg-1 markers for a pending spec_checkpoint story (`Halt after planning.` + BMAD_LOOP_PLAN_HALT).""" @@ -3200,13 +3428,13 @@ def test_platform_preflight_lists_multiple_backends(mux_registry, monkeypatch): "alpha", lambda p: p == _sys.platform, lambda: _MuxStub(avail=True, version="alpha 1.2") ) mux_registry.register_multiplexer("beta", lambda p: False, lambda: _MuxStub(avail=False)) - notes, problems = cli._platform_preflight() + notes, problems = _preflight_notes_problems() assert any("mux backends:" in n and "alpha*" in n and "beta (unavailable)" in n for n in notes) def test_platform_preflight_single_backend_gets_no_listing(mux_registry): mux_registry.register_multiplexer("alpha", lambda p: True, lambda: _MuxStub(avail=True)) - notes, problems = cli._platform_preflight() + notes, problems = _preflight_notes_problems() assert not any("mux backends:" in n for n in notes) @@ -3214,5 +3442,5 @@ def test_platform_preflight_notes_forced_selection_provenance(mux_registry, monk mux_registry.register_multiplexer("alpha", lambda p: False, lambda: _MuxStub(avail=True)) monkeypatch.setenv("BMAD_LOOP_MUX_BACKEND", "alpha") mux_registry.get_multiplexer.cache_clear() - notes, problems = cli._platform_preflight() + notes, problems = _preflight_notes_problems() assert any("forced by BMAD_LOOP_MUX_BACKEND" in n for n in notes) diff --git a/tests/test_install.py b/tests/test_install.py index 48102e3c..839050f8 100644 --- a/tests/test_install.py +++ b/tests/test_install.py @@ -606,7 +606,7 @@ def test_missing_base_skills_reports_absent_and_incomplete(tmp_path): # them on every run, regardless of the orchestrator's follow-up review) problems = missing_base_skills(tmp_path, [claude.skill_tree]) assert len(problems) == 4 - assert all("install the BMad Method" in p for p in problems) + assert all("install the BMad Method" in p.message for p in problems) # install everything → no problems _install_base_skills(tmp_path, claude.skill_tree) @@ -616,8 +616,8 @@ def test_missing_base_skills_reports_absent_and_incomplete(tmp_path): (tmp_path / claude.skill_tree / "bmad-dev-auto" / "step-04-review.md").unlink() problems = missing_base_skills(tmp_path, [claude.skill_tree]) assert len(problems) == 1 - assert "incomplete" in problems[0] - assert "step-04-review.md" in problems[0] + assert "incomplete" in problems[0].message + assert "step-04-review.md" in problems[0].message # restore it, then drop customize.toml (the review-layer config marker, # BMAD-METHOD #2535/#2550) → a pre-July bmm install is caught as incomplete @@ -625,8 +625,8 @@ def test_missing_base_skills_reports_absent_and_incomplete(tmp_path): (tmp_path / claude.skill_tree / "bmad-dev-auto" / "customize.toml").unlink() problems = missing_base_skills(tmp_path, [claude.skill_tree]) assert len(problems) == 1 - assert "incomplete" in problems[0] - assert "customize.toml" in problems[0] + assert "incomplete" in problems[0].message + assert "customize.toml" in problems[0].message # the newest review layer (verification-gap) reported by name when absent _install_base_skills(tmp_path, claude.skill_tree) # re-complete everything @@ -635,8 +635,8 @@ def test_missing_base_skills_reports_absent_and_incomplete(tmp_path): _shutil.rmtree(tmp_path / claude.skill_tree / "bmad-review-verification-gap") problems = missing_base_skills(tmp_path, [claude.skill_tree]) assert len(problems) == 1 - assert "bmad-review-verification-gap" in problems[0] - assert "install the BMad Method" in problems[0] + assert "bmad-review-verification-gap" in problems[0].message + assert "install the BMad Method" in problems[0].message def test_missing_stories_support_probes_step01_content(tmp_path): @@ -652,19 +652,83 @@ def test_missing_stories_support_probes_step01_content(tmp_path): # step-01 absent → reported (older/half install) problems = missing_stories_support(tmp_path, [tree]) - assert len(problems) == 1 and "not found" in problems[0] + assert len(problems) == 1 and "not found" in problems[0].message # present but WITHOUT the folder+id dispatch marker (a pre-#2549 skill) step01.parent.mkdir(parents=True, exist_ok=True) step01.write_text("# Step 1\nold clarify-and-route, no dispatch protocol\n", encoding="utf-8") problems = missing_stories_support(tmp_path, [tree]) - assert len(problems) == 1 and "folder+id dispatch" in problems[0] + assert len(problems) == 1 and "folder+id dispatch" in problems[0].message # present WITH the marker → OK step01.write_text("route a **folder+id dispatch** invocation\n", encoding="utf-8") assert missing_stories_support(tmp_path, [tree]) == [] +def test_missing_base_skills_findings_carry_ids_and_detail(tmp_path): + """#205: the problems are Findings, so `validate --json` can key on the check id + rather than on remediation prose. The two failure modes are distinct ids, and + `missing_markers` is a list — the message's ", " join is a rendering of it, and + a consumer must not have to split a separator the message is free to change.""" + from bmad_loop.checks import VALIDATE_CHECKS + + claude = get_profile("claude") + absent = missing_base_skills(tmp_path, [claude.skill_tree]) + assert {f.check for f in absent} == {"skills.base-missing"} + assert all(f.severity == "problem" for f in absent) + assert all(f.check in VALIDATE_CHECKS for f in absent) + assert {f.detail["skill"] for f in absent} == { + "bmad-dev-auto", + "bmad-review-adversarial-general", + "bmad-review-edge-case-hunter", + "bmad-review-verification-gap", + } + assert all(f.detail["tree"] == claude.skill_tree for f in absent) + + _install_base_skills(tmp_path, claude.skill_tree) + (tmp_path / claude.skill_tree / "bmad-dev-auto" / "step-04-review.md").unlink() + (tmp_path / claude.skill_tree / "bmad-dev-auto" / "customize.toml").unlink() + incomplete = missing_base_skills(tmp_path, [claude.skill_tree]) + assert len(incomplete) == 1 + assert incomplete[0].check == "skills.base-incomplete" + # a LIST of markers, not the joined string the message renders + assert incomplete[0].detail["missing_markers"] == ["step-04-review.md", "customize.toml"] + for marker in incomplete[0].detail["missing_markers"]: + assert marker in incomplete[0].message + + +def test_missing_stories_support_findings_split_absent_from_stale(tmp_path): + """#205: a half install and a too-old install are different conditions with + different remediations, so they get different check ids — a script pinning a + version bump must be able to tell "reinstall" from "update".""" + from bmad_loop.checks import VALIDATE_CHECKS + from bmad_loop.install import ( + STORIES_PROBE_FILE, + STORIES_PROBE_SKILL, + STORIES_PROBE_TEXT, + missing_stories_support, + ) + + claude = get_profile("claude") + tree = claude.skill_tree + step01 = tmp_path / tree / STORIES_PROBE_SKILL / STORIES_PROBE_FILE + + absent = missing_stories_support(tmp_path, [tree]) + assert [f.check for f in absent] == ["skills.stories-dispatch-missing"] + assert absent[0].detail == { + "tree": tree, + "skill": STORIES_PROBE_SKILL, + "file": STORIES_PROBE_FILE, + } + + step01.parent.mkdir(parents=True, exist_ok=True) + step01.write_text("old clarify-and-route, no dispatch protocol\n", encoding="utf-8") + stale = missing_stories_support(tmp_path, [tree]) + assert [f.check for f in stale] == ["skills.stories-dispatch-stale"] + assert stale[0].detail["marker"] == STORIES_PROBE_TEXT + assert all(f.check in VALIDATE_CHECKS for f in (*absent, *stale)) + + def test_missing_stories_support_reports_non_utf8_probe_without_crashing(tmp_path): """C1: a binary/non-UTF-8 step-01 file must be reported as a problem, not crash the preflight — read_text(encoding="utf-8") raises UnicodeDecodeError (a @@ -682,7 +746,7 @@ def test_missing_stories_support_reports_non_utf8_probe_without_crashing(tmp_pat step01.write_bytes(b"\xff\xfe\x00\x01 not utf-8 \x80\x81") # invalid UTF-8 problems = missing_stories_support(tmp_path, [tree]) - assert len(problems) == 1 and "not found" in problems[0] + assert len(problems) == 1 and "not found" in problems[0].message def test_new_dev_auto_skill_is_additive_for_sprint_mode(tmp_path):