Add timeout support with ScopeConfig scoping and packaging metadata - #22
Conversation
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughIntroduce runtime timeout support and TimeoutExpired; add ScopeConfig to carry scoped defaults (including timeout) and update CuprumContext/scoped API; propagate timeout resolution through SafeCmd and Pipeline execution paths; update docs, examples and tests to use ScopeConfig and validate timeout behaviour. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User Code
participant SafeCmd as SafeCmd.run()
participant Resolver as _resolve_timeout()
participant Context as ExecutionContext
participant Scope as ScopeConfig
User->>SafeCmd: run(timeout=5.0, context=ctx)
SafeCmd->>Resolver: resolve(timeout=5.0, context=ctx)
alt Explicit timeout provided
Resolver-->>SafeCmd: return 5.0
else Explicit timeout None, context provided
Resolver->>Context: check context.timeout
alt context.timeout set
Context-->>Resolver: return context.timeout
else context.timeout None
Resolver->>Scope: check scoped ScopeConfig.timeout
Scope-->>Resolver: return scoped timeout or None
end
else No explicit timeout, no context
Resolver->>Scope: check scoped ScopeConfig.timeout
Scope-->>Resolver: return scoped timeout or None
end
Resolver-->>SafeCmd: effective timeout
SafeCmd->>SafeCmd: execute subprocess with resolved timeout
sequenceDiagram
participant SafeCmd as SafeCmd.run()
participant Subproc as Subprocess
participant Wait as _wait_for_exit_code()
participant Streams as Stream Consumers
participant Observer as Hooks/Observers
participant Error as TimeoutExpired
SafeCmd->>Subproc: spawn process, start Streams
SafeCmd->>Wait: await exit (with timeout)
alt Completes within timeout
Subproc-->>Wait: exit code
Streams->>Observer: emit captured data/events
Wait->>SafeCmd: return CommandResult
else Timeout expires
Wait->>Subproc: terminate/kill process
Streams->>Wait: gather partial stdout/stderr
Wait->>Error: construct TimeoutExpired(timeout=T, stdout=..., stderr=...)
Wait->>Observer: emit timeout event
Wait-->>SafeCmd: raise TimeoutExpired
SafeCmd->>Streams: cancel remaining consumers and cleanup
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Reviewer's GuideUpdates Cuprum design and roadmap docs to document new timeout support and scoped runtime defaults for SafeCmd and Pipeline execution, including API signatures, timeout semantics, precedence rules, and minor style-guide tweaks. Sequence diagram for SafeCmd.run timeout resolution and process lifetimesequenceDiagram
actor User
participant SafeCmd
participant CuprumContext
participant ExecutionContext
participant Subprocess
participant TimeoutExpired
User->>SafeCmd: run(capture, echo, timeout_arg, context)
alt context is provided
SafeCmd->>ExecutionContext: read timeout
ExecutionContext-->>SafeCmd: context_timeout
end
SafeCmd->>CuprumContext: read runtime_timeout
CuprumContext-->>SafeCmd: scoped_timeout
Note over SafeCmd: Resolve effective_timeout
alt timeout_arg is not None
SafeCmd-->>SafeCmd: effective_timeout = timeout_arg
else timeout_arg is None and context_timeout is not None
SafeCmd-->>SafeCmd: effective_timeout = context_timeout
else
SafeCmd-->>SafeCmd: effective_timeout = scoped_timeout
end
SafeCmd->>Subprocess: start process
alt effective_timeout is None
Subprocess-->>SafeCmd: complete normally
SafeCmd-->>User: return result
else effective_timeout is set
SafeCmd-->>Subprocess: wait until completion or timeout
alt process exceeds effective_timeout
SafeCmd->>Subprocess: terminate
SafeCmd-->>Subprocess: wait cancel_grace
alt still running
SafeCmd->>Subprocess: kill
end
Subprocess-->>SafeCmd: partial stdout, partial stderr
SafeCmd->>TimeoutExpired: construct(cmd, effective_timeout, stdout, stderr)
TimeoutExpired-->>User: raise exception
else process completes in time
Subprocess-->>SafeCmd: stdout, stderr, exit_code
SafeCmd-->>User: return result
end
end
Class diagram for Cuprum timeout-enabled execution APIclassDiagram
class SafeCmd {
+async run(capture bool = True, echo bool = False, timeout float|None = None, context ExecutionContext|None = None) object
+run_sync(capture bool = True, echo bool = False, timeout float|None = None, context ExecutionContext|None = None) object
+before(hook BeforeHook) SafeCmd
+after(hook AfterHook) SafeCmd
+__or__(other SafeCmd) Pipeline
}
class Pipeline {
<<generic>>
+run(capture bool = True, echo bool = False, timeout float|None = None, context ExecutionContext|None = None) PipelineResult
+run_sync(capture bool = True, echo bool = False, timeout float|None = None, context ExecutionContext|None = None) PipelineResult
}
class ExecutionContext {
+timeout float|None
+cancel_grace float
}
class CuprumContext {
+runtime_timeout float|None
+before_hooks list
+after_hooks list
+output_hooks list
+scoped(timeout float|None) CuprumContextManager
}
class TimeoutExpired {
+cmd object
+timeout float
+stdout object|None
+stderr object|None
}
SafeCmd --> ExecutionContext : uses
Pipeline --> ExecutionContext : uses
SafeCmd --> CuprumContext : reads defaults
Pipeline --> CuprumContext : reads defaults
SafeCmd --> TimeoutExpired : raises
Pipeline --> TimeoutExpired : raises
Flow diagram for timeout precedence resolutionflowchart TD
A[Start timeout resolution] --> B{timeout_arg is not None}
B -- Yes --> C[effective_timeout = timeout_arg]
B -- No --> D{context is provided and context.timeout is not None}
D -- Yes --> E[effective_timeout = context.timeout]
D -- No --> F{CuprumContext.runtime_timeout is not None}
F -- Yes --> G[effective_timeout = CuprumContext.runtime_timeout]
F -- No --> H[effective_timeout = None]
C --> I[Use effective_timeout for process run]
E --> I
G --> I
H --> I
I --> J[End timeout resolution]
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}
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The roadmap item 3.4.2 currently says scoped timeouts have precedence over per-call values, which contradicts the earlier precedence list (explicit timeout > ExecutionContext.timeout > CuprumContext); please reword to match the intended order.
- In
docs/cuprum-design.md, the section is labeled8.1.4 Timeouts (proposal)while the API signatures above already includetimeoutandcontext; consider clarifying whether this is still a proposal or now a committed design to avoid confusion. - The timeout precedence description could be tightened by explicitly stating what happens when both an
ExecutionContextand aCuprumContextdefault are present but the context’stimeoutisNone(i.e., whether it falls back to the scoped default or is treated as “no timeout”).
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The roadmap item 3.4.2 currently says scoped timeouts have precedence over per-call values, which contradicts the earlier precedence list (explicit timeout > ExecutionContext.timeout > CuprumContext); please reword to match the intended order.
- In `docs/cuprum-design.md`, the section is labeled `8.1.4 Timeouts (proposal)` while the API signatures above already include `timeout` and `context`; consider clarifying whether this is still a proposal or now a committed design to avoid confusion.
- The timeout precedence description could be tightened by explicitly stating what happens when both an `ExecutionContext` and a `CuprumContext` default are present but the context’s `timeout` is `None` (i.e., whether it falls back to the scoped default or is treated as “no timeout”).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: 2
🤖 Fix all issues with AI agents
In @docs/cuprum-design.md:
- Line 994: The sentence joining two independent clauses needs a comma before
"so": edit the line referencing `.stdout` / `.stderr` to insert a comma
immediately before "so" so it reads that `.stdout` / `.stderr` carry any
captured output, so callers can inspect; update the text where the `.stdout` /
`.stderr` phrase appears to include this comma.
In @docs/roadmap.md:
- Line 104: Replace the ASCII three-dot ellipsis in the code example with the
Unicode ellipsis: find the string `sh.scoped(timeout=...)` in the
docs/roadmap.md content and change it to `sh.scoped(timeout=…)` so the example
uses the proper Unicode ellipsis character.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
docs/cuprum-design.mddocs/documentation-style-guide.mddocs/roadmap.md
🧰 Additional context used
📓 Path-based instructions (6)
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.Add troubleshooting section for common issues: missing wheel on exotic platforms, forced fallback behaviour, and benchmark result interpretation.
Files:
docs/roadmap.mddocs/documentation-style-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/documentation-style-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/documentation-style-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/documentation-style-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/documentation-style-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/documentation-style-guide.mddocs/cuprum-design.md
docs/cuprum-design.md
📄 CodeRabbit inference engine (docs/roadmap.md)
Extend
docs/cuprum-design.mdwith Section 13 covering Rust extension architecture, API boundary, fallback strategy, and performance characteristics.
Files:
docs/cuprum-design.md
🪛 LanguageTool
docs/roadmap.md
[style] ~104-~104: Consider using the typographical ellipsis character here instead.
Context: ...can be set once per scope (for example, sh.scoped(timeout=...)) with clear precedence over per-cal...
(ELLIPSIS)
docs/cuprum-design.md
[uncategorized] ~994-~994: Use a comma before ‘so’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...t/.stderr` carry any captured output so callers can inspect partial results...
(COMMA_COMPOUND_SENTENCE_2)
⏰ 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)
docs/documentation-style-guide.md (1)
162-164: Typographic standardisation on Unicode ellipsis is good.These changes align with modern documentation practices and make the roadmap formatting examples consistent. No concerns.
docs/cuprum-design.md (4)
273-274: Update terminology: "misc flags" → "runtime defaults" for clarity.The rename from "misc flags" to "runtime defaults" provides clearer semantics and aligns with the timeout design proposal in section 8.1.4. This change improves documentation precision.
350-365: SafeCmd API signatures correctly document new timeout and context parameters.The signature updates properly reflect the PR's timeout and execution context additions. Parameter names and types are clear; documentation integrates well with the existing API shape.
389-390: Pipeline API signatures correctly document new timeout and context parameters.Parallel updates to
Pipeline.run()andrun_sync()maintain consistency withSafeCmdand accurately reflect the expanded API surface for pipeline timeouts.Also applies to: 397-398
953-999: Section 8.1.4 provides comprehensive timeout design proposal.The new section is well-structured, covering:
- Default-off semantics and per-call vs. scoped defaults;
- Clear resolution order (explicit argument → ExecutionContext → CuprumContext);
- Exception surface and partial output capture semantics;
- Pipeline-specific timeout handling.
The proposal aligns with PR objectives and existing design patterns. Cross-references to section 5.3 (runtime defaults) and roadmap (section 3.4) are properly established.
leynos
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @docs/cuprum-design.md:
- Around line 1000-1001: Add a comma before "so" in the sentence describing
.stdout / .stderr: change ".stdout / .stderr carry any captured output so
callers can inspect partial results (set to `None` when `capture=False`)." to
include a comma after "output" so it reads ".stdout / .stderr carry any captured
output, so callers can inspect partial results (set to `None` when
`capture=False`)."
In @docs/roadmap.md:
- Line 98: Update the subsection heading to match the existing numbering style
by changing "### 3.4 Execution timeouts" to "### 3.4. Execution timeouts" so it
includes the trailing period after the section number like other headings
(locate the heading text in docs/roadmap.md and add the missing dot).
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
cuprum/__init__.pycuprum/_meta.pydocs/cuprum-design.mddocs/roadmap.md
🧰 Additional context used
📓 Path-based instructions (7)
**/*.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/_meta.pycuprum/__init__.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/_meta.pycuprum/__init__.py
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.Add troubleshooting section for common issues: missing wheel on exotic platforms, forced fallback behaviour, and benchmark result interpretation.
Files:
docs/roadmap.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/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/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/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/cuprum-design.md
docs/**/*.{md,mdx}
📄 CodeRabbit inference engine (docs/documentation-style-guide.md)
Follow markdownlint recommendations for Markdown formatting
Files:
docs/roadmap.mddocs/cuprum-design.md
docs/cuprum-design.md
📄 CodeRabbit inference engine (docs/roadmap.md)
Extend
docs/cuprum-design.mdwith Section 13 covering Rust extension architecture, API boundary, fallback strategy, and performance characteristics.
Files:
docs/cuprum-design.md
🪛 LanguageTool
docs/cuprum-design.md
[typographical] ~986-~986: The word ‘when’ starts a question. Add a question mark (“?”) at the end of the sentence.
Context: ...xt.timeoutwhen provided and notNone. If a context is provided with timeo...
(WRB_QUESTION_MARK)
[uncategorized] ~1000-~1000: Use a comma before ‘so’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...t/.stderr` carry any captured output so callers can inspect partial results...
(COMMA_COMPOUND_SENTENCE_2)
🔍 Remote MCP
Let me search the web for information about Cuprum to provide context for the PR review:The search returned results about Plumbum (a different shell combinators library), not Cuprum. Let me search more specifically for Cuprum and its timeout semantics:Based on my research, I have gathered sufficient context about timeout patterns in Python and subprocess handling. Let me provide a summary of relevant findings for the PR review.
Summary of Relevant Context for PR #22 Review
Timeout Implementation Patterns & Standards
subprocess.TimeoutExpired Exception Model:
subprocess.TimeoutExpired is a subclass of SubprocessError, raised when a timeout expires while waiting for a child process. It includes attributes for the command (cmd) that was used to spawn the process, the timeout in seconds, and output/stdout/stderr if captured by run(). When the timeout expires, the child process will be killed and waited for, and the TimeoutExpired exception will be re-raised after the child process has terminated.
Process Cleanup on Timeout:
Because the child process does not end if the timeout expires, to properly clean up you must explicitly end the child process and finish communication. Compliant implementations explicitly kill the process when TimeoutExpired is caught, then communicate to finalize.
Graceful Termination Pattern:
If you are interested in stopping the execution of the external command after a given timeout, subprocess.run() allows catching TimeoutExpired exception. For more complex scenarios like child processes, the workaround is to create a new session by setting start_new_session=True in the Popen constructor, equivalent to using preexec_fn=os.setsid.
Context Manager Timeout Semantics
Python's asyncio.timeout() Model:
The asyncio.timeout() context manager is what transforms asyncio.CancelledError into a TimeoutError, which means the TimeoutError can only be caught outside of the context manager. If a task takes more than the specified seconds to complete, the context manager will cancel the current task and handle the resulting asyncio.CancelledError internally, transforming it into a TimeoutError.
Key Review Considerations for Cuprum PR
-
Timeout Precedence Resolution: The PR specifies a clear precedence order (explicit timeout > ExecutionContext.timeout > scoped default), which aligns with standard Python patterns where local parameters override context defaults.
-
Exception Signature Alignment: The PR's plan to raise a TimeoutExpired-like exception with
.cmd,.timeout, and.stdout/.stderrattributes follows subprocess module conventions and ensures consistency with Python's standard library expectations. -
Partial Output Capture: The requirement to include partial stdout/stderr output when capture=True on timeout is a sensible design choice, allowing debugging of where processes got stuck.
-
Pipeline Timeout Semantics: The approach of applying timeout to the entire pipeline execution with termination of all stages is appropriate for batch operations.
-
Grace Period Implementation: The mentioned "cancel grace period" before forceful killing mirrors real-world process termination patterns (SIGTERM → wait → SIGKILL).
[::web_search::] for subprocess timeout patterns and Python asyncio context manager semantics
⏰ 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/_meta.py (1)
1-3: LGTM with minor docstring note.The module structure is correct and the constant follows naming conventions. The docstring is functional but sparse—coding guidelines specify that every module must begin with a docstring explaining its purpose, utility, and usage. For a simple metadata module this is acceptable, though a brief note on where the constant is consumed (e.g.,
cuprum.__init__) would aid discoverability.cuprum/__init__.py (1)
16-16: LGTM!The import from
cuprum._metafollows the existing import pattern and maintains backwards compatibility by re-exportingPACKAGE_NAMEvia__all__. This cleanly decouples package metadata from initialization code.docs/roadmap.md (1)
100-108: LGTM!The roadmap items accurately reflect the PR objectives: per-call timeouts, scoped defaults via
CuprumContext, and comprehensive testing including partial output capture. The cross-reference todocs/users-guide.mdin 3.4.3 ensures documentation is tracked.docs/cuprum-design.md (5)
273-274: LGTM!The terminology update from "misc flags" to "runtime defaults" accurately reflects the scoped timeout semantics and aligns with the API additions in this PR.
350-365: LGTM!The
SafeCmdmethod signatures correctly addtimeoutandcontextparameters with sensible defaults (None). The parameter ordering (capture, echo, timeout, context) is logical—runtime behaviour first, then timeout, then context for additional overrides.
384-399: LGTM!The
Pipelinemethod signatures mirrorSafeCmdexactly, maintaining API consistency. The documentation note that timeout applies to the entire pipeline run (not per-stage) is correctly captured in Section 8.1.4.
953-957: LGTM!The "planned design" framing is appropriate for documenting the target API surface whilst implementation is pending. The API sketches serve as a contract for implementers.
991-1005: LGTM!The timeout semantics are well-specified and align with
subprocess.TimeoutExpiredconventions:
- Wall-clock limit in seconds (float)
- Graceful termination with escalation to kill
- Exception with
.cmd,.timeout,.stdout/.stderrattributes- Pipeline-wide timeout with all-stage termination
The partial output capture on timeout is a valuable debugging aid.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @docs/cuprum-design.md:
- Around line 953-1006: The sentence "`.stdout` / `.stderr` carry any captured
output so callers can inspect partial results (set to `None` when
`capture=False`)." is missing a comma before "so"; update that sentence to read
"... carry any captured output, so callers can inspect partial results ..."
leaving the rest of the sentence unchanged to improve clarity.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
docs/cuprum-design.mddocs/roadmap.md
🧰 Additional context used
📓 Path-based instructions (6)
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.Add troubleshooting section for common issues: missing wheel on exotic platforms, forced fallback behaviour, and benchmark result interpretation.
Files:
docs/roadmap.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/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/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/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/cuprum-design.md
docs/**/*.{md,mdx}
📄 CodeRabbit inference engine (docs/documentation-style-guide.md)
Follow markdownlint recommendations for Markdown formatting
Files:
docs/roadmap.mddocs/cuprum-design.md
docs/cuprum-design.md
📄 CodeRabbit inference engine (docs/roadmap.md)
Extend
docs/cuprum-design.mdwith Section 13 covering Rust extension architecture, API boundary, fallback strategy, and performance characteristics.
Files:
docs/cuprum-design.md
🪛 LanguageTool
docs/cuprum-design.md
[uncategorized] ~1000-~1000: Use a comma before ‘so’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...t/.stderr` carry any captured output so callers can inspect partial results...
(COMMA_COMPOUND_SENTENCE_2)
⏰ 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)
docs/roadmap.md (1)
98-109: LGTM!The execution timeouts subsection is well structured and aligns with the design document. The tasks clearly describe the API additions (per-call timeout parameters), scoped defaults via CuprumContext, and testing requirements. Line wrapping at 80 columns is correct, and the references to
subprocess.runsemantics anddocs/users-guide.mdprovide clear guidance for implementers.docs/cuprum-design.md (5)
273-275: LGTM!The addition of runtime defaults to the execution context description correctly introduces the concept that scoped timeouts and other execution defaults can be applied when explicit overrides are not supplied. This aligns with the scoped context model described elsewhere in the document.
384-399: LGTM!The Pipeline API signatures mirror the SafeCmd additions, maintaining consistency across the execution surface. The timeout and context parameters follow the same pattern, which will provide a uniform experience for callers.
951-951: Clarification improves async cancellation semantics.The updated wording "this ensures
run_sync()does not leak pending tasks" makes the behaviour explicit. This is important for understanding how synchronous execution interacts with async observers.
976-981: No action required—the documentation already provides complete timeout resolution semantics.The examples at lines 976–981, followed immediately by the "Resolution order for timeouts" section (lines 983–989), adequately clarify the timeout mechanism. Step 3 explicitly states that
CuprumContextruntime default is the scoped default, and step 2 documents the optionalExecutionContextparameter. Scoped timeout is automatically resolved fromCuprumContextwithout requiring explicit context parameter passing in the examples, which is the correct and intended behaviour. No changes needed.
350-365: Documentation already correctly reflects timeout precedence.Section 8.1.4 (line 953) documents timeout resolution with clear precedence rules: explicit timeout arguments take priority, then ExecutionContext timeouts, then CuprumContext defaults. The API signatures at lines 350-365 align perfectly with this design—both
runandrun_syncmethods includetimeoutandcontextparameters that match the documented precedence order. No inconsistencies exist between the API surface and the timeout handling documentation.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @docs/cuprum-design.md:
- Around line 953-1005: In the "8.1.4 Timeouts (planned design)" section two
prose lines exceed the 80-column limit; rewrap the long sentences so each line
is ≤80 characters while preserving wording and punctuation (do not change
semantics), specifically adjust the long sentence describing the proposed API
shape and the long sentence describing resolution order step 2 so they break at
natural word boundaries and keep the bulleted/numbered structure intact.
In @docs/roadmap.md:
- Around line 98-110: Line 103 in the "3.4. Execution timeouts" section exceeds
the 80-column limit; rewrap the long bullet for 3.4.1 so no prose line is longer
than 80 characters. Specifically, break the sentence that starts "3.4.1. Add
`timeout` parameters to `SafeCmd.run` / `run_sync` and `Pipeline.run` /
`run_sync`, matching `subprocess.run` semantics and surfacing a `TimeoutExpired`
exception with partial output when captured." into two or more lines (for
example, put "matching `subprocess.run` semantics and surfacing a
`TimeoutExpired` exception" on the next line or split after "and"), ensuring
each resulting line is ≤80 chars while preserving the exact text and
backtick-wrapped symbols (`SafeCmd.run`, `run_sync`, `Pipeline.run`,
`TimeoutExpired`).
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
docs/cuprum-design.mddocs/roadmap.md
🧰 Additional context used
📓 Path-based instructions (6)
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.Add troubleshooting section for common issues: missing wheel on exotic platforms, forced fallback behaviour, and benchmark result interpretation.
Files:
docs/cuprum-design.mddocs/roadmap.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/cuprum-design.mddocs/roadmap.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/cuprum-design.mddocs/roadmap.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/cuprum-design.mddocs/roadmap.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/cuprum-design.mddocs/roadmap.md
docs/**/*.{md,mdx}
📄 CodeRabbit inference engine (docs/documentation-style-guide.md)
Follow markdownlint recommendations for Markdown formatting
Files:
docs/cuprum-design.mddocs/roadmap.md
docs/cuprum-design.md
📄 CodeRabbit inference engine (docs/roadmap.md)
Extend
docs/cuprum-design.mdwith Section 13 covering Rust extension architecture, API boundary, fallback strategy, and performance characteristics.
Files:
docs/cuprum-design.md
🔍 Remote MCP
Let me search the web for information about Cuprum's existing architecture and timeout handling patterns.Based on my search attempts, I was unable to locate detailed information about the Cuprum Python project directly. However, I can provide relevant context about timeout patterns in Python that are applicable to this PR's design.
Summary of Relevant Context for PR Review
Key Python Timeout Patterns and Precedents
The subprocess module raises TimeoutExpired if the timeout expires before the process exits, which directly aligns with the PR's stated behavior of raising a TimeoutExpired-like exception. The PR correctly models its design after Python's subprocess module timeout handling.
Process Termination on Timeout: A timeout may be specified in seconds and is internally passed to Popen.communicate(); if the timeout expires, the child process will be killed and waited for. This matches the PR's stated behavior of terminating the process and waiting a cancel grace period before killing if needed.
Critical Implementation Consideration: If the process doesn't terminate after timeout seconds, a TimeoutExpired exception is raised. Because the child process does not end if the timeout expires, to properly clean up you must explicitly end the child process and finish communication. This highlights an important implementation detail the reviewer should verify — that the PR's cleanup logic properly handles the grace period and forceful termination sequence.
Platform Limitations: The initial process creation itself cannot be interrupted on many platform APIs, so you are not guaranteed to see a timeout exception until at least after however long process creation takes. The reviewer should verify that the PR's documentation appropriately sets expectations about timeout accuracy.
Design Considerations for This PR
-
Timeout Precedence: The PR implements a clear resolution order (explicit timeout > ExecutionContext.timeout > scoped default), which provides flexibility while maintaining predictability.
-
Backward Compatibility: All new parameters default to
None, ensuring existing code remains unaffected — a sound design decision. -
Partial Output Capture: The PR specifies that captured partial output is included in the exception when
capture=True, which is important for debugging and distinguishes it from basic subprocess behavior. -
Pipeline-wide Timeouts: Applying timeouts to entire pipeline runs (covering all stages) is a higher-level abstraction than subprocess timeouts and simplifies the mental model for users orchestrating multi-stage operations.
-
Scoped Defaults via CuprumContext: Using context managers for scoped defaults (
with sh.scoped(timeout=...)) is a Pythonic pattern consistent with how frameworks like asyncio handle configuration.
⏰ 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 (3)
docs/cuprum-design.md (3)
273-274: LGTM: Terminology clarified.The shift from "misc flags" to "runtime defaults" with a concrete example improves clarity and aligns with the scoped timeout feature introduced in this PR.
350-365: LGTM: Signatures extended with timeout and context parameters.The API additions preserve backward compatibility via
Nonedefaults and maintain parity between async and sync variants.
384-399: LGTM: Pipeline signatures align with SafeCmd.The uniform API extension across command and pipeline types ensures consistency and predictability for users.
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: Excess Number of Function Argumentscuprum/context.py: CuprumContext.narrow What lead to degradation?CuprumContext.narrow has 5 arguments, max arguments = 4 Why does this problem occur?Functions with many arguments indicate either a) low cohesion where the function has too many responsibilities, or b) a missing abstraction that encapsulates those arguments. How to fix it?Start by investigating the responsibilities of the function. Make sure it doesn't do too many things, in which case it should be split into smaller and more cohesive functions. Consider the refactoring INTRODUCE PARAMETER OBJECT to encapsulate arguments that refer to the same logical concept. |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Excess Number of Function Argumentscuprum/context.py: scoped What lead to degradation?scoped has 5 arguments, max arguments = 4 Why does this problem occur?Functions with many arguments indicate either a) low cohesion where the function has too many responsibilities, or b) a missing abstraction that encapsulates those arguments. How to fix it?Start by investigating the responsibilities of the function. Make sure it doesn't do too many things, in which case it should be split into smaller and more cohesive functions. Consider the refactoring INTRODUCE PARAMETER OBJECT to encapsulate arguments that refer to the same logical concept. |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cuprum/unittests/test_context.py (1)
86-124: LGTM! Add a test for timeout narrowing viaScopeConfig.The updated tests correctly use
ScopeConfigfor allowlist and hook narrowing. However, there is no test verifying thatnarrow()correctly handles thetimeoutfield inScopeConfig. Add a test to ensure inner scope timeouts override outer scope timeouts.Suggested test
def test_narrow_overrides_timeout() -> None: """narrow() uses config.timeout when not None, otherwise preserves parent.""" parent = CuprumContext(timeout=5.0) # Config with timeout overrides parent narrowed = parent.narrow(ScopeConfig(timeout=2.0)) assert narrowed.timeout == 2.0 # Config with timeout=None preserves parent unchanged = parent.narrow(ScopeConfig()) assert unchanged.timeout == 5.0
🤖 Fix all issues with AI agents
In `@cuprum/context.py`:
- Around line 242-248: The `# noqa: PLR0913` on the scoped function needs a
justification comment; update the scoped definition to replace the bare noqa
with a short inline comment starting with "FIXME:" or a ticket/issue URL that
explains the suppression is intentional because the public API for scoped (the
function signature with five parameters: allowlist, before_hooks, after_hooks,
observe_hooks, timeout) is preserved for backward compatibility; reference the
function name scoped and the PLR0913 rule in the comment so future maintainers
know this is deliberate and link to the relevant compatibility ticket or note.
In `@cuprum/sh.py`:
- Around line 204-215: The _resolve_timeout function is implemented but never
used; update SafeCmd.run, SafeCmd.run_sync, Pipeline.run, and Pipeline.run_sync
to call _resolve_timeout with the explicit timeout arg and the current
ExecutionContext (or None) and pass the resulting timeout value into the
subprocess execution path (i.e., into _execute_subprocess or whatever helper
that accepts a timeout), and ensure any intermediate callers that invoke
_execute_subprocess propagate this resolved timeout parameter; if you prefer to
defer integration, add a clear TODO comment in those methods documenting that
timeout resolution via _resolve_timeout must be wired into the execution flow in
a follow-up PR.
In `@cuprum/unittests/test_timeout_resolution.py`:
- Around line 1-13: Add a parametrized test for _resolve_timeout that covers
precedence: explicit timeout > ExecutionContext(timeout=...) >
sh.scoped(timeout=...), plus the all-None case; use pytest.mark.parametrize with
cases [(5.0, 3.0, 1.0, 5.0), (None, 3.0, 1.0, 3.0), (None, None, 1.0, 1.0),
(None, None, None, None)], wrap each case in with
sh.scoped(timeout=scoped_timeout), create ctx =
ExecutionContext(timeout=ctx_timeout) when ctx_timeout is not None else
ExecutionContext(), call result = _resolve_timeout(timeout=explicit,
context=ctx) and assert result == expected so explicit wins, otherwise context
wins, otherwise scoped wins, and all None yields None.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
cuprum/_testing.pycuprum/context.pycuprum/sh.pycuprum/unittests/test_context.pycuprum/unittests/test_timeout_resolution.py
🧰 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_timeout_resolution.pycuprum/context.pycuprum/sh.pycuprum/unittests/test_context.pycuprum/_testing.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_timeout_resolution.pycuprum/context.pycuprum/sh.pycuprum/unittests/test_context.pycuprum/_testing.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_timeout_resolution.pycuprum/unittests/test_context.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_timeout_resolution.pycuprum/unittests/test_context.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_timeout_resolution.pycuprum/unittests/test_context.pycuprum/_testing.py
🧬 Code graph analysis (5)
cuprum/unittests/test_timeout_resolution.py (2)
cuprum/sh.py (2)
ExecutionContext(167-201)_resolve_timeout(204-214)cuprum/context.py (1)
scoped(242-283)
cuprum/context.py (1)
cuprum/catalogue.py (1)
allowlist(67-69)
cuprum/sh.py (1)
cuprum/context.py (3)
current_context(198-200)observe(472-487)scoped(242-283)
cuprum/unittests/test_context.py (1)
cuprum/context.py (3)
ScopeConfig(43-50)narrow(101-143)CuprumContext(54-186)
cuprum/_testing.py (1)
cuprum/sh.py (1)
_resolve_timeout(204-214)
🔍 Remote MCP
Let me search for the Cuprum repository directly using web search to find the GitHub project:The web search didn't find the Cuprum repository directly. Based on the PR context, it appears to be a private or less well-known project. Let me search more specifically for information about the Cuprum shell/command execution library by looking at typical Python subprocess.TimeoutExpired exception design to understand best practices for the implementation:Based on the search results and the PR context provided, I now have enough information to synthesize relevant facts for the PR review. Let me compile the findings.
Additional Context for PR #22 Review
Key Design Pattern: Standard subprocess.TimeoutExpired Exception
The Python standard library's TimeoutExpired exception includes attributes: cmd, output, stdout, stderr, and timeout. This is directly relevant to the PR's stated design of raising "a TimeoutExpired-like exception containing .cmd (argv or pipeline description), .timeout, and .stdout/.stderr (or None when capture=False)."
Review relevance: The PR claims to follow subprocess semantics. Confirm that Cuprum's timeout exception implementation matches the standard attributes and maintains consistency with Python's expected behavior.
Timeout Lifecycle & Process Termination
When timeout expires, the child process will be killed and waited for, with the TimeoutExpired exception re-raised after the child process has terminated. The PR summary mentions that "Cuprum terminates the process, waits a cancel grace period, then kills if needed."
Review relevance: Verify that the implementation's "cancel grace period" is properly documented and that the process cleanup sequence (SIGTERM → grace period → SIGKILL) is correctly implemented before the exception is raised.
Timeout Precedence & Resolution Order
The PR documentation states the resolution order as: (1) explicit timeout, (2) ExecutionContext.timeout, (3) CuprumContext scoped default. This follows the principle of explicit > contextual > default, which is a standard Python pattern.
Review relevance: Check that _resolve_timeout() in cuprum/sh.py correctly implements this precedence hierarchy and has test coverage for boundary cases (e.g., when context.timeout is None, fallback to scoped default).
Partial Output Handling on Timeout
When TimeoutExpired is raised, output is not lost, and catching the exception and retrying communication will not lose any output. The PR states it surfaces partial output for pipelines "according to capture and pipeline rules."
Review relevance: Verify that partial stdout/stderr is correctly captured and included in the exception even when a timeout interrupts the process mid-execution. Examine test_timeout_resolution.py and any pipeline timeout tests to ensure this behavior is tested.
Backward Compatibility
Python 3.5 added the timeout parameter to subprocess.run, which is passed to Popen.communicate(). The PR adds timeout parameters to SafeCmd.run/run_sync and Pipeline.run/run_sync with defaults of None.
Review relevance: All timeout parameters default to None, preserving backward compatibility. Verify that no existing code paths break when timeout=None and no ExecutionContext is provided.
ScopeConfig Refactoring
The PR introduces a ScopeConfig dataclass to encapsulate five context narrowing parameters (allowlist, before_hooks, after_hooks, observe_hooks, timeout). This addresses a code-quality concern about functions with >4 parameters.
Review relevance:
- Verify ScopeConfig is properly immutable (frozen dataclass)
- Confirm that all callers of CuprumContext.narrow() have been updated to use ScopeConfig
- Check that scoped() maintains backward compatibility by accepting timeout as a parameter and constructing ScopeConfig internally
- Ensure tests in test_context.py properly validate the new ScopeConfig-based API
Packaging Metadata Addition
The PR introduces cuprum/_meta.py with PACKAGE_NAME constant and re-exports it from cuprum/init.py. This is a low-risk change for future version management.
Review relevance: Verify that PACKAGE_NAME is not used elsewhere in the codebase prematurely, and that the constant is accessible via cuprum.PACKAGE_NAME as documented.
⏰ 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)
cuprum/_testing.py (1)
33-33: LGTM!The import and re-export of
_resolve_timeoutfollows the established pattern for test-only internal helpers. This correctly exposes the timeout resolution function for unit tests without polluting the public API.Also applies to: 51-51
cuprum/context.py (5)
42-51: LGTM! ScopeConfig addresses the excess arguments concern.The frozen dataclass correctly encapsulates the five configuration parameters (allowlist, before_hooks, after_hooks, observe_hooks, timeout) into a cohesive Parameter Object. This is the recommended fix for the static analysis warnings about excess function arguments.
67-77: LGTM!The
timeoutfield addition follows the existing dataclass pattern with proper type annotation and default value.
101-143: LGTM! Refactor successfully reduces argument count.The
narrow()method now accepts a singleScopeConfigparameter, addressing the static analysis warning. The timeout precedence logic correctly allows inner scopes to override outer timeouts when explicitly specified.
223-226: LGTM!The
_ScopedContext.__init__refactor correctly reduces the argument count by accepting a singleScopeConfigand delegating tonarrow().
498-498: LGTM!
ScopeConfigis correctly added to the public exports in alphabetical order.cuprum/sh.py (3)
41-43: LGTM!The imports correctly alias
current_contextwith an underscore prefix to indicate internal use, and re-exportscopedfor convenient access fromcuprum.sh.
178-179: LGTM!The
timeoutfield onExecutionContextis correctly typed and documented.Also applies to: 196-196
576-576: LGTM!Re-exporting
scopedfromcuprum.shprovides a convenient single-module import path for users.cuprum/unittests/test_context.py (1)
18-18: LGTM!
ScopeConfigimport added correctly to support the refactorednarrow()API.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 917
The current implementation maintains the five-parameter signature for backward compatibility. The function is a thin public API wrapper that immediately constructs a OptionsYou have three paths forward: Option 1: Accept the suppression (current state) Option 2: Breaking API change def scoped(config: ScopeConfig) -> _ScopedContext:
"""Create a scoped context manager for narrowed execution."""
return _ScopedContext(config)Callers would need to update from: with scoped(allowlist=..., timeout=5.0):to: with scoped(ScopeConfig(allowlist=..., timeout=5.0)):Option 3: Deprecation path RecommendationIf the Would you like me to generate a prompt for Option 2 or Option 3? |
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: 4
🤖 Fix all issues with AI agents
In `@cuprum/context.py`:
- Around line 273-276: The docstring describing detach() in cuprum/context.py
splits the phrase "regardless of subsequent context modifications" across lines,
hurting readability; rewrap that sentence so it reads "regardless of subsequent
context modifications." on one line (or otherwise keep the phrase intact) within
the detach() documentation to avoid the awkward line break.
In `@cuprum/unittests/test_safe_cmd_run.py`:
- Around line 193-207: The test docstring for
test_timeout_raises_timeout_expired is inaccurate: it says "with captured output
when enabled" but the test calls execute(..., {"timeout": 0.1, "capture":
False}) and asserts stdout/stderr are None; update the docstring to reflect that
this test verifies TimeoutExpired behavior when capture is disabled (e.g.
"Timeouts raise TimeoutExpired with no captured output when capture is
disabled"), or alternatively add a new complementary test that sets capture=True
and asserts captured stdout/stderr are populated.
In `@docs/cuprum-design.md`:
- Line 639: Update the example to use the actual ScopeConfig field names:
replace the incorrect allow={GIT} with allowlist=(GIT,) and rename
before=[audit_hook] and after=[metrics_hook] to before_hooks=(audit_hook,) and
after_hooks=(metrics_hook,) so the call to sh.scoped(ScopeConfig(...)) matches
the ScopeConfig API (use tuples for the hook and allowlist values, not lists).
In `@tests/behaviour/test_structured_events.py`:
- Line 11: Replace the private module import with the public re-export: change
the import of ScopeConfig and scoped to import them from the top-level cuprum
package (use "from cuprum import ScopeConfig, scoped") so tests use the public
API surface consistently with other tests like test_stream_fidelity.py; update
the import statement that currently references cuprum.context accordingly.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (26)
cuprum/__init__.pycuprum/_pipeline_internals.pycuprum/adapters/__init__.pycuprum/adapters/logging_adapter.pycuprum/adapters/metrics_adapter.pycuprum/adapters/tracing_adapter.pycuprum/context.pycuprum/sh.pycuprum/unittests/test_adapters.pycuprum/unittests/test_context.pycuprum/unittests/test_logging_hook.pycuprum/unittests/test_observe.pycuprum/unittests/test_pipeline.pycuprum/unittests/test_safe_cmd_run.pycuprum/unittests/test_timeout_resolution.pydocs/cuprum-design.mddocs/roadmap.mddocs/users-guide.mdtests/behaviour/test_context_hooks.pytests/behaviour/test_execution_runtime.pytests/behaviour/test_logging_hook_behaviour.pytests/behaviour/test_pipeline_execution.pytests/behaviour/test_stream_fidelity.pytests/behaviour/test_structured_events.pytests/behaviour/test_telemetry_adapters.pytests/features/execution_runtime.feature
🧰 Additional context used
📓 Path-based instructions (11)
**/*.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_adapters.pycuprum/unittests/test_context.pycuprum/_pipeline_internals.pytests/behaviour/test_context_hooks.pycuprum/unittests/test_safe_cmd_run.pycuprum/adapters/metrics_adapter.pycuprum/unittests/test_logging_hook.pycuprum/unittests/test_timeout_resolution.pytests/behaviour/test_execution_runtime.pycuprum/unittests/test_pipeline.pycuprum/__init__.pycuprum/unittests/test_observe.pycuprum/adapters/__init__.pycuprum/adapters/tracing_adapter.pytests/behaviour/test_pipeline_execution.pytests/behaviour/test_telemetry_adapters.pytests/behaviour/test_logging_hook_behaviour.pycuprum/adapters/logging_adapter.pytests/behaviour/test_stream_fidelity.pycuprum/context.pycuprum/sh.pytests/behaviour/test_structured_events.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_adapters.pycuprum/unittests/test_context.pycuprum/_pipeline_internals.pytests/behaviour/test_context_hooks.pycuprum/unittests/test_safe_cmd_run.pycuprum/adapters/metrics_adapter.pycuprum/unittests/test_logging_hook.pycuprum/unittests/test_timeout_resolution.pytests/behaviour/test_execution_runtime.pycuprum/unittests/test_pipeline.pycuprum/__init__.pycuprum/unittests/test_observe.pycuprum/adapters/__init__.pycuprum/adapters/tracing_adapter.pytests/behaviour/test_pipeline_execution.pytests/behaviour/test_telemetry_adapters.pytests/behaviour/test_logging_hook_behaviour.pycuprum/adapters/logging_adapter.pytests/behaviour/test_stream_fidelity.pycuprum/context.pycuprum/sh.pytests/behaviour/test_structured_events.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_adapters.pycuprum/unittests/test_context.pycuprum/unittests/test_safe_cmd_run.pycuprum/unittests/test_logging_hook.pycuprum/unittests/test_timeout_resolution.pycuprum/unittests/test_pipeline.pycuprum/unittests/test_observe.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_adapters.pycuprum/unittests/test_context.pytests/behaviour/test_context_hooks.pycuprum/unittests/test_safe_cmd_run.pycuprum/unittests/test_logging_hook.pycuprum/unittests/test_timeout_resolution.pytests/behaviour/test_execution_runtime.pycuprum/unittests/test_pipeline.pycuprum/unittests/test_observe.pytests/behaviour/test_pipeline_execution.pytests/behaviour/test_telemetry_adapters.pytests/behaviour/test_logging_hook_behaviour.pytests/behaviour/test_stream_fidelity.pytests/behaviour/test_structured_events.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_adapters.pycuprum/unittests/test_context.pytests/behaviour/test_context_hooks.pycuprum/unittests/test_safe_cmd_run.pycuprum/unittests/test_logging_hook.pycuprum/unittests/test_timeout_resolution.pytests/behaviour/test_execution_runtime.pycuprum/unittests/test_pipeline.pycuprum/unittests/test_observe.pytests/behaviour/test_pipeline_execution.pytests/behaviour/test_telemetry_adapters.pytests/behaviour/test_logging_hook_behaviour.pytests/behaviour/test_stream_fidelity.pytests/behaviour/test_structured_events.py
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.Add troubleshooting section for common issues: missing wheel on exotic platforms, forced fallback behaviour, and benchmark result interpretation.
Files:
docs/cuprum-design.mddocs/users-guide.mddocs/roadmap.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/cuprum-design.mddocs/users-guide.mddocs/roadmap.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/cuprum-design.mddocs/users-guide.mddocs/roadmap.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/cuprum-design.mddocs/users-guide.mddocs/roadmap.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/cuprum-design.mddocs/users-guide.mddocs/roadmap.md
docs/**/*.{md,mdx}
📄 CodeRabbit inference engine (docs/documentation-style-guide.md)
Follow markdownlint recommendations for Markdown formatting
Files:
docs/cuprum-design.mddocs/users-guide.mddocs/roadmap.md
docs/cuprum-design.md
📄 CodeRabbit inference engine (docs/roadmap.md)
Extend
docs/cuprum-design.mdwith Section 13 covering Rust extension architecture, API boundary, fallback strategy, and performance characteristics.
Files:
docs/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.Add performance guidance to
docs/users-guide.mdexplaining when to use each pathway, how to configure selection via environment variable, and expected throughput improvements.
Files:
docs/users-guide.md
🧬 Code graph analysis (16)
cuprum/unittests/test_adapters.py (1)
cuprum/context.py (3)
ScopeConfig(43-50)scoped(242-261)observe(452-467)
cuprum/unittests/test_context.py (1)
cuprum/context.py (5)
ScopeConfig(43-50)narrow(101-143)CuprumContext(54-186)scoped(242-261)current_context(198-200)
tests/behaviour/test_context_hooks.py (1)
cuprum/context.py (3)
ScopeConfig(43-50)scoped(242-261)current_context(198-200)
cuprum/unittests/test_safe_cmd_run.py (4)
cuprum/sh.py (5)
TimeoutExpired(204-224)SafeCmd(481-620)stdout(161-163)stdout(222-224)make(683-699)tests/helpers/catalogue.py (1)
python_builder(29-32)cuprum/context.py (3)
ForbiddenProgramError(38-39)ScopeConfig(43-50)scoped(242-261)cuprum/catalogue.py (1)
allowlist(67-69)
cuprum/unittests/test_logging_hook.py (3)
cuprum/context.py (3)
ScopeConfig(43-50)current_context(198-200)scoped(242-261)cuprum/catalogue.py (1)
allowlist(67-69)cuprum/logging_hooks.py (1)
logging_hook(58-90)
cuprum/unittests/test_timeout_resolution.py (2)
cuprum/sh.py (2)
ExecutionContext(167-201)_resolve_timeout(227-237)cuprum/context.py (2)
ScopeConfig(43-50)scoped(242-261)
tests/behaviour/test_execution_runtime.py (1)
cuprum/sh.py (6)
TimeoutExpired(204-224)ExecutionContext(167-201)run_sync(583-620)run_sync(664-680)run(503-581)run(646-662)
cuprum/unittests/test_pipeline.py (2)
cuprum/context.py (2)
ScopeConfig(43-50)scoped(242-261)cuprum/sh.py (3)
TimeoutExpired(204-224)run_sync(583-620)run_sync(664-680)
cuprum/__init__.py (2)
cuprum/context.py (1)
ScopeConfig(43-50)cuprum/sh.py (1)
TimeoutExpired(204-224)
cuprum/unittests/test_observe.py (1)
cuprum/context.py (4)
ScopeConfig(43-50)current_context(198-200)scoped(242-261)observe(452-467)
tests/behaviour/test_pipeline_execution.py (2)
cuprum/context.py (2)
ScopeConfig(43-50)scoped(242-261)cuprum/catalogue.py (1)
allowlist(67-69)
tests/behaviour/test_telemetry_adapters.py (3)
cuprum/context.py (3)
ScopeConfig(43-50)scoped(242-261)observe(452-467)cuprum/catalogue.py (1)
allowlist(67-69)cuprum/adapters/logging_adapter.py (1)
hook(112-119)
tests/behaviour/test_logging_hook_behaviour.py (1)
cuprum/context.py (2)
ScopeConfig(43-50)scoped(242-261)
tests/behaviour/test_stream_fidelity.py (2)
cuprum/context.py (2)
ScopeConfig(43-50)scoped(242-261)cuprum/catalogue.py (1)
allowlist(67-69)
cuprum/context.py (1)
cuprum/catalogue.py (1)
allowlist(67-69)
cuprum/sh.py (3)
cuprum/context.py (3)
current_context(198-200)observe(452-467)scoped(242-261)cuprum/_process_lifecycle.py (1)
_terminate_process(19-29)cuprum/_pipeline_internals.py (3)
_StageObservation(67-98)emit(75-98)_EventDetails(102-106)
🪛 LanguageTool
docs/cuprum-design.md
[style] ~280-~280: Consider using the typographical ellipsis character here instead.
Context: ...y. Contexts are nested: entering a new with sh.scoped(ScopeConfig(...)) creates a derived context, and exiti...
(ELLIPSIS)
docs/users-guide.md
[typographical] ~203-~203: The word ‘when’ starts a question. Add a question mark (“?”) at the end of the sentence.
Context: ...xt.timeoutwhen provided and notNone. - ScopeConfig(timeout=...)` default se...
(WRB_QUESTION_MARK)
[style] ~250-~250: Consider using the typographical ellipsis character here instead.
Context: ...f keyword parameters. Update calls like with scoped(allowlist=...) to `with scoped(ScopeConfig(allowlist...
(ELLIPSIS)
[style] ~251-~251: Consider using the typographical ellipsis character here instead.
Context: ...ls like with scoped(allowlist=...) to with scoped(ScopeConfig(allowlist=...)). When you call SafeCmd.run() or `...
(ELLIPSIS)
⏰ 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 (85)
cuprum/_pipeline_internals.py (4)
116-121: LGTM!Clean encapsulation of pipeline spawn state. The frozen dataclass with slots is appropriate for this immutable data structure.
124-148: LGTM!The timeout deadline computation and conditional
asyncio.wait_forwrapping are correctly implemented. Themax(0.0, ...)guard ensures no negative timeout values.
282-348: LGTM!The timeout integration into the pipeline execution flow is well-structured. Exception handling correctly distinguishes between
TimeoutExpired(where output has been collected) and other exceptions (where stream tasks need cancellation). The_PipelineSpawnResultencapsulation improves readability.
175-190: Verify task completion after TimeoutError.After
asyncio.wait_forraisesTimeoutError, the pipeline processes should be terminated, butstderr_tasksandstdout_taskmay still be awaiting I/O. If the underlying streams are not closed, lines 176–177 could block indefinitely.Ensure that
_wait_for_pipeline(or its timeout handling) closes the process streams so that these tasks complete promptly. Alternatively, apply a secondary timeout or cancel these tasks explicitly.cuprum/adapters/tracing_adapter.py (1)
15-27: LGTM!The example correctly demonstrates the new
ScopeConfig-based API pattern. The import and usage align with the updated public interface.cuprum/unittests/test_observe.py (2)
44-46: LGTM!Correct migration to the
ScopeConfig-based API for scoped contexts.
58-59: LGTM!Consistent application of the
ScopeConfigwrapper for allowlist configuration.tests/behaviour/test_telemetry_adapters.py (1)
171-174: LGTM!Consistent migration to the
ScopeConfig-based scoped context pattern, matching changes across the test suite.tests/features/execution_runtime.feature (1)
14-18: Verify step definitions exist for timeout scenarioThe new timeout scenario correctly reuses existing steps where appropriate and follows the established pattern. However, I cannot verify in the current environment whether the step definitions for "When I run the command with a timeout" and "Then a timeout error is raised" actually exist in the test step implementations. Confirm these steps are defined in the step definitions file before merging.
tests/behaviour/test_pipeline_execution.py (2)
11-11: LGTM!Import correctly includes
ScopeConfigfrom the top-levelcuprumpackage, consistent with the updated public API.
189-204: LGTM!Both synchronous and asynchronous pipeline execution paths correctly wrap the allowlist in
ScopeConfig, aligning with the updatedscoped()signature.cuprum/adapters/__init__.py (1)
15-41: LGTM!Example usage correctly demonstrates the updated
ScopeConfig-based scoping pattern. The examples are clear and consistent across all three telemetry adapter demonstrations.cuprum/adapters/metrics_adapter.py (1)
14-27: LGTM!Docstring example correctly updated to use
ScopeConfigfor the scoped context. The example remains clear and demonstrates proper integration with the metrics hook.cuprum/unittests/test_logging_hook.py (2)
9-9: LGTM!Import correctly sources
ScopeConfigfromcuprum.context, the module where it is defined.
22-22: LGTM!All
scoped()invocations consistently use theScopeConfigwrapper, maintaining uniform test setup across the suite.Also applies to: 46-46, 79-79, 97-97, 113-113
cuprum/unittests/test_adapters.py (4)
26-26: LGTM!Import correctly sources
ScopeConfigfromcuprum.context.
80-82: LGTM!
TestStructuredLoggingHooktests correctly applyScopeConfigwrapping with the catalogue's allowlist. The nested context managers for scoping and observing are properly structured.Also applies to: 102-104, 127-129
180-181: LGTM!
TestMetricsHooktests consistently apply theScopeConfigpattern across all test methods. Coverage includes counters, histograms, failures, and label verification.Also applies to: 203-204, 217-218, 232-233, 245-246, 295-296
346-347: LGTM!
TestTracingHooktests correctly useScopeConfigwrapping. The_run_traced_commandhelper method and individual test methods all follow the consistent pattern.Also applies to: 359-360, 407-408, 460-461, 474-475
cuprum/adapters/logging_adapter.py (1)
14-23: LGTM!The example correctly demonstrates the updated API pattern with
ScopeConfig. The context manager combination on lines 19–21 properly nestsscoped(ScopeConfig(...))withsh.observe(...).tests/behaviour/test_stream_fidelity.py (2)
11-11: LGTM!Import correctly updated to include
ScopeConfigfrom the publiccuprumnamespace.
102-103: LGTM!Test correctly updated to use
scoped(ScopeConfig(allowlist=allowlist))in line with the refactored API.tests/behaviour/test_structured_events.py (1)
66-67: LGTM!The scoped context manager correctly wraps the allowlist in
ScopeConfigand combines withsh.observe(hook).cuprum/unittests/test_context.py (5)
18-25: LGTM!Import correctly updated to include
ScopeConfigfromcuprum.context.
86-124: LGTM!Context narrowing tests correctly updated to pass
ScopeConfigtonarrow(). The tests properly verify:
- Allowlist intersection behaviour
- Widening prevention when parent is non-empty
- Base establishment when parent is empty
- Before-hook appending (FIFO)
- After-hook prepending (LIFO)
148-183: LGTM!Scoped context manager tests correctly updated to use
scoped(ScopeConfig(...)). Docstrings accurately reflect the new API pattern. Tests cover:
- Allowlist narrowing within block
- Context restoration after block exit
- Context restoration on exception
- Nested scope stacking
191-249: LGTM!Allow and hook registration tests correctly updated. Using
scoped(ScopeConfig())with an empty config to establish a clean scope for registration/detach testing is appropriate.
324-365: LGTM!Thread and async task isolation tests correctly updated to use
scoped(ScopeConfig(allowlist=programs)). These tests verify that context isolation works correctly across concurrent execution boundaries.docs/roadmap.md (1)
98-111: Verify whether the timeout implementation items (3.4.1–3.4.3) are complete.Check whether
SafeCmd.run/run_syncandPipeline.run/run_syncnow accepttimeoutparameters, whetherTimeoutExpiredexception is implemented, whetherScopeConfigsupports timeout scoping, and whether comprehensive tests and documentation have been added todocs/users-guide.md. Update the checkboxes from[ ]to[x]if these items are complete, or clarify any outstanding work if not.tests/behaviour/test_context_hooks.py (6)
13-19: LGTM!Import of
ScopeConfigcorrectly added alongside existing context imports. This aligns with the updated public API.
97-109: LGTM!Nested scoped contexts correctly demonstrate allowlist narrowing using the new
ScopeConfigAPI. The test properly captures the inner context for subsequent assertions.
126-137: LGTM!Context restoration test correctly migrated to
ScopeConfigAPI. The before/after capture pattern properly validates scope exit behaviour.
260-275: LGTM!Hook detachment test correctly uses an empty
ScopeConfig()to establish a scope. The registration/detach sequence is properly validated.
297-316: LGTM!Thread isolation test correctly migrated. Each worker thread establishes its own
ScopeConfig-based scope, properly demonstrating context isolation across threads.
344-365: LGTM!Async task isolation test correctly migrated. The
asyncio.sleep(0.01)interleaving andScopeConfigusage properly demonstrate context isolation across concurrent tasks.tests/behaviour/test_logging_hook_behaviour.py (2)
11-13: LGTM!Import statement correctly updated to include
ScopeConfigalongsidescoped.
43-55: LGTM!Logging hook test correctly uses
ScopeConfigfor allowlist configuration. The combined context manager usage is clear and concise.cuprum/__init__.py (4)
16-16: LGTM!Centralising
PACKAGE_NAMEincuprum._metais good practice for package metadata management.
30-46: LGTM!
ScopeConfigcorrectly added to the context module re-exports, providing convenient top-level access.
50-58: LGTM!
TimeoutExpiredcorrectly added to the sh module re-exports.
62-102: LGTM!
__all__correctly extended withScopeConfigandTimeoutExpired, maintaining alphabetical ordering.cuprum/unittests/test_safe_cmd_run.py (7)
15-16: LGTM!
TimeoutExpiredcorrectly imported from the top-levelcuprumpackage.
352-367: LGTM!Test correctly validates
ForbiddenProgramErrorwhen programme is not in allowlist. TheScopeConfigmigration is correct.
370-381: LGTM!Allowlist success test correctly migrated to
ScopeConfigAPI.
393-414: LGTM!Before hooks FIFO ordering test correctly uses
ScopeConfigwith bothallowlistandbefore_hooksfields.
417-441: LGTM!LIFO ordering test correctly uses nested
ScopeConfigscopes. The# noqa: SIM117suppression is justified as the nesting demonstrates the intentional hook ordering behaviour.
444-474: LGTM!Hook argument passing test correctly constructs
ScopeConfigwith bothbefore_hooksandafter_hooks. Multi-line formatting improves readability.
477-508: LGTM!Cancellation hook semantics test correctly migrated to
ScopeConfig. This is an important edge case ensuring after hooks are skipped on task cancellation.tests/behaviour/test_execution_runtime.py (6)
14-15: LGTM!
TimeoutExpiredcorrectly imported fromcuprumtop-level package.
48-54: LGTM!New timeout termination scenario correctly defined. Docstring accurately describes the behavioural coverage.
116-127: LGTM!Adding
cleanup_contexttracking enables differentiated failure messages in the shared cleanup assertion step.
149-154: LGTM!Cleanup assertion correctly retrieves context with a sensible default fallback. The context parameter improves failure message diagnostics.
157-163: LGTM!Timeout error assertion step correctly validates both the exception type and its
timeoutattribute value.
129-146: Verify PID file write timing relative to timeout window.The
_wait_for_pidcall at line 145 occurs after the command has timed out and been terminated. If the subprocess is killed before writing its PID file,_wait_for_pidwill raiseTimeoutError.Confirm that the worker script writes the PID file before entering the sleep loop, and that the 0.5 second timeout allows sufficient margin for process startup to complete the file write operation. Verify this assumption holds under typical CI load conditions.
cuprum/unittests/test_pipeline.py (4)
11-11: LGTM!Import statement correctly includes the new
ScopeConfigandTimeoutExpiredtypes required for the updated API surface.
176-177: LGTM!Correct migration to
ScopeConfig-basedscoped()call.
191-192: LGTM!Allowlist correctly includes both
ECHOandpython_programfor the pipeline under test.
204-222: Test logic is correct.The test properly validates
TimeoutExpiredsemantics for pipelines, including thetimeoutattribute andNoneoutput whencapture=False.The 0.2s timeout against a 5s sleep provides sufficient margin, though in exceptionally slow CI environments, process startup overhead could theoretically cause flakiness. The current margin is acceptable.
cuprum/unittests/test_timeout_resolution.py (3)
1-11: LGTM!Module setup is correct. The imports and docstring follow conventions, and exposing
_resolve_timeoutviacuprum._testingis appropriate for test access to internal resolution logic.
15-19: Test addresses roadmap item 3.4.3.This test validates the documented behaviour that
ExecutionContext(timeout=None)falls through to the scoped default rather than disabling timeouts.
22-43: Comprehensive precedence coverage.The parametrized test validates all timeout resolution levels as suggested in past reviews. The
_CONTEXT_OMITTEDsentinel elegantly distinguishes "context not provided" from "context with explicitNone".cuprum/context.py (6)
42-50: LGTM!
ScopeConfigdataclass is well-designed: frozen for immutability, slots for memory efficiency, and sensible defaults for all fields. This cleanly encapsulates the parameter object pattern recommended in PR comments.
67-77: LGTM!The
timeoutattribute is properly documented and defaults toNonefor backward compatibility.
101-143: LGTM!The refactored
narrow()method correctly:
- Preserves allowlist intersection/base semantics
- Maintains hook ordering (FIFO for before, LIFO for after)
- Propagates timeout with config value taking precedence over parent
223-226: LGTM!Clean delegation to
narrow(config)keeps the implementation DRY.
242-261: LGTM!The refactor to a single
ScopeConfigparameter elegantly resolves the "Excess Number of Function Arguments" static analysis warning, eliminating the need fornoqa: PLR0913suppression.
478-478: LGTM!
ScopeConfigcorrectly added to__all__exports, maintaining alphabetical ordering.docs/users-guide.md (4)
114-114: LGTM!Import statement updated to include
ScopeConfigfor the pipeline example.
192-221: LGTM!The Timeouts section comprehensively documents:
- Opt-in behaviour with
Nonedefault- Resolution precedence hierarchy
TimeoutExpiredexception handling- Pipeline-wide timeout semantics
The example code is clear and idiomatic.
249-251: LGTM!Clear upgrade note with before/after migration pattern. The
...notation in code examples is standard and appropriate despite the static analysis suggestion for typographical ellipsis.
268-283: LGTM!Scoped context examples consistently updated throughout the document to use the
ScopeConfigpattern.docs/cuprum-design.md (5)
273-282: LGTM!Terminology updated from "misc flags" to "runtime defaults" accurately reflects the
ScopeConfigtimeout semantics. Thescoped(ScopeConfig(...))reference aligns with the implementation.
350-365: LGTM!API signatures correctly document the new
timeoutandcontextparameters for bothrun()andrun_sync().
384-399: LGTM!Pipeline API signatures updated consistently with
SafeCmd, maintaining documentation parity.
953-1010: LGTM!Section 8.1.4 comprehensively documents:
- Default-off semantics with per-call and scoped defaults
- Resolution precedence with clear examples
TimeoutExpiredexception attributes- Pipeline timeout behaviour with partial output handling
Past review concerns (comma before "so", resolution clarifications) are addressed in this version.
280-282: Static analysis false positive.The
...inScopeConfig(...)is standard code notation, not prose requiring a typographical ellipsis character.cuprum/sh.py (9)
41-43: LGTM!The import aliasing is correct:
_current_contextuses the underscore prefix to signal internal usage, whilstobserveandscopeduse the explicitaspattern for re-export clarity.
204-224: LGTM!The
TimeoutExpiredexception correctly mirrors thesubprocess.TimeoutExpiredAPI, including thestdoutproperty alias foroutput. The N818 suppression is justified by the inline comment.
240-265: LGTM!The timeout-aware wait correctly uses
asyncio.wait_forwhen a timeout is configured. Exception handling properly terminates the process and awaits consumers before re-raising, preventing resource leaks.
278-294: LGTM!Clean internal exception type that preserves captured output and timing information for conversion to the public
TimeoutExpiredat the_execute_subprocessboundary.
374-393: LGTM!Solid defensive programming: the check at lines 383-385 guards against spurious
TimeoutErrorwithout a configured timeout. Awaiting consumers on the timeout path (line 382) ensures partial output is captured before raising the internal exception.
404-459: LGTM!The dual exception paths correctly differentiate between the no-capture (
TimeoutError) and capture (_SubprocessTimeoutError) branches. Exit events are emitted before raisingTimeoutExpired, maintaining observability even on timeout.
503-581: LGTM!Timeout resolution at line 538 correctly uses
_resolve_timeoutwith explicit timeout and context, then propagates the effective value through to_SubprocessExecution. The docstring accurately documentsTimeoutExpiredin the Raises section.
646-662: LGTM!Consistent timeout resolution pattern with
SafeCmd.run. The resolved timeout is passed to_run_pipeline, applying to the entire pipeline as documented.
702-714: LGTM!The
__all__exports correctly include the new public symbolsTimeoutExpiredandscoped.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Complex Methodcuprum/sh.py: _execute_subprocess What lead to degradation?_execute_subprocess has a cyclomatic complexity of 10, threshold = 9 Why does this problem occur?A Complex Method has a high cyclomatic complexity. The recommended threshold for the Python language is a cyclomatic complexity lower than 9. How to fix it?There are many reasons for Complex Method. Sometimes, another design approach is beneficial such as a) modeling state using an explicit state machine rather than conditionals, or b) using table lookup rather than long chains of logic. In other scenarios, the function can be split using EXTRACT FUNCTION. Just make sure you extract natural and cohesive functions. Complex Methods can also be addressed by identifying complex conditional expressions and then using the DECOMPOSE CONDITIONAL refactoring. Helpful refactoring examplesTo get a general understanding of what this code health issue looks like - and how it might be addressed - we have prepared some diffs for illustrative purposes. SAMPLE# complex_method.js
function postItem(item) {
if (!item.id) {
- if (item.x != null && item.y != null) {
- post(item);
- } else {
- throw Error("Item must have x and y");
- }
+ // extract a separate function for creating new item
+ postNew(item);
} else {
- if (item.x < 10 && item.y > 25) {
- put(item);
- } else {
- throw Error("Item must have an x and y value between 10 and 25");
- }
+ // and one for updating existing items
+ updateItem(item);
}
}
+
+function postNew(item) {
+ validateNew(item);
+ post(item);
+}
+
+function updateItem(item) {
+ validateUpdate(item);
+ put(item);
+}
+ |
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Large Methodcuprum/sh.py: _run_subprocess_with_streams What lead to degradation?_run_subprocess_with_streams has 77 lines, threshold = 70 Why does this problem occur?Overly long functions make the code harder to read. The recommended maximum function length for the Python language is 70 lines of code. Severity: Brain Method - Complex Method - Long Method. How to fix it?We recommend to be careful here -- just splitting long functions don't necessarily make the code easier to read. Instead, look for natural chunks inside the functions that expresses a specific task or concern. Often, such concerns are indicated by a Code Comment followed by an if-statement. Use the EXTRACT FUNCTION refactoring to encapsulate that concern. |
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Large Methodcuprum/sh.py: run What lead to degradation?run has 72 lines, threshold = 70 Why does this problem occur?Overly long functions make the code harder to read. The recommended maximum function length for the Python language is 70 lines of code. Severity: Brain Method - Complex Method - Long Method. How to fix it?We recommend to be careful here -- just splitting long functions don't necessarily make the code easier to read. Instead, look for natural chunks inside the functions that expresses a specific task or concern. Often, such concerns are indicated by a Code Comment followed by an if-statement. Use the EXTRACT FUNCTION refactoring to encapsulate that concern. |
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Complex Methodcuprum/_pipeline_internals.py: _collect_pipeline_inputs What lead to degradation?_collect_pipeline_inputs has a cyclomatic complexity of 10, threshold = 9 Why does this problem occur?A Complex Method has a high cyclomatic complexity. The recommended threshold for the Python language is a cyclomatic complexity lower than 9. How to fix it?There are many reasons for Complex Method. Sometimes, another design approach is beneficial such as a) modeling state using an explicit state machine rather than conditionals, or b) using table lookup rather than long chains of logic. In other scenarios, the function can be split using EXTRACT FUNCTION. Just make sure you extract natural and cohesive functions. Complex Methods can also be addressed by identifying complex conditional expressions and then using the DECOMPOSE CONDITIONAL refactoring. Helpful refactoring examplesTo get a general understanding of what this code health issue looks like - and how it might be addressed - we have prepared some diffs for illustrative purposes. SAMPLE# complex_method.js
function postItem(item) {
if (!item.id) {
- if (item.x != null && item.y != null) {
- post(item);
- } else {
- throw Error("Item must have x and y");
- }
+ // extract a separate function for creating new item
+ postNew(item);
} else {
- if (item.x < 10 && item.y > 25) {
- put(item);
- } else {
- throw Error("Item must have an x and y value between 10 and 25");
- }
+ // and one for updating existing items
+ updateItem(item);
}
}
+
+function postNew(item) {
+ validateNew(item);
+ post(item);
+}
+
+function updateItem(item) {
+ validateUpdate(item);
+ put(item);
+}
+ |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![terragon-labs[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
…objects - Introduced _PipelineOutputs, _TimeoutContext, _ExecutionTracking, and _SubprocessTimeoutContext dataclasses - Consolidated timeout exception raising logic into _handle_subprocess_timeout helper - Updated SafeCmd.run to use _ExecutionTracking for hooks and pending tasks management - Improved code clarity and immutability by using frozen dataclasses - Simplified _execute_subprocess timeout exception handling by delegating to _handle_subprocess_timeout - Refactored _build_timeout_expired_error to accept _PipelineOutputs instead of separate parameters This refactor improves structure and maintainability of subprocess timeout handling and pipeline internal behaviors. 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.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/users-guide.md (1)
723-754: Inconsistent API usage in concurrent execution examples.These examples still use the old keyword-based
scoped(allowlist=...)pattern rather than the newscoped(ScopeConfig(allowlist=...))pattern documented in the upgrade note at line 249. Update them for consistency.Proposed fix
-with scoped(allowlist=frozenset([ECHO])): +with scoped(ScopeConfig(allowlist=frozenset([ECHO]))):Apply this change at lines 728, 748, 774, 800, 814, 830, and 864.
🤖 Fix all issues with AI agents
In `@cuprum/sh.py`:
- Around line 411-444: Add brief justification comments for the PLR0913
suppressions on the functions _emit_exit_event and _raise_timeout_expired: next
to each "noqa: PLR0913" add a short inline comment explaining why multiple
parameters are necessary (e.g., parameter list reflects structured event fields
or preserves rich exception capture and chaining) and cite the rationale
(clarity/explicitness of required args) to satisfy lint-guideline requirements;
keep the comment concise and adjacent to the noqa token so it's clear why this
specific function is exempt.
- Around line 530-557: The function _prepare_execution_observation has a noqa:
PLR0913 suppression without justification; add a brief inline comment above the
function explaining why multiple parameters are appropriate (they represent
cohesive pieces of the execution observation: cmd, context, capture, echo,
execution_hooks, pending_tasks) and referencing the purpose of the function
(_prepare_execution_observation) so future reviewers know this is intentional
and not accidental complexity.
In `@docs/roadmap.md`:
- Around line 98-111: The roadmap precedence list is out of order: update the
timeout precedence so the scoped runtime default (ScopeConfig / sh.scoped)
appears before the CuprumContext default; specifically, state precedence as
explicit timeout > ExecutionContext.timeout > scoped default (ScopeConfig via
sh.scoped) > CuprumContext default, and ensure references to
SafeCmd.run/run_sync and Pipeline.run/run_sync mention this corrected chain and
the intended behaviour when ExecutionContext(timeout=None) falls through to the
scoped default.
In `@tests/behaviour/test_execution_runtime.py`:
- Around line 129-146: The pytest.raises usage in when_run_command_with_timeout
should assert the TimeoutExpired message with a regex; update the context
manager call to include a match parameter (e.g., pytest.raises(TimeoutExpired,
match=r"timed out|TimeoutExpired|timed out after .*s")) so the test verifies the
specific timeout error from command.run_sync; keep storing exc_info.value into
behaviour_state["timeout_error"] unchanged so downstream code still receives the
exception instance.
In `@tests/behaviour/test_pipeline_execution.py`:
- Line 11: The test file imports (ECHO, ScopeConfig, scoped, sh) were changed
but you must run the mandated validation commands (make test, make lint, make
check-fmt, make typecheck) locally and fix any failures they report; run the
four commands in sequence, address failing unit tests, lint errors, formatting
issues, or type errors in tests/behaviour/test_pipeline_execution.py and related
test changes (including the other modified test blocks noted), update imports or
test code to satisfy linters/formatters/typechecker and re-run the commands
until all pass.
♻️ Duplicate comments (10)
cuprum/unittests/test_safe_cmd_run.py (2)
193-206: Add a message match to the TimeoutExpired assertion.Assert the error message to avoid broad exception checks and keep the test aligned
withpytest.raisesguidance. Confirm the match text against the
TimeoutExpired message. As per coding guidelines.✏️ Proposed update
- with pytest.raises(TimeoutExpired) as exc_info: + with pytest.raises(TimeoutExpired, match=r"timed out") as exc_info:
434-438: Add a FIXME justification to the SIM117 suppression.Inline suppressions require a rationale next to the noqa. As per coding
guidelines.✏️ Proposed update
- with scoped(ScopeConfig(allowlist=frozenset([ECHO]), after_hooks=(outer_hook,))): # noqa: SIM117 + with scoped( + ScopeConfig(allowlist=frozenset([ECHO]), after_hooks=(outer_hook,)) + ): # noqa: SIM117 # FIXME: retain nested scope for explicit LIFO hook orderingcuprum/unittests/test_pipeline.py (1)
214-222: Add amatchpattern to tighten theTimeoutExpiredassertion.Per coding guidelines, use
pytest.raiseswith a regexmatchparameter for specific exception assertions. TheTimeoutExpiredmessage contains "timed out", so include a match pattern.Suggested fix
with ( scoped(ScopeConfig(allowlist=frozenset([python_program]))), - pytest.raises(TimeoutExpired) as exc_info, + pytest.raises(TimeoutExpired, match=r"timed out") as exc_info, ): pipeline.run_sync(timeout=0.2, capture=False)cuprum/context.py (2)
42-51: ExpandScopeConfigdocstring to NumPy style.Document the public attributes per the docstring guidelines. This is a public API type that should have full documentation of its fields.
Proposed fix
`@dc.dataclass`(frozen=True, slots=True) class ScopeConfig: - """Configuration object for scoped execution context updates.""" + """Configuration object for scoped execution context updates. + + Attributes + ---------- + allowlist: + Optional allowlist for the scope. When ``None``, inherit the current + allowlist. + before_hooks: + Hooks invoked before command execution (FIFO order). + after_hooks: + Hooks invoked after command execution (LIFO order). + observe_hooks: + Hooks invoked for structured execution events. + timeout: + Optional default timeout in seconds for calls within the scope. + + """
347-350: Minor formatting nit.The phrase "regardless of subsequent context modifications" is split awkwardly across lines 349-350. Rewrap for readability.
Proposed fix
even when used outside scoped(ScopeConfig()) blocks. This means detach() restores the exact context that existed when the registration was created, - regardless of - subsequent context modifications. + regardless of subsequent context modifications.docs/cuprum-design.md (2)
350-365: Update return type annotations in type sketch.The documented return types are
object, but the actual implementation returnsCommandResult. Even for a type sketch, use accurate return types to avoid confusion.Proposed fix
async def run( self, *, capture: bool = True, echo: bool = False, timeout: float | None = None, context: ExecutionContext | None = None, - ) -> object: ... + ) -> CommandResult: ... def run_sync( self, *, capture: bool = True, echo: bool = False, timeout: float | None = None, context: ExecutionContext | None = None, - ) -> object: ... + ) -> CommandResult: ...
639-639: Fixallowlisttype in the example.The
allowlistfield expectsfrozenset[Program] | None, but the example passes a tuple(GIT,). Usefrozenset([GIT])instead.Proposed fix
- async with sh.scoped(ScopeConfig(allowlist=(GIT,), before_hooks=(audit_hook,), after_hooks=(metrics_hook,))): + async with sh.scoped(ScopeConfig(allowlist=frozenset([GIT]), before_hooks=(audit_hook,), after_hooks=(metrics_hook,))):cuprum/_pipeline_internals.py (1)
159-177: Add justification comment fornoqa: PLR0913.The suppression lacks an accompanying explanation. Add a brief justification per coding guidelines.
Proposed fix
-def _build_timeout_expired_error( # noqa: PLR0913 +def _build_timeout_expired_error( # noqa: PLR0913 — cohesive timeout/output bundlecuprum/sh.py (2)
391-401: Add invariant comment for defensive check.The
if timeout is Noneguard is defensive programming against impossible states. Add a brief comment documenting the invariant.Proposed fix
except TimeoutError as exc: stdout_text, stderr_text = await asyncio.gather(*consumers) if timeout is None: + # Invariant: TimeoutError only raised when timeout is configured msg = "TimeoutError without a configured timeout" raise RuntimeError(msg) from exc
251-259: Add clarifying comment forTimeoutErrorcatch.The
except TimeoutErroris correct (in Python 3.11+,asyncio.TimeoutErroris an alias forTimeoutError), but a brief comment improves maintainability.Proposed fix
- except TimeoutError: + except TimeoutError: # Raised by asyncio.wait_for on timeout expiry await _terminate_process(process, ctx.cancel_grace)
|
@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 +567 to +589 def _prepare_execution_observation( # noqa: PLR0913
cmd: SafeCmd,
context: ExecutionContext,
tracking: _ExecutionTracking,
*,
capture: bool,
echo: bool,
) -> _StageObservation:
"""Prepare the observation context for command execution."""
cwd = Path(context.cwd) if context.cwd is not None else None
env_overlay = _freeze_str_mapping(context.env)
tags = _merge_tags(
{"project": cmd.project.name, "capture": capture, "echo": echo},
context.tags,
)
return _StageObservation(
cmd=cmd,
hooks=tracking.execution_hooks,
cwd=cwd,
env_overlay=env_overlay,
tags=tags,
pending_tasks=tracking.pending_tasks,
)❌ New issue: Excess Number of Function Arguments |
|
@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 file from cuprum._pipeline_internals import (
_MIN_PIPELINE_STAGES,
_EventDetails,
_ExecutionHooks,❌ New issue: Low Cohesion |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
…ted module - Move subprocess execution logic from cuprum/sh.py to new module cuprum/_subprocess_execution.py - Encapsulate subprocess spawning, stream handling, and timeout management - Simplify cuprum/sh.py by importing execution functions from the new module - Improve code organization and reduce circular imports - Adjust related context and observation handling to use new subprocess execution flow - Update tests and docs for consistent usage and error messages 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/users-guide.md (2)
774-776: Update to useScopeConfigfor consistency.The concurrent execution examples still use the old
scoped(allowlist=...)syntax. Update to match the newScopeConfigpattern documented in the upgrade note.✏️ Proposed fix
-with scoped(allowlist=frozenset([ECHO])): +with scoped(ScopeConfig(allowlist=frozenset([ECHO]))): result = run_concurrent_sync(*commands, config=config)Apply the same pattern to lines 800-802, 814, 830, and 864.
762-762: AddScopeConfigto the import statement.The
ConcurrentConfigexample importsscopedbut omitsScopeConfig, which is required for the corrected syntax.✏️ Proposed fix
-from cuprum import ECHO, ConcurrentConfig, run_concurrent_sync, scoped, sh +from cuprum import ECHO, ConcurrentConfig, run_concurrent_sync, ScopeConfig, scoped, sh
🤖 Fix all issues with AI agents
In `@cuprum/context.py`:
- Around line 62-66: The Context.timeout field currently allows negative floats;
add validation in the Context class (implement __post_init__ or a timeout
property setter) to raise a ValueError when timeout is not None and timeout < 0,
and ensure non-numeric values are rejected or coerced (e.g., accept ints by
casting to float). Update Context.__post_init__ (or the timeout setter) to
perform this check and raise a clear error message referencing the invalid
timeout value.
In `@docs/roadmap.md`:
- Around line 98-112: Update the roadmap checklist to mark the implemented
timeout items as completed by changing the unchecked boxes to checked: switch "-
[ ] 3.4.1. Add `timeout` parameters to `SafeCmd.run` / `run_sync` and
`Pipeline.run` / `run_sync`..." to "- [x]"; change "- [ ] 3.4.2. Introduce
scoped runtime defaults via `CuprumContext`..." to "- [x]"; and change "- [ ]
3.4.3. Add unit and behavioural tests..." to "- [x]". Ensure the text still
references the relevant symbols (`SafeCmd.run`, `run_sync`, `Pipeline.run`,
`Pipeline.run_sync`, `CuprumContext`, `ExecutionContext.timeout`,
`ScopeConfig.timeout`, and `sh.scoped`) but only update the checkbox markers to
reflect completion.
- Introduce a _validate_timeout function to validate and convert timeout values. - Apply validation in the __post_init__ of ScopeConfig and CuprumContext to ensure timeouts are non-negative and converted to floats. - Add comprehensive unit tests covering valid and invalid timeouts for both classes. - Update roadmap to mark timeout-related tasks as completed. 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: 1
🤖 Fix all issues with AI agents
In `@cuprum/unittests/test_context.py`:
- Around line 392-468: The six nearly-identical timeout tests for ScopeConfig
and for CuprumContext should be collapsed into two parametrised pytest functions
per class (e.g., test_scope_config_timeout_validation and
test_cuprum_context_timeout_validation) using pytest.mark.parametrize over cases
(None, 0.0, 5.0, 5, -1.0, -5) with expected results or expected exceptions;
remove the duplicated test functions (test_scope_config_* and
test_cuprum_context_*). For the int cases use typing.cast("float", ...) when
constructing ScopeConfig(timeout=...) and CuprumContext(timeout=...) instead of
the unannotated "# type: ignore[arg-type]" comments to keep strict typing, and
assert coercion to float and the proper ValueError messages for negative inputs
using pytest.raises where appropriate.
…d tests Replaced multiple individual tests validating timeout values for ScopeConfig and CuprumContext with parameterized tests. This improves test maintainability by reducing duplication and covers cases including None, zero, positive float, positive int (coerced to float), and negative values raising errors. 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: 1
🤖 Fix all issues with AI agents
In `@cuprum/unittests/test_context.py`:
- Around line 392-463: Extract the duplicated parametrize data into a
module-level constant named _TIMEOUT_VALIDATION_CASES and reference it in both
parametrize decorators; specifically, create _TIMEOUT_VALIDATION_CASES
(containing the six pytest.param entries) before the timeout validation tests,
then replace the duplicated lists in the decorators for
test_scope_config_timeout_validation and test_cuprum_context_timeout_validation
with _TIMEOUT_VALIDATION_CASES so both functions use the same constant.
…parameter Extracts repeated timeout validation parameters into a shared list constant `_TIMEOUT_VALIDATION_CASES` used by multiple timeout-related tests to reduce duplication and improve maintainability. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
The drain runs while a failure is already propagating, so it can neither raise what it finds nor report it: a reader that broke decodes to the same empty string as one that simply had nothing to say. Nothing recorded the difference, and `gather(..., return_exceptions=True)` has swallowed reader exceptions unexamined since #22 — long before this branch narrowed the gap by mapping a capturing failure to `""` rather than `None`. Record two things at DEBUG, in the shape `_log_suppressed_stream_close_error` established: `stream_consumer_failed` for a settled reader whose result is an exception other than a plain `CancelledError`, and `capture_eof_grace_expired` counting the readers still parked when the window closed. Cancellation stays unrecorded — every teardown cancels something, so recording it would make the record routine enough to ignore. Neither is a metric or a trace event; that belongs with the `ExecEvent` contract in #285 and #286, not with a teardown helper. The grace window moves into `_await_eof_grace` so the wait and its diagnosis sit together, and `_drain` sheds its per-chunk echo and callback branches to `_tee_chunk`, keeping it inside the complexity limit now that it handles cancellation too.
1 participant
Summary
Changes
API
timeout: float | None = Noneandcontext: ExecutionContext | None = None.timeout: float | None = Noneandcontext: ExecutionContext | None = None.timeout: float | None = Noneandcontext: ExecutionContext | None = None.timeout: float | None = Noneandcontext: ExecutionContext | None = None.scoped(...)(e.g.,with scoped(ScopeConfig(timeout=...))). The old keyword-argument form is superseded by ScopeConfig usage in code and tests.Behavior
TimeoutExpired-like exception with:.cmdcontaining the executed argv (or pipeline description),.timeoutcontaining the configured timeout value,.stdout/.stderrcarrying captured output (orNonewhencapture=False).Scoped defaults
with scoped(ScopeConfig(timeout=...))to apply a default for calls lacking an explicit timeout.timeouton the call, 2)ExecutionContext.timeoutwhen a context is provided and notNone, 3)ScopeConfig-based scoped default, 4) CuprumContext runtime default.Packaging
PACKAGE_NAMEconstant.PACKAGE_NAMEfrom cuprum._meta.Examples
Documentation
Tests
SafeCmd.run/run_sync.scopedusingScopeConfig.Packaging (internal)
_meta.pywithPACKAGE_NAME = "cuprum"and wired into__init__viafrom cuprum._meta import PACKAGE_NAME.Rationale
subprocess.runsemantics while enabling policy-based timeout management via scoped defaults. ScopeConfig allows centralized timeout policy without mutating global state.Backwards compatibility
None; existing call sites remain unaffected. The new scoping API usesScopeConfigwithscoped(...).How to verify locally
cmd.run(timeout=5.0)andcmd.run_sync(timeout=2.5).cmd,timeout, and capturedstdout/stderrwhen applicable.Generated by Terry (Terragon Labs)
ℹ️ Tag @terragon-labs to ask questions and address PR feedback
📎 Task: https://www.terragonlabs.com/task/0dc36768-5c08-4259-94a3-4ee9487aba48
📎 Task: https://www.terragonlabs.com/task/a2d25a28-d3e9-4c0a-857c-f2d7c9903152