Add SafeCmd.run_sync with full parity tests

[leynos]

Summary

• Implement SafeCmd.run_sync to drive the event loop synchronously with identical semantics to run()
• Add comprehensive unit tests for parity and edge-cases
• Update docs to reflect the synchronous API and usage

Changes

Core Functionality

• Implemented SafeCmd.run_sync in cuprum/sh.py using asyncio.run(self.run(...)) to mirror async behavior and identical return semantics (capture, echo, context).

Tests

• Added extensive unit tests in cuprum/unittests/test_safe_cmd_run.py to verify:
  • run_sync captures stdout/stderr and exit code by default
  • Capturing and stderr-only scenarios
  • echo=True echoes output while capturing
  • env overrides via ExecutionContext are correctly applied without leaking to global env
  • Non-zero exit codes are captured and ok flag is set accordingly
  • cwd override works as expected
  • capture=False disables capturing stdout/stderr
  • Echoing to custom sinks via ExecutionContext stdout_sink and stderr_sink
  • Decoding with configured encoding and error handling
• Extended behaviour tests:
  • tests/behaviour/test_execution_runtime.py: added scenario to cover synchronous run captures output by default
  • tests/features/execution_runtime.feature: added corresponding feature scenario

Documentation

• docs/roadmap.md: mark run_sync parity as implemented (x)
• docs/users-guide.md: document synchronous execution with run_sync and its parity with run()

Test Plan

• Run unit tests for SafeCmd run_sync parity (e.g., pytest cuprum/unittests/test_safe_cmd_run.py -q)
• Run full test suite (pytest) including tests related to execution runtime and behaviour
• Optionally run behave to execute updated behaviours/features (tests/features/execution_runtime.feature)

🌿 Generated by Terry

---

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

📎 Task: https://www.terragonlabs.com/task/9122a1b4-abf8-4c90-a8a1-c87d1e77cf7e

[coderabbitai]

Summary by CodeRabbit

Release Notes

• New Features

  • Added synchronous execution API—users can now use run_sync() to execute commands synchronously with identical parameters and return behaviour as the async variant.

• Documentation

  • Updated user guide with synchronous execution examples and guidance.

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

Walkthrough

A new synchronous execution method run_sync() is introduced to the SafeCmd class, wrapping the existing async run() via asyncio.run(). Supporting tests, documentation, and behavioural scenarios are added to demonstrate and validate synchronous command execution across both test suites and user guides.

Changes

Cohort / File(s)	Summary
Core implementation cuprum/sh.py	Adds run_sync() method to SafeCmd class, mirroring async run() with identical parameters and return type; executes via asyncio.run().
Unit test expansion cuprum/unittests/test_safe_cmd_run.py	Introduces parametrised execution strategy fixture to run existing test suite against both async run() and sync run_sync() paths; adds helper functions _execute_async() and _execute_sync() for generic command execution routing.
Documentation updates docs/roadmap.md, docs/users-guide.md	Marks run_sync task as completed in roadmap; adds "Synchronous execution" section to user guide with code example and parameter notes.
Behavioural tests tests/behaviour/test_execution_runtime.py, tests/features/execution_runtime.feature	Adds new scenario test_sync_run_captures_output() and corresponding step when_run_command_sync() to verify synchronous execution captures output by default; introduces Gherkin scenario for sync run validation.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

• Inspect run_sync() implementation in cuprum/sh.py to verify asyncio.run() invocation and parameter mapping match run() precisely.
• Verify parametrised fixture logic in test module correctly routes both execution paths without duplicating test logic.
• Confirm behavioural test assertions and step definitions align with existing async path expectations.

Poem

Sync comes to Cuprum, no await required,
A wrapper spins the loop, the task transpired,
From async roots, a sibling path unfurls,
Both run() and run_sync() dance and twirl. 🔄✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main change: implementing SafeCmd.run_sync and adding comprehensive tests for parity between sync and async execution.
Description check	✅ Passed	The description is well-structured and directly related to the changeset, covering core functionality, tests, documentation updates, and a test plan with clear context about the run_sync implementation.
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/implement-safe-cmd-run-sync-smyczh

---

📜 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 ff46283 and 6c1d45c.

📒 Files selected for processing (6)

