fix(doctor): the rows nothing asserted, and the parser that hid them - #196
Merged
Conversation
Follows #195, which pinned one doctor row — `vector-generation.open` — after finding that the ACTION it recommends was tested and the row itself was not. That looked like one oversight. Measuring it turned up three layers. **The measurement.** A new detector (C8 in `verification-audit.mjs`) lists every `createCheck`/`createInfo` id in `doctor.ts` and asks whether it appears anywhere in test code. Twelve of twenty-four appeared nowhere. **Layer two: six of those twelve were the detector lying.** `stripComments` in `scripts/lib/reference-corpus.mjs` — shared by C3, C6 and now C8 — removed comments with two regexes, and the block-comment one could not tell a real opener from the same two characters inside a line comment or a string. `tests/core/doctor.test.ts` carries the tsconfig glob `**` + opener + `.test.ts` inside a `//` comment, as prose. The regex opened a block there and closed it at the next real terminator hundreds of lines below, blanking every line between: fourteen openers against thirteen closers in that one file. Every id in that hole read as referenced by nothing. Replaced with a single-pass scanner that tracks code / string / comment state; C8 went 12 → 7, matching an independent hand count of the same file. **Layer one: the remaining seven were real.** Now pinned: - `hooks-config` — a FAIL row about an install whose hooks cannot load, with three distinct failure codes and none of them asserted - `llm_probe` — including the NOT-VERIFIED branch, whose entire purpose is that an expired key and a working setup must not read the same - `install-channel`, `capabilities`, `transcript-mining`, `install_id` - `node-runtime` had six tests that asserted what the row SAYS and never what it IS; the id is what the dashboard and the i18n catalogue key on, so renaming it would have broken both with the suite green **A defect found while pinning them.** `install_id` was `createCheck(..., 'pass', ...)` with no branch that could fail, so it rendered as `[PASS]` and counted toward `Overall` — the exact case the `informational` flag exists for, whose docstring names the Capabilities row as the instance that was fixed. This one was left behind. Now `createInfo`. No verdict moves (the row was always `pass`); what changes is that "N/N PASS" stops counting a row that verified nothing. In CHANGELOG under `[Unreleased]`. **Layer three: the detector had the same shape it was written to catch.** `denominator=24` was unguarded, so reformatting `doctor.ts` until the id-extraction missed half the call sites would print `denominator=12 hits=0` and exit 0 — clean-looking coverage over twelve unexamined rows. A call site whose id cannot be read is now its own hit. Break-tested in an isolated worktree, four mutants: comment stripper back to the two regexes KILLED install_id back to a hardcoded pass KILLED hooks-config stops noticing a missing hook type KILLED llm_probe probes without --probe KILLED The first SURVIVED on the initial run: the fixture built the glob from a closing sequence instead of an opening one, producing a string that contained no opener at all, which both stripper versions handled identically. The test proved nothing until the mutation showed it. Fixture fixed, and the reason is written beside it. Verification, in a clean worktree at 3b921a2 + this change — isolated because the main working tree carries an unrelated in-progress edit that would have moved the numbers: npm run typecheck exit=0 node scripts/run-tests-isolated.mjs 158 files, 2335 tests passed, exit=0 npm run build exit=0 (dist/core/doctor.* regenerated and committed here — the release gate caught them stale, and plugin-marketplace installs run dist/ as committed) node scripts/audit/verification-audit.mjs exit=0 `verify:release` is not quoted as green: its last run in this tree was the one that FAILED on the stale dist, and the rebuild has not been put back through it. CI runs it on this branch.
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.
Summary
#195 pinned one
doctorrow after finding its diagnostic untested while the action it recommends was tested. This measures whether that was one oversight, and it was not — it was three layers. A new detector (C8) lists everycreateCheck/createInfoid indoctor.tsand asks whether any test names it: twelve of twenty-four appeared nowhere. Six of those twelve turned out to be the audit's own comment-stripper corrupting the corpus; seven were real (one overlaps); one row had an implementation defect found while pinning it.Type of change
feat)fix) — the comment stripper, and theinstall_idrow's severityrefactor)docs)test) — the seven unpinned rowsrelease)What changed
1. The audit's comment stripper was corrupting its own corpus.
stripComments(scripts/lib/reference-corpus.mjs, shared by C3, C6 and now C8) used two regexes, and the block-comment one could not tell a real opener from the same two characters inside a line comment or a string.tests/core/doctor.test.tscarries a tsconfig glob inside a//comment, as prose; the regex opened a block there and closed it at the next real terminator hundreds of lines below — fourteen openers against thirteen closers in that one file — blanking everything between. Replaced with a single-pass scanner tracking code / string / comment state. C8 went 12 → 7 hits, matching an independent hand count.2. Seven doctor rows had no assertion anywhere. Now pinned, in
tests/core/doctor.test.ts(+ one case innode-runtime-check.test.ts):hooks-configllm_probeinstall-channelcapabilitiestranscript-mininginstall_idnode-runtime3.
install_idwas a check that could not fail.createCheck(..., 'pass', ...)with no failing branch, so it rendered[PASS]and counted towardOverall— the exact caseDoctorCheck.informationalexists for, whose docstring names the Capabilities row as the instance that was fixed. This one was left behind. NowcreateInfo. No verdict changes (the row was alwayspass); what changes is that "N/N PASS" stops counting a row that verified nothing. CHANGELOG entry under[Unreleased].4. C8 had the shape it was written to catch.
denominator=24was unguarded: reformatdoctor.tsuntil id-extraction misses half the call sites and it printsdenominator=12 hits=0, exit 0 — clean-looking coverage over twelve unexamined rows. A call site whose id cannot be read is now its own hit. (Verified in passing: the first version of that guard produced four hits, all of them prose mentioningcreateCheck(— including the comment I had just written next toinstall_id.)Verification
Run in an isolated worktree at
3b921a2a+ this change, so the numbers are not affected by unrelated edits in the main working tree:npx tsc --noEmitclean (vianpm run typecheck)npm run buildclean — required here: the release gate caughtdist/core/doctor.*stale after theinstall_idchange, and plugin-marketplace installs rundist/as committed rather than building. The regenerated files are in this commit.npm run verify:release— not quoted as green. Its only run in this tree is the one that FAILED on the stale dist; the rebuild has not been put back through it. CI runs it on this branch.npm test -- --runpassing — asscripts/run-tests-isolated.mjs(throwaway HOME)llm_probe's tests injectprobeProviderImpland assert zero live calls without--probeBreak-test, four mutants, isolated worktree, restoration by writing the original string back (never
git checkout):install_idback to a hardcoded passhooks-configstops noticing a missing hook typellm_probeprobes without--probeThe first one SURVIVED on the initial run. The fixture built the glob from a closing sequence instead of an opening one, producing a string with no opener in it at all — which both stripper versions handled identically. The test was green and proved nothing; only the mutation showed it. Fixed, with the reason written beside the fixture.
Test plan
npx vitest run tests/core/doctor.test.ts tests/core/node-runtime-check.test.ts tests/reference-corpus.test.tsnode scripts/audit/verification-audit.mjs—C8: denominator=24 hits=0stripJsCommentsto the two regexes → the glob-in-a-line-comment case goes redmemesh doctor— the Install ID row now prints[INFO], not[PASS]Known limitations / follow-ups
vector_generationitself passed that bar for months while only ever being seeded. It finds rows nobody has named; reading the hit is still the job. Written down in the detector./pattern/containing a quote can still confuse the string states. Same failure direction as before (over-strip, which fails loudly), and doing it properly needs the preceding-token analysis a real lexer does.doctor.tsis covered. The same "the fix is tested, the diagnostic is not" shape can exist wherever code emits user-facing findings; nothing here measures that.