refactor(cli): unify diagnose/probe-adapter --json with the pure-document contract - #201
Conversation
…ment contract (#195) `--json` had two incompatible meanings: status/list emitted a pure JSON document, while diagnose/probe-adapter appended a fenced ```json block to a markdown report that a consumer had to scrape out of prose. Both now emit the document instead of the report. stdout parses whole, every human-facing line (ok: trailers, the leak-backstop warning, the unknown-profile notice) moves to stderr, and with --out the document goes to the file while stdout stays empty. Text mode is unchanged. diagnostics.SCHEMA_VERSION deliberately stays at 1 -- it versions the document, and only the packaging changed. probe gains schema_version 1 alongside its existing `version` key, which still holds the probed CLI's --version output. Also hardens the leak guard. Skipping the markdown render in JSON mode turned out to weaken it: json.dumps escapes, so a Windows home path (C:\\Users) evaded _ABS_HOME_RE and a non-ASCII sensitive value evaded the username rule -- both were previously caught by the markdown pass, which refused to emit. _ABS_HOME_RE now matches the escaped separator and render_json uses ensure_ascii=False, with regression tests that fail if either is reverted. Incidental fixes: diagnose no longer renders a markdown report it was about to discard (which double-counted every backstop repair in the warning), and the probe document is sort_keys-stable so two probes diff cleanly. Two follow-ups are filed rather than fixed here: #199 (probe has no egress leak guard and no pseudonymizer) and #200 (ensure_ascii=False lets raw non-ASCII reach stdout, which can raise UnicodeEncodeError on a non-UTF-8 console).
|
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 (6)
🚧 Files skipped from review as they are similar to previous changes (4)
WalkthroughChangesUnified machine-readable output
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/bmad_loop/cli.py (1)
1948-1964: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider centralizing the
--outwrite + trailer pattern.
cmd_probeandcmd_diagnoseeach re-implement "write report/document to--out(json gets+ "\n") and print anok:confirmation on the right stream, else emit to stdout" independently. Sincemachine.pyalready centralizes half of this contract (emit/emit_document), a small helper there (e.g.machine.write_or_emit(rendered, *, out_path, json_mode, ok_message)) would keep both commands' file-vs-stdout branching in one place and reduce the risk of the two commands drifting apart as the contract evolves.♻️ Sketch
def write_or_emit(rendered: str, *, out_path: Path | None, json_mode: bool, ok_message: str, trailer_stream) -> None: if out_path is not None: out_path.write_text((rendered + "\n") if json_mode else rendered, encoding="utf-8") print(ok_message, file=trailer_stream) elif json_mode: emit_document(rendered) else: print(rendered)Optional given the two call sites aren't byte-identical (differing trailer text/conditions), so weigh the abstraction against the modest duplication.
Also applies to: 2056-2072
🤖 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/cli.py` around lines 1948 - 1964, The `--out` file-writing, JSON newline handling, stdout emission, and success trailer logic are duplicated in `cmd_probe` and `cmd_diagnose`. Add a small helper in `machine.py` that accepts the rendered report, optional output path, JSON mode, success message, and trailer stream, then replace both command branches with calls to it while preserving their existing messages and conditions.src/bmad_loop/sanitize.py (1)
76-82: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueConsider supporting forward slashes for Windows paths.
While the regex correctly handles the backslash doubling caused by JSON serialization, Windows paths can sometimes be normalized with forward slashes (e.g.,
C:/Users/alicein Git Bash, MSYS, or certain Node.js environments). You can optionally use[\\/]to catch both separator forms.💡 Proposed refactor
- _ABS_HOME_RE = re.compile(r"/home/|/Users/|/root/|[A-Za-z]:\\{1,2}Users\\{1,2}", re.I) + _ABS_HOME_RE = re.compile(r"/home/|/Users/|/root/|[A-Za-z]:[\\/]{1,2}Users[\\/]{1,2}", re.I)🤖 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/sanitize.py` around lines 76 - 82, Update _ABS_HOME_RE to recognize Windows home paths using either backslash or forward-slash separators, while preserving support for JSON-doubled backslashes and the existing POSIX prefixes.
🤖 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/cli.py`:
- Around line 1948-1964: The `--out` file-writing, JSON newline handling, stdout
emission, and success trailer logic are duplicated in `cmd_probe` and
`cmd_diagnose`. Add a small helper in `machine.py` that accepts the rendered
report, optional output path, JSON mode, success message, and trailer stream,
then replace both command branches with calls to it while preserving their
existing messages and conditions.
In `@src/bmad_loop/sanitize.py`:
- Around line 76-82: Update _ABS_HOME_RE to recognize Windows home paths using
either backslash or forward-slash separators, while preserving support for
JSON-doubled backslashes and the existing POSIX prefixes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7d27e62c-e630-4f3a-ab24-5314365680de
📒 Files selected for processing (13)
.github/ISSUE_TEMPLATE/bug-report.yamlCHANGELOG.mdREADME.mddocs/FEATURES.mddocs/adapter-authoring-guide.mdsrc/bmad_loop/cli.pysrc/bmad_loop/diagnostics.pysrc/bmad_loop/machine.pysrc/bmad_loop/probe.pysrc/bmad_loop/sanitize.pytests/test_cli.pytests/test_diagnostics.pytests/test_probe.py
`machine.emit_document` parses a document before printing it, so the contract "stdout is either one complete valid document or empty" holds structurally. The `--out` branch bypassed that: both commands called `write_text` directly, so `--json --out FILE` could write a malformed document that stdout would have refused to print — backwards, since the file is the one nobody eyeballs before feeding it to a parser. Add `machine.write_document`, the `--out` sibling of `emit_document`, sharing a `_validated` parse-or-raise. It refuses before creating the file, and appends the same trailing newline `print` would, so `--json --out FILE` and `--json > FILE` are byte-identical — now pinned by a test rather than by a comment duplicated at two call sites. The trailer logic stays where it is: `cmd_probe` prints one in both branches with different text, `cmd_diagnose` only under `--out`, so folding those together would need a helper that fits neither. Also pin every arm of `_ABS_HOME_RE`. The table covered only `/home/alice/x`, leaving `/Users/`, `/root/` and the forward-slash Windows form (`C:/Users/…`, what `Path.as_posix` and MSYS-ish tooling produce) correct but untested — the last of these matches via the `/Users/` arm, not the drive-letter arm, which is for backslashes only. Verified by deleting the `/Users/` arm and watching both new cases go red. A negative case bounds the rule in the other direction: firing is fail-closed, so matching every absolute path would turn an ordinary dump into a refusal.
|
Both nitpicks validated. One is a false positive; the other pointed at a real gap, though not the one it named. Pushed as f204c2e.
|
| value | current | proposed |
|---|---|---|
C:/Users/alice/proj |
True | True |
c:/users/bob |
True | True |
C:\Users\alice (and JSON-doubled C:\\Users\\alice) |
True | True |
/c/Users/alice (git-bash) |
True | True |
C:\Users/alice (mixed separators) |
False | True |
D:/data/alice (non-home drive) |
False | False |
The only delta is the mixed-separator oddity, which has no realistic producer — and any string carrying a separator at all is rejected by looks_like_identifier upstream (_IDENTIFIER_RE admits neither / nor \) and redacted to <redacted:str> before it can reach the backstop.
Worth saying that the finding was reasonable to raise: the comment above the rule explained only the backslash-doubling rationale, so it read as if Windows were handled solely by the drive-letter arm. That was a real documentation gap even though the code was right. Fixed both ways — the comment now states why the drive-letter arm is backslash-only, and test_assert_no_leak_fires now pins /Users/, /root/ and C:/Users/… instead of just /home/alice/x. Those cases were correct only by accident; a future "simplification" of the alternation could have silently dropped them. Verified by deleting the /Users/ arm and confirming both new cases go red.
Added a negative case too (D:/data/alice, /var/lib/alice/x), because the bound matters in both directions: firing is fail-closed, so a rule that matched every absolute path would turn an ordinary dump into a refusal.
cli.py — centralizing the --out write + trailer: taking the direction, not the signature
The sketched write_or_emit(..., ok_message, trailer_stream) can't express cmd_probe, which prints a trailer in both branches with different text computed from different data (finding.mode/args.cli vs. out_path), while cmd_diagnose prints one only under --out. Threading that through would need a second message parameter used by one caller — an abstraction that fits neither site.
But the duplication was hiding something. machine.emit_document parses a document before printing it, deliberately, so "stdout is either one complete valid document or empty" holds structurally rather than by convention. The --out branch bypassed it entirely — plain write_text — so --json --out FILE could write a malformed document that stdout would have refused to print. That's backwards: the file is the one nobody eyeballs before feeding it to a parser.
So the extracted piece is the contract, not the control flow. New machine.write_document(path, rendered) is the --out sibling of emit_document, sharing a _validated parse-or-raise; it refuses before creating the file, and appends the same trailing newline print would. --json --out FILE and --json > FILE are now byte-identical — pinned by a test rather than by an explanatory comment duplicated at two call sites. Trailer logic stays exactly where it was.
Smoke-confirmed live on both commands: --out leaves stdout at 0 bytes, the file parses, no fences, and it diffs clean against the piped form.
Docstring coverage (57.78%) — no action
Chronic rather than a regression from this PR: #187 merged at 44.44%, #188 at 60.00%, #191 at 66.67%. Nothing enforces it — pyproject.toml has no [tool.ruff.lint] table at all (so no pydocstyle D rules), there's no .coderabbit.yaml opting into the 80% default, and .trunk/trunk.yaml enables no docstring linter. This PR's only genuinely new function, machine.emit_document, is fully documented; the percentage is measuring pre-existing cmd_* handlers that the repo leaves undocumented as house style, with their contracts in argparse help and module docstrings. Clearing it would mean documenting ~7 untouched functions in a style the codebase deliberately doesn't use.
2547 passed, 1 skipped (+6 from the 2541 baseline); trunk check clean across 14 files.
|
@CodeRabbit review |
✅ Action performedReview finished.
|
Closes #195.
What
--jsonhad two incompatible meanings.status/listemitted a pure JSON document;diagnose/probe-adapterappended a fenced```jsonblock to a markdown report, so a consumer could notjson.loadsthe stream — it had to scrape the fence out of prose.Both commands now emit the document instead of the report:
ok:trailers, the leak-backstop warning, theunknown profilenotice — moves to stderr.--json --out FILEwrites the document to the file, leaves stdout empty, and puts the confirmation on stderr.--json) is unchanged; the human-readable report is still what users hand to maintainers.machine.py's carve-out naming these two commands as exceptions is deleted, along with its mirrors inFEATURES.md, the adapter-authoring guide,README.md, and the bug-report issue template (which told reporters to expect a fenced block).Schema versions
diagnostics.SCHEMA_VERSIONstays at 1, deliberately.machine.pydefines that number as a property of the document — it bumps when a field is removed or renamed, a type changes, or a value's meaning changes — and none of those happened here. Only the packaging changed. Bumping it would falsely tell a consumer pinned to v1 that the fields it reads are gone, while a consumer genuinely broken by the repackaging finds out immediately: the fence is gone andjson.loadsfails.probegainsschema_version: 1as its baseline, alongside its existingversionkey — which still holds the probed CLI's--versionoutput and is deliberately not renamed.Leak-guard hardening (the part worth reviewing closely)
Making
diagnose --jsonskiprender_markdownturned out to weaken the sanitizer, becausejson.dumpsescapes. Both of these fire the guard as raw text and evaded it entirely inside the JSON document:C:\Users\aliceabsolute-home-path\doubled past_ABS_HOME_REsensitive[username]\uXXXXPreviously the markdown pass caught both and refused to emit. Fixed at the source:
_ABS_HOME_REnow matches the escaped separator, andrender_jsonusesensure_ascii=Falseso values reach the guard as themselves. Both have regression tests that were verified to fail when the fix is reverted.Incidental fixes
diagnose --jsonno longer renders a markdown report it was about to discard — which was double-counting every leak-backstop repair in the operator warning (x2for a single occurrence). The count is now pinned by an assertion.sort_keys-stable, so two probes of the same CLI diff cleanly. Pinned by a key-order assertion (a plainjson.loadsround-trip cannot see order).Testing
2541 passed, 1 skipped;trunk checkclean across 13 files.Beyond the suite, each new guarantee was checked by reverting the fix and confirming the test fails — the guard hardening, the repair count, and
sort_keys. Two pre-existing tests needed retargeting: they monkeypatchedrender_markdownto force the refusal path, which JSON mode no longer reaches, so they would have passed vacuously. They are now parametrized[text, json]and patch whichever render the mode actually uses.Live smoke on both commands: pure document, zero fences,
--outleaving stdout at 0 bytes, error paths exiting nonzero with empty stdout.Deliberately out of scope
probe.pyhas no egress leak guard and no pseudonymizer.sanitize.py's own docstring notes thatscrub_jsonpasses story keys and branch names verbatim, and probe never re-scans its rendered bytes. Untouched here; this PR changes packaging, not what may appear inside the document.ensure_ascii=Falselets raw non-ASCII reach stdout, which can raiseUnicodeEncodeErroron a non-UTF-8 console. Narrow (needs non-sensitive non-ASCII and a legacy console) and fails safe: the encode precedes any write, so stdout stays empty, and--outpinsencoding="utf-8". Note for reviewers: do not "fix" it by revertingensure_ascii=False— that flag is what lets the guard see non-ASCII values at all. The issue documents the tradeoffs.Summary by CodeRabbit
--jsonforstatus,list,diagnose, andprobe-adapternow emits a pure stable JSON document to stdout; when using--out FILE, stdout is empty and only the file receives JSON.ok:trailers go to stderr in JSON mode, with consistent JSON across output paths.--jsonbehavior.