Make _get_stage_stream_fds the canonical pipeline stdio policy (#112) - #134
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
Summary
WalkthroughCentralise pipeline stdio selection through ChangesCanonical stdio policy and cwd refactor
Sequence Diagram(s)sequenceDiagram
participant Pipeline
participant ProcessLifecycle
participant StageStreams
participant Subprocess
Pipeline->>ProcessLifecycle: run pipeline stages
ProcessLifecycle->>StageStreams: get stage stream descriptors
StageStreams-->>ProcessLifecycle: return stdin/stdout/stderr policy
ProcessLifecycle->>Subprocess: spawn each stage with policy and cwd
Subprocess-->>Pipeline: return stage results and captured streams
Possibly related PRs
Suggested reviewers: Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning)
✅ Passed checks (18 passed)
📋 Issue PlannerBuilt with CodeRabbit's Coding Plans for faster development and fewer bugs. View plan used: ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. cuprum/unittests/test_tee_profile_worker_concurrency.py Comment on file """Structural type for the ``RLock`` operations this test instruments."""
def acquire(self, *, blocking: bool = True, timeout: float = -1) -> bool: ...
def acquire(self, *, blocking: bool = True, timeout: float = -1) -> bool:❌ New issue: Low Cohesion |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
87657ee to
9c490ea
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
9c490ea to
9f38b2e
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
9f38b2e to
2749fc3
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
2749fc3 to
6a7f716
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Reviewer's GuideRoutes pipeline spawning through a canonical helper for stdio and cwd handling, removes duplicated policy and dead re-exports, and adds exhaustive tests and documentation around the new canonical helpers while slightly adjusting tooling integration. Sequence diagram for canonical pipeline and single-command spawn helperssequenceDiagram
participant PipelineLifecycle as _spawn_pipeline_processes
participant SubprocessExecution as _spawn_subprocess
participant StageStreams as _get_stage_stream_fds
participant SubprocessContext as _cwd_arg
participant Asyncio as asyncio.create_subprocess_exec
rect rgb(235, 245, 255)
loop for each pipeline stage
PipelineLifecycle->>StageStreams: _get_stage_stream_fds(idx, last_idx, capture_or_echo)
StageStreams-->>PipelineLifecycle: _StageStreamConfig
PipelineLifecycle->>SubprocessContext: _cwd_arg(config.ctx.cwd)
SubprocessContext-->>PipelineLifecycle: cwd_arg
PipelineLifecycle->>Asyncio: create_subprocess_exec(..., stdin=stream_fds.stdin,
PipelineLifecycle-->>Asyncio: stdout=stream_fds.stdout, stderr=stream_fds.stderr, cwd=cwd_arg)
end
end
rect rgb(245, 245, 245)
SubprocessExecution->>SubprocessContext: _cwd_arg(execution.ctx.cwd)
SubprocessContext-->>SubprocessExecution: cwd_arg
SubprocessExecution->>Asyncio: create_subprocess_exec(..., cwd=cwd_arg)
end
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
6a7f716 to
8029b56
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Has this now been resolved in the latest commit? Use codegraph analysis to determine your answer. If this comment is now resolved, please mark it as such using the API. Otherwise, please provide an AI agent prompt for the remaining work to be done to address this comment. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== cuprum/_subprocess_context.py ==="
cat cuprum/_subprocess_context.py
echo ""
echo "=== cuprum/unittests/test_stage_stream_fds.py ==="
cat cuprum/unittests/test_stage_stream_fds.pyLength of output: 5863 🏁 Script executed: #!/bin/bash
# Find and resolve the review comment about test_intermediate_stage_always_pipes_stdout
gh api \
-H "Accept: application/vnd.github+json" \
/repos/leynos/cuprum/pulls/134/comments \
--paginate \
--jq '.[] | select(.user.login == "coderabbitai[bot]") | {id: .id, body: .body[0:120]}'Length of output: 545 🏁 Script executed: #!/bin/bash
# List all PR review comments without login filter
gh api \
-H "Accept: application/vnd.github+json" \
/repos/leynos/cuprum/pulls/134/comments \
--paginate \
--jq '.[] | {id: .id, user: .user.login, body: .body[0:150]}'Length of output: 490 Both issues are confirmed resolved in the latest commit. However, the review comment (ID 3402845314) was filed by Summary of resolution:
Dismiss the Sourcery comment manually in the GitHub UI, or ask a maintainer to do so. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
The `test_intermediate_stage_always_pipes_stdout` example exercised `_get_stage_stream_fds(0, 1, ...)`, which is the *first* stage of a two-stage pipeline, not an intermediate one. Target the middle stage of a three-stage pipeline (`idx=1, last_idx=2`) so the test covers the documented intermediate-stage behaviour its name claims. `_cwd_arg`'s docstring example called `Path(...)` at runtime while `Path` is a `TYPE_CHECKING`-only import (the `from __future__ import annotations` present in the module already prevents the annotation `NameError` the reviewer noted). Keeping `Path` type-only satisfies the `TC` ruff rule, so rework the doctest to use a string argument and note that `Path` inputs are covered exhaustively by `test_cwd_arg_conversion`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
`_cwd_arg` renders its argument with `str`, so a `Path` expectation must be
`str(Path(...))` rather than a hard-coded POSIX string: on Windows
`str(Path("/srv/data"))` is `\srv\data`, which the literal `"/srv/data"`
would never match. The plain-string rows pass through unchanged and stay
correct as-is.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
"Context helpers for subprocess execution" did not convey that the module hosts the canonical `_cwd_arg` working-directory conversion shared by the single-command and pipeline spawn paths. Describe each utility the module provides — `_cwd_arg`, `_resolve_timeout`, and the lazy-import shims — so the canonical helper's role is discoverable from the module summary. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Expand the module-level documentation around pipeline stdio ownership and replace the mock-based delegation test with an end-to-end pipeline execution test that exercises capture and discard modes.
d5ab9b1 to
d7f0737
Compare
![codescene-access[bot]](https://avatars.githubusercontent.com/in/3755875?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Exercise the supported `RunOutputOptions` API in the pipeline stdio integration test and clarify the distinct re-use scopes of `_cwd_arg` and `_get_stage_stream_fds`.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Include the canonical Oxford-spelling mappings emitted by the repository generator.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Make failures in pipeline stdio and stage-stream policy tests identify the active inputs and checked stage attribute.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/developers-guide.md`:
- Line 23: Update docs/developers-guide.md at lines 23-23 and 1077-1077 by
inserting the missing commas before “so”: change “They exist so” to “They exist,
so” and “must use it so” to “must use it, so”.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f396094c-2f91-4553-84dc-7b157499e7b6
📒 Files selected for processing (11)
cuprum/_pipeline_stage_streams.pycuprum/_pipeline_streams.pycuprum/_process_lifecycle.pycuprum/_subprocess_context.pycuprum/_subprocess_execution.pycuprum/unittests/__snapshots__/test_maturin_build.ambrcuprum/unittests/test_pipeline.pycuprum/unittests/test_stage_stream_fds.pydocs/developers-guide.mddocs/execplans/4-1-1-performance-extension-foundation.mdtypos.toml
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
leynos/shared-actions(auto-detected)leynos/pylint-pypy-shim(auto-detected)leynos/whitaker(auto-detected)
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/developers-guide.md`:
- Line 23: Update docs/developers-guide.md at lines 23-23 and 1077-1077 by
inserting the missing commas before “so”: change “They exist so” to “They exist,
so” and “must use it so” to “must use it, so”.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f396094c-2f91-4553-84dc-7b157499e7b6
📒 Files selected for processing (11)
cuprum/_pipeline_stage_streams.pycuprum/_pipeline_streams.pycuprum/_process_lifecycle.pycuprum/_subprocess_context.pycuprum/_subprocess_execution.pycuprum/unittests/__snapshots__/test_maturin_build.ambrcuprum/unittests/test_pipeline.pycuprum/unittests/test_stage_stream_fds.pydocs/developers-guide.mddocs/execplans/4-1-1-performance-extension-foundation.mdtypos.toml
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
leynos/shared-actions(auto-detected)leynos/pylint-pypy-shim(auto-detected)leynos/whitaker(auto-detected)
🛑 Comments failed to post (1)
docs/developers-guide.md (1)
23-23: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Insert the missing commas before “so”.
docs/developers-guide.md#L23-L23: ChangeThey exist sotoThey exist, so.docs/developers-guide.md#L1077-L1077: Changemust use it sotomust use it, so.Triage:
[type:grammar]As per path instructions, comments and documentation must follow en-GB-oxendict spelling and grammar.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~23-~23: Use a comma before ‘so’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...dently ofsh.makewrapping. They exist so the command construction contract can b...(COMMA_COMPOUND_SENTENCE_2)
📍 Affects 1 file
docs/developers-guide.md#L23-L23(this comment)docs/developers-guide.md#L1077-L1077🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/developers-guide.md` at line 23, Update docs/developers-guide.md at lines 23-23 and 1077-1077 by inserting the missing commas before “so”: change “They exist so” to “They exist, so” and “must use it so” to “must use it, so”.Sources: Path instructions, Linters/SAST tools
2 participants
Summary
This branch makes
_get_stage_stream_fdsthe canonical PIPE-versus-DEVNULL stdio policy for pipeline spawning and removes the inline duplication flagged by the refactoring audit.Closes #112.
Previously the stdio policy lived twice:
_spawn_pipeline_processesderived the stdin/stdout/stderr flags inline, while the purpose-built_get_stage_stream_fdshelper was dead code reachable only through a self-referential re-export. The branch routes the spawn path through the helper, extracts a shared_cwd_argconversion used by both the single-command and pipeline spawn sites, deletes the dead re-exports, adds exhaustive tests over the full finite input domain, and documents the canonical helpers in the developers' guide.Note: the branch is stacked on #133 (baseline gate repairs); the first commit belongs to that PR and this branch should be rebased once #133 merges.
Review walkthrough
_spawn_pipeline_processesrouted through the helper and_cwd_arg._cwd_arghelper; cuprum/_subprocess_execution.py consumes it on the single-command path.Validation
make check-fmt: passmake lint: passmake typecheck: passmake test: pass (579 passed, 43 skipped; Rust suite 4 passed)make markdownlint: passNotes
The wheel-build snapshot (
test_maturin_build.ambr) is refreshed because it records the package file list, which now includes the new test module. The developers' guide diff includes mdformat reflow of neighbouring sections produced bymake fmt.Summary by Sourcery
Canonicalize pipeline stdio and cwd handling across subprocess spawn paths and align tooling invocation with uv-based workflows.
Enhancements:
_get_stage_stream_fdsand document it as the single source of truth for PIPE-versus-DEVNULL selection._cwd_arghelper for consistent working-directory conversion used by both single-command and pipeline subprocess spawns.Build:
uv runand classify it as a venv tool rather than a global tool dependency.CI:
Tests:
_get_stage_stream_fdsand_cwd_arg, ensuring agreement with the single-command stdio policy and uniform cwd rendering.References