Build the Rust extension in CI and fail loudly without it (#258) - #269
Merged
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:
Summary
WalkthroughAdd Windows-target Rust linting. Add ChangesRust extension CI coverage
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 2 warnings)
✅ Passed checks (17 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
Reviewer's GuideThis PR ensures the Python/Rust boundary tests are actually built and exercised in CI by introducing a shared Sequence diagram for extension-gated CI tests with native build and session guardsequenceDiagram
actor GitHubActions
participant CIWorkflow as extension-tests_job
participant Makefile
participant PythonEnv
participant Pytest
participant RustBackend as _rust_backend
GitHubActions->>CIWorkflow: start job extension-tests
CIWorkflow->>CIWorkflow: cargo fetch --locked
CIWorkflow->>Makefile: make develop
Makefile->>PythonEnv: uv run python -m ensurepip --upgrade
Makefile->>PythonEnv: uv run maturin develop --manifest-path cuprum-rust/Cargo.toml
CIWorkflow->>Pytest: uv run pytest -v (CUPRUM_REQUIRE_RUST_EXTENSION=1)
Pytest->>Pytest: pytest_configure(config)
Pytest->>RustBackend: _rust_backend.is_available()
alt [extension available]
RustBackend-->>Pytest: True
Pytest->>Pytest: continue session
Pytest->>Pytest: run extension-gated modules
else [extension missing]
RustBackend-->>Pytest: False
Pytest->>Pytest: raise pytest.UsageError
Pytest-->>GitHubActions: job fails (missing cuprum._rust_backend_native)
end
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![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.
leynos
changed the title
leynos
marked this pull request as ready for review
August 1, 2026 02:07
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d26c765464
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Collaborator
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 @.github/workflows/ci.yml:
- Around line 148-152: Update the “Check out repository” and “Install uv”
workflow steps to use immutable 40-character commit SHAs instead of the current
version tags. Add persist-credentials: false to the checkout step’s
configuration.
In `@conftest.py`:
- Around line 48-54: Update the UsageError message in the native backend
validation block to describe the Rust extension as unavailable rather than
saying it could not be imported, covering both import failures and
_rust_backend.is_available() returning False while preserving the existing
guidance.
- Around line 29-42: The pytest_configure docstring currently lacks the required
NumPy-style sections. Update pytest_configure to document config under
Parameters with its pytest.Config type, document pytest.UsageError under Raises,
and move the existing rationale into a Notes section while preserving its
content.
In `@Makefile`:
- Around line 65-73: Shorten the develop target recipe by removing its lengthy
rationale comments, retaining only one concise comment and the two existing
commands: ensurepip and maturin develop. Move the removed explanation to
docs/developers-guide.md, preserving the rationale for dependency syncing,
extension compilation, identical local/CI invocation, and pip availability.
🪄 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: 660c298d-6401-4918-a3d7-538f3747ccff
📒 Files selected for processing (4)
.github/workflows/ci.ymlMakefileconftest.pydocs/developers-guide.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)
lodyai
Bot
force-pushed
the
ci-build-rust-extension
branch
from
August 1, 2026 12:34
d26c765 to
804a45f
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Collaborator
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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_extension_requirement_guard.py`:
- Line 78: Update the assertion checking that message is not None to include a
descriptive diagnostic message, following the required assert condition,
"message" format; do not leave it as a bare assertion.
- Around line 133-147: Update the _SilentCase dataclass decorator to enable
slots while preserving its frozen behavior by adding slots=True to the existing
dataclass configuration.
In `@docs/developers-guide.md`:
- Around line 1447-1451: Update the developer guide command following make
develop so it does not present the full make test suite as the extension test
command. Replace it with the selected test command used by extension-tests, or
reference a dedicated safe Make target, while preserving the documented
test_pipeline.py behavior described in Lines 1462-1468.
In `@tests/helpers/extension_requirement.py`:
- Line 16: Update the module containing REQUIRE_EXTENSION_ENV by importing
typing as typ and annotating REQUIRE_EXTENSION_ENV with typ.Final[str],
preserving its existing environment-variable value.
🪄 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: 81457415-5684-4e49-bf4c-e9307b0f34ee
📒 Files selected for processing (7)
.github/workflows/ci.ymlMakefileconftest.pycuprum/unittests/__snapshots__/test_maturin_build.ambrcuprum/unittests/test_extension_requirement_guard.pydocs/developers-guide.mdtests/helpers/extension_requirement.py
🔗 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)
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
lodyai
Bot
force-pushed
the
ci-build-rust-extension
branch
from
August 2, 2026 11:26
a1b4d02 to
f12af27
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
lodyai
Bot
force-pushed
the
ci-build-rust-extension
branch
from
August 2, 2026 14:01
f12af27 to
e37aa4c
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Collaborator
|
@coderabbitai review |
✅ Action performedReview finished.
|
lodyai Bot
pushed a commit
that referenced
this pull request
Aug 3, 2026
The Windows case added in 85bc50c derived its expectations from the very exception it was testing: it read `winerror` off the raised `OSError`, built a second `OSError` from that number and the raised `strerror`, and compared the two. That pins CPython's own derivation, which was never in doubt, and says nothing about the value the extension supplied. A Rust side that hard-coded a plausible Win32 code, or reported one taken from some other failure, would have satisfied every assertion. The pre-merge review was right to call it circular. Obtain the expected code from outside the boundary instead. The test now issues the same failing `ReadFile` through `ctypes` — a Windows stream handle is a `std::fs::File`, whose `read` is `ReadFile`, so this is the same system call on the same handle — and reads the code back from `GetLastError`. `ctypes.WinError` then turns that code into the `OSError` CPython itself would raise, supplying the expected `errno`, `strerror`, and subclass. Nothing the extension returns feeds an expectation any more, and the `[WinError N]` guard names the code rather than accepting any digits. `use_last_error` keeps the interpreter from overwriting `GetLastError` before it is read, and explicit `argtypes` stop a pointer-sized `HANDLE` being truncated to a C int. Give the POSIX arm an oracle too. `strerror` was only asserted truthy; it is now compared against `os.strerror(EBADF)`, which CPython looks up independently of the exception. Cover the fallback arm, which nothing exercised. `io_error_to_py_err` hands an `io::Error` carrying no `raw_os_error` to PyO3 unchanged, and that arm is live in production: the write paths in `io_utils` raise `ErrorKind::WriteZero` for a write that makes no progress, and such an error has no number to preserve. Asserting on the built `PyErr` would need a live interpreter, and the crate is compiled with `pyo3/extension-module`, so no `cargo test` binary can link one — a probe fails at link time on undefined `PyExc_*` symbols. Split the decision out into `raw_os_error_parts`, which is pure, and test that directly for both `io::Error` representations. Mutation-verified: making the helper report code 0 instead of no code fails all three synthesized cases. Verified the POSIX assertions against a built extension (`maturin develop`): four pass, the Windows case skips. The Windows arm stays unexecuted, as no job runs the Python suite on Windows. No CI job is added here. POSIX extension-gated execution is #269's `extension-tests` job, and native Windows runtime coverage is #277; this branch does not touch `ci.yml`, and adding a job would conflict with #269 on rebase. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
The modules covering the Python/Rust boundary are gated on `cuprum._rust_backend_native` being importable, and no CI job built it: the test jobs reach the suite through `make build`, which is only `uv sync --group dev`. They have skipped on every run since they were written, so a green build was indistinguishable from one that never exercised the boundary at all. Add `make develop` as the single definition of that build, used by both contributors and CI, so the two cannot drift. It runs `ensurepip` before `maturin develop`, the ordering the benchmark job already proved necessary on a fresh virtual environment. Add a `CUPRUM_REQUIRE_RUST_EXTENSION` guard in `conftest.py` so a missing extension fails the run instead of skipping. The check is session-level rather than per-module because the gated modules gate three different ways — fixture, module-level guard, availability probe — and a new module should not be able to opt out of the requirement by skipping differently. Run the gated modules in a dedicated `extension-tests` job rather than installing the extension for `typecheck-test`. That is forced, not stylistic: with the extension present, `test_pipeline.py` trips the descriptor close race tracked by `#124` (roadmap 8.1.1) and aborts the interpreter in roughly three runs in four. Until that lands, the general test run must stay extension-free. Widening the job is the follow-up. All six gated modules pass under the new job — 70 tests, none skipped — including the four `TestRustConsumeStream` replacement scenarios that remain the end-to-end regression coverage for `#105`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Five review findings on the extension-test job, none of which changed the #124 constraint that shaped it. The guard had no test at all, so deleting it would have left the job green — the exact failure it exists to prevent. Move the decision into `tests/helpers/extension_requirement.py` and cover it there: a root conftest is shadowed by the per-package one and cannot be imported by name from a test module, which is why this could not simply be tested in place. The hook is reached through the plugin manager so the raise itself is covered, not just the message. Mutation-verified: disabling the raise fails that test. Also correct the message. `is_available()` can report false without an `ImportError`, so "could not be imported" would misdescribe that case. Add the three extension-gated behavioural modules. Without an extension four of their scenarios skip — confirmed with `pytest -rs` — so they were never boundary coverage in the ordinary test jobs either, which is the same reason the unit modules are listed. All 88 selected tests pass with the extension present, none skipped. Pin `actions/checkout` and `astral-sh/setup-uv` to the SHAs the rest of the workflow already uses, rather than the mutable tags this job was added with. Pin the Rust toolchain and set up Python for the same reason: the job compiled the extension against whatever the runner image shipped. Move the `develop` rationale above the target so its body fits checkmake's five-line limit, and give `pytest_configure` a NumPy-style docstring. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Five review findings on the extension-test job. The guide told contributors to run `CUPRUM_REQUIRE_RUST_EXTENSION=1 make test`, and then explained three paragraphs later that the extension makes the full suite abort the interpreter. Following the documented command would have done exactly that. Fix it where it starts. The module list moves into the Makefile as `EXTENSION_TEST_TARGETS`, behind a `test-extension` target that sets the guard; CI and the guide both call the target, so the list has one definition instead of living inline in the workflow and being restated in prose. Verified both ways: without the extension the target exits 2 naming `make develop`, and with it 88 tests pass, none skipped. Add `persist-credentials: false` to the extension-tests checkout. This job compiles and runs repository code, so the token must not be left in `.git/config` where that code can reach it. The other job that runs built code already does this. Annotate `REQUIRE_EXTENSION_ENV` as `typ.Final[str]`, give `_SilentCase` `slots=True`, and add the missing message to a bare assertion. Three further findings were already resolved in the previous round and needed no change: the actions are SHA-pinned, `pytest_configure` has its NumPy sections, and the `develop` recipe is two lines. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The original commit here added a `windows-cross-check` CI job on the premise that nothing compiled the `cfg(windows)` arm of `os_error_to_py_err` on a pull request, because `build-wheels.yml` is `workflow_call` only. That premise is wrong: `ci.yml` calls `build-wheels.yml` unconditionally, so the `windows-2022` matrix entry builds the extension natively for `x86_64-pc-windows-msvc` on every pull request. Run `30699964270` shows that job green on a pull request event. Drop the job rather than pay for a second, weaker compile of the same code — a plain `cargo check` with no `-D warnings` — and record the real mechanism in the developers' guide instead. Keep the local cross-compilation recipe: checking the Windows arm without a Windows machine is still useful before pushing. While here, bring the POSIX bullet up to date. `#258` is no longer "in flight": `test_rust_errno.py` is one of the `EXTENSION_TEST_TARGETS`, so the `extension-tests` job runs it with `CUPRUM_REQUIRE_RUST_EXTENSION=1` and fails when the extension was never built. Gate the two `EINTR` retry counters on `cfg(unix)`. Only the Unix read and write paths retry on `EINTR`, so both are dead code on Windows and the wheel build compiles them there for nothing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
checkmake counts the comment block above `develop` as part of the target body and flagged it at eight lines against a limit of five. The rationale belongs in prose anyway, so move it into "Building the extension for tests" and leave the recipe pointing there. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The previous commit gated the two `EINTR` retry counters on `cfg(unix)` because they are dead code on Windows. Nothing in CI would have caught that gate being wrong. The Windows wheel build compiles the arm on every pull request, but it is a plain `maturin build`, so it runs without `-D warnings`: it catches a Windows arm that fails to compile, and only that. A misplaced `#[cfg(unix)]` leaves dead code behind, which is a warning, and the build stays green. Add `make lint-windows`, which runs Clippy for `x86_64-pc-windows-msvc` with the same `--all-targets --all-features -- -D warnings` the host gate uses, and call it from the `lint-test` job after `make lint`. Clippy type-checks without linking, so the target standard library is enough and no Windows runner is involved. PyO3 cannot probe an interpreter for the target, so the Makefile states the ABI version. Verified non-vacuous by mutation. Dropping `#[cfg(unix)]` from `record_write_retry` fails the new target with `error: function record_write_retry is never used`; a type error inside the `#[cfg(windows)]` arm of `write_all_windows` fails it with `E0308`. Host `cargo clippy -- -D warnings` passes with that same type error in place, which is the gap being closed. Keep it out of `make lint`. The target hard-fails when the Windows standard library is absent rather than skipping quietly, and making every contributor install a second standard library before any lint run is a poor trade for a check this narrow. CI installs the target, which is where the check has to hold. This also answers the review's suggestion of a trybuild case, which cannot cover this: trybuild compiles its fixtures for the host target, so on a Linux runner it only ever sees the `cfg(unix)` arm. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
lodyai
Bot
force-pushed
the
ci-build-rust-extension
branch
from
August 4, 2026 01:36
02b3b56 to
c0e572e
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.
Collaborator
|
@coderabbitai review |
✅ Action performedReview finished.
|
`make develop` was described as the single definition of the extension
build, but `ci.yml`'s `benchmark-ratchet` job still ran the target's exact
three-step sequence inline — `make build`, `ensurepip --upgrade`, then
`maturin develop --manifest-path …` — with `--release` bolted on. Two
copies of the same sequence drift, and the copy the ratchet owns is the
one nobody would notice going stale.
Add `MATURIN_DEVELOP_FLAGS` to the `develop` target and have
`benchmark-ratchet` pass `--release` through it. The flag is the only
thing that job needed differently, so it is now the only thing it states.
It stays empty by default: a debug build is what contributors and the
`extension-tests` job want.
The variable resolves identically from the benchmark job's `pushd
"${workspace}"`, because the Makefile's `UV_ENV` sets the same relative
`UV_CACHE_DIR` and `UV_TOOL_DIR` the inline commands did, against the
same working directory.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The guard's own tests covered the decision and the `pytest_configure` hook, but nothing covered the wiring that makes the guard fire at all. Delete `CUPRUM_REQUIRE_RUST_EXTENSION=1` from the `test-extension` recipe, drop a module from `EXTENSION_TEST_TARGETS`, or remove the `make develop` step from the `extension-tests` job, and the suite stayed green — which left this branch's central claim asserted by nothing. Add `test_extension_build_contract.py`, which reads the wiring back. It reads the Makefile through `make --dry-run` rather than by parsing it: the expanded recipe is the command line CI actually runs, so the guard variable and the module list are asserted about the same string. `EXTENSION_TEST_TARGETS` gets two independent checks, because neither implies the other. A scan derives the gated modules from the suite — those requesting the `rust_streams` fixture, skipping with the shared "Rust extension is not installed" reason, or naming `cuprum._rust_backend_native` — and requires each to be declared; that is what would notice a *new* gated module being forgotten, which a hard-coded copy of today's list never would. It covers nine of the ten entries today with no false positives. The tenth, `test_extension_requirement_guard.py`, does not gate at all, so it is named explicitly with the reason it belongs in the job anyway. A third test fails when any signal stops matching, so a renamed fixture empties the scan loudly rather than quietly. The module lives under `cuprum/unittests/` rather than extending `tests/test_workflow_contract.py`: `PYTEST_TARGETS` globs only `cuprum/unittests/test_*.py` and `tests/behaviour/test_*.py`, so a contract test in `tests/` is never run by `make test` and would not gate anything. It sits beside `test_maturin_pins.py` and `test_maturin_toolchain.py`, which parse repository config the same way. Every assertion was checked against the mutation it claims to catch: removing the guard variable, deleting either a derived or the companion module, removing the `make develop` step, dropping `--release` from the benchmark job, breaking the Makefile's flag forwarding, and restoring the inline `maturin develop` all fail the suite. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Owner
Author
|
Both pre-merge findings addressed in Testing (Overall) — ERRORCorrect: nothing asserted the Makefile or CI wiring. Added Where it lives. Not an extension of How the Makefile is read. Through What the
A third test fails when any signal stops matching anything, so a renamed fixture empties the Architectural Complexity — WARNINGAlso correct, and the diagnosis was exact: On Non-vacuityEvery assertion was checked against the mutation it claims to catch. Each was applied, the
Baseline before and after every mutation: 11 passed. Gates
|
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.
Owner
Author
|
@coderabbitai Have the following failed checks now been resolved? If further work is required, please provide an AI agent prompt for the remaining work to be done to address these failures. Do not treat warnings as optional or aspirational. Where a change is out of scope for this PR, propose a GitHub issue unless one exists already. (Treat o11y, code safety, documentation and validation coverage as in scope). ❌ Failed checks (1 error, 1 warning)
|
This comment was marked as resolved.
This comment was marked as resolved.
Owner
Author
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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_extension_build_contract.py`:
- Around line 165-183: Replace typ.Any in _workflow() and _job_steps() with
shared TypedDict models (or a recursive YAML value alias) representing the
workflow, jobs, job steps, and step objects, including the string-keyed run
field. Keep the existing isinstance validation, then use cast or structured
narrowing to return the typed values and reuse these shapes in _run_scripts()
and _step_index_running(); run make typecheck to verify the annotations.
- Around line 77-95: Update the environment filter in _dry_run to exclude
MAKEOVERRIDES alongside the existing Make variables, ensuring nested make runs
use repository defaults; also replace typ.Any with object in the return
annotations of _workflow and _job_steps to satisfy strict typing while
preserving runtime YAML validation.
In `@docs/developers-guide.md`:
- Around line 1653-1659: Update the CI build scope sentence in the documented
extension-build section to exclude the separate wheel job, stating that every CI
development and test extension build uses this target while preserving the
surrounding benchmark-ratchet and MATURIN_DEVELOP_FLAGS guidance.
🪄 Autofix
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: 68b4f7c4-1b3c-41e7-8cd3-6160a8679427
📒 Files selected for processing (5)
.github/workflows/ci.ymlMakefilecuprum/unittests/__snapshots__/test_maturin_build.ambrcuprum/unittests/test_extension_build_contract.pydocs/developers-guide.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)
`make` exports each of its command-line variable overrides into every recipe's environment under that variable's own name, and a `?=` assignment yields to a name already in the environment. The contract module shells out to `make --dry-run` and stripped only `MAKEFLAGS`, `MAKELEVEL` and `MFLAGS`, so the override arrived by the other route and the nested `make` reported the caller's configuration as the repository's. Measured, on GNU Make 4.4.1: run as `make … EXTENSION_TEST_TARGETS=test_extension_requirement_guard.py` the module went 11 passed to 1 failed, and as `make … MATURIN_DEVELOP_FLAGS=--release` the debug-default assertion failed. Bare `pytest` passed in both cases — the contract held or broke according to how the suite was invoked. Scrub every name the Makefile declares with `?=`, read back from the Makefile so the set cannot go stale as variables come and go, and pin the behaviour with a test that plants an override and requires it not to reach the nested `make`. `MAKEOVERRIDES` is not the route: GNU Make does not place it in a recipe's environment at all, so filtering it changes nothing. Split the CI half of the contract into `test_extension_ci_contract.py`, along the seam of what each module reads — the Makefile, or `ci.yml`. The split is what makes room under the 400-line cap, and the new module declares the workflow shapes it reads (jobs, steps, and a step's `run:`) in place of `typing.Any`, so a misspelled key is a type error rather than a `None` that quietly satisfies the assertion above it. Step *values* stay `object`: they come from a file this suite does not control, so they are narrowed where they are read. Correct the guide's claim that every extension build in CI goes through `make develop`. The wheel build does not: `build-wheels.yml` runs `maturin build` for a distributable artefact, never installing into a virtual environment. What is true is that every CI job which installs the extension and then runs against it goes through the target — `extension-tests` and `benchmark-ratchet`, and no others. All seven mutations this contract exists to catch still fail: dropping the guard variable, dropping either a derived or the companion module from `EXTENSION_TEST_TARGETS`, removing the `make develop` step, dropping `--release` from the benchmark job, breaking the flag forwarding, and restoring the inline `maturin develop`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Owner
Author
|
@coderabbitai Have the following failed checks now been resolved? If further work is required, please provide an AI agent prompt for the remaining work to be done to address these failures. Do not treat warnings as optional or aspirational. Where a change is out of scope for this PR, propose a GitHub issue unless one exists already. (Treat o11y, code safety, documentation and validation coverage as in scope). ❌ Failed checks (1 error, 1 warning)
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '== PR branch HEAD =='
git rev-parse --short HEAD
printf '%s\n' '== Relevant Makefile and CI definitions =='
rg -n -C 5 \
-e '^(MATURIN_DEVELOP_FLAGS|EXTENSION_TEST_TARGETS)\s*[:?+]?=' \
-e '^(develop|test-extension):' \
-e '\bmake\s+develop\b' \
-e '\bmaturin\s+develop\b' \
-e 'CUPRUM_REQUIRE_RUST_EXTENSION' \
Makefile .github/workflows/ci.yml
contract_test="$(fd -t f '^test_extension_build_contract\.py$' cuprum)"
printf '\n== Contract test outline ==\n'
ast-grep outline "$contract_test" --items all
printf '\n== Contract test assertions and relevant helpers ==\n'
rg -n -C 4 \
-e 'test_extension_recipe_sets_the_guard_variable' \
-e 'test_every_extension_gated_module_is_a_declared_target' \
-e 'test_always_run_companions_stay_declared' \
-e 'test_extension_job_builds_the_extension_before_running_the_gated_tests' \
-e 'test_benchmark_job_builds_through_the_develop_target' \
-e 'test_develop_target_forwards_the_release_flag' \
-e 'test_no_ci_step_invokes_maturin_develop_directly' \
"$contract_test"Length of output: 11258 🏁 Script executed: #!/bin/bash
set -euo pipefail
contract_test="cuprum/unittests/test_extension_build_contract.py"
printf '%s\n' '== Extension-tests workflow commands =='
rg -n -C 12 \
-e '^ extension-tests:' \
-e 'run: make (develop|test-extension)' \
.github/workflows/ci.yml
printf '%s\n' '== Remaining contract test =='
sed -n '285,390p' "$contract_test"
printf '%s\n' '== CI workflow references from contract test =='
rg -n -C 6 \
-e 'ci\.yml|workflow|extension-tests|make test-extension|make develop|maturin develop' \
"$contract_test"Length of output: 11487 🏁 Script executed: #!/bin/bash
set -euo pipefail
ci_contract="$(fd -t f '^test_extension_ci_contract\.py$' cuprum)"
printf '%s\n' '== CI contract test outline =='
ast-grep outline "$ci_contract" --items all
printf '%s\n' '== CI contract test =='
cat -n "$ci_contract"
printf '%s\n' '== Collection path =='
rg -n -C 5 \
-e '^PYTEST_TARGETS\s*[:?+]?=' \
-e 'cuprum/unittests/test_\*\.py' \
Makefile
printf '%s\n' '== Direct CI maturin develop invocations =='
rg -n -C 3 '\bmaturin\s+develop\b' .github/workflows/ci.ymlLength of output: 9244
Do not create a follow-up issue. Do not create an AI agent prompt. No remaining work is required for these findings. Static inspection verified PR HEAD You are interacting with an AI system. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The modules covering the Python/Rust boundary are gated on
cuprum._rust_backend_nativebeing importable, and no CI job built it. The test jobs reach the suite throughmake build, which is onlyuv sync --group dev, so those modules have skipped on every run since they were written — a green build was indistinguishable from one that never exercised the boundary.Closes #258.
What changed
make developis now the single definition of the extension build, used by contributors and CI alike so the two cannot drift. It runsensurepipbeforematurin develop, the ordering thebenchmark-ratchetjob already proved necessary on a fresh virtual environment.CUPRUM_REQUIRE_RUST_EXTENSIONturns a missing extension into a failure rather than a skip. The check is session-level inconftest.pyrather than per-module, because the gated modules gate three different ways — fixture, module-level guard, availability probe — and a new module should not be able to opt out of the requirement by skipping differently.A dedicated
extension-testsjob builds the extension and runs the gated modules with the guard set.Why a separate job, not
typecheck-testThis is forced, not stylistic, and it is the main thing to review.
With the extension present,
cuprum/unittests/test_pipeline.py::test_pipeline_run_sync_failure_semanticstrips the file-descriptor close race tracked by #124 (roadmap 8.1.1, still unchecked) and aborts the interpreter:Measured 6 aborts in 8 runs. Installing the extension for the general test run would therefore break CI outright. Until #124 lands, the general run must stay extension-free; widening this job is the natural follow-up.
Results
All six gated modules pass under the new job — 70 tests, none skipped:
test_rust_streams.py,test_rust_streams_boundary_property.py,test_rust_extension.py,test_rust_splice.py,test_rust_errno.py, and the extension-dependenttest_backend.pycases.That includes the four
TestRustConsumeStreamreplacement scenarios, which remain the end-to-end regression coverage for #105.Guard behaviour verified both ways: without the extension the run fails with an actionable message; with it built, everything passes.
Validation
make check-fmt,make lint,make typecheck,make testall pass in a clean (extension-free) environment;mbake validate Makefileandmarkdownlintclean;cs deltareports no issues.Note
Stacked on #268, which is stacked on #241. #268 fixes the one gated test that would otherwise fail this job.