ci(github-actions): integrate bandit vulnerability scanner and coverage guard#528
Conversation
📝 WalkthroughWalkthroughAdds a new GitHub Actions workflow that runs Bandit security scans and pytest coverage checks on pull requests to main, backed by a new ChangesPR Coverage & Security Guard
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant GitHub as GitHub Actions
participant Bandit
participant Pytest
participant Script as verify_coverage.py
GitHub->>GitHub: checkout, setup Python 3.12, install .[dev]
GitHub->>Bandit: scan agentwatch/
Bandit-->>GitHub: scan results
GitHub->>Pytest: run tests with --cov=agentwatch/
Pytest-->>GitHub: coverage.json
GitHub->>Script: run verify_coverage.py
Script->>Script: compare coverage vs 70% threshold
Script-->>GitHub: exit 0 (pass) or exit 1 (fail)
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
🧪 PR Test Results
Python 3.12 · commit a2dbde8 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
.github/workflows/coverage_security_guard.yml (1)
11-11: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winSet
persist-credentials: falseon checkout.The default
actions/checkout@v4persists the GitHub token in the local git config, which later steps (e.g.,pip install .[dev]pulling third-party packages) could read or exfiltrate.🔒 Suggested fix
- uses: actions/checkout@v4 + with: + persist-credentials: false🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/coverage_security_guard.yml at line 11, The checkout step in the coverage_security_guard workflow currently uses actions/checkout@v4 with default credential persistence, which leaves the GitHub token available to later steps. Update the checkout configuration to disable credential persistence by setting persist-credentials to false on the actions/checkout invocation in this workflow.Source: Linters/SAST tools
scripts/verify_coverage.py (2)
13-31: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract pure comparison logic for testability.
Currently
main()bundles subprocess invocation, file I/O, and the threshold check together, forcing tests to mocksubprocess.runeven to exercise the comparison logic. Splitting out a pure function makes it trivial to add a test for the failing-threshold branch (currently untested intests/test_coverage_guard.py).♻️ Suggested refactor
+def check_threshold(percent: float, required_threshold: float = 70.0) -> bool: + """Return True if coverage meets the threshold.""" + return percent >= required_threshold + + def main() -> None: # Run coverage report in JSON format subprocess.run(["coverage", "json"], check=True) with open("coverage.json", "r") as f: data = json.load(f) percent = data["totals"]["percent_covered"] required_threshold = 70.0 print(f"Current Code Coverage: {percent:.2f}%") print(f"Required Coverage Threshold: {required_threshold}%") - if percent < required_threshold: + if not check_threshold(percent, required_threshold): print("Code coverage is below the required threshold! PR blocked.") sys.exit(1)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/verify_coverage.py` around lines 13 - 31, Extract the threshold comparison out of main() in verify_coverage.py into a small pure helper that takes the coverage percent and required threshold and returns whether the PR should pass. Keep main() responsible only for subprocess.run, reading coverage.json, and printing/exiting, then call the new helper for the check. This will let tests in tests/test_coverage_guard.py cover the failing-threshold branch without mocking subprocess.run.
15-20: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueNo error handling for missing/malformed coverage data.
If
coverage jsonfails to produce output,coverage.jsonis missing, or thetotals/percent_coveredkeys are absent, this raises an unhandledFileNotFoundError/KeyError/json.JSONDecodeErrorwith a raw traceback instead of a clear failure message for CI logs.🛡️ Suggested fix
- with open("coverage.json", "r") as f: - data = json.load(f) - - percent = data["totals"]["percent_covered"] + try: + with open("coverage.json", "r") as f: + data = json.load(f) + percent = data["totals"]["percent_covered"] + except (FileNotFoundError, json.JSONDecodeError, KeyError) as exc: + print(f"Failed to read coverage data: {exc}") + sys.exit(1)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/verify_coverage.py` around lines 15 - 20, The coverage parsing flow in verify_coverage.py lacks defensive handling around subprocess.run("coverage json"), reading coverage.json, and accessing data["totals"]["percent_covered"]. Update the script to catch failures from generating or reading the report (including missing file, malformed JSON, or absent keys) and fail with a clear, CI-friendly message instead of an uncaught traceback. Use the existing main coverage-check logic in verify_coverage.py to centralize the validation and emit a descriptive error before exiting non-zero.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/coverage_security_guard.yml:
- Around line 27-33: The workflow currently checks coverage with a custom script
instead of pytest’s built-in gate. Update the test step in the
coverage_security_guard workflow (the one running pytest for agentwatch/) to
include --cov-fail-under=70, and remove the separate Verify Minimum Coverage
Thresholds step that calls scripts/verify_coverage.py. Keep the existing pytest
coverage collection, but consolidate the threshold enforcement in the pytest
invocation.
In `@tests/test_coverage_guard.py`:
- Around line 12-35: Add a test in test_coverage_guard.py that covers the
failing-threshold path in verify_coverage.main, alongside
test_coverage_script_passes. Mock coverage.json with percent_covered below the
70% threshold, keep subprocess.run patched as in the passing test, and assert
that main raises SystemExit with exit code 1. Use the existing main entry point
and the coverage.json setup so the new test exercises the same script behavior.
---
Nitpick comments:
In @.github/workflows/coverage_security_guard.yml:
- Line 11: The checkout step in the coverage_security_guard workflow currently
uses actions/checkout@v4 with default credential persistence, which leaves the
GitHub token available to later steps. Update the checkout configuration to
disable credential persistence by setting persist-credentials to false on the
actions/checkout invocation in this workflow.
In `@scripts/verify_coverage.py`:
- Around line 13-31: Extract the threshold comparison out of main() in
verify_coverage.py into a small pure helper that takes the coverage percent and
required threshold and returns whether the PR should pass. Keep main()
responsible only for subprocess.run, reading coverage.json, and
printing/exiting, then call the new helper for the check. This will let tests in
tests/test_coverage_guard.py cover the failing-threshold branch without mocking
subprocess.run.
- Around line 15-20: The coverage parsing flow in verify_coverage.py lacks
defensive handling around subprocess.run("coverage json"), reading
coverage.json, and accessing data["totals"]["percent_covered"]. Update the
script to catch failures from generating or reading the report (including
missing file, malformed JSON, or absent keys) and fail with a clear, CI-friendly
message instead of an uncaught traceback. Use the existing main coverage-check
logic in verify_coverage.py to centralize the validation and emit a descriptive
error before exiting non-zero.
🪄 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 Plus
Run ID: 66eddd99-aed9-4504-a014-46db77b5d743
📒 Files selected for processing (5)
.github/workflows/coverage_security_guard.ymldocs/security_ci.mdpyproject.tomlscripts/verify_coverage.pytests/test_coverage_guard.py
| - name: Run Tests and Measure Coverage | ||
| run: | | ||
| pytest --cov=agentwatch/ tests/ | ||
|
|
||
| - name: Verify Minimum Coverage Thresholds | ||
| run: | | ||
| python scripts/verify_coverage.py |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Consider using pytest's native --cov-fail-under=70 instead of a separate script.
The coding guidelines state coverage is Maintain minimum test coverage of 70% (enforced in CI via pytest with --cov-fail-under=70). This workflow instead reimplements the threshold check via scripts/verify_coverage.py, adding a custom script, extra CI step, and test to maintain, when pytest --cov=agentwatch/ --cov-fail-under=70 tests/ would achieve the same gate natively.
If the custom script is intentional (e.g., for richer output/logging), consider ignoring this; otherwise consolidating removes duplicate logic.
As per coding guidelines, tests/**/*.py: "Maintain minimum test coverage of 70% (enforced in CI via pytest with --cov-fail-under=70)".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/coverage_security_guard.yml around lines 27 - 33, The
workflow currently checks coverage with a custom script instead of pytest’s
built-in gate. Update the test step in the coverage_security_guard workflow (the
one running pytest for agentwatch/) to include --cov-fail-under=70, and remove
the separate Verify Minimum Coverage Thresholds step that calls
scripts/verify_coverage.py. Keep the existing pytest coverage collection, but
consolidate the threshold enforcement in the pytest invocation.
Source: Coding guidelines
| def test_coverage_script_passes(tmp_path, monkeypatch): | ||
| # Mock coverage.json file | ||
| json_path = tmp_path / "coverage.json" | ||
| dummy_data = { | ||
| "totals": { | ||
| "percent_covered": 75.5 | ||
| } | ||
| } | ||
| with open(json_path, "w") as f: | ||
| json.dump(dummy_data, f) | ||
|
|
||
| monkeypatch.chdir(tmp_path) | ||
| # Monkeypatch subprocess to mock the raw coverage call | ||
| monkeypatch.setattr(subprocess, "run", lambda *args, **kwargs: None) | ||
|
|
||
| # Ensure root path is in sys.path | ||
| root_path = os.path.abspath(os.path.join(os.path.dirname(__file__), "..")) | ||
| if root_path not in sys.path: | ||
| sys.path.insert(0, root_path) | ||
|
|
||
| from scripts.verify_coverage import main | ||
| with pytest.raises(SystemExit) as excinfo: | ||
| main() | ||
| assert excinfo.value.code == 0 |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add a test for the failing-threshold branch.
Only the passing case (coverage ≥ 70%) is tested. The sys.exit(1) branch when coverage is below threshold is never exercised, leaving a core behavior of the script unverified.
✅ Suggested addition
+def test_coverage_script_fails(tmp_path, monkeypatch):
+ json_path = tmp_path / "coverage.json"
+ dummy_data = {"totals": {"percent_covered": 50.0}}
+ with open(json_path, "w") as f:
+ json.dump(dummy_data, f)
+
+ monkeypatch.chdir(tmp_path)
+ monkeypatch.setattr(subprocess, "run", lambda *args, **kwargs: None)
+
+ root_path = os.path.abspath(os.path.join(os.path.dirname(__file__), ".."))
+ if root_path not in sys.path:
+ sys.path.insert(0, root_path)
+
+ from scripts.verify_coverage import main
+ with pytest.raises(SystemExit) as excinfo:
+ main()
+ assert excinfo.value.code == 1📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_coverage_script_passes(tmp_path, monkeypatch): | |
| # Mock coverage.json file | |
| json_path = tmp_path / "coverage.json" | |
| dummy_data = { | |
| "totals": { | |
| "percent_covered": 75.5 | |
| } | |
| } | |
| with open(json_path, "w") as f: | |
| json.dump(dummy_data, f) | |
| monkeypatch.chdir(tmp_path) | |
| # Monkeypatch subprocess to mock the raw coverage call | |
| monkeypatch.setattr(subprocess, "run", lambda *args, **kwargs: None) | |
| # Ensure root path is in sys.path | |
| root_path = os.path.abspath(os.path.join(os.path.dirname(__file__), "..")) | |
| if root_path not in sys.path: | |
| sys.path.insert(0, root_path) | |
| from scripts.verify_coverage import main | |
| with pytest.raises(SystemExit) as excinfo: | |
| main() | |
| assert excinfo.value.code == 0 | |
| def test_coverage_script_passes(tmp_path, monkeypatch): | |
| # Mock coverage.json file | |
| json_path = tmp_path / "coverage.json" | |
| dummy_data = { | |
| "totals": { | |
| "percent_covered": 75.5 | |
| } | |
| } | |
| with open(json_path, "w") as f: | |
| json.dump(dummy_data, f) | |
| monkeypatch.chdir(tmp_path) | |
| # Monkeypatch subprocess to mock the raw coverage call | |
| monkeypatch.setattr(subprocess, "run", lambda *args, **kwargs: None) | |
| # Ensure root path is in sys.path | |
| root_path = os.path.abspath(os.path.join(os.path.dirname(__file__), "..")) | |
| if root_path not in sys.path: | |
| sys.path.insert(0, root_path) | |
| from scripts.verify_coverage import main | |
| with pytest.raises(SystemExit) as excinfo: | |
| main() | |
| assert excinfo.value.code == 0 | |
| def test_coverage_script_fails(tmp_path, monkeypatch): | |
| json_path = tmp_path / "coverage.json" | |
| dummy_data = {"totals": {"percent_covered": 50.0}} | |
| with open(json_path, "w") as f: | |
| json.dump(dummy_data, f) | |
| monkeypatch.chdir(tmp_path) | |
| monkeypatch.setattr(subprocess, "run", lambda *args, **kwargs: None) | |
| root_path = os.path.abspath(os.path.join(os.path.dirname(__file__), "..")) | |
| if root_path not in sys.path: | |
| sys.path.insert(0, root_path) | |
| from scripts.verify_coverage import main | |
| with pytest.raises(SystemExit) as excinfo: | |
| main() | |
| assert excinfo.value.code == 1 |
🧰 Tools
🪛 ast-grep (0.44.0)
[warning] 19-19: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(json_path, "w")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_coverage_guard.py` around lines 12 - 35, Add a test in
test_coverage_guard.py that covers the failing-threshold path in
verify_coverage.main, alongside test_coverage_script_passes. Mock coverage.json
with percent_covered below the 70% threshold, keep subprocess.run patched as in
the passing test, and assert that main raises SystemExit with exit code 1. Use
the existing main entry point and the coverage.json setup so the new test
exercises the same script behavior.
1 participant
Summary
Integrates a robust CI/CD pipeline step using GitHub Actions to perform static security scans with Bandit and enforce minimum test coverage limits. This ensures that every Pull Request is thoroughly vetted for security vulnerabilities and code test coverage before merging.
Proposed Changes
Verification Plan
Verified that python -m pytest tests/test_coverage_guard.py executes and passes cleanly.
Summary by CodeRabbit
New Features
Documentation
Tests
Closes #518