Open
Conversation
Create a throwaway coverage virtual environment and install `slipcover`, `pytest`, and `coverage` into it with `uv pip --python`.
Avoid relying on the separate `uvx` shim being present on PATH when running formatting and lint targets from automation.
Prefer the common user-local install locations for `uv` and `action-validator` so automation with a narrow PATH can still run the format, lint, and typecheck targets.
Contributor
OverviewThis pull request changes how Python coverage tooling is executed by creating and using a short‑lived, job‑local virtual environment (.venv-coverage) instead of relying on system interpreters or per‑invocation Key technical changes
Commits and notable fixes
Code‑quality and API notes
Validation
Design choices
WalkthroughCreate and reuse a dedicated Changes
Sequence Diagram(s)sequenceDiagram
participant Script as run_python.py
participant UV as uv
participant Venv as .venv-coverage
participant Tooling as slipcover/pytest/coverage
Script->>Venv: Query cached Python (_coverage_python_cmd/_find_coverage_python)
alt missing or invalid
Script->>UV: Create or repair venv (`uv venv .venv-coverage`)
UV-->>Script: venv created
Script->>UV: Sync deps (`uv sync --inexact --python <venv_python>`)
UV-->>Script: deps synced
Script->>UV: Install tooling (`uv pip install --python <venv_python> slipcover pytest coverage`)
UV-->>Script: tooling installed
else cached valid
Venv-->>Script: Return cached Python
end
Script->>Tooling: Run `python -m slipcover` / `python -m pytest` / `python -m coverage` via cached venv Python
Tooling-->>Script: Emit coverage data / reports
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (3 errors, 5 warnings)
✅ Passed checks (10 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
Reviewer's GuideRun Python coverage generation inside an isolated, lazily-created .venv-coverage virtualenv instead of Sequence diagram for Python coverage venv setup and reusesequenceDiagram
actor GitHubActions
participant generate_coverage_action
participant run_python_script
participant coverage_python_cmd as _coverage_python_cmd
participant create_venv
participant install_tools as install_coverage_tools
participant uv_cli as uv
participant slipcover_pytest_coverage
GitHubActions->>generate_coverage_action: run Python coverage step
generate_coverage_action->>run_python_script: invoke run_python.py
Note over run_python_script,coverage_python_cmd: First coverage invocation
run_python_script->>coverage_python_cmd: request coverage Python
coverage_python_cmd->>create_venv: create_venv()
alt venv_does_not_exist
create_venv->>uv_cli: venv .venv-coverage
uv_cli-->>create_venv: venv created
else venv_exists
create_venv-->>create_venv: reuse existing venv
end
create_venv-->>coverage_python_cmd: venv_python_path
coverage_python_cmd->>install_tools: install_coverage_tools(venv_python_path)
install_tools->>uv_cli: pip install --python venv_python slipcover pytest coverage
uv_cli-->>install_tools: tools installed
install_tools-->>coverage_python_cmd: success
coverage_python_cmd-->>run_python_script: BoundCommand for venv_python
run_python_script->>slipcover_pytest_coverage: run slipcover/pytest/coverage via venv_python
slipcover_pytest_coverage-->>run_python_script: coverage results
run_python_script-->>generate_coverage_action: Cobertura XML
generate_coverage_action-->>GitHubActions: upload coverage artefact
Note over run_python_script,coverage_python_cmd: Subsequent coverage invocation
GitHubActions->>generate_coverage_action: run Python coverage step again
generate_coverage_action->>run_python_script: invoke run_python.py
run_python_script->>coverage_python_cmd: request coverage Python
coverage_python_cmd-->>run_python_script: reuse cached BoundCommand
run_python_script->>slipcover_pytest_coverage: run tools via cached venv_python
slipcover_pytest_coverage-->>run_python_script: coverage results
Flow diagram for coverage venv lifecycle functionsflowchart TD
start[Start coverage tooling setup]
cmd[_coverage_python_cmd]
venv_check[Does .venv-coverage exist]
create[create_venv]
install[install_coverage_tools]
uv_venv[uv venv .venv-coverage]
detect_python[Locate venv python executable]
python_missing[Python executable missing]
recreate[Remove and recreate .venv-coverage]
uv_pip[uv pip install --python venv_python slipcover pytest coverage]
cache_cmd[Cache BoundCommand for venv python]
return_cmd[Return cached BoundCommand]
start --> cmd
cmd -->|_COVERAGE_PYTHON_CMD is None| create
cmd -->|_COVERAGE_PYTHON_CMD is set| return_cmd
create --> venv_check
venv_check -->|no| uv_venv
venv_check -->|yes| detect_python
uv_venv --> detect_python
detect_python -->|found| install
detect_python -->|not found| python_missing
python_missing --> recreate
recreate --> uv_venv
install --> uv_pip
uv_pip --> cache_cmd
cache_cmd --> return_cmd
return_cmd --> end_node[Coverage venv python ready]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The coverage venv Python path is hardcoded to
.venv-coverage / 'bin' / 'python', which assumes a POSIX layout; if this action might ever run on Windows or non-standard layouts it would be safer to derive the executable path viasysconfig/venvmetadata or by invokingpython -cwithin the created venv. - In
create_venv, you always runuv venvinto.venv-coveragewithout checking for its existence; if this script is called multiple times in the same workspace, a small guard (e.g.,if not COVERAGE_VENV.exists()) could avoid redundant work or potential conflicts from re-creating an existing environment.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The coverage venv Python path is hardcoded to `.venv-coverage / 'bin' / 'python'`, which assumes a POSIX layout; if this action might ever run on Windows or non-standard layouts it would be safer to derive the executable path via `sysconfig`/`venv` metadata or by invoking `python -c` within the created venv.
- In `create_venv`, you always run `uv venv` into `.venv-coverage` without checking for its existence; if this script is called multiple times in the same workspace, a small guard (e.g., `if not COVERAGE_VENV.exists()`) could avoid redundant work or potential conflicts from re-creating an existing environment.
## Individual Comments
### Comment 1
<location path=".github/actions/generate-coverage/tests/test_scripts.py" line_range="2023-2024" />
<code_context>
+ return local[python][_coverage_args(fmt, out)]
@contextlib.contextmanager
-def tmp_coveragepy_xml(out: Path) -> cabc.Generator[Path]:
+def tmp_coveragepy_xml(out: Path, python: str) -> cabc.Generator[Path]:
</code_context>
<issue_to_address>
**suggestion (testing):** Capture and assert the `python` argument passed to `tmp_coveragepy_xml` in the coveragepy path to ensure it matches the venv interpreter.
The tests that monkeypatch `tmp_coveragepy_xml` still ignore the new `python: str` parameter, so they no longer verify that `main()` passes the venv interpreter through. To preserve regression coverage, have the fake `tmp_coveragepy_xml` capture the `_python` argument and assert it matches `_coverage_python(run_python_module)` (or the `create_venv` return value), and optionally assert that non-coveragepy formats never invoke `tmp_coveragepy_xml`. This ensures the coverage XML export remains tied to the throwaway venv interpreter rather than a system Python.
</issue_to_address>
### Comment 2
<location path=".github/actions/generate-coverage/scripts/run_python.py" line_range="67" />
<code_context>
-def coverage_cmd_for_fmt(fmt: str, out: Path) -> BoundCommand:
+def coverage_cmd_for_fmt(fmt: str, out: Path, python: str) -> BoundCommand:
"""Return the slipcover command for the requested format."""
- python_cmd = _uv_python_cmd()
</code_context>
<issue_to_address>
**issue (complexity):** Consider reintroducing a single helper abstraction for a coverage-capable python command instead of passing a python string through multiple functions.
You can keep the throwaway venv behavior but restore a single, clear abstraction for “python with coverage tooling” and avoid threading `python: str` through unrelated functions.
Concretely:
1. **Introduce a `_coverage_python_cmd()` helper with lazy initialization**
```python
from plumbum import local
_COVERAGE_PYTHON_CMD: BoundCommand | None = None # module-level cache
def _coverage_python_cmd() -> BoundCommand:
"""Return a python command with coverage tooling available in a throwaway venv."""
global _COVERAGE_PYTHON_CMD
if _COVERAGE_PYTHON_CMD is None:
python = create_venv()
install_coverage_tools(python)
_COVERAGE_PYTHON_CMD = local[python]
return _COVERAGE_PYTHON_CMD
```
- Keeps venv creation + tooling install in one place.
- Ensures a single venv per process without re-creating it for `coveragepy` vs slipcover paths.
2. **Simplify `coverage_cmd_for_fmt` and `tmp_coveragepy_xml` back to “just build commands”**
```python
def coverage_cmd_for_fmt(fmt: str, out: Path) -> BoundCommand:
"""Return the slipcover command for the requested format."""
python_cmd = _coverage_python_cmd()
return python_cmd[_coverage_args(fmt, out)]
```
```python
@contextlib.contextmanager
def tmp_coveragepy_xml(out: Path) -> cabc.Generator[Path]:
"""Generate a cobertura XML from coverage.py and clean up afterwards."""
xml_tmp = out.with_suffix(".xml")
python_cmd = _coverage_python_cmd()
try:
cmd = python_cmd["-m", "coverage", "xml", "-o", str(xml_tmp)]
run_cmd(cmd)
except ProcessExecutionError as exc:
typer.echo(
f"coverage xml failed with code {exc.retcode}: {exc.stderr}",
err=True,
)
raise typer.Exit(code=exc.retcode or 1) from exc
try:
yield xml_tmp
finally:
xml_tmp.unlink(missing_ok=True)
```
3. **Keep `main` free of venv protocol knowledge**
With the above, `main` can go back to:
```python
out.parent.mkdir(parents=True, exist_ok=True)
cmd = coverage_cmd_for_fmt(fmt, out)
run_cmd(cmd, method="run_fg")
if fmt == "coveragepy":
with tmp_coveragepy_xml(out) as xml_tmp:
percent = get_line_coverage_percent_from_cobertura(xml_tmp)
Path(".coverage").replace(out)
else:
percent = get_line_coverage_percent_from_cobertura(out)
```
This preserves the new behavior (throwaway venv for coverage tooling) while:
- Eliminating the fragile `create_venv` + `install_coverage_tools` call sequence from callers.
- Removing the need to thread a `python: str` through multiple functions.
- Restoring a single, cohesive abstraction for “coverage-capable python”.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c6669c4d08
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![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: 3
🤖 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/actions/generate-coverage/scripts/run_python.py:
- Around line 46-54: The create_venv function currently returns COVERAGE_VENV /
"bin" / "python" which is not cross-platform; update create_venv to detect the
scripts directory for the current platform (e.g., use sys.platform or
sysconfig.get_path("scripts")) and return COVERAGE_VENV / "Scripts" /
"python.exe" on Windows and COVERAGE_VENV / "bin" / "python" on POSIX so the
returned python path works on all OSes; reference the create_venv function and
COVERAGE_VENV constant when making the change.
In @.github/actions/generate-coverage/tests/test_scripts.py:
- Line 2024: The function signature for fake_tmp_coveragepy_xml should mark the
unused parameter as intentionally unused: rename the parameter out to _out in
the def of fake_tmp_coveragepy_xml and update any internal references (if any)
to use _out; leave the function body and behavior unchanged so the stub still
writes to the fixed path but the linter no longer complains.
- Line 2070: The parameter naming doesn't follow the unused-parameter underscore
convention in fake_tmp_coveragepy_xml; rename the parameter out to _out (and
update any internal references) so unused arguments are clearly marked, i.e.,
change def fake_tmp_coveragepy_xml(out: Path, _python: str) to def
fake_tmp_coveragepy_xml(_out: Path, _python: str) and adjust usages within the
function body accordingly.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 126101e1-a4ee-429a-b9a7-c7b595beb5bc
📒 Files selected for processing (3)
.github/actions/generate-coverage/scripts/run_python.py.github/actions/generate-coverage/tests/test_scripts.pyMakefile
Cache the coverage-capable Python command behind one helper, reuse an existing coverage venv when present, and resolve the venv interpreter across POSIX and Windows layouts. Extend the run_python tests to cover cached command reuse and the coverage.py export path using the same venv interpreter.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/actions/generate-coverage/tests/test_scripts.py (1)
2106-2106:⚠️ Potential issue | 🟡 MinorRename intentionally unused stub parameters with an underscore prefix.
Apply the project’s unused-argument convention at both locations. Use
_outto silence lint noise and keep intent explicit.🔧 Proposed fix
- def fake_tmp_coveragepy_xml(out: Path) -> typ.Iterator[Path]: + def fake_tmp_coveragepy_xml(_out: Path) -> typ.Iterator[Path]: captured_python.append( next(iter(run_python_module._coverage_python_cmd().formulate())) ) xml_path = tmp_path / "coverage.xml" @@ - def fake_tmp_coveragepy_xml(out: Path) -> typ.Iterator[Path]: + def fake_tmp_coveragepy_xml(_out: Path) -> typ.Iterator[Path]: captured_python.append( next(iter(run_python_module._coverage_python_cmd().formulate())) ) xml_path = tmp_path / "coverage.xml"Also applies to: 2160-2160
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/actions/generate-coverage/tests/test_scripts.py at line 2106, Rename the intentionally unused parameter named out to _out in the test helper function fake_tmp_coveragepy_xml and in the other test helper stub in this file that currently accepts an unused out parameter; update the function signatures to use _out and adjust any internal references accordingly to follow the project's unused-argument convention and silence lint warnings.
🤖 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/actions/generate-coverage/scripts/run_python.py:
- Around line 62-67: The create_venv function currently only recreates the
virtualenv when COVERAGE_VENV directory is absent, but fails if the directory
exists without a valid interpreter; update create_venv to detect a missing or
invalid Python at _coverage_python_path() and in that case delete or recreate
COVERAGE_VENV (e.g., remove directory or call the same run_cmd(uv["venv",
str(COVERAGE_VENV)]) flow) before returning the python path, so the function
ensures a working interpreter even when the venv dir is present but broken.
---
Duplicate comments:
In @.github/actions/generate-coverage/tests/test_scripts.py:
- Line 2106: Rename the intentionally unused parameter named out to _out in the
test helper function fake_tmp_coveragepy_xml and in the other test helper stub
in this file that currently accepts an unused out parameter; update the function
signatures to use _out and adjust any internal references accordingly to follow
the project's unused-argument convention and silence lint warnings.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 9cef7b30-7a9d-4570-a36b-3c0e1a320cfd
📒 Files selected for processing (2)
.github/actions/generate-coverage/scripts/run_python.py.github/actions/generate-coverage/tests/test_scripts.py
Owner
Author
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. .github/actions/generate-coverage/tests/test_scripts.py Comment on file def _get_coverage_cmd_parts(
tmp_path: Path,
run_python_module: ModuleType,
monkeypatch: pytest.MonkeyPatch,❌ New issue: Excess Number of Function Arguments |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Repair `.venv-coverage` when the directory exists without a usable Python interpreter, so coverage setup can recover from incomplete or stale venvs. Reject non-file interpreter placeholders before reusing an existing venv.
This comment was marked as resolved.
This comment was marked as resolved.
Introduce `CoverageFmtSpec` so the coverage command test helper stays within the argument-count limit while preserving the same test behaviour.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Rename unused `fake_tmp_coveragepy_xml` parameters in coveragepy tests so the stubs clearly follow the unused-argument convention.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Owner
Author
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Ensure that this is validated against the current version of the codegraph. If further refinement to address this finding would be deleterious, please supply a clear explanatory one to two paragraph markdown message I can paste into the CodeScene web ui's diagnostic suppression function so this diagnostic can be silenced. Code Duplication.github/actions/generate-coverage/tests/test_scripts.py: What lead to degradation?introduced similar code in: test_create_venv_recreates_broken_coverage_venv,test_create_venv_recreates_invalid_python_candidate,test_create_venv_returns_coverage_python Why does this problem occur?Duplicated code often leads to code that's harder to change since the same logical change has to be done in multiple functions. More duplication gives lower code health. How to fix it?A certain degree of duplicated code might be acceptable. The problems start when it is the same behavior that is duplicated across the functions in the module, ie. a violation of the Don't Repeat Yourself (DRY) principle. DRY violations lead to code that is changed together in predictable patterns, which is both expensive and risky. DRY violations can be identified using CodeScene's X-Ray analysis to detect clusters of change coupled functions with high code similarity. Read More |
This comment was marked as resolved.
This comment was marked as resolved.
Add observable setup and reuse logging around the coverage venv, expand helper docstrings, and document the venv lifecycle and singleton model. Cover broken-cache recovery and missing interpreter detection in the run_python tests.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Resolve `markdownlint` from the Bun install path when automation runs with a restricted `PATH`, and lint branch-changed Markdown files before falling back to the full tree. Keep the coverage design API table under local Markdown lint disables so the requested table remains readable without surfacing unrelated repository-wide Markdown debt.
Owner
Author
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Complex Method.github/actions/generate-coverage/scripts/run_python.py: _ensure_coverage_venv What lead to degradation?_ensure_coverage_venv has a cyclomatic complexity of 9, threshold = 9 Why does this problem occur?A Complex Method has a high cyclomatic complexity. The recommended threshold for the Python language is a cyclomatic complexity lower than 9. How to fix it?There are many reasons for Complex Method. Sometimes, another design approach is beneficial such as a) modeling state using an explicit state machine rather than conditionals, or b) using table lookup rather than long chains of logic. In other scenarios, the function can be split using EXTRACT FUNCTION. Just make sure you extract natural and cohesive functions. Complex Methods can also be addressed by identifying complex conditional expressions and then using the DECOMPOSE CONDITIONAL refactoring. Helpful refactoring examplesTo get a general understanding of what this code health issue looks like - and how it might be addressed - we have prepared some diffs for illustrative purposes. SAMPLE# complex_method.js
function postItem(item) {
if (!item.id) {
- if (item.x != null && item.y != null) {
- post(item);
- } else {
- throw Error("Item must have x and y");
- }
+ // extract a separate function for creating new item
+ postNew(item);
} else {
- if (item.x < 10 && item.y > 25) {
- put(item);
- } else {
- throw Error("Item must have an x and y value between 10 and 25");
- }
+ // and one for updating existing items
+ updateItem(item);
}
}
+
+function postNew(item) {
+ validateNew(item);
+ post(item);
+}
+
+function updateItem(item) {
+ validateUpdate(item);
+ put(item);
+}
+ |
Add regressions for coverage venv creation that leaves no interpreter and for symlink cache repair, and keep markdownlint changed-file failures from being masked by the per-file pipeline. Update the coverage venv design wording to use the requested installation noun form.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Store formulated command arguments before recording them and guard the uv venv check by argument length so short command lists cannot raise IndexError.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Change the lint target to invoke action-validator once per action YAML file so its single-path CLI contract is respected.
Owner
Author
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Ensure that this is validated against the current version of the codegraph. If further refinement to address this finding would be deleterious, please supply a clear explanatory one to two paragraph markdown message I can paste into the CodeScene web ui's diagnostic suppression function so this diagnostic can be silenced. .github/actions/generate-coverage/tests/test_scripts.py Comment on file captured_python: list[str] = []
@contextlib.contextmanager
def fake_tmp_coveragepy_xml(out: Path) -> typ.Iterator[Path]:❌ Getting worse: Code Duplication |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 1894
File: AnalysisThe CodeScene finding references a stale diff commit ( assert python == str(setup.coverage_venv / "bin" / "python")
_assert_venv_rebuild_commands(
setup.recorded,
setup.coverage_venv,
setup.coverage_venv / "bin" / "python",
)The path FixAdd def _assert_venv_default_python_rebuild(
python: str,
setup: VenvTestSetup,
) -> None:
"""Assert venv was rebuilt and Python resolved to the default POSIX path."""
expected = setup.coverage_venv / "bin" / "python"
assert python == str(expected)
_assert_venv_rebuild_commands(setup.recorded, setup.coverage_venv, expected)Rewrite def test_ensure_coverage_venv_replaces_broken_symlink_cache(
tmp_path: Path,
run_python_module: ModuleType,
monkeypatch: pytest.MonkeyPatch,
) -> None:
"""The helper unlinks a symlink cache placeholder before recreate."""
setup = _setup_coverage_venv_test(tmp_path, run_python_module, monkeypatch)
target = tmp_path / "not-a-venv"
target.write_text("not a directory")
setup.coverage_venv.symlink_to(target)
python = run_python_module._ensure_coverage_venv()
assert not setup.coverage_venv.is_symlink()
_assert_venv_default_python_rebuild(python, setup)Rewrite def test_ensure_coverage_venv_replaces_broken_file_cache(
tmp_path: Path,
run_python_module: ModuleType,
monkeypatch: pytest.MonkeyPatch,
) -> None:
"""The helper replaces a non-directory cache placeholder before recreate."""
setup = _setup_coverage_venv_test(tmp_path, run_python_module, monkeypatch)
setup.coverage_venv.write_text("not a venv")
python = run_python_module._ensure_coverage_venv()
_assert_venv_default_python_rebuild(python, setup)AI coding agent prompt |
Split coverage venv removal and recreation out of _ensure_coverage_venv so the setup flow stays below the complexity threshold while preserving existing behaviour.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/actions/generate-coverage/tests/test_scripts.py:
- Around line 1855-1856: The assertions comparing the executable name use
Path(...).name which fails on Windows due to a .exe extension; update the checks
in test_scripts.py where venv_parts is asserted (e.g., the assertions
referencing venv_parts[0] and venv_parts[1:]) to compare Path(...).stem == "uv"
(and similarly replace .name with .stem) so the tests normalize the executable
name across platforms; apply the same change at the other occurrences around the
sections mentioned (the groups near lines referenced as 1943 and 2056).
In `@docs/generate-coverage-design.md`:
- Around line 194-195: Update the design note to match the implementation:
change the phrase that says the helper "removes the directory and recreates it"
to describe that it removes the existing path (including unlinking broken
symlinks and removing file placeholders) and then recreates the virtualenv, so
the docs for generate-coverage-design.md accurately reflect the behavior
implemented in run_python.py.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: cb7912d7-6eac-4610-be29-ef9b5587b1a1
📒 Files selected for processing (3)
.github/actions/generate-coverage/tests/test_scripts.pyMakefiledocs/generate-coverage-design.md
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 `@Makefile`:
- Around line 72-81: The markdownlint Makefile target body is too long and
should be moved into a small shell script to satisfy the style rule; create a
script (e.g., scripts/markdownlint-check.sh) that implements the current
conditional using the same variables/logic (use MARKDOWNLINT_BASE, MDLINT and
the git diff fallback and diagnostic echo behavior), make the Makefile
markdownlint target call that script with the same environment variables, and
ensure the script preserves exit codes and the diagnostic echo on fallback so
behavior remains identical.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 01a2c3e5-6ae6-4aa4-9bf7-ac3dda70f774
📒 Files selected for processing (3)
.github/actions/generate-coverage/scripts/run_python.py.github/actions/generate-coverage/tests/test_scripts.pyMakefile
Replace the long Makefile markdownlint recipe with a call to a small shell script that preserves the existing diff-based linting, fallback warning, and exit-code behaviour.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Makefile`:
- Line 17: The ACTION_VALIDATOR default selection currently checks ~/.cargo/bin
before the system lookup, which conflicts with CI that installs via Bun; change
the Makefile's ACTION_VALIDATOR assignment to probe ~/.bun/bin first, then
~/.cargo/bin, then fall back to the plain action-validator command (use a
firstword over wildcard results) so the CI-provided Bun-installed binary is
chosen when present. Update the ACTION_VALIDATOR variable assignment
accordingly.
In `@workflow_scripts/markdownlint-check.sh`:
- Around line 11-21: The MDLINT variable is unquoted at each invocation which
can cause word-splitting; update every call to use a quoted variable. Replace
${MDLINT} "${file}" with "${MDLINT}" "${file}", replace -exec ${MDLINT} {} +
with -exec "${MDLINT}" {} +, and likewise quote the MDLINT in the other find ...
-exec invocation so all three usages use "${MDLINT}" to prevent argv drift.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 77a28bdf-f75b-4e4c-ad0f-d66b479ca3dc
📒 Files selected for processing (2)
Makefileworkflow_scripts/markdownlint-check.sh
Owner
Author
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Ensure that this is validated against the current version of the codegraph. If further refinement to address this finding would be deleterious, please supply a clear explanatory one to two paragraph markdown message I can paste into the CodeScene web ui's diagnostic suppression function so this diagnostic can be silenced. Code Duplication.github/actions/generate-coverage/tests/test_scripts.py: What lead to degradation?introduced similar code in: test_ensure_coverage_venv_replaces_broken_file_cache,test_ensure_coverage_venv_replaces_broken_symlink_cache Why does this problem occur?Duplicated code often leads to code that's harder to change since the same logical change has to be done in multiple functions. More duplication gives lower code health. How to fix it?A certain degree of duplicated code might be acceptable. The problems start when it is the same behavior that is duplicated across the functions in the module, ie. a violation of the Don't Repeat Yourself (DRY) principle. DRY violations lead to code that is changed together in predictable patterns, which is both expensive and risky. DRY violations can be identified using CodeScene's X-Ray analysis to detect clusters of change coupled functions with high code similarity. Read More |
This comment was marked as resolved.
This comment was marked as resolved.
Choose the Bun-installed action-validator before Cargo or PATH so CI uses its provided binary, and quote markdownlint invocations in the helper script to avoid shell word splitting.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
workflow_scripts/markdownlint-check.sh (1)
17-21:⚠️ Potential issue | 🟠 MajorPropagate markdownlint failures in fallback branches.
Replace the fallback
find ... -exec "${MDLINT}" {} +on Line 17 and Line 21 with explicit status aggregation so lint failures cannot be masked when diff detection is empty or fails.Patch
MARKDOWNLINT_BASE="${MARKDOWNLINT_BASE:-origin/main}" MDLINT="${MDLINT:-markdownlint}" +lint_all_markdown() { + status=0 + while IFS= read -r file; do + [ -n "${file}" ] || continue + "${MDLINT}" "${file}" || status=1 + done <<EOF +$(find . -type f -name '*.md' -not -path './target/*') +EOF + return "${status}" +} + if files=$(git diff --name-only --diff-filter=ACMRT "${MARKDOWNLINT_BASE}...HEAD" -- '*.md' 2>/dev/null); then if [ -n "${files}" ]; then status=0 @@ - find . -type f -name '*.md' -not -path './target/*' -exec "${MDLINT}" {} + + lint_all_markdown fi else echo "markdownlint: git diff failed or base '${MARKDOWNLINT_BASE}' not found; linting all .md files" >&2 - find . -type f -name '*.md' -not -path './target/*' -exec "${MDLINT}" {} + + lint_all_markdown fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workflow_scripts/markdownlint-check.sh` around lines 17 - 21, The fallback branches currently run find ... -exec "${MDLINT}" {} + which hides individual lint failures; change both fallback blocks to collect file paths (using the same find criteria) and iterate running "${MDLINT}" on each file, accumulating an exit status (e.g., keep a global ret=0 and set ret=1 if any run fails) and then exit with that aggregated status so failures propagate; update the occurrences referencing MDLINT and MARKDOWNLINT_BASE and replace the inline -exec invocations with this per-file invocation + status aggregation logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@workflow_scripts/markdownlint-check.sh`:
- Around line 17-21: The fallback branches currently run find ... -exec
"${MDLINT}" {} + which hides individual lint failures; change both fallback
blocks to collect file paths (using the same find criteria) and iterate running
"${MDLINT}" on each file, accumulating an exit status (e.g., keep a global ret=0
and set ret=1 if any run fails) and then exit with that aggregated status so
failures propagate; update the occurrences referencing MDLINT and
MARKDOWNLINT_BASE and replace the inline -exec invocations with this per-file
invocation + status aggregation logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2add9514-44e6-41e8-81e0-6835526d6e16
📒 Files selected for processing (2)
Makefileworkflow_scripts/markdownlint-check.sh
Add a helper for tests that rebuild the coverage venv to the default POSIX Python path, removing duplicated assertions from file and symlink cache cases.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
.github/actions/generate-coverage/tests/test_scripts.py (3)
1854-1856:⚠️ Potential issue | 🟡 MinorUse
.steminstead of.namefor cross-platform executable assertions.Line 1855's assertion
Path(venv_parts[0]).name == "uv"fails on Windows where the executable isuv.exe.🛠️ Proposed fix
venv_parts = setup.recorded[0] - assert Path(venv_parts[0]).name == "uv" + assert Path(venv_parts[0]).stem == "uv" assert venv_parts[1:] == ["venv", str(setup.coverage_venv)]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/actions/generate-coverage/tests/test_scripts.py around lines 1854 - 1856, The assertion using Path(venv_parts[0]).name is brittle on Windows because executables include extensions; update the check to use Path(venv_parts[0]).stem instead so the first element comparison (venv_parts[0]) normalizes exe suffixes, keeping the rest of the assertion venv_parts[1:] == ["venv", str(setup.coverage_venv)] unchanged; locate this in the test referencing setup.recorded and venv_parts.
2055-2059:⚠️ Potential issue | 🟡 MinorUse
.steminstead of.namefor cross-platform executable assertions.Line 2056's assertion
Path(parts[0]).name == "uv"fails on Windows.🛠️ Proposed fix
parts = setup.recorded[1] - assert Path(parts[0]).name == "uv" + assert Path(parts[0]).stem == "uv" assert parts[1:5] == ["pip", "install", "--python", str(python)]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/actions/generate-coverage/tests/test_scripts.py around lines 2055 - 2059, The assertion that Path(parts[0]).name == "uv" is brittle on Windows; change it to use Path(parts[0]).stem to compare the executable base name instead. Update the assertion in the test where parts is taken from setup.recorded[1] (the block referencing parts[0], parts[1:5], and run_python_module.TOOLING_PACKAGES) so it asserts Path(parts[0]).stem == "uv" while leaving the other assertions intact.
1942-1944:⚠️ Potential issue | 🟡 MinorUse
.steminstead of.namefor cross-platform executable assertions.Line 1943's assertion
Path(setup.recorded[0][0]).name == "uv"fails on Windows.🛠️ Proposed fix
assert len(setup.recorded) == 1 - assert Path(setup.recorded[0][0]).name == "uv" + assert Path(setup.recorded[0][0]).stem == "uv" assert setup.recorded[0][1:] == ["venv", str(setup.coverage_venv)]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/actions/generate-coverage/tests/test_scripts.py around lines 1942 - 1944, Change the cross-platform executable assertion to compare the stem instead of the filename: instead of using Path(setup.recorded[0][0]).name use Path(setup.recorded[0][0]).stem when asserting against "uv"; update the assertion that references setup.recorded (the second assertion) so it uses .stem to avoid Windows extension differences while keeping the rest of the check (setup.recorded[0][1:] == ["venv", str(setup.coverage_venv)]) unchanged.
🤖 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/actions/generate-coverage/tests/test_scripts.py:
- Around line 1765-1776: Change platform-sensitive filename comparisons to use
Path.stem instead of Path.name: in _assert_python_command_structure replace
Path(parts[0]).name == "python" with checking Path(parts[0]).stem == "python" so
Windows "python.exe" passes; and in _assert_coverage_python_path compare the
final path components by stem (e.g., Path(actual).stem == Path(expected).stem or
compare the last path parts with stems) so the coverage venv python executable
matches across platforms. Ensure these edits target the
_assert_python_command_structure and _assert_coverage_python_path functions.
---
Duplicate comments:
In @.github/actions/generate-coverage/tests/test_scripts.py:
- Around line 1854-1856: The assertion using Path(venv_parts[0]).name is brittle
on Windows because executables include extensions; update the check to use
Path(venv_parts[0]).stem instead so the first element comparison (venv_parts[0])
normalizes exe suffixes, keeping the rest of the assertion venv_parts[1:] ==
["venv", str(setup.coverage_venv)] unchanged; locate this in the test
referencing setup.recorded and venv_parts.
- Around line 2055-2059: The assertion that Path(parts[0]).name == "uv" is
brittle on Windows; change it to use Path(parts[0]).stem to compare the
executable base name instead. Update the assertion in the test where parts is
taken from setup.recorded[1] (the block referencing parts[0], parts[1:5], and
run_python_module.TOOLING_PACKAGES) so it asserts Path(parts[0]).stem == "uv"
while leaving the other assertions intact.
- Around line 1942-1944: Change the cross-platform executable assertion to
compare the stem instead of the filename: instead of using
Path(setup.recorded[0][0]).name use Path(setup.recorded[0][0]).stem when
asserting against "uv"; update the assertion that references setup.recorded (the
second assertion) so it uses .stem to avoid Windows extension differences while
keeping the rest of the check (setup.recorded[0][1:] == ["venv",
str(setup.coverage_venv)]) unchanged.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 522aaafd-d20f-436b-9c6f-131abb6e7f43
📒 Files selected for processing (1)
.github/actions/generate-coverage/tests/test_scripts.py
Compare executable stems instead of full file names in coverage command tests so Windows .exe suffixes do not break path assertions.
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/actions/generate-coverage/tests/test_scripts.py:
- Around line 2151-2173: Combine the two duplicate tests
test_coverage_cmd_default_branch_has_shared_args and
test_non_cobertura_formats_do_not_emit_out_flag into a single parametrized test
(or a single test) that calls _get_coverage_cmd_parts once with
CoverageFmtSpec("coveragepy", "dat") and asserts all three conditions: "--xml"
not in parts, str(out) not in parts, and "--out" not in parts; update the test
name to something like test_non_cobertura_formats_do_not_emit_cobertura_flags
and remove the redundant test, ensuring you still accept the same fixtures
(tmp_path, run_python_module, monkeypatch) and reference _get_coverage_cmd_parts
and CoverageFmtSpec in the new test.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 08cf8053-3367-4eec-bf59-da8f4f6a8f23
📒 Files selected for processing (1)
.github/actions/generate-coverage/tests/test_scripts.py
Comment on lines
2151
to
2173
| def test_coverage_cmd_default_branch_has_shared_args( | ||
| tmp_path: Path, run_python_module: ModuleType | ||
| tmp_path: Path, | ||
| run_python_module: ModuleType, | ||
| monkeypatch: pytest.MonkeyPatch, | ||
| ) -> None: | ||
| """Non-Cobertura formats reuse the shared slipcover arguments.""" | ||
| parts, out = _get_coverage_cmd_parts( | ||
| tmp_path, run_python_module, "coveragepy", "dat" | ||
| tmp_path, run_python_module, monkeypatch, CoverageFmtSpec("coveragepy", "dat") | ||
| ) | ||
| assert "--xml" not in parts | ||
| assert str(out) not in parts | ||
|
|
||
|
|
||
| def test_non_cobertura_formats_do_not_emit_out_flag( | ||
| tmp_path: Path, run_python_module: ModuleType | ||
| tmp_path: Path, | ||
| run_python_module: ModuleType, | ||
| monkeypatch: pytest.MonkeyPatch, | ||
| ) -> None: | ||
| """Ensure slipcover output targeting stays isolated to Cobertura runs.""" | ||
| parts, _ = _get_coverage_cmd_parts(tmp_path, run_python_module, "coveragepy", "dat") | ||
| parts, _ = _get_coverage_cmd_parts( | ||
| tmp_path, run_python_module, monkeypatch, CoverageFmtSpec("coveragepy", "dat") | ||
| ) | ||
| assert "--out" not in parts |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Merge overlapping non-Cobertura tests into one parameterized assertion.
Collapse these two tests into one. They execute the same setup path and duplicate intent around non-Cobertura output flags.
♻️ Proposed simplification
def test_coverage_cmd_default_branch_has_shared_args(
tmp_path: Path,
run_python_module: ModuleType,
monkeypatch: pytest.MonkeyPatch,
) -> None:
"""Non-Cobertura formats reuse the shared slipcover arguments."""
parts, out = _get_coverage_cmd_parts(
tmp_path, run_python_module, monkeypatch, CoverageFmtSpec("coveragepy", "dat")
)
assert "--xml" not in parts
+ assert "--out" not in parts
assert str(out) not in parts
-
-
-def test_non_cobertura_formats_do_not_emit_out_flag(
- tmp_path: Path,
- run_python_module: ModuleType,
- monkeypatch: pytest.MonkeyPatch,
-) -> None:
- """Ensure slipcover output targeting stays isolated to Cobertura runs."""
- parts, _ = _get_coverage_cmd_parts(
- tmp_path, run_python_module, monkeypatch, CoverageFmtSpec("coveragepy", "dat")
- )
- assert "--out" not in parts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/actions/generate-coverage/tests/test_scripts.py around lines 2151 -
2173, Combine the two duplicate tests
test_coverage_cmd_default_branch_has_shared_args and
test_non_cobertura_formats_do_not_emit_out_flag into a single parametrized test
(or a single test) that calls _get_coverage_cmd_parts once with
CoverageFmtSpec("coveragepy", "dat") and asserts all three conditions: "--xml"
not in parts, str(out) not in parts, and "--out" not in parts; update the test
name to something like test_non_cobertura_formats_do_not_emit_cobertura_flags
and remove the redundant test, ensuring you still accept the same fixtures
(tmp_path, run_python_module, monkeypatch) and reference _get_coverage_cmd_parts
and CoverageFmtSpec in the new test.
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
This branch runs the
generate-coveragePython coverage phase from a short-lived virtual environment instead of relying onuv runor any system interpreter install path. The change avoids PEP 668 failures on GitHub-hosted Ubuntu runners where Python 3.12 is externally managed, while keeping coverage tooling isolated from the system interpreter.It also hardens the Makefile check targets so automation with a narrow
PATHcan still finduvandaction-validatorfrom their common user-local install locations.Review walkthrough
.venv-coveragecreation,uv pip install --pythontooling install, and venv Python reuse for slipcover, pytest, and coverage XML export.--systeminstalls.UVandACTION_VALIDATORcommand resolution used by local and hook-driven checks.Validation
python -c "import ast, sys; ast.parse(open('.github/actions/generate-coverage/scripts/run_python.py').read()); print('parse ok')": passeduv run --with typer --with packaging --with plumbum --with pyyaml pytest .github/actions/generate-coverage/tests/test_scripts.py -k 'coverage_cmd_uses_venv_python or tmp_coveragepy_xml_invokes_venv_python or run_python_cobertura_passes_out_flag' -v: passed, 3 selected tests passedmake check-fmt: passedmake typecheck: passedmake lint: passedmake test: passed, 668 passed and 90 skippedmake check-fmt lint typecheck: passedenv PATH=/usr/bin:/bin make check-fmt lint typecheck: passedNotes
The branch deliberately does not add
--break-system-packagesor restore a system install path. Coverage tooling is installed into.venv-coverageand invoked through that venv's Python interpreter.Summary by Sourcery
Run Python coverage tooling from an isolated, job-local virtual environment instead of via
uv run, and make local tooling resolution in the Makefile more robust.New Features:
.venv-coveragevirtual environment and helper functions to create it, install coverage tooling into it, and reuse its Python interpreter for all coverage commands.Bug Fixes:
uv run --withfor coverage, preventing failures on externally managed Python installations such as GitHub-hosted Ubuntu runners.Enhancements:
uv,action-validator, andmarkdownlintfrom common user-local install locations and to lint only changed Markdown files where possible.Documentation: