ci: add quality gates for coverage and type checking#427
Open
raballew wants to merge 5 commits intojumpstarter-dev:mainfrom
Open
ci: add quality gates for coverage and type checking#427raballew wants to merge 5 commits intojumpstarter-dev:mainfrom
raballew wants to merge 5 commits intojumpstarter-dev:mainfrom
Conversation
✅ Deploy Preview for jumpstarter-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdded a CI type-check job that runs when Python paths change, added a PR-only diff-cover step to enforce 80% changed-lines coverage, and updated Python dev tooling to include diff-cover and enable branch coverage in coverage config. Changes
Sequence Diagram(s)(No sequence diagrams generated — changes are CI/workflow and tooling adjustments.) Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
raballew
force-pushed
the
040-quality-gates-v2
branch
from
2bd2321 to
b6c2149
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/python-tests.yaml:
- Around line 103-107: The current workflow step "Check coverage on changed
lines" uses the glob packages/*/coverage.xml which only matches packages that
already emit XML, so changed packages without --cov-report=xml are skipped;
modify the job to first compute the set of changed packages (git diff
--name-only origin/${{ github.base_ref }}...HEAD), then for each changed package
check for packages/<pkg>/coverage.xml and for any missing run that package's
tests to produce an XML report (e.g., run pytest or the package's test script
with --cov-report=xml) before invoking diff-cover, or alternatively fail the job
with a clear message if a changed package cannot produce a coverage XML; update
the step that runs diff-cover to pass only the discovered coverage.xml files
rather than the static glob.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4c49de4b-965b-4e2a-a494-9ebd5413583d
📒 Files selected for processing (1)
.github/workflows/python-tests.yaml
… checking Enable branch coverage measurement, add diff-cover to enforce >=80% coverage on changed lines in PRs, and add ty type checking to the lint workflow (non-blocking until existing diagnostics are resolved). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Packages without test coverage XML are now surfaced as GitHub Actions warnings instead of silently skipping. Also handles the case where no coverage.xml files exist at all. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Use PYTEST_ADDOPTS to ensure all packages produce coverage.xml, not just the ones that inherit the root pytest config. This closes the gap where changed lines in 30 packages were silently skipped by diff-cover. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
raballew
force-pushed
the
040-quality-gates-v2
branch
from
8e0664b to
8d6eb29
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
.github/workflows/python-tests.yaml (1)
109-114:⚠️ Potential issue | 🟠 MajorChanged-package coverage artifacts are still not validated before gating.
At Line 109-114, the step gathers existing
coverage.xmlfiles, but it does not verify that each changed package underpython/packages/*produced one. That means changed packages missingcoverage.xmlcan still slip through this gate.Proposed hardening
- name: Check coverage on changed lines if: github.event_name == 'pull_request' working-directory: python run: | - coverage_files=$(find packages -name coverage.xml 2>/dev/null | sort) - if [ -z "$coverage_files" ]; then + mapfile -t coverage_files < <(find packages -name coverage.xml 2>/dev/null | sort) + if [ ${`#coverage_files`[@]} -eq 0 ]; then echo "::error::No coverage.xml files found" exit 1 fi - uv run diff-cover $coverage_files --compare-branch=origin/${{ github.base_ref }} --fail-under=80 + + mapfile -t changed_packages < <( + git diff --name-only "origin/${{ github.base_ref }}"...HEAD \ + | sed -nE 's#^python/packages/([^/]+)/.*#\1#p' \ + | sort -u + ) + + missing=0 + for pkg in "${changed_packages[@]}"; do + if [ ! -f "packages/${pkg}/coverage.xml" ]; then + echo "::error::Changed package missing coverage.xml: ${pkg}" + missing=1 + fi + done + [ "$missing" -eq 0 ] || exit 1 + + uv run diff-cover "${coverage_files[@]}" --compare-branch=origin/${{ github.base_ref }} --fail-under=80🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/python-tests.yaml around lines 109 - 114, The workflow currently collects coverage.xml into coverage_files but doesn’t ensure every changed package under python/packages/* produced one; enhance the step to (1) compute the set of changed package names (e.g., via git diff against origin/${{ github.base_ref }} for paths under python/packages/), (2) for each changed package verify a coverage.xml exists (e.g., look for python/packages/<pkg>/coverage.xml or the path you expect) and fail with a clear ::error:: if any are missing, and only then run the existing uv run diff-cover $coverage_files --compare-branch=origin/${{ github.base_ref }} --fail-under=80; reference the variables/commands coverage_files, python/packages/*, coverage.xml and the diff-cover invocation when locating where to add these checks.
🧹 Nitpick comments (1)
.github/workflows/python-tests.yaml (1)
105-106: Run diff-cover once instead of on every matrix leg.Line 105-106 currently executes this PR coverage gate in every OS/Python matrix job. Consider limiting it to a single canonical matrix combination to reduce CI time and duplicate noise.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/python-tests.yaml around lines 105 - 106, The "Check coverage on changed lines" step currently runs in every matrix leg; restrict it to a single canonical matrix combination by adding an additional condition to its if expression (e.g., require a specific matrix value like matrix.os == 'ubuntu-latest' and matrix.python-version == '3.11') so the step only executes for that one leg while keeping the existing github.event_name == 'pull_request' check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In @.github/workflows/python-tests.yaml:
- Around line 109-114: The workflow currently collects coverage.xml into
coverage_files but doesn’t ensure every changed package under python/packages/*
produced one; enhance the step to (1) compute the set of changed package names
(e.g., via git diff against origin/${{ github.base_ref }} for paths under
python/packages/), (2) for each changed package verify a coverage.xml exists
(e.g., look for python/packages/<pkg>/coverage.xml or the path you expect) and
fail with a clear ::error:: if any are missing, and only then run the existing
uv run diff-cover $coverage_files --compare-branch=origin/${{ github.base_ref }}
--fail-under=80; reference the variables/commands coverage_files,
python/packages/*, coverage.xml and the diff-cover invocation when locating
where to add these checks.
---
Nitpick comments:
In @.github/workflows/python-tests.yaml:
- Around line 105-106: The "Check coverage on changed lines" step currently runs
in every matrix leg; restrict it to a single canonical matrix combination by
adding an additional condition to its if expression (e.g., require a specific
matrix value like matrix.os == 'ubuntu-latest' and matrix.python-version ==
'3.11') so the step only executes for that one leg while keeping the existing
github.event_name == 'pull_request' check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9a5005dd-e56b-4f4e-b815-8056a169d412
⛔ Files ignored due to path filters (1)
python/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
.github/workflows/lint.yaml.github/workflows/python-tests.yamlpython/pyproject.toml
✅ Files skipped from review due to trivial changes (1)
- python/pyproject.toml
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/lint.yaml
The read_pty_output reader could exit (via stop flag) without draining remaining data from the kernel PTY buffer. This caused flaky test failures on macOS when coverage overhead changed async task timing. Also removes redundant PYTEST_ADDOPTS env var from CI since the root pyproject.toml addopts already includes --cov --cov-report=xml. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Member
Author
|
@ambient-code why is CI failing? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
branch = truein[tool.coverage.run]) to catch untestedif/elsepathscontinue-on-error: true) to surface type errors before merge -- can be made blocking once the 49 existing diagnostics are resolveddiff-coverto dev dependenciesRationale
As AI-generated code contributions increase, automated quality gates that prevent regression are essential for maintaining confidence when accepting changes. These three changes are the highest-impact, lowest-effort improvements:
if/elseis tested -- common in AI-generated error handling paths.Test plan
fetch-depth: 0anddiff-coverstep🤖 Generated with Claude Code