Add async SafeCmd.run with cancellation and expose API types - #8
Conversation
![terragon-labs[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
… and cancellation Implemented SafeCmd.run method providing asynchronous command execution with options for output capture, echoing, environment and working directory overrides, and predictable cancellation behavior. Added CommandResult dataclass for structured results including exit code, pid, stdout, and stderr. Includes termination logic on cancellation to ensure subprocess cleanup. Added comprehensive unit and behavioural tests, updated documentation, and extended imports in __init__. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
|
Warning Rate limit exceeded@leynos has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 7 minutes and 27 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Summary by CodeRabbitRelease Notes
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAdd asynchronous execution to SafeCmd: introduce frozen dataclasses CommandResult and ExecutionContext, implement SafeCmd.run to spawn subprocesses with merged env and cwd, non‑blocking I/O and echo/capture semantics, and support cancellation escalation (terminate → wait → kill); expose new types and add tests and docs. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant SafeCmd
participant ProcessMgr as Process Manager
participant Subprocess
participant IOHandler as I/O Handler
participant Canceller as Cancellation
Client->>SafeCmd: run(capture, echo, context)
activate SafeCmd
SafeCmd->>ProcessMgr: spawn subprocess (argv + merged env/cwd)
activate ProcessMgr
ProcessMgr->>Subprocess: fork/exec
ProcessMgr-->>SafeCmd: return handle + streams
deactivate ProcessMgr
par Concurrent I/O
SafeCmd->>IOHandler: start stdout reader
IOHandler->>Subprocess: read stdout
Subprocess-->>IOHandler: stdout chunks
IOHandler-->>SafeCmd: accumulate/echo
SafeCmd->>IOHandler: start stderr reader
IOHandler->>Subprocess: read stderr
Subprocess-->>IOHandler: stderr chunks
IOHandler-->>SafeCmd: accumulate
end
Subprocess-->>ProcessMgr: exit (pid, code)
ProcessMgr->>SafeCmd: deliver exit_code + pid
opt Cancellation Path
Canceller->>SafeCmd: cancel awaiting task
SafeCmd->>ProcessMgr: send SIGTERM
ProcessMgr->>Subprocess: SIGTERM
ProcessMgr->>ProcessMgr: wait cancel_grace
alt still alive
ProcessMgr->>Subprocess: SIGKILL
end
ProcessMgr->>IOHandler: cancel and cleanup streams
end
SafeCmd-->>Client: return CommandResult(exit_code, pid, stdout, stderr, ok)
deactivate SafeCmd
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
Comment |
Reviewer's GuideImplements an asynchronous SafeCmd.run runtime with structured CommandResult/ExecutionContext APIs, controlled env/cwd configuration, capture/echo I/O semantics, and robust cancellation, plus tests and docs for the new execution runtime behaviour. Sequence diagram for SafeCmd.run async execution and cancellationsequenceDiagram
actor ClientTask
participant SafeCmd
participant ExecutionContext
participant AsyncIO as asyncio
participant Subprocess as subprocess
participant StdoutTask as stdout_consumer
participant StderrTask as stderr_consumer
participant Terminator as _terminate_process
ClientTask->>SafeCmd: run(capture, echo, context)
SafeCmd->>ExecutionContext: create_default_if_none()
SafeCmd->>AsyncIO: create_subprocess_exec(argv_with_program, env, cwd, stdout, stderr)
AsyncIO-->>SafeCmd: subprocess.Process
alt capture_or_echo_enabled
SafeCmd->>AsyncIO: create_task(_consume_stream(stdout))
AsyncIO-->>SafeCmd: stdout_task
SafeCmd->>AsyncIO: create_task(_consume_stream(stderr))
AsyncIO-->>SafeCmd: stderr_task
end
rect rgb(235,235,235)
alt normal_completion
SafeCmd->>Subprocess: wait()
Subprocess-->>SafeCmd: exit_code
SafeCmd->>AsyncIO: gather(stdout_task, stderr_task)
AsyncIO-->>SafeCmd: stdout_text, stderr_text
SafeCmd-->>ClientTask: CommandResult(program, argv, exit_code, pid, stdout_text, stderr_text)
else cancellation_during_wait
SafeCmd->>Subprocess: wait()
Subprocess--xSafeCmd: CancelledError
SafeCmd->>Terminator: _terminate_process(process, cancel_grace)
Terminator-->>SafeCmd: subprocess_terminated
SafeCmd->>AsyncIO: gather(stdout_task, stderr_task, return_exceptions=True)
AsyncIO-->>SafeCmd: tasks_completed
SafeCmd--xClientTask: re_raise CancelledError
end
end
Class diagram for SafeCmd async runtime and API typesclassDiagram
class Program
class CommandResult {
+Program program
+tuple~str~ argv
+int exit_code
+int pid
+str stdout
+str stderr
+bool ok()
}
class ExecutionContext {
+mapping~str,str~ env
+str cwd
+float cancel_grace
}
class SafeCmd {
+Program program
+tuple~str~ argv
+tuple~str~ argv_with_program()
+CommandResult run(bool capture, bool echo, ExecutionContext context)
}
SafeCmd ..> Program : uses
SafeCmd ..> ExecutionContext : uses
SafeCmd --> CommandResult : returns
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.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Comment on lines +99 to +167 async def run( # noqa: PLR0913
self,
*,
capture: bool = True,
echo: bool = False,
env: _EnvMapping = None,
cwd: _CwdType = None,
cancel_grace: float = _DEFAULT_CANCEL_GRACE,
) -> CommandResult:
"""Execute the command asynchronously with predictable cancellation.
``capture`` controls whether stdout/stderr are retained; when disabled
the returned result contains ``None`` for the respective fields.
``echo`` mirrors stdout/stderr to the parent process while still
respecting the ``capture`` flag.
"""
resolved_env = _merge_env(env)
stdout_target = (
asyncio.subprocess.PIPE if capture or echo else asyncio.subprocess.DEVNULL
)
stderr_target = (
asyncio.subprocess.PIPE if capture or echo else asyncio.subprocess.DEVNULL
)
process = await asyncio.create_subprocess_exec(
*self.argv_with_program,
stdout=stdout_target,
stderr=stderr_target,
env=resolved_env,
cwd=str(cwd) if cwd is not None else None,
)
stdout_task = asyncio.create_task(
_consume_stream(
process.stdout,
capture_output=capture,
echo_output=echo,
sink=sys.stdout,
),
)
stderr_task = asyncio.create_task(
_consume_stream(
process.stderr,
capture_output=capture,
echo_output=echo,
sink=sys.stderr,
),
)
try:
exit_code = await process.wait()
except asyncio.CancelledError:
await _terminate_process(process, cancel_grace)
await asyncio.gather(stdout_task, stderr_task, return_exceptions=True)
raise
stdout_text, stderr_text = await asyncio.gather(
stdout_task,
stderr_task,
)
return CommandResult(
program=self.program,
argv=self.argv,
exit_code=exit_code,
pid=process.pid or -1,
stdout=stdout_text,
stderr=stderr_text,
)❌ New issue: Excess Number of Function Arguments |
This comment was marked as resolved.
This comment was marked as resolved.
Introduce ExecutionContext dataclass to encapsulate execution parameters such as environment overlays, working directory, and cancellation grace period for SafeCmd asynchronous command execution. Modify SafeCmd.run() to accept an optional ExecutionContext instance, replacing prior env, cwd, and cancel_grace parameters. Update tests and documentation to reflect usage of ExecutionContext for improved command execution configurability and cleaner API. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- In
_terminate_process,asyncio.wait_forraisesasyncio.TimeoutErrorrather than the built-inTimeoutError, so the exception handler will never trigger as written; consider catchingasyncio.TimeoutErrorexplicitly. SafeCmd.runalways creates_consume_streamtasks even when bothcaptureandechoareFalse; you could short-circuit in that case and avoid spawning the tasks entirely to reduce event-loop overhead.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `_terminate_process`, `asyncio.wait_for` raises `asyncio.TimeoutError` rather than the built-in `TimeoutError`, so the exception handler will never trigger as written; consider catching `asyncio.TimeoutError` explicitly.
- `SafeCmd.run` always creates `_consume_stream` tasks even when both `capture` and `echo` are `False`; you could short-circuit in that case and avoid spawning the tasks entirely to reduce event-loop overhead.
## Individual Comments
### Comment 1
<location> `cuprum/sh.py:234-236` </location>
<code_context>
+ return buffer.decode("utf-8", errors="replace")
+
+
+def _write_chunk(sink: typ.IO[str], chunk: bytes) -> None:
+ """Write a bytes chunk to a text sink without blocking the event loop."""
+ buffer = getattr(sink, "buffer", None)
+ if buffer is not None:
</code_context>
<issue_to_address>
**suggestion:** The docstring overstates non-blocking behavior of synchronous I/O.
Because this writes synchronously to `sink` (or `sink.buffer`), it may still block on slow or back-pressured outputs. Please adjust the docstring so it describes the real behavior (e.g., avoiding extra encoding but still performing a blocking write) rather than implying non-blocking I/O.
```suggestion
def _write_chunk(sink: typ.IO[str], chunk: bytes) -> None:
"""Write a bytes chunk to a text sink synchronously, avoiding extra encoding.
This writes directly to ``sink.buffer`` when available, or to ``sink`` itself,
and may still block if the underlying I/O is slow or back-pressured.
"""
buffer = getattr(sink, "buffer", None)
```
</issue_to_address>
### Comment 2
<location> `cuprum/unittests/test_safe_cmd_run.py:42-50` </location>
<code_context>
+ return sh.make(program, catalogue=catalogue)
+
+
+def test_run_captures_output_and_exit_code() -> None:
+ """run() captures stdout/stderr and exit code by default."""
+ command = sh.make(ECHO)("-n", "hello")
+
+ result = asyncio.run(command.run())
+
+ assert result.exit_code == 0
+ assert result.stdout == "hello"
+ assert result.stderr == ""
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Add coverage for non-zero exit codes and the CommandResult.ok convenience property
These tests only cover the success path of `run()`. Please add a case where the subprocess exits non-zero (e.g. `python -c 'import sys; sys.exit(3)'` or a failing `ECHO`-style command) and assert both the `exit_code` and `result.ok` values. That will verify the structured result behaves correctly on failures as well.
```suggestion
def test_run_captures_output_and_exit_code() -> None:
"""run() captures stdout/stderr and exit code by default."""
command = sh.make(ECHO)("-n", "hello")
result = asyncio.run(command.run())
assert result.exit_code == 0
assert result.ok is True
assert result.stdout == "hello"
assert result.stderr == ""
def test_run_captures_nonzero_exit_code_and_ok_flag(python_builder: typ.Callable[..., SafeCmd]) -> None:
"""run() captures non-zero exit code and exposes it via the ok flag."""
# Use the current Python interpreter to reliably produce a non-zero exit.
command = python_builder("-c", "import sys; sys.exit(3)")
result = asyncio.run(command.run())
assert result.exit_code == 3
assert result.ok is False
```
</issue_to_address>
### Comment 3
<location> `tests/behaviour/test_execution_runtime.py:110-119` </location>
<code_context>
+ return {"command": command, "pid_file": pid_file}
+
+
+@when("I cancel the command after it starts")
+def when_cancel_command(
+ behaviour_state: dict[str, object],
+ long_running_command: dict[str, object],
+) -> None:
+ """Cancel the running command and record the child PID."""
+ command = typ.cast("SafeCmd", long_running_command["command"])
+ pid_file = typ.cast("Path", long_running_command["pid_file"])
+
+ async def orchestrate() -> int:
+ task = asyncio.create_task(
+ command.run(
+ capture=False,
+ context=ExecutionContext(
+ env={"CUPRUM_PID_FILE": pid_file.as_posix()},
+ ),
+ ),
+ )
+ pid = await _wait_for_pid(pid_file)
+ await asyncio.sleep(0.1)
+ task.cancel()
+ with pytest.raises(asyncio.CancelledError):
+ await task
+ return pid
+
+ behaviour_state["pid"] = asyncio.run(orchestrate())
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Add coverage for cancellation grace behaviour and escalation to kill when the subprocess does not exit cleanly
This already verifies cancellation of a cooperative long-running subprocess. To fully cover the behaviour, please also add a scenario (here or as a unit test) where the child ignores SIGTERM/SIGINT so `_terminate_process` must escalate after the grace period. That test should confirm both that the task is cancelled and that the subprocess is eventually killed, ensuring `cancel_grace` is honoured for non-cooperative processes.
Suggested implementation:
```python
import pytest
@pytest.mark.asyncio
async def test_non_cooperative_subprocess_is_escalated_and_killed(tmp_path: Path) -> None:
"""Ensure that a non-cooperative subprocess is killed after cancel_grace expires.
This simulates a child process that ignores SIGTERM / SIGINT so that the runtime
must escalate to a hard kill after the grace period.
"""
# Create a Python script that traps SIGTERM/SIGINT and keeps running.
script = tmp_path / "non_cooperative_child.py"
script.write_text(
"import signal, time\n"
"def handler(signum, frame):\n"
" # Ignore termination signals\n"
" pass\n"
"signal.signal(signal.SIGTERM, handler)\n"
"signal.signal(signal.SIGINT, handler)\n"
"while True:\n"
" time.sleep(0.1)\n"
)
# Construct the command using the same runtime interface that behaviour tests use.
# The exact import path for SafeCmd / ExecutionContext may differ in your codebase.
from cuprum.runtime import ExecutionContext, SafeCmd # type: ignore[import]
command = SafeCmd(
argv=[sys.executable, script.as_posix()],
context=ExecutionContext(),
)
async def run_and_cancel() -> int:
task = asyncio.create_task(
command.run(
capture=False,
cancel_grace=0.1, # force quick escalation for the test
),
)
# Allow the child to start.
await asyncio.sleep(0.1)
task.cancel()
with pytest.raises(asyncio.CancelledError):
await task
# Access the underlying child PID so we can assert it has been killed.
return command.process.pid # type: ignore[attr-defined]
pid = await run_and_cancel()
# After cancellation and grace-period escalation, the process must be gone.
# A final os.kill with signal 0 should fail if the process has been reaped.
with pytest.raises(ProcessLookupError):
os.kill(pid, 0)
```
The above test assumes some details that you may need to align with the existing code:
1. **Import paths / types**
- Adjust `from cuprum.runtime import ExecutionContext, SafeCmd` to match where `SafeCmd` and `ExecutionContext` are actually defined in your project (or reuse the same imports used elsewhere in this file for the cooperative test).
2. **SafeCmd construction**
- If `SafeCmd` is not instantiated directly with `argv`/`context`, adapt the construction to use your existing helpers (e.g. `sh.make(...)`, `ProgramCatalogue`, etc.) so it matches how commands are normally created in these behaviour tests.
3. **`cancel_grace` parameter name**
- If the cancellation grace period is configured via a different keyword (e.g. `termination_grace`, `cancel_timeout`, etc.), update `cancel_grace=0.1` accordingly so the test really exercises the escalation path.
4. **Accessing the child PID**
- If `command.process` is not publicly available, either:
- expose the PID in a supported way (e.g. `command.pid`), or
- extend `SafeCmd` to record the PID on completion/cancellation so the test can assert the process is dead.
5. **Platform considerations**
- If your test suite runs on Windows, you may need a platform-guard (e.g. `pytest.mark.skipif(sys.platform == "win32", ...)`) because signal semantics and `os.kill(pid, 0)` behave differently there.
</issue_to_address>
### Comment 4
<location> `cuprum/sh.py:69` </location>
<code_context>
return positional + flags
+@dc.dataclass(frozen=True, slots=True)
+class CommandResult:
+ """Structured result returned by command execution."""
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying the new async execution API by flattening configuration parameters, narrowing result and helper surfaces, and inlining small utilities to reduce indirection without changing behaviour.
You can trim a fair bit of complexity without losing any behaviour:
---
### 1. Flatten `ExecutionContext` at the public API
`ExecutionContext` is only a thin wrapper for `env`, `cwd`, `cancel_grace`, and it’s currently part of the public surface (`__all__`). You can keep it internally for future extensibility but make the common path simpler by taking keyword args directly on `run` and constructing the context inside.
```python
@dc.dataclass(frozen=True, slots=True)
class ExecutionContext:
env: _EnvMapping = None
cwd: _CwdType = None
cancel_grace: float = _DEFAULT_CANCEL_GRACE
@dc.dataclass(frozen=True, slots=True)
class SafeCmd:
...
async def run(
self,
*,
capture: bool = True,
echo: bool = False,
env: _EnvMapping = None,
cwd: _CwdType = None,
cancel_grace: float = _DEFAULT_CANCEL_GRACE,
context: ExecutionContext | None = None, # keep for advanced usage
) -> CommandResult:
if context is not None:
ctx = context
else:
ctx = ExecutionContext(env=env, cwd=cwd, cancel_grace=cancel_grace)
resolved_env = _merge_env(ctx.env)
...
process = await asyncio.create_subprocess_exec(
*self.argv_with_program,
stdout=stdout_target,
stderr=stderr_target,
env=resolved_env,
cwd=str(ctx.cwd) if ctx.cwd is not None else None,
)
...
```
And narrow the public surface:
```python
__all__ = [
"CommandResult",
# "ExecutionContext", # keep internal
"SafeCmd",
"SafeCmdBuilder",
"UnknownProgramError",
"make",
]
```
This keeps all existing functionality (context still works) but the common call site is simpler:
```python
await cmd.run(capture=True, echo=True, env={"FOO": "bar"}, cwd=path)
```
---
### 2. Reduce `CommandResult` surface (or make it internal)
If callers don’t actually need `program`/`argv` back, you can simplify the result object:
```python
@dc.dataclass(frozen=True, slots=True)
class CommandResult:
exit_code: int
pid: int
stdout: str | None
stderr: str | None
@property
def ok(self) -> bool:
return self.exit_code == 0
```
And in `run`:
```python
return CommandResult(
exit_code=exit_code,
pid=process.pid or -1,
stdout=stdout_text,
stderr=stderr_text,
)
```
If you do want the richer shape for internal use, keep that as a private class and export a narrower public type (`CommandResult` as an alias) that only surfaces what callers actually consume.
---
### 3. Collapse `_consume_stream` / `_write_chunk`
You can keep the streaming behaviour and reduce branching/generalisation by inlining `_write_chunk` into `_consume_stream` and specialising for stdout/stderr (the only sinks you pass):
```python
async def _consume_stream(
stream: asyncio.StreamReader | None,
*,
capture_output: bool,
echo_output: bool,
sink: typ.IO[str],
) -> str | None:
if stream is None:
return "" if capture_output else None
buffer = bytearray() if capture_output else None
# Fast path for common case: stdout/stderr with a `.buffer` attribute
raw_sink = getattr(sink, "buffer", None)
while True:
chunk = await stream.read(_READ_SIZE)
if not chunk:
break
if buffer is not None:
buffer.extend(chunk)
if echo_output:
if raw_sink is not None:
raw_sink.write(chunk)
raw_sink.flush()
else:
sink.write(chunk.decode("utf-8", errors="replace"))
sink.flush()
if buffer is None:
return None
return buffer.decode("utf-8", errors="replace")
```
Then `_write_chunk` can be dropped entirely.
Behaviour is unchanged (streaming, echoing, capturing), but the indirection and flag combinations are easier to track in a single function.
---
### 4. Simplify `_terminate_process` control flow
The helper is small and only used in one place. If you prefer to keep it, you can both simplify and fix the exception type:
```python
async def _terminate_process(
process: asyncio.subprocess.Process,
grace_period: float,
) -> None:
if process.returncode is not None:
return
if grace_period <= 0:
process.kill()
await process.wait()
return
process.terminate()
try:
await asyncio.wait_for(process.wait(), grace_period)
except asyncio.TimeoutError:
process.kill()
await process.wait()
```
This makes the “zero grace” behaviour explicit and flattens the logic. If you inline into `run`, you can keep the same structure directly in the `CancelledError` handler.
</issue_to_address>
### Comment 5
<location> `cuprum/sh.py:204` </location>
<code_context>
def _merge_env(extra: _EnvMapping) -> dict[str, str] | None:
"""Overlay extra environment variables when provided."""
if extra is None:
return None
merged = os.environ.copy()
merged.update(extra)
return merged
</code_context>
<issue_to_address>
**suggestion (code-quality):** Merge dictionary updates via the union operator ([`dict-assign-update-to-union`](https://docs.sourcery.ai/Reference/Default-Rules/suggestions/dict-assign-update-to-union/))
```suggestion
merged |= extra
```
</issue_to_address>
### Comment 6
<location> `cuprum/sh.py:229-231` </location>
<code_context>
async def _consume_stream(
stream: asyncio.StreamReader | None,
*,
capture_output: bool,
echo_output: bool,
sink: typ.IO[str],
) -> str | None:
"""Read from a subprocess stream, teeing to sink when requested."""
if stream is None:
return "" if capture_output else None
buffer = bytearray() if capture_output else None
while True:
chunk = await stream.read(_READ_SIZE)
if not chunk:
break
if buffer is not None:
buffer.extend(chunk)
if echo_output:
_write_chunk(sink, chunk)
if buffer is None:
return None
return buffer.decode("utf-8", errors="replace")
</code_context>
<issue_to_address>
**suggestion (code-quality):** We've found these issues:
- Lift code into else after jump in control flow ([`reintroduce-else`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/reintroduce-else/))
- Replace if statement with if expression ([`assign-if-exp`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/assign-if-exp/))
```suggestion
return None if buffer is None else buffer.decode("utf-8", errors="replace")
```
</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: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (8)
cuprum/__init__.py(2 hunks)cuprum/sh.py(5 hunks)cuprum/unittests/test_safe_cmd_run.py(1 hunks)docs/cuprum-design.md(1 hunks)docs/roadmap.md(1 hunks)docs/users-guide.md(1 hunks)tests/behaviour/test_execution_runtime.py(1 hunks)tests/features/execution_runtime.feature(1 hunks)
🧰 Additional context used
📓 Path-based instructions (10)
docs/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
docs/**/*.md: Use markdown files within thedocs/directory as a knowledge base and source of truth for project requirements, dependency choices, and architectural decisions.
When new decisions are made, requirements change, libraries are added/removed, or architectural patterns evolve, proactively update the relevant file(s) in thedocs/directory to reflect the latest state.
All documentation must adhere to the documentation style guide atdocs/documentation-style-guide.md.
Record any design decisions made in the relevant design document.
Files:
docs/roadmap.mddocs/users-guide.mddocs/cuprum-design.md
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
**/*.md: For Markdown files (.md only), ensure linting passes by runningmake markdownlint.
For Markdown files, validate Mermaid diagrams by runningmake nixie.
Markdown paragraphs and bullet points must be wrapped at 80 columns.
Code blocks in Markdown must be wrapped at 120 columns.
Markdown tables and headings must not be wrapped.
Use dashes (-) for list bullets in Markdown.
Use GitHub-flavoured Markdown footnotes ([^1]) for references and footnotes.
Validate Markdown files usingmake markdownlint.
Runmake fmtafter any documentation changes to format all Markdown files and fix table markup.
Validate Mermaid diagrams in Markdown files by runningmake nixie.
Files:
docs/roadmap.mddocs/users-guide.mddocs/cuprum-design.md
⚙️ CodeRabbit configuration file
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Headings must not be wrapped.
- Documents must start with a level 1 heading
- Headings must correctly increase or decrease by no more than one level at a time
- Use GitHub-flavoured Markdown style for footnotes and endnotes.
- Numbered footnotes must be numbered by order of appearance in the document.
Files:
docs/roadmap.mddocs/users-guide.mddocs/cuprum-design.md
docs/**/*.{md,mdx,rst,txt}
📄 CodeRabbit inference engine (docs/documentation-style-guide.md)
docs/**/*.{md,mdx,rst,txt}: Use British English based on Oxford English Dictionary (en-GB-oxendict) conventions: use -ize suffixes (realize, organization), -lyse suffixes (analyse, paralyse, catalyse), -our suffixes (colour, behaviour, neighbour), -re suffixes (calibre, centre, fibre), double 'l' (cancelled, counsellor, cruellest), maintain 'e' (likeable, liveable, rateable), -ogue suffixes (analogue, catalogue)
The word 'outwith' is acceptable in documentation
Use the Oxford comma in documentation: 'ships, planes, and hovercraft' where it aids comprehension
Treat company names as collective nouns in documentation, for example 'Lille Industries are expanding'
Write headings in sentence case in documentation
Use Markdown headings (#,##,###, and so on) in order without skipping levels
Always provide a language identifier for fenced code blocks in documentation; use 'plaintext' for non-code text
Use-as the first level bullet and renumber lists when items change in documentation
Prefer inline links using[text](url)or angle brackets around the URL in documentation
Ensure blank lines before and after bulleted lists and fenced blocks in documentation
Ensure tables have a delimiter line below the header row in documentation
Expand any uncommon acronym on first use in documentation, for example 'Continuous Integration (CI)'
Wrap paragraphs at 80 columns in documentation
Wrap code at 120 columns in documentation
Do not wrap tables in documentation
Use footnotes referenced with[^label]in documentation
Include Mermaid diagrams in documentation where they add clarity
When embedding figures in documentation, useand provide brief alt text describing the content
Add a short description before each Mermaid diagram in documentation so screen readers can understand it
Files:
docs/roadmap.mddocs/users-guide.mddocs/cuprum-design.md
docs/**/*.{md,mdx,rst,txt,rs}
📄 CodeRabbit inference engine (docs/documentation-style-guide.md)
Keep US spelling when used in API contexts, for example 'color'
Files:
docs/roadmap.mddocs/users-guide.mddocs/cuprum-design.md
docs/**/*.{md,mdx}
📄 CodeRabbit inference engine (docs/documentation-style-guide.md)
Follow markdownlint recommendations for Markdown formatting
Files:
docs/roadmap.mddocs/users-guide.mddocs/cuprum-design.md
docs/users-guide.md
📄 CodeRabbit inference engine (AGENTS.md)
docs/users-guide.md: Ensure new functionality is clearly documented in thedocs/users-guide.mdfile.
Ensure revised functionality is clearly documented in thedocs/users-guide.mdfile.
Files:
docs/users-guide.md
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: For Python files, ensure testing passes all relevant unit and behavioral tests by runningmake test.
For Python files, ensure linting passes by runningmake lint.
For Python files, ensure formatting adheres to standards by runningmake check-fmtand applyingmake fmtif needed.
For Python files, ensure type checking passes by runningmake typecheck.
For Python development, refer to Python-specific guidelines in the.rules/directory, including Python Code Style Guidelines, Context Managers, Exceptions and Logging, Generators, Project Configuration, Return Patterns, and Typing.
**/*.py: Use snake_case for Python filenames (e.g., http_client.py, task_queue.py)
Use PascalCase for class names
Use snake_case for variable and function names
Use UPPER_SNAKE_CASE for module-level constants
Prefix private/internal functions and variables with a single underscore (_)
Enable full static type coverage using Pyright and maintain typing throughout the codebase
Use TypedDict or Dataclass for structured data, preferring @DataClass(slots=True) for internal-only usage
Avoid using Any type; use Unknown, generics, or cast() with documentation instead
Provide explicit return type annotations (e.g., -> None, -> str) for all public functions and class methods
Enforce strict mode in Pyright and treat all Pyright warnings as CI errors; use # pyright: ignore sparingly with explanation
Avoid side effects at import time; modules should not modify global state or perform actions on import
Use .env or settings modules for environment-specific configuration; never hardcode secrets
Use Ruff for formatting; let Ruff handle whitespace and formatting entirely
Use NumPy-format docstrings for public functions, classes, and modules
Use inline comments to explain tricky or non-obvious code logic and decisions
**/*.py: Use context managers (withcontextlib.contextmanageror class-based__enter__/__exit__) to encapsulate setup and teardown logic for resource management (f...
Files:
cuprum/__init__.pytests/behaviour/test_execution_runtime.pycuprum/unittests/test_safe_cmd_run.pycuprum/sh.py
⚙️ CodeRabbit configuration file
**/*.py: - Keep C90 / mccabe complexity ≤ 9
- Follow single responsibility and CQRS (command/query segregation)
- Prefer structural pattern matching to
- Prefer structural pattern matching over
isinstance()or imperative decomposition.- Docstrings must follow the
numpystyle guide. Use a single-line summary for private functions and methods, and full structured docs for all public interfaces.- Move conditionals with >2 branches to predicate/helper functions
- Avoid
eval,exec,pickle, monkey-patching,ctypes, unsafe shell- Every module must begin with a triple-quoted docstring explaining its purpose, utility, and usage, including example calls if appropriate.
- Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Lint suppressions:
- Blanket
# noqa, file-level skips, and categories are forbidden- Only narrow in-line disables (
# noqa: XYZ) are permitted, and must be accompanied byFIXME:or a ticket link, and used only as a last resort.- Use
pytestfixtures for shared setup (conftest.pyorfixtures/)- Replace duplicate tests with
@pytest.mark.parametrize- Prefer
pytest-mockorunittest.mockfor stubs/mocks- Use
assert …, "message"over bare asserts- Reflect all API/behaviour changes in
docs/and update roadmap on completion- Files must not exceed 400 logical lines:
- Decompose large modules into subpackages
- Split large
match/caseor dispatch tables by domain and collocate with targets if appropriate- Move bulky data (fixtures, templates) to external files for parsing at runtime
- Mutable defaults and shadowed built-ins are forbidden
- All code must have clear type hints using modern style (
A | B,list[str],class Foo[A]:,type Bar = int, etc.), with ABC imports drawn from the correct stdlib module.- All path manipulation must be performed using pathlib for cross platform safety. Do not use string manipulation for extraction of path components or filename ele...
Files:
cuprum/__init__.pytests/behaviour/test_execution_runtime.pycuprum/unittests/test_safe_cmd_run.pycuprum/sh.py
**/test_*.py
📄 CodeRabbit inference engine (.rules/python-00.md)
**/test_*.py: Use pytest idioms: prefer fixtures over setup/teardown methods, parametrize broadly, avoid unnecessary mocks
Group related tests using class with method names prefixed by test_
Write tests from a user's perspective; test public behavior rather than internals
Avoid mocking too much; prefer test doubles only for external services or non-deterministic behaviors
Files:
tests/behaviour/test_execution_runtime.pycuprum/unittests/test_safe_cmd_run.py
**/*test*.py
📄 CodeRabbit inference engine (.rules/python-exception-design-raising-handling-and-logging.md)
Use specific exception types and message constraints with
pytest.raises(SpecificError, match=r"pattern")in tests; avoid overly broad exception assertions (B017)
Files:
tests/behaviour/test_execution_runtime.pycuprum/unittests/test_safe_cmd_run.py
**/unittests/test_*.py
📄 CodeRabbit inference engine (.rules/python-00.md)
Colocate unit tests with code using an unittests subdirectory with test_ prefix (e.g., user_auth/unittests/test_models.py)
Files:
cuprum/unittests/test_safe_cmd_run.py
🧠 Learnings (1)
📚 Learning: 2025-12-04T02:32:27.233Z
Learnt from: CR
Repo: leynos/cuprum PR: 0
File: docs/roadmap.md:0-0
Timestamp: 2025-12-04T02:32:27.233Z
Learning: Applies to docs/**/*.py : Implement async `SafeCmd.run` with capture/echo toggles, env/cwd overrides, structured result object, and cancellation that sends terminate then kill after a grace period; add integration tests that assert cleanup on cancellation.
Applied to files:
docs/roadmap.md
🧬 Code graph analysis (2)
cuprum/__init__.py (1)
cuprum/sh.py (3)
CommandResult(70-83)ExecutionContext(87-92)SafeCmd(96-177)
tests/behaviour/test_execution_runtime.py (2)
cuprum/catalogue.py (2)
ProgramCatalogue(56-119)ProjectSettings(30-40)cuprum/sh.py (6)
ExecutionContext(87-92)CommandResult(70-83)SafeCmd(96-177)builder(192-194)make(180-196)run(108-177)
🪛 LanguageTool
docs/users-guide.md
[uncategorized] ~115-~115: Loose punctuation mark.
Context: ... escalating to SIGKILL. - exit_code, pid, and ok on the CommandResult ...
(UNLIKELY_OPENING_PUNCTUATION)
[locale-violation] ~115-~115: Lower-case ‘ok’ is American English. For British English use “OK”.
Context: ...to SIGKILL. - exit_code, pid, and ok on the CommandResult make it easy to...
(OK)
docs/cuprum-design.md
[locale-violation] ~797-~797: Lower-case ‘ok’ is American English. For British English use “OK”.
Context: ...err, exit_code, and pid, plus an ok` helper for convenience. - Output strea...
(OK)
🔍 Remote MCP
Based on the context provided in the PR, I have sufficient information to provide a comprehensive summary. The PR documentation includes design files, user guides, and test cases that detail the implementation. Let me synthesize the relevant findings:
Review Context Summary
Key Design Decisions & Architecture
From the PR documentation provided, the implementation introduces several critical design patterns:
-
CommandResult Structure: A frozen immutable dataclass with fields (program, argv, exit_code, pid, stdout, stderr) and an
okproperty comparing exit_code to 0. This provides a structured, type-safe return value for command execution. -
ExecutionContext Dataclass: Groups three related parameters—
env(environment overrides),cwd(working directory), andcancel_grace(cancellation timeout in seconds)—into a single configuration object. This addresses the parameter count threshold issue noted by the reviewer (reducing from 5 to 3 parameters excluding self). -
Async Execution Pattern: SafeCmd.run uses
asyncio.create_subprocess_execwith concurrent stdout/stderr reading and UTF-8 decoding with replacement handling, avoiding blocking I/O and potential deadlocks in pipelines. -
Graceful Cancellation: Implements a two-stage termination flow—sends SIGTERM and waits for the configured grace period (default 0.5s), then sends SIGKILL if still running—ensuring no orphaned processes.
Backward Compatibility
The changes maintain backward compatibility with existing SafeCmd API. SafeCmd itself remains unchanged; run() is a new method, not a modification to existing behavior. Environment overrides are merged onto os.environ without mutating global state.
Public API Expansion
Both CommandResult and ExecutionContext are exported from the public API via cuprum.__init__, extending the top-level names alongside existing SafeCmd and SafeCmdBuilder.
Test Coverage
The PR includes three test layers:
- Unit tests: Five tests covering capture, echo, env/cwd overrides, and disabled capture
- Behavioral tests: Default capture and cancellation cleanup verification
- Feature specifications: BDD-style scenarios documenting expected behavior
Documentation
Implementation details are documented in docs/cuprum-design.md (8.1-8.2 sections), user guide examples in docs/users-guide.md, and roadmap completion in docs/roadmap.md.
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Sourcery review
- GitHub Check: Sourcery review
🔇 Additional comments (14)
cuprum/sh.py (7)
1-34: LGTM!Imports and type aliases are well-organised. The module docstring accurately reflects the new async runtime capability.
69-84: LGTM!Immutable dataclass with slots is an excellent choice for structured results. The
okproperty provides a clean success check.
86-93: LGTM!The
ExecutionContextdataclass cleanly groups runtime configuration, reducing the parameter count onrun()as recommended in the PR discussion.
199-205: LGTM!Correct copy-then-update pattern that avoids mutating
os.environ.
208-231: LGTM!Efficient stream consumption with proper UTF-8 decoding and replacement handling. The bytearray buffer avoids repeated string concatenation overhead.
246-259: LGTM!Robust termination logic with proper SIGTERM→SIGKILL escalation. The defensive clamping of
grace_periodprevents negative timeouts.
262-269: LGTM!Public API surface is correctly declared.
tests/features/execution_runtime.feature (1)
1-12: LGTM!Feature scenarios are well-defined and align with the runtime behaviour documented in the user guide. The Given/When/Then structure is clear and testable.
docs/roadmap.md (1)
24-27: LGTM!Roadmap correctly updated to reflect completion of the async
SafeCmd.runimplementation with all specified features.docs/users-guide.md (1)
99-133: LGTM!The execution runtime documentation is clear and comprehensive. The example code demonstrates the key features effectively.
Regarding the static analysis hint about
okbeing American English: this is a false positive sinceokis the property name onCommandResultand must match the code.cuprum/__init__.py (2)
30-30: LGTM!Public API correctly expanded to expose
CommandResultandExecutionContext.
45-46: LGTM!The
__all__list is correctly updated and maintains alphabetical ordering.docs/cuprum-design.md (1)
794-804: Keep implementation notes aligned with current runtime behaviourRetain these bullets as they accurately document
SafeCmd.runreturningCommandResult, UTF‑8 decoding with replacement, environment overlays viaExecutionContext, and terminate‑then‑kill cancellation semantics.tests/behaviour/test_execution_runtime.py (1)
110-169: Retain cancellation orchestration and cleanup logicKeep the
when_cancel_commandand_wait_for_pidlogic as written. The test reliably:
- Starts the long‑running worker under
ExecutionContextwith an env override.- Waits for the worker to publish its PID.
- Cancels the
runtask and asserts propagation ofCancelledError.- Verifies child termination within a bounded deadline using
_is_process_alive.This gives high confidence that
SafeCmd.runimplements the documented terminate‑then‑kill semantics and cleans up I/O tasks correctly.
- run() now returns CommandResult with exit code for non-capture, non-echo runs - use new async process wait and cancellation logic - improve environment merging with |= operator - add tests for run capturing non-zero exit and ok flag - add tests for non-cooperative subprocess cancellation escalation - misc improvements and type hints Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
leynos
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
cuprum/unittests/test_safe_cmd_run.py (2)
66-82: Harden environment leak assertion against pre-existing variables.The current assertion fails if
CUPRUM_TEST_ENVis already set in the test environment. Capture the original environment value before running the command and assert that it remains unchanged afterwards.Apply this diff:
def test_run_applies_env_overrides( python_builder: typ.Callable[..., SafeCmd], ) -> None: """run() overlays provided env vars on top of the current environment.""" env_var = "CUPRUM_TEST_ENV" + original_value = os.environ.get(env_var) command = python_builder( "-c", f"import os;print(os.getenv('{env_var}'))", ) result = asyncio.run( command.run(context=ExecutionContext(env={env_var: "present"})), ) assert result.stdout is not None assert result.stdout.strip() == "present" - assert env_var not in os.environ, "Environment overlays must not leak globally" + assert os.environ.get(env_var) == original_value, ( + "Environment overlays must not leak globally" + )As per coding guidelines, stabilise tests against ambient environment state.
🤖 Prompt for AI Agents
In cuprum/unittests/test_safe_cmd_run.py around lines 66 to 82, the test currently asserts that the env var is absent after running the command which fails if the variable existed beforehand; capture the original value with original_value = os.environ.get(env_var) before invoking command.run, then after the run assert os.environ.get(env_var) == original_value (instead of checking absence) to ensure the environment was not mutated by the overlay; keep the other assertions about result.stdout unchanged.
97-109: Fix cwd assertion to be platform-independent.Avoid comparing POSIX string paths to
os.getcwd()output directly, which breaks on Windows due to path separator differences. Normalise both sides asPathobjects before comparison.Apply this diff:
def test_run_applies_cwd_override( python_builder: typ.Callable[..., SafeCmd], tmp_path: Path, ) -> None: """run() executes in the provided working directory when supplied.""" working_dir = tmp_path / "work" working_dir.mkdir() command = python_builder("-c", "import os;print(os.getcwd())") result = asyncio.run(command.run(context=ExecutionContext(cwd=working_dir))) assert result.stdout is not None - assert result.stdout.strip() == working_dir.as_posix() + cwd_result = Path(result.stdout.strip()) + assert cwd_result == working_dirAs per coding guidelines, normalise and compare paths using
pathlibto preserve cross-platform behaviour.🤖 Prompt for AI Agents
In cuprum/unittests/test_safe_cmd_run.py around lines 97 to 109, the test compares the command stdout string to working_dir.as_posix() which fails on Windows; update the assertion to normalise both sides as pathlib.Path objects before comparing (e.g., convert result.stdout to a Path and compare to working_dir or use Path(result.stdout.strip()) == working_dir) so the test is platform-independent.cuprum/sh.py (1)
108-192: Async execution runtime is well-structured, but fix PID fallback edge case.The run method correctly implements subprocess spawning, I/O streaming, and cancellation escalation. The overall structure is sound and follows asyncio best practices.
However, the expression
process.pid or -1on lines 151 and 189 treats0as falsy. While PID 0 is reserved for the kernel and should never appear for a subprocess, use an explicitNonecheck for defensive correctness.Apply this diff:
return CommandResult( program=self.program, argv=self.argv, exit_code=exit_code, - pid=process.pid or -1, + pid=process.pid if process.pid is not None else -1, stdout=None, stderr=None, ) stdout_task = asyncio.create_task( _consume_stream( process.stdout, capture_output=capture, echo_output=echo, sink=sys.stdout, ), ) stderr_task = asyncio.create_task( _consume_stream( process.stderr, capture_output=capture, echo_output=echo, sink=sys.stderr, ), ) try: exit_code = await process.wait() except asyncio.CancelledError: await _terminate_process(process, ctx.cancel_grace) await asyncio.gather(stdout_task, stderr_task, return_exceptions=True) raise stdout_text, stderr_text = await asyncio.gather( stdout_task, stderr_task, ) return CommandResult( program=self.program, argv=self.argv, exit_code=exit_code, - pid=process.pid or -1, + pid=process.pid if process.pid is not None else -1, stdout=stdout_text, stderr=stderr_text, )🤖 Prompt for AI Agents
In cuprum/sh.py on lines 151 and 189, change `process.pid or -1` to `process.pid if process.pid is not None else -1` to use an explicit None check instead of relying on truthiness, avoiding incorrect treatment of a hypothetical 0 PID value.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
cuprum/sh.py(5 hunks)cuprum/unittests/test_safe_cmd_run.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: For Python files, ensure testing passes all relevant unit and behavioral tests by runningmake test.
For Python files, ensure linting passes by runningmake lint.
For Python files, ensure formatting adheres to standards by runningmake check-fmtand applyingmake fmtif needed.
For Python files, ensure type checking passes by runningmake typecheck.
For Python development, refer to Python-specific guidelines in the.rules/directory, including Python Code Style Guidelines, Context Managers, Exceptions and Logging, Generators, Project Configuration, Return Patterns, and Typing.
**/*.py: Use snake_case for Python filenames (e.g., http_client.py, task_queue.py)
Use PascalCase for class names
Use snake_case for variable and function names
Use UPPER_SNAKE_CASE for module-level constants
Prefix private/internal functions and variables with a single underscore (_)
Enable full static type coverage using Pyright and maintain typing throughout the codebase
Use TypedDict or Dataclass for structured data, preferring @DataClass(slots=True) for internal-only usage
Avoid using Any type; use Unknown, generics, or cast() with documentation instead
Provide explicit return type annotations (e.g., -> None, -> str) for all public functions and class methods
Enforce strict mode in Pyright and treat all Pyright warnings as CI errors; use # pyright: ignore sparingly with explanation
Avoid side effects at import time; modules should not modify global state or perform actions on import
Use .env or settings modules for environment-specific configuration; never hardcode secrets
Use Ruff for formatting; let Ruff handle whitespace and formatting entirely
Use NumPy-format docstrings for public functions, classes, and modules
Use inline comments to explain tricky or non-obvious code logic and decisions
**/*.py: Use context managers (withcontextlib.contextmanageror class-based__enter__/__exit__) to encapsulate setup and teardown logic for resource management (f...
Files:
cuprum/unittests/test_safe_cmd_run.pycuprum/sh.py
⚙️ CodeRabbit configuration file
**/*.py: - Keep C90 / mccabe complexity ≤ 9
- Follow single responsibility and CQRS (command/query segregation)
- Prefer structural pattern matching to
- Prefer structural pattern matching over
isinstance()or imperative decomposition.- Docstrings must follow the
numpystyle guide. Use a single-line summary for private functions and methods, and full structured docs for all public interfaces.- Move conditionals with >2 branches to predicate/helper functions
- Avoid
eval,exec,pickle, monkey-patching,ctypes, unsafe shell- Every module must begin with a triple-quoted docstring explaining its purpose, utility, and usage, including example calls if appropriate.
- Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Lint suppressions:
- Blanket
# noqa, file-level skips, and categories are forbidden- Only narrow in-line disables (
# noqa: XYZ) are permitted, and must be accompanied byFIXME:or a ticket link, and used only as a last resort.- Use
pytestfixtures for shared setup (conftest.pyorfixtures/)- Replace duplicate tests with
@pytest.mark.parametrize- Prefer
pytest-mockorunittest.mockfor stubs/mocks- Use
assert …, "message"over bare asserts- Reflect all API/behaviour changes in
docs/and update roadmap on completion- Files must not exceed 400 logical lines:
- Decompose large modules into subpackages
- Split large
match/caseor dispatch tables by domain and collocate with targets if appropriate- Move bulky data (fixtures, templates) to external files for parsing at runtime
- Mutable defaults and shadowed built-ins are forbidden
- All code must have clear type hints using modern style (
A | B,list[str],class Foo[A]:,type Bar = int, etc.), with ABC imports drawn from the correct stdlib module.- All path manipulation must be performed using pathlib for cross platform safety. Do not use string manipulation for extraction of path components or filename ele...
Files:
cuprum/unittests/test_safe_cmd_run.pycuprum/sh.py
**/unittests/test_*.py
📄 CodeRabbit inference engine (.rules/python-00.md)
Colocate unit tests with code using an unittests subdirectory with test_ prefix (e.g., user_auth/unittests/test_models.py)
Files:
cuprum/unittests/test_safe_cmd_run.py
**/test_*.py
📄 CodeRabbit inference engine (.rules/python-00.md)
**/test_*.py: Use pytest idioms: prefer fixtures over setup/teardown methods, parametrize broadly, avoid unnecessary mocks
Group related tests using class with method names prefixed by test_
Write tests from a user's perspective; test public behavior rather than internals
Avoid mocking too much; prefer test doubles only for external services or non-deterministic behaviors
Files:
cuprum/unittests/test_safe_cmd_run.py
**/*test*.py
📄 CodeRabbit inference engine (.rules/python-exception-design-raising-handling-and-logging.md)
Use specific exception types and message constraints with
pytest.raises(SpecificError, match=r"pattern")in tests; avoid overly broad exception assertions (B017)
Files:
cuprum/unittests/test_safe_cmd_run.py
🧬 Code graph analysis (1)
cuprum/unittests/test_safe_cmd_run.py (2)
cuprum/catalogue.py (3)
ProgramCatalogue(56-119)ProjectSettings(30-40)allowlist(67-69)cuprum/sh.py (5)
ExecutionContext(87-92)SafeCmd(96-192)make(195-211)run(108-192)ok(81-83)
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (15)
cuprum/unittests/test_safe_cmd_run.py (7)
1-20: LGTM: Clean module structure and imports.The module docstring is clear, imports are well-organized, and the TYPE_CHECKING guard correctly defers the SafeCmd import to avoid circular dependencies.
22-31: LGTM: Test catalogue construction is clear.The helper correctly constructs a test-specific catalogue containing the current Python interpreter.
34-39: LGTM: Fixture provides clean test helper.The fixture correctly constructs a SafeCmd builder for the Python interpreter and is properly typed.
42-51: LGTM: Basic capture test is correct.The test verifies default capture semantics, exit code, and the
okconvenience property.
54-63: LGTM: Echo test correctly validates tee behaviour.The test properly uses
capfdto verify that output is both captured and echoed.
85-94: LGTM: Non-zero exit code test is correct.The test properly verifies that non-zero exit codes are captured and reflected in the
okflag.
112-120: LGTM: Capture disabling test is correct.The test properly verifies that
capture=Falseprevents stdout/stderr retention.cuprum/sh.py (8)
1-67: LGTM: Module structure and imports are clean.The updated module docstring accurately reflects the new async runtime capabilities. Type aliases and constants follow coding guidelines.
69-83: LGTM: CommandResult dataclass is well-designed.The immutable dataclass with slots correctly represents execution results, and the
okproperty provides a convenient success predicate.
86-92: LGTM: ExecutionContext dataclass is correctly structured.The immutable dataclass cleanly groups runtime parameters with sensible defaults.
214-220: LGTM: Environment overlay helper is correct.The function correctly overlays extra environment variables atop a copy of
os.environwithout mutating global state.
223-244: LGTM: Stream consumption logic is correct.The function properly handles concurrent reading, optional capture, and optional echoing with appropriate UTF-8 decoding and error handling.
247-260: LGTM: Helper correctly writes chunks with accurate documentation.The function appropriately writes bytes to text sinks, preferring the raw buffer when available. The docstring correctly describes the synchronous nature of the writes, addressing previous concerns.
263-276: LGTM: Process termination implements correct escalation.The helper properly implements graceful termination with SIGTERM followed by SIGKILL after the grace period expires. The exception handling and noqa comment are appropriate.
279-286: LGTM: Public API exports are correctly defined.The
__all__list properly exposes the execution runtime types and functions, aligning with the PR objectives.
Extract Python executable catalogue and SafeCmd builder to shared helper module tests/helpers/catalogue.py. Update tests to use these shared helpers to reduce duplication and improve maintainability. Also fix SafeCmd pid extraction logic to handle None explicitly. Cleanup path string conversions in tests for consistency. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
cuprum/sh.py(5 hunks)cuprum/unittests/test_safe_cmd_run.py(1 hunks)tests/behaviour/test_execution_runtime.py(1 hunks)tests/helpers/__init__.py(1 hunks)tests/helpers/catalogue.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: For Python files, ensure testing passes all relevant unit and behavioral tests by runningmake test.
For Python files, ensure linting passes by runningmake lint.
For Python files, ensure formatting adheres to standards by runningmake check-fmtand applyingmake fmtif needed.
For Python files, ensure type checking passes by runningmake typecheck.
For Python development, refer to Python-specific guidelines in the.rules/directory, including Python Code Style Guidelines, Context Managers, Exceptions and Logging, Generators, Project Configuration, Return Patterns, and Typing.
**/*.py: Use snake_case for Python filenames (e.g., http_client.py, task_queue.py)
Use PascalCase for class names
Use snake_case for variable and function names
Use UPPER_SNAKE_CASE for module-level constants
Prefix private/internal functions and variables with a single underscore (_)
Enable full static type coverage using Pyright and maintain typing throughout the codebase
Use TypedDict or Dataclass for structured data, preferring @DataClass(slots=True) for internal-only usage
Avoid using Any type; use Unknown, generics, or cast() with documentation instead
Provide explicit return type annotations (e.g., -> None, -> str) for all public functions and class methods
Enforce strict mode in Pyright and treat all Pyright warnings as CI errors; use # pyright: ignore sparingly with explanation
Avoid side effects at import time; modules should not modify global state or perform actions on import
Use .env or settings modules for environment-specific configuration; never hardcode secrets
Use Ruff for formatting; let Ruff handle whitespace and formatting entirely
Use NumPy-format docstrings for public functions, classes, and modules
Use inline comments to explain tricky or non-obvious code logic and decisions
**/*.py: Use context managers (withcontextlib.contextmanageror class-based__enter__/__exit__) to encapsulate setup and teardown logic for resource management (f...
Files:
tests/helpers/__init__.pycuprum/sh.pytests/behaviour/test_execution_runtime.pytests/helpers/catalogue.pycuprum/unittests/test_safe_cmd_run.py
⚙️ CodeRabbit configuration file
**/*.py: - Keep C90 / mccabe complexity ≤ 9
- Follow single responsibility and CQRS (command/query segregation)
- Prefer structural pattern matching to
- Prefer structural pattern matching over
isinstance()or imperative decomposition.- Docstrings must follow the
numpystyle guide. Use a single-line summary for private functions and methods, and full structured docs for all public interfaces.- Move conditionals with >2 branches to predicate/helper functions
- Avoid
eval,exec,pickle, monkey-patching,ctypes, unsafe shell- Every module must begin with a triple-quoted docstring explaining its purpose, utility, and usage, including example calls if appropriate.
- Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Lint suppressions:
- Blanket
# noqa, file-level skips, and categories are forbidden- Only narrow in-line disables (
# noqa: XYZ) are permitted, and must be accompanied byFIXME:or a ticket link, and used only as a last resort.- Use
pytestfixtures for shared setup (conftest.pyorfixtures/)- Replace duplicate tests with
@pytest.mark.parametrize- Prefer
pytest-mockorunittest.mockfor stubs/mocks- Use
assert …, "message"over bare asserts- Reflect all API/behaviour changes in
docs/and update roadmap on completion- Files must not exceed 400 logical lines:
- Decompose large modules into subpackages
- Split large
match/caseor dispatch tables by domain and collocate with targets if appropriate- Move bulky data (fixtures, templates) to external files for parsing at runtime
- Mutable defaults and shadowed built-ins are forbidden
- All code must have clear type hints using modern style (
A | B,list[str],class Foo[A]:,type Bar = int, etc.), with ABC imports drawn from the correct stdlib module.- All path manipulation must be performed using pathlib for cross platform safety. Do not use string manipulation for extraction of path components or filename ele...
Files:
tests/helpers/__init__.pycuprum/sh.pytests/behaviour/test_execution_runtime.pytests/helpers/catalogue.pycuprum/unittests/test_safe_cmd_run.py
**/test_*.py
📄 CodeRabbit inference engine (.rules/python-00.md)
**/test_*.py: Use pytest idioms: prefer fixtures over setup/teardown methods, parametrize broadly, avoid unnecessary mocks
Group related tests using class with method names prefixed by test_
Write tests from a user's perspective; test public behavior rather than internals
Avoid mocking too much; prefer test doubles only for external services or non-deterministic behaviors
Files:
tests/behaviour/test_execution_runtime.pycuprum/unittests/test_safe_cmd_run.py
**/*test*.py
📄 CodeRabbit inference engine (.rules/python-exception-design-raising-handling-and-logging.md)
Use specific exception types and message constraints with
pytest.raises(SpecificError, match=r"pattern")in tests; avoid overly broad exception assertions (B017)
Files:
tests/behaviour/test_execution_runtime.pycuprum/unittests/test_safe_cmd_run.py
**/unittests/test_*.py
📄 CodeRabbit inference engine (.rules/python-00.md)
Colocate unit tests with code using an unittests subdirectory with test_ prefix (e.g., user_auth/unittests/test_models.py)
Files:
cuprum/unittests/test_safe_cmd_run.py
🔍 Remote MCP Ref
Now let me search for best practices on function parameter organization:## Summary of Additional Context Found
1. Function Parameter Design and Dataclass Pattern
Four or more function parameters is usually considered bad practice, which aligns with the PR's threshold of 4. Creating a dataclass or other structure to group multiple arguments is indeed a useful pattern. By breaking out parameters into a dataclass, developers gain advantages including standard documentation practices, type hinting, and the ability to set reasonable defaults.
The PR already implements this refactoring suggestion: Grouping arguments in a dataclass is a nice way to organize and document arguments, and it also simplifies type hinting of functions.
2. Asyncio Subprocess Cancellation Best Practices
The PR's cancellation implementation aligns with Python asyncio patterns. By catching asyncio.CancelledError, any running task can perform necessary cleanup when it's cancelled. The PR implements a two-stage termination strategy: first SIGTERM via proc.terminate(), then SIGKILL via proc.kill() after a grace period.
Key implementation details from Python documentation: The terminate() method sends SIGTERM to stop the child process on POSIX systems, while kill() sends SIGKILL. The grace period (cancel_grace defaulting to 0.5 seconds) is a standard pattern for allowing processes time to clean up before forced termination.
3. ExecutionContext Design
The PR's ExecutionContext dataclass groups three execution-time parameters:
env: Environment variable overlayscwd: Working directory overridecancel_grace: Graceful shutdown timeout
This pattern addresses the principle of grouping parameters bound to related concerns, allowing a User-like struct to be passed instead of individual parameters.
4. Return Type Documentation
The PR's CommandResult dataclass with fields (exit_code, pid, stdout, stderr, ok) follows asyncio subprocess conventions. The ok property provides a convenient way to check success status (comparing exit_code to 0).
5. Test Coverage Expectations
The PR includes comprehensive test coverage:
- Unit tests for capture, echo, env, cwd, and cancellation scenarios
- Behavioral/feature tests validating default capture and cancellation cleanup
- Test helpers (
python_catalogue,python_builder) for creating Python subprocess fixtures
This aligns with async best practices requiring explicit cleanup testing and signal handler verification.
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (10)
tests/helpers/__init__.py (1)
1-1: Keep the package docstring as-isRetain this concise package-level docstring; it correctly documents the helpers namespace and satisfies the module-docstring guideline.
cuprum/sh.py (5)
69-92: Retain CommandResult and ExecutionContext shapesKeep these frozen, slotted dataclasses; they give a clear, typed contract for execution results and runtime configuration, and the
okproperty is a useful convenience for callers.
108-192: Keep SafeCmd.run cancellation and I/O orchestration as implementedLeave this async execution flow intact: env/cwd resolution, conditional PIPE/DEVNULL wiring, concurrent stdout/stderr consumption, and the
CancelledErrorpath that terminates with grace then re‑raises are coherent and match the tests for both co‑operative and non‑co‑operative children. Ensuremake test,make lint, andmake typecheckrun clean after any further tweaks to this method.
214-221: Preserve _merge_env overlay semanticsKeep
_merge_envreturningNonewhen no extra mapping is provided and returning a copied, overlaiddictotherwise; this avoids unnecessary copying, honours the parent environment by default, and satisfies the env overlay tests and documentation.
263-277: Retain _terminate_process escalation strategyKeep this termination helper as written: clamping negative grace to zero, short‑circuiting on already‑finished processes, sending terminate then waiting with
asyncio.wait_for, and escalating to kill onasyncio.TimeoutErrormatches the intended SIGTERM→SIGKILL semantics and is covered by the non‑co‑operative subprocess test.
279-286: Export the new runtime types via allContinue exporting
CommandResultandExecutionContextalongsideSafeCmd,SafeCmdBuilder,UnknownProgramError, andmake; this aligns the public API with the new tests and documentation that import these symbols fromcuprum.sh/cuprum.tests/behaviour/test_execution_runtime.py (2)
24-68: Keep behavioural coverage for capture semanticsRetain these BDD steps and scenarios; they exercise the default capture behaviour of
SafeCmd.runend‑to‑end and assert on the structuredCommandResultshape from a user perspective.
70-143: Keep cancellation scenario and PID polling logicLeave this long‑running command scenario and the
_wait_for_pid/_is_process_alivehelpers in place; they give valuable behavioural coverage for cancellation, termination, and cleanup of a co‑operative child process.tests/helpers/catalogue.py (1)
17-33: Retain shared Python catalogue and builder helpersKeep
python_catalogueandpython_builderas the centralised way to construct a Python-focusedProgramCatalogueandSafeCmdbuilder; they remove duplication from tests and clearly describe the single-allowlisted-program contract.cuprum/unittests/test_safe_cmd_run.py (1)
26-109: Keep core SafeCmd.run unit coverageRetain these tests for capture/echo, env overlays, non-zero exits, cwd override, and disabling capture; they exercise the primary runtime API surface and align with the documented behaviour.
- Changed env variable key from 'CUPRUM_PID_FILE' to 'CU_PR_PID_FILE' in unittests for consistency. - Converted path to string consistently using str(path) instead of .as_posix(). - Removed unused env from ExecutionContext in test_safe_cmd_run.py. - Added a wait loop to ensure PID file creation within 5 seconds to avoid flaky asynchronous test failures. - Improved robustness of cancellation test by handling possible CI slowness. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
Updated the _write_chunk function docstring to explain that the blocking write to stdio echo is acceptable and future handling of slow sinks can be implemented with a background writer. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
Add a new scenario to verify that cancellation escalates to killing a non-cooperative subprocess after a short grace period. The test spawns a subprocess ignoring termination signals and ensures it is killed after escalation to cover that edge case in subprocess management. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
leynos
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
cuprum/sh.py(5 hunks)cuprum/unittests/test_safe_cmd_run.py(1 hunks)tests/behaviour/test_execution_runtime.py(1 hunks)tests/features/execution_runtime.feature(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: For Python files, ensure testing passes all relevant unit and behavioral tests by runningmake test.
For Python files, ensure linting passes by runningmake lint.
For Python files, ensure formatting adheres to standards by runningmake check-fmtand applyingmake fmtif needed.
For Python files, ensure type checking passes by runningmake typecheck.
For Python development, refer to Python-specific guidelines in the.rules/directory, including Python Code Style Guidelines, Context Managers, Exceptions and Logging, Generators, Project Configuration, Return Patterns, and Typing.
**/*.py: Use snake_case for Python filenames (e.g., http_client.py, task_queue.py)
Use PascalCase for class names
Use snake_case for variable and function names
Use UPPER_SNAKE_CASE for module-level constants
Prefix private/internal functions and variables with a single underscore (_)
Enable full static type coverage using Pyright and maintain typing throughout the codebase
Use TypedDict or Dataclass for structured data, preferring @DataClass(slots=True) for internal-only usage
Avoid using Any type; use Unknown, generics, or cast() with documentation instead
Provide explicit return type annotations (e.g., -> None, -> str) for all public functions and class methods
Enforce strict mode in Pyright and treat all Pyright warnings as CI errors; use # pyright: ignore sparingly with explanation
Avoid side effects at import time; modules should not modify global state or perform actions on import
Use .env or settings modules for environment-specific configuration; never hardcode secrets
Use Ruff for formatting; let Ruff handle whitespace and formatting entirely
Use NumPy-format docstrings for public functions, classes, and modules
Use inline comments to explain tricky or non-obvious code logic and decisions
**/*.py: Use context managers (withcontextlib.contextmanageror class-based__enter__/__exit__) to encapsulate setup and teardown logic for resource management (f...
Files:
cuprum/unittests/test_safe_cmd_run.pycuprum/sh.pytests/behaviour/test_execution_runtime.py
⚙️ CodeRabbit configuration file
**/*.py: - Keep C90 / mccabe complexity ≤ 9
- Follow single responsibility and CQRS (command/query segregation)
- Prefer structural pattern matching to
- Prefer structural pattern matching over
isinstance()or imperative decomposition.- Docstrings must follow the
numpystyle guide. Use a single-line summary for private functions and methods, and full structured docs for all public interfaces.- Move conditionals with >2 branches to predicate/helper functions
- Avoid
eval,exec,pickle, monkey-patching,ctypes, unsafe shell- Every module must begin with a triple-quoted docstring explaining its purpose, utility, and usage, including example calls if appropriate.
- Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Lint suppressions:
- Blanket
# noqa, file-level skips, and categories are forbidden- Only narrow in-line disables (
# noqa: XYZ) are permitted, and must be accompanied byFIXME:or a ticket link, and used only as a last resort.- Use
pytestfixtures for shared setup (conftest.pyorfixtures/)- Replace duplicate tests with
@pytest.mark.parametrize- Prefer
pytest-mockorunittest.mockfor stubs/mocks- Use
assert …, "message"over bare asserts- Reflect all API/behaviour changes in
docs/and update roadmap on completion- Files must not exceed 400 logical lines:
- Decompose large modules into subpackages
- Split large
match/caseor dispatch tables by domain and collocate with targets if appropriate- Move bulky data (fixtures, templates) to external files for parsing at runtime
- Mutable defaults and shadowed built-ins are forbidden
- All code must have clear type hints using modern style (
A | B,list[str],class Foo[A]:,type Bar = int, etc.), with ABC imports drawn from the correct stdlib module.- All path manipulation must be performed using pathlib for cross platform safety. Do not use string manipulation for extraction of path components or filename ele...
Files:
cuprum/unittests/test_safe_cmd_run.pycuprum/sh.pytests/behaviour/test_execution_runtime.py
**/unittests/test_*.py
📄 CodeRabbit inference engine (.rules/python-00.md)
Colocate unit tests with code using an unittests subdirectory with test_ prefix (e.g., user_auth/unittests/test_models.py)
Files:
cuprum/unittests/test_safe_cmd_run.py
**/test_*.py
📄 CodeRabbit inference engine (.rules/python-00.md)
**/test_*.py: Use pytest idioms: prefer fixtures over setup/teardown methods, parametrize broadly, avoid unnecessary mocks
Group related tests using class with method names prefixed by test_
Write tests from a user's perspective; test public behavior rather than internals
Avoid mocking too much; prefer test doubles only for external services or non-deterministic behaviors
Files:
cuprum/unittests/test_safe_cmd_run.pytests/behaviour/test_execution_runtime.py
**/*test*.py
📄 CodeRabbit inference engine (.rules/python-exception-design-raising-handling-and-logging.md)
Use specific exception types and message constraints with
pytest.raises(SpecificError, match=r"pattern")in tests; avoid overly broad exception assertions (B017)
Files:
cuprum/unittests/test_safe_cmd_run.pytests/behaviour/test_execution_runtime.py
🧬 Code graph analysis (1)
cuprum/unittests/test_safe_cmd_run.py (3)
cuprum/sh.py (5)
ExecutionContext(87-92)SafeCmd(96-192)make(195-211)run(108-192)ok(81-83)tests/helpers/catalogue.py (1)
python_builder(29-33)tests/behaviour/test_execution_runtime.py (2)
orchestrate(113-127)orchestrate(216-235)
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (5)
tests/features/execution_runtime.feature (1)
1-17: Keep feature scenarios as the canonical description of runtime behaviourDefine these three scenarios as the contract for default capture, cooperative cancellation, and non‑cooperative escalation; they line up cleanly with the behavioural tests and give clear coverage of the new runtime.
tests/behaviour/test_execution_runtime.py (2)
24-68: Wire BDD scenarios and simple echo flow as the baseline behaviour suiteRetain this structure; the BDD bindings and simple echo scenario give a clear, low-friction check that async SafeCmd.run defaults to captured output with a clean CommandResult surface.
70-143: Exercise cooperative cancellation and cleanup robustlyKeep using the generated Python worker plus
_wait_for_pidand_is_process_alivehere; this pattern gives stable behavioural coverage that cancellation both propagates to the child and results in the subprocess actually disappearing, without relying on arbitrary sleeps.cuprum/unittests/test_safe_cmd_run.py (1)
20-109: Retain this unit suite as the reference for SafeCmd runtime behaviourKeep these tests as the canonical examples: they validate stdout/stderr capture, echoing, env overlays without global leaks, cwd overrides, non-zero exits via
ok, and capture disabling, all via the publicExecutionContextandCommandResultsurfaces.cuprum/sh.py (1)
69-92: Preserve the async execution, streaming, and cancellation structureKeep this design:
CommandResultandExecutionContextgive a clear public API,_merge_envhonours env overlays without leaking intoos.environ,_consume_streamand_write_chunkhandle capture/echo correctly, andSafeCmd.runplus_terminate_processprovide predictable cancellation that terminates the child, waits for a grace period, escalates to a kill if needed, and always drains the stream tasks before bubbling cancellation to the caller. This aligns well with the new tests.Also applies to: 108-192, 214-221, 223-245, 247-276
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
tests/behaviour/test_execution_runtime.py (1)
240-252: Consolidate with shared helper.This is nearly identical to
then_subprocess_stops_cleanly. Once_wait_for_process_deathis extracted (see earlier comment), this becomes:@then("the subprocess is killed after escalation") def then_subprocess_killed_after_escalation( behaviour_state: dict[str, object], ) -> None: """Assert that a stubborn subprocess is eventually killed.""" pid = typ.cast("int", behaviour_state["pid"]) - deadline = time.time() + 5 - while time.time() < deadline: - if not _is_process_alive(pid): - break - time.sleep(0.05) - else: # pragma: no cover - defensive failure - pytest.fail("Non-cooperative subprocess still running after escalation") + _wait_for_process_death(pid, context="escalation after SIGTERM ignored")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
cuprum/sh.py(5 hunks)tests/behaviour/test_execution_runtime.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: For Python files, ensure testing passes all relevant unit and behavioral tests by runningmake test.
For Python files, ensure linting passes by runningmake lint.
For Python files, ensure formatting adheres to standards by runningmake check-fmtand applyingmake fmtif needed.
For Python files, ensure type checking passes by runningmake typecheck.
For Python development, refer to Python-specific guidelines in the.rules/directory, including Python Code Style Guidelines, Context Managers, Exceptions and Logging, Generators, Project Configuration, Return Patterns, and Typing.
**/*.py: Use snake_case for Python filenames (e.g., http_client.py, task_queue.py)
Use PascalCase for class names
Use snake_case for variable and function names
Use UPPER_SNAKE_CASE for module-level constants
Prefix private/internal functions and variables with a single underscore (_)
Enable full static type coverage using Pyright and maintain typing throughout the codebase
Use TypedDict or Dataclass for structured data, preferring @DataClass(slots=True) for internal-only usage
Avoid using Any type; use Unknown, generics, or cast() with documentation instead
Provide explicit return type annotations (e.g., -> None, -> str) for all public functions and class methods
Enforce strict mode in Pyright and treat all Pyright warnings as CI errors; use # pyright: ignore sparingly with explanation
Avoid side effects at import time; modules should not modify global state or perform actions on import
Use .env or settings modules for environment-specific configuration; never hardcode secrets
Use Ruff for formatting; let Ruff handle whitespace and formatting entirely
Use NumPy-format docstrings for public functions, classes, and modules
Use inline comments to explain tricky or non-obvious code logic and decisions
**/*.py: Use context managers (withcontextlib.contextmanageror class-based__enter__/__exit__) to encapsulate setup and teardown logic for resource management (f...
Files:
tests/behaviour/test_execution_runtime.pycuprum/sh.py
⚙️ CodeRabbit configuration file
**/*.py: - Keep C90 / mccabe complexity ≤ 9
- Follow single responsibility and CQRS (command/query segregation)
- Prefer structural pattern matching to
- Prefer structural pattern matching over
isinstance()or imperative decomposition.- Docstrings must follow the
numpystyle guide. Use a single-line summary for private functions and methods, and full structured docs for all public interfaces.- Move conditionals with >2 branches to predicate/helper functions
- Avoid
eval,exec,pickle, monkey-patching,ctypes, unsafe shell- Every module must begin with a triple-quoted docstring explaining its purpose, utility, and usage, including example calls if appropriate.
- Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Lint suppressions:
- Blanket
# noqa, file-level skips, and categories are forbidden- Only narrow in-line disables (
# noqa: XYZ) are permitted, and must be accompanied byFIXME:or a ticket link, and used only as a last resort.- Use
pytestfixtures for shared setup (conftest.pyorfixtures/)- Replace duplicate tests with
@pytest.mark.parametrize- Prefer
pytest-mockorunittest.mockfor stubs/mocks- Use
assert …, "message"over bare asserts- Reflect all API/behaviour changes in
docs/and update roadmap on completion- Files must not exceed 400 logical lines:
- Decompose large modules into subpackages
- Split large
match/caseor dispatch tables by domain and collocate with targets if appropriate- Move bulky data (fixtures, templates) to external files for parsing at runtime
- Mutable defaults and shadowed built-ins are forbidden
- All code must have clear type hints using modern style (
A | B,list[str],class Foo[A]:,type Bar = int, etc.), with ABC imports drawn from the correct stdlib module.- All path manipulation must be performed using pathlib for cross platform safety. Do not use string manipulation for extraction of path components or filename ele...
Files:
tests/behaviour/test_execution_runtime.pycuprum/sh.py
**/test_*.py
📄 CodeRabbit inference engine (.rules/python-00.md)
**/test_*.py: Use pytest idioms: prefer fixtures over setup/teardown methods, parametrize broadly, avoid unnecessary mocks
Group related tests using class with method names prefixed by test_
Write tests from a user's perspective; test public behavior rather than internals
Avoid mocking too much; prefer test doubles only for external services or non-deterministic behaviors
Files:
tests/behaviour/test_execution_runtime.py
**/*test*.py
📄 CodeRabbit inference engine (.rules/python-exception-design-raising-handling-and-logging.md)
Use specific exception types and message constraints with
pytest.raises(SpecificError, match=r"pattern")in tests; avoid overly broad exception assertions (B017)
Files:
tests/behaviour/test_execution_runtime.py
🧬 Code graph analysis (1)
tests/behaviour/test_execution_runtime.py (3)
cuprum/sh.py (6)
ExecutionContext(104-120)CommandResult(70-100)SafeCmd(124-229)builder(244-246)make(232-248)run(136-229)tests/helpers/catalogue.py (1)
python_catalogue(17-26)cuprum/unittests/test_safe_cmd_run.py (1)
orchestrate(141-159)
🔍 Remote MCP Ref
Based on the information I've gathered, here's the additional relevant context for reviewing this PR:
Additional Context for PR Review
1. Async Subprocess Best Practices
The PR's implementation aligns with Python's asyncio subprocess patterns:
-
The standard approach uses
asyncio.create_subprocess_exec()to create subprocesses with PIPE for stdout/stderr, andcommunicate()to interact with processes. The PR's use ofcreate_subprocess_execis appropriate. -
The Process class provides
terminate()(sends SIGTERM on POSIX) andkill()(sends SIGKILL on POSIX) methods for process control. The PR's two-phase termination (SIGTERM → wait grace period → SIGKILL) follows a well-established shutdown pattern. -
A critical consideration: using
process.stdin.write(),await process.stdout.read()orawait process.stderr.read()directly can cause deadlocks; the documented recommendation is to usecommunicate()or manage concurrent stream reading. The PR implements_consume_stream()for concurrent I/O consumption, which addresses this deadlock risk.
2. Process Termination Semantics
The PR's cancel_grace default of 0.5 seconds is a reasonable grace period for subprocess termination:
- Allows cooperative processes to clean up on SIGTERM
- Falls back to SIGKILL for non-cooperative subprocesses
- Short enough to prevent excessive hanging
3. Key Implementation Considerations for Review
Based on the PR context, pay attention to:
-
ExecutionContext Refactoring: Verify that the parameter consolidation from 5 parameters to
context: ExecutionContext | Noneproperly preserves all behavioral variants (env overlays, cwd, cancel_grace). -
Concurrent Stream Consumption: The
_consume_stream()helper and parallel stdout/stderr reading are critical to avoid subprocess I/O deadlocks—verify both streams are read concurrently, not sequentially. -
UTF-8 Decoding with Replacement: The PR notes UTF-8 decoding with replacement to handle potentially malformed output gracefully.
-
Cancellation Cleanup: Verify that when the run() coroutine is cancelled, it:
- Calls
_terminate_process()with the grace period - Properly awaits pending I/O tasks before re-raising
CancelledError - Cleans up background resources
- Calls
-
Environment Overlay Isolation: Ensure env overrides via
_merge_env()do not mutateos.environglobally—the PR explicitly states this. -
Test Duplication: The flagged polling logic duplication in
test_execution_runtime.pyshould be verified as either refactored or justified if left as-is.
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (18)
cuprum/sh.py (10)
1-6: LGTM!Module docstring accurately reflects the expanded scope to include async runtime execution.
30-34: LGTM!Type aliases and constants follow project conventions. The 4096-byte read size and 0.5s grace period are sensible defaults for subprocess I/O and cancellation semantics.
69-101: LGTM!
CommandResultis well-designed: frozen for immutability, slots for memory efficiency, and the NumPy-style docstring documents all attributes clearly.
103-121: LGTM!
ExecutionContextprovides a clean grouping of execution parameters with sensible defaults. The frozen design ensures callers cannot inadvertently mutate context after creation.
136-229: LGTM!The async execution implementation is solid:
- Concurrent stdout/stderr consumption via
asyncio.create_taskprevents I/O deadlocks.- Cancellation properly terminates the subprocess, awaits pending stream tasks, then re-raises
CancelledError.- The fast path (no capture, no echo) avoids unnecessary task creation.
- PID fallback uses explicit
Nonecheck as per prior feedback.
251-257: LGTM!
_merge_envcorrectly copiesos.environbefore merging, ensuring the global environment is never mutated. ReturningNonewhen no overlay is provided allows the subprocess to inherit the parent environment naturally.
260-281: LGTM!Stream consumption is efficient: buffer allocation is conditional on
capture_output, and UTF-8 decoding witherrors="replace"handles potentially malformed subprocess output gracefully.
284-297: LGTM!The docstring accurately describes the synchronous, potentially blocking nature of the write. Using
sink.bufferwhen available avoids redundant encoding overhead.
300-313: LGTM!The termination escalation logic is correct: SIGTERM first, wait with timeout, then SIGKILL if needed. The
grace_periodclamping to>= 0is a sensible defensive measure. The# noqa: UP041comment is justified—usingasyncio.TimeoutErrorexplicitly in async context aids clarity.
316-323: LGTM!All public types and the factory function are correctly exported.
tests/behaviour/test_execution_runtime.py (8)
1-22: LGTM!Imports are well-organised with appropriate
TYPE_CHECKINGguards for annotation-only types.
24-38: LGTM!Scenario definitions are clear and follow pytest-bdd conventions.
40-67: LGTM!The simple echo command scenario correctly exercises the basic async execution path and validates captured output.
70-101: LGTM!The cooperative worker script correctly publishes its PID and exits cleanly on termination signals. Using
python_catalogue()from the shared helper module addresses prior feedback about deduplication.
104-130: LGTM!The cancellation orchestration correctly uses
_wait_for_pidand properly expectsCancelledErrorafter task cancellation.
145-163: LGTM!Both helpers are concise and well-documented.
_wait_for_pidcorrectly raisesTimeoutErrorwith a descriptive message on failure.
166-172: LGTM!Scenario definition is clear.
255-261: LGTM!The module-level skip marker correctly guards against running POSIX-specific signal tests on Windows.
…r clarity - Extracted polling for process death into reusable _wait_for_process_death helper - Replaced repeated polling code with helper to reduce duplication - Updated coroutine to use _wait_for_pid helper for waiting on PID file - Improved cancellation test setup for environment variables - Overall test code cleanup and improved readability Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
leynos
changed the title
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
tests/behaviour/test_execution_runtime.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: For Python files, ensure testing passes all relevant unit and behavioral tests by runningmake test.
For Python files, ensure linting passes by runningmake lint.
For Python files, ensure formatting adheres to standards by runningmake check-fmtand applyingmake fmtif needed.
For Python files, ensure type checking passes by runningmake typecheck.
For Python development, refer to Python-specific guidelines in the.rules/directory, including Python Code Style Guidelines, Context Managers, Exceptions and Logging, Generators, Project Configuration, Return Patterns, and Typing.
**/*.py: Use snake_case for Python filenames (e.g., http_client.py, task_queue.py)
Use PascalCase for class names
Use snake_case for variable and function names
Use UPPER_SNAKE_CASE for module-level constants
Prefix private/internal functions and variables with a single underscore (_)
Enable full static type coverage using Pyright and maintain typing throughout the codebase
Use TypedDict or Dataclass for structured data, preferring @DataClass(slots=True) for internal-only usage
Avoid using Any type; use Unknown, generics, or cast() with documentation instead
Provide explicit return type annotations (e.g., -> None, -> str) for all public functions and class methods
Enforce strict mode in Pyright and treat all Pyright warnings as CI errors; use # pyright: ignore sparingly with explanation
Avoid side effects at import time; modules should not modify global state or perform actions on import
Use .env or settings modules for environment-specific configuration; never hardcode secrets
Use Ruff for formatting; let Ruff handle whitespace and formatting entirely
Use NumPy-format docstrings for public functions, classes, and modules
Use inline comments to explain tricky or non-obvious code logic and decisions
**/*.py: Use context managers (withcontextlib.contextmanageror class-based__enter__/__exit__) to encapsulate setup and teardown logic for resource management (f...
Files:
tests/behaviour/test_execution_runtime.py
⚙️ CodeRabbit configuration file
**/*.py: - Keep C90 / mccabe complexity ≤ 9
- Follow single responsibility and CQRS (command/query segregation)
- Prefer structural pattern matching to
- Prefer structural pattern matching over
isinstance()or imperative decomposition.- Docstrings must follow the
numpystyle guide. Use a single-line summary for private functions and methods, and full structured docs for all public interfaces.- Move conditionals with >2 branches to predicate/helper functions
- Avoid
eval,exec,pickle, monkey-patching,ctypes, unsafe shell- Every module must begin with a triple-quoted docstring explaining its purpose, utility, and usage, including example calls if appropriate.
- Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Lint suppressions:
- Blanket
# noqa, file-level skips, and categories are forbidden- Only narrow in-line disables (
# noqa: XYZ) are permitted, and must be accompanied byFIXME:or a ticket link, and used only as a last resort.- Use
pytestfixtures for shared setup (conftest.pyorfixtures/)- Replace duplicate tests with
@pytest.mark.parametrize- Prefer
pytest-mockorunittest.mockfor stubs/mocks- Use
assert …, "message"over bare asserts- Reflect all API/behaviour changes in
docs/and update roadmap on completion- Files must not exceed 400 logical lines:
- Decompose large modules into subpackages
- Split large
match/caseor dispatch tables by domain and collocate with targets if appropriate- Move bulky data (fixtures, templates) to external files for parsing at runtime
- Mutable defaults and shadowed built-ins are forbidden
- All code must have clear type hints using modern style (
A | B,list[str],class Foo[A]:,type Bar = int, etc.), with ABC imports drawn from the correct stdlib module.- All path manipulation must be performed using pathlib for cross platform safety. Do not use string manipulation for extraction of path components or filename ele...
Files:
tests/behaviour/test_execution_runtime.py
**/test_*.py
📄 CodeRabbit inference engine (.rules/python-00.md)
**/test_*.py: Use pytest idioms: prefer fixtures over setup/teardown methods, parametrize broadly, avoid unnecessary mocks
Group related tests using class with method names prefixed by test_
Write tests from a user's perspective; test public behavior rather than internals
Avoid mocking too much; prefer test doubles only for external services or non-deterministic behaviors
Files:
tests/behaviour/test_execution_runtime.py
**/*test*.py
📄 CodeRabbit inference engine (.rules/python-exception-design-raising-handling-and-logging.md)
Use specific exception types and message constraints with
pytest.raises(SpecificError, match=r"pattern")in tests; avoid overly broad exception assertions (B017)
Files:
tests/behaviour/test_execution_runtime.py
🧬 Code graph analysis (1)
tests/behaviour/test_execution_runtime.py (2)
cuprum/sh.py (6)
ExecutionContext(104-120)CommandResult(70-100)SafeCmd(124-229)builder(244-246)make(232-248)run(136-229)tests/helpers/catalogue.py (1)
python_catalogue(17-26)
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (5)
tests/behaviour/test_execution_runtime.py (5)
1-22: LGTM!Imports are well-organised, with appropriate use of
TYPE_CHECKINGguard for type-only imports and correct usage of the sharedpython_cataloguehelper.
24-43: LGTM!Scenario definitions and the shared state fixture follow standard pytest-bdd patterns.
46-67: LGTM!The simple echo command steps correctly exercise the default capture semantics with appropriate assertions.
104-136: LGTM!The cancellation steps correctly use the extracted
_wait_for_pidand_wait_for_process_deathhelpers, addressing prior review feedback.
139-174: LGTM!The helper functions are well-structured with clear docstrings. The polling patterns and timeout handling are appropriate for test utilities.
…llation Refactor execution runtime cancellation tests by introducing helper functions _create_worker_command and _cancel_command_with_grace. These encapsulate the creation of worker scripts with customizable signal handlers and the orchestration of command cancellation, reducing code duplication and improving clarity. Also add pytest mark to skip tests on Windows due to reliance on POSIX signals for cancellation escalation. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: tests/behaviour/test_execution_runtime.py Comment on lines +82 to +93 def given_long_running_command(tmp_path: Path) -> dict[str, object]:
"""Construct a SafeCmd that blocks until cancelled."""
return _create_worker_command(
tmp_path,
script_name="sleepy_worker.py",
signal_handler_body=(
"def _stop(_signum, _frame):",
" sys.exit(0)",
"signal.signal(signal.SIGTERM, _stop)",
"signal.signal(signal.SIGINT, _stop)",
),
)❌ New issue: Code Duplication |
This comment was marked as resolved.
This comment was marked as resolved.
…ve flag in test worker scripts The test_execution_runtime.py suite refactors worker command creation by replacing the explicit signal_handler_body tuples with a cooperative boolean flag. This flag controls whether the generated worker installs signal handlers that either exit cleanly on termination signals or ignore them, improving readability and flexibility in test setup. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
tests/behaviour/test_execution_runtime.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: For Python files, ensure testing passes all relevant unit and behavioral tests by runningmake test.
For Python files, ensure linting passes by runningmake lint.
For Python files, ensure formatting adheres to standards by runningmake check-fmtand applyingmake fmtif needed.
For Python files, ensure type checking passes by runningmake typecheck.
For Python development, refer to Python-specific guidelines in the.rules/directory, including Python Code Style Guidelines, Context Managers, Exceptions and Logging, Generators, Project Configuration, Return Patterns, and Typing.
**/*.py: Use snake_case for Python filenames (e.g., http_client.py, task_queue.py)
Use PascalCase for class names
Use snake_case for variable and function names
Use UPPER_SNAKE_CASE for module-level constants
Prefix private/internal functions and variables with a single underscore (_)
Enable full static type coverage using Pyright and maintain typing throughout the codebase
Use TypedDict or Dataclass for structured data, preferring @DataClass(slots=True) for internal-only usage
Avoid using Any type; use Unknown, generics, or cast() with documentation instead
Provide explicit return type annotations (e.g., -> None, -> str) for all public functions and class methods
Enforce strict mode in Pyright and treat all Pyright warnings as CI errors; use # pyright: ignore sparingly with explanation
Avoid side effects at import time; modules should not modify global state or perform actions on import
Use .env or settings modules for environment-specific configuration; never hardcode secrets
Use Ruff for formatting; let Ruff handle whitespace and formatting entirely
Use NumPy-format docstrings for public functions, classes, and modules
Use inline comments to explain tricky or non-obvious code logic and decisions
**/*.py: Use context managers (withcontextlib.contextmanageror class-based__enter__/__exit__) to encapsulate setup and teardown logic for resource management (f...
Files:
tests/behaviour/test_execution_runtime.py
⚙️ CodeRabbit configuration file
**/*.py: - Keep C90 / mccabe complexity ≤ 9
- Follow single responsibility and CQRS (command/query segregation)
- Prefer structural pattern matching to
- Prefer structural pattern matching over
isinstance()or imperative decomposition.- Docstrings must follow the
numpystyle guide. Use a single-line summary for private functions and methods, and full structured docs for all public interfaces.- Move conditionals with >2 branches to predicate/helper functions
- Avoid
eval,exec,pickle, monkey-patching,ctypes, unsafe shell- Every module must begin with a triple-quoted docstring explaining its purpose, utility, and usage, including example calls if appropriate.
- Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Lint suppressions:
- Blanket
# noqa, file-level skips, and categories are forbidden- Only narrow in-line disables (
# noqa: XYZ) are permitted, and must be accompanied byFIXME:or a ticket link, and used only as a last resort.- Use
pytestfixtures for shared setup (conftest.pyorfixtures/)- Replace duplicate tests with
@pytest.mark.parametrize- Prefer
pytest-mockorunittest.mockfor stubs/mocks- Use
assert …, "message"over bare asserts- Reflect all API/behaviour changes in
docs/and update roadmap on completion- Files must not exceed 400 logical lines:
- Decompose large modules into subpackages
- Split large
match/caseor dispatch tables by domain and collocate with targets if appropriate- Move bulky data (fixtures, templates) to external files for parsing at runtime
- Mutable defaults and shadowed built-ins are forbidden
- All code must have clear type hints using modern style (
A | B,list[str],class Foo[A]:,type Bar = int, etc.), with ABC imports drawn from the correct stdlib module.- All path manipulation must be performed using pathlib for cross platform safety. Do not use string manipulation for extraction of path components or filename ele...
Files:
tests/behaviour/test_execution_runtime.py
**/test_*.py
📄 CodeRabbit inference engine (.rules/python-00.md)
**/test_*.py: Use pytest idioms: prefer fixtures over setup/teardown methods, parametrize broadly, avoid unnecessary mocks
Group related tests using class with method names prefixed by test_
Write tests from a user's perspective; test public behavior rather than internals
Avoid mocking too much; prefer test doubles only for external services or non-deterministic behaviors
Files:
tests/behaviour/test_execution_runtime.py
**/*test*.py
📄 CodeRabbit inference engine (.rules/python-exception-design-raising-handling-and-logging.md)
Use specific exception types and message constraints with
pytest.raises(SpecificError, match=r"pattern")in tests; avoid overly broad exception assertions (B017)
Files:
tests/behaviour/test_execution_runtime.py
🧬 Code graph analysis (1)
tests/behaviour/test_execution_runtime.py (2)
cuprum/sh.py (6)
ExecutionContext(104-120)CommandResult(70-100)SafeCmd(124-229)builder(244-246)make(232-248)run(136-229)tests/helpers/catalogue.py (1)
python_catalogue(17-26)
🔍 Remote MCP Ref
Based on the documentation I've gathered, here's the relevant context for effectively reviewing this PR:
Additional Context for PR Review
Async Subprocess Patterns
The PR implements SafeCmd.run() using asyncio.create_subprocess_exec() with concurrent stdout/stderr consumption. This approach aligns with Python's asyncio subprocess best practices:
- The asyncio subprocess module provides high-level async/await APIs to create and manage subprocesses, with
create_subprocess_exec()returning aProcessinstance - The documentation explicitly warns against deadlocks when using
stdout=PIPEorstderr=PIPE, recommending thecommunicate()method or concurrent reading to avoid this condition
The PR's implementation of _consume_stream() helper functions addresses this by concurrently reading stdout and stderr, which is the correct pattern to avoid subprocess deadlocks.
Signal Handling for Process Termination
The PR implements a cancellation flow: terminate → wait grace period → kill if needed. This aligns with standard subprocess termination patterns:
- On POSIX systems,
terminate()sendsSIGTERMto the child process, whilekill()sendsSIGKILL - The grace period strategy allows cooperative processes to shutdown gracefully before forceful termination, which is a best practice for resource cleanup
Code Review Concerns
The two flagged issues in the PR comments merit attention:
-
Function Parameter Complexity: Codescene flagged 5 parameters (excluding self) exceeding a threshold of 4. CodeRabbit's recommendation to consolidate
env,cwd, andcancel_graceinto anExecutionContextparameter is sound for maintainability and follows the principle of grouping related configuration concerns. -
Test Code Duplication: The behavioral tests contain repeated polling and subprocess creation logic. The suggested
_wait_for_process_death()helper and refactoring_create_worker_command()to accept acooperativeflag would improve test maintainability by reducing duplication.
Key Points for Review
- The
CommandResultdataclass structure (program, argv, exit_code, pid, stdout, stderr, ok) provides comprehensive result information - UTF-8 decoding with replacement is specified, which prevents crashes on invalid Unicode
- The implementation exports both
CommandResultandExecutionContextto the public API, expanding the module's usability - Test coverage includes capture/echo behavior, environment overrides, working directory changes, and graceful vs. forceful termination scenarios
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (6)
tests/behaviour/test_execution_runtime.py (6)
1-29: Well-structured module with correct platform guard placement.The imports are properly organised, the
TYPE_CHECKINGguard prevents unnecessary runtime imports, and thepytestmarkis correctly placed at module level to ensure Windows skip applies during collection.
32-46: Scenario definitions are clean and follow pytest-bdd conventions.
48-75: Basic capture scenario steps are well-implemented.The
asyncio.run()usage is appropriate for executing async code within synchronous BDD steps, and the assertions validate the expected capture behaviour.
78-116: Cancellation steps properly delegate to shared helpers.The
_is_process_aliveimplementation correctly uses theos.kill(pid, 0)probe pattern. The step functions are now lean, delegating to extracted helpers as suggested in prior reviews.
170-229: Helper functions are well-designed and address prior feedback.The
_wait_for_process_deathhelper centralises polling logic,_wait_for_pidproperly raisesTimeoutErrorwith a message variable (EM101 compliant), and_cancel_command_with_graceincludes the brief delay before cancellation as requested in prior reviews.Using
ExecutionContext().cancel_graceto obtain the default grace period is a pragmatic approach that avoids importing private module constants.
232-275: Escalation scenario provides critical test coverage.This scenario addresses the prior feedback from sourcery-ai regarding coverage for non-cooperative subprocess escalation. The
cancel_grace=0.1ensures the SIGTERM → wait → SIGKILL escalation path is exercised quickly, and_wait_for_process_deathconfirms the subprocess is eventually killed.The test now verifies both:
- Cooperative cancellation (subprocess exits cleanly on SIGTERM)
- Non-cooperative escalation (subprocess ignores SIGTERM and is forcefully killed)
Refactor the python_catalogue helper function to return a tuple containing both the ProgramCatalogue and the Python Program instance. Adjust usages in test_execution_runtime.py to unpack and use both values accordingly. This change improves clarity and access to the Python program instance alongside its catalogue in tests. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
leynos
changed the title
Skip test_non_cooperative_subprocess_is_escalated_and_killed on Windows platform because cancellation escalation semantics rely on POSIX signals and are not applicable on Windows. Also improve test_execution_runtime.py assertion message for subprocess kill timing. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
leynos
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
cuprum/unittests/test_safe_cmd_run.py(1 hunks)tests/behaviour/test_execution_runtime.py(1 hunks)tests/helpers/catalogue.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: For Python files, ensure testing passes all relevant unit and behavioral tests by runningmake test.
For Python files, ensure linting passes by runningmake lint.
For Python files, ensure formatting adheres to standards by runningmake check-fmtand applyingmake fmtif needed.
For Python files, ensure type checking passes by runningmake typecheck.
For Python development, refer to Python-specific guidelines in the.rules/directory, including Python Code Style Guidelines, Context Managers, Exceptions and Logging, Generators, Project Configuration, Return Patterns, and Typing.
**/*.py: Use snake_case for Python filenames (e.g., http_client.py, task_queue.py)
Use PascalCase for class names
Use snake_case for variable and function names
Use UPPER_SNAKE_CASE for module-level constants
Prefix private/internal functions and variables with a single underscore (_)
Enable full static type coverage using Pyright and maintain typing throughout the codebase
Use TypedDict or Dataclass for structured data, preferring @DataClass(slots=True) for internal-only usage
Avoid using Any type; use Unknown, generics, or cast() with documentation instead
Provide explicit return type annotations (e.g., -> None, -> str) for all public functions and class methods
Enforce strict mode in Pyright and treat all Pyright warnings as CI errors; use # pyright: ignore sparingly with explanation
Avoid side effects at import time; modules should not modify global state or perform actions on import
Use .env or settings modules for environment-specific configuration; never hardcode secrets
Use Ruff for formatting; let Ruff handle whitespace and formatting entirely
Use NumPy-format docstrings for public functions, classes, and modules
Use inline comments to explain tricky or non-obvious code logic and decisions
**/*.py: Use context managers (withcontextlib.contextmanageror class-based__enter__/__exit__) to encapsulate setup and teardown logic for resource management (f...
Files:
tests/behaviour/test_execution_runtime.pycuprum/unittests/test_safe_cmd_run.pytests/helpers/catalogue.py
⚙️ CodeRabbit configuration file
**/*.py: - Keep C90 / mccabe complexity ≤ 9
- Follow single responsibility and CQRS (command/query segregation)
- Prefer structural pattern matching to
- Prefer structural pattern matching over
isinstance()or imperative decomposition.- Docstrings must follow the
numpystyle guide. Use a single-line summary for private functions and methods, and full structured docs for all public interfaces.- Move conditionals with >2 branches to predicate/helper functions
- Avoid
eval,exec,pickle, monkey-patching,ctypes, unsafe shell- Every module must begin with a triple-quoted docstring explaining its purpose, utility, and usage, including example calls if appropriate.
- Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Lint suppressions:
- Blanket
# noqa, file-level skips, and categories are forbidden- Only narrow in-line disables (
# noqa: XYZ) are permitted, and must be accompanied byFIXME:or a ticket link, and used only as a last resort.- Use
pytestfixtures for shared setup (conftest.pyorfixtures/)- Replace duplicate tests with
@pytest.mark.parametrize- Prefer
pytest-mockorunittest.mockfor stubs/mocks- Use
assert …, "message"over bare asserts- Reflect all API/behaviour changes in
docs/and update roadmap on completion- Files must not exceed 400 logical lines:
- Decompose large modules into subpackages
- Split large
match/caseor dispatch tables by domain and collocate with targets if appropriate- Move bulky data (fixtures, templates) to external files for parsing at runtime
- Mutable defaults and shadowed built-ins are forbidden
- All code must have clear type hints using modern style (
A | B,list[str],class Foo[A]:,type Bar = int, etc.), with ABC imports drawn from the correct stdlib module.- All path manipulation must be performed using pathlib for cross platform safety. Do not use string manipulation for extraction of path components or filename ele...
Files:
tests/behaviour/test_execution_runtime.pycuprum/unittests/test_safe_cmd_run.pytests/helpers/catalogue.py
**/test_*.py
📄 CodeRabbit inference engine (.rules/python-00.md)
**/test_*.py: Use pytest idioms: prefer fixtures over setup/teardown methods, parametrize broadly, avoid unnecessary mocks
Group related tests using class with method names prefixed by test_
Write tests from a user's perspective; test public behavior rather than internals
Avoid mocking too much; prefer test doubles only for external services or non-deterministic behaviors
Files:
tests/behaviour/test_execution_runtime.pycuprum/unittests/test_safe_cmd_run.py
**/*test*.py
📄 CodeRabbit inference engine (.rules/python-exception-design-raising-handling-and-logging.md)
Use specific exception types and message constraints with
pytest.raises(SpecificError, match=r"pattern")in tests; avoid overly broad exception assertions (B017)
Files:
tests/behaviour/test_execution_runtime.pycuprum/unittests/test_safe_cmd_run.py
**/unittests/test_*.py
📄 CodeRabbit inference engine (.rules/python-00.md)
Colocate unit tests with code using an unittests subdirectory with test_ prefix (e.g., user_auth/unittests/test_models.py)
Files:
cuprum/unittests/test_safe_cmd_run.py
🧬 Code graph analysis (1)
tests/helpers/catalogue.py (2)
cuprum/catalogue.py (2)
ProgramCatalogue(56-119)ProjectSettings(30-40)cuprum/sh.py (2)
SafeCmd(124-229)make(232-248)
🔍 Remote MCP Ref
Summary of additional relevant facts for review
-
Core implementation (cuprum/sh.py):
- Introduces CommandResult (frozen, slots) with fields: program, argv, exit_code, pid, stdout, stderr and an ok property.
- Introduces ExecutionContext (frozen, slots) with fields: env, cwd, cancel_grace and default cancel grace _DEFAULT_CANCEL_GRACE = 0.5. SafeCmd.run accepts an optional context param and uses ctx = context or ExecutionContext().
- SafeCmd.run uses asyncio.create_subprocess_exec, chooses PIPE vs DEVNULL based on capture/echo, concurrently consumes stdout/stderr via _consume_stream, and implements cancellation as: on CancelledError call _terminate_process(process, ctx.cancel_grace), wait for I/O tasks, then re-raise. _terminate_process sends terminate(), waits grace, then kill() on timeout. Streams are UTF‑8 decoded with replacement. Helpers: _merge_env, _consume_stream, _write_chunk, _terminate_process; constants: _READ_SIZE=4096, _DEFAULT_CANCEL_GRACE=0.5.
-
Public API surface changes:
- make(...) signature now accepts catalogue: ProgramCatalogue = DEFAULT_CATALOGUE and returns SafeCmdBuilder. all updated to export "CommandResult" and "ExecutionContext" (plus existing exports). cuprum/init.py updated to re-export these names at package top-level (exposed via PR files).
-
Tests and docs added/updated in the PR:
- Unit tests: cuprum/unittests/test_safe_cmd_run.py covering capture/echo, env/cwd overrides, disabling capture, non-zero exit and ok flag, and escalation of non-cooperative subprocesses.
- Behaviour and feature tests: tests/behaviour/test_execution_runtime.py and tests/features/execution_runtime.feature validating cancellation and escalation behaviour (POSIX gated). Test helpers added under tests/helpers (catalogue and init).
- Docs updated: docs/cuprum-design.md, docs/users-guide.md, docs/roadmap.md describing CommandResult shape, ExecutionContext behavior, cancellation semantics, and pipeline notes.
-
Implementation vs earlier review notes:
- The PR already applies the suggested refactor of grouping env/cwd/cancel_grace into ExecutionContext and uses the context parameter in SafeCmd.run (so the Codescene/PLR0913 parameter-count concern has been addressed by this PR).
Files inspected (for reviewer reference)
- cuprum/sh.py (implementation) — raw file from branch.
- PR overview and changed files list (pull/8 and pull/8/files).
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (11)
tests/helpers/catalogue.py (1)
1-32: LGTM!The helper module is well-structured, with appropriate use of
TYPE_CHECKINGguards,pathlibfor path handling, and clear docstrings. The separation betweenpython_catalogue()andpython_builder()provides flexibility for tests that need catalogue access versus those that only need a builder.tests/behaviour/test_execution_runtime.py (6)
1-30: LGTM!The module-level
pytestmarkis correctly positioned after imports, ensuring the Windows skip is applied during collection. The imports are well-organised withTYPE_CHECKINGguards for type-only imports.
32-51: LGTM!Scenario definitions and the
behaviour_statefixture follow pytest-bdd conventions correctly.
54-75: LGTM!The echo command steps are clear and correctly validate the expected capture behaviour.
78-107: LGTM!The long-running command steps properly delegate to the extracted
_create_worker_commandand_cancel_command_with_gracehelpers, addressing prior code duplication feedback.
110-228: LGTM!The helper functions are well-extracted and address prior review feedback. The
_create_worker_commandhelper cleanly handles both cooperative and non-cooperative signal handling via thecooperativeparameter, and_wait_for_process_deathcentralises the polling logic with appropriate defensive guards.
231-278: LGTM!The non-cooperative escalation scenario is well-implemented, reusing the shared helpers and correctly exercising the
cancel_graceescalation path with a short timeout.cuprum/unittests/test_safe_cmd_run.py (4)
1-24: LGTM!Imports are well-organised with
TYPE_CHECKINGguard. Thepython_builderfixture correctly delegates to the shared helper module.
27-48: LGTM!The capture and echo tests are well-structured, with appropriate use of
capfdfor validating echoed output alongside captured result.
51-98: LGTM!The environment and cwd override tests address prior review feedback: the env leak assertion captures the original value, and the cwd assertion uses
Pathcomparison for platform independence.
101-109: LGTM!The test correctly verifies that
capture=Falseresults inNonefor stdout and stderr.
Refactored test_non_cooperative_subprocess_is_escalated_and_killed to use pathlib for pid file operations, added environment variable setup for subprocess, replaced asyncio.sleep with time.sleep for test timing, and cleaned up imports. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
cuprum/unittests/test_safe_cmd_run.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: For Python files, ensure testing passes all relevant unit and behavioral tests by runningmake test.
For Python files, ensure linting passes by runningmake lint.
For Python files, ensure formatting adheres to standards by runningmake check-fmtand applyingmake fmtif needed.
For Python files, ensure type checking passes by runningmake typecheck.
For Python development, refer to Python-specific guidelines in the.rules/directory, including Python Code Style Guidelines, Context Managers, Exceptions and Logging, Generators, Project Configuration, Return Patterns, and Typing.
**/*.py: Use snake_case for Python filenames (e.g., http_client.py, task_queue.py)
Use PascalCase for class names
Use snake_case for variable and function names
Use UPPER_SNAKE_CASE for module-level constants
Prefix private/internal functions and variables with a single underscore (_)
Enable full static type coverage using Pyright and maintain typing throughout the codebase
Use TypedDict or Dataclass for structured data, preferring @DataClass(slots=True) for internal-only usage
Avoid using Any type; use Unknown, generics, or cast() with documentation instead
Provide explicit return type annotations (e.g., -> None, -> str) for all public functions and class methods
Enforce strict mode in Pyright and treat all Pyright warnings as CI errors; use # pyright: ignore sparingly with explanation
Avoid side effects at import time; modules should not modify global state or perform actions on import
Use .env or settings modules for environment-specific configuration; never hardcode secrets
Use Ruff for formatting; let Ruff handle whitespace and formatting entirely
Use NumPy-format docstrings for public functions, classes, and modules
Use inline comments to explain tricky or non-obvious code logic and decisions
**/*.py: Use context managers (withcontextlib.contextmanageror class-based__enter__/__exit__) to encapsulate setup and teardown logic for resource management (f...
Files:
cuprum/unittests/test_safe_cmd_run.py
⚙️ CodeRabbit configuration file
**/*.py: - Keep C90 / mccabe complexity ≤ 9
- Follow single responsibility and CQRS (command/query segregation)
- Prefer structural pattern matching to
- Prefer structural pattern matching over
isinstance()or imperative decomposition.- Docstrings must follow the
numpystyle guide. Use a single-line summary for private functions and methods, and full structured docs for all public interfaces.- Move conditionals with >2 branches to predicate/helper functions
- Avoid
eval,exec,pickle, monkey-patching,ctypes, unsafe shell- Every module must begin with a triple-quoted docstring explaining its purpose, utility, and usage, including example calls if appropriate.
- Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Lint suppressions:
- Blanket
# noqa, file-level skips, and categories are forbidden- Only narrow in-line disables (
# noqa: XYZ) are permitted, and must be accompanied byFIXME:or a ticket link, and used only as a last resort.- Use
pytestfixtures for shared setup (conftest.pyorfixtures/)- Replace duplicate tests with
@pytest.mark.parametrize- Prefer
pytest-mockorunittest.mockfor stubs/mocks- Use
assert …, "message"over bare asserts- Reflect all API/behaviour changes in
docs/and update roadmap on completion- Files must not exceed 400 logical lines:
- Decompose large modules into subpackages
- Split large
match/caseor dispatch tables by domain and collocate with targets if appropriate- Move bulky data (fixtures, templates) to external files for parsing at runtime
- Mutable defaults and shadowed built-ins are forbidden
- All code must have clear type hints using modern style (
A | B,list[str],class Foo[A]:,type Bar = int, etc.), with ABC imports drawn from the correct stdlib module.- All path manipulation must be performed using pathlib for cross platform safety. Do not use string manipulation for extraction of path components or filename ele...
Files:
cuprum/unittests/test_safe_cmd_run.py
**/unittests/test_*.py
📄 CodeRabbit inference engine (.rules/python-00.md)
Colocate unit tests with code using an unittests subdirectory with test_ prefix (e.g., user_auth/unittests/test_models.py)
Files:
cuprum/unittests/test_safe_cmd_run.py
**/test_*.py
📄 CodeRabbit inference engine (.rules/python-00.md)
**/test_*.py: Use pytest idioms: prefer fixtures over setup/teardown methods, parametrize broadly, avoid unnecessary mocks
Group related tests using class with method names prefixed by test_
Write tests from a user's perspective; test public behavior rather than internals
Avoid mocking too much; prefer test doubles only for external services or non-deterministic behaviors
Files:
cuprum/unittests/test_safe_cmd_run.py
**/*test*.py
📄 CodeRabbit inference engine (.rules/python-exception-design-raising-handling-and-logging.md)
Use specific exception types and message constraints with
pytest.raises(SpecificError, match=r"pattern")in tests; avoid overly broad exception assertions (B017)
Files:
cuprum/unittests/test_safe_cmd_run.py
🧬 Code graph analysis (1)
cuprum/unittests/test_safe_cmd_run.py (3)
cuprum/sh.py (5)
ExecutionContext(104-120)SafeCmd(124-229)make(232-248)run(136-229)ok(98-100)tests/helpers/catalogue.py (1)
python_builder(29-32)tests/behaviour/test_execution_runtime.py (1)
orchestrate(206-226)
🔍 Remote MCP Ref
Additional relevant facts found
-
Implementation details (cuprum/sh.py):
- CommandResult and ExecutionContext are frozen, slots dataclasses; ExecutionContext defaults cancel_grace = 0.5 and _READ_SIZE = 4096 is used for stream reads.
- SafeCmd.run signature: async run(self, *, capture: bool = True, echo: bool = False, context: ExecutionContext | None = None) -> CommandResult. It chooses PIPE vs DEVNULL based on capture/echo, creates subprocess with asyncio.create_subprocess_exec, and either waits directly (no capture/echo) or concurrently consumes stdout/stderr via tasks. On asyncio.CancelledError it calls _terminate_process(process, ctx.cancel_grace), waits for I/O tasks, then re-raises. Streams are UTF‑8 decoded with replacement. Helpers: _merge_env, _consume_stream, _write_chunk, _terminate_process (terminate → wait grace → kill on timeout).
- make(...) now accepts catalogue: ProgramCatalogue = DEFAULT_CATALOGUE and returns a SafeCmdBuilder.
-
Public surface (cuprum/init.py):
- cuprum.init re-exports CommandResult and ExecutionContext (and SafeCmd, SafeCmdBuilder, etc.) so they are available at package top-level.
-
PR metadata:
- Pull request: "Add async SafeCmd.run with cancellation and expose API types" (PR #8) — branch terragon/implement-safe-cmd-run-9hymm0 → main; diff +782 −7; files changed include cuprum/sh.py, cuprum/init.py, tests and docs.
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (8)
cuprum/unittests/test_safe_cmd_run.py (8)
1-20: LGTM! Clean module structure and imports.The module docstring is clear, imports are properly ordered (future, stdlib, third-party, local), and the
TYPE_CHECKINGguard correctly prevents circular imports while preserving type hints.
22-26: LGTM! Fixture follows pytest conventions.The fixture correctly delegates to the shared helper and provides a typed, reusable builder for Python-based test commands.
28-38: LGTM! Comprehensive capture semantics test.The test verifies all aspects of the default capture behaviour: exit code, the convenience
okproperty, and both stdout and stderr streams.
40-50: LGTM! Echo behaviour correctly tested.The test properly uses
capfdto verify thatecho=Truetees output to the parent process whilst still capturing it in the result.
52-72: LGTM! Environment overlay correctly isolated.The test now properly captures the original environment state and verifies that the overlay merges without mutating
os.environglobally, addressing the earlier review feedback.
74-84: LGTM! Non-zero exit path verified.The test confirms that non-zero exit codes are captured correctly and reflected in the
okproperty, completing the coverage of success and failure paths.
86-100: LGTM! Working directory override correctly tested.The test now uses
pathlib.Pathcomparison to ensure platform independence, addressing the earlier Windows compatibility concern.
102-111: LGTM! Capture disabling verified.The test confirms that
capture=Falseprevents stream retention whilst still capturing the exit code, validating the expected behaviour.
Replace direct os.kill and sleep with a _poll_process_death helper that polls for subprocess exit within a timeout, improving test reliability and clarity. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- SafeCmd.run currently hardcodes sys.stdout/sys.stderr as echo sinks; consider allowing the sink(s) to be injected (e.g. via ExecutionContext or parameters) to make the runtime easier to embed in non-CLI environments and to test alternative output routing.
- Output decoding is fixed to UTF-8 with replacement in _consume_stream; if you expect to support non-UTF-8 subprocesses, you may want to make the encoding and error handling configurable via ExecutionContext or similar.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- SafeCmd.run currently hardcodes sys.stdout/sys.stderr as echo sinks; consider allowing the sink(s) to be injected (e.g. via ExecutionContext or parameters) to make the runtime easier to embed in non-CLI environments and to test alternative output routing.
- Output decoding is fixed to UTF-8 with replacement in _consume_stream; if you expect to support non-UTF-8 subprocesses, you may want to make the encoding and error handling configurable via ExecutionContext or similar.
## Individual Comments
### Comment 1
<location> `cuprum/sh.py:300-309` </location>
<code_context>
+ sink.flush()
+
+
+async def _terminate_process(
+ process: asyncio.subprocess.Process,
+ grace_period: float,
+) -> None:
+ """Terminate a running process, escalating to kill after the grace period."""
+ grace_period = max(0.0, grace_period)
+ if process.returncode is not None:
+ return
+ process.terminate()
+ try:
+ await asyncio.wait_for(process.wait(), grace_period)
+ except asyncio.TimeoutError: # noqa: UP041 - explicit asyncio timeout needed
+ process.kill()
+ await process.wait()
</code_context>
<issue_to_address>
**issue (bug_risk):** Guard against race where the process exits between the returncode check and terminate/kill calls.
The process may still exit between the returncode check and the `terminate()` / `kill()` calls, which can raise `ProcessLookupError` (and possibly `OSError`) on some platforms. Please wrap these calls in a small `try/except ProcessLookupError` (and optionally `OSError`) so that cancellation paths don’t surface spurious errors.
</issue_to_address>
### Comment 2
<location> `cuprum/unittests/test_safe_cmd_run.py:28-37` </location>
<code_context>
+ return build_python_builder()
+
+
+def test_run_captures_output_and_exit_code() -> None:
+ """run() captures stdout/stderr and exit code by default."""
+ command = sh.make(ECHO)("-n", "hello")
+
+ result = asyncio.run(command.run())
+
+ assert result.exit_code == 0
+ assert result.ok is True
+ assert result.stdout == "hello"
+ assert result.stderr == ""
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Add a dedicated test for stderr capture (and optionally echo) to ensure separation of stdout/stderr.
Current tests only cover commands where stderr is empty, so we never verify that stderr is wired and decoded independently from stdout. Please add a test using `python_builder` that writes only to stderr (e.g. `python -c 'import sys; print("err", file=sys.stderr)'`) and assert `result.stdout` is empty while `result.stderr.strip() == "err"`. Optionally add a variant with `echo=True` to confirm stderr is both echoed to `sys.stderr` and captured.
Suggested implementation:
```python
@pytest.fixture
def python_builder() -> typ.Callable[..., SafeCmd]:
"""Provide a SafeCmd builder for the current Python interpreter."""
return build_python_builder()
def test_run_captures_stderr_only(
python_builder: typ.Callable[..., SafeCmd],
) -> None:
"""run() captures stderr independently when only stderr is written."""
command = python_builder(
"-c",
'import sys; print("err", file=sys.stderr)',
)
result = asyncio.run(command.run())
assert result.exit_code == 0
assert result.ok is True
assert result.stdout == ""
assert result.stderr.strip() == "err"
def test_run_captures_and_echoes_stderr(
python_builder: typ.Callable[..., SafeCmd],
capsys: pytest.CaptureFixture[str],
) -> None:
"""run(echo=True) both echoes to stderr and captures stderr separately."""
command = python_builder(
"-c",
'import sys; print("err", file=sys.stderr)',
)
result = asyncio.run(command.run(echo=True))
captured = capsys.readouterr()
assert result.exit_code == 0
assert result.ok is True
assert result.stdout == ""
assert result.stderr.strip() == "err"
# Ensure the stderr output was echoed to the real stderr stream.
assert captured.out == ""
assert captured.err.strip() == "err"
from __future__ import annotations
```
If the type alias `SafeCmd` or the helper `build_python_builder` are defined in a different module than assumed, make sure:
1. `SafeCmd` is correctly imported under the `typ.TYPE_CHECKING` guard.
2. `build_python_builder` is imported at module level so that `python_builder` uses the correct builder for the current Python interpreter.
Also ensure that `pytest` and `asyncio` are available in this module (they appear in the provided snippet, but if not, add the appropriate imports).
</issue_to_address>
### Comment 3
<location> `docs/users-guide.md:113` </location>
<code_context>
+ - `env` overlays key/value pairs on top of the current environment without
+ mutating `os.environ`; use it to pass per-command settings.
+ - `cwd` sets the working directory for the subprocess when provided.
+ - `cancel_grace` controls how long Cuprum waits after `SIGTERM` before
+ escalating to `SIGKILL`.
+- `exit_code`, `pid`, and `ok` on the `CommandResult` make it easy to branch on
</code_context>
<issue_to_address>
**issue (review_instructions):** `SIGTERM` and `SIGKILL` are introduced without being defined, which violates the requirement to define uncommon acronyms on first use.
Consider expanding the first occurrence, for example: `SIGTERM (termination signal)` and `SIGKILL (kill signal)`, so that the acronyms are defined when first mentioned.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Define uncommon acronyms on first use.
</details>
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| async def _terminate_process( | ||
| process: asyncio.subprocess.Process, | ||
| grace_period: float, | ||
| ) -> None: | ||
| """Terminate a running process, escalating to kill after the grace period.""" | ||
| grace_period = max(0.0, grace_period) | ||
| if process.returncode is not None: | ||
| return | ||
| process.terminate() | ||
| try: |
There was a problem hiding this comment.
issue (bug_risk): Guard against race where the process exits between the returncode check and terminate/kill calls.
The process may still exit between the returncode check and the terminate() / kill() calls, which can raise ProcessLookupError (and possibly OSError) on some platforms. Please wrap these calls in a small try/except ProcessLookupError (and optionally OSError) so that cancellation paths don’t surface spurious errors.
| def test_run_captures_output_and_exit_code() -> None: | ||
| """run() captures stdout/stderr and exit code by default.""" | ||
| command = sh.make(ECHO)("-n", "hello") | ||
|
|
||
| result = asyncio.run(command.run()) | ||
|
|
||
| assert result.exit_code == 0 | ||
| assert result.ok is True | ||
| assert result.stdout == "hello" | ||
| assert result.stderr == "" |
There was a problem hiding this comment.
suggestion (testing): Add a dedicated test for stderr capture (and optionally echo) to ensure separation of stdout/stderr.
Current tests only cover commands where stderr is empty, so we never verify that stderr is wired and decoded independently from stdout. Please add a test using python_builder that writes only to stderr (e.g. python -c 'import sys; print("err", file=sys.stderr)') and assert result.stdout is empty while result.stderr.strip() == "err". Optionally add a variant with echo=True to confirm stderr is both echoed to sys.stderr and captured.
Suggested implementation:
@pytest.fixture
def python_builder() -> typ.Callable[..., SafeCmd]:
"""Provide a SafeCmd builder for the current Python interpreter."""
return build_python_builder()
def test_run_captures_stderr_only(
python_builder: typ.Callable[..., SafeCmd],
) -> None:
"""run() captures stderr independently when only stderr is written."""
command = python_builder(
"-c",
'import sys; print("err", file=sys.stderr)',
)
result = asyncio.run(command.run())
assert result.exit_code == 0
assert result.ok is True
assert result.stdout == ""
assert result.stderr.strip() == "err"
def test_run_captures_and_echoes_stderr(
python_builder: typ.Callable[..., SafeCmd],
capsys: pytest.CaptureFixture[str],
) -> None:
"""run(echo=True) both echoes to stderr and captures stderr separately."""
command = python_builder(
"-c",
'import sys; print("err", file=sys.stderr)',
)
result = asyncio.run(command.run(echo=True))
captured = capsys.readouterr()
assert result.exit_code == 0
assert result.ok is True
assert result.stdout == ""
assert result.stderr.strip() == "err"
# Ensure the stderr output was echoed to the real stderr stream.
assert captured.out == ""
assert captured.err.strip() == "err"
from __future__ import annotationsIf the type alias SafeCmd or the helper build_python_builder are defined in a different module than assumed, make sure:
SafeCmdis correctly imported under thetyp.TYPE_CHECKINGguard.build_python_builderis imported at module level so thatpython_builderuses the correct builder for the current Python interpreter.
Also ensure that pytest and asyncio are available in this module (they appear in the provided snippet, but if not, add the appropriate imports).
| - `env` overlays key/value pairs on top of the current environment without | ||
| mutating `os.environ`; use it to pass per-command settings. | ||
| - `cwd` sets the working directory for the subprocess when provided. | ||
| - `cancel_grace` controls how long Cuprum waits after `SIGTERM` before |
There was a problem hiding this comment.
issue (review_instructions): SIGTERM and SIGKILL are introduced without being defined, which violates the requirement to define uncommon acronyms on first use.
Consider expanding the first occurrence, for example: SIGTERM (termination signal) and SIGKILL (kill signal), so that the acronyms are defined when first mentioned.
Review instructions:
Path patterns: **/*.md
Instructions:
Define uncommon acronyms on first use.
1 participant
Summary
Changes
Core API
Runtime & I/O
Internal helpers & constants
Tests
Documentation
Testing plan
Migration considerations
📎 Task: https://www.terragonlabs.com/task/c90490d8-8a93-48f5-8cd0-62484b3411cc
Summary by Sourcery
Introduce an asynchronous SafeCmd.run execution runtime with structured results, configurable execution context, and robust cancellation semantics, and document and test this behaviour.
New Features:
Enhancements:
Documentation:
Tests: