Section 3a golden change tree (#144) - #157
Open
houfu wants to merge 7 commits into
Open
Conversation
A declarative, serialisable scope over a change tree -- kinds, address prefixes, labels, roles, min_chars and has_inline -- rather than a bag of keyword arguments, because the same specification has to travel as a CLI flag set, an MCP tool argument and (in 1.1) verify's `allowed` scope (ADR-0025, ADR-0033). Address-prefix matching is segment-aligned so `/section[1]` cannot swallow `/section[11]`, both of which exist in the sample pair. Comparison.filter() and the filter's wiring into ComparisonConfig land in the next commit, alongside statistics, since both consume this module.
redlines/statistics.py adds ChangeCounts, SectionStatistics and ComparisonStatistics, computed fresh from a Comparison's change tree and both block trees rather than cached. A "section" is reinterpreted from #139's literal "top-level section": it is every block of kind `section` that has a `heading` child, wherever it sits, with each change attributed to its nearest enclosing such unit only. Read literally, the sample pair's actual top-level section is the entire ~120-block body of the agreement, and a single density number for it says nothing a reviewer could use; #139's own next clause ("using the heading breadcrumb from ADR-0029") is what the reinterpretation is answering. Comparison gains statistics() and filter(): filter() is the promised convenience over ChangeFilter/filter_changes, returning a Comparison with the same unpruned trees and alignment, a filtered change list, and config.filter set to the spec that produced it. ComparisonConfig gains a `filter` field for the same honesty reason -- a filtered payload must not look like a full one. This extends the JSON v2 schema frozen in #137, before that freeze PR has merged: `statistics` is a new, always-present top-level key, and `config.filter` is a new, nullable key. Both are additive, minor- version changes under ADR-0011, and tests/test_json_v2.py's own "nothing here yet" claims are updated alongside the schema they were describing.
One small, throwaway function local to this test module turns a comparison's inline ops into (address, old, new, context) tuples, proving R27a's constraint is met without shipping a public helper before 1.1's applier (R26) exists to shape it. Checked against the sample pair under both profiles: every tuple is complete, its offsets slice back to the exact old/new text, its address resolves and agrees with the context, the context is unique among the source tree's blocks, the repetitive schedule's one edited item is named directly and shown unique in the raw source bytes too, and replaying every block's ops together reconstructs its test text exactly.
SectionStatistics.blocks tallied every block in test.walk(), including each section-with-heading unit's own container block against itself, since _nearest_enclosing is (by design) inclusive of the starting address. This inflated blocks/density by one per section: /section[1] reported 3 instead of ADR-0033's 2, and /section[1]/section[3] reported 7/0.4286 instead of the ADR's worked example of 6/0.5. Skip a block that is itself a section unit when tallying block_counts, and pin the corrected literal values in tests/test_statistics.py so this can't regress silently again. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…aude/m2-144-golden
The sample pair has been frozen since #108 as four block trees: what M1 reads. What M2 makes of the difference between the two versions was left for this issue, and until now lived only in the eight named assertions of tests/test_sample_pair_change_tree.py. regenerate.py grows a second pairings table and writes expected/change_tree.{contract,markdown}.json, the whole JSON v2 document per twin -- both block trees, ten change nodes, the alignment and the statistics. Two files, not one, because the twins genuinely diverge at CHANGES.md change 6: markdown gets a row insert inside the deliverables table and the plain text, which has no table, an inserted paragraph. Written with include_alignment=True so a golden is a complete comparison that Comparison.from_dict can read back, and under an explicitly named difflib backend rather than the "auto" default, because auto resolves to rapidfuzz where the [fuzzy] extra is installed and the resolved name goes on the wire -- one golden has to be correct on both CI legs. The four block-tree goldens are byte-identical after this run. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The eight named tests say what CHANGES.md promises. Six new whole-tree tests per twin say that nothing else happened: exactly ten change nodes and no eleventh; the golden matched key by key and then byte for byte; its source and test sections still byte-for-byte BlockTree.to_dict, which ADR-0033 states as a conformance requirement and nothing checked until now; a round trip through Comparison.from_dict; determinism within a process; and difflib and rapidfuzz agreeing on every node, pair and statistic, which is what makes one golden under one named backend honest rather than convenient. The whole-tree tests compose the comparison through the public compare() rather than importing the regeneration script, so a drift between what the engine does and what the script wrote is a failure and not a silently regenerated golden -- the discipline tests/test_sample_pair.py already applies to the block trees. test_json_v2.py validates the stored goldens against the published schema as well as freshly built output: a hand-edited golden fails there rather than passing an equality check against itself. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
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.
What and why
PRD § 3a calls the sample-pair change tree "the first test written and the last one allowed to fail". The first half landed earlier in this stack: eight named tests in
tests/test_sample_pair_change_tree.py, one per row oftests/corpus/sample_pair/CHANGES.md, written as the specificationredlines.comparisonwas built against. This PR is the second half.tests/corpus/sample_pair/regenerate.pygrows a second pairings table and abuild_comparison/dump_comparisonpair beside the existingbuild_tree/dump, so one command still regenerates the whole directory — now six files rather than four:tests/corpus/sample_pair/expected/change_tree.contract.json(313 KB)tests/corpus/sample_pair/expected/change_tree.markdown.json(341 KB)Each is the whole JSON v2 document
Comparison.to_dict()emits: both block trees, the ten change nodes, the alignment and the statistics. The four block-tree goldens are byte-identical after the run.This branch carries a merge of
origin/claude/m2-138-consumers(filters, statistics, and the schema'sstatistics/changeFilterdefinitions), on top oforigin/claude/m2-143-metric. The merge was clean — no conflicts inpyproject.toml,README.mdor anywhere else — and the diff is clean once that branch is merged in order. Without it the goldens would carry nostatisticssection, which is half of what makes them worth freezing.The by-eye review: eight promises, ten nodes
Read against
tests/corpus/sample_pair/CHANGES.md, node by node. The markdown twin is quoted; the contract twin is identical except promise 6.modifyat/section[1]/section[2]/list_item[4],2.4 → 2.4,role: "definition", one inlineinsertsrc[133:133] → test[133:204]="confidential, or that the receiving party ought reasonably to treat as ".span_types: []— thedefined_termspan sits at the head of the definition, far from the edit, so a summary can say "a definition was widened" without claiming the defined term changed.move/section[1]/section[7]/list_item[5] → /section[1]/section[9]/list_item[6],7.5 → 9.6,matched_by: "move",confidence: 1.0,inline: [],source_text == test_textbyte for byte. The edit rides inmodify.../list_item[5]/paragraph[1] → .../list_item[6]/paragraph[1], onereplace"three " → "five "atsrc[44:50], carrying both addresses — the node ADR-0033 cites as the reason every node carries both. Itsmatched_byis"fuzzy"(confidence0.9333); the design refused to predict this one on paper and the golden records what the engine actually says.insertat/section[1]/section[3]/list_item[3], label3.3,matched_by: "unmatched",confidence: 0.0,span_types: ["party"]. Then tworenumbernodes,list_item[3] → list_item[4](3.3 → 3.4) andlist_item[4] → list_item[5](3.4 → 3.5), bothmatched_by: "exact",confidence: 1.0,inline: []. Three nodes, not three inserts, andexacton both is what proves ADR-0032's pass order held: the surviving clauses were matched on their text beforelabelcould pair old 3.3 with the newly inserted 3.3.modifyat/section[1]/section[9]/list_item[2],9.2 → 9.2, onereplacesrc[164:168] "3.3." → "3.4.", andspan_types: ["cross_reference"]— not["cross_reference", "party"]. The block carries twopartyspans that the op does not overlap; ADR-0033's "touched" reading is what keeps the one signal PRD § 3a promises here from being buried.deleteat/section[1]/section[5]/list_item[4]/list_item[3],source_label: "(c)",test_address: null,test_text: "",role: "sub_clause",source_textbeginning"The Client pays the undisputed part of the invoice on the due date.". (a) and (b) keep their addresses and produce nothing: exactly one node under/section[1]/section[5]/list_item[4].insertat/section[3]/list_item[3]/table[1]/row[5],block_kind: "row",role: "schedule", with its threecellchildren emitting nothing — topmost-wins by construction, which is Table alignment for markdown pipe tables: row insert/delete, cell-level inline diff #134's stated bar met without a table special case. contract:insertat/section[3]/list_item[3]/paragraph[5],block_kind: "paragraph",test_text: "The training day is due by Week 9 and is owned by the Client.". The two goldens differ in this node and in nothing else that CHANGES.md did not predict./section[1]/section[11], and the statistics row for that section readstotal: 0. The alignment shows why:("/section[1]/section[11]/list_item[5]", same)ismatched_by: "exact", confidence: 1.0. Both readers rejoined the hard wrap, so the change had disappeared before alignment ever saw it.modifyat/section[4]/list_item[3],3 → 3,role: "schedule",matched_by: "label", threereplaceops:"four " → "two "at 60,"two " → "one "at 109,"Days." → "Day."at 122. Three rather than two because_merge_ops_split_by_punctuationmerges only across punctuation-only equal runs and"Business "sits between them. Nothing else in/section[4]is reported, though seven near-identical siblings surround it — the ADR-0010 pathology, not reproduced.Also checked, and also correct by absence: the "Go-live sign-off" row slides
row[5] → row[6]and emits nothing, while the alignment carries it asmatched_by: "exact", moved: false. "An address shift alone is never a change" (ADR-0033), demonstrated rather than asserted.Two arithmetic checks on the rest of the payload. Totality: markdown 118 pairs + 5 inserted = 123 test blocks, 118 + 1 deleted = 119 source blocks; contract 101 + 2 = 103 and 101 + 1 = 102. Derived counts:
chars_added92 = 71 + 4 + 5 + 12 andchars_deleted24 = 4 + 6 + 14, both summing the inline ops above exactly, so the filter'smin_charsand the statistics agree by construction as D-4's resolution requires.New tests
Six whole-tree tests per twin in
tests/test_sample_pair_change_tree.py, beneath the eight named ones:(kind, source, test)list — the readable half of the golden, so an eleventh node names itself instead of printing 300 KB.config,changes,statistics,alignment,source,test) and then on the stored text, which is what catches a golden reformatted rather than regenerated.source/testare still byte-for-byte the M1 goldens. ADR-0033 states this as a conformance requirement; nothing checked it until now.Comparison.from_dict.tests/test_determinism.pycovers the hash-seed axis).tests/test_json_v2.pygains a test that validates the two goldens as stored againstcomparison-v2.json— the freshly-built-output tests there cannot catch a hand-edited golden.All of them compose the comparison through the public
compare()rather than importing the regeneration script, so a drift between engine and script is a failure and not a silently regenerated golden — the disciplinetests/test_sample_pair.pyalready applies to the block trees.Decisions taken where the research document was silent
include_alignment=True.Comparison.from_dictrefuses a payload without an alignment, so a golden without one could not be read back, and a golden that cannot round-trip is a weaker freeze than one that can. It also pins the correspondence set — the thing the benchmark scores and the thing the change tree structurally cannot express — which is where promise 7's proof actually lives. Cost is about 20 KB per file against 300 KB of block trees.difflibbackend, not the"auto"default.autoresolves torapidfuzzwhere the[fuzzy]extra is installed anddifflibwhere it is not, and the resolved name goes on the wire inconfig.similarityandalignment.backend— so a golden generated underautowould fail on whichever CI leg was not the one that wrote it (3.10 installs no extras, 3.11+ installs--all-extras).difflibis the floor PRD § 12 runs the site on and the backendbenchmark/REPORT.mdquotes. It costs no coverage, and test 6 above is what says so rather than leaving it as an assumption. Every threshold is left at the shipped default.CHANGES.mdwas updated, because its closing paragraph said the change tree "is M2's golden, not this directory's" and that is now false. It records the six files, the eight-promises-to-ten-nodes arithmetic, and where the per-promise tests live.Reinterpretation of the issue's wording
The issue says the golden shows "one of each thing the engine detects", listing eight items. The change tree has ten nodes, and that is not a discrepancy: change 3 is an insert plus the two renumbers it causes, and change 2 is a move plus the edit that rode along in its body — in both cases the extra node is the point of the row, not an artefact. Changes 7 and 8's promises are partly discharged by absence (no node at all for the whitespace rewrap; no spurious nodes among the seven repetitive siblings). Stated here so nobody reads "eight changes" against "ten nodes" and concludes the engine over-reported.
What was deliberately left out
strict=Truemakes the suite say so: the run reports7 xfailed, 0 xpassed. Four are the PRD § 10 gates intests/test_benchmark_gates.py— synthetic F1 0.9412 (< 0.95), synthetic move recall 0.7391 (< 0.90), hand F1 not assessable while its 644 rows arestatus: proposed, and 3 unreviewed engine moves on the hand set. Three are the M1-scope reader hard cases intests/test_text_reader.py, out of scope here. Removing any of them would have been weakening a test, not flipping a gate.ROADMAP.mdis untouched: the maintainer records milestone completion.benchmark/REPORT.mdorresults/latest.jsondiff.uv run python -m benchmark.runwas re-run after the consumers merge and rewrote both files identically;--checkreportsup to date. Nothing to commit rather than an empty commit.by_blockarray — ADR-0033 declines both for 1.0; the golden would otherwise have been the first thing to freeze them.M2 exit-criteria status
tests/test_sample_pair_change_tree.pyis 27 passed, 0 xfail.status: proposed(engine-seeded drafts, so the 1.0000 F1 measures self-agreement) and 3 engine moves have nomove_verdictsentry, which fails closed by design. Itsxfail(strict=True)tests stay on and will fail the day they are earned.benchmark/REPORT.mdandbenchmark/results/latest.json, both tiers, both backends, current as of this branch.redlines/schemas/comparison-v2.json, draft-07,schema_version: "2.0"(#137), and as of this PR the two committed goldens validate against it as stored.Test evidence
The 14 new tests are 12 in
tests/test_sample_pair_change_tree.py(six per twin) and 2 intests/test_json_v2.py. The 2 skips are the two extras-absent paths, skipped becauseuv sync --all-extras --devinstalls both (nupunkt is installed,the [fuzzy] extra is installed).Closes #144
🤖 Generated with Claude Code