refactor(cli): extract the --json document builders into documents.py - #211
Conversation
The six per-command `--json` builders and their schema constants lived as
private functions in cli.py, where they had grown to ~314 lines of the
dispatch module and were reachable only through argparse. The contract they
serve (machine.py) is not a CLI feature: the planned web backend needs to
call these directly rather than shell out to the CLI and parse its stdout.
Move them verbatim to a new documents.py — the library-level read-model
projection layer, domain object in, contract document dict out. This is the
split probe.py and diagnostics.py already make ("one finding, two render
targets"), generalized to the commands whose document is a dict rather than
a rendered string.
Pure relocation: zero behavior change, zero schema change. The moved bodies
are byte-identical to their previous definitions, and the existing tests —
which pin doc["schema_version"] == cli.X_SCHEMA_VERSION — pass untouched,
which is what proves it. cli.py re-imports the names so those references
keep resolving.
_run_token_totals moves too: _status_document calls it, and leaving it in
cli.py would have made documents.py import the CLI. cmd_status still uses
it for the text rendering, via the same re-import.
Verified: full suite green with zero test edits (2577 passed), and
`status --json` / `list --json` byte-identical before vs after on a fixture
run covering all four terminal statuses, a weighted-token snapshot and a
deferred task.
WalkthroughThe CLI’s JSON document builders and schema constants were moved into a new library-level ChangesJSON projection centralization
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
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.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/bmad_loop/documents.py (1)
46-349: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winVerbatim relocation confirmed; signatures match every
cli.pycall site.Cross-checked
_validate_document,_decisions_document,_run_token_totals,_status_document,_list_document,_cleanup_document, and_clean_documentagainst their call sites incli.py(lines 501, 1518, 1565, 1575, 1621, 1758-1762, 1878-1889) — parameters, defaults, and return shapes all line up, consistent with the stated pure-move objective.One design note worth considering (not a defect): every builder is named with a leading underscore (
_validate_document, etc.), yet the module docstring explicitly states the goal is for a future non-CLI frontend (the planned web backend) imports these builders directly and serializes them itself. A leading underscore conventionally signals "internal, do not import from outside this module," which works against that stated goal — external tooling/linters will flag the import as a private-member access. Since this module is explicitly meant to be a library-level public surface, consider dropping the underscore prefix (or introducing public aliases) before non-CLI consumers start depending on it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/bmad_loop/documents.py` around lines 46 - 349, Consider exposing the document builders as public library APIs by removing the leading underscore from `_validate_document`, `_decisions_document`, `_run_token_totals`, `_status_document`, `_list_document`, `_cleanup_document`, and `_clean_document`, or add public aliases for them. Update all `cli.py` call sites and related references consistently while preserving each function’s signature and return shape.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/bmad_loop/documents.py`:
- Around line 46-349: Consider exposing the document builders as public library
APIs by removing the leading underscore from `_validate_document`,
`_decisions_document`, `_run_token_totals`, `_status_document`,
`_list_document`, `_cleanup_document`, and `_clean_document`, or add public
aliases for them. Update all `cli.py` call sites and related references
consistently while preserving each function’s signature and return shape.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a0a50554-26f7-44ab-ae96-56532dbf172c
📒 Files selected for processing (2)
src/bmad_loop/cli.pysrc/bmad_loop/documents.py
|
@coderabbitai Valid, and the argument is stronger than stated — but deliberately out of scope here. Filed as #212. You're right that the leading underscore contradicts the module docstring's own stated purpose. What sharpens it: the precedent this layer is modeled on names its whole projection surface public. I also checked the constraint your suggestion would run into, and it isn't there: no test calls a builder directly (the only grep hit is prose inside a docstring), so a rename would not force test edits. Not doing it in this PR, for a reason specific to what this PR is. Its deliverable is a provable pure relocation — the bodies were verified byte-identical by AST-extracting each definition from #212 covers it, including the one real judgement call you didn't have the context to see: |
|
Tip For best results, initiate chat on the files or code changes.
🐇🔍 ✏️ Learnings added
|
…#212) documents.py is the library-level read-model projection layer, and its docstring says outright that a non-CLI frontend (the planned web backend) imports these builders directly. Every builder nonetheless kept the leading underscore it had as a private function inside cli.py, which says the opposite. The six *_SCHEMA_VERSION constants beside them were already public; the split had no rationale, it was an artifact of the origin. Deferred out of bmad-code-org#211 because that PR's deliverable was a provable verbatim relocation (AST-identical bodies, untouched tests as the proof) — renaming in the same commit would have voided both. run_token_totals goes public too. The issue framed it as an open question, on the grounds that it is a shared helper rather than document API and could stay a private sibling. It cannot: cmd_status imports it across the module boundary and calls it on the text path, and a private name imported by another module is the exact contradiction being removed. The run_ prefix stays — it marks run-level vs per-task aggregation, which is what the function exists to get right. No back-compat cli._x_document aliases: nothing in-tree needs them and they would re-create the private surface this removes (same call the Unreleased notes already record for runs.tmux_sessions -> mux_sessions). No __all__: no leaf module in the package has one, including probe.py and diagnostics.py, which this module is modelled on. No new CHANGELOG entry — nothing user-visible changes — but the Unreleased entry naming _run_token_totals is updated, since it is unshipped notes rather than history. Also adds the library path's first coverage. Every other --json test reaches the builders through cli.main, so nothing pinned the docstring's central promise that the two paths agree. Two parity tests drive one fixture down both and assert the same dict comes back. Equality is against the raw builder return, not a json round-trip: a consumer holds this dict before serializing, so a tuple where a list belongs is a real defect a round-trip would hide — verified by injecting exactly that and watching the test fail. conftest.make_validate_document (new in bmad-code-org#215) was already calling the builder directly, so it needed the rename too. Note make_validate_document contains _validate_document as a substring — the rename is word-boundary anchored so it and its 13 call sites are untouched.
Pure relocation, no issue (precedent: #197, #206).
Why
The six per-command
--jsonbuilders and their schema constants lived as private functions incli.py:_validate_documentVALIDATE_SCHEMA_VERSION_decisions_documentDECISIONS_SCHEMA_VERSION_status_documentSTATUS_SCHEMA_VERSION_list_documentLIST_SCHEMA_VERSION_cleanup_documentCLEANUP_SCHEMA_VERSION_clean_documentCLEAN_SCHEMA_VERSIONThat was ~314 lines of the dispatch module, growing by a few hundred with every command that adopts the flag, and reachable only through
argparse. But the contract they serve (machine.py) is not a CLI feature — the planned web app's Python backend needs to import these as a library, not shell out to the CLI and parse its stdout.probe.py/diagnostics.pyalready make this split ("one finding, two render targets"). This generalizes it to the commands whose document is a dict rather than a rendered string.What
New
src/bmad_loop/documents.py: the library-level read-model projection layer — domain object in, contract document dict out, obeyingmachine.py's pure-document contract. No I/O, no process state, no printing, no exit codes: the caller loads, this layer projects,machine.emitwrites. New commands add their builder here, not incli.py.cli.pyre-imports the names explicitly, socli.STATUS_SCHEMA_VERSIONandcli._status_documentkeep resolving for existing callers._run_token_totalsmoves too —_status_documentcalls it, and leaving it behind would have madedocuments.pyimport the CLI.cmd_statusstill uses it for text rendering, through the same re-import.This is a pure relocation
Zero behavior change, zero schema change, and no test file was touched. The existing tests are the proof: they pin
doc["schema_version"] == cli.X_SCHEMA_VERSION, so they only pass if both the re-export and the document shape survived.git show HEAD:src/bmad_loop/cli.pyand from the new module, compared as text: all 13 (7 functions + 6 constants) byte-identical, and none still defined incli.py.git diff -- tests/empty.status --jsonandlist --jsondiffed before vs after on a fixture run covering all four terminal statuses (finished/stopped/paused/crashed), a persistedcache_read_weightsnapshot, and a deferred task with a defer reason. Identical stdout, rc and stderr.trunk check(no filter) clean.One note for review
The six constants are re-exported but not referenced in
cli.py, so ruff F401 flags them — and its autofix removes the re-export, breaking everycli.*_SCHEMA_VERSIONreference. Caught here by the test suite. Each import carries a# noqa: F401 — re-exportwith a comment saying why; worth keeping when the next command lands.import bmad_loop.documentscosts ~35ms and pulls notextual, so the library layer is usable from a non-CLI frontend without dragging in the TUI.Summary by CodeRabbit