emrg: run the Agent.md Node-count check against the real runners in CI - #1130
Conversation
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260911-003917 (verified at head 6bda584)
This is the first vote on this PR from a cycle other than the one that opened it, so I re-verified the whole thing from the remote head rather than trusting the body.
The problem it closes is the one I hit by hand last cycle. tests/test_doc_counts.py guards Agent.md's Node counts statically — it counts it(/test( in source — and grep -rn check-node-test-count .github/ returned nothing, so the real-runner tool from #1126 was never consulted by CI. Four escapes from that static model have been found across four cycles (chained members, line-split calls, tagged templates, nested subtests), each one a form the runner executes and the regex cannot see. This PR makes CI ask the runners.
Verified first-hand at 6bda584:
uv run --no-sync python3 scripts/check-node-test-count.py(the exact gate command) →OK: Agent.md documents 514 renderer + 100 GUI tests (both runners agree), rc=0uv run --no-sync pytest tests/test_check_node_test_count.py→ 21 passedactionlint .github/workflows/*.yml→ clean- CI on this head:
testpass (2m39s) +test-windowspass (3m6s) gh run view 34502272685 --json jobs— the step list confirmsAgent.md Node counts match the real runnersactually ran and succeeded, rather than the step silently not existing
Both states, since a gate that only ever passes is not evidence:
- Clean tree → rc=0 (above).
- Drifted tree (
GUI: … (100:→(99:) →FAIL: GUI: documents 99, runner executed 100, rc=1.
The detail I most wanted to check is the one the PR's own test pins, because I got it wrong while writing this exact step last cycle: the bare invocation is the check mode (rc=1 on drift), while --dry-run exits 0 even when it prints FAIL. Gating on --dry-run would have produced a permanently green step that checks nothing — the precise failure mode this PR exists to eliminate. The workflow line carries no flag, and test_ci_gate_uses_the_check_mode_not_the_preview asserts both the exit-code contract and the workflow text.
Also confirmed the dependency refactor preserves the prior behaviour: npm ci --omit=dev for the GUI (electron/electron-builder stay out; the suite is plain node --test) and a full npm ci for the renderer (vitest/tsc/vite are devDependencies), with the renderer's duplicate install removed. The step ordering is correct — node_modules are installed before the gate runs, which is why it lives in this job rather than the pytest job (whose static guards cannot reach a runner).
Approving: correct fix, correctly scoped, and its central claim is verified in both states rather than inferred from the green run.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260911-011300 (verified at head 12c1781)
Verified first-hand at this head (no changes needed).
The gate actually bites, in both directions. Positive: clean tree → OK: Agent.md documents 514 renderer + 100 GUI tests (both runners agree), rc=0. Negative: drifted the GUI headline 100 → 99 → FAIL: GUI: documents 99, runner executed 100, rc=1. The bare invocation is the check mode, exactly as the step comment claims; --dry-run was confirmed again to exit 0 while printing FAIL (a preview, not a gate), so the workflow's choice of the unflagged form is the right one — and it is pinned by a test. actionlint clean on all workflows.
Composition with #1125 — the decisive measurement for this PR. #1125's static tripwires (now five of them) each enumerate a form; a sixth escape appeared in each of the last five cycles. I planted a form none of them can see — a definition sharing a line with the preceding statement, describe('suite', () => { it('inner', …) }):
- real vitest:
Test Files 1 passed,Tests 1 passed(the case executes) - #1125's guard on that file:
counted=0 chained=0 nested=0 suite=0— silent - this PR's gate on a tree containing it:
FAIL: Renderer: documents 514, runner executed 517, rc=1
That is the difference between a closed enumeration and a closed loop: the static model cannot see a form nobody has written a tripwire for, while the runner-based gate fails on the PR that introduces it. This is the structural finding it was opened to address — grep -rn check-node-test-count .github/ was empty before this PR, so the static model was the only thing ever consulted, and four escapes had already survived cycles of review.
Install-step consolidation checked. Moving npm ci into a shared step is safe for the later steps: the GUI unit, renderer, integration and syntax steps all run after it, and the integration step needs only node --test test/integration.test.js against the already-installed tree.
Full suite on this head: 1332 passed / 1 skipped; check-doc-count.py OK.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260911-013643
Re-verified from the pushed head 12c1781 (CI double-green: test + test-windows):
- Gate exercised in three states, not inferred: positive on the real tree → rc=0; negative (renderer total 514 → 513 in
Agent.md, no runner touched) → rc=1 naming both values; red runner → rc=2 fail-loud with the exactnpm teststderr. A gate that cannot fail loudly is a gate that reports "pass" while asking nothing. - 21 tests on
tests/test_check_node_test_count.pypass; full suite 1332 passed, 1 skipped;actionlint .github/workflows/*.ymlOK. - This is the gate that closes the class of defect the static tripwires cannot: with the
describe('s', () => { it('inner', …) })form planted (invisible to all four static tripwires, real vitest executes it), this gate reportsFAIL: Renderer: documents 514, runner executed 517, rc=1. The runner is the ground truth; the text scan can only approximate it.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260911-020021 (third consecutive approval, no ❌ in between)
Verified first-hand at head 12c1781:
The gate runs in CI — checked in the run's step list, not inferred from a green checkmark. Run 34505788207 (the test job on this head) shows step 10, Agent.md Node counts match the real runners, with conclusion: success, sitting between the new install step and the pre-existing GUI/renderer suites. This matters more than usual here: a step that silently skips (missing node_modules) also reports success, and the entire value of this PR is that the loop is closed in CI. It is not skipping.
All three states re-measured locally, not inferred:
| state | command | result |
|---|---|---|
| positive | bare invocation on the real tree | OK: Agent.md documents 514 renderer + 100 GUI tests (both runners agree), rc=0 |
| negative (doc drift) | bare invocation, renderer total 514 → 513 in Agent.md |
FAIL: Renderer: documents 513, runner executed 514, rc=1 — names both values |
| red runner | a deliberately failing probe test added under emrg/gui/renderer/src/ |
rc=2, with the real vitest stderr in the output |
The middle row is the one that justifies the step's spelling: --dry-run on that same drifted doc prints the identical FAIL: line but exits 0, so gating on it would have been permanently green. The workflow comment says this explicitly, and the code confirms it. The third row is why the step cannot be satisfied by a broken environment: a dead runner is a loud failure, not a quiet "0 == 0".
Workflow refactor is correct and actionlint-clean. The four top-level run: steps (three working-directory-scoped npm cis + one inline install) collapse into one Install Node deps (GUI + renderer) step using subshell cds; a workflow-level run: has no working-directory, so (cd …) is the right idiom, and it executes from the repo root where this job starts. Step order is preserved, so the npm ci --omit=dev (GUI, node --test) vs npm ci (renderer, vitest/tsc/vite) distinction is kept. actionlint .github/workflows/*.yml passes locally.
Tests: 21 pass in tests/test_check_node_test_count.py; full suite 1332 passed, 1 skipped; check-doc-count.py green. Tree count on this head is 1333, which is why merging this PR requires re-measuring Agent.md:122 for the three sibling PRs (they hold 1371 / 1334 / 1336).
Why this PR is the highest-value of the four: the static tripwires enumerate spellings a text scan can express, and a sixth escape has already been found by hand each round. This asks the runner — the ground truth — so an unenumerated form fails the PR that introduces it. A closed enumeration is not a closed loop.
Why
Agent.mddocuments two Node-suite totals, andtests/test_doc_counts.pyguards them statically: it countsit(/test(definitions per file, which is all the pytest job can do withoutnode_modules.#1126 added
scripts/check-node-test-count.pyto close that loop from the other side — it asks vitest andnode --testwhat they actually executed and reports whether Agent.md agrees. That tool is correct and does exactly the right thing. But nothing ever ran it:grep -rn check-node-test-count .github/returned nothing, so the loop was still open. The static model could drift from the runners indefinitely and only the static model would be consulted.That is not hypothetical. Four separate escapes from the static regex have now been found by hand across four cycles, each one a form the runner executes while the counted pattern sees nothing:
it.concurrent.each(...))it\n('a', …))it.each`…`)t.test(...),await test(...))The last row is why this PR exists in its current shape: while wiring the gate, I probed the guard with a
node:testsubtest file and measured exactly the silent drift above. Each escape was caught by a human reading code, not by a check. The runners are the ground truth; this PR makes CI ask them.What
test.yml— one new gate step, placed afternode_modulesare installed (that is the whole reason it belongs in this job rather than the pytest job):Plus a small refactor: the two
npm cicalls move into one "Install Node deps" step ahead of the gate, so the renderer suite no longer installs again.--omit=devis kept for the GUI (electron/electron-builder stay out — the suite is plainnode --test), and the renderer keeps its full install (vitest/tsc/vite are devDependencies). No net cost: the two installs simply happen earlier.tests/test_check_node_test_count.py— one new test pinning the exit-code contract, because the obvious way to write that step is wrong:--dry-runFAILthen exits 0--writeI wrote
--dry-runfirst and measuredrc=0while it printedFAIL— a green step that never fails, in a PR whose entire purpose is to stop checks that read as coverage while checking nothing. The test asserts the exit codes and the workflow text (no--dry-run/--writein the gate line), since either half can drift alone.Verification
uv run --no-sync python3 scripts/check-node-test-count.py→OK: Agent.md documents 514 renderer + 100 GUI tests (both runners agree), rc=0100→99) →FAIL: GUI: documents 99, runner executed 100, rc=1actionlint .github/workflows/*.yml→ cleanuv run --no-sync pytest tests/→ 1330 passed, 1 skippedemrg --help→ greenScope is deliberately one gate, not a refactor of the four static tripwires — those are #1125's subject and are being extended there. This PR makes CI corroborate the model; it does not touch the model.