Conversation
Reviewer's GuideThis PR introduces a standalone shellstub utility for stubbing CLI tools and integrates behavioural tests for the generate-coverage scripts under a new test directory, updating pytest configuration and adding fixtures to leverage the stubs in subprocess-based tests. Class diagram for the new shellstub utilityclassDiagram
class Call {
+list~str~ argv
+str cwd
+str timestamp
}
class Variant {
+Sequence~str~ match
+str stdout = ""
+str stderr = ""
+int exit_code = 0
}
class StubManager {
-Path dir
-Path _calls_file
-dict~str, dict~ _specs
+__init__(dir_: Path)
+register(name: str, variants: list~dict~|None, stdout: str = "", stderr: str = "", exit_code: int = 0)
+calls_of(name: str) list~Call~
+env dict~str, str~
-_wrapper_source(name: str, spec: dict) str
}
StubManager --> Call
StubManager --> Variant
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Summary by CodeRabbit
Summary by CodeRabbit
WalkthroughThis update introduces a shell command stubbing framework ( Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Function
participant Stub as StubManager
participant Script as Coverage Script
participant Shell as Shell Stub
Test->>Stub: Register shell command stub(s)
Test->>Script: Run script with stubbed environment
Script->>Shell: Invoke shell command (e.g., cargo, uvx)
Shell->>Stub: Log call, match variant, return response
Script->>Test: Output result, write files, exit
Test->>Stub: Retrieve and assert logged calls
Possibly related PRs
Poem
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @leynos - I've reviewed your changes - here's some feedback:
- Rather than inlining the full JSON spec in the generated wrapper, consider writing the spec to a separate file and loading it in the stub to avoid complex escaping and improve maintainability.
- The tests hard-code calling
uv run --script; you might extract that into a configurable helper or fixture to avoid test failures if the CLI tool name or invocation changes. StubManager.envcurrently merges the entire host environment, which could introduce unintended variables—consider only injecting PATH and PYTHONPATH and clearing or isolating other env vars for more deterministic tests.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Rather than inlining the full JSON spec in the generated wrapper, consider writing the spec to a separate file and loading it in the stub to avoid complex escaping and improve maintainability.
- The tests hard-code calling `uv run --script`; you might extract that into a configurable helper or fixture to avoid test failures if the CLI tool name or invocation changes.
- `StubManager.env` currently merges the entire host environment, which could introduce unintended variables—consider only injecting PATH and PYTHONPATH and clearing or isolating other env vars for more deterministic tests.
## Individual Comments
### Comment 1
<location> `shellstub.py:53` </location>
<code_context>
+ out: list[Call] = []
+ if not self._calls_file.exists():
+ return out
+ for line in self._calls_file.read_text().splitlines():
+ rec = json.loads(line)
+ if rec["cmd"] == name:
+ out.append(Call(rec["argv"], rec["cwd"], rec["ts"]))
</code_context>
<issue_to_address>
No error handling for malformed JSON lines in calls file.
Add error handling around json.loads to skip or log malformed lines, ensuring the function continues processing valid entries.
</issue_to_address>
### Comment 2
<location> `shellstub.py:61` </location>
<code_context>
+
+ @property
+ def env(self) -> dict[str, str]:
+ return {
+ "PATH": f"{self.dir}{os.pathsep}{os.getenv('PATH', '')}",
+ "PYTHONPATH": os.getenv("PYTHONPATH", ""),
+ **os.environ,
+ }
+
</code_context>
<issue_to_address>
Order of environment variable merging may unintentionally override PATH and PYTHONPATH.
Merging os.environ last will overwrite the custom PATH and PYTHONPATH values. To preserve your changes, reverse the merge order or use a more explicit strategy.
</issue_to_address>
### Comment 3
<location> `shellstub.py:29` </location>
<code_context>
+ self._calls_file = self.dir / "calls.jsonl"
+ self._specs: dict[str, dict] = {}
+
+ def register(
+ self,
+ name: str,
</code_context>
<issue_to_address>
Consider writing the stub spec to a separate JSON file and using a static template for the wrapper script instead of embedding the entire spec in a generated f-string.
```suggestion
You don’t need to embed the entire JSON‐spec into a huge f-string at registration time. Instead, write out a small `<name>.json` and use a static template that the stub reads at runtime. You can then drop both the unused `Variant` class and the in-memory `_specs` dict entirely.
1) Define a module‐level template:
```python
_WRAPPER_TEMPLATE = """#!/usr/bin/env python3
import json, os, sys
import datetime as _dt
# load spec from JSON file
with open({spec_path!r}) as f:
spec = json.load(f)
# record invocation
rec = {{
"cmd": {cmd!r},
"argv": sys.argv[1:],
"cwd": os.getcwd(),
"ts": _dt.datetime.utcnow().isoformat() + 'Z'
}}
with open({calls_file!r}, "a") as fh:
json.dump(rec, fh); fh.write("\\n")
# pick matching variant
for var in spec["variants"]:
if var.get("match") is None or var["match"] == sys.argv[1:]:
sys.stdout.write(var.get("stdout", ""))
sys.stderr.write(var.get("stderr", ""))
sys.exit(var.get("exit_code", 0))
sys.exit(127)
"""
```
2) Simplify `StubManager.register` to write the spec file and plug in paths:
```python
def register(self, name: str, *, variants: list[dict] | None = None, …) -> None:
# default single‐variant
if variants is None:
variants = [{"match": None, "stdout": stdout, "stderr": stderr, "exit_code": exit_code}]
# write out spec JSON
spec_path = self.dir / f"{name}.json"
spec_path.write_text(json.dumps({"variants": variants}))
# render wrapper
wrapper = _WRAPPER_TEMPLATE.format(
spec_path=str(spec_path),
calls_file=str(self._calls_file),
cmd=name,
)
stub_path = self.dir / name
stub_path.write_text(wrapper)
stub_path.chmod(0o755)
```
3) Remove the in‐memory `self._specs` and the `Variant` dataclass entirely, since neither is used outside of generating the JSON file.
This keeps the same behavior while dramatically shrinking `_wrapper_source` and avoiding complex f-string embedding.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
.github/actions/generate-coverage/tests/conftest.py(1 hunks).github/actions/generate-coverage/tests/test_scripts.py(1 hunks)pytest.ini(1 hunks)shellstub.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`.github/actions/*/tests/**/*`: Contract tests must validate that declared inputs/outputs round-trip correctly
.github/actions/*/tests/**/*: Contract tests must validate that declared inputs/outputs round-trip correctly
📄 Source: CodeRabbit Inference Engine (AGENTS.md)
List of files the instruction was applied to:
.github/actions/generate-coverage/tests/conftest.py.github/actions/generate-coverage/tests/test_scripts.py
🧬 Code Graph Analysis (1)
.github/actions/generate-coverage/tests/conftest.py (1)
shellstub.py (1)
StubManager(22-90)
🪛 Ruff (0.11.9)
.github/actions/generate-coverage/tests/conftest.py
12-12: Missing type annotation for function argument tmp_path
(ANN001)
12-12: Missing type annotation for function argument monkeypatch
(ANN001)
17-17: No teardown in fixture shell_stubs, use return instead of yield
Replace yield with return
(PT022)
shellstub.py
4-4: sys imported but unused
Remove unused import: sys
(F401)
7-7: Import from collections.abc instead: Sequence
Import from collections.abc
(UP035)
23-23: Missing return type annotation for special method __init__
Add return type annotation: None
(ANN204)
39-39: Unnecessary dict() call (rewrite as a literal)
Rewrite as a literal
(C408)
.github/actions/generate-coverage/tests/test_scripts.py
9-9: subprocess call: check for execution of untrusted input
(S603)
11-11: Missing return type annotation for public function test_run_rust_success
Add return type annotation: None
(ANN201)
11-11: Missing type annotation for function argument tmp_path
(ANN001)
11-11: Missing type annotation for function argument shell_stubs
(ANN001)
42-42: Missing return type annotation for public function test_run_rust_failure
Add return type annotation: None
(ANN201)
42-42: Missing type annotation for function argument tmp_path
(ANN001)
42-42: Missing type annotation for function argument shell_stubs
(ANN001)
62-62: Missing return type annotation for public function test_merge_cobertura
Add return type annotation: None
(ANN201)
62-62: Missing type annotation for function argument tmp_path
(ANN001)
62-62: Missing type annotation for function argument shell_stubs
(ANN001)
71-71: Unnecessary dict() call (rewrite as a literal)
Rewrite as a literal
(C408)
71-71: Trailing comma missing
Add trailing comma
(COM812)
84-84: Assertion should be broken down into multiple parts
Break down assertion into multiple parts
(PT018)
86-86: Assertion should be broken down into multiple parts
Break down assertion into multiple parts
(PT018)
🪛 Pylint (3.3.7)
.github/actions/generate-coverage/tests/conftest.py
[convention] 1-1: Missing module docstring
(C0114)
[error] 9-9: Unable to import 'shellstub'
(E0401)
[convention] 9-9: Import "from shellstub import StubManager" should be placed at the top of the module
(C0413)
[convention] 12-12: Missing function or method docstring
(C0116)
[convention] 2-2: standard import "os" should be placed before third party import "pytest"
(C0411)
[convention] 3-3: standard import "sys" should be placed before third party import "pytest"
(C0411)
[convention] 4-4: standard import "pathlib.Path" should be placed before third party import "pytest"
(C0411)
shellstub.py
[convention] 1-1: Missing module docstring
(C0114)
[convention] 10-10: Missing class docstring
(C0115)
[convention] 16-16: Missing class docstring
(C0115)
[convention] 22-22: Missing class docstring
(C0115)
[convention] 29-29: Missing function or method docstring
(C0116)
[refactor] 29-29: Too many arguments (6/5)
(R0913)
[refactor] 39-39: Consider using '{"match": None, "stdout": stdout, "stderr": stderr, "exit_code": exit_code, ... }' instead of a call to 'dict'.
(R1735)
[convention] 49-49: Missing function or method docstring
(C0116)
[convention] 60-60: Missing function or method docstring
(C0116)
[warning] 4-4: Unused import sys
(W0611)
.github/actions/generate-coverage/tests/test_scripts.py
[convention] 87-87: Trailing newlines
(C0305)
[convention] 1-1: Missing module docstring
(C0114)
[convention] 7-7: Missing function or method docstring
(C0116)
[warning] 9-9: 'subprocess.run' used without explicitly defining the value for 'check'.
(W1510)
[convention] 11-11: Missing function or method docstring
(C0116)
[convention] 42-42: Missing function or method docstring
(C0116)
[convention] 62-62: Missing function or method docstring
(C0116)
[refactor] 71-71: Consider using '{"match": ['merge-cobertura', str(rust), str(py)], "stdout": '', ... }' instead of a call to 'dict'.
(R1735)
🪛 Flake8 (7.2.0)
shellstub.py
[error] 4-4: 'sys' imported but unused
(F401)
🔇 Additional comments (3)
pytest.ini (1)
1-2: LGTM! Clean pytest configuration.The configuration correctly restricts test discovery to the
.github/actionsdirectory, which aligns with the new test infrastructure being added.shellstub.py (1)
67-90: No code-injection risk in_wrapper_source
The f-string embedsspecviajson.dumpsand quotesname/calls_filewithrepr, so untrusted data is inserted only as JSON or string literals—there’s noevalof user input. You can ignore the earlier security warning.Likely an incorrect or invalid review comment.
.github/actions/generate-coverage/tests/test_scripts.py (1)
62-87: No changes needed – input/output contract is fully tested.The existing test in
.github/actions/generate-coverage/tests/test_scripts.pyalready:
- Sets
RUST_FILE,PYTHON_FILEandOUTPUT_PATHvia environment variables- Stubs and asserts the
uvx merge-cobertura <rust> <python>invocation- Verifies a zero exit code
- Confirms the merged output is written to
OUTPUT_PATH- Confirms the input files are removed post-merge
All declared inputs/outputs are round-tripped and validated according to the guidelines.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (8)
shellstub.py (8)
1-1: Add module docstring.The module lacks documentation explaining its purpose as a shell command stubbing framework for testing.
7-7: ImportSequencefromcollections.abcfor modern Python compatibility.-from typing import Sequence +from collections.abc import Sequence
9-21: Add docstrings to dataclasses for better maintainability.The
CallandVariantdataclasses lack documentation explaining their purpose and attributes.
22-28: Add class docstring and return type annotation.The
StubManagerclass lacks documentation and the__init__method is missing a return type annotation.
29-39: Add docstring and use dictionary literal.The method lacks documentation and uses
dict()instead of a literal.
51-51: Add method documentation.The method lacks a docstring explaining its purpose and return value.
55-56: Add error handling for malformed JSON.No error handling around
json.loadswhich could crash on malformed lines.
61-67: Fix environment variable merging order and add documentation.The property lacks documentation and the merging order will override custom PATH values.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
shellstub.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.11.9)
shellstub.py
4-4: sys imported but unused
Remove unused import: sys
(F401)
7-7: Import from collections.abc instead: Sequence
Import from collections.abc
(UP035)
23-23: Missing return type annotation for special method __init__
Add return type annotation: None
(ANN204)
39-39: Unnecessary dict() call (rewrite as a literal)
Rewrite as a literal
(C408)
🪛 Flake8 (7.2.0)
shellstub.py
[error] 4-4: 'sys' imported but unused
(F401)
🪛 Pylint (3.3.7)
shellstub.py
[convention] 1-1: Missing module docstring
(C0114)
[convention] 10-10: Missing class docstring
(C0115)
[convention] 16-16: Missing class docstring
(C0115)
[convention] 22-22: Missing class docstring
(C0115)
[convention] 29-29: Missing function or method docstring
(C0116)
[refactor] 29-29: Too many arguments (6/5)
(R0913)
[refactor] 39-39: Consider using '{"match": None, "stdout": stdout, "stderr": stderr, "exit_code": exit_code, ... }' instead of a call to 'dict'.
(R1735)
[convention] 51-51: Missing function or method docstring
(C0116)
[convention] 62-62: Missing function or method docstring
(C0116)
[warning] 4-4: Unused import sys
(W0611)
🔇 Additional comments (1)
shellstub.py (1)
45-49: Good implementation of external spec file.The method now correctly saves the spec to an external JSON file and passes its path to the wrapper, which addresses the previous concern about embedding large JSON strings.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (7)
.github/actions/generate-coverage/tests/conftest.py (2)
1-11: Import organisation needs improvementThe imports should follow standard convention with standard library imports first, then third-party imports.
13-23: Add type annotations to fixture parametersThe fixture parameters
tmp_pathandmonkeypatchneed type annotations for clarity..github/actions/generate-coverage/tests/test_scripts.py (4)
7-13: Improve type annotation and documentationThe function needs a more specific return type annotation and documentation.
15-44: Contract test must validate complete input/output round-tripPer coding guidelines, contract tests must validate that declared inputs/outputs round-trip correctly. The test should verify all input parameters are properly passed through to the cargo command.
47-64: Add type annotationsFunction parameters need type annotations for consistency.
68-98: Improve code style and assertion clarityBreak down complex assertions and use literal syntax instead of dict() call.
shellstub.py (1)
101-109: Fix environment variable merge orderThe current merge order will overwrite your custom PATH and PYTHONPATH with values from os.environ.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (9)
.github/actions/generate-coverage/scripts/ratchet_coverage.py(1 hunks).github/actions/generate-coverage/scripts/run_python.py(1 hunks).github/actions/generate-coverage/scripts/run_rust.py(1 hunks).github/actions/generate-coverage/tests/conftest.py(1 hunks).github/actions/generate-coverage/tests/test_scripts.py(1 hunks).github/actions/ratchet-coverage/scripts/install_cargo_llvm_cov.py(1 hunks).github/actions/ratchet-coverage/scripts/ratchet_coverage.py(1 hunks).github/actions/ratchet-coverage/scripts/run_coverage.py(1 hunks)shellstub.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`.github/actions/*/tests/**/*`: Contract tests must validate that declared inputs/outputs round-trip correctly
.github/actions/*/tests/**/*: Contract tests must validate that declared inputs/outputs round-trip correctly
📄 Source: CodeRabbit Inference Engine (AGENTS.md)
List of files the instruction was applied to:
.github/actions/generate-coverage/tests/conftest.py.github/actions/generate-coverage/tests/test_scripts.py
🧬 Code Graph Analysis (1)
.github/actions/generate-coverage/tests/conftest.py (1)
shellstub.py (1)
StubManager(37-147)
🪛 Pylint (3.3.7)
.github/actions/ratchet-coverage/scripts/install_cargo_llvm_cov.py
[error] 6-6: Unable to import 'typer'
(E0401)
.github/actions/generate-coverage/tests/conftest.py
[convention] 1-1: Missing module docstring
(C0114)
[error] 10-10: Unable to import 'shellstub'
(E0401)
[convention] 10-10: Import "from shellstub import StubManager" should be placed at the top of the module
(C0413)
.github/actions/generate-coverage/tests/test_scripts.py
[convention] 99-99: Trailing newlines
(C0305)
[convention] 1-1: Missing module docstring
(C0114)
[warning] 13-13: 'subprocess.run' used without explicitly defining the value for 'check'.
(W1510)
shellstub.py
[convention] 149-149: Trailing newlines
(C0305)
[convention] 1-1: Missing module docstring
(C0114)
[warning] 3-3: String statement has no effect
(W0105)
[convention] 5-5: Import "import json" should be placed at the top of the module
(C0413)
[convention] 6-6: Import "import os" should be placed at the top of the module
(C0413)
[convention] 7-7: Import "import sys" should be placed at the top of the module
(C0413)
[convention] 8-8: Import "import textwrap" should be placed at the top of the module
(C0413)
[convention] 9-9: Import "from collections.abc import Callable, Sequence" should be placed at the top of the module
(C0413)
[convention] 10-10: Import "from dataclasses import dataclass" should be placed at the top of the module
(C0413)
[convention] 11-11: Import "from pathlib import Path" should be placed at the top of the module
(C0413)
[convention] 32-32: Missing class docstring
(C0115)
[refactor] 48-48: Too many arguments (7/5)
(R0913)
[warning] 7-7: Unused import sys
(W0611)
[warning] 8-8: Unused import textwrap
(W0611)
🪛 Ruff (0.11.9)
.github/actions/generate-coverage/tests/conftest.py
14-14: Missing type annotation for function argument tmp_path
(ANN001)
14-14: Missing type annotation for function argument monkeypatch
(ANN001)
21-21: Trailing comma missing
Add trailing comma
(COM812)
23-23: No teardown in fixture shell_stubs, use return instead of yield
Replace yield with return
(PT022)
.github/actions/generate-coverage/tests/test_scripts.py
8-8: Trailing comma missing
Add trailing comma
(COM812)
13-13: subprocess call: check for execution of untrusted input
(S603)
15-15: Missing type annotation for function argument tmp_path
(ANN001)
15-15: Missing type annotation for function argument shell_stubs
(ANN001)
47-47: Missing type annotation for function argument tmp_path
(ANN001)
47-47: Missing type annotation for function argument shell_stubs
(ANN001)
68-68: Missing type annotation for function argument tmp_path
(ANN001)
68-68: Missing type annotation for function argument shell_stubs
(ANN001)
82-82: Trailing comma missing
Add trailing comma
(COM812)
96-96: Assertion should be broken down into multiple parts
Break down assertion into multiple parts
(PT018)
98-98: Assertion should be broken down into multiple parts
Break down assertion into multiple parts
(PT018)
shellstub.py
5-5: Module level import not at top of file
(E402)
6-6: Module level import not at top of file
(E402)
7-7: Module level import not at top of file
(E402)
7-7: sys imported but unused
Remove unused import: sys
(F401)
8-8: Module level import not at top of file
(E402)
8-8: textwrap imported but unused
Remove unused import: textwrap
(F401)
9-9: Module level import not at top of file
(E402)
10-10: Module level import not at top of file
(E402)
11-11: Module level import not at top of file
(E402)
66-66: Trailing comma missing
Add trailing comma
(COM812)
76-76: Trailing comma missing
Add trailing comma
(COM812)
82-82: Trailing comma missing
Add trailing comma
(COM812)
🪛 Flake8 (7.2.0)
shellstub.py
[error] 7-7: 'sys' imported but unused
(F401)
[error] 8-8: 'textwrap' imported but unused
(F401)
🔇 Additional comments (7)
.github/actions/ratchet-coverage/scripts/ratchet_coverage.py (1)
6-8: Import grouping/spacing looks correct – no action required.Placing a blank line after the last standard-library import matches PEP 8 and the rest of the codebase; the change is purely cosmetic and safe to merge.
.github/actions/generate-coverage/scripts/ratchet_coverage.py (1)
6-8: Consistent import spacing – good job.The added newline keeps builtin and third-party imports visually separated. No further feedback.
.github/actions/ratchet-coverage/scripts/install_cargo_llvm_cov.py (1)
6-8: Confirmtyperis available in the runtime image.Static analysis flagged
import typeras unresolved (E0401). The shebang’suvstub will install declared dependencies, but double-check that the CI image used for this step includestyper>=0.9or pin it explicitly to silence tooling warnings..github/actions/generate-coverage/scripts/run_python.py (1)
8-12: Import block tidy-up acknowledged.Grouping standard-library imports (
contextlib,Path) before third-party ones (defusedxml,typer) improves readability and matches the other scripts..github/actions/ratchet-coverage/scripts/run_coverage.py (1)
8-9: Import reordering looks goodThe reordering of imports maintains proper grouping and improves consistency across the codebase.
shellstub.py (2)
14-35: Well-documented dataclassesThe dataclasses are properly documented with clear docstrings explaining their purpose.
1-11: Fix module docstring placementThe module docstring should be the first statement after the future import, before other imports.
from __future__ import annotations -"""Minimal framework for stubbing command line tools in tests.""" - import json import os import sys import textwrap from collections.abc import Callable, Sequence from dataclasses import dataclass from pathlib import Path +"""Minimal framework for stubbing command line tools in tests."""Actually, I need to reconsider - module docstrings should come immediately after imports in Python. The current placement is actually correct. Let me skip this comment.
| import re | ||
| from pathlib import Path | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Minor import reorder – verify formatter settings.
re and Path are both std-lib, so the new order is fine; just ensure your isort/black configuration won’t shuffle them back again to avoid future diff noise. Consider a blank line before the first third-party import (typer) if you want strict PEP 8 spacing.
🤖 Prompt for AI Agents
In .github/actions/generate-coverage/scripts/run_rust.py around lines 8 to 10,
the imports of standard library modules 're' and 'Path' are reordered correctly,
but to prevent future unnecessary diffs, verify your isort and black formatter
settings to keep this order stable. Also, add a blank line after the standard
library imports and before the third-party import 'typer' to comply with strict
PEP 8 import grouping and spacing conventions.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
.github/actions/generate-coverage/tests/conftest.py (1)
24-24: Add type annotations and keep yield for proper fixture behaviour.The fixture needs type annotations, and the yield is correct for pytest fixtures that require cleanup context.
-def shell_stubs(tmp_path, monkeypatch) -> StubManager: +def shell_stubs(tmp_path: pytest.TempPathFactory, monkeypatch: pytest.MonkeyPatch) -> StubManager:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
.github/actions/generate-coverage/tests/conftest.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`.github/actions/*/tests/**/*`: Contract tests must validate that declared inputs/outputs round-trip correctly
.github/actions/*/tests/**/*: Contract tests must validate that declared inputs/outputs round-trip correctly
📄 Source: CodeRabbit Inference Engine (AGENTS.md)
List of files the instruction was applied to:
.github/actions/generate-coverage/tests/conftest.py
🧬 Code Graph Analysis (1)
.github/actions/generate-coverage/tests/conftest.py (1)
shellstub.py (1)
StubManager(38-156)
🪛 Ruff (0.11.9)
.github/actions/generate-coverage/tests/conftest.py
14-14: Avoid specifying long messages outside the exception class
(TRY003)
24-24: Missing type annotation for function argument tmp_path
(ANN001)
24-24: Missing type annotation for function argument monkeypatch
(ANN001)
33-33: Trailing comma missing
Add trailing comma
(COM812)
35-35: No teardown in fixture shell_stubs, use return instead of yield
Replace yield with return
(PT022)
🔇 Additional comments (2)
.github/actions/generate-coverage/tests/conftest.py (2)
17-18: LGTM! Robust path calculation.The
_find_rootfunction properly addresses the previous fragile path calculation concern by dynamically searching for project markers.
29-30: LGTM! Proper global state management.Setting the global manager ensures the stubs work correctly across subprocess calls. The yield in the fixture is appropriate for cleanup context.
| monkeypatch.setenv( | ||
| "PYTHONPATH", f"{ROOT}{os.pathsep}{os.getenv('PYTHONPATH','')}" | ||
| ) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add trailing comma for consistency.
monkeypatch.setenv(
- "PYTHONPATH", f"{ROOT}{os.pathsep}{os.getenv('PYTHONPATH','')}"
+ "PYTHONPATH", f"{ROOT}{os.pathsep}{os.getenv('PYTHONPATH','')}",
)📝 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.
| monkeypatch.setenv( | |
| "PYTHONPATH", f"{ROOT}{os.pathsep}{os.getenv('PYTHONPATH','')}" | |
| ) | |
| monkeypatch.setenv( | |
| "PYTHONPATH", f"{ROOT}{os.pathsep}{os.getenv('PYTHONPATH','')}", | |
| ) |
🧰 Tools
🪛 Ruff (0.11.9)
33-33: Trailing comma missing
Add trailing comma
(COM812)
🤖 Prompt for AI Agents
In .github/actions/generate-coverage/tests/conftest.py around lines 32 to 34,
the call to monkeypatch.setenv is missing a trailing comma after the last
argument. Add a trailing comma after the f-string argument to maintain
consistency and improve code readability.
| def _find_root(start: Path) -> Path: | ||
| """Return the nearest ancestor containing ``pyproject.toml`` or ``.git``.""" | ||
|
|
||
| for parent in (start, *start.parents): | ||
| if (parent / "pyproject.toml").exists() or (parent / ".git").exists(): | ||
| return parent | ||
| raise FileNotFoundError(f"Project root not found from {start}") |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider shortening the exception message.
The exception message is quite long. Consider using a shorter message or define a custom exception class.
- raise FileNotFoundError(f"Project root not found from {start}")
+ raise FileNotFoundError("Project root not found")📝 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 _find_root(start: Path) -> Path: | |
| """Return the nearest ancestor containing ``pyproject.toml`` or ``.git``.""" | |
| for parent in (start, *start.parents): | |
| if (parent / "pyproject.toml").exists() or (parent / ".git").exists(): | |
| return parent | |
| raise FileNotFoundError(f"Project root not found from {start}") | |
| def _find_root(start: Path) -> Path: | |
| """Return the nearest ancestor containing ``pyproject.toml`` or ``.git``.""" | |
| for parent in (start, *start.parents): | |
| if (parent / "pyproject.toml").exists() or (parent / ".git").exists(): | |
| return parent | |
| raise FileNotFoundError("Project root not found") |
🧰 Tools
🪛 Ruff (0.11.9)
14-14: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
In .github/actions/generate-coverage/tests/conftest.py around lines 8 to 14, the
FileNotFoundError exception message is too long. Shorten the message to a
concise phrase like "Project root not found" or create a custom exception class
with a brief message to replace the current long message.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
shellstub.py (1)
96-98: Add error handling for malformed JSON in calls fileThe JSON parsing could fail if the calls file contains malformed data. Consider adding error handling to skip invalid lines.
for line in self._calls_file.read_text().splitlines(): - rec = json.loads(line) - if rec["cmd"] == name: - out.append(Call(rec["argv"], rec["cwd"], rec["ts"])) + try: + rec = json.loads(line) + if rec["cmd"] == name: + out.append(Call(rec["argv"], rec["cwd"], rec["ts"])) + except (json.JSONDecodeError, KeyError) as e: + # Log or skip malformed entries + continue.github/actions/generate-coverage/tests/conftest.py (1)
34-36: Add trailing comma for consistencymonkeypatch.setenv( - "PYTHONPATH", f"{ROOT}{os.pathsep}{os.getenv('PYTHONPATH','')}" + "PYTHONPATH", f"{ROOT}{os.pathsep}{os.getenv('PYTHONPATH','')}", )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
.github/actions/generate-coverage/tests/conftest.py(1 hunks).github/actions/generate-coverage/tests/test_scripts.py(1 hunks)shellstub.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`.github/actions/*/tests/**/*`: Contract tests must validate that declared inputs/outputs round-trip correctly
.github/actions/*/tests/**/*: Contract tests must validate that declared inputs/outputs round-trip correctly
📄 Source: CodeRabbit Inference Engine (AGENTS.md)
List of files the instruction was applied to:
.github/actions/generate-coverage/tests/conftest.py.github/actions/generate-coverage/tests/test_scripts.py
🧬 Code Graph Analysis (2)
.github/actions/generate-coverage/tests/conftest.py (1)
shellstub.py (1)
StubManager(38-155)
.github/actions/generate-coverage/tests/test_scripts.py (2)
shellstub.py (4)
StubManager(38-155)env(103-109)register(49-87)calls_of(90-100).github/actions/generate-coverage/tests/conftest.py (1)
shell_stubs(26-37)
🪛 Ruff (0.11.9)
.github/actions/generate-coverage/tests/conftest.py
16-16: Avoid specifying long messages outside the exception class
(TRY003)
35-35: Trailing comma missing
Add trailing comma
(COM812)
37-37: No teardown in fixture shell_stubs, use return instead of yield
Replace yield with return
(PT022)
.github/actions/generate-coverage/tests/test_scripts.py
10-10: Trailing comma missing
Add trailing comma
(COM812)
15-15: subprocess call: check for execution of untrusted input
(S603)
97-97: Trailing comma missing
Add trailing comma
(COM812)
114-114: Assertion should be broken down into multiple parts
Break down assertion into multiple parts
(PT018)
shellstub.py
5-5: Module level import not at top of file
(E402)
6-6: Module level import not at top of file
(E402)
7-7: Module level import not at top of file
(E402)
7-7: sys imported but unused
Remove unused import: sys
(F401)
8-8: Module level import not at top of file
(E402)
9-9: Module level import not at top of file
(E402)
10-10: Module level import not at top of file
(E402)
67-67: Trailing comma missing
Add trailing comma
(COM812)
77-77: Trailing comma missing
Add trailing comma
(COM812)
83-83: Trailing comma missing
Add trailing comma
(COM812)
🔇 Additional comments (5)
shellstub.py (2)
13-36: Well-structured data model for stub frameworkThe dataclasses provide a clean and well-documented interface for defining stub behaviour and recording invocations. The use of type annotations and default values is appropriate.
111-155: Excellent wrapper script generation with proper func supportThe wrapper source correctly integrates with the global manager to support custom function callbacks, addressing previous concerns. The script properly logs calls, handles variant matching, and executes custom functions when provided.
.github/actions/generate-coverage/tests/conftest.py (1)
10-17: Robust project root detectionThe
_find_rootfunction provides a reliable way to locate the project root by searching for common project markers. This is more maintainable than using a fixed directory depth..github/actions/generate-coverage/tests/test_scripts.py (2)
17-60: Comprehensive contract validation for Rust coverage scriptThe test properly validates the input/output contract by:
- Verifying the output path is correctly passed to cargo
- Confirming the format flag (--lcov) matches the input
- Ensuring feature flags are properly handled
- Checking that default features are correctly disabled
- Validating the coverage percentage output
This meets the coding guideline requirement for contract tests.
62-80: Proper error propagation testingThe test correctly validates that cargo failures are propagated with the same exit code and appropriate error messaging.
| if variants is None: | ||
| variants = [ | ||
| { | ||
| "match": None, | ||
| "stdout": stdout, | ||
| "stderr": stderr, | ||
| "exit_code": exit_code, | ||
| } | ||
| ] | ||
| parsed = [] | ||
| for v in variants: | ||
| parsed.append( | ||
| Variant( | ||
| match=v["match"], | ||
| stdout=v.get("stdout", ""), | ||
| stderr=v.get("stderr", ""), | ||
| exit_code=v.get("exit_code", 0), | ||
| ) | ||
| ) | ||
| spec = StubSpec(parsed, func=func) | ||
| self._specs[name] = spec | ||
| spec_file = self.dir / f"{name}.json" | ||
| spec_file.write_text( | ||
| json.dumps({"variants": [v.__dict__ for v in parsed]}) | ||
| ) | ||
| path = self.dir / name | ||
| path.write_text(self._wrapper_source(name, spec_file)) | ||
| path.chmod(0o755) | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add trailing commas for consistency
Missing trailing commas in multi-line data structures.
{
"match": None,
"stdout": stdout,
"stderr": stderr,
"exit_code": exit_code,
- }
+ },
]
parsed = []
for v in variants:
parsed.append(
Variant(
match=v["match"],
stdout=v.get("stdout", ""),
stderr=v.get("stderr", ""),
exit_code=v.get("exit_code", 0),
- )
+ ),
)
spec = StubSpec(parsed, func=func)
self._specs[name] = spec
spec_file = self.dir / f"{name}.json"
spec_file.write_text(
- json.dumps({"variants": [v.__dict__ for v in parsed]})
+ json.dumps({"variants": [v.__dict__ for v in parsed]}),
)📝 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.
| if variants is None: | |
| variants = [ | |
| { | |
| "match": None, | |
| "stdout": stdout, | |
| "stderr": stderr, | |
| "exit_code": exit_code, | |
| } | |
| ] | |
| parsed = [] | |
| for v in variants: | |
| parsed.append( | |
| Variant( | |
| match=v["match"], | |
| stdout=v.get("stdout", ""), | |
| stderr=v.get("stderr", ""), | |
| exit_code=v.get("exit_code", 0), | |
| ) | |
| ) | |
| spec = StubSpec(parsed, func=func) | |
| self._specs[name] = spec | |
| spec_file = self.dir / f"{name}.json" | |
| spec_file.write_text( | |
| json.dumps({"variants": [v.__dict__ for v in parsed]}) | |
| ) | |
| path = self.dir / name | |
| path.write_text(self._wrapper_source(name, spec_file)) | |
| path.chmod(0o755) | |
| if variants is None: | |
| variants = [ | |
| { | |
| "match": None, | |
| "stdout": stdout, | |
| "stderr": stderr, | |
| "exit_code": exit_code, | |
| }, | |
| ] | |
| parsed = [] | |
| for v in variants: | |
| parsed.append( | |
| Variant( | |
| match=v["match"], | |
| stdout=v.get("stdout", ""), | |
| stderr=v.get("stderr", ""), | |
| exit_code=v.get("exit_code", 0), | |
| ), | |
| ) | |
| spec = StubSpec(parsed, func=func) | |
| self._specs[name] = spec | |
| spec_file = self.dir / f"{name}.json" | |
| spec_file.write_text( | |
| json.dumps({"variants": [v.__dict__ for v in parsed]}), | |
| ) | |
| path = self.dir / name | |
| path.write_text(self._wrapper_source(name, spec_file)) | |
| path.chmod(0o755) |
🧰 Tools
🪛 Ruff (0.11.9)
67-67: Trailing comma missing
Add trailing comma
(COM812)
77-77: Trailing comma missing
Add trailing comma
(COM812)
83-83: Trailing comma missing
Add trailing comma
(COM812)
🤖 Prompt for AI Agents
In shellstub.py between lines 60 and 88, add trailing commas to all multi-line
data structures such as dictionaries and function arguments to maintain
consistency and improve readability. Locate each multi-line dictionary and
function call and ensure a comma is placed after the last item or argument.
| def run_script( | ||
| script: Path, env: dict[str, str], *args: str | ||
| ) -> subprocess.CompletedProcess[str]: |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add trailing comma for consistency
def run_script(
- script: Path, env: dict[str, str], *args: str
+ script: Path, env: dict[str, str], *args: str,
) -> subprocess.CompletedProcess[str]:📝 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 run_script( | |
| script: Path, env: dict[str, str], *args: str | |
| ) -> subprocess.CompletedProcess[str]: | |
| def run_script( | |
| script: Path, env: dict[str, str], *args: str, | |
| ) -> subprocess.CompletedProcess[str]: | |
| cmd = ["uv", "run", "--script", str(script), *args] | |
| return subprocess.run(cmd, capture_output=True, text=True, env=env) |
🧰 Tools
🪛 Ruff (0.11.9)
10-10: Trailing comma missing
Add trailing comma
(COM812)
🤖 Prompt for AI Agents
In .github/actions/generate-coverage/tests/test_scripts.py around lines 9 to 11,
the function definition for run_script is missing a trailing comma after the
last parameter for consistency. Add a trailing comma after the *args parameter
in the function signature to maintain consistent formatting.
| assert not rust.exists() | ||
| assert not py.exists() | ||
| calls = shell_stubs.calls_of("uvx") | ||
| assert calls and calls[0].argv[:1] == ["merge-cobertura"] |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Break down complex assertion for clarity
- assert calls and calls[0].argv[:1] == ["merge-cobertura"]
+ assert len(calls) > 0
+ assert calls[0].argv[0] == "merge-cobertura"📝 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.
| assert calls and calls[0].argv[:1] == ["merge-cobertura"] | |
| calls = shell_stubs.calls_of("uvx") | |
| assert len(calls) > 0 | |
| assert calls[0].argv[0] == "merge-cobertura" |
🧰 Tools
🪛 Ruff (0.11.9)
114-114: Assertion should be broken down into multiple parts
Break down assertion into multiple parts
(PT018)
🤖 Prompt for AI Agents
In .github/actions/generate-coverage/tests/test_scripts.py at line 114, the
assertion combines multiple checks into one, which reduces clarity. Split the
assertion into separate statements: first assert that 'calls' is not empty, then
assert that the first element of 'calls[0].argv' matches ["merge-cobertura"].
This makes the test easier to read and debug.
| shell_stubs.register( | ||
| "uvx", | ||
| variants=[ | ||
| { | ||
| "match": ["merge-cobertura", str(rust), str(py)], | ||
| "stdout": "<merged/>", | ||
| } | ||
| ], | ||
| ) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add trailing comma for consistency
variants=[
{
"match": ["merge-cobertura", str(rust), str(py)],
"stdout": "<merged/>",
- }
+ },
],📝 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.
| shell_stubs.register( | |
| "uvx", | |
| variants=[ | |
| { | |
| "match": ["merge-cobertura", str(rust), str(py)], | |
| "stdout": "<merged/>", | |
| } | |
| ], | |
| ) | |
| shell_stubs.register( | |
| "uvx", | |
| variants=[ | |
| { | |
| "match": ["merge-cobertura", str(rust), str(py)], | |
| "stdout": "<merged/>", | |
| }, | |
| ], | |
| ) |
🧰 Tools
🪛 Ruff (0.11.9)
97-97: Trailing comma missing
Add trailing comma
(COM812)
🤖 Prompt for AI Agents
In .github/actions/generate-coverage/tests/test_scripts.py around lines 91 to
99, the dictionary inside the variants list is missing a trailing comma after
the last key-value pair "stdout": "<merged/>". Add a trailing comma there to
maintain consistency with the surrounding code style.
1 participant
Summary
shellstubhelper for stubbing command line tools.github/actionsgenerate-coveragescripts using subprocess and stubsTesting
pytest -qhttps://chatgpt.com/codex/tasks/task_e_686af1b0fd348322a84c565ee94e79c9
Summary by Sourcery
Add a reusable shellstub utility and enable behavioural tests for coverage scripts by registering stubs and configuring pytest to include the .github/actions directory
New Features:
Tests: