feat: add python line and branch coverage governance to ci - #317
Conversation
Add scripts/coverage_gate.py and make targets that measure line and branch coverage for the raven package and enforce three gates: a monotonic ratchet against an audited baseline, 90% coverage on the executable lines a pull request changes, and a baseline that cannot be lowered relative to the target branch. The CI unit job runs the suite once with coverage and reuses the report for every gate, uploading xml, json, and html artifacts on every run. Co-authored-by: Claude (claude-opus-4-8) <noreply@anthropic.com>
|
Claude finished @forrestjgq's task in 1m 32s —— View job Review: python coverage governance CINo blockers; this can merge.
Both findings from the earlier review round were fixed in the follow-up commit
The coverage-baseline.json was also regenerated against the Ubuntu/3.12 CI run itself so the ratchet reproduces at delta 0.00, matching the commit's stated intent. No new issues found in the delta: gate logic (ratchet, diff, baseline-check), Makefile targets, and |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
|
||
| - name: Install dependencies | ||
| run: uv sync --locked --all-extras --dev | ||
| run: uv sync --frozen --all-extras --dev --python ${{ matrix.python }} |
There was a problem hiding this comment.
[blocker] This silently removes the only CI check that verifies uv.lock is up to date with pyproject.toml. The previous step used uv sync --locked ..., which fails if the lockfile is stale; switching to --frozen (needed to add --python ${{ matrix.python }}) makes uv sync accept a stale lockfile without error. Grepping the repo, no other job or Makefile target uses --locked for this purpose (Makefile's install-deps already used --frozen before this PR, but this was the one CI step enforcing lockfile freshness). After this change, a PR that edits a dependency in pyproject.toml without running uv lock will no longer be caught by CI.
Suggest keeping the freshness check, e.g. uv sync --locked --all-extras --dev and setting the interpreter via uv python pin/UV_PYTHON env instead of the --python flag, or adding a separate uv lock --check/--locked step.
| def parse_changed_lines(diff: str) -> dict[str, set[int]]: | ||
| """Return new-side hunk lines; deleted lines never enter the denominator.""" | ||
| changed: dict[str, set[int]] = {} | ||
| path: str | None = None | ||
| for line in diff.splitlines(): | ||
| if line.startswith("+++ b/"): |
There was a problem hiding this comment.
[nit] path is never reset/cleared between files, and only updated on +++ b/... lines. A deleted file's diff shows +++ /dev/null, which doesn't match, so its hunks would silently be attributed to whatever path was last set from an earlier file. Currently harmless because the only caller (git_diff_inputs) filters with --diff-filter=ACMR (no deletions ever reach this function), but it's a latent bug if parse_changed_lines is ever reused on a raw diff. Consider setting path = None when the new-file line isn't +++ b/....
The bootstrap baseline was generated on macOS; the Ubuntu 3.12 CI run produces slightly lower line and branch coverage, so the ratchet failed by more than its 0.05pp tolerance. Replace it with the candidate emitted by that CI run so the ratchet reproduces at delta 0.00. Also restore uv sync --locked on the unit job, since dropping it for --frozen removed the only CI check that uv.lock matches pyproject.toml, and reset the parser path on a +++ /dev/null target so a deleted file's hunks cannot be misattributed if parse_changed_lines runs on an unfiltered diff. Co-authored-by: Claude (claude-opus-4-8) <noreply@anthropic.com>
gloryfromca
left a comment
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
My own pass over github/main...e6e4b31f (independent of the bot review already on the thread). What I covered: AGENTS.md rules, the whole diff (workflow, Makefile, pyproject, gate script, baseline, tests, tests/README.md), the gate's callers and the CI job log for the current head, backward compatibility of the changed Make targets, and whether the tests actually assert.
Verification I ran
uv run --frozen --python 3.12 --all-extras pytest tests/test_coverage_gate.py -q-> 14 passed, 0 skipped. No graceful-skip, no env-gated assertion;test_git_diff_inputs_includes_top_level_raven_filesbuilds a real temp repo and exercises the:(glob)raven/**/*.pypathspec, which is exactly the trap the comment above it describes.- Read the unit job log for this head: every gate really executed.
Line 75.56% / Branch 64.04%,Ratchet line delta=+0.00pp,Ratchet branch delta=+0.00pp. The committed baseline reproduces bit-for-bit on CI, so the ratchet is not sitting on rounding luck. - Note for the record:
Diff coverage: 100.00% (0/0 executable changed lines)-- this PR touches noraven/file, so the diff gate is green vacuously here. Its logic is covered by unit tests, not by this run.
Both earlier findings are genuinely fixed in e6e4b31f, not just closed: uv sync --locked --all-extras --dev --python 3.12 restores the lockfile-freshness check while keeping the matrix pin (--frozen on the later uv run calls only skips the lock check, the sync already enforced it), and parse_changed_lines now resets path = None on a non-b/ target with a test for the +++ /dev/null case.
Things I looked at and decided are not findings
OMITTED_PATHSduplicated betweenpyproject.tomland the script: already guarded bytest_documented_omissions_match_run_and_report_configuration, so drift fails loudly.exclude_alsorather thanexclude_lines, so coverage.py'spragma: no coverdefaults survive. Correct choice.--python $(PYTHON_VERSION)on everyuv runwill rebuild a contributor's 3.13.venvat 3.12. That is the point of a reproducible baseline, andtests/README.mddocuments the override.- Baseline JSON is 96 KiB and not an asset type AGENTS.md section 7 blocks;
repository filesis green.
Three inline notes, all non-blocking. The first one is the only one I would actually act on, and it can be a follow-up.
| target = float(previous["totals"][metric]) | ||
| delta = value - target | ||
| print(f"Baseline {metric}: proposed={value:.6f}% target={target:.6f}% delta={delta:+.6f}pp") | ||
| if value < target: |
There was a problem hiding this comment.
[non-blocking, but the one worth acting on] The two gates together make deleting well-covered production code unmergeable, with no documented escape.
Overall coverage is C/S. Remove a chunk whose own coverage c/s is above the project average and the total drops: deleting a 100%-covered 200-statement module takes line coverage from 32494/43003 = 75.5622% to 32294/42803 = 75.4497%, i.e. -0.11pp, past the 0.05pp tolerance -> coverage-ratchet fails. The obvious remedy is to lower the baseline in the same PR, but check_baseline_update rejects any decrease (if value < target, strict, no tolerance), so coverage-baseline-check fails too. tests/README.md closes the last door explicitly: "Never edit the percentages by hand or lower the baseline to make a change pass."
So the author's only options are editing COVERAGE_RATCHET_TOLERANCE in the Makefile (a permanent loosening for a one-off deletion) or padding unrelated tests until the number comes back. This is not an exotic case -- removing a dead but well-tested module is routine, and 200 statements is enough to trigger it.
Worth defining the sanctioned path before people hit it. Options, cheapest first: (a) document that a baseline decrease is allowed when the PR is a net deletion, and have check_baseline_update accept it behind an explicit marker rather than a blanket ban; (b) compare against coverage recomputed on the target branch instead of a static file, so deletions cancel out; or (c) ratchet on covered/total counts of surviving files rather than the whole-project ratio. Same reasoning applies to branch_percent.
Secondary point on this function: the ratchet tolerates 0.05pp of noise but check_baseline_update tolerates none, so a candidate refreshed from a main run that dipped by 0.000001pp is rejected. Worth sharing one tolerance.
| } | ||
| }, | ||
| "python": "3.12", | ||
| "reference_commit": "840d05101b393354e24bba0191b6d9ae40790df6", |
There was a problem hiding this comment.
[nit] This sha is not resolvable in a clone. git cat-file -t 840d0510 -> fatal: could not get object info; via the API it is Merge af70757b into cd686453, i.e. the ephemeral refs/pull/317/merge commit GitHub synthesises for the PR, reachable from no branch and gone after the squash-merge.
It comes from coverage-baseline-candidate running git rev-parse HEAD while the unit job sits on the merge ref (the pre-commit job pins ref: github.event.pull_request.head.sha for exactly this reason; the unit job does not). tests/README.md tells the reviewer to "Review the reference commit and totals in the candidate before replacement", which this value cannot support.
Post-bootstrap candidates from main push runs will record a real commit, so the damage is limited to the tracked bootstrap baseline plus any candidate a contributor pulls from a PR run to sanity-check a refresh. Cheap fix: thread --commit ${{ github.event.pull_request.head.sha || github.sha }} through the Make target.
|
|
||
| ### §1.1 Top rule: don't add comments unless necessary | ||
|
|
||
| - Every new code file must document its purpose in English with an appropriate |
There was a problem hiding this comment.
[nit] This is a new repo-wide hard constraint riding along in a CI-coverage PR. AGENTS.md is self-describing as "hard constraints only (violations get reverted / rejected)", so from merge onward every new file in the repo is subject to it, and reviewers looking at a coverage PR are not the audience that would weigh in on it. The bullet itself is fine and the follow-on sentence resolves the tension with the "don't comment unless necessary" top rule -- it is the bundling I would flag.
Also: the summary table at the top still gives section 1 as "Don't comment unless necessary; comments in English", which no longer describes a section that now mandates a module docstring in every new file. If the bullet stays, the gist row should mention it.
4 participants
Summary
Adds Python coverage governance to CI so existing coverage cannot silently
regress and new code stays well tested, without pretending the project is
already near 100%.
scripts/coverage_gate.pyplusmake coverage*targets compute line andbranch coverage for the
ravenpackage and drive four gates:.github/coverage-baseline.json(0.05pp tolerance for rounding noise);(comments, deletions, and non-executable lines are excluded);
branch coverage relative to the target branch.
every gate, uploads xml/json/html artifacts on every run (
if: always()),and keeps diff coverage as a PR-only gate.
pyproject.tomlconfigurescoverage.py(branch mode,ravensource, threedocumented omissions); generated reports are gitignored.
than auto-raising, so the diff-coverage gate is what guards new code while the
ratchet only blocks regression.
The production-file pathspec uses git glob magic (
:(glob)raven/**/*.py) sotop-level modules such as
raven/__init__.pyare not silently dropped from diffcoverage; a regression test covers this.
Type
Verification
uv run --frozen --python 3.12 pytest tests/test_coverage_gate.py -q-> 13 passeduv run --frozen --python 3.12 --extra dev ruff check scripts/coverage_gate.py tests/test_coverage_gate.py-> All checks passeduv run --frozen --python 3.12 --extra dev ruff format --check scripts/coverage_gate.py tests/test_coverage_gate.py-> already formattedRelevant tests pass locally
Relevant lint / type checks pass locally
User-facing docs or screenshots are updated when needed
Risk
No runtime
ravencode changes; this is CI and tooling only. The new gates canfail PRs that lower coverage, which is the intended behavior. Rollback is a
plain revert of this commit (or disabling the coverage steps in the unit job).
Related Issues
N/A