Add executed rust_consume_stream I/O-error boundary coverage (#276) - #278
Add executed rust_consume_stream I/O-error boundary coverage (#276)#278leynos wants to merge 6 commits into
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 executed ChangesRust stream validation
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 warnings, 2 inconclusive)
✅ Passed checks (16 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Reviewer's GuideAdds platform-neutral I/O error propagation coverage for rust_consume_stream and splits out non-extension integration guards into a dedicated test module, updating docs and the maturin build snapshot accordingly. Sequence diagram for rust_consume_stream I/O-error propagation coveragesequenceDiagram
actor TestRustConsumeStream
participant _rust_backend as _rust_backend
participant rust_consume_stream as rust_consume_stream
participant io_error_to_py_err as io_error_to_py_err
participant OS
TestRustConsumeStream->>_rust_backend: rust_consume_stream(closed_descriptor)
_rust_backend->>rust_consume_stream: rust_consume_stream(closed_descriptor)
rust_consume_stream->>OS: read(descriptor)
OS-->>rust_consume_stream: [EBADF or EINVAL]
rust_consume_stream->>io_error_to_py_err: io_error_to_py_err(io_error)
io_error_to_py_err-->>_rust_backend: OSError(errno)
_rust_backend-->>TestRustConsumeStream: raise OSError(errno)
TestRustConsumeStream->>TestRustConsumeStream: assert errno in {EBADF, EINVAL}
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
f12af27 to
e37aa4c
Compare
b0c0138 to
4cf4b64
Compare
4cf4b64 to
530701f
Compare
530701f to
02fea4a
Compare
4045613 to
c81ad46
Compare
02fea4a to
051f500
Compare
`test_rust_streams.py` carried two unrelated concerns: the behaviour of the Rust pump and consume entry points, and an AST scan asserting that production code does not yet route through `rust_consume_stream`. The scan never touches the compiled extension, so it does not belong in an extension-gated module, and at 394 lines the file had no room left under the 400-line cap for the consume-side I/O-error coverage issue #276 asks for. Lift the scanner, its self-test, and the two status guards into `test_rust_consume_integration_guard.py`, and repoint the ADR and roadmap references at the new home. The guards keep running under the ordinary test glob; they are deliberately left outwith `EXTENSION_TEST_TARGETS` because they have no dependency on the extension. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The Rust-side tests over `consume_stream_files` exercise the read-and-decode loop below the PyO3 boundary, so they cannot see what the boundary makes of an `io::Error`. `rust_pump_stream` has had that coverage since it was written; `rust_consume_stream` had none of its own. Add the counterpart beside the other consume tests: a closed descriptor must surface as an `OSError` whose `errno` names an unusable descriptor. Accept either `EBADF` or `EINVAL`, matching the pump test, because Windows reports an invalid handle rather than a bad POSIX descriptor. `test_rust_errno.py` reaches the same conversion, but for a different reason: it pins the conversion contract itself — the exact number, `strerror`, and subclass selection — and skips on Windows to do so. Say so in both docstrings so neither reads as a stray copy of the other. Verified against a built extension: the test passes, and fails with `errno` of `None` when `io_error_to_py_err` is reduced to `err.into()`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both `pytest.raises(OSError)` guards in this module matched on `strerror` text — "Bad file descriptor", plus the Windows variants. `strerror` comes from the C library and glibc translates it under `LC_MESSAGES`, so under a locale with the catalogue installed the guard would fail before the test reached the errno assertion it exists for. CodeRabbit raised exactly this against the sibling `test_rust_errno.py`; the new consume-side test mirrored the pump-side one and so inherited a second instance of it. Anchor on the `[Errno N]` prefix CPython formats itself, built from the same `errno` constants the assertions use, so the pattern and the assertion cannot drift and neither depends on the platform's language. This also satisfies ruff PT011, which is why a `match` is required here at all. The guard now catches more than it did. Under a mutant that drops errno preservation, the old pattern still matched — the English text is present either way — and only the assertion failed. The new one fails at the `match`, which is the earlier and more accurate signal. Verified with the extension built: both tests pass (81 passed, 0 skipped via `make test-extension`), and both fail under the mutant. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
051f500 to
0148d75
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
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_rust_consume_integration_guard.py`:
- Around line 27-42: Introduce a module-level base domain exception and make
_ModuleReferenceScanError inherit from it, retaining the Error suffix. In
_ModuleReferenceScanError.__init__, store path and symbol on the instance and
build the formatted failure message in a local variable before passing it to
super().__init__. Update the surrounding raise and catch logic for
_module_references_symbol to reference the concrete inspection error while
preserving the existing error details.
In `@cuprum/unittests/test_rust_streams.py`:
- Around line 304-323: Update the NumPy-style docstrings for public test
interfaces: in cuprum/unittests/test_rust_streams.py:304-323, add Parameters and
Returns sections to test_propagates_io_errors; in
cuprum/unittests/test_rust_errno.py:125-133, add the same sections to
test_consume_error_reports_a_branchable_errno; and in
cuprum/unittests/test_rust_consume_integration_guard.py:67-98, add structured
NumPy-style documentation to each public test function, documenting
fixtures/arguments and the None return behavior.
🪄 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: 91bc23b2-0a03-4353-b681-f23b7f4a3705
📒 Files selected for processing (6)
cuprum/unittests/__snapshots__/test_maturin_build.ambrcuprum/unittests/test_rust_consume_integration_guard.pycuprum/unittests/test_rust_errno.pycuprum/unittests/test_rust_streams.pydocs/adr-002-additional-rust-components.mddocs/roadmap.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)
`_ModuleReferenceScanError` reported which module it could not inspect only inside its rendered message, so the one place that handles it had to re-render the exception to say anything useful, and the test that pins the failure asserted on a substring of English. CodeRabbit asked for a two-level hierarchy here — a module-level base plus a concrete error. Every test-local exception in `cuprum/unittests` derives directly from `Exception` or `BaseException` and none has a base class: `_AsyncObserveHookError` and `_SyncObserveHookError` in `test_cqrs_helpers.py`, the four in `test_cqrs_hook_behaviour.py`, `_MetricsBackendError` in `test_metrics_adapter_stateful.py`, and `_UnexpectedProbeFailure` in `test_line_splitting.py`. A base class with exactly one subclass, private to one test module, buys no caller the ability to catch a family — there is no family, and nothing outside this file can import either name. So take the part of the guideline that pays: structured attributes. Store `path` and `symbol`, and build the message in a local before handing it to `super().__init__`. The parametrised test now asserts on those attributes rather than matching "cannot inspect", and the production scan builds its report from `exc.path` relative to the package root, which is both shorter and more precise than the absolute path the message carried. That is the whole point of machine-readable failure detail, and it is now exercised rather than merely available. Also document the three test functions in NumPy style, per AGENTS.md. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
AGENTS.md requires NumPy-style docstrings on public interfaces, and the test functions in these modules were only partly converted: `test_rust_streams.py` documented its four module-level pump tests but none of the seven `TestRustConsumeStream` methods, and `test_rust_errno.py` documented its two private Win32 helpers but none of its five tests. CodeRabbit named three functions; converting only those would have left both modules exactly as inconsistent, in different places. Document every test function in both, and keep the explanatory prose that was already there — it is the more valuable half — by moving it under `Notes`, below the sections a reader scans for first. The paragraph in `test_propagates_io_errors` distinguishing its platform-neutral remit from `test_rust_errno.py`'s stricter conversion contract is preserved verbatim. That pushed both modules past the 400-line ceiling pylint enforces: `test_rust_errno.py` to 413 and `test_rust_streams.py` to 414. Shaving the `Returns` sections to fit would have made the documentation inconsistent again in order to satisfy a rule about file size, so split along the seams the modules had already drawn for themselves: - `test_rust_errno.py` said in its own docstring that the conversion has a POSIX arm and a Windows arm that hand the number over differently. The Windows arm shares no fixture with the POSIX one, so it moves whole to `test_rust_errno_windows.py` with the `ctypes` machinery it needs. - `test_rust_streams.py` covered two entry points and had drawn the line between them as a class. `TestRustConsumeStream` and its `_consume_payload` helper move to `test_rust_consume_stream.py`. `INVALID_FD_ERRNOS` and its derived match pattern are now wanted by both the pump and consume modules, so they move to `tests/helpers/stream_pipes` rather than being restated. No test body changed. Modules land at 165, 268, 228 and 217 lines. Cross-references updated in `test_rust_errno.py`, `test_rust_streams_boundary_property.py`, the consume integration guard, and two passages of the developers' guide. Wheel-manifest snapshot regenerated for the two new files. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`cs delta` flagged code duplication in the new consume module:
`test_uses_default_buffer_size`, `test_replaces_invalid_bytes` and
`test_replaces_incomplete_sequence` had the same three-line body, and the
same body again as the already-parameterised `test_decodes_payload` above
them — build a payload, consume it, compare against
`payload.decode("utf-8", errors="replace")`.
They were never really four tests. They are four inputs to one property:
the extension's output equals Python's own replacement decoding. Move
them into the existing parameter list, where the case names survive as
pytest ids, and widen `buffer_size` to `int | None` so the default-buffer
case can omit the argument. `_consume_payload` already dropped a `None`
`buffer_size` before forwarding, which is precisely the path that case
needs, so no helper changed.
Verified without building the extension: driving `_consume_payload`
against a stub `rust_consume_stream` shows all five cases round-trip, and
the `None` case forwards `{}` rather than `{"buffer_size": None}` — the
same call the removed test made by omitting the argument.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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_rust_consume_integration_guard.py`:
- Around line 56-64: Update _node_references_symbol to use structural match/case
instead of the isinstance chain, and add an ast.Call pattern that recognizes
getattr lookups whose second argument is the literal string
"rust_consume_stream". Add a parameterized test covering this dynamic-reference
form alongside the existing symbol-reference cases.
In `@cuprum/unittests/test_rust_consume_stream.py`:
- Around line 22-26: Remove the import dependency on tests.helpers.stream_pipes
in test_rust_consume_stream.py by relocating INVALID_FD_ERRNOS,
INVALID_FD_MESSAGE_RE, and _safe_close into the cuprum.unittests package, or
otherwise packaging that helper module with the wheel. Update
test_rust_consume_stream.py to use the packaged location while preserving the
existing helper behavior.
In `@cuprum/unittests/test_rust_errno_windows.py`:
- Around line 139-151: Update fixture_write_only_handle to yield the original
CRT file descriptor fd instead of calling msvcrt.get_osfhandle. Keep the
platform-specific conversion in rust_consume_stream via
_convert_fd_for_platform, while retaining the existing write-only handle setup
and cleanup.
In `@cuprum/unittests/test_rust_errno.py`:
- Around line 39-44: Update the Python test job workflow to run maturin develop
before make test, then explicitly fail when cuprum.is_rust_available() is false.
Ensure the existing test command remains in place so the POSIX closed-descriptor
regression executes instead of being skipped.
In `@docs/developers-guide.md`:
- Line 1579: Wrap the Markdown prose in the referenced sentence to keep each
line within 80 columns, while preserving the inline code path
`cuprum/unittests/test_rust_consume_stream.py` intact.
In `@tests/helpers/stream_pipes.py`:
- Around line 22-27: Extend INVALID_FD_MESSAGE_RE to include escaped “[WinError
N]” alternatives for the relevant invalid file-descriptor error codes, while
preserving the existing “[Errno N]” matches used by pump and consume tests.
🪄 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: cda732a0-111d-4464-9365-fabfd65bd206
📒 Files selected for processing (11)
cuprum/unittests/__snapshots__/test_maturin_build.ambrcuprum/unittests/test_rust_consume_integration_guard.pycuprum/unittests/test_rust_consume_stream.pycuprum/unittests/test_rust_errno.pycuprum/unittests/test_rust_errno_windows.pycuprum/unittests/test_rust_streams.pycuprum/unittests/test_rust_streams_boundary_property.pydocs/adr-002-additional-rust-components.mddocs/developers-guide.mddocs/roadmap.mdtests/helpers/stream_pipes.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)
| def _node_references_symbol(node: ast.AST, symbol: str) -> bool: | ||
| """Return whether an AST node references *symbol* as a Python symbol.""" | ||
| if isinstance(node, ast.Name): | ||
| return node.id == symbol | ||
| if isinstance(node, ast.Attribute): | ||
| return node.attr == symbol | ||
| if isinstance(node, ast.alias): | ||
| return symbol in {node.name, node.asname} | ||
| return False |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -HI -t f '^(pyproject\.toml|setup\.cfg|tox\.ini|\.python-version)$' . \
-x rg -n -C 2 'requires-python|python_version|target-version' {} || true
sed -n '56,122p' cuprum/unittests/test_rust_consume_integration_guard.pyRepository: leynos/cuprum
Length of output: 3028
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- guard implementation and callers ---'
rg -n -C 5 '_node_references_symbol|_module_references_symbol|rust_consume_stream|getattr' cuprum pyproject.toml README.md docs 2>/dev/null || true
printf '%s\n' '--- complete guard file outline and relevant sections ---'
wc -l cuprum/unittests/test_rust_consume_integration_guard.py
sed -n '1,190p' cuprum/unittests/test_rust_consume_integration_guard.py
printf '%s\n' '--- Python configuration ---'
sed -n '1,80p' pyproject.tomlRepository: leynos/cuprum
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- guard scan and production source files ---'
sed -n '139,180p' cuprum/unittests/test_rust_consume_integration_guard.py
rg -l 'rust_consume_stream' cuprum --glob '*.py' | sort
printf '%s\n' '--- exact configuration ---'
sed -n '1,55p' pyproject.toml
printf '%s\n' '--- standalone AST behaviour probe ---'
python3 - <<'PY'
import ast
symbol = "rust_consume_stream"
def current(node: ast.AST) -> bool:
if isinstance(node, ast.Name):
return node.id == symbol
if isinstance(node, ast.Attribute):
return node.attr == symbol
if isinstance(node, ast.alias):
return symbol in {node.name, node.asname}
return False
def proposed(node: ast.AST) -> bool:
match node:
case ast.Name(id=name):
return name == symbol
case ast.Attribute(attr=name):
return name == symbol
case ast.alias(name=name, asname=alias):
return symbol in {name, alias}
case ast.Call(
func=ast.Name(id="getattr"),
args=[_, ast.Constant(value=name), *_],
) if name == symbol:
return True
case _:
return False
cases = {
"literal getattr call": 'getattr(streams, "rust_consume_stream")(reader_fd)\n',
"non-literal getattr": 'getattr(streams, name)(reader_fd)\n',
"keyword getattr": 'getattr(object=streams, name="rust_consume_stream")(reader_fd)\n',
"aliased getattr": 'lookup(streams, "rust_consume_stream")(reader_fd)\n',
}
for label, source in cases.items():
tree = ast.parse(source)
print(label, "current=", any(current(n) for n in ast.walk(tree)),
"proposed=", any(proposed(n) for n in ast.walk(tree)))
PYRepository: leynos/cuprum
Length of output: 3801
🏁 Script executed (no clone):
#!/bin/bash
set -euo pipefail
python3 --version
for executable in python3.12 python3.13 python3.14; do
if command -v "$executable" >/dev/null 2>&1; then
"$executable" --version
fi
done
python3 - <<'PY'
import ast
symbol = "rust_consume_stream"
source = 'getattr(streams, "rust_consume_stream")(reader_fd)\n'
tree = ast.parse(source)
def current(node):
if isinstance(node, ast.Name):
return node.id == symbol
if isinstance(node, ast.Attribute):
return node.attr == symbol
if isinstance(node, ast.alias):
return symbol in {node.name, node.asname}
return False
print("AST nodes:")
for node in ast.walk(tree):
if isinstance(node, ast.Call):
function = ast.dump(node.func, include_attributes=False)
arguments = [ast.dump(argument, include_attributes=False) for argument in node.args]
print(f"Call(func={function}, args={arguments})")
print("current_match:", any(current(node) for node in ast.walk(tree)))
PY
Length of output: 482
Detect literal dynamic references and use structural pattern matching.
Add an ast.Call case for literal getattr(..., "rust_consume_stream") lookups and a parameterized test. Replace the isinstance() chain with match/case.
🤖 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 `@cuprum/unittests/test_rust_consume_integration_guard.py` around lines 56 -
64, Update _node_references_symbol to use structural match/case instead of the
isinstance chain, and add an ast.Call pattern that recognizes getattr lookups
whose second argument is the literal string "rust_consume_stream". Add a
parameterized test covering this dynamic-reference form alongside the existing
symbol-reference cases.
Source: Path instructions
| from tests.helpers.stream_pipes import ( | ||
| INVALID_FD_ERRNOS, | ||
| INVALID_FD_MESSAGE_RE, | ||
| _safe_close, | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -t f -a 'test_maturin_build.ambr' .
rg -n -C3 'cuprum/unittests|tests/helpers|test_rust_consume_stream' . || trueRepository: leynos/cuprum
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- package and build configuration ---'
rg -n -C4 '^(include|exclude|packages|package|wheel|sdist|module-name|build-backend)|cuprum|tests' \
pyproject.toml setup.cfg setup.py Cargo.toml Makefile 2>/dev/null || true
printf '%s\n' '--- relevant files ---'
git ls-files \
cuprum/unittests/test_rust_consume_stream.py \
tests/helpers/stream_pipes.py \
tests/__init__.py \
tests/helpers/__init__.py \
cuprum/unittests/__init__.py \
cuprum/unittests/__pycache__ 2>/dev/null || true
printf '%s\n' '--- wheel snapshot content ---'
rg -n -C8 'test_maturin|contents|files|cuprum/unittests|tests/helpers|stream_pipes' \
cuprum/unittests/__snapshots__/test_maturin_build.ambr 2>/dev/null || true
printf '%s\n' '--- import sites and helper definition ---'
rg -n -C5 'from tests\.helpers\.stream_pipes|INVALID_FD_ERRNOS|INVALID_FD_MESSAGE_RE|def _safe_close|def drain_blocking_payload_size' \
cuprum tests 2>/dev/null || trueRepository: leynos/cuprum
Length of output: 24756
Remove the wheel’s dependency on tests.helpers.stream_pipes.
The wheel includes cuprum/unittests/test_rust_consume_stream.py but not tests/helpers/stream_pipes.py, so test collection raises ModuleNotFoundError. Move the shared helpers into cuprum/unittests or include them in the wheel.
🤖 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 `@cuprum/unittests/test_rust_consume_stream.py` around lines 22 - 26, Remove
the import dependency on tests.helpers.stream_pipes in
test_rust_consume_stream.py by relocating INVALID_FD_ERRNOS,
INVALID_FD_MESSAGE_RE, and _safe_close into the cuprum.unittests package, or
otherwise packaging that helper module with the wheel. Update
test_rust_consume_stream.py to use the packaged location while preserving the
existing helper behavior.
| @pytest.fixture(name="write_only_handle") | ||
| def fixture_write_only_handle(tmp_path: pathlib.Path) -> cabc.Iterator[int]: | ||
| """Yield a native Windows handle open for writing, which cannot be read. | ||
|
|
||
| The Windows entry points take a native ``HANDLE`` rather than a C runtime | ||
| descriptor, so the descriptor is converted with ``msvcrt.get_osfhandle``. | ||
| ``ReadFile`` on a handle opened ``GENERIC_WRITE`` fails, which is what puts | ||
| a Win32 code on the ``io::Error`` the conversion then has to preserve. | ||
| """ | ||
| msvcrt = pytest.importorskip("msvcrt", reason="Windows-only handle conversion") | ||
| fd = os.open(tmp_path / "write-only.bin", os.O_WRONLY | os.O_CREAT) | ||
| try: | ||
| yield msvcrt.get_osfhandle(fd) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline cuprum/_streams_rs.py --items all --type function
rg -n -C5 \
'def _convert_fd_for_platform|msvcrt\.get_osfhandle|def rust_consume_stream' \
cuprum/_streams_rs.py
rg -n -C4 \
'fixture_write_only_handle|rust_consume_stream\(write_only_handle' \
cuprum/unittests/test_rust_errno_windows.pyRepository: leynos/cuprum
Length of output: 1852
🏁 Script executed:
sed -n '35,125p' cuprum/_streams_rs.py
sed -n '135,205p' cuprum/unittests/test_rust_errno_windows.py
rg -n -C3 'rust_pump_stream|_convert_fd_for_platform|ReadFile|GetLastError|get_osfhandle' .Repository: leynos/cuprum
Length of output: 50371
Yield the CRT descriptor from fixture_write_only_handle.
rust_consume_stream already calls _convert_fd_for_platform, which invokes msvcrt.get_osfhandle. Convert the descriptor only when calling _win32_readfile_error; otherwise Windows performs the conversion twice and the test fails before ReadFile.
🤖 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 `@cuprum/unittests/test_rust_errno_windows.py` around lines 139 - 151, Update
fixture_write_only_handle to yield the original CRT file descriptor fd instead
of calling msvcrt.get_osfhandle. Keep the platform-specific conversion in
rust_consume_stream via _convert_fd_for_platform, while retaining the existing
write-only handle setup and cleanup.
| # The assertions are scoped to the platform they describe rather than the module | ||
| # being skipped wholesale, because the two arms carry different taxonomies: the | ||
| # POSIX cases below name an `errno` and the subclass CPython derives from it. No | ||
| # job executes them today (see `docs/developers-guide.md`, "Preserving the | ||
| # operating-system error code"); POSIX execution arrives with the | ||
| # `extension-tests` job in #269. Until then they encode the contract. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -t f -a 'ci.yml' .github
rg -n -C4 \
'maturin (develop|build)|_rust_backend_native|extension-tests|pytest|make test' \
.github Makefile pyproject.toml 2>/dev/null || trueRepository: leynos/cuprum
Length of output: 11542
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- .github/workflows/ci.yml ---'
sed -n '90,155p' .github/workflows/ci.yml
sed -n '260,315p' .github/workflows/ci.yml
printf '%s\n' '--- Makefile test targets ---'
sed -n '40,75p' Makefile
sed -n '145,160p' Makefile
printf '%s\n' '--- test_rust_errno.py ---'
cat -n cuprum/unittests/test_rust_errno.py
printf '%s\n' '--- native availability and skip mechanisms ---'
rg -n -C3 \
'skip|skipif|is_rust_available|_rust_backend_native|rust_backend' \
cuprum/unittests conftest.py tests .github Makefile pyproject.toml 2>/dev/null || true
printf '%s\n' '--- workflow job structure and test commands ---'
python3 - <<'PY'
from pathlib import Path
path = Path(".github/workflows/ci.yml")
lines = path.read_text().splitlines()
for index, line in enumerate(lines, 1):
if line.startswith(" ") and not line.startswith(" ") and line.rstrip().endswith(":"):
print(f"{index}: {line}")
if "run: make test" in line or "run: pytest" in line or "run: uv run pytest" in line:
print(f"{index}: {line}")
PYRepository: leynos/cuprum
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- workflow files and relevant commands ---'
find .github -type f -maxdepth 3 -print | sort
rg -n -C2 \
'test_rust_errno|maturin develop|maturin build|make test|pytest|generate-coverage|extension-tests' \
.github/workflows .github/actions docs/developers-guide.md
printf '%s\n' '--- native fixture and backend probe ---'
cat -n conftest.py | sed -n '1,85p'
cat -n cuprum/_rust_backend.py | sed -n '1,100p'
printf '%s\n' '--- relevant documentation ---'
rg -n -C5 \
'Preserving the operating-system error code|extension-tests|native extension|maturin develop|Rust extension' \
docs README.md
printf '%s\n' '--- deterministic workflow check ---'
python3 - <<'PY'
from pathlib import Path
workflow_dir = Path(".github/workflows")
terms = (
"maturin develop",
"maturin build",
"make test",
"pytest",
"test_rust_errno",
"generate-coverage",
"build-wheels",
)
for path in sorted(workflow_dir.glob("*")):
if not path.is_file():
continue
lines = path.read_text(errors="replace").splitlines()
hits = [
(index, line.strip())
for index, line in enumerate(lines, 1)
if any(term in line for term in terms)
]
if hits:
print(path)
for index, line in hits:
print(f" {index}: {line}")
PYRepository: leynos/cuprum
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
find .github/actions -maxdepth 3 -type f -print | sort
rg -n -C4 \
'pytest|maturin|build|native|pytest-workers' \
.github/actions .github/workflows 2>/dev/null || trueRepository: leynos/shared-actions
Length of output: 50378
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- documented CI status ---'
sed -n '1270,1302p' docs/developers-guide.md
printf '%s\n' '--- focused workflow verifier ---'
python3 - <<'PY'
from pathlib import Path
path = Path(".github/workflows/ci.yml")
lines = path.read_text(encoding="utf-8").splitlines()
jobs = {}
current_job = None
current_step = None
for number, line in enumerate(lines, 1):
if line.startswith(" ") and not line.startswith(" ") and line.rstrip().endswith(":"):
current_job = line.strip()[:-1]
jobs[current_job] = []
current_step = None
elif current_job and line.startswith(" - name: "):
current_step = {"line": number, "name": line.split(": ", 1)[1], "commands": []}
jobs[current_job].append(current_step)
elif current_step and line.startswith(" run: "):
current_step["commands"].append(line.split(": ", 1)[1])
elif current_step and line.startswith(" "):
current_step["commands"].append(line.strip())
for job, steps in jobs.items():
names = [step["name"] for step in steps]
test_indexes = [
index for index, step in enumerate(steps)
if any(command == "make test" for command in step["commands"])
]
native_indexes = [
index for index, step in enumerate(steps)
if any("maturin develop" in command for command in step["commands"])
]
if test_indexes or native_indexes:
print(f"{job}:")
print(f" test steps: {[(names[i], steps[i]['line']) for i in test_indexes]}")
print(f" native-build steps: {[(names[i], steps[i]['line']) for i in native_indexes]}")
if test_indexes:
print(f" native build precedes make test: "
f"{bool(native_indexes and max(native_indexes) < min(test_indexes))}")
print("Makefile test target:")
makefile = Path("Makefile").read_text(encoding="utf-8")
start = makefile.index("test: ")
end = makefile.find("\n\n", start)
target = makefile[start:end if end != -1 else None]
print(target)
print("maturin develop in Makefile test target:", "maturin develop" in target)
PY
printf '%s\n' '--- coverage action entry point ---'
find .github/actions/generate-coverage -maxdepth 2 -type f -print 2>/dev/null | sortRepository: leynos/cuprum
Length of output: 2887
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
make_lines = Path("Makefile").read_text(encoding="utf-8").splitlines()
test_start = next(
index for index, line in enumerate(make_lines)
if line.startswith("test: ")
)
test_lines = []
for line in make_lines[test_start:]:
if test_lines and line and not line.startswith(("\t", " ")):
break
test_lines.append(line)
print("Makefile test target:")
print("\n".join(test_lines))
print("contains maturin develop:", any("maturin develop" in line for line in test_lines))
workflow_lines = Path(".github/workflows/ci.yml").read_text(encoding="utf-8").splitlines()
run_tests = next(
index for index, line in enumerate(workflow_lines)
if line.strip() == "run: make test"
)
job_start = max(
index for index, line in enumerate(workflow_lines[:run_tests])
if line.startswith(" ") and not line.startswith(" ") and line.rstrip().endswith(":")
)
job_lines = workflow_lines[job_start:run_tests]
native_build_lines = [
(job_start + offset + 1, line.strip())
for offset, line in enumerate(job_lines)
if "maturin develop" in line or "maturin build" in line
]
print("make test workflow line:", run_tests + 1)
print("native build commands before it:", native_build_lines)
print("native build precedes make test:", bool(native_build_lines))
PYRepository: leynos/cuprum
Length of output: 1073
Build and require the native extension in the Python test job.
Run maturin develop before make test, then fail if cuprum.is_rust_available() is false. make test does not build the extension, and conftest.py skips the POSIX cases when it is absent. CI can therefore pass without running the closed-descriptor regression.
🤖 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 `@cuprum/unittests/test_rust_errno.py` around lines 39 - 44, Update the Python
test job workflow to run maturin develop before make test, then explicitly fail
when cuprum.is_rust_available() is false. Ensure the existing test command
remains in place so the POSIX closed-descriptor regression executes instead of
being skipped.
| These Rust-side cases are not the only coverage of these categories, and it is | ||
| worth knowing why they carry the load. `TestRustConsumeStream` in | ||
| `cuprum/unittests/test_rust_streams.py` already defines Python/Rust boundary | ||
| `cuprum/unittests/test_rust_consume_stream.py` already defines Python/Rust boundary |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Wrap the changed Markdown prose.
Wrap Line 1579 at 80 columns. Keep the inline code path intact.
As per coding guidelines, “Wrap Markdown prose and bullets at 80 columns”; as
per path instructions, “wrap prose at 80 columns.”
Triage: [type:docstyle]
🤖 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 1579, Wrap the Markdown prose in the
referenced sentence to keep each line within 80 columns, while preserving the
inline code path `cuprum/unittests/test_rust_consume_stream.py` intact.
Sources: Coding guidelines, Path instructions
| # `pytest.raises(OSError)` needs a `match` to satisfy ruff PT011, but `strerror` | ||
| # comes from the C library and is translated under a non-English locale. Anchor | ||
| # on the `[Errno N]` prefix CPython formats itself, which no locale changes. | ||
| INVALID_FD_MESSAGE_RE = "|".join( | ||
| re.escape(f"[Errno {code}]") for code in INVALID_FD_ERRNOS | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Match the Windows OSError prefix.
Extend INVALID_FD_MESSAGE_RE to accept [WinError N]. The Windows boundary
test expects that prefix for the same native conversion. Both pump and consume
tests use this pattern, so Windows tests fail at pytest.raises before their
valid errno assertions run.
Proposed fix
INVALID_FD_MESSAGE_RE = "|".join(
- re.escape(f"[Errno {code}]") for code in INVALID_FD_ERRNOS
+ (
+ r"\[(?:"
+ + "|".join(re.escape(f"Errno {code}") for code in INVALID_FD_ERRNOS)
+ + r"|WinError \d+)\]"
+ )
)🤖 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 `@tests/helpers/stream_pipes.py` around lines 22 - 27, Extend
INVALID_FD_MESSAGE_RE to include escaped “[WinError N]” alternatives for the
relevant invalid file-descriptor error codes, while preserving the existing
“[Errno N]” matches used by pump and consume tests.
Closes #276.
What this adds
TestRustConsumeStream::test_propagates_io_errorsincuprum/unittests/test_rust_streams.py. It handsrust_consume_streamaclosed descriptor and asserts the failure arrives as an
OSErrorwhose.errnoiserrno.EBADForerrno.EINVAL— the same{EBADF, EINVAL}setthe pump-side
test_rust_pump_stream_propagates_io_errorsalready uses,because Windows reports an invalid handle rather than a bad POSIX descriptor.
The
consume_stream_filessnapshots and properties inrust/cuprum-rust/src/consume_snapshot_tests.rsare untouched. They exercisethe read-and-decode loop below the PyO3 boundary and so cannot observe error
translation at it; this test is additional coverage, not a replacement.
Why this is based on #269, not #268
#269 sits on top of #268, so this is still stacked above #268. But #269 is
where the
extension-testsCI job andmake test-extensionlive, andtest_rust_streams.pyis already listed in the Makefile'sEXTENSION_TEST_TARGETS. Basing on #268 alone would leave the new testskipping in CI, failing two of the issue's acceptance criteria.
Resulting stack:
main<-fix-pyo3-errno(#268) <-ci-build-rust-extension(#269) <- this branch.
gh stackcould not adopt this branch cleanly —gh stack addonly createsnew branches rather than adopting one that already carries commits, and
gh stack initwould have rewritten the membership of the live stack behind#268 and #269. The base is therefore set directly with
gh pr create --base ci-build-rust-extension.Overlap with
test_rust_errno.py#268 added
test_consume_error_reports_a_branchable_errno, which also callsrust_consume_streamon a closed descriptor. The two are kept, and eachdocstring now states what it covers that the other does not:
test_rust_errno.pypins the errno conversion contract — the exactPOSIX number,
strerrorpopulation, subclass selection, and messageformatting. The whole module skips on Windows for that reason.
beside the rest of that entry point's coverage, and stays platform neutral.
Module split
test_rust_streams.pywas at 394 lines against an enforced pylintmax-module-lines = 400, leaving no room. The ADR-002 integration guard itcarried — an AST scan asserting production code does not yet route through
rust_consume_stream, plus the docstring-status check — never touches thecompiled extension and does not belong in an extension-gated module. It moves
to
cuprum/unittests/test_rust_consume_integration_guard.py, with the ADR androadmap references repointed. The guards still run under the ordinary test
glob and are deliberately left outwith
EXTENSION_TEST_TARGETS. The maturinwheel-manifest snapshot is regenerated for the new file.
Verification
Built with
make develop, thenmake test-extension:81 passed, 0 skipped, with
TestRustConsumeStream::test_propagates_io_errors PASSED.Mutation-checked for vacuity: reducing
io_error_to_py_errinrust/cuprum-rust/src/errors.rstoerr.into()makes the test fail withassert None in {9, 22}—OSError('Bad file descriptor (os error 9)').errnois
None. Restored, and the test passes again.Gates green:
make check-fmt,make lint,make typecheck,make test,make markdownlint,make nixie, pluscs deltawith no findings. The built.sowas removed anduv sync --group devre-run before committing;_rust_backend.is_available()reportsFalseand no.sois committed.References
Summary by Sourcery
Add platform-neutral coverage for rust_consume_stream I/O failures while separating integration guard tests into their own module.
New Features:
Enhancements:
Documentation:
Tests: