refactor: separate Behavior Diff report rendering - #6
Merged
Conversation
Signed-off-by: Kent Huang <kent@infuseai.io>
Signed-off-by: Kent Huang <kent@infuseai.io>
Signed-off-by: Kent Huang <kent@infuseai.io>
Signed-off-by: Kent Huang <kent@infuseai.io>
Signed-off-by: Kent Huang <kent@infuseai.io>
Signed-off-by: Kent Huang <kent@infuseai.io>
Signed-off-by: Kent Huang <kent@infuseai.io>
Signed-off-by: Kent Huang <kent@infuseai.io>
Signed-off-by: Kent Huang <kent@infuseai.io>
Signed-off-by: Kent Huang <kent@infuseai.io>
Signed-off-by: Kent Huang <kent@infuseai.io>
Signed-off-by: Kent Huang <kent@infuseai.io>
Signed-off-by: Kent Huang <kent@infuseai.io>
Signed-off-by: Kent Huang <kent@infuseai.io>
Signed-off-by: Kent Huang <kent@infuseai.io>
Signed-off-by: Kent Huang <kent@infuseai.io>
Signed-off-by: Kent Huang <kent@infuseai.io>
Signed-off-by: Kent Huang <kent@infuseai.io>
There was a problem hiding this comment.
🟡 Changes recommended
A few small robustness gaps (notably result.kind validation and clearer renderer failures) can still lead to KeyError crashes or misleading warnings in edge-case inputs.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Refactors Behavior Diff report generation into a layered, import-safe pipeline with a versioned internal ReportData schema and frozen byte-for-byte fixtures, while keeping the existing render.py CLI and report outputs stable.
Changes:
- Introduces immutable, versioned
ReportData+report-data.json, and splits loading/shared copy/Markdown/HTML/CSS intoreporting/. - Makes
render.pya thin orchestrator that renders all outputs in memory, then writesreport-data.json,report.md,report-artifact.html, andreport.html. - Expands deterministic contracts: schema round-trips, import-safety, stdout stability, CSS token checks, and byte-for-byte fixtures for captured + self-reported runs.
File summaries
| File | Description |
|---|---|
| tests/report-schema-test.py | Adds schema round-trip + renderer determinism + import-safety checks. |
| tests/live-report-contract.sh | Extends contract test to validate stdout, fixtures, and report-data.json invariants; adds fixture update mode and usage checks. |
| tests/fixtures/report-rendering/self-reported/report.md | Golden Markdown fixture for self-reported mode. |
| tests/fixtures/report-rendering/self-reported/report.html | Golden full HTML fixture for self-reported mode. |
| tests/fixtures/report-rendering/self-reported/report-artifact.html | Golden HTML artifact fixture for self-reported mode. |
| tests/fixtures/report-rendering/captured/report.md | Golden Markdown fixture for captured mode. |
| tests/fixtures/report-rendering/captured/report.html | Golden full HTML fixture for captured mode. |
| tests/fixtures/report-rendering/captured/report-artifact.html | Golden HTML artifact fixture for captured mode. |
| plugin/skills/behavior-diff/scripts/reporting/init.py | Marks reporting/ as a private package. |
| plugin/skills/behavior-diff/scripts/reporting/schema.py | Defines immutable report schema + deterministic (de)serialization. |
| plugin/skills/behavior-diff/scripts/reporting/content.py | Centralizes shared, format-neutral report copy and headings. |
| plugin/skills/behavior-diff/scripts/reporting/load.py | Loads persisted run evidence into ReportData (flow, decisions, diff, counts). |
| plugin/skills/behavior-diff/scripts/reporting/render_markdown.py | Pure Markdown renderer from ReportData. |
| plugin/skills/behavior-diff/scripts/reporting/render_html.py | Pure HTML renderer (artifact + document) from ReportData + CSS. |
| plugin/skills/behavior-diff/scripts/reporting/report.css | Moves CSS into a source file with __RESULT_BG__ substitution token. |
| plugin/skills/behavior-diff/scripts/render.py | Replaces monolithic renderer with orchestration + writes + stable stdout. |
| plans/2026-09-04-report-rendering-structure.md | Design doc for the refactor and invariants/acceptance criteria. |
| plans/2026-09-04-report-rendering-structure-implementation.md | Implementation plan and sequencing for the refactor. |
| CODING_GUIDELINES.md | Updates py_compile guidance to include reporting/*.py. |
| .github/workflows/ci.yml | Updates CI py_compile step to include reporting/*.py. |
Review details
- Files reviewed: 19/20 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+193
to
+196
| diff_heading=names["diff"], | ||
| decision_heading=names["decision"], | ||
| decision_blurb=decision_blurb(self_reported, before_total), | ||
| flow_heading=names["flow"], |
Contributor
Author
There was a problem hiding this comment.
Fixed in 57759dd. build_content() now enables the one-trial caution only when both before_total == 1 and after_total == 1 (content.py:195-197). The regression test covers (1, 2), (2, 1), and (1, 1).
Comment on lines
+16
to
+20
| def _resolve_css(css: str, result_kind: str) -> str: | ||
| if css.count("__RESULT_BG__") != 1: | ||
| raise ValueError("report.css must contain __RESULT_BG__ exactly once") | ||
| return css.replace("__RESULT_BG__", _RESULT_BACKGROUNDS[result_kind]) | ||
|
|
Contributor
Author
There was a problem hiding this comment.
Fixed in 57759dd. _resolve_css() keeps the CSS-token check first, then rejects an unsupported result kind with ValueError: unsupported report result kind: <kind> (render_html.py:16-21). Tests cover the new error and token-validation precedence.
Comment on lines
+226
to
+231
| def _result(value, path): | ||
| value = _expect_dict(value, path) | ||
| return ResultData( | ||
| text=_expect_str(_field(value, "text", path), path + ".text"), | ||
| kind=_expect_str(_field(value, "kind", path), path + ".kind"), | ||
| ) |
Contributor
Author
There was a problem hiding this comment.
Fixed in 57759dd. Schema-v1 loading now restricts result.kind to good, bad, or neutral and raises the path-aware ValueError at schema.py:227-235. Tests cover both an unsupported string and a non-string value.
- require both variants for the one-trial caution - validate result kinds at schema and renderer boundaries - cover asymmetric counts and error precedence Signed-off-by: Kent Huang <kent@infuseai.io>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ReportDatastructure and internalreport-data.jsonrender.pycommandTest plan
docker run --rm -v "$PWD:/mnt" -w /mnt mvdan/shfmt:v3.14.0 -d -i 2 -ci .uvx ruff@0.16.5 format --check --diff .bash tests/hooks-test.shpython3 plugin/skills/behavior-diff/scripts/decisions.py --checkbash tests/live-report-contract.shbash tests/release-workflow-test.shNotes
origin/main; this branch adds none