feat(coverage): decide the touched-function CRAP verdict

[thymikee]

Summary

Gives check:coverage-crap --base <ref> an exit code that means something. It is report-only today: it prints the functions a diff touches and exits 0 whatever it finds.

It now returns 1 when a touched function scores CRAP ≥ 30 — including one that already scored that high — and when the reports cannot be trusted: no coverage map, a malformed map, a map missing a file the same run's coverage/lcov.info enumerated, a touched file the sanitizer dropped or the run never recorded, and a touched function the map predates. Each names the files and the command that clears it. --all stays the advisory main backlog.

Completeness is per path, not a count: two coverage runs mixed into one directory give equal counts over different files. Attribution reuses the rename-aware diff, so a pure move owes nothing.

Part of #3426. Stack: this PR → #3454, which flips CI to blocking. Review follow-ups: the model owns enumeration_missing, so no caller decides it by passing a set; and one relativizer covers map keys, lcov paths and diff paths — three that disagreed meant a foreign map matched nothing.

Validation

• SHA 03a37c7b8, on main after refactor(daemon): split the /rpc request-end handler into its four questions #3452 and the G2 stack merged: 40 tests; check:affected --base origin/main --run, check:fallow --base, tsc --strict green.
• Your three unknowns are answered in my review reply; ci(coverage): fail a PR whose touched functions score CRAP 30 or more #3454 asserts two through the entrypoint.
• Risk: blocking is proven by tests until ci(coverage): fail a PR whose touched functions score CRAP 30 or more #3454 flips CI.

[github-actions]

Size Report

Metric	Base	Current	Diff
Installed (including dependencies)	5.17 MB	5.17 MB	0 B
Package (unpacked)	5.17 MB	5.17 MB	0 B
Package (download)	1.55 MB	1.55 MB	-8 B

Startup median (7 runs, lower is better):

Scenario	Base	Current	Diff
CLI --version	21.7 ms	21.6 ms	-0.2 ms
CLI --help	60.1 ms	59.7 ms	-0.4 ms

[cubic-dev-ai]

All reported issues were addressed across 3 files

<sub>Reply with feedback, questions, or to request a fix.

View guided diff | Turn on auto-fix | Re-trigger cubic</sub>

[cubic-dev-ai]

All reported issues were addressed across 3 files (changes from recent commits).

<sub>Reply with feedback, questions, or to request a fix.

View guided diff | Turn on auto-fix | Re-trigger cubic</sub>

[thymikee]

The code in 8203d7f looks right to me and I found no defect that should stop the merge, but one piece of evidence is still missing: the PR-side CRAP step stays continue-on-error until #3454, so the 14 green checks never reach the blocking exit. I have not run the tests, tsc or fallow, so the 40 passing tests and the clean lint are not confirmed by me. I also have not checked that fallow's finding.path values are repo-relative (if they are absolute, unmeasured_touched never fires), that vitest v8 with include writes untested files into both lcov and the json map, or that --diff-file scoping includes a high-CRAP function whose lines this diff edits. No conflicts. Before #3454 stacks on this, please prove the blocking exit once: a run() test with a fixture map, an lcov file and a stubbed fallow that returns 1 for a finding and covers the enumeration_missing branch in run.test.ts, or the end-to-end proof in #3454 as planned.

Could the four problem kinds (map_partial, unscored_touched, unmeasured_touched, enumeration_missing) be replaced by one check? They all detect a coverage/ directory that this checkout's run did not write, and lcov and the json map come from the same vitest coverage map, so in a fresh run they cannot disagree. If test:coverage stamped HEAD plus a tree hash into coverage/, the model could refuse on a mismatch. That would make the stale class impossible, and it would also catch new files and sub-threshold estimates that the current detectors miss. corrupt_touched would stay, plus a declared "uninstrumented" set read from coverage.exclude. This needs a stamp written by the test:coverage script or a vitest reporter first. The size is fine at 611 production lines. Short of that, please move enumeration_missing into decideCrapVerdict with a nullable enumeration, so the model alone owns that fact.

Not blocking, and you can take or leave these: run.ts decides "enumeration missing" and then passes an empty Set, and model.ts re-derives it from size === 0, so enumeratedPaths should be ReadonlySet<string> | null and the model should emit the problem; three helpers relativize coverage paths (coverageMapPaths, relativeToRoot in run.ts, and normalizePath in coverage-changed/model.ts), so keys outside the root can never match, and one shared helper would fix that and let you delete relativeToRoot; a touched file in neither report goes to non-blocking unscoped (files removed by coverage.exclude, or any new file with a stale local coverage/), and unmeasured_touched only sees estimates of 30 or more, so please name the excluded populations in the #3454 testing.md text; and one run() test would cover the blocking exit, as above.

