Skip to content

feat(ci): report CRAP from measured test coverage (report-only) - #3408

Merged
thymikee merged 7 commits into
mainfrom
ci/coverage-crap
Oct 11, 2026
Merged

thymikee merged 7 commits into
mainfrom
ci/coverage-crap

Conversation

@thymikee

@thymikee thymikee commented Oct 11, 2026 •

Copy link
Copy Markdown
Member

Summary

CRAP now comes from measured test coverage instead of an estimate from which tests import what. Report-only for now: the first runs produce the list to fix.

  • check:fallow stops scoring CRAP (health.maxCrap: 0). The estimate disagreed with measured coverage on most findings.
  • The Vitest coverage run also writes coverage/coverage-istanbul.json. That name is not coverage-final.json, so local fallow runs never pick it up by accident.
  • pnpm check:coverage-crap drops file records with impossible hit counts and runs fallow 3.32 with fallow-crap.json, which enables only the CRAP rule. Vitest's v8 provider produces negative counts for modules loaded twice (vitest#11541), and fallow rejects the whole map on one; today 7 files are dropped, and the report names them. Findings outside the map (tests, scripts) are excluded rather than estimated.
  • CI's Coverage job reports touched functions on PRs (job summary plus warning annotations) and every function on main (--all). It never fails on a score.
  • The health baseline drops its 84 crap_* counts, which can no longer match.

Today on main: 58 functions at or above 30. Code that runs only out of process (workers, browser-evaluated scripts) measures 0%, so expect a few false positives to exempt before gating.

17 files, +585/−373, mostly baseline cleanup.

Validation

Tested 96b117457 on main 8a2580d10: pnpm check:affected --run passed (all runnable checks). A full local coverage run (1,692 files) fed check:coverage-crap --all (58 findings) and --base origin/main (none touched). CRAP tests 12/12; coverage-changed 23/23.

🤖 Generated with Claude Code

@github-actions

github-actions Bot commented Oct 11, 2026 •

Copy link
Copy Markdown
Contributor
PR Preview Action v1.8.1
Preview removed because the pull request was closed.
2026-10-11 10:25 UTC

@github-actions

github-actions Bot commented Oct 11, 2026 •

Copy link
Copy Markdown
Contributor

Size Report

Metric Base Current Diff
Installed (including dependencies) 5.15 MB 5.15 MB +87 B
Package (unpacked) 5.15 MB 5.15 MB +87 B
Package (download) 1.55 MB 1.55 MB -184 B

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 27.9 ms 27.1 ms -0.9 ms
CLI --help 75.6 ms 76.9 ms +1.3 ms

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 13 files

Reply with feedback, questions, or to request a fix.

View guided diff | Turn on auto-fix | Re-trigger cubic

Comment thread scripts/coverage-crap/model.ts Outdated
Comment thread scripts/coverage-crap/run.ts Outdated
Comment thread .github/workflows/ci.yml Outdated
Comment thread fallow-crap.json Outdated
Comment thread CONTRIBUTING.md Outdated
@thymikee

Copy link
Copy Markdown
Member Author

The code looks fine at 18b3fa7, but I can't call it ready yet because the CI proof is still pending. All 10 checks are queued or running, with no failures so far. The Coverage job runs the new reporter, the coverage-crap-model and coverage-crap steps, and the maxCrap change in .fallowrc.json that the Repo Guards fallow gate depends on. I could not confirm that health.maxCrap: 0 turns CRAP off in fallow 3.x, or that --max-crap on the CLI overrides the inherited config. I also did not run the coverage suite or fallow, so the 58-finding count, the 7 dropped files, and the diff-scoped "touched" behavior are not reproduced. I checked the fallow-baselines/health.json removal by hunk shape only. Please let Coverage and Repo Guards finish green at 18b3fa7. They show whether the gate stays quiet and whether the CRAP report step runs end to end. No conflicts.

Not blocking, and you can take or leave these: the comment at scripts/coverage-crap/model.ts line 31 says "negative or zeroed counts", but isHitCount accepts 0, so it should say "negative counts"; the touched-function diff at scripts/coverage-crap/run.ts line 59 lacks the --find-renames=90% that scripts/coverage-changed/run.ts line 80 pins, so one shared base...HEAD diff helper in scripts/lib would keep both gates in step; the only run() test covers the missing-map exit, so a fixture test of parseJsonReport plus readCrapFindings on a captured fallow 3.32 report would cover the main path; and the report steps in .github/workflows/ci.yml can still fail the Coverage job on tooling errors, so add continue-on-error: true if "report-only" should mean non-blocking.

Simplicity question: fallow already has --format markdown, review-github and --diff-stdin, so could most of renderCrapReport and renderCrapAnnotations be fallow output, leaving only sanitizeIstanbulCoverage and the coverage_source filter as custom code? I found no smaller design that keeps the estimated-source exclusion.

