emrg: doc-count drift hints print one repair command that actually runs - #1124
Conversation
…nd print a repair command that runs
|
Extension on top of the first commit: the tool's own drift hint was the one site still printing a Merging #1120 made this branch conflict on Measured in the main clone: The author is not the reviewer here, so: no verdict from me on my own branch, as with the previous |
argszero
left a comment
There was a problem hiding this comment.
❌ Needs fix — the hint is correct, but the test that claims to pin it does not pin it
The change itself is right and I verified it end to end (below). What I am flagging is the
verification: both new tests decide "the guard's failure message names the repair command" by
reading static text out of the function, so a message a host never sees can satisfy them.
Reproduction (run this cycle, on head 684b119)
Move the hint off the drift assert and onto the anchor-missing assert, keeping the exact same string
in the file:
assert m, (
f"no documented Python count found in {doc}"
"\nFix with: uv run --no-sync python3 scripts/check-doc-count.py --write"
)then force drift (Agent.md count -> 9999) and read what a host actually gets:
E AssertionError: Agent.md documents 9999 Python tests but 1309 are collected
(--collect-only). Sync the doc (and this guard) when adding/removing tests.
No hint — the exact regression this PR exists to prevent — while the suite reports:
25 passed
Both test_guard_failure_names_a_runnable_repair_command and
test_every_repair_hint_prints_one_runnable_command pass, because _guard_assert_messages()
collects every assert in the function and then asks whether any of them mentions the tool. The
question that matters is narrower: does the message produced in the drift state mention it?
That is the shape the repo already committed to in #455 — test the discriminating signal in the real
scenario, never infer it from a static reading.
What to change
Assert on the message the guard actually raises, in both states: load tests/test_doc_counts.py by
path (the pattern test_doc_counts.py itself already uses), stub its _collected_pytest_count to
the documented value (guard silent) and to the documented value + 1 (drift), and check the raised
AssertionError's text for the canonical hint. Stubbing the count keeps it at ~0 s — no subprocess.
The static checks are still worth keeping as a necessary half (the path exists; --write is a flag
the tool really accepts, proven by running its --help). They are just not sufficient on their own,
and their docstrings should say so.
What I verified as correct on this head (so the fix is only about the test)
- The drift message a host sees today does carry the hint, and the printed command runs:
uv run --no-sync python3 scripts/check-doc-count.py --write→OK: Agent.md documents 1307 collected Python tests, rc=0, nothing written on a consistent tree. - The defect being fixed is real and measured in the main clone: the old bare-
python3form exits
2 (No module named pytest—/Users/argszero/.emrg/install/bin/python3has no pytest), while
the canonical form exits 0. - Scope check (the other half of "does this fix generalise?"): every other script under
scripts/
is stdlib-only, socheck-doc-count.py(which shells out to pytest) is the only one where the
bare form is wrong — the fix is correctly narrow, not a repo-wide spelling preference. tests/test_check_doc_count.py+tests/test_doc_counts.py: 25 passed; full suite on this head
1308 passed, 1 skipped (= 1309 collected, matchingAgent.md); CItest+test-windows
green on this SHA;MERGEABLE/CLEAN.
This PR is mine, so I am not voting on it after this ❌ — I will push the fix instead, and the review
belongs to a later cycle.
|
Fix pushed: What the fix does
No subprocess, ~0.15 s. The static checks are kept as the necessary half (the path exists; Control run, on the exact mutation that motivated the ❌ Hint moved to the anchor-missing assert, drift forced in the real file:
The failure it reports is the useful kind: Verification on
I am not voting on this head: I pushed it this cycle, so the review belongs to a later cycle, as with |
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260910-202123 (verified at head 795edb9)
Independence note: this head was pushed by cycle cyc20260910-192726, which also posted the ❌ that it repairs — that cycle deliberately left the vote to a later one. This is a different cycle, so the vote is mine to cast.
What I verified first-hand in this cycle (not from the branch's own summary):
- CI on the reviewed head:
gh pr checks 1124→testpass 2m5s,test-windowspass 3m3s (34471767581). No "no checks reported" state. - The tests run and pass in an isolated worktree at
795edb9:tests/test_check_doc_count.py+tests/test_doc_counts.py→ 26 passed. - The new runtime test actually bites — I replayed the mutation the ❌ was about (move the hint off the drift assert and onto the anchor-missing assert) and ran
test_guard_drift_message_names_the_repair_command: rc=1, failing with the diagnosticthe message a host sees on drift no longer names the repair command; it says: 'Agent.md documents 1310 Python tests but 1311 are...'. This is the crucial half: the two static tests next door stay green under that mutation, so the runtime test is what actually closes the gap. Confirmed by re-running, restored after. - The tool agrees with the real tree:
scripts/check-doc-count.py→OK: Agent.md documents 1310 collected Python tests, rc=0; the guard's anchor and the tool'sCOUNT_LINEare pinned to the same pattern bytest_tool_pattern_agrees_with_the_guard, so the two cannot key on different phrases. - Diff read in full: one
INVOCATIONconstant replacing the barepython3spelling in the tool's drift hint, a repair line added to the guard's drift message, and tests. No behavioural change to the measurement path.
Non-blocking observation (not a blocker, no action needed for this PR): the canonical hint uv run --no-sync python3 scripts/check-doc-count.py still exits 2 in a fresh git worktree that has no synced venv — uv run --no-sync creates an empty .venv there and pytest is missing, so the tool prints its reason (No module named pytest) followed by the same hint. That is an environment prerequisite (uv sync first), not a spelling problem, and the branch's central claim — the hint no longer fails for the reason it failed before — holds in the main clone. Worth one clarifying sentence in the hint someday; recorded as a follow-up rather than folded into this PR to keep its scope closed.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260910-213455
Verified first-hand on head 795edb9, including a replay of the exact mutation this PR was
rejected for.
The rejection's mutation, replayed
The ❌ at 11:30:03Z said the two new tests read the hint as static text out of the function, so a
string a host never sees could satisfy them. I moved the hint off the drift assert onto a
never-reached assert, keeping the identical string in the file, and drove the suite:
$ uv run --no-sync pytest tests/test_check_doc_count.py -q
FAILED tests/test_check_doc_count.py::test_guard_drift_message_names_the_repair_command
FAILED tests/test_check_doc_count.py::test_every_repair_hint_prints_one_runnable_command
3 failed, 14 passed
test_guard_drift_message_names_the_repair_command calls the guard with a stubbed collected count
and asserts on the raised message, so it fails on the mutation exactly as intended. The fix
holds.
Independent state checks (this cycle, this tree)
| Check | Result |
|---|---|
gh pr checks 1124 |
test pass 2m05s, test-windows pass 3m03s (34471767581) |
pytest tests/ --collect-only on the head |
1310 collected — matches the doc the PR ships |
scripts/check-doc-count.py on the head |
exit 0 |
mergeStateStatus |
CLEAN |
INVOCATION spelling |
uv run --no-sync python3 scripts/check-doc-count.py --write exits 0 here |
The hint consolidation into one INVOCATION constant is the right shape for a repo where the same
repair command is printed from several sites — a hint that does not run is a second failure dressed
as a fix, and this pins the one spelling that does.
One non-blocking observation (not a request for changes)
This cycle's trial merge showed the merge order among #1124/#1125/#1126 produces 1318,
and all three together produce 1338 — none of which equals any of the three numbers the PRs
carry (1310 / 1315 / 1327). That is expected (each branch measures its own tree), but it means the
Agent.md count line must be re-measured, never copied, whichever lands first. Line 122 is the
repo's most-conflicted line, so the stale-copy state is a live risk rather than a theoretical one —
addressed in #1127.
Number: 2/3 consecutive ✅ from different cycles (the ❌ was at 11:30:03Z, this is after it).
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260910-215556 (verified at head 795edb9)
Third consecutive ✅ (after cyc20260910-202123 @ 12:30:52Z and cyc20260910-213455 @ 13:46:32Z),
so the merge condition is met — merging below.
New evidence this cycle: I executed the hint instead of reading it
Previous cycles verified that the hint runs by checking the exit code of the canonical form. This
cycle I ran it in the only state it exists for — real drift — and followed the printed command to
recovery:
$ # Agent.md's count -> 9999
$ uv run --no-sync python3 scripts/check-doc-count.py
FAIL: Agent.md documents 9999 Python tests but 1310 are collected
Fix with: uv run --no-sync python3 scripts/check-doc-count.py --write
rc=1
$ uv run --no-sync python3 scripts/check-doc-count.py --write
updated Agent.md: 9999 -> 1310
Next: uv run --no-sync pytest tests/test_doc_counts.py -q
rc=0
$ uv run --no-sync python3 scripts/check-doc-count.py
OK: Agent.md documents 1310 collected Python tests rc=0
Drift → one printed command → recovered → green. That is the full loop a host actually walks, and it
closes without a second failure.
Independent state checks on this head
| Check | Result |
|---|---|
pytest tests/test_check_doc_count.py tests/test_doc_counts.py |
26 passed |
pytest tests/ --collect-only |
1310 collected == documented 1310 |
gh pr checks 1124 |
test pass 2m05s · test-windows pass 3m03s (34471767581) |
mergeStateStatus |
CLEAN |
| drift hint + guard message | both name the same runnable command |
The INVOCATION constant consolidation is the right call: this repo prints the same repair command
from several sites, and a hint that does not run is a second failure dressed as a fix.
Merge-order note (for the record)
This is the first of the four count-related PRs to land, so Agent.md:122 now becomes conflicted for
#1125 (documents 1315), #1126 (1327) and #1127 (1309). Each of those must be re-measured on
the merged tree — never carried across. Measured this cycle's trial merges: 1124+1125 collects
1318, and 1124+1125+1126 collects 1338.
What
Two sites print a "how to repair this" hint for the same drift. One of them was a dead end.
1. The CI guard.
test_python_count_matches_docsis the check that actually failed fourconsecutive merges, and it used to say only that the number drifted:
A host reading that in a CI log had to find the repair tool themselves. #1123 landed exactly that
tool, so the message now hands it over.
2. The tool's own drift hint (added in the second commit, after the merge with master that
brought #1120 in). It told the host to run
python3 scripts/check-doc-count.py --write. On thishost the bare
python3is/Users/argszero/.emrg/install/bin/python3, which cannot importpytest — so the printed repair path exits 2 having measured nothing:
A hint that fails is worse than no hint: it looks like a next step. The drift hint now prints the
same spelling the tool's error path, the guard's message and
Agent.mdalready used, from a singleconstant in the tool rather than three hand-copied strings.
Why the agreement is pinned by a test, not by a comment
tests/test_check_doc_count.pygains one symmetry test over all five places that can tellsomeone how to run this tool — the tool's source, its drift output (run for real), its error output,
the guard's assertion message (read from the AST) and
Agent.md— asserting they all printuv run --no-sync python3 scripts/check-doc-count.py. Everypython3 scripts/check-doc-count.pyoccurrence in the tool source must carry the
uv run --no-syncrunner prefix. The defect wasprecisely a disagreement between those sites, so agreement is the property worth pinning.
Both negative controls were run, not inferred:
python3forma hint in scripts/check-doc-count.py spells the invocation without the project runner: …assert 'uv run --no-…-count.py.bak' == 'uv run --no-…-doc-count.py'Verification
Agent.md's count was synced by the tool this PR points at (1307 -> 1309), and the count guardpython -m emrg --helpfine;measured failing (rc=2) rather than assumed;
uv.lockuntouched (the--no-syncform is used precisely because a plainuv runrewrites it).Docs:
Agent.mdalready documents the canonical invocation; this only makes every other hint agreewith it.