mirror of
https://github.com/zernie/vigiles.git
synced 2026-09-14 20:53:57 +08:00
zernie
b7458a6526
* fix(ci): delete the root test that read site/ instead of patching around it #219 was a site-only PR that deleted a file a ROOT test read off disk. The `changes` filter set root=false, the root jobs were SKIPPED, the PR merged GREEN and main broke on an ENOENT — a skipped job and a passing job are the same tick in the checks list, so nothing announced it. That was patched twice before anyone asked whether the test should exist. It should not. What it asserted: expect(hero).toMatch(/\bLANGUAGES\b[^;]*from ["']@\/lib\/linters["']/s); expect(heroCode).not.toMatch(/Any language/); Both are properties of the SITE — an import line and a piece of copy — checked by regexing the SOURCE TEXT of two site files from a test about the engine's linter registry. Three defects, only the third visible: it asserts the shape of the text rather than the value, so a rename or a reformat breaks it; a substring search cannot tell an assertion from a comment discussing one, and it had already fired on exactly that (its own comment says so); and it made root depend on site, which is the dependency that produced the green-merge-red-main above. Deleted. The property moves to where it belongs and gets stronger on the way: site/src/lib/linters.browser.test.ts now RECOMPUTES the derivation from the engine and compares values — expect(LINTER_NAMES).toEqual(BUILTIN_LINTERS.map((l) => LINTER_LABELS[l] ?? l)); expect(LANGUAGES).toEqual(LANGUAGE_ORDER.filter((lang) => …)); so a hand-typed array is no longer forbidden, it is made HARMLESS: it must equal what the engine implies and goes red the moment BUILTIN_LINTERS moves under it. That test runs in the `site` job, which is gated on site changes — the job that would have run for #219. With the read gone, the filter's own invariant is true again, and now it is CHECKED rather than asserted in a comment: src/ci-path-filter.test.ts fails if any root test reads site/ off disk. Mutation-proven — reinstating one read fails exactly that test. ⚠️ Two of my own errors on the way here, both mine and both worth the record. The first fix I wrote for this declared the cross-read in ci.yml instead of removing it — treating the symptom a third time. The second was in the guard itself: `grep -rl` exits 1 when it finds nothing and execFileSync throws on a non-zero exit, so the check went RED precisely when the property HELD. It now reads the exit code instead of letting it decide. Gates: npm run check 18/18. npm run coverage 3959 passed, 0 failed; the 99.94% statement gate is src/mock-model.ts:147,339,374, untouched here and green on CI (25 tests skip locally and never reach those lines). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(site): assert the derivation's properties, not a copy of its formula The two tests added in8f790e5compared each exported value to a retyped copy of its own definition: export const LINTER_NAMES = BUILTIN_LINTERS.map((l) => LINTER_LABELS[l] ?? l); expect(LINTER_NAMES).toEqual(BUILTIN_LINTERS.map((l) => LINTER_LABELS[l] ?? l)); That passes by construction, and anyone editing the formula edits the copy with it — the same defect the header of src/ci-path-filter.test.ts names: a test that agrees with its own copy of the rule proves nothing about the rule that runs. Replaced with properties that survive a refactor of the mapping and still fail on a wrong value: - every shipped linter is named exactly once, under its label (containment + count => the exact set; order deliberately not asserted) - LANGUAGES is EXACTLY the covered language set, not merely a subset of the display order — which is all the pre-existing test could see Net new coverage over what was already there: the CONTENTS of LINTER_NAMES (only its length was checked) and the reverse inclusion for LANGUAGES. site: 51 passed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(ci): close the second site cross-read, and the guard that could not see it Codex flagged that the guard added in8f790e5does not enforce its advertised invariant: `src/comparison-snapshot.test.ts` reads `site/src/comparison/validate-overlap.json` off disk and the pattern missed it. Verified — the file exists, the read is real, and the grep found nothing. TWO independent reasons it evaded, either enough on its own: 1. `grep` is LINE-based and prettier had split the call, so `resolve(` and the literal are never on one line: const SNAPSHOT = resolve( __dirname, "..", "site/src/comparison/validate-overlap.json", ); 2. the pattern encoded ONE SPELLING (`"../../site/`) — the path assembled from separate segments did not match it. Both are the same defect: a guard written from the spelling its author had just deleted rather than from the shape it forbids. The data moves instead of the dependency being declared. The snapshot is WRITTEN by a root tool (tools/measure-validate-overlap.mjs) and ASSERTED by a root test, so it now lives at the root; the site imports it through a new `@measured/validate-overlap.json` alias. The arrow points site -> root, which a site-only diff cannot break. The guard is Node, not grep: it matches the whole CALL across line breaks, so neither reason above can recur. It keys on a `site/` segment inside a string literal passed to resolve/join/read*, NOT on the mere mention — measured, not assumed: forbidding the mention fires on 3 legitimate fixtures today (the classifier is FED "site/src/App.tsx", it does not read it), and a guard with false positives gets switched off, which is how the first hole survived. | | | | --- | --- | | root reads under site/ | 1 -> 0 | | forms the guard sees | one spelling, single-line -> any call, any layout | | new fixture | the exact multi-line call the old guard missed | Mutation-proven: restoring the site/ path in comparison-snapshot.test.ts (patch landed, grep count 1) fails exactly `finds no disk read of site/ anywhere under src/`, and reverting restores green. Two of my own errors on the way, both caught by a run rather than by reading: dropping the `execFileSync` import broke `decide()` six tests above, and the first `SITE_LITERAL` anchored `^` to the start of the argument STRING instead of the position after the quote, so it never matched a literal that was not first in the call — its own fixture caught that one. Gates: npm run check 18/18 · coverage 3960 passed 0 failed · site build ok · site test:browser 51 passed. The 99.94% statement gate fails locally on src/mock-model.ts (untouched; 25 tests skip locally), green on CI. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(site): cover the hero chip's CONSUMER, not just the value it renders Codex: moving the language assertion into the site suite validated only the exported array, so a hand-typed chip — or `Hero` dropping the import entirely — now passes everything. Verified: NO site test renders `Hero`, and the root test this PR deleted did cover the consumer (it checked the import line and the absence of the old "Any language" copy). The consumer half was genuinely dropped; this restores it. Restored on the RENDERED OUTPUT rather than the source text, which is what the deleted test got wrong: it regexed `Hero.tsx` for an import line, so a rename or a reformat broke it and a substring could not tell an assertion from a comment discussing one — on top of making a root job read a site file (#219). `Hero` is NOT rendered whole, and that is measured, not squeamishness: it mounts `DemoAudit`, which on mount sweeps idb and issues one `fetchStars` request per featured chip. A test that reaches GitHub from a browser runner is a flake, and a flaky test is a disabled test. The chip is its own `HeroChip` component instead — the same move `Guard.tsx` already makes by exporting `BATTERY_ROWS`, for the same reason. Three assertions, each covering a defect the others miss: - every language the engine covers is rendered (with a non-empty guard, since a vacuous loop proves nothing) - the chip contains the derivation VERBATIM — containment alone passes a chip naming one extra language, or the right ones in a stale order - "Any language" never returns Mutation-proven: replacing `LANGUAGES.join(" · ")` with the old hand-typed "Any language · 11 linter catalogs" (patch landed, grep count 1) fails all three; reverting restores 54 passed. Gates: npm run check 18/18 · site build ok · site test:browser 54 passed (was 51). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(ci): derive the guard's scan roots from vitest.config, not from memory Codex, third finding in this same file: the scan walked `src/` only, while the root `unit` project also runs `scripts/**/*.test.ts` and `eslint-rules/**/*.test.ts`. A site read in either was invisible to the guard AND skipped by a site-only diff — exactly the hole this PR closes, in the part of the suite the guard could not see. Verified against vitest.config.mjs. Three findings in one file is the signal to stop patching and fix the root: all three were a SPELLING I remembered (one path form, then one directory list) where the rule was sitting in a config file, readable. So the roots are now EXTRACTED from the unit project's own `include` globs — the same move `patternFor` already makes for the classifier's patterns in ci.yml, and for the same reason: a copy of a rule cannot prove anything about the rule that runs, and it rots on the next glob someone adds. A restructured config now fails LOUD (a named error saying the guard can no longer see what it must cover) rather than walking nothing, and every derived root is checked to exist — a scan that quietly resolves to empty reports a clean repo forever. ⚠️ Deliberately NOT asserting `roots.length > 1`: narrowing the unit project back to `src/` alone is a legitimate change, and a guard that goes red on a legitimate change is a guard someone deletes. Mutation-proven twice, patch landed both times: - a `resolve(__dirname, "..", "site/…")` + readFileSync planted in `scripts/` fails the guard, which names that exact file (the old scan saw nothing) - renaming the config's `name: "unit"` fails with the named error rather than silently passing Gates: npm run check 18/18 · coverage 3960 passed, 0 failed. The 99.94% statement gate fails locally on src/mock-model.ts (untouched; 25 tests skip locally), green on CI. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(ci): decide whether the call resolves under site/, instead of matching spellings Codex, fourth finding, fourth SPELLING: `resolve(__dirname, "..", "site", "src/foo")` — `site` as its own argument — produced no offender, because the matcher required one literal to contain `site/`. Four rounds, four spellings, one cause. Widening the pattern a fourth time is patching a symptom of a check that looks at text which RESEMBLES a site path rather than deciding where the call lands. So the literal segments of the call are now JOINED IN ORDER and the joined path is judged: resolve(__dirname, "..", "site", "src/foo") -> ../site/src/foo ✗ resolve(__dirname, "../../site/src/x.ts") -> ../../site/src/x ✗ join(ROOT, "website", "index.html") -> website/index.html ✓ legal However an author breaks the path up, the join puts it back together, so the class is closed rather than one more member of it. The segment test is a PATH SEGMENT (`(?:^|/)site(?:/|$)`), not a substring, so `website/` stays legal. The single fixture became a TABLE of every spelling review has caught plus the shapes that must stay legal — a guard that goes red on `["site/src/App.tsx"]` (bare data the classifier is FED, not a read) is a guard someone deletes. HONEST LIMIT, stated in the file: an argument that is not a literal (`resolve(dir, name)`) is not decidable statically and is out of scope. It is a narrow gap — a test names the fixture it reads. Mutation-proven, patch landed: a file in `scripts/` using the newly-caught `"site", "src/…"` form fails the guard and is named in the output; removing it returns green. Gates: npm run check 18/18 · coverage 3967 passed, 0 failed (was 3960; +7 table rows). The 99.94% statement gate fails locally on src/mock-model.ts (untouched; 25 tests skip locally), green on CI. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(ci): cover every file the skipped jobs run, and imports as well as reads Two more from Codex, both real, both about this guard's own completeness rather than a new defect in the repo: 1. it walked only what the vitest `unit` project lists, and only `.ts`. The root jobs also run `test/runners` under vitest (`.mjs`) and under Jest (`.cjs`) — a site read in either was invisible AND skipped by a site-only diff. 2. it inspected CALLS only, so a static or dynamic import (`import snapshot from "../site/…json"`) depended on a site file without ever being an offender. For (1) the scan now takes EVERY test glob out of vitest.config.mjs and jest's testMatch, and uses BOTH halves of each: the leading directory says where to walk, the extension says what counts as a file there. Hard-coding either is what put the runner suites outside the scan — the third time in this file that a value was remembered while the config that owns it sat unread. For (2) module specifiers go through the SAME verdict function as call arguments; only the channel differs. Also fixed while widening: the glob regex spanned newlines, so `[^"]*` swallowed a 14-line comment and produced one absurd "glob" whose directory did not exist. The existing sanity assertion caught it — a directory that must exist and an extension that must be non-empty, per glob, so a mis-parse fails loud instead of walking nothing. Mutation-proven, patch landed each time: - `test/runners/__mutantA.vitest.mjs` reading a site file -> named as an offender (the previous scan took no `.mjs` anywhere) - `scripts/__mutantB.test.ts` importing a site JSON -> named as an offender (no matching call anywhere in it) - removing both -> green Gates: npm run check 18/18 · coverage 3967 passed, 0 failed. The 99.94% statement gate fails locally on src/mock-model.ts (untouched; 25 tests skip locally), green on CI. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * refactor(ci): parse the guard's inputs instead of pattern-matching their text Five rounds of review, five spellings this guard did not match — a call split across lines, `"site"` as its own argument, a static import, files whose extension the walk never took, and one where `[^"]*` ran through newlines and swallowed a 14-line comment as a "glob". Each fix was a wider pattern, i.e. the sixth waiting to be found. They are one defect, and this repo already names it: `parse-structured-input-with-a-real-parser` — the rule that routed five markdown detectors onto one markdown-it oracle for exactly this reason. The guard was regexing TypeScript. `typescript` is already a runtime dependency and `src/core/compile-generator.ts` already parses with it. TWO inputs, two corrections: - THE CONFIGS ARE NOW READ, NOT PARSED. They are modules; importing them yields the include globs as real arrays, so there is nothing to pattern-match. `unknown` is narrowed once at that boundary (parse, don't validate) and typed values go inward. - THE SOURCES ARE NOW PARSED. A call is a CallExpression however it is formatted; an import is its own node kind, found by asking rather than by adding an alternation. What the AST buys that no pattern could, now pinned as two table rows: a site path inside a COMMENT -> not a dependency a site path inside a STRING that is not a call -> not a dependency Which is why the self-exclusion is GONE. The text version had to skip this file, because its own fixture table looked like a violation to it. Also dropped: the computed-vs-literal import specifier is deliberate — a literal `.mjs` import is a tsc error here (TS7016, no declaration file), and shipping a .d.ts for a config would be ceremony around a value we only want to read. Mutation-proven, patch landed each time: a read planted in `scripts/` and a static import planted in `test/runners/*.mjs` are each named as offenders; removing both returns green. Gates: npm run check 18/18 · coverage 3972 passed, 0 failed. The 99.94% statement gate fails locally on src/mock-model.ts (untouched; 25 tests skip locally), green on CI. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(ci): discover the vitest config's filename instead of spelling it `vitest.config.mjs` is plain JS while `site/vitest.config.ts` is TypeScript. Measured: that is an inconsistency, not a constraint — renaming the root config to `.ts` typechecks and vitest picks it up unchanged. Settling it touches eight prose references including the compiled CLAUDE.md and a skill, so it is a separate change, NOT this one. What this change does is remove the landmine that separate change would hit: the guard no longer knows the extension. It finds `vitest.config.*` at the repo root and throws by name if there is none. Mutation-proven three ways: green on `.mjs`, green after renaming the config to `.ts`, and a loud named failure with no config present. Gates: npm run check 18/18 · coverage 3972 passed, 0 failed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(site): render the real Hero, not just the chip component Codex: the consumer test rendered `HeroChip` directly, so deleting `<HeroChip />` from `Hero` left every assertion green while the first screen showed no languages at all. That is the exact gap the test was added to close, reproduced one level down — the test proved the component works, not that the page uses it. `Hero` itself is rendered now, with `DemoAudit` stubbed. Stubbing the one heavy child is what makes it deterministic: on mount `DemoAudit` sweeps idb and issues one `fetchStars` request per featured chip, and a browser test that reaches GitHub is a flake — a flaky test is a disabled test. Mutation-proven, patch landed: replacing `<HeroChip />` with a comment fails all three assertions; restoring it returns 54 passed. The earlier version stayed green through that same mutation, which is the finding. Gates: npm run check 18/18 · site build ok · site test:browser 54 passed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude <noreply@anthropic.com>