Review threads from cubic-dev-ai: none of the five apply at this head, so please resolve all of them. Command escaping and its test (#3408 (comment)) is fixed. process.exitCode instead of process.exit() (#3408 (comment)) is fixed. The !cancelled() and coverage-outcome gating in ci.yml (#3408 (comment)) is fixed. The single --max-crap threshold source (#3408 (comment)) is fixed. The CONTRIBUTING.md wording (#3408 (comment)) is fixed.

@thymikee

Copy link
Copy Markdown
Member Author

Thanks. The CI proof is in, and every non-blocking item is taken, at 27dbf7970.

CI at 18b3fa7. All 22 checks are green, including Coverage and Repo Guards. The Coverage job ran both new steps: Test coverage-based CRAP report passed, and Report coverage-based CRAP printed "no touched function(s) at or above 30" plus the same 7 unscored files as my local run. … for the whole repository was skipped, as intended on a PR.

The two fallow behaviors (3.32.0, measured locally):

  • health.maxCrap: 0 turns CRAP off. fallow health --complexity with the base config drops from 90 findings to 29, none carrying a CRAP score, and Repo Guards' fallow gate stays green.
  • --max-crap on the CLI overrides the inherited maxCrap: 0. fallow config --config fallow-crap.json resolves maxCrap to 0, yet check:coverage-crap --all (which passes --max-crap 30) reports 55 measured findings.

Taken:

  • The sanitizer comment now says it detects negative counts; zeroed counts from the same bug look like untested code and stay.
  • readChangedLinesDiff in scripts/lib/repo-git.ts is now the single base...HEAD diff both gates use, with --find-renames=90%.
  • parseFallowJsonReport moved into the model and is tested on a captured fallow 3.32 report (leading output plus an estimated finding it must drop), along with the no-JSON error.
  • Both report steps have continue-on-error: true, so "report-only" also covers tooling errors.

Simplicity question. I kept the custom rendering. --format markdown and review-github render every finding with no coverage_source filter, so estimated scores would come back; there's no per-source option. review-github also needs a PR-write token, and this workflow runs with contents: read. What's left custom is about 60 lines: the filter, a table, and escaped annotations.

All five cubic threads were already resolved.

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Reply with feedback, questions, or to request a fix.

View guided diff | Turn on auto-fix | Re-trigger cubic

Comment thread scripts/lib/repo-git.ts
@thymikee

Copy link
Copy Markdown
Member Author

Repo Guards fails at Check import-direction DAG because main itself fails it, not because of this PR. R81 reports src/daemon/platform-services.ts and src/daemon/device/claim-recovery-gateway.ts in no layer, a merge race between #3391 and #3403/#3404; main's own CI at 59d00bed3 fails the same way. This branch passes check:layering, and its check:affected --run is green at d86700533. The fix is #3412 (two manifest entries, matching #3395's assignments). Once #3412 lands I'll re-run Repo Guards here.

@thymikee

Copy link
Copy Markdown
Member Author

The code in d867005 looks good, and both earlier fixes are in. The cafe.ts regression test now runs through run(), and the report steps in ci.yml use !cancelled() && steps.run-coverage.outcome == 'success'. Coverage, the job that runs this change, is green at this head. Repo Guards fails at the import-direction step on src/daemon/platform-services.ts and src/daemon/device/claim-recovery-gateway.ts, which this PR does not touch, and you report that main fails the same way. Smoke is still running and does not reach any changed path. I did not run the coverage-changed or coverage-crap tests locally, and I did not re-measure fallow's --max-crap behavior. I relied on your numbers and on Coverage being green. The PR has no conflicts. Repo Guards stays red until #3412 lands on main, so please re-run it here after that.

On the other threads: the cubic-dev-ai threads #3408 (comment) (quotePath), #3408 (comment) (annotation escaping) and #3408 (comment) (report step conditions) are fixed at this head, so you can resolve them. The thread #3408 (comment) on the duplicated 30 threshold is benign and can be resolved. I did not re-check #3408 (comment) (run.ts exit handling) or #3408 (comment) (CONTRIBUTING wording), since both are outside this change.

@thymikee thymikee added the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Oct 11, 2026
thymikee and others added 7 commits October 11, 2026 11:47
check:coverage-crap reads the Istanbul map the coverage run now also writes
(coverage/coverage-istanbul.json, a name fallow does not pick up on its own),
drops file records with impossible hit counts (vitest#11541 merges a module
loaded both through the runner and natively into negative counts, and fallow
refuses the whole map on one), and asks fallow 3.32 to score CRAP only from
measured coverage. It scores the functions the diff against --base touches, or
every function with --all, and is report-only: a job-summary table plus
warning annotations for touched functions.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Review follow-up. Annotation properties and messages are escaped as
workflow-command data, so a path or function name with a newline, `:`, `,` or
`%` cannot end the command early. The entry point sets process.exitCode
instead of calling process.exit(), so a large --all report written to a pipe
is not truncated. The threshold is passed to fallow as --max-crap.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…scores at or above 30

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…n a real fallow report

Review follow-up. readChangedLinesDiff in scripts/lib/repo-git.ts is the one
zero-context base...HEAD diff both changed-line gates score, so the CRAP report
now pairs renames at 90% like the changed-line coverage gate. The report parser
moves into the model and is tested on a captured fallow 3.32 health report,
including an estimated finding it must drop. The sanitizer comment says which
counts it can detect: negative ones, not zeroed ones.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Git quotes non-ASCII paths in diff headers by default, so a change to a file
such as src/café.ts never matched its coverage record and the changed-line
gate skipped its lines. readChangedLinesDiff runs with core.quotePath=false,
which also covers the CRAP report's diff scope; a coverage-changed run test
pins the case.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
check:fallow's CRAP was estimated from imports and disagreed with measured
coverage on most findings, so the base config turns it off (health.maxCrap 0)
and fallow-crap.json re-enables it, CRAP only, for check:coverage-crap. The
health baseline drops its crap_* counts, which can no longer match. The
Coverage job runs the report on PRs (touched functions) and on main (--all),
and the report's model tests run beside the changed-line coverage model.
Both report steps add !cancelled() so a failed changed-line gate does not
skip them, and continue-on-error so a tooling error never fails the job, and the threshold lives only in run.ts (--max-crap).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@thymikee
thymikee merged commit 5f84627 into main Oct 11, 2026
25 checks passed
@thymikee
thymikee deleted the ci/coverage-crap branch October 11, 2026 10:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-human Valid work that needs human implementation, judgment, or maintainer merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant