diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 3229c3ee..a9484d0c 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -263,6 +263,7 @@ jobs: tests/test_pdf_watermark_recall.py tests/test_formatters_text_serializer.py tests/test_pdf_xml_withheld_recall.py + tests/test_cli_api_parity.py # The required status check on `develop` and `main` is a context named exactly # `test`. The matrix jobs above report per-leg contexts (e.g., `fast-tests (3.12)`), diff --git a/README.md b/README.md index 117a62cd..8b489461 100644 --- a/README.md +++ b/README.md @@ -66,9 +66,9 @@ The product commands are the executable `.py` scripts in the project root — `d | `./tools/fetch_bills.py download-all --start_year --end_year ` | Download all appropriations bills in a year range (or `--file ` for a specific set; `--source govinfo\|api`) | | `./tools/fetch_bills.py search "" [--congress N] [--type hr] [--appropriations]` | Find bills by title over a local BILLSTATUS index (keyless, offline; **requires the index** — fetch it first with `fetch-index`, see below) | | `./tools/fetch_bills.py fetch-index --congress [--type hr]` | Download just the scoped BILLSTATUS ZIP(s) that `search` reads (keyless; tens of MB, not the multi-GB full bulk set) — the lightweight on-ramp for `search` | -| `./diff_bill.py compare ` | Diff two XML versions (HTML by default; `--format json`, `--financial`, `--filter`, `-o`) | +| `./diff_bill.py compare ` | Diff two XML versions (HTML by default; `--format json` for canonical JSON, `--financial`, `--filter`, `-o`) | | `./diff_bill.py compare ` | Diff two versions of a downloaded bill by ordinal, resolved under `--bills-dir` (default `bills/`); a bare `` lists that bill's local versions | -| `./diff_pdf.py -o ` | Diff two PDF versions into the same HTML report | +| `./diff_pdf.py -o ` | Diff two PDF versions into the same HTML report (`--format json` for the same canonical JSON, `--v1-label`/`--v2-label`) | | `./tools/fetch_bill_archives.py` | Bulk-build a full bill-metadata index (all of 112–119) from govinfo archives — **see the warning below** | | `./tools/fetch_bill_text_archives.py --from-congress --to-congress ` | Bulk-download bill text from govinfo into `bills/` (no API key; `--min-versions 2` keeps only bills comparable across versions) | @@ -133,21 +133,19 @@ The index is read from BILLSTATUS ZIPs in `bills/`, which are **not** part of a # Filter to a specific section ./diff_bill.py compare old.xml new.xml --filter "military construction" -# Include unchanged sections -./diff_bill.py compare old.xml new.xml --include-unchanged - -# Save the engine's internal diff dictionary (output defaults to HTML, so request json explicitly) -./diff_bill.py compare old.xml new.xml --format json -o internal-diff.json +# Save the canonical diff JSON (output defaults to HTML, so request json explicitly) +./diff_bill.py compare old.xml new.xml --format json -o diff.json # Generate a standalone HTML report ./diff_bill.py compare old.xml new.xml --format html -o reports/report.html ``` -**Building something against the output?** `--format json` emits the engine's current -*internal* diff dictionary, not the versioned canonical JSON that is the published -interchange contract between the engine and its consumers. Use the canonical document -instead: [`schema/canonical-diff.md`](schema/canonical-diff.md) specifies it, and the HTML -report's **Export and share → Download `diff.json`** button produces one. +**Building something against the output?** `--format json` gives you the canonical diff +JSON, the versioned interchange contract between the engine and its consumers, specified +in [`schema/canonical-diff.md`](schema/canonical-diff.md). It is the same document +`POST /api/compare?output=json` returns and the same one the HTML report's **Export and +share → Download `diff.json`** button saves, so a script, the web service and a browser +download all read one shape. `./diff_pdf.py --format json` does the same for a PDF pair. ### HTML report diff --git a/docs/architecture.md b/docs/architecture.md index fe2499b1..db17d6a1 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -40,9 +40,11 @@ enter through them — so one bill pair renders one way no matter who asked. Tho modules' docstrings name each stage they call, and are the shortest accurate map of the pipeline. -One path deliberately does not go through them: `diff_bill.py compare --format json` -emits the older diff-dict shape straight from `bill_diff_to_dict`, not canonical JSON. -If you are consuming diff output programmatically, take the canonical JSON. +Every path now goes through them, in both output formats. `diff_bill.py compare +--format json` emitted the older diff-dict shape straight from `bill_diff_to_dict` until +[#693](https://github.com/AgoraDMV/DeltaTrack/issues/693); that shape is a pipeline stage +rather than a second output format, and only the XML branch has one. It remains available +as a library call, which is why no flag was kept to select it. ## Pipeline tour diff --git a/scripts/compare_differs.py b/scripts/compare_differs.py index fe69a7cb..b3dc3b93 100644 --- a/scripts/compare_differs.py +++ b/scripts/compare_differs.py @@ -17,27 +17,32 @@ """ import difflib -import json -import subprocess import sys from collections import Counter from pathlib import Path -HERE = Path(__file__).resolve().parent.parent -PY = sys.executable # the interpreter running this script already has the deps +from deltatrack.bill_tree import normalize_bill +from deltatrack.diff_bill import bill_diff_to_dict, diff_bills, filter_diff def our_tool(old_xml: Path, new_xml: Path) -> dict: - """DeltaTrack structured financial diff.""" - out = subprocess.run( - [PY, "diff_bill.py", "compare", str(old_xml), str(new_xml), "--financial", "--format", "json"], - cwd=HERE, - capture_output=True, - text=True, - ) - if out.returncode != 0: - raise SystemExit(f"diff_bill.py failed ({out.returncode}):\n{out.stderr}") - return json.loads(out.stdout) + """DeltaTrack structured financial diff. + + Calls the engine as a library rather than shelling out to `diff_bill.py compare + --financial --format json`, which returns the canonical diff document since #693. + That document deliberately carries no money on a change: #671 removed + `amount_entries`, and the account-level model that would let a figure say what it IS + (top-line appropriation, sub-allocation, ceiling, limitation) is deferred to #115. + The paired old/new amounts this comparison exists to show are therefore not in it. + + `bill_diff_to_dict` is where they do live. It is the engine's internal diff + dictionary, a pipeline stage rather than a published contract, and calling it here is + the intended way to reach it: #693 removed that shape from the command-line surface + precisely because it stays available as a library call. Nothing outside this + repository should read it; this script is inside it. + """ + diff = filter_diff(diff_bills(normalize_bill(old_xml), normalize_bill(new_xml)), financial_only=True) + return bill_diff_to_dict(diff, financial=True) def xmldiff_actions(old_xml: Path, new_xml: Path) -> list: @@ -81,7 +86,10 @@ def main() -> None: print(f" {fin} accounts with dollar changes, each as paired old->new amounts") for c in d["changes"][:3]: f = c["financial"] - path = " > ".join(c.get("match_path", [])) + # Subscript rather than `.get("match_path", [])`: the default silently printed an + # empty breadcrumb for every row once the shape changed, which reads as a bill + # with unnamed accounts rather than as a broken script (#693). + path = " > ".join(c["match_path"]) print(f" {path}: {f['old_amounts'][:1]} -> {f['new_amounts'][:1]} ...") print("\nxmldiff (off-the-shelf structural XML differ)") diff --git a/src/deltatrack/compare/xml.py b/src/deltatrack/compare/xml.py index 3a6c281e..0b0b5f21 100644 --- a/src/deltatrack/compare/xml.py +++ b/src/deltatrack/compare/xml.py @@ -16,8 +16,8 @@ ``versions.v2.source == "xml"`` to drop the PDF line-number gutter. **This module is the only place a bill-XML report is assembled** (#42). The web app, -the ``diff_bill.py compare --format html`` CLI, and ``render_examples.py`` all enter -here, so one bill pair renders one way no matter which surface asked for it. Each of +the ``diff_bill.py compare`` CLI (both of its formats, #693), and ``render_examples.py`` +all enter here, so one bill pair renders one way no matter which surface asked for it. Each of those three used to assemble the canonical → view → HTML chain itself, and the copies had already drifted apart in which version metadata they set. ``diff_pdf.py`` delegates to ``compare/pdf.py`` for the same reason; this is the XML half of that pattern. @@ -44,7 +44,6 @@ def _build_from_trees( end_label: str | None, old_version_number: int | None = None, new_version_number: int | None = None, - include_unchanged: bool = False, filter_text: str | None = None, financial_only: bool = False, ) -> tuple[dict, str]: @@ -57,11 +56,15 @@ def _build_from_trees( None to keep the embedded names. The version *numbers* are the bill's legislative ordinals, which are known when the input is a numbered corpus filename and unknown for a web upload — the renderer prefixes the header with ``v1:``/``v2:`` only when - they are supplied. Financial enrichment is unconditional on the HTML path. + they are supplied. Financial enrichment is unconditional here, on every caller's + behalf, so ``financial_only`` filters and nothing else. + + Unchanged nodes are never carried: ``xml_diff_to_canonical`` drops them, so a + ``filter_diff(include_unchanged=True)`` here would only inflate + ``summary.unchanged`` to a count of entries the document does not contain (#693). """ result = filter_diff( diff_bills(old_tree, new_tree), - include_unchanged=include_unchanged, filter_text=filter_text, financial_only=financial_only, ) @@ -137,6 +140,40 @@ def _render(canonical: dict, title: str) -> str: return format_diff_html(canonical, title) +def compare_xml_trees( + old_tree: BillTree, + new_tree: BillTree, + *, + start_label: str | None = None, + end_label: str | None = None, + old_version_number: int | None = None, + new_version_number: int | None = None, + filter_text: str | None = None, + financial_only: bool = False, +) -> dict: + """Canonical diff JSON for two already-parsed versions (see schema/canonical-diff.md). + + The JSON sibling of :func:`compare_xml_trees_html`, and what + ``diff_bill.py compare --format json`` returns. The two formats are the same + document rendered two ways, which is the point of routing both through here (#693): + the command line used to serialize the engine's internal diff dictionary instead, + so a consumer could reach the published contract only by downloading it from a + rendered report in a browser. + + See :func:`_build_from_trees` for what the version metadata does. + """ + return _build_from_trees( + old_tree, + new_tree, + start_label=start_label, + end_label=end_label, + old_version_number=old_version_number, + new_version_number=new_version_number, + filter_text=filter_text, + financial_only=financial_only, + )[0] + + def compare_xml_trees_html( old_tree: BillTree, new_tree: BillTree, @@ -145,7 +182,6 @@ def compare_xml_trees_html( end_label: str | None = None, old_version_number: int | None = None, new_version_number: int | None = None, - include_unchanged: bool = False, filter_text: str | None = None, financial_only: bool = False, ) -> str: @@ -162,7 +198,6 @@ def compare_xml_trees_html( end_label=end_label, old_version_number=old_version_number, new_version_number=new_version_number, - include_unchanged=include_unchanged, filter_text=filter_text, financial_only=financial_only, ) diff --git a/src/deltatrack/diff_bill.py b/src/deltatrack/diff_bill.py index 7ed0006a..def95381 100644 --- a/src/deltatrack/diff_bill.py +++ b/src/deltatrack/diff_bill.py @@ -2062,40 +2062,36 @@ def cmd_compare(args: argparse.Namespace) -> None: old_path, new_path = _compare_targets(args) old_tree = normalize_bill(old_path) new_tree = normalize_bill(new_path) - fmt = getattr(args, "format", "json") - - if fmt == "html": - # Imported here, not at module scope: compare.xml imports this module. - # It owns the whole XML → HTML chain (#42), so the CLI, the web app, and - # render_examples.py cannot drift into rendering the same pair differently. - from deltatrack.compare.xml import compare_xml_trees_html - - old_stem, new_stem = old_path.stem, new_path.stem - output = compare_xml_trees_html( - old_tree, - new_tree, - start_label=label_from_stem(old_stem), - end_label=label_from_stem(new_stem), - old_version_number=version_number_from_stem(old_stem), - new_version_number=version_number_from_stem(new_stem), - include_unchanged=args.include_unchanged, - filter_text=args.filter, - financial_only=args.financial, - ) - else: - result = filter_diff( - diff_bills(old_tree, new_tree), - include_unchanged=args.include_unchanged, - filter_text=args.filter, - financial_only=args.financial, - ) - diff_dict = bill_diff_to_dict(result, financial=args.financial) - # Extract version numbers from filenames (e.g., "1_reported-in-house.xml" -> 1) - for key, path in (("old_version_number", old_path), ("new_version_number", new_path)): - num = version_number_from_stem(path.stem) - if num is not None: - diff_dict[key] = num - output = json.dumps(diff_dict, indent=2) + fmt = getattr(args, "format", "html") + + # Imported here, not at module scope: compare.xml imports this module. + # It owns the whole XML → report chain (#42), so the CLI, the web app, and + # render_examples.py cannot drift into rendering the same pair differently. + # + # BOTH formats enter it (#693). `--format json` used to serialize this module's + # own `bill_diff_to_dict` output, which is a pipeline stage rather than a + # published contract: it carries no `schema_version`, is specified nowhere, and + # shared only two top-level keys with what `POST /api/compare?output=json` + # returns. So the two surfaces answered the same question in two vocabularies, + # and the canonical document was reachable only by rendering HTML and clicking + # the download button in a browser, which a script cannot do. The internal + # shape stays reachable as a library call (`bill_diff_to_dict`); what is gone is + # its appearance on a command-line surface that documents the other one. + from deltatrack.compare.xml import compare_xml_trees, compare_xml_trees_html + + old_stem, new_stem = old_path.stem, new_path.stem + build = compare_xml_trees_html if fmt == "html" else compare_xml_trees + result = build( + old_tree, + new_tree, + start_label=label_from_stem(old_stem), + end_label=label_from_stem(new_stem), + old_version_number=version_number_from_stem(old_stem), + new_version_number=version_number_from_stem(new_stem), + filter_text=args.filter, + financial_only=args.financial, + ) + output = result if fmt == "html" else json.dumps(result, indent=2) if args.output: with open(args.output, "w", encoding="utf-8") as f: @@ -2179,29 +2175,27 @@ def build_parser() -> argparse.ArgumentParser: "directory listing (compare ), which doesn't need this flag." ), ) - compare.add_argument("-o", "--output", help="Output JSON file (default: stdout)") - compare.add_argument( - "--include-unchanged", - action="store_true", - help="Include unchanged nodes in output", - ) + compare.add_argument("-o", "--output", help="Output file (default: stdout)") compare.add_argument( "--filter", - help="Only include nodes whose match_path contains this substring", + help=( + "Only include changes whose section breadcrumb contains this substring " + "(case-insensitive; the division is not part of the breadcrumb matched)." + ), ) compare.add_argument( "--financial", action="store_true", - help=( - "Only show sections whose set of dollar figures differs between versions; " - "adds each side's amounts to the JSON output" - ), + help="Only show sections whose set of dollar figures differs between versions", ) compare.add_argument( "--format", choices=["json", "html"], default="html", - help="Output format (default: html)", + help=( + "Output format: an HTML report, or the canonical diff JSON the web endpoint " + "returns (schema/canonical-diff.md). Default: html." + ), ) return parser diff --git a/src/deltatrack/diff_pdf.py b/src/deltatrack/diff_pdf.py index 4bc53a31..283c901a 100644 --- a/src/deltatrack/diff_pdf.py +++ b/src/deltatrack/diff_pdf.py @@ -35,6 +35,7 @@ import argparse import difflib +import json import sys from collections import Counter from collections.abc import Mapping @@ -1336,32 +1337,70 @@ def render_pdf_diff_html( ) +def render_pdf_diff_json( + v1_pdf: Path, + v2_pdf: Path, + *, + v1_label: str | None = None, + v2_label: str | None = None, +) -> dict: + """Canonical diff JSON for two PDF paths (see schema/canonical-diff.md). + + The JSON sibling of :func:`render_pdf_diff_html`, delegating to the same + `compare.pdf` entry point the web app calls, so one bill pair produces one document + whichever surface asked for it. + + This command had no JSON output at all before #693, which is a consequence of how + the two pipelines are shaped rather than a decision: the XML branch built an + intermediate dictionary on the way to canonical and the command line serialized it, + while the PDF branch goes from `PdfDiff` straight to canonical and had nothing lying + around to serialize. Once `diff_bill.py compare --format json` returns the contract + rather than that intermediate, the same flag means the same thing on both commands. + """ + from deltatrack.compare.pdf import compare_pdfs + + return compare_pdfs( + v1_pdf.read_bytes(), + v2_pdf.read_bytes(), + start_label=v1_label if v1_label is not None else label_from_stem(v1_pdf.stem), + end_label=v2_label if v2_label is not None else label_from_stem(v2_pdf.stem), + ) + + def build_parser() -> argparse.ArgumentParser: parser = argparse.ArgumentParser( description="Diff two PDF bill versions and produce an HTML diff page " - "(full-bill view, search, and export included).", + "(full-bill view, search, and export included) or the canonical diff JSON.", ) parser.add_argument("v1_pdf", type=Path, help="Path to the older PDF") parser.add_argument("v2_pdf", type=Path, help="Path to the newer PDF") - parser.add_argument("-o", "--output", type=Path, help="Output HTML file (default: stdout)") + parser.add_argument("-o", "--output", type=Path, help="Output file (default: stdout)") parser.add_argument("--v1-label", help="Label for the older version (default: filename stem)") parser.add_argument("--v2-label", help="Label for the newer version (default: filename stem)") + parser.add_argument( + "--format", + choices=["json", "html"], + default="html", + help=( + "Output format: an HTML report, or the canonical diff JSON the web endpoint " + "returns (schema/canonical-diff.md). Default: html." + ), + ) return parser def main(argv: list[str] | None = None) -> None: args = build_parser().parse_args(argv) - html = render_pdf_diff_html( - args.v1_pdf, - args.v2_pdf, - v1_label=args.v1_label, - v2_label=args.v2_label, - ) + labels = {"v1_label": args.v1_label, "v2_label": args.v2_label} + if args.format == "html": + output = render_pdf_diff_html(args.v1_pdf, args.v2_pdf, **labels) + else: + output = json.dumps(render_pdf_diff_json(args.v1_pdf, args.v2_pdf, **labels), indent=2) if args.output: - args.output.write_text(html, encoding="utf-8") + args.output.write_text(output, encoding="utf-8") print(f"Wrote {args.output}", file=sys.stderr) else: - print(html) + print(output) if __name__ == "__main__": diff --git a/tests/test_cli_api_parity.py b/tests/test_cli_api_parity.py new file mode 100644 index 00000000..d3a2e974 --- /dev/null +++ b/tests/test_cli_api_parity.py @@ -0,0 +1,162 @@ +"""One bill pair, both surfaces, one document (#693). + +Nothing ran a single input through the command line *and* the HTTP endpoint and +compared what came back. ``tests/test_pipeline_parity.py`` compares the XML and PDF +*pipelines*; ``tests/test_surface_boundary.py`` enforces import direction. Neither +looks across the two surfaces, which is why ``./diff_bill.py compare --format json`` +could return the engine's internal diff dictionary while +``POST /api/compare?output=json`` returned the canonical contract, two documents +sharing two of eight top-level keys, with the whole suite green. + +The gate is document equality rather than "both look canonical", because a shape check +passes on two documents that are wrong in the same way. The schema test covers the +direction equality cannot: both surfaces drifting together, away from +``schema/canonical-diff.schema.json``. + +**Equality is of the parsed documents, not of the bytes**, and the two surfaces really +do serialize differently: the command writes ``json.dumps(..., indent=2)``, which +indents and escapes non-ASCII, while the endpoint returns Starlette's ``JSONResponse``, +which emits compact UTF-8. Measured on the fixture pair below, 551,433 bytes against +380,599, with ``\u2014`` on one side and raw em dash bytes on the other. Byte identity +would mean indenting the HTTP response to match a file on disk, which costs every API +caller about 45% more payload and buys nothing: ``schema/canonical-diff.md`` specifies +a document, and #691 (the epic making every surface produce the same answer) asks the +surfaces to agree on the answer, not on the whitespace. So do not "strengthen" this +into a byte comparison; it would fail on formatting while saying nothing about whether +the two agree. + +Both formats are held to it. ``./diff_pdf.py`` gained ``--format json`` in the same +change, and the PDF half is the one with no prior behaviour to preserve, so pinning it +now is what keeps it from acquiring a second vocabulary the way the XML half did. + +Real bill documents, so ``@pytest.mark.slow`` (see AGENTS.md). The fixture pair is +committed and manifested in both formats, so these fail closed rather than skipping. +""" + +from __future__ import annotations + +import json +import shutil +from pathlib import Path + +import pytest + +from deltatrack.diff_bill import build_parser, cmd_compare +from deltatrack.diff_pdf import main as diff_pdf_main +from tests.corpus_paths import FIXTURES_DIR + +ROOT = Path(__file__).resolve().parent.parent +SCHEMA = ROOT / "schema" / "canonical-diff.schema.json" + +BILL_DIR = FIXTURES_DIR / "118-hr-8752" +V1_STEM = "1_reported-in-house" +V2_STEM = "2_engrossed-in-house" + + +def _cli_json(tmp_dir: Path, old: Path, new: Path) -> str: + """The command's JSON output, driven through its real argument parser. + + Dispatches on the extension, because the two commands are meant to be the same + offer: `./diff_bill.py compare --format json` and `./diff_pdf.py --format json`. + """ + out = tmp_dir / "cli.json" + if old.suffix == ".xml": + cmd_compare(build_parser().parse_args(["compare", str(old), str(new), "--format", "json", "-o", str(out)])) + else: + diff_pdf_main([str(old), str(new), "--format", "json", "-o", str(out)]) + return out.read_text(encoding="utf-8") + + +def _endpoint_json(old: Path, new: Path) -> dict: + """``POST /api/compare?output=json``, driven through the real FastAPI route.""" + from fastapi.testclient import TestClient + + from web.app import app + + fmt = old.suffix.lstrip(".") + with open(old, "rb") as start, open(new, "rb") as end: + response = TestClient(app).post( + f"/api/compare?format={fmt}&output=json", + files={ + "start_file": (old.name, start, "application/octet-stream"), + "end_file": (new.name, end, "application/octet-stream"), + }, + ) + assert response.status_code == 200, response.text + return response.json() + + +@pytest.fixture(scope="module", params=["xml", "pdf"]) +def unprefixed_pair(request, tmp_path_factory) -> tuple[Path, Path]: + """The fixture pair copied under stems carrying no ``_`` legislative ordinal. + + The two surfaces derive a version's identity from the filename by different + algorithms: ``version_stems.label_from_stem`` strips a numeric prefix and + ``version_number_from_stem`` reads the ordinal off it, while + ``web/app.py::_label_from_filename`` strips only the path and the extension and has + no ordinal to read at all. On ``1_reported-in-house.xml`` they therefore disagree, + and that disagreement is #692 (one bill pair, three different version headings), a + property of the two label algorithms rather than of the diff. + + Removing the prefix removes that variable, so the parity gate below measures the + document rather than re-measuring #692. What the corpus filenames *do* change is + asserted separately, so the exclusion stays one named key wide. + """ + ext = request.param + tmp = tmp_path_factory.mktemp(f"unprefixed-{ext}") + old, new = tmp / f"reported-in-house.{ext}", tmp / f"engrossed-in-house.{ext}" + shutil.copyfile(BILL_DIR / f"{V1_STEM}.{ext}", old) + shutil.copyfile(BILL_DIR / f"{V2_STEM}.{ext}", new) + return old, new + + +@pytest.fixture(scope="module", params=["xml", "pdf"]) +def corpus_pair(request) -> tuple[Path, Path]: + """The committed fixture pair under its real ``_