The open inline thread on the per-path comparison and the three below are fixed at this head, so you can resolve them: zero files turning into null, exemption parse failures and empty enumeration returning no unscoped entries. The new file listed as unscoped thread is benign in CI, where coverage runs on the same checkout with src/** included, so only stale local runs remain and you can resolve it too.

[thymikee]

Fixed at 46845a916, plus the two answers you asked for that I could measure.

The blocking exit is proven in #3454, as you allowed. 19 cases drive run() against a real git repo and real fallow; the one you asked for is the blocking exit follows the reported finding, not this suite's guess about scoring — a fixture map, an lcov file and a stand-in fallow reporting one touched finding at CRAP 42, asserting exit 1, and reading back the flags to prove --diff-file reached it (--all with the same finding asserts exit 0 and no --diff-file). enumeration_missing is covered from both sides: absent file and present-but-empty. Two more assert that a finding cannot outvote a gap — the missing enumeration and an estimated score in a file the map predates still exit 1 with the finding on the table.

Your three unknowns, measured against this checkout's real coverage run and the captured fallow 3.32 report:

• finding.path is repo-relative: 3 of 3 findings in the real report (src/daemon/handlers/session-state.ts), none absolute. So unmeasured_touched's join holds; ci(coverage): fail a PR whose touched functions score CRAP 30 or more #3454's estimated-score case pins it through the entrypoint.
• Vitest v8 with all does write untested files into both reports: 1,724 files in each, 176 of them with every statement count 0, and the sample (packages/ad-replay/src/index.ts) present in lcov too. That is what makes map-vs-lcov per path the right staleness detector.
• --diff-file scoping does include a high-CRAP function whose lines the diff edits: editing a function that already scores too high blocks edits one decision into a function already at 42, and real fallow with --diff-file reports it at 56 — exit 1, with the annotation.

enumeration_missing is now the model's. enumeratedPaths is ReadonlySet<string> | null, decideCrapVerdict emits the problem, and the ioProblems channel is gone with it since that was its only user — a caller can no longer decide the fact by passing a set. The two ways it goes missing (no file, file listing nothing) are one message with one fix, since usableEnumeration's empty-set rule now lives beside the read that needs the filesystem. --all deliberately does not claim its backlog is incomplete: that scope scores from the map alone.

One relativizer: scripts/lib/repo-path.ts owns root-relativization and posix form; coverageMapPaths, the dropped-record list, and normalizePath all go through it, and relativeToRoot is deleted. The old pair disagreed exactly where you said: a foreign absolute key stayed absolute under the prefix test and became ../… under path.relative, so the two could never join. They now fail to match identically, which is the honest outcome for a map from another checkout.

The stamp is the better design and I did not attempt it here: it makes the whole stale class unreachable and catches what my four detectors only approximate (new files, sub-threshold estimates). It needs test:coverage to write it first, which is a different change than this gate. The coordinator will file it as a follow-up.

[thymikee]

The PR is ready at 03a37c7. The two issues from the earlier review are fixed: the per-path comparison now holds at model.ts:299, and an empty lcov listing becomes null in run.ts:96-99 and reaches enumeration_missing at model.ts:287. All 22 checks pass. I found no conflicts. Nothing blocks merge.

Not blocking, and you can take or leave these: (1) scripts/coverage-crap/run.ts:149-151 is a leftover comment that repeats the one after it and still describes the old per-file rule, so delete it; (2) the empty-lcov-to-null rule now lives in readEnumeratedPaths and nothing at this head tests it, so make sure #3454 lands that case or add one run() case here; (3) the header in scripts/lib/repo-path.ts calls it "the one rule", but scripts/depgraph/affected-run.ts:113 and scripts/mutation/modules.ts:147 still relativize on their own, so narrow it to coverage reports and diffs.

The CRAP step stays continue-on-error until #3454, so a green Coverage job does not exercise the blocking exit. #3454 should land its run() proof of the blocking exit, both enumeration_missing variants, and --diff-file reaching fallow before it removes continue-on-error. I did not run tests, tsc or fallow here. I also did not reproduce your measurements of repo-relative finding.path and untested files appearing in both lcov and the map.

On the other review threads, the cubic threads on the unrecorded set difference (#3460 (comment)), zero files becoming null (#3460 (comment)), exemption parse failures (#3460 (comment)) and null enumeratedPaths giving unscoped [] (#3460 (comment)) are fixed at this head, so you can resolve them. The thread on a new file listed as unscoped (#3460 (comment)) is benign: it only happens with a stale local coverage/ directory, and CI runs coverage on the same checkout with src/** included, so you can resolve it too.