Apply assorted consistency and dead-code cleanups (#129) - #136
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
Covers the cleanup objectives tracked in issue WalkthroughThis PR relocates ChangesConsistency and pipeline cleanup
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 20✅ Passed checks (20 passed)
📋 Issue PlannerBuilt with CodeRabbit's Coding Plans for faster development and fewer bugs. View plan used: ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Reviewer's GuideTightens backend and pipeline infrastructure consistency by enforcing exhaustive stream backend selection, centralizing hook/observation types, simplifying FD extraction, and cleaning up dead code, imports, and minor docs/spelling issues while keeping behavior intact. File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
c84c2f5 to
3d755a0
Compare
3d755a0 to
b99d59b
Compare
b99d59b to
674527d
Compare
674527d to
c19f6f6
Compare
c19f6f6 to
662e147
Compare
662e147 to
6fd419b
Compare
6fd419b to
30711c2
Compare
30711c2 to
b7f7caf
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
b7f7caf to
fa0f02b
Compare
fa0f02b to
0815998
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@cuprum/unittests/test_pipeline_stream_backend_selection.py`:
- Around line 227-232: Update the test around _pump_stream_dispatch to use an
explicit writer object alongside the reader, and make the mocked
_extract_stream_fd return write_fd only for that writer and None for unknown
streams. Pass the writer to _pump_stream_dispatch so the Rust FD extraction path
is tested without incorrectly treating None as a valid writer; preserve or add a
separate assertion that writer=None uses the Python fallback.
In `@docs/execplans/4-1-1-performance-extension-foundation.md`:
- Around line 345-355: Remove one of the duplicated “Artefacts and Notes”
sections in the ExecPlan, preferably the newly added subsection around the
listed Rust, build-system, CI, and documentation artefacts. Retain a single
canonical artefact checklist with the complete expected entries.
🪄 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: 5537f3c8-a689-46dd-829d-6adaae28b6d7
📒 Files selected for processing (16)
cuprum/__init__.pycuprum/_backend.pycuprum/_pipeline_internals.pycuprum/_pipeline_spawn.pycuprum/_pipeline_stage_streams.pycuprum/_pipeline_streams.pycuprum/_pipeline_types.pycuprum/_process_lifecycle.pycuprum/_subprocess_execution.pycuprum/adapters/logging_adapter.pycuprum/context.pycuprum/logging_hooks.pycuprum/unittests/__snapshots__/test_maturin_build.ambrcuprum/unittests/test_backend.pycuprum/unittests/test_pipeline_stream_backend_selection.pydocs/execplans/4-1-1-performance-extension-foundation.md
🔗 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)
💤 Files with no reviewable changes (2)
- cuprum/_pipeline_spawn.py
- cuprum/unittests/snapshots/test_maturin_build.ambr
Exercise Rust stream dispatch with a real writer-shaped test double and retain explicit coverage for the writer-less Python fallback. Point type-only imports at their definition module and remove the duplicated ExecPlan artefact checklist. Refresh the trybuild diagnostic snapshot for the current Rust compiler.
Remove structural duplication introduced while replaying the pipeline cleanup over the CQRS refactor. Preserve main's command-query helpers and use the lifecycle module that replaced the deleted spawn shim. Drop `_run_before_hooks` because main's separated allowlist and hook collection operations supersede it.
Cover the supported top-level and `cuprum.events` exports while ensuring `cuprum.context` does not regain the removed re-export. Add user migration guidance and align the design and developer documents with the CQRS helper boundary inherited from main.
Use the diagnostic emitted by both pinned CI toolchains for the `is_available` non-const contract. Avoid blessing output from a newer floating compiler that CI does not use.
21fe1e5 to
980bdcf
Compare
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Record the authorization and hook-query ordering for pipeline stages. Keep shared pipeline types passive and dependency-safe, and prohibit reintroducing the superseded combined helper.
This comment was marked as resolved.
This comment was marked as resolved.
The FD-lifecycle module's scope note said it is consumed only by `cuprum/_pipeline_streams.py`. That holds for production code, but four `test_pipeline_stream*` unit modules import the seams directly to inject the partial failures the public entry point cannot provoke. Distinguish the two rather than leave a statement `rg` falsifies in one command. The 4.3.1 execplan's supersession note listed `_extract_stream_fd` among the helpers that moved, while the inventory it corrects names `_extract_reader_fd` and `_extract_writer_fd`, so the two sets never joined up. The record now says what happened: both were one-line pass-throughs to `_extract_stream_fd` from the outset, and #136 deleted them as dead code and inlined their call sites. Nothing was consolidated into `_extract_stream_fd`; it had been there all along.
The FD-lifecycle module's scope note said it is consumed only by `cuprum/_pipeline_streams.py`. That holds for production code, but four `test_pipeline_stream*` unit modules import the seams directly to inject the partial failures the public entry point cannot provoke. Distinguish the two rather than leave a statement `rg` falsifies in one command. The 4.3.1 execplan's supersession note listed `_extract_stream_fd` among the helpers that moved, while the inventory it corrects names `_extract_reader_fd` and `_extract_writer_fd`, so the two sets never joined up. The record now says what happened: both were one-line pass-throughs to `_extract_stream_fd` from the outset, and #136 deleted them as dead code and inlined their call sites. Nothing was consolidated into `_extract_stream_fd`; it had been there all along.
The FD-lifecycle module's scope note said it is consumed only by `cuprum/_pipeline_streams.py`. That holds for production code, but four `test_pipeline_stream*` unit modules import the seams directly to inject the partial failures the public entry point cannot provoke. Distinguish the two rather than leave a statement `rg` falsifies in one command. The 4.3.1 execplan's supersession note listed `_extract_stream_fd` among the helpers that moved, while the inventory it corrects names `_extract_reader_fd` and `_extract_writer_fd`, so the two sets never joined up. The record now says what happened: both were one-line pass-throughs to `_extract_stream_fd` from the outset, and #136 deleted them as dead code and inlined their call sites. Nothing was consolidated into `_extract_stream_fd`; it had been there all along.
The FD-lifecycle module's scope note said it is consumed only by `cuprum/_pipeline_streams.py`. That holds for production code, but four `test_pipeline_stream*` unit modules import the seams directly to inject the partial failures the public entry point cannot provoke. Distinguish the two rather than leave a statement `rg` falsifies in one command. The 4.3.1 execplan's supersession note listed `_extract_stream_fd` among the helpers that moved, while the inventory it corrects names `_extract_reader_fd` and `_extract_writer_fd`, so the two sets never joined up. The record now says what happened: both were one-line pass-throughs to `_extract_stream_fd` from the outset, and #136 deleted them as dead code and inlined their call sites. Nothing was consolidated into `_extract_stream_fd`; it had been there all along.
The FD-lifecycle module's scope note said it is consumed only by `cuprum/_pipeline_streams.py`. That holds for production code, but four `test_pipeline_stream*` unit modules import the seams directly to inject the partial failures the public entry point cannot provoke. Distinguish the two rather than leave a statement `rg` falsifies in one command. The 4.3.1 execplan's supersession note listed `_extract_stream_fd` among the helpers that moved, while the inventory it corrects names `_extract_reader_fd` and `_extract_writer_fd`, so the two sets never joined up. The record now says what happened: both were one-line pass-throughs to `_extract_stream_fd` from the outset, and #136 deleted them as dead code and inlined their call sites. Nothing was consolidated into `_extract_stream_fd`; it had been there all along.
The FD-lifecycle module's scope note said it is consumed only by `cuprum/_pipeline_streams.py`. That holds for production code, but four `test_pipeline_stream*` unit modules import the seams directly to inject the partial failures the public entry point cannot provoke. Distinguish the two rather than leave a statement `rg` falsifies in one command. The 4.3.1 execplan's supersession note listed `_extract_stream_fd` among the helpers that moved, while the inventory it corrects names `_extract_reader_fd` and `_extract_writer_fd`, so the two sets never joined up. The record now says what happened: both were one-line pass-throughs to `_extract_stream_fd` from the outset, and #136 deleted them as dead code and inlined their call sites. Nothing was consolidated into `_extract_stream_fd`; it had been there all along.
Summary
This branch applies the cluster of small consistency and dead-code fixes from the refactoring audit, gated together as one atomic change.
Closes #129.
Each item is individually trivial; they are grouped because none warrants its own PR:
get_stream_backendexhaustiveness — a terminalcase _: raise AssertionError(...)stops a futureStreamBackendmember silently falling off thematchand returningNonetyped asStreamBackend.ExecHookprovenance — imported from its definition sitecuprum.events, dropped fromcuprum.context's public re-export, and demoted to aTYPE_CHECKINGimport incontext(annotations-only use).type: ignore— removed fromlogging_hooks.py; the fields are alreadyHookRegistration | None._extract_reader_fd/_extract_writer_fdinlined to_extract_stream_fdand deleted._run_before_hooksrelocated into the leaf modulecuprum._pipeline_types, turning the in-function imports of_StageObservation/_EventDetails/_run_before_hooksinto top-level imports; the_pipeline_spawn.pyre-export shim is deleted.Review walkthrough
ExecHookprovenance fix._run_before_hooks; cuprum/_process_lifecycle.py, cuprum/_pipeline_stage_streams.py, and cuprum/_pipeline_internals.py switch to top-level imports;cuprum/_pipeline_spawn.pyis removed.type: ignoreremoval and spelling fixes.Validation
make check-fmt: passmake lint: passmake typecheck: passmake test: pass (Python: 654 passed, 39 skipped; Rust: 29 passed)make markdownlint: passcoderabbit review --agent: 0 findingsNotes
The wheel-build snapshot (
test_maturin_build.ambr) dropscuprum/_pipeline_spawn.pyfrom the recorded package file list.Summary by Sourcery
Tighten backend and pipeline behaviour while cleaning up dead code, imports, and spelling across the package.
Bug Fixes:
Enhancements:
Documentation:
Tests:
Chores:
References