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
|
@coderabbitai Have the following now been resolved?
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- Documentation headings and artefact sections ---'
rg -n -i -C 12 '^## artefacts and notes|^### expected artefacts|artefacts and notes' \
docs/execplans/4-1-1-performance-extension-foundation.md || true
printf '%s\n' '--- Relevant Python module outlines ---'
ast-grep outline \
cuprum/_process_lifecycle.py \
cuprum/_pipeline_stage_streams.py \
cuprum/_pipeline_internals.py \
cuprum/_pipeline_types.py \
--items all --view expanded || true
printf '%s\n' '--- Pipeline type and import references ---'
rg -n -C 4 \
'TYPE_CHECKING|_pipeline_internals|_pipeline_types|_EventDetails|_StageObservation' \
cuprum/_process_lifecycle.py \
cuprum/_pipeline_stage_streams.py \
cuprum/_pipeline_internals.py \
cuprum/_pipeline_types.py || trueLength of output: 21463
|
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
A cluster of small consistency and dead-code fixes surfaced by the refactoring audit, each individually trivial and gated together. - **`get_stream_backend` exhaustiveness**: add a terminal `case _: raise AssertionError(...)` so a future `StreamBackend` member cannot fall off the `match` and silently return `None` typed as `StreamBackend`. Covered by a new test that forces an out-of-band requested value. - **`ExecHook` provenance**: import `ExecHook` from its definition site `cuprum.events` in `cuprum/__init__.py`, and drop it from `cuprum.context`'s public re-export. `context` now imports `ExecHook` only under `TYPE_CHECKING`, since it is used solely in annotations. - **Spurious `type: ignore[assignment]`**: remove the two ignores in `logging_hooks.py`; the fields are typed `HookRegistration | None`, so assigning `None` needs no suppression. - **Dead FD aliases**: inline `_extract_reader_fd` / `_extract_writer_fd` (pure pass-throughs to `_extract_stream_fd`) at their call sites and delete them; update the backend-selection tests to patch the canonical helper. - **Lazy in-function imports / import cycle**: relocate `_run_before_hooks` into the dependency-free leaf module `cuprum._pipeline_types` (beside the shared dataclasses), so `_process_lifecycle`, `_pipeline_stage_streams`, and `_subprocess_execution` import `_StageObservation` / `_EventDetails` / `_run_before_hooks` at the top level instead of inside functions. Delete the `_pipeline_spawn.py` re-export shim and import `_spawn_pipeline_processes` from `_process_lifecycle` directly. The genuine `cuprum.sh` cycle still routes through `_sh_module()`. - **en-GB spelling**: "Programme" -> "Program" in `logging_adapter.py`, and align "serialises" to the Oxford "-ize" spelling used elsewhere. The wheel-build snapshot drops `cuprum/_pipeline_spawn.py` from the recorded package file list.
Regenerate the Typos configuration with the current policy inputs and use `artefact` terminology in maintained prose so the spelling gate is reproducible.
Preserve the current pipeline helper ownership while removing obsolete imports and restoring formatter-required spacing.
Resolving the rebase conflicts onto the refactored `get_stream_backend` left two mechanical formatting regressions that the commit gates flag: - `test_backend.py`: the merged `test_unhandled_backend_member_raises_ assertion` lacked the surrounding two blank lines (ruff E302). - `4-1-1-performance-extension-foundation.md`: a duplicate blank line at the seam before "Artefacts and Notes" (markdownlint MD012). Restore the expected spacing so `check-fmt`, `lint`, and `markdownlint` pass again. No behavioural change.
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.
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