• cuprum/sh.py (1 hunks)
• cuprum/unittests/test_safe_cmd_run.py (4 hunks)
• docs/roadmap.md (1 hunks)
• docs/users-guide.md (1 hunks)
• tests/behaviour/test_execution_runtime.py (2 hunks)
• tests/features/execution_runtime.feature (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/roadmap.md
• 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/roadmap.md
• 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/roadmap.md
• 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/roadmap.md
• 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/roadmap.md
• docs/users-guide.md

docs/**/*.{md,mdx}

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

Follow markdownlint recommendations for Markdown formatting

Files:

• docs/roadmap.md
• 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/unittests/test_safe_cmd_run.py
• cuprum/sh.py
• 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 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/unittests/test_safe_cmd_run.py
• cuprum/sh.py
• tests/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.py
• 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:

• cuprum/unittests/test_safe_cmd_run.py
• tests/behaviour/test_execution_runtime.py

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

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

🧬 Code graph analysis (2)

cuprum/unittests/test_safe_cmd_run.py (2)

cuprum/sh.py (7)

• CommandResult (72-102)
• ExecutionContext (106-134)
• SafeCmd (149-288)
• run (161-259)
• run_sync (261-288)
• make (291-307)
• ok (100-102)

tests/helpers/catalogue.py (1)

• python_builder (29-32)

tests/behaviour/test_execution_runtime.py (1)

cuprum/sh.py (3)

• SafeCmd (149-288)
• CommandResult (72-102)
• run_sync (261-288)

🔍 Remote MCP Ref

Summary of Relevant Context for PR Review

Based on my research, here are the key relevant facts for reviewing this pull request:

asyncio.run() Design Pattern

The implementation uses asyncio.run() as the entry point for running an asynchronous program, which creates and manages the event loop and cleans up after it completes. This is a standard pattern and is designed to be called once per thread, making it suitable for the synchronous wrapper approach taken in run_sync().

Architectural Considerations

The PR implements run_sync() as a wrapper around run() via asyncio.run(), maintaining identical parameter signatures and return semantics. This provides users with:

• A synchronous API entry point without duplicating async logic
• Identical behavior and capture semantics between run() and run_sync()
• Clear API parity as documented in the updated users guide

Testing Pattern Notes

The context indicates the test suite has identified and is addressing code duplication through parametrization (the suggested fixture-based approach with execution_strategy). The proposed refactoring—consolidating 10 nearly-identical test pairs into unified tests with parametrization—is aligned with pytest best practices for testing multiple code paths with identical logic.

Coverage Quality

The test suite comprehensively covers both execution paths (async and sync) for:

• Output/exit code capture
• Stderr capture with selection
• Echo behavior
• Environment and working directory overrides
• Encoding/decoding configuration
• Non-zero exit handling
• Custom output sinks

🔇 Additional comments (11)

cuprum/sh.py (1)

261-289: Clean, minimal implementation of the synchronous wrapper.

The implementation correctly uses asyncio.run() to drive the event loop and mirrors the run() signature exactly. The docstring clearly documents the parity and behavior. Parameter forwarding is correct, and letting exceptions propagate naturally is the right choice.

tests/features/execution_runtime.feature (1)

19-22: Appropriate BDD coverage for the synchronous execution path.

The scenario correctly mirrors the async test structure and exercises the new run_sync() API through the behaviour test layer.

docs/roadmap.md (1)

28-30: Roadmap correctly updated to reflect completion.

The task status accurately reflects the delivered functionality.

docs/users-guide.md (1)

139-157: Clear and comprehensive documentation of the synchronous API.

The new section appropriately describes when to use run_sync(), demonstrates its usage with a parallel example, and documents the implementation approach. The context clarification ("scripts or contexts where async/await is not available") helps users choose the right method.

tests/behaviour/test_execution_runtime.py (2)

48-54: Scenario function correctly added for sync execution coverage.

The test follows the existing pattern and integrates cleanly with the BDD suite.

---

78-85: Step definition cleanly exercises the synchronous path.

The implementation directly calls run_sync() and reuses existing given/then steps appropriately, minimizing duplication.

cuprum/unittests/test_safe_cmd_run.py (5)

16-16: Explicit imports improve clarity.

Adding CommandResult and ExecutionContext to the import list makes the module's dependencies clear and supports the refactored test signatures.

---

22-41: Excellent parametrization fixture eliminates test duplication.

The execution_strategy fixture follows pytest best practices:

• Clear parametrization over both execution paths
• Readable test IDs (run(), run_sync())
• Type alias ExecuteFn documents the helper signature
• Helper functions correctly route to async and sync implementations
• Fixture return tuple enables consistent test patterns

This approach consolidates the test pairs into unified tests whilst maintaining identical coverage, addressing the code duplication concern raised in the PR objectives.

---

49-191: Parametrized tests provide comprehensive coverage for both execution paths.

The refactored tests correctly:

• Accept and unpack the execution_strategy fixture
• Update docstrings to reflect dual coverage ("Both run() and run_sync()...")
• Use the generic execute(command, kwargs) pattern
• Preserve all original assertions and test logic
• Cover stdout/stderr capture, echo behavior, environment overlays, working directory, non-zero exits, and capture toggles

Each test now runs twice (once for async, once for sync), ensuring parity whilst eliminating duplication.

---

193-254: New tests expand coverage for sinks and encoding configuration.

test_echoes_to_custom_sinks() verifies that stdout_sink and stderr_sink in ExecutionContext correctly route echoed output, ensuring the parent process streams remain unaffected.

test_decodes_with_configured_encoding() confirms that both execution paths respect the configured encoding and errors parameters when decoding subprocess output (demonstrated with cp1252 encoding of byte 0x96 → U+2013).

Both tests exercise features across async and sync paths via the parametrization fixture.

---

256-328: Async-only test correctly preserved and separated.

The cancellation escalation test appropriately remains async-only because it relies on asyncio.create_task() and task cancellation, which are inherent to the async execution model. The section header clearly marks the boundary, and the test logic exercises the async run() method's cancellation semantics (SIGTERM → SIGKILL escalation) that cannot be replicated in the sync path.

---

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

[sourcery-ai]

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

Please try again later or upgrade to continue using Sourcery

[leynos]

@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix:

cuprum/unittests/test_safe_cmd_run.py

Comment on lines +442 to +461

def test_run_sync_decodes_with_configured_encoding(
    python_builder: typ.Callable[..., SafeCmd],
) -> None:
    """run_sync() uses the configured encoding/errors when decoding output."""
    command = python_builder(
        "-c",
        ("import sys; sys.stdout.buffer.write(bytes([0x96])); sys.stdout.flush()"),
    )

    result = command.run_sync(
        context=ExecutionContext(
            encoding="cp1252",
            errors="strict",
        ),
    )

    assert result.exit_code == 0
    assert result.ok is True
    assert result.stdout == "\u2013"
    assert result.stderr == ""

❌ New issue: Code Duplication
The module contains 12 functions with similar structure: test_run_applies_cwd_override,test_run_applies_env_overrides,test_run_captures_and_echoes_stderr,test_run_captures_stderr_only and 8 more functions

[sourcery-ai]

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

Please try again later or upgrade to continue using Sourcery

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".