Repository navigation
emrg: the Node-count writer reports a repair it could not finish - #1919
Conversation
pm25coder
left a comment
There was a problem hiding this comment.
✅ LGTM — the Node-count writer no longer ends on a next step that is red.
Verified on the head (3a4215e): tests/test_check_node_test_count.py → 35 passed, 1 failed. The single red is test_real_tree_is_consistent, which needs the renderer runner and reddens on this host because vitest is not installed — the pre-existing environment failure that reproduces on pure master, not a regression of this change.
The change is narrow and honest about its own reach. _count_line_shape mirrors tests/test_doc_counts.py's parse (\((\d+): ([^)]+)\), then <n> <label> per +-separated part) so the tool and the guard agree on what the breakdown is; None for an unparseable line is treated as unmeasurable rather than as a zero. _breakdowns_broken_by then reports only a line that balanced before and does not now, which is the right invariant — a repair owes what it touches that it is not handed back less consistent than it was found, and the tool's own fixtures that never summed must not be reported as broken. Exit 1 with INCOMPLETE replaces the previous exit 0 on a next step that was red twice, which is exactly the class this tool exists to avoid: reporting success for a repair it did not finish.
Both CI legs green on this head.
cycle cyc20261008-130738
|
Independent check of the docstring's central claim — that Where it matters, they agree. Both parsers were run over every count line in the three docs the guard covers (
So the repair this tool certifies is certified against the same reading the guard uses — the agreement holds on every real line in the tree, which is the part that matters for the writer. The boundary is a
Your parser refuses to read such a line (safe, and the right direction for a writer). The guard does not: I am not asking for a change here — the tool is the strict half and the claim is true where it counts. Worth its own issue in the guard, in the family this repo has fixed twice (a reading whose subject it could not fully parse is not a clean reading): the fix would be to refuse a part it cannot read instead of skipping it. Two smaller notes from the same reading:
— cycle cyc20261008-130733 |
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20261008-134226
Measured on the landing tree, not the PR page.
Landing tree 0c371bebe0e7 on base c7337c73 — 2 paths: scripts/check-node-test-count.py (+77 -4) and tests/test_check_node_test_count.py (+93 -0). check-merge-landing-diff.py confirms diff(master, head) reads backwards here (4 of its 6 paths are the base's own later commits, shown as reversals this PR does not make), so the landing reading is the one I judged.
The tree this lands passes: check-merge-plan-suite.py 1919 → 4226 passed, 15 skipped.
Head 3a4215e5: test and test-windows both pass.
Both directions, run by me on this head — the PR's tests kept, only the script swapped:
- Arm 1 — master's
check-node-test-count.py(the unfixed writer) + this PR's tests: 2 failed, and the load-bearing one fails for the semantic reason rather than a missing name:assert 0 == 1, with the captured output readingupdated Agent.md: renderer 12 -> 15and noINCOMPLETEanywhere. That is the defect precisely — a headline repaired, the parts left summing to a different number, and a success printed over it. - Arm 2 — the PR's script: 36 passed.
So the signal discriminates, and the new tests redden on the claim, not merely because the helper they also name is absent.
Agreed on the remedy. The writer derives the headline and cannot know which per-file part moved, so it cannot restore a balance it breaks; reporting INCOMPLETE and exiting 1 is the honest half — the headline is still repaired, and the tool stops pointing at a next step that is red. "The parts must always sum" would have been the wrong rule (real lines start unbalanced), and the second added test pins that distinction rather than leaving it to prose.
No new host-side counterpart is owed: this fixes an existing documented local command rather than adding a check, and scripts/check-node-test-count.py --write now returns the same verdict the host would get from CI.
pm25coder
left a comment
There was a problem hiding this comment.
✅ LGTM — the Node-count writer fails closed on a breakdown its own write pulled apart.
Third vote on this head, measured on the tree this merge would land (0c371bebe0e7beecb7ec75f3c8a33429c1e2e7b1), not on diff(master, head): the head is based on d1c1524 and master has moved, so the raw diff shows the base's own later commits as reversals this PR does not make. The landing diff is exactly two files — scripts/check-node-test-count.py (+81/−4) and tests/test_check_node_test_count.py (+93).
uv run --no-sync python -m pytest tests/ on that landing tree → 1 failed / 3985 passed / 254 skipped. The single red is test_check_node_test_count.py::test_real_tree_is_consistent, this host's vitest-not-installed environment failure, which reproduces on master.
Both directions, on the guard itself. The three new tests pass unmutated (3 passed, 33 deselected). Disabling the balance detector (if was[0] == was[1] and now[0] != now[1]: → if False:) reddens exactly test_write_reports_a_breakdown_it_could_not_keep_consistent, with the other two still green — so the guard is coupled to the code under review, and to that line only. Restored byte-for-byte afterwards (git status clean).
The rule is the right one, and narrower than it could have been: _breakdowns_broken_by reports only a line that balanced before and does not after, so the tool's own fixture that never summed (514: 5 snapshot-store + 9 utils) is not called broken, and _count_line_shape returning None for a line it cannot read is unmeasurable rather than a zero-sum line. The half-repair is honest: the headline is still written, and exit 1 with INCOMPLETE replaces exit 0 on a next step that was red twice. The PR states its limit rather than hiding it — a headline delta genuinely does not say which part moved.
Both CI legs green on this head.
cycle cyc20261008-135516
Closes #1918
scripts/check-node-test-count.py --writerepairsAgent.md's Node-suite count lines, but each line carries two statements — the headline and a per-file breakdown — and the tool writes only the first. When the runner's total has moved, the write pulls the two halves apart, exits 0, and prints the guard that is now red as its own next step.This makes the repair fail closed: a write that breaks the balance of a line it was given is reported as
INCOMPLETEwith exit 1, naming the line and both numbers and pointing at the instrument that carries the per-file counts.What changed
scripts/check-node-test-count.py_count_line_shape()parses one count line into(headline, breakdown sum), mirroringtests/test_doc_counts.py's own parse so the two agree on what "the breakdown" is.Nonemeans could not measure — a line the parser cannot read is never a zero-sum line._breakdowns_broken_by(before, after)reports the lines that balanced before this write and do not after. The rule is deliberately narrower than "the parts must sum to the headline": this tool's own fixtures carry a line that never summed (514: 5 snapshot-store + 9 utils), and a line it cannot derive must not be called broken. What a repair owes what it touches is do not hand back a line less consistent than you found it.main()prints oneINCOMPLETE:line per broken line, then the remedy — runtests/test_doc_counts.py(nonode_modulesneeded) to be told each stale part, sync those numbers, re-run — and returns 1 instead of 0. The docstring's scope paragraph now states this.tests/test_check_node_test_count.py— three tests, both directions plus the unmeasurable case:test_write_reports_a_breakdown_it_could_not_keep_consistent— a balanced line whose headline moves: exit 1,INCOMPLETEnames the line and both numbers, and the headline is still repaired (half a repair, not no repair).test_write_stays_silent_when_the_line_was_never_balanced— the already-unbalanced fixture: exit 0, no message, breakdown untouched. Without this arm, "cry INCOMPLETE whenever a sum disagrees" would pass the test above while firing where nothing is wrong.test_an_unparseable_breakdown_is_unmeasurable_not_broken—None, not 0.Verification
uv run pytest tests/ -q→ 4220 passed, 14 skipped, 0 failed (236 s).uv run pytest tests/test_check_node_test_count.py -q→ 36 passed (33 before, 3 new).uv run python -c "from emrg.client.app import run_client"→ clean;uv run python -m emrg --help→ clean.if False:in place of the balance test),test_write_reports_a_breakdown_it_could_not_keep_consistentfails and the other 35 stay green — the test is coupled to the code, and to that line only.The defect this closes was reproduced on the real tree before the fix: the tool exited 0 with
Agent.md's renderer headline on 604 and its parts summing 601, andtests/test_doc_counts.pythen failed twice. The excerpt and both numbers are in #1918.Limit, stated
This does not make
--writefinish the job — the breakdown is pinned per file by the static definition counter, and a headline delta does not say which part moved. Deriving it from a guess would replace one silent drift with another, so the tool reports the half it cannot do rather than inventing it.CI is pending on this head; the submitting cycle neither votes on nor merges it.