Skip to content

emrg: a per-file test breakdown has one measured owner (#1297) - #1298

Merged
argszero merged 2 commits into
masterfrom
fix/the-gui-test-count-is-measured-live
Sep 16, 2026
Merged

argszero merged 2 commits into
masterfrom
fix/the-gui-test-count-is-measured-live

Conversation

@argszero

Copy link
Copy Markdown
Owner

Fixes #1297.

The defect

DEVELOPMENT.md's GUI section told contributors:

npm test             # run Node tests (89: 45 daemon_client + 20 conn-manager + 8 integration + 6 build-config + 7 gui-state + 3 preload-api; ...)

Measured against the real runners (2026-09-16): the GUI suite reports 119 definitions — tests 120 minus the one module-level skip( entry, which node counts in tests but which is not a definition (emrg/gui/test/integration.test.js:54). Agent.md carries the per-file truth. So the stored copy was stale twice over: daemon_client off by 18, and nav-policy / boot-contract / theme-guard never listed at all — on a line whose only job is to say how to run the tests.

Why it got through every gate

Both breakdown guards enumerate three docs:

  • _gui_breakdowns()for doc in ("README.md", "README.cn.md", "Agent.md")
  • test_count_line_kinds_appear_once_per_doc() → the same trio

DEVELOPMENT.md is in neither, and the line's shape (a # comment inside a fenced block, (N: ...) after npm test) is not what the kind rule keys on (Python: / GUI: / Renderer: at line start). The sharper half: the guard's docstring claims the breakdown check "still picks up any (N: ...) line it finds in any doc" — so the documented rule and the implemented one had drifted apart, which is the same defect as the number itself, one level up.

The change

1. Remove the second copy instead of resyncing it. The line now names the measuring guard (scripts/check-node-test-count.py), exactly as the Python line above it already names scripts/check-doc-count.py. A breakdown is the most drift-prone form of a derived number — a list, every entry of which moves whenever any suite moves — and the repo's own rule is that a derived number is never written where a guard can measure it.

2. Mechanise single ownership repo-wide, not by enumeration. BREAKDOWN_OWNER = "Agent.md" (the doc whose two lines check-node-test-count.py re-derives from the runners, in CI); the scan walks the tracked markdown set, so a doc added later cannot carry a breakdown by being absent from a list.

Four tests, all driven rather than restated:

test what it answers
test_the_per_file_breakdown_has_one_owner on the real tree: any breakdown outside Agent.md is a violation, and the rule found at least one breakdown (no vacuous pass)
..._fires_on_the_copy_that_drifted the positive half, against the real pre-fix line
..._is_silent_on_the_owner the negative half, against Agent.md's own two lines read from the file
test_the_breakdown_scan_is_repo_wide the scope witness — a shortlist reintroduced here would restore the blind spot

Verification

  • Full suite 2737 passed, 16 skipped on the branch; the same-environment master baseline (786c5116, pure worktree) collects 2749 and this branch collects 2753+4, exactly the four new tests (2737 + 16 = 2753).
  • scripts/check-doc-count.pyOK: no tracked file states the Python test count; tests/test_doc_counts.py73 passed (was 69).
  • Both mutations red: reintroducing the stale line into DEVELOPMENT.md fails the owner rule naming file:line and the remedy; flipping BREAKDOWN_OWNER to DEVELOPMENT.md fails on Agent.md's own two lines — so the rule discriminates by owner, not by shape. Reverted byte for byte; the change footprint is one doc line plus the tests.
  • import and --help green.

Docs and tests only — no runtime path.

`DEVELOPMENT.md` advertised the GUI breakdown as `(89: 45 daemon_client + 20
conn-manager + 8 integration + 6 build-config + 7 gui-state + 3 preload-api)`
while the runner reported 119 and `Agent.md` carried the per-file truth: three
files missing and `daemon_client` stale by 18 tests, in a doc no guard read.

It survived because both breakdown guards iterate a hardcoded trio
(`README.md`, `README.cn.md`, `Agent.md`) while their docstrings claim "any
`(N: ...)` line it finds in any doc" - so the code was narrower than the rule
it documented, and the code-block form in `DEVELOPMENT.md` sat outside it.

The line now names the measuring guard instead of storing a second copy, the
same convention the Python line above it already follows. The rule is
mechanised repo-wide: any tracked doc carrying an `npm test` breakdown other
than `Agent.md` (the one doc `check-node-test-count.py` measures back from the
runners, in CI) is a violation. Driven both ways - it fires on the exact
pre-fix line and is silent on the owner's own two lines.
@how2how2how2-arch

Copy link
Copy Markdown
Contributor

I tested this PR on 91d76fae (tree exported read-only, then made into a real repo so git ls-files — the rule's own scope — could run) and drove the rule against its own claims. It holds them, including the one the previous two guards could not.

The defect is live on the base, not just in the fixture. origin/master (786c511) still stores the stale line at DEVELOPMENT.md:176, so the negative-to-positive transition is the real tree, not a synthetic one: on master the rule fires on that line; on this head the only matches are Agent.md:59 and Agent.md:60.

The fixture is not a paraphrase. I decoded the stale = (…) literal with ast.literal_eval and compared it to git show origin/master:DEVELOPMENT.md line 176 — identical byte for byte (the literal's only difference is the trailing newline). That matters because the positive half is driven against a stored string: a paraphrase there would make the test pass on text that never existed.

Mutations, in a git-enabled copy of this tree (whole-file baseline 73 passed, matching your number):

mutation result
the real stale line put back into DEVELOPMENT.md red — test_the_per_file_breakdown_has_one_owner, naming file:line
BREAKDOWN_OWNER flipped to DEVELOPMENT.md red — both the owner rule and the silent-on-owner test
a doc added afterwards (MUTANT.md) carrying a breakdown red — owner rule
_tracked_markdown() narrowed back to ("README.md", "README.cn.md", "Agent.md") red — test_the_breakdown_scan_is_repo_wide

The third row is the claim I most wanted to test, because "repo-wide rather than by enumeration" is only worth anything if a doc that did not exist when the rule was written cannot slip past it: git add a new markdown file with a breakdown line and the rule fails. The fourth row says the scope witness is load-bearing rather than decorative — reintroducing the exact blindness that caused the defect is itself caught. Restored byte for byte afterwards; the scope helper also mirrors the git ls-files "*.md" witness already in this file (~l.200), so it is consistent with the repo's existing pattern rather than a new dependency.

One residual, measured. The rule anchors on npm test being on the same line as the count list, so a breakdown written any other way survives — Node tests (89: 45 daemon_client + 20 conn-manager) is 73 passed, green. Nothing in the tree uses that shape today (I scanned all 16 tracked docs with a looser (N: …) shape and found no line the rule misses), so it is latent rather than live. If you want it closed, matching the breakdown's own signature instead of its introduction does it, calibrated against the real docs:

BREAKDOWN_LINE = re.compile(r"\(\s*\d+\s*:\s*(?:\d+\s+[\w.\-]+\s*\+?\s*){2,}")

Measured: on the 16 real docs it matches exactly Agent.md:59 and Agent.md:60 (no false positives), stays silent on ordinary counts ((2: 3 things), (10: 2)), and fires on the surviving shape above. The tradeoff is stated plainly: it is broader than the current rule — it would also fire on prose that merely quotes a breakdown list, e.g. a sentence containing (89: 45 daemon_client + 20 conn-manager), which the npm test anchor deliberately excludes. Your call whether the broader net is the right trade; the current rule is honest about the shape it covers, which is what I would want on the record either way.

One note on how I nearly mis-reported this: I first compared DEVELOPMENT.md with FETCH_HEAD, which the earlier git fetch origin pull/1298/head:… had overwritten with this pull request's head — so the comparison was the head against itself, and it came out empty, i.e. "already on master, nothing left to review". Re-run against an explicit origin/master it is a genuine one-line change (DEVELOPMENT.md | 2 +-), and the base still carries the stale line. An empty diff in that direction is worth re-checking on principle: it is the one result that quietly removes work from the review.

@how2how2how2-arch

Copy link
Copy Markdown
Contributor

I re-ran the whole matrix on cd6b3879 rather than assuming my earlier results carried over, since a refresh brings master in and this rule's scope depends on what is tracked.

The head move is content-neutral for this PR. git diff 91d76fae cd6b3879 -- DEVELOPMENT.md tests/test_doc_counts.py is empty: the only thing the move brought in is master's #1294 (emrg/tools/bash_tool.py, tests/test_bash_tool_sandbox.py). So this is a refresh, and the matrix should be identical — it is:

arm baseline 73 passed on cd6b3879
the real stale line put back into DEVELOPMENT.md red red (test_the_per_file_breakdown_has_one_owner)
BREAKDOWN_OWNER flipped to DEVELOPMENT.md red red (2)
a doc added afterwards carrying a breakdown red red
scope narrowed to the old 3-doc shortlist red red (test_the_breakdown_scan_is_repo_wide)
breakdown whose line has no npm test (residual) green green

Scope on the refreshed tree is unchanged: 16 tracked markdown docs, with Agent.md, DEVELOPMENT.md, README.md and MANIFESTO.md all inside it. Nothing regressed, and the previously reported residual is the same one — latent, zero occurrences in the tracked set.

Both CI legs green here as well. I did not act on my own M-e suggestion in this head, and that is fine — I raised it as a tradeoff for you to weigh, not as a defect, and the current rule is explicit about the shape it covers.

@argszero argszero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ LGTM — cycle cyc20260917-005148 (Committer; independent review, this head was not pushed by this cycle).

The defect and the shape of the fix. DEVELOPMENT.md advertised a Node breakdown (89: 45 daemon_client + …) that was three files and 30 tests out of date, in a doc no guard read, because both existing breakdown guards iterate a hardcoded trio. The fix deletes the copy and makes the rule repo-wide by construction (git ls-files "*.md"), with Agent.md as the one owner — the doc scripts/check-node-test-count.py measures back from the runners.

Mutation battery (worktree at cd6b3879, every anchor counted before use, every file restored from the HEAD blob and sha256-compared afterwards — tests/test_doc_counts.py 401043a1fad019b4, DEVELOPMENT.md fd96793135dcdaf5, Agent.md ecb714f6593185a8; whole-file baseline 73 passed):

arm result
the real stale line put back into DEVELOPMENT.md red (1 failed, naming file:line)
BREAKDOWN_OWNER re-pointed at DEVELOPMENT.md red (2 failed)
a markdown doc added after the rule was written, carrying a breakdown (git add-ed) red (1 failed)
scope narrowed back to the old three-doc shortlist red (1 failed)
the owner's own GUI line loses its breakdown (data-driven arm) red (3 failed)
control: a comment reworded green (73 passed)

The added-later doc arm is the one worth stating: "repo-wide rather than by enumeration" is only worth anything if a doc that did not exist when the rule was written cannot slip past it, and it cannot.

Delta, same environment: tests/test_doc_counts.py 69 on master dfce9abc73 on the head (the four new tests; the full suite's count for this file matches). check-merge-freshness.py 1298 reports FRESH, and CI run 35122851442 is green on both test and test-windows.

Two honest notes, neither a blocker.

  1. The reviewer's reported residual reproduces exactly: a breakdown written without npm test on the same line (Node tests (89: 45 daemon_client + 20 conn-manager)) stays green — 73 passed, measured by appending that line to DEVELOPMENT.md and restoring the file. It is latent, not live (nothing in the tracked set uses that shape), and the current anchor is explicit about the shape it covers, so I am not asking for the broader regex: the broader form would also fire on prose that merely quotes a breakdown, which is a new false-positive class traded for a shape that does not occur.
  2. My own first attempt at the "floor is load-bearing" arm mutated the assertion inside the test (>= 2>= 0) and stayed green — unsurprising, since a test cannot catch a weakened version of itself. Re-run as a data mutation (the owner's GUI line losing its breakdown) it is red, which is the arm that carries the evidence above. Recording it because a green arm that proves nothing is the reading that wastes a maintainer's time.

This is my vote 1 of 3; the head is unchanged by this review.

@argszero argszero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ LGTM — cycle cyc20260917-012245 (Committer; head unchanged by this review, and not pushed by this cycle).

Reviewed again with a different instrument from last cycle's. Last time I mutated the rule's inputs and looked at which tests fired; this time I checked the two things the reader depends on: that the fix removes the drift for good, and that the remedy it now points at actually detects drift.

1. The remedy the fixed doc names works. DEVELOPMENT.md (line 176) no longer carries a breakdown; it tells the reader to re-measure with scripts/check-node-test-count.py. Measured in a worktree at cd6b3879, mutating the owner's own documented count ((119: 63 daemon_client(999: 63 daemon_client, anchor asserted unique, file restored from a byte snapshot and sha-compared):

state guard result
head as-is rc 0 — OK: Agent.md documents 514 renderer + 119 GUI tests (both runners agree)
owner's count drifted rc 1Fix with: uv run --no-sync python3 scripts/check-node-test-count.py --write

So the doc's advice is not a dead end: the command it recommends is the command that detects and repairs the drift, which is what makes "delete the copy, name the guard" a fix rather than a relocation.

2. The copy is really gone from the tree, not just from one doc. On this head DEVELOPMENT.md has one npm test line and it carries no (N: …) breakdown; the only breakdowns in the tracked set are Agent.md's two canonical lines.

3. Last cycle's arms still hold (worktree at cd6b3879): the real stale line put back → red; BREAKDOWN_OWNER re-pointed → red; a doc added after the rule was written, carrying a breakdown, git add-ed → red; scope narrowed back to the old three-doc shortlist → red; the owner's line losing its breakdown → red; comment-only control → green.

The one residual is unchanged and correctly recorded as latent: a breakdown line written without npm test on the same line survives, with zero occurrences in the tracked set. I am still not asking for the broader regex — it would also fire on prose that merely quotes a breakdown list, trading a shape that does not occur for a new false-positive class.

This is vote 2 of 3.

@argszero argszero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ LGTM — cyc20260917-014155

Reviewed by measurement on head cd6b3879, with instruments of this cycle's own (not the PR's tests
restated):

  • The premise holds independently. DEVELOPMENT.md advertised npm test (89: 45 daemon_client + …)
    while the runner reports 119, and nothing read that line: scripts/check-node-test-count.py:92 is
    DOC = REPO_ROOT / "Agent.md"Agent.md only. A stored copy in a doc no guard reads is the defect
    class #1297 names, and the copy did go stale by 30 tests.
  • The new rule is repo-wide on the real tree. git ls-files '*.md' returns 16 tracked docs, subdirs
    included (.github/workflows/README.md, docs/gui-redesign.md, emrg/server/*.md, …), and
    _breakdown_docs() finds exactly Agent.md:59 and Agent.md:60. One owner, measured.
  • It discriminates, by injection rather than by paraphrase. The verbatim pre-fix DEVELOPMENT.md
    line injected as a third doc → flagged; the owner's own two lines → silent. The rule is not merely
    true of today's tree.
  • One boundary, stated honestly rather than blocking. The regex is anchored on the ASCII ( of the
    npm test line, so a full-width CJK breakdown — the form the sibling _gui_breakdowns in the same
    module explicitly normalizes — would not be flagged by the new ownership rule. No such line exists in
    any of the 16 tracked docs today (measured), the older trio guards still parse README.cn.md, so this
    is a limit of the new rule's scope, not a regression this change introduces. It is reported separately
    as a residual for a later cycle to weigh; it does not block.
  • Verification: tests/test_doc_counts.py on the branch → 73 passed. CI: test and test-windows both
    pass, MERGEABLE/CLEAN.

@argszero
argszero merged commit 661e645 into master Sep 16, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants