From 4897da7b8de2f817d0e3afbc568ce8d1df539d66 Mon Sep 17 00:00:00 2001 From: pbean Date: Mon, 20 Jul 2026 12:55:15 -0700 Subject: [PATCH] refactor(documents): make the document builders public (#212) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 #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 #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. --- CHANGELOG.md | 2 +- src/bmad_loop/cli.py | 28 +++++++++---------- src/bmad_loop/documents.py | 30 +++++++++++++------- tests/conftest.py | 4 +-- tests/test_cli.py | 56 +++++++++++++++++++++++++++++++++++++- 5 files changed, 92 insertions(+), 28 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5d59e396..883a81f1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -245,7 +245,7 @@ breaking changes may land in a minor release. from #190 — one JSON object on stdout, inline `schema_version`, additive-only evolution, errors → stderr with empty stdout — now live in one module with shared `emit`/`add_json_flag` helpers; `status --json` uses them (output byte-identical) and the duplicated token-total math - folded into `_run_token_totals`. All four `--json` commands share the contract (#195); `--json` + folded into `run_token_totals`. All four `--json` commands share the contract (#195); `--json` adoption on more commands is tracked in #196. - **Backend-neutral naming for the seam-backed helpers and operator messages.** The multiplexer seam has non-tmux backends now, so the helpers that wrap it drop their legacy tmux names — diff --git a/src/bmad_loop/cli.py b/src/bmad_loop/cli.py index 2bd0f43f..cc444ccc 100644 --- a/src/bmad_loop/cli.py +++ b/src/bmad_loop/cli.py @@ -46,13 +46,13 @@ from .documents import STATUS_SCHEMA_VERSION # noqa: F401 — re-export from .documents import VALIDATE_SCHEMA_VERSION # noqa: F401 — re-export from .documents import ( - _clean_document, - _cleanup_document, - _decisions_document, - _list_document, - _run_token_totals, - _status_document, - _validate_document, + clean_document, + cleanup_document, + decisions_document, + list_document, + run_token_totals, + status_document, + validate_document, ) from .engine import Engine from .journal import Journal, load_state, save_state @@ -504,7 +504,7 @@ def cmd_validate(args: argparse.Namespace) -> int: 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)) + machine.emit(validate_document(report, stories_on, spec_folder)) else: report.render() return 0 if report.passed else 1 @@ -1521,7 +1521,7 @@ def cmd_decisions(args: argparse.Namespace) -> int: # document, not the text line), and regardless of --list: --json *is* # the listing. It cannot fall through to the prompter, which reads stdin # and prints per-answer progress — neither survives a pure document. - machine.emit(_decisions_document(pending)) + machine.emit(decisions_document(pending)) return 0 if not pending: print("no unanswered decisions from past sweeps") @@ -1568,7 +1568,7 @@ def cmd_status(args: argparse.Namespace) -> int: return 1 state = load_state(run_dir) if args.json: - machine.emit(_status_document(state)) + machine.emit(status_document(state)) return 0 kind = f" [{state.run_type}]" if state.run_type != "story" else "" print(f"run {state.run_id}{kind} started {state.started_at}") @@ -1578,7 +1578,7 @@ def cmd_status(args: argparse.Namespace) -> int: print(f"status: PAUSED ({state.paused_stage}) — {state.paused_reason}") else: print("status: in progress (or interrupted)") - raw_total, weighted_total, weight = _run_token_totals(state) + raw_total, weighted_total, weight = run_token_totals(state) if raw_total: print( f"tokens: {weighted_total:,} weighted " @@ -1624,7 +1624,7 @@ def cmd_list(args: argparse.Namespace) -> int: project = _project(args) infos = discover_runs(project) # oldest first if args.json: - machine.emit(_list_document(infos)) + machine.emit(list_document(infos)) return 0 if not infos: print("no runs found") @@ -1762,7 +1762,7 @@ def cmd_cleanup(args: argparse.Namespace) -> int: ) if args.json: machine.emit( - _cleanup_document( + cleanup_document( dry_run=args.dry_run, killed=killed, live=live, unknown=unknown, windows=windows ) ) @@ -1881,7 +1881,7 @@ def cmd_clean(args: argparse.Namespace) -> int: if args.json: machine.emit( - _clean_document( + clean_document( dry_run=dry, retain=retain, cleanup_policy=pol.cleanup, diff --git a/src/bmad_loop/documents.py b/src/bmad_loop/documents.py index 1b846932..23eb61dd 100644 --- a/src/bmad_loop/documents.py +++ b/src/bmad_loop/documents.py @@ -24,6 +24,16 @@ So: add a new command's builder here, not in ``cli.py``. ``cli.py`` re-imports these names, which is what keeps ``cli.STATUS_SCHEMA_VERSION`` and friends resolving for existing callers and tests. + +Every name in this module is public, and a new builder must be too: the +projection surface *is* the API, so a leading underscore would say "do not +import this" about the one thing the module exists to be imported for. The +builders carried one until #212 only because they were born private inside +``cli.py``. ``run_token_totals`` is public for the same reason even though it +projects no document of its own — ``cmd_status`` calls it across the module +boundary to render text, and a private name imported by another module is the +contradiction this module is meant not to have. Keep genuinely intra-module +helpers private, as :mod:`bmad_loop.probe` and :mod:`bmad_loop.diagnostics` do. """ from __future__ import annotations @@ -43,7 +53,7 @@ VALIDATE_SCHEMA_VERSION = 1 -def _validate_document(report: ValidationReport, stories_on: bool, spec_folder: str) -> dict: +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; @@ -76,7 +86,7 @@ def _validate_document(report: ValidationReport, stories_on: bool, spec_folder: "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. + # the same convention as list_document's paused_stage. "spec_folder": spec_folder if stories_on else "", "counts": report.counts(), "findings": [ @@ -94,7 +104,7 @@ def _validate_document(report: ValidationReport, stories_on: bool, spec_folder: DECISIONS_SCHEMA_VERSION = 1 -def _decisions_document(pending: list[Decision]) -> dict[str, object]: +def decisions_document(pending: list[Decision]) -> dict[str, object]: """The `decisions --json` document: every pending decision, in DW order. Obeys the pure-document contract in machine.py (additive-only evolution; @@ -140,7 +150,7 @@ def _decisions_document(pending: list[Decision]) -> dict[str, object]: STATUS_SCHEMA_VERSION = 1 -def _run_token_totals(state: RunState) -> tuple[int, int, float]: +def run_token_totals(state: RunState) -> tuple[int, int, float]: """Run-level token totals as ``(raw, weighted, weight)``. The weight is the run's persisted snapshot — never live policy — so the @@ -156,7 +166,7 @@ def _run_token_totals(state: RunState) -> tuple[int, int, float]: return raw, weighted, weight -def _status_document(state: RunState) -> dict[str, object]: +def status_document(state: RunState) -> dict[str, object]: """The `status --json` document: the stable machine-readable contract. Obeys the pure-document contract in machine.py (additive-only evolution; @@ -164,9 +174,9 @@ def _status_document(state: RunState) -> dict[str, object]: status text, which is best-effort and free to change. Everything here is derived from state.json alone — never from live policy or other project files — so a consumer can reproduce the document, and the weight matches - what the run actually enforced (see _run_token_totals). + what the run actually enforced (see run_token_totals). """ - raw_total, weighted_total, weight = _run_token_totals(state) + raw_total, weighted_total, weight = run_token_totals(state) if state.finished: status = "finished" elif state.paused: @@ -220,7 +230,7 @@ def _status_document(state: RunState) -> dict[str, object]: LIST_SCHEMA_VERSION = 1 -def _list_document(infos: list[RunInfo]) -> dict[str, object]: +def list_document(infos: list[RunInfo]) -> dict[str, object]: """The `list --json` document: one entry per run, oldest first. Obeys the pure-document contract in machine.py (additive-only evolution; @@ -252,7 +262,7 @@ def _list_document(infos: list[RunInfo]) -> dict[str, object]: CLEANUP_SCHEMA_VERSION = 1 -def _cleanup_document( +def cleanup_document( *, dry_run: bool, killed: list[str], @@ -290,7 +300,7 @@ def _cleanup_document( CLEAN_SCHEMA_VERSION = 1 -def _clean_document( +def clean_document( *, dry_run: bool, retain: int, diff --git a/tests/conftest.py b/tests/conftest.py index c8ea2fcc..274fc56e 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -174,7 +174,7 @@ def make_validate_document(findings, *, stories_on: bool = False, spec_folder: s document (a specific severity mix, a specific detail shape) and no subprocess. - It is built by driving the same ValidationReport -> _validate_document path + It is built by driving the same ValidationReport -> validate_document path the CLI drives, so the shape cannot drift from the contract by being hand-written. Going through ValidationReport.add also means its assert (checks.py) rejects invented check ids: a test cannot quietly pin behaviour @@ -183,7 +183,7 @@ def make_validate_document(findings, *, stories_on: bool = False, spec_folder: s report = ValidationReport() for check, severity, message, detail in findings: report.add(check, severity, message, detail) - return documents._validate_document(report, stories_on, spec_folder) + return documents.validate_document(report, stories_on, spec_folder) @pytest.fixture(scope="session") diff --git a/tests/test_cli.py b/tests/test_cli.py index f2a3cc20..fffb493f 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -1057,6 +1057,60 @@ def test_list_json_paused_run_carries_stage(project, capsys): assert entry["paused_stage"] == PAUSE_ESCALATION +# ---------------------------------------- documents.py as an imported library + +# documents.py exists so a non-CLI frontend (the planned web backend) can import +# the builders and serialize them itself, never shelling out to the CLI to parse +# its stdout. These two pin that promise the only way that matters: drive one +# fixture down BOTH paths and assert the same dict comes back. Without them the +# library path has no coverage of its own — every other --json test reaches the +# builders through `cli.main`, so a change that reached `status --json` but not +# `status_document` (or the reverse) would land green and only break in a +# consumer. Equality is asserted against the RAW builder return, not a +# json round-trip: a consumer holds this dict before serializing it, so a tuple +# where a list belongs is a real defect the round-trip would hide. + + +def test_status_document_library_call_matches_the_cli(project, capsys): + from bmad_loop.documents import status_document + from bmad_loop.journal import load_state + from bmad_loop.model import Phase, StoryTask, TokenUsage + + task = StoryTask(story_key="1-1-login", epic=1, phase=Phase.DONE) + task.tokens = TokenUsage( + input_tokens=100, output_tokens=50, cache_creation_tokens=10, cache_read_tokens=1000 + ) + run_dir = _make_run_with_tokens(project, {"1-1-login": task}, weight=0.5) + + from_cli = _status_json(project, capsys) + from_library = status_document(load_state(run_dir)) + + assert from_library == from_cli + + +def test_list_document_library_call_matches_the_cli(project, capsys): + # cmd_list sources its RunInfos the same lazy way — data.py has no textual + # imports, so this does not drag the TUI into a library consumer's process. + from bmad_loop.documents import list_document + from bmad_loop.tui.data import discover_runs + + # Deterministic statuses only: running/interrupted probe pid liveness and + # would flake (see _make_list_run). + _make_list_run(project, "20260101-000000-aaaa", started_at="2026-01-01T00:00:00", finished=True) + _make_list_run( + project, + "20260102-000000-bbbb", + started_at="2026-01-02T00:00:00", + run_type="sweep", + stopped=True, + ) + + from_cli = _list_json(project, capsys) + from_library = list_document(discover_runs(project.project)) + + assert from_library == from_cli + + def test_attach_records_return_pane_inside_tmux(project, monkeypatch): from bmad_loop.tui import launch @@ -3192,7 +3246,7 @@ def test_validate_json_mode_and_spec_folder_reflect_the_flag(project, capsys, sp """`--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.""" + 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