Skip to content

refactor(coverage): give the changed-lines diff one owner and pass it on stdin - #3561

Merged
thymikee merged 3 commits into
mainfrom
chore/changed-lines-diff-owner
Oct 13, 2026
Merged

thymikee merged 3 commits into
mainfrom
chore/changed-lines-diff-owner

Conversation

@thymikee

@thymikee thymikee commented Oct 13, 2026 •

Copy link
Copy Markdown
Member

Summary

parseUnifiedDiff, ChangedFileDiff, and normalizePath move from scripts/coverage-changed/model.ts to scripts/lib (repo-git.ts beside readChangedLinesDiff, the diff they parse; repo-path.ts for the path rule), with their tests moving alongside (scripts/lib/repo-git.test.ts, repo-path.test.ts, run by the coverage-model gate). The two changed-line gates stay separate — only coverage-changed has a waiver label — but neither owns the diff format anymore.

check:coverage-crap now pipes the diff to fallow health --diff-stdin (fallow 3.32; documented as equivalent to --diff-file -) instead of writing coverage/fallow-crap.diff. No artifact of an earlier run can supply a scope, and run.test.ts seeds a stale artifact at that path with a debt-charging scope and proves the run ignores it and leaves it untouched.

11 files, +174/−158. No behavior change; scope is exactly the two audit items.

Validation

Head 3b208bfb0. pnpm check:affected --run passed on the head under the gate lock. Before/after proof for the stdin swap on a real git repo + real fallow 3.32 (fixture shaped like run.test.ts, edited debt function): reports and exit codes byte-identical (diff clean, both exit 1 with the CRAP 56.0 finding); after the change the run writes only fallow-crap-coverage.json under coverage/. Moved parser/tests are byte-identical (git diff -M90% + byte-compare); both test suites (23 + 59) green at their new paths. format, lint, typecheck, check:gate-manifest green. CI owns the instrumented Coverage and CRAP verdicts on the PR head.

parseUnifiedDiff, ChangedFileDiff, and normalizePath now live in
scripts/lib beside readChangedLinesDiff, the diff they parse, instead of
inside the changed-line gate's model where the CRAP gate had to import
across families. Both gates import from scripts/lib.

Hand the diff to fallow health on stdin (--diff-stdin) instead of
writing and reading coverage/fallow-crap.diff, so no artifact of an
earlier run can supply a scope.

@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 11 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/run.test.ts Outdated
@github-actions

github-actions Bot commented Oct 13, 2026 •

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

@github-actions

Copy link
Copy Markdown
Contributor

Size Report

Metric Base Current Diff
Installed (including dependencies) 5.17 MB 5.17 MB +59 B
Package (unpacked) 5.17 MB 5.17 MB +59 B
Package (download) 1.55 MB 1.55 MB +25 B

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 28.8 ms 27.8 ms -1.0 ms
CLI --help 81.2 ms 76.5 ms -4.7 ms

@thymikee

Copy link
Copy Markdown
Member Author

This PR is ready at 3b208bf. CI is green, with 26 checks and none failing. The coverage-changed and coverage-crap gates and their moved tests run in the Coverage CI job, and the package.json change adds those tests to the coverage-model and coverage-crap-model gates. There are no conflicts, and nothing else stands between this PR and merge. Not blocking, and you can take or leave both: the doc comment on parseUnifiedDiff at scripts/lib/repo-git.ts:85 explains why it sits beside its producer, and the module layout already shows that, so one line describing the function is enough; the comment at scripts/coverage-crap/run.test.ts:219 recalls what an earlier release wrote, and it could state only the rule that no file under coverage/ supplies or receives the scope. I did not run the script test suites or pnpm check:affected. My local fallow is 2.104.0, not the pinned ^3.32.0, so I could not confirm the empty-stdin behavior on 3.32 or reproduce the byte-identical before/after run described in the PR body. I also did not mutation-test the seeded-artifact regression, and I judged it by reading the old code, which wrote the file.

@thymikee thymikee added the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Oct 13, 2026
@thymikee
thymikee merged commit 6087be3 into main Oct 13, 2026
26 checks passed
@thymikee
thymikee deleted the chore/changed-lines-diff-owner branch October 13, 2026 05:44
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