Skip to content

Enhance SafeCmd.run: error handling, sinks, and decoding - #9

Merged
leynos merged 1 commit into
mainfrom
terragon/improve-safecmd-run-error-handling-jr0std
Dec 8, 2025
Merged

Enhance SafeCmd.run: error handling, sinks, and decoding#9
leynos merged 1 commit into
mainfrom
terragon/improve-safecmd-run-error-handling-jr0std

Conversation

@leynos

@leynos leynos commented Dec 7, 2025

Copy link
Copy Markdown
Owner

Summary

  • Improves error handling and cancellation flow in SafeCmd.run to be more robust during termination and kill phases.
  • Adds configurable sinks and decoding options for echoed streams, enabling custom output targets and encoding behavior.
  • Refines stream consumption to support capturing and echoing with per-stream configuration.
  • Expands test coverage for stderr handling, custom sinks, and encoding configurations.
  • Updates user guide to document new options and behaviors.

Changes

Core

  • Introduced default encoding and error handling: _DEFAULT_ENCODING = "utf-8", _DEFAULT_ERROR_HANDLING = "replace".
  • Expanded ExecutionContext with:
    • stdout_sink and stderr_sink (text sinks for echoed output), defaulting to sys.stdout/sys.stderr.
    • encoding and errors for decoding subprocess output.
  • Added _StreamConfig dataclass to encapsulate per-stream decoding and echo behavior:
    • capture_output, echo_output, sink, encoding, errors.
  • SafeCmd.run now wires sinks from context and uses a per-stream _StreamConfig for both stdout and stderr consumption.
  • Reworked stream consumption: _consume_stream accepts a config and respects per-stream capture, echo, and decoding settings.
  • Replaced ad-hoc stdout/stderr handling with a unified task approach using consumers list and asyncio.gather.
  • Improved _write_chunk to support custom encoding and error handling when echoing to sinks.
  • Hardened _terminate_process to safely handle missing processes on terminate/kill paths.

Tests

  • Added new unit tests in cuprum/unittests/test_safe_cmd_run.py:
    • test_run_captures_stderr_only – verifies capturing only stderr while stdout remains empty.
    • test_run_captures_and_echoes_stderr – ensures stderr is captured and echoed to the terminal when echo=True.
    • test_run_echoes_to_custom_sinks – validates that echoed output can be directed to custom sinks provided via ExecutionContext.
    • test_run_decodes_with_configured_encoding – confirms decoding uses configured encoding and errors (e.g., CP1252 with strict error handling).

Docs

  • Updated docs/users-guide.md:
    • Clarified the cancel_grace wording to describe termination vs. kill signals.
    • Documented stdout_sink and stderr_sink options for redirecting echoed output.
    • Documented encoding and errors options with defaults ("utf-8" and "replace").

Why

  • Provides safer, more predictable error handling when subprocesses are long-running or non-cooperative.
  • Enables flexible output handling by allowing custom sinks and explicit decoding behavior, improving testability and integration scenarios.

Testing plan

  • Run unit tests: pytest cuprum/unittests/test_safe_cmd_run.py
  • Validate new encoding test against CP1252 environments.
  • Manually inspect behavior when using custom sinks and echo=True for both stdout and stderr.

Compatibility

  • Backwards compatible for existing users: default behavior remains stdout/stderr echoed to the terminal and decoded with UTF-8 using replace errors unless overridden.
  • New options are optional and opt-in via ExecutionContext and SafeCmd.run parameters.

🌿 Generated by Terry


ℹ️ Tag @terragon-labs to ask questions and address PR feedback

📎 Task: https://www.terragonlabs.com/task/39077fba-50c2-4c7a-911b-a04a8a1747d4

Summary by Sourcery

Enhance SafeCmd.run stream handling with configurable sinks, decoding, and more robust process termination behavior.

New Features:

  • Allow SafeCmd.run to direct echoed stdout and stderr to configurable text sinks via ExecutionContext.
  • Support configurable character encoding and error handling for decoding subprocess output, applied per stream.

Enhancements:

  • Unify stdout and stderr consumption through a shared stream configuration, enabling independent capture and echo semantics.
  • Improve subprocess termination logic to safely handle missing or already-exited processes during terminate/kill flows.

Documentation:

  • Update the user guide to document new ExecutionContext options for cancel_grace semantics, output sinks, and encoding/error configuration.

Tests:

  • Add unit tests covering stderr-only capture, combined capture and echo, custom echo sinks, and non-default encoding/decoding behavior.

…ss run

- Introduce stdout_sink and stderr_sink in ExecutionContext to allow redirection of echoed subprocess output to custom text streams.
- Add encoding and errors options to ExecutionContext for decoding subprocess output with configurable character encoding and error handling.
- Refactor internal stream consumption to use _StreamConfig dataclass for unified stream handling.
- Update _write_chunk to respect encoding and error parameters.
- Ensure run(echo=True) respects and writes to provided sinks instead of default sys.stdout/stderr.
- Add tests covering stderr-only capture, echoing to custom sinks, and decoding with configured encoding/errors.
- Update user guide to document new ExecutionContext options controlling subprocess output handling.

Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
@coderabbitai

coderabbitai Bot commented Dec 7, 2025

Copy link
Copy Markdown

Summary by CodeRabbit

Release Notes

  • New Features

    • Added customisable output sinks to direct echoed stdout and stderr to alternative streams.
    • Introduced configurable character encoding and error handling options for command execution.
  • Improvements

    • Enhanced subprocess I/O handling for more reliable command output management.
    • Improved process termination resilience with better error handling.
  • Tests

    • Added comprehensive test coverage for stderr capture and custom output routing scenarios.
  • Documentation

    • Updated execution context reference with new configuration options.

✏️ Tip: You can customize this high-level summary in your review settings.

Walkthrough

The cuprum library's subprocess execution context has been refactored to introduce unified stream configuration. ExecutionContext gained encoding, error handling, and custom sink parameters. Subprocess I/O handling now uses a coordinated _StreamConfig mechanism for stdout/stderr, with enhanced process termination and improved decoding consistency across streams.

Changes

Cohort / File(s) Change Summary
Stream configuration refactoring
cuprum/sh.py
Added _DEFAULT_ENCODING and _DEFAULT_ERROR_HANDLING constants; introduced _StreamConfig dataclass for per-stream decoding and echoing settings; expanded ExecutionContext with stdout_sink, stderr_sink, encoding, and errors fields; replaced separate stdout/stderr consumption tasks with coordinated consumers; reworked _consume_stream and _write_chunk to accept encoding/error parameters; hardened _terminate_process to handle ProcessLookupError/OSError.
Test coverage expansion
cuprum/unittests/test_safe_cmd_run.py
Added four test functions covering stderr capture, stderr capture with echoing, custom output sinks, and configured encoding/error handling; added io module import.
Documentation updates
docs/users-guide.md
Expanded cancel_grace description to specify SIGTERM and SIGKILL escalation; documented new stdout_sink and stderr_sink options; documented encoding and errors configuration parameters with defaults.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

  • The _StreamConfig dataclass and its integration throughout the I/O pipeline
  • Buffering logic and capture-conditional buffering in _consume_stream
  • Process termination error handling and exception propagation
  • Stream coordination mechanism between stdout and stderr consumers

Poem

Streams now flow through config's guiding hand,
Sinks redirect where echo commands stand,
Encoding whispers through the pipeline's dance,
Graceful termination's second chance,
Process I/O, refined and planned! 🔄

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and directly summarises the main changes: enhanced error handling, configurable output sinks, and decoding options for SafeCmd.run.
Description check ✅ Passed The description is comprehensive and directly related to the changeset, detailing enhancements to SafeCmd.run including error handling, sinks, decoding, tests, and documentation updates.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch terragon/improve-safecmd-run-error-handling-jr0std

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 29a80e4 and 75106ee.

📒 Files selected for processing (3)
  • cuprum/sh.py (7 hunks)
  • cuprum/unittests/test_safe_cmd_run.py (3 hunks)
  • docs/users-guide.md (1 hunks)
🧰 Additional context used
📓 Path-based instructions (11)
docs/**/*.md

📄 CodeRabbit inference engine (AGENTS.md)

docs/**/*.md: Use markdown files within the docs/ 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 the docs/ directory to reflect the latest state.
All documentation must adhere to the documentation style guide at docs/documentation-style-guide.md.
Record any design decisions made in the relevant design document.

docs/**/*.md: Document hook usage patterns
Document concurrency helper patterns and examples

Files:

  • docs/users-guide.md
docs/users-guide.md

📄 CodeRabbit inference engine (AGENTS.md)

docs/users-guide.md: Ensure new functionality is clearly documented in the docs/users-guide.md file.
Ensure revised functionality is clearly documented in the docs/users-guide.md file.

docs/users-guide.md: Document the expected builder pattern in docs/users-guide.md
Provide a scaffold and guidance for project-specific builders, including a template module and checklist in docs/users-guide.md

Files:

  • docs/users-guide.md
**/*.md

📄 CodeRabbit inference engine (AGENTS.md)

**/*.md: For Markdown files (.md only), ensure linting passes by running make markdownlint.
For Markdown files, validate Mermaid diagrams by running make 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 using make markdownlint.
Run make fmt after any documentation changes to format all Markdown files and fix table markup.
Validate Mermaid diagrams in Markdown files by running make nixie.

Files:

  • docs/users-guide.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/users-guide.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, use ![alt text](path/to/image) and 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/users-guide.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/users-guide.md
docs/**/*.{md,mdx}

📄 CodeRabbit inference engine (docs/documentation-style-guide.md)

Follow markdownlint recommendations for Markdown formatting

Files:

  • docs/users-guide.md
docs/{users-guide.md,**/RELEASE*,**/CHANGELOG*}

📄 CodeRabbit inference engine (docs/roadmap.md)

Document policy switches and recommended defaults in docs/users-guide.md and add release notes describing the migration path for existing users

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 running make test.
For Python files, ensure linting passes by running make lint.
For Python files, ensure formatting adheres to standards by running make check-fmt and applying make fmt if needed.
For Python files, ensure type checking passes by running make 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 (with contextlib.contextmanager or class-based __enter__/__exit__) to encapsulate setup and teardown logic for resource management (f...

Files:

  • cuprum/sh.py
  • 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 numpy style 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 by FIXME: or a ticket link, and used only as a last resort.
  • Use pytest fixtures for shared setup (conftest.py or fixtures/)
  • Replace duplicate tests with @pytest.mark.parametrize
  • Prefer pytest-mock or unittest.mock for 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/case or 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/sh.py
  • 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 (2)
tests/helpers/catalogue.py (1)
  • python_builder (29-32)
cuprum/sh.py (4)
  • SafeCmd (149-259)
  • run (161-259)
  • ok (100-102)
  • ExecutionContext (106-134)
🔍 Remote MCP Ref

Based on my search results, I now have sufficient context to provide a focused summary relevant to reviewing this PR.

Summary of Additional Context for PR Review

Python Error Handling Context

The PR's default error handling strategy of "replace" is a standard Python codec error handler that replaces malformed data with a replacement marker (on decoding, uses , U+FFFD), making it a safe default choice for subprocess output that may contain encoding mismatches. This is more lenient than "strict" (which raises exceptions) but still preserves visibility of encoding issues.

Process Termination Best Practices

The PR's hardening of process termination aligns with Python's asyncio subprocess behavior: on POSIX systems, terminate() sends SIGTERM and kill() sends SIGKILL. The introduction of exception handling for ProcessLookupError and OSError during terminate/kill sequences is appropriate, as ProcessLookupError is raised when a given process doesn't exist, which can occur if the process exits between termination attempts.

Stream Configuration Architecture

The PR's approach of unifying stream handling through a _StreamConfig dataclass and using asyncio.gather for coordinated consumer tasks aligns with standard Python asyncio patterns for managing multiple concurrent I/O operations on separate streams.

Test Coverage Considerations

The new test functions (test_run_captures_stderr_only, test_run_captures_and_echoes_stderr, test_run_echoes_to_custom_sinks, test_run_decodes_with_configured_encoding) appropriately target encoding/decoding edge cases and output routing, which are the core new features. The use of pytest's capsys fixture is standard for testing captured output behavior.

Backwards Compatibility

The PR maintains backwards compatibility by preserving existing behavior through sensible defaults: UTF-8 encoding with "replace" error handling matches common subprocess expectations, and the new stdout_sink/stderr_sink parameters are optional.,

🔇 Additional comments (10)
docs/users-guide.md (1)

113-118: Align documentation with ExecutionContext encoding and sink behaviour

Keep these bullets as written; they accurately document cancel_grace signal semantics, the new stdout_sink/stderr_sink echo routing, and the encoding/errors defaults, and they match the ExecutionContext API in cuprum/sh.py.

cuprum/unittests/test_safe_cmd_run.py (4)

41-57: Validate independent stderr capture semantics

Keep test_run_captures_stderr_only as written; it validates that SafeCmd.run() captures stderr even when stdout is unused, and that CommandResult.stdout is the empty string while CommandResult.stderr carries the message.


59-80: Retain combined capture-and-echo test for stderr

Keep test_run_captures_and_echoes_stderr; it exercises the echo=True path for stderr, asserting both the captured stderr content and the echoed bytes observed via capsys, and it confirms stdout remains empty in this scenario.


213-244: Exercise custom stdout/stderr sink routing under echo

Keep test_run_echoes_to_custom_sinks; it verifies that ExecutionContext.stdout_sink and ExecutionContext.stderr_sink receive echoed output and that nothing leaks to the real stdio captured by capsys, which is essential to validate the new sink wiring in SafeCmd.run.


246-268: Confirm configured encoding and strict error handling are honoured

Keep test_run_decodes_with_configured_encoding; it asserts that ExecutionContext(encoding="cp1252", errors="strict") governs decoding, and that byte 0x96 on stdout decodes to "\u2013" (EN DASH) with no stderr output.

In Python's standard encodings, does the `cp1252` codec map the byte 0x96 to the Unicode character U+2013 (EN DASH)?
cuprum/sh.py (5)

35-37: Preserve backwards-compatible decoding defaults in ExecutionContext

Keep _DEFAULT_ENCODING = "utf-8" and _DEFAULT_ERROR_HANDLING = "replace" and wire them into ExecutionContext.encoding and ExecutionContext.errors as done; this preserves prior tolerant decoding behaviour while exposing the configuration cleanly, and appending the new fields at the end of the dataclass keeps positional construction compatible.

Also applies to: 128-135


105-135: Retain ExecutionContext sink and stream configuration design

Keep the stdout_sink/stderr_sink and encoding/errors fields on ExecutionContext and the _StreamConfig dataclass as implemented; this design centralises stream-related configuration, makes the echo routing explicit, and avoids mutable defaults or hidden globals beyond the intended stdio fallbacks.

Also applies to: 137-145


185-241: Maintain unified SafeCmd.run stream wiring and cancellation handling

Keep the updated SafeCmd.run implementation; it:

  • Derives sinks from ExecutionContext, defaulting to sys.stdout/sys.stderr at call time.
  • Uses pipes only when either capture or echo is enabled, falling back to DEVNULL otherwise.
  • Shares a single _StreamConfig between streams, overriding only the sink for stderr.
  • Ensures that cancellation triggers _terminate_process and then awaits both consumer tasks with return_exceptions=True before re-raising, avoiding task leakage.

This structure gives predictable stdout/stderr semantics and robust cancellation behaviour without altering external SafeCmd.run signatures.

#!/bin/bash
# Inspect SafeCmd.run usage patterns and confirm no callers rely on previous
# stdout/stderr pipe behaviour (e.g. expecting pipes when capture=False, echo=False).

rg -n "run\(" cuprum -S -C3

Also applies to: 250-259


290-316: Keep _consume_stream and _write_chunk semantics for capture and echo

Retain the updated _consume_stream and _write_chunk logic; it:

  • Returns None for non-capturing configurations so CommandResult correctly exposes None for disabled capture.
  • Buffers bytes only when capturing, reducing memory use when echo-only.
  • Delegates echoing to _write_chunk, which writes to sink.buffer where available, and otherwise decodes once using the configured encoding and errors before writing to a text-only sink.

This design ensures that ExecutionContext.encoding/errors govern both captured text and echo on non-buffered sinks, while stdio echo remains efficient and encoding-agnostic.

Does Python's `io.TextIOBase` guarantee a `.buffer` attribute only for text wrappers around binary buffers (such as `sys.stdout` and `sys.stderr`), and omit it for `io.StringIO`?

Also applies to: 318-337


340-360: Retain hardened termination and kill error handling

Keep the revised _terminate_process implementation; it correctly:

  • Short-circuits when the process has already exited.
  • Guards both process.terminate() and process.kill() against ProcessLookupError and OSError, handling races where the process exits between calls.
  • Waits up to grace_period for graceful exit and only escalates to kill when necessary.

This behaviour improves robustness during cancellation without surfacing spurious errors when the child process has already died.

What exceptions can `asyncio.subprocess.Process.terminate()` and `.kill()` raise when the target process has already exited, according to the Python 3.11+ documentation?

Warning

Review ran into problems

🔥 Problems

Errors were encountered while retrieving linked issues.

Errors (1)
  • UTF-8: Entity not found: Issue - Could not find referenced Issue.

Comment @coderabbitai help to get the list of available commands and usage tips.

@sourcery-ai

sourcery-ai Bot commented Dec 7, 2025

Copy link
Copy Markdown
Contributor

Reviewer's Guide

Refactors SafeCmd.run’s subprocess stream handling to support per-stream configuration (capture/echo, sinks, encoding/errors), hardens termination logic against missing processes, and documents/tests the new behavior while preserving default behavior.

Sequence diagram for SafeCmd.run with per-stream config and cancellation

sequenceDiagram
    actor Caller
    participant SafeCmd
    participant ExecutionContext
    participant Subprocess as asyncio_subprocess_exec
    participant StdoutConsumer as _consume_stream_stdout
    participant StderrConsumer as _consume_stream_stderr
    participant Terminator as _terminate_process

    Caller->>SafeCmd: run(capture, echo, context)
    SafeCmd->>ExecutionContext: resolve ctx (env, sinks, encoding, errors)
    SafeCmd->>Subprocess: create_subprocess_exec(argv_with_program, stdout, stderr, env, cwd)
    Subprocess-->>SafeCmd: Process(process)

    SafeCmd->>SafeCmd: build _StreamConfig from ctx
    SafeCmd->>StdoutConsumer: create_task(_consume_stream(process.stdout, config_stdout))
    SafeCmd->>StderrConsumer: create_task(_consume_stream(process.stderr, config_stderr))

    par stream_consumers
        StdoutConsumer->>StdoutConsumer: read, optionally capture
        StdoutConsumer->>StdoutConsumer: echo via _write_chunk(stdout_sink, encoding, errors)
        StderrConsumer->>StderrConsumer: read, optionally capture
        StderrConsumer->>StderrConsumer: echo via _write_chunk(stderr_sink, encoding, errors)
    end

    alt normal_completion
        SafeCmd->>Subprocess: wait()
        Subprocess-->>SafeCmd: exit_code
        SafeCmd->>StdoutConsumer: gather result
        SafeCmd->>StderrConsumer: gather result
        StdoutConsumer-->>SafeCmd: stdout_text
        StderrConsumer-->>SafeCmd: stderr_text
        SafeCmd-->>Caller: CommandResult(stdout, stderr, exit_code)
    else cancellation
        Caller--xSafeCmd: cancel task (CancelledError)
        SafeCmd->>Terminator: _terminate_process(process, cancel_grace)
        Terminator->>Subprocess: terminate()
        Terminator->>Subprocess: wait with timeout
        alt timeout
            Terminator->>Subprocess: kill()
            Subprocess-->>Terminator: exit
        end
        SafeCmd->>StdoutConsumer: gather(return_exceptions=True)
        SafeCmd->>StderrConsumer: gather(return_exceptions=True)
        SafeCmd-->>Caller: propagate CancelledError
    end
Loading

Class diagram for updated SafeCmd.run stream handling

classDiagram
    class SafeCmd {
        +list argv_with_program
        +str program
        +run(capture, echo, context) CommandResult
    }

    class ExecutionContext {
        +_EnvMapping env
        +_CwdType cwd
        +float cancel_grace
        +IO~str~ stdout_sink
        +IO~str~ stderr_sink
        +str encoding
        +str errors
    }

    class _StreamConfig {
        +bool capture_output
        +bool echo_output
        +IO~str~ sink
        +str encoding
        +str errors
    }

    class CommandResult {
        +str program
        +list argv
        +int exit_code
        +str stdout
        +str stderr
    }

    class _consume_stream {
        +_consume_stream(stream, config) str
    }

    class _write_chunk {
        +_write_chunk(sink, chunk, encoding, errors) void
    }

    class _terminate_process {
        +_terminate_process(process, grace_period) void
    }

    SafeCmd --> ExecutionContext : uses
    SafeCmd --> CommandResult : returns
    SafeCmd --> _StreamConfig : configures
    SafeCmd ..> _consume_stream : calls
    SafeCmd ..> _terminate_process : calls
    _consume_stream ..> _write_chunk : calls
    ExecutionContext --> _StreamConfig : provides defaults
Loading

File-Level Changes

Change Details Files
Introduce configurable execution context defaults for encoding, error handling, and output sinks.
  • Add _DEFAULT_ENCODING and _DEFAULT_ERROR_HANDLING constants for subprocess output decoding.
  • Extend ExecutionContext to include stdout_sink, stderr_sink, encoding, and errors with sensible defaults.
  • Default stdout/stderr sinks to the active sys.stdout/sys.stderr when not explicitly provided.
cuprum/sh.py
docs/users-guide.md
Refactor SafeCmd.run to use a unified per-stream configuration for stdout/stderr capture, echo, and decoding.
  • Add _StreamConfig dataclass to represent capture/echo, sink, encoding, and errors for a stream.
  • Change subprocess creation to always use PIPE when capture or echo is enabled and DEVNULL otherwise.
  • Build a base _StreamConfig from ExecutionContext and use it for stdout, with a copy for stderr using the stderr sink.
  • Replace separate stdout/stderr tasks with a consumers tuple and asyncio.gather for symmetric handling and cancellation cleanup.
cuprum/sh.py
Update stream consumption and echoing to honor custom encodings, error handling, and sinks.
  • Change _consume_stream to take a _StreamConfig instead of multiple flags and to decode captured output with the configured encoding/errors.
  • Update _write_chunk to accept encoding/errors and decode the echoed bytes using those settings, including special handling for buffered sinks.
  • Ensure captured stdout/stderr return decoded text according to the ExecutionContext configuration.
cuprum/sh.py
Harden process termination logic to be resilient to already-terminated or missing processes.
  • Wrap process.terminate() and process.kill() in try/except blocks to ignore ProcessLookupError and OSError.
  • Keep the existing grace-period behavior using asyncio.wait_for around process.wait() and escalate to kill only after timeout when possible.
cuprum/sh.py
Expand tests and documentation for new stderr behavior, sinks, and encoding options.
  • Add tests covering stderr-only capture, combined capture+echo of stderr, custom stdout/stderr sinks via ExecutionContext, and non-UTF-8 decoding behavior.
  • Document stdout_sink, stderr_sink, encoding, and errors in the user guide along with clearer wording for cancel_grace behavior.
  • Verify that default behavior (utf-8 with replace, echo to stdio) remains unchanged when new options are not provided.
cuprum/unittests/test_safe_cmd_run.py
docs/users-guide.md

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@leynos
leynos marked this pull request as ready for review December 7, 2025 17:28

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry @leynos, you have reached your weekly rate limit of 2500000 diff characters.

Please try again later or upgrade to continue using Sourcery

@leynos
leynos merged commit ff46283 into main Dec 8, 2025
3 checks passed
@leynos
leynos deleted the terragon/improve-safecmd-run-error-handling-jr0std branch December 8, 2025 00:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant