Skip to content

Add maturin and PyO3 compatibility tests - #22

Merged
leynos merged 20 commits into
mainfrom
test/maturin-pyo3-test-upgrade
Jun 12, 2026
Merged

Add maturin and PyO3 compatibility tests#22
leynos merged 20 commits into
mainfrom
test/maturin-pyo3-test-upgrade

Conversation

@lodyai

@lodyai lodyai Bot commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

Summary

This branch pins maturin to 1.13.3 and adds compatibility tests so future maturin and PyO3 upgrades can be validated through focused wheel-build and compile-time checks before the full release path fails.

Execplan: docs/execplans/maturin-pyo3-test-upgrade.md

Review walkthrough

Validation

  • make markdownlint: passed
  • make nixie: passed
  • make check-fmt: passed
  • make lint: passed
  • make typecheck: passed
  • make test: passed; nextest reported 86 Rust tests passing, and pytest reported 85 Python tests passing with 4 snapshots passing

Notes

No issue or roadmap task was identified for this branch. The pull request implements the linked ExecPlan rather than carrying a pre-implementation plan only.

Summary by Sourcery

Pin maturin to a specific version and add compatibility tests and documentation to validate future maturin and PyO3 upgrades.

New Features:

  • Add Python-based compatibility tests and snapshotting for maturin-built native wheels, including synchronized version pin checks and normalized wheel metadata/layout snapshots.
  • Introduce Rust trybuild-based compile-time UI tests for key PyO3 macro patterns used by the bridge crate.

Enhancements:

  • Pin maturin to version 1.13.3 in both development and build-system dependencies and document the synchronized pinning requirements for maintainers.
  • Document the maturin and PyO3 compatibility testing strategy, including how to run and update wheel snapshots and trybuild UI fixtures, and link the associated ExecPlan from the docs index.

Documentation:

  • Add a new execplan describing the maturin and PyO3 compatibility test upgrade and wire it into the documentation contents index.
  • Update the developer guide with maturin pin requirements and detailed instructions for running and maintaining the new compatibility tests.

Tests:

  • Add Python tests and support utilities to verify maturin pin synchronization, installed version alignment, and native wheel build output via syrupy snapshots.
  • Add Rust trybuild-based UI tests, including pass and fail fixtures, to enforce compile-time PyO3 module and detach usage contracts.

@coderabbitai

coderabbitai Bot commented Jun 4, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@lodyai[bot], we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 36 minutes and 1 second. Learn how PR review limits work.

Your organization has run out of usage credits. Purchase more credits in the billing tab to continue.

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: a390226c-cabd-4daf-8cc4-d778346bcddc

📥 Commits

Reviewing files that changed from the base of the PR and between e97db9d and e933fcf.

⛔ Files ignored due to path filters (2)
  • Cargo.lock is excluded by !**/*.lock
  • uv.lock is excluded by !**/*.lock
📒 Files selected for processing (18)
  • Cargo.toml
  • crates/stilyagi-pyext/Cargo.toml
  • crates/stilyagi-pyext/tests/compile_tests.rs
  • crates/stilyagi-pyext/tests/ui/fail/invalid_pymodule_arg.rs
  • crates/stilyagi-pyext/tests/ui/fail/invalid_pymodule_arg.stderr
  • crates/stilyagi-pyext/tests/ui/fail/invalid_pymodule_return.rs
  • crates/stilyagi-pyext/tests/ui/fail/invalid_pymodule_return.stderr
  • crates/stilyagi-pyext/tests/ui/fail/pymodule_three_args.rs
  • crates/stilyagi-pyext/tests/ui/fail/pymodule_three_args.stderr
  • crates/stilyagi-pyext/tests/ui/pass/pyo3_detach.rs
  • crates/stilyagi-pyext/tests/ui/pass/pyo3_pymodule.rs
  • docs/contents.md
  • docs/developers-guide.md
  • docs/execplans/maturin-pyo3-test-upgrade.md
  • pyproject.toml
  • tests/__snapshots__/test_maturin_build.ambr
  • tests/test_maturin_build.py
  • tests/test_maturin_build_errors.py

Walkthrough

Pin maturin to 1.13.3, add trybuild as a workspace/crate dev dependency, add Python wheel build and snapshot tests, add Rust trybuild UI compile-pass/fail fixtures for PyO3, and update developer docs and an ExecPlan.

Changes

Maturin pinning and PyO3 compatibility testing

Layer / File(s) Summary
Maturin pin and trybuild dependency setup
pyproject.toml, Cargo.toml, crates/stilyagi-pyext/Cargo.toml
maturin==1.13.3 pinned in dependency-groups.dev and [build-system].requires; trybuild = "1.0.116" added to workspace dependencies and trybuild.workspace = true set in crate dev-dependencies.
Rust compile-time UI tests for PyO3 compatibility
crates/stilyagi-pyext/tests/compile_tests.rs, crates/stilyagi-pyext/tests/ui/pass/*.rs, crates/stilyagi-pyext/tests/ui/fail/*.rs, *.stderr
Add trybuild harness that runs compile-pass fixtures (py.detach example, correct pymodule) and compile-fail fixtures (invalid pymodule arg types, return type, arity) with recorded stderr expectations.
Python maturin helpers and wheel introspection
tests/test_maturin_build.py, tests/__snapshots__/test_maturin_build.ambr
Add helpers to read maturin pins from pyproject.toml, detect toolchain and maturin availability, run python -m maturin build --release with pinned manifest, and produce a normalised wheel snapshot including generator and Root-Is-Purelib.
Pytest assertions for maturin pins and wheel snapshots
tests/test_maturin_build.py, tests/__snapshots__/test_maturin_build.ambr
Add tests asserting maturin pin synchronisation, optional installed maturin version check, wheel build snapshot assertion against syrupy snapshot, wheel install+execute integration test, and parametrised negative tests for malformed pins and corrupted wheels.
Developer guide updates and compatibility ExecPlan
docs/contents.md, docs/developers-guide.md, docs/execplans/maturin-pyo3-test-upgrade.md
Update docs and contents with the new maturin requirement, document trybuild UI test workflow and snapshot refresh procedure, and add a detailed ExecPlan capturing scope, constraints, progress, and outcomes.

Possibly related PRs

  • leynos/cuprum#59: Add trybuild-based PyO3 UI tests and stderr fixtures similar to this PR’s Rust UI tests.

Poem

Lock maturin tight and build the wheel,
Run trybuild checks where macros squeal,
Snapshot the wheel, assert its trace,
Fail and pass fixtures show the case,
Update docs and seal the place.

🚥 Pre-merge checks | ✅ 18 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 61.54% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Developer Documentation ⚠️ Warning The execplan contradicts the actual pins: it says uv.lock resolves maturin 1.13.1, but pyproject.toml and uv.lock pin maturin to 1.13.3. Edit docs/execplans/maturin-pyo3-test-upgrade.md to replace the stale “1.13.1” baseline with the correct 1.13.3 pin and keep the decision log/progress checklist consistent.
✅ Passed checks (18 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarises the main changeset, which pins maturin and adds comprehensive compatibility tests across Python and Rust.
Description check ✅ Passed The description is thorough and directly related to the changeset, covering maturin pinning, Python and Rust test additions, documentation updates, and validation results.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Testing (Overall) ✅ Passed PASS: trybuild fixtures use real pass/compile-fail .stderr oracles, and Python wheel tests build a wheel, snapshot parsed metadata, install it, and assert runtime extension outputs.
User-Facing Documentation ✅ Passed PR #22 updates maturin/trybuild compatibility tests and maintainer docs only (docs/contents, docs/developers-guide, execplan); GitHub file list shows no docs/users-guide.md or README changes.
Module-Level Documentation ✅ Passed All PR-changed modules have module-level documentation: tests/test_maturin_build.py uses a top triple-quote docstring; Rust fixtures start with //! module docs.
Testing (Unit And Behavioural) ✅ Passed PASS: tests/test_maturin_build.py end-to-end builds a wheel, snapshots metadata/layout, installs via uv, imports and asserts supported_syntaxes; includes pytest.raises error-path tests. crates/stil...
Testing (Property / Proof) ✅ Passed No proptest/Hypothesis/Kani/CrossHair/Verus/fast-check/verus-style property testing or proofs were introduced; the change adds trybuild UI compile checks and fixed wheel snapshot assertions.
Testing (Compile-Time / Ui) ✅ Passed trybuild is wired via compile_tests.rs (pass + compile_fail globs) and wheel UI output uses focused, normalised snapshots with pinned generator and redacted platform/tag fields.
Unit Architecture ✅ Passed Reviewed tests/test_maturin_build.py + compile_tests.rs: read/query helpers only read/parse files/zips; side effects (mkdir/unlink, maturin/uv subprocess calls) are confined to build and runtim...
Domain Architecture ✅ Passed PR adds trybuild compile-time UI fixtures and maturin/PyO3 wheel compatibility tests plus docs/config; it doesn’t change core domain model logic or mix HTTP/SQL/IO concerns into domain code.
Observability ✅ Passed PR adds trybuild UI compile tests and maturin wheel-snapshot tests (dev-only). No production code changes adding logging/metrics/tracing; failures remain visible via pytest/trybuild .stderr fixtures.
Security And Privacy ✅ Passed Reviewed new/updated files (tests/test_maturin_build.py, trybuild UI fixtures, Cargo/pyproject, docs, snapshot); no secrets/credentials, no shell=True, subprocess uses trusted argv lists, and parsi...
Performance And Resource Use ✅ Passed PASS: test_maturin_build builds one wheel via module fixture, then reads only WHEEL/METADATA from the zip (bounded). trybuild fixtures are finite (2 pass, 3 fail UI files).
Concurrency And State ✅ Passed Inspected new/changed code (trybuild compile_tests.rs and tests/test_maturin_build.py + UI fixtures): no async/threads/locks or shared mutable globals; shared state is confined to a single module f...
Architectural Complexity And Maintainability ✅ Passed PASS: Adds localized trybuild UI harness and small, named helpers for wheel/snapshot parsing; no new orchestration/lifecycle layers, no registries/state machines, and functions stay short and expli...
Rust Compiler Lint Integrity ✅ Passed Reviewed PR Rust changes (compile_tests.rs and all crates/stilyagi-pyext/tests/ui/*.rs): no #[allow]/#![allow] or #[expect] suppressions and no .clone() usage detected; code has no obvious unused-c...

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch test/maturin-pyo3-test-upgrade

Comment @coderabbitai help to get the list of available commands and usage tips.

@sourcery-ai

sourcery-ai Bot commented Jun 4, 2026

Copy link
Copy Markdown

Reviewer's Guide

Pins maturin to 1.13.3 across tooling and build-system, adds Python helpers and tests to snapshot native wheel metadata, introduces Rust trybuild-based PyO3 compile-time UI tests, and documents the compatibility-testing and upgrade workflow for future maturin/PyO3 changes.

Flow diagram for Python maturin pin and wheel snapshot tests

flowchart TD
    Developer --> pytest_maturin_build[pytest tests/test_maturin_build.py]

    pytest_maturin_build --> pin_sync_checks[test_maturin_pins_in_sync and related pin checks]
    pin_sync_checks --> pyproject_pins[pyproject.toml maturin==1.13.3 pins]

    pytest_maturin_build --> wheel_snapshot_test[test_maturin_wheel_build_snapshot]
    wheel_snapshot_test --> maturin_cli[maturin build via tests/support/maturin.py]
    maturin_cli --> native_wheel[normalized native wheel]
    native_wheel --> syrupy_snapshot[snapshot comparison in test_maturin_wheel_build_snapshot]
Loading

Flow diagram for Rust trybuild-based PyO3 compile-time UI tests

flowchart TD
    Developer --> cargo_test_compile_time_ui[cargo test -p stilyagi-pyext compile_time_ui]

    cargo_test_compile_time_ui --> trybuild_harness[compile_time_ui in crates/stilyagi-pyext/tests/compile_tests.rs]
    trybuild_harness --> pass_fixtures[tests/ui/pass/*.rs]
    trybuild_harness --> fail_fixtures[tests/ui/fail/*.rs]

    pass_fixtures --> rustc_pass[trybuild compile pass]
    fail_fixtures --> rustc_fail[trybuild compile fail with diagnostics]
    rustc_fail --> stderr_expectations[*.stderr expectation files]
Loading

File-Level Changes

Change Details Files
Pin maturin consistently in project configuration and lockfiles.
  • Update developer docs to require maturin 1.13.3 and describe pinning rationale.
  • Change pyproject dev dependency group and build-system requirements to use an exact maturin==1.13.3 pin with cross-references between the two locations.
  • Refresh uv.lock and Cargo.lock to reflect the updated maturin and Rust dev-test dependencies.
docs/developers-guide.md
pyproject.toml
uv.lock
Cargo.lock
Add Python helpers and tests to validate maturin pin synchronization and snapshot native wheel builds.
  • Introduce tests/support/maturin.py with TOML readers, installed-version checks, a pinned maturin build wrapper, and wheel metadata/layout normalization helpers.
  • Add pytest tests for synchronized maturin pins, installed maturin version alignment with the pin, and a syrupy-backed wheel snapshot regression check with timeout disabled for long native builds.
  • Add the corresponding wheel snapshot artifact for regression comparison.
tests/support/maturin.py
tests/test_maturin_build.py
tests/__snapshots__/test_maturin_build.ambr
Introduce Rust trybuild-based UI tests to enforce PyO3 macro and module-setup contracts in the bridge crate.
  • Add trybuild as a workspace dev dependency and wire it into the stilyagi-pyext crate for testing.
  • Create a compile_tests.rs harness that runs pass and compile-fail UI fixtures via trybuild.
  • Add PyO3 compile-pass fixtures covering detach usage and well-formed #[pymodule] plus #[pyfunction] registrations.
  • Add a compile-fail fixture and stderr expectation asserting that a #[pymodule] function returning i32 is rejected by PyO3.
Cargo.toml
crates/stilyagi-pyext/Cargo.toml
crates/stilyagi-pyext/tests/compile_tests.rs
crates/stilyagi-pyext/tests/ui/pass/pyo3_detach.rs
crates/stilyagi-pyext/tests/ui/pass/pyo3_pymodule.rs
crates/stilyagi-pyext/tests/ui/fail/invalid_pymodule_return.rs
crates/stilyagi-pyext/tests/ui/fail/invalid_pymodule_return.stderr
Document the maturin/PyO3 compatibility testing workflow and link it into the docs index.
  • Add an ExecPlan describing the maturin pin update, compatibility-testing strategy, risks, decisions, and verification steps.
  • Extend the developer guide with a dedicated section on maturin and PyO3 compatibility tests, including commands for updating wheel snapshots and trybuild stderr expectations.
  • Register the new ExecPlan in the docs contents index under the roadmap and execution plans section.
docs/execplans/maturin-pyo3-test-upgrade.md
docs/developers-guide.md
docs/contents.md

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

codescene-delta-analysis[bot]

This comment was marked as outdated.

codescene-delta-analysis[bot]

This comment was marked as outdated.

@leynos
leynos marked this pull request as ready for review June 9, 2026 16:54
sourcery-ai[bot]

This comment was marked as resolved.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@docs/execplans/maturin-pyo3-test-upgrade.md`:
- Line 8: Update the document's status line from the non-terminal value to a
terminal one: replace "Status: IN PROGRESS" with "Status: COMPLETED" (or another
terminal value) so the Status header matches the later documented outcomes and
finality in the ExecPlan.

In `@tests/support/maturin.py`:
- Around line 82-87: The installed_maturin_version helper can raise
importlib.metadata.PackageNotFoundError when the maturin module is importable
but distribution metadata is missing; update the installed_maturin_version
function to call im.version("maturin") inside a try/except that catches
im.PackageNotFoundError and returns None in that case so the helper remains
stable (refer to function installed_maturin_version and the im.version call).
- Around line 112-135: In build_native_wheel_artifact, stale wheel files in
out_dir can cause the wheels list check to fail; before running maturin (i.e.,
before the command = [...] and subprocess.run call), remove any existing "*.whl"
files from out_dir by iterating out_dir.glob("*.whl") and calling unlink() on
each so the post-build wheels = sorted(out_dir.glob("*.whl")) reflects only the
newly produced wheel; keep out_dir.mkdir(...) and the rest of the function
unchanged.
- Line 3: Remove the top-level "from __future__ import annotations" and import
pathlib unconditionally (move the pathlib import out of the TYPE_CHECKING block)
so runtime annotations are safe; update installed_maturin_version() to catch
importlib.metadata.PackageNotFoundError around im.version("maturin") and return
None or a sensible fallback when metadata is missing (this hardens it relative
to _maturin_module_available() which uses find_spec); and make
build_native_wheel_artifact() robust by cleaning out_dir before running maturin
(remove or move existing *.whl files) and stop asserting exactly one
wheel—either select the newest/most-recent *.whl or explicitly control the
output filename so stale wheels cannot make the test flaky.

In `@tests/test_maturin_build.py`:
- Line 3: Remove the forbidden future import added in
tests/test_maturin_build.py: delete the line "from __future__ import
annotations" so the file no longer contains that import; ensure no other code
depends on postponed evaluation of annotations (adjust any type hints if
necessary) and run tests to confirm nothing breaks.
🪄 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: d1951aac-f5dc-4fc5-8918-1467a582c459

📥 Commits

Reviewing files that changed from the base of the PR and between 3e8b7da and 5badd29.

⛔ Files ignored due to path filters (2)
  • Cargo.lock is excluded by !**/*.lock
  • uv.lock is excluded by !**/*.lock
📒 Files selected for processing (14)
  • Cargo.toml
  • crates/stilyagi-pyext/Cargo.toml
  • crates/stilyagi-pyext/tests/compile_tests.rs
  • crates/stilyagi-pyext/tests/ui/fail/invalid_pymodule_return.rs
  • crates/stilyagi-pyext/tests/ui/fail/invalid_pymodule_return.stderr
  • crates/stilyagi-pyext/tests/ui/pass/pyo3_detach.rs
  • crates/stilyagi-pyext/tests/ui/pass/pyo3_pymodule.rs
  • docs/contents.md
  • docs/developers-guide.md
  • docs/execplans/maturin-pyo3-test-upgrade.md
  • pyproject.toml
  • tests/__snapshots__/test_maturin_build.ambr
  • tests/support/maturin.py
  • tests/test_maturin_build.py

Comment thread docs/execplans/maturin-pyo3-test-upgrade.md Outdated
Comment thread tests/support/maturin.py Outdated
Comment thread tests/support/maturin.py Outdated
Comment thread tests/support/maturin.py Outdated
Comment thread tests/test_maturin_build.py Outdated
codescene-delta-analysis[bot]

This comment was marked as outdated.

@leynos

leynos commented Jun 9, 2026

Copy link
Copy Markdown
Owner

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jun 9, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai coderabbitai Bot added the Roadmap label Jun 9, 2026
@leynos

leynos commented Jun 9, 2026

Copy link
Copy Markdown
Owner

@coderabbitai resume

@coderabbitai

coderabbitai Bot commented Jun 9, 2026

Copy link
Copy Markdown
✅ Action performed

Reviews resumed.

codescene-delta-analysis[bot]

This comment was marked as outdated.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@tests/test_maturin_build.py`:
- Around line 29-33: The uv_candidates list currently builds two paths via
f-strings; replace those with pathlib construction using REPOSITORY_ROOT /
".uv-tools" / "uv" and REPOSITORY_ROOT / ".venv" / "bin" / "uv" and pass their
str() to any API that requires a string (e.g., shutil.which). Update the
uv_candidates assignment in tests/test_maturin_build.py to use
str(REPOSITORY_ROOT / ".uv-tools" / "uv") and str(REPOSITORY_ROOT / ".venv" /
"bin" / "uv") instead of the f-strings so all path manipulation uses pathlib.
🪄 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: e5b9fc53-411d-454c-aeaf-30f411ab292d

📥 Commits

Reviewing files that changed from the base of the PR and between 7268b03 and 824e217.

📒 Files selected for processing (5)
  • crates/stilyagi-pyext/tests/ui/fail/invalid_pymodule_arg.rs
  • crates/stilyagi-pyext/tests/ui/fail/invalid_pymodule_arg.stderr
  • crates/stilyagi-pyext/tests/ui/fail/pymodule_three_args.rs
  • crates/stilyagi-pyext/tests/ui/fail/pymodule_three_args.stderr
  • tests/test_maturin_build.py

Comment thread tests/test_maturin_build.py Outdated
@leynos

leynos commented Jun 9, 2026

Copy link
Copy Markdown
Owner

@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response.

tests/test_maturin_build.py

Comment on lines +163 to +173

def test_read_maturin_pins_raises_when_no_maturin_pin(tmp_path: pathlib.Path) -> None:
    """read_maturin_pins raises AssertionError when no maturin pin is found."""
    pyproject = tmp_path / "pyproject.toml"
    pyproject.write_text(
        "[dependency-groups]\n"
        'dev = ["pytest==8.4.2"]\n'
        "[build-system]\n"
        'requires = ["setuptools"]\n'
    )
    with pytest.raises(AssertionError, match="Could not locate"):
        read_maturin_pins(tmp_path)

❌ New issue: Code Duplication
The module contains 2 functions with similar structure: test_read_maturin_pins_raises_when_no_maturin_pin,test_read_maturin_pins_raises_when_requires_not_a_list

@coderabbitai

This comment was marked as resolved.

codescene-delta-analysis[bot]

This comment was marked as outdated.

codescene-delta-analysis[bot]

This comment was marked as outdated.

@leynos

leynos commented Jun 9, 2026

Copy link
Copy Markdown
Owner

@coderabbitai resume

@coderabbitai

coderabbitai Bot commented Jun 9, 2026

Copy link
Copy Markdown
✅ Action performed

Reviews resumed.

@leynos

This comment was marked as resolved.

@coderabbitai

This comment was marked as resolved.

@leynos

leynos commented Jun 10, 2026

Copy link
Copy Markdown
Owner

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jun 10, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai

This comment was marked as resolved.

@leynos

leynos commented Jun 10, 2026

Copy link
Copy Markdown
Owner

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jun 10, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@leynos

This comment was marked as resolved.

@leynos

This comment was marked as resolved.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
tests/test_maturin_build.py (2)

107-124: ⚠️ Potential issue | 🟠 Major

Assert the wheel Tag: from .dist-info/WHEEL instead of hard-coding.
Stop hard-coding "tag": "<platform-tag>"; _parse_wheel_header() only parses Generator: and Root-Is-Purelib:, so the tests never validate the actual Tag: line in the wheel metadata. Parse Tag: from .dist-info/WHEEL, fail if missing, and normalise only the machine-specific suffix before snapshotting.

🤖 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/test_maturin_build.py` around lines 107 - 124, The test currently
hard-codes "tag": "<platform-tag>" in _wheel_build_snapshot which prevents
validating the actual Tag: field from the .dist-info/WHEEL; update
_wheel_build_snapshot to extract the Tag(s) from the wheel_payload (the same
string you pass into _parse_wheel_header), raise an AssertionError if no Tag
line is present, normalize any machine-specific suffixes (e.g., platform triples
or build IDs) from the parsed tag value(s) before snapshotting, and return that
normalized tag under "wheel": {"root_is_purelib": root_is_purelib, "tag":
parsed_and_normalized_tag} instead of the hard-coded value. Ensure you reference
the existing helpers (_parse_wheel_header and any normalization helper or add a
small normalizer) and keep the failure behavior consistent with the existing
metadata error handling.

44-53: ⚠️ Potential issue | 🟠 Major

Prefer checkout-local uv before ambient PATH in _resolve_uv().

In tests/test_maturin_build.py, _resolve_uv() checks "uv" (PATH) before .uv-tools/... and .venv/..., so test_maturin_wheel_executes_correctly can silently use a different uv than the repo-provided one.

♻️ Prefer the checkout-local executable first
-    uv_candidates = ["uv", *(str(path) for path in local_uv_paths)]
+    uv_candidates = [*(str(path) for path in local_uv_paths), "uv"]
🤖 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/test_maturin_build.py` around lines 44 - 53, The function _resolve_uv
currently lists the ambient "uv" first causing PATH to be preferred over
repo-local executables; change the uv_candidates construction in _resolve_uv so
local_uv_paths (from local_uv_paths / ".uv-tools" and ".venv") appear before the
generic "uv" entry (i.e., build uv_candidates as the local paths first, then
"uv") so shutil.which checks checkout-local executables before falling back to
PATH.
🤖 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 `@tests/test_maturin_build.py`:
- Around line 1-10: Update the module docstring in tests/test_maturin_build.py
to reference the actual pytest entry points used in this file instead of
"runtime_smoke_test" and "refresh_snapshot" (mention the test name
test_maturin_wheel_build_snapshot and the file's snapshot-update helper), and
replace the POSIX-only ".venv/bin/python" invocation with a portable "python -m
pytest ..." example; also add single-line NumPy-style summaries to the private
helper functions (e.g., _build_native_wheel_artifact, _wheel_build_snapshot and
the local refresh_snapshot helper) and ensure all docstrings follow the numpy
style guide so the wheel-build contract and snapshot workflow are clearly
documented.

---

Outside diff comments:
In `@tests/test_maturin_build.py`:
- Around line 107-124: The test currently hard-codes "tag": "<platform-tag>" in
_wheel_build_snapshot which prevents validating the actual Tag: field from the
.dist-info/WHEEL; update _wheel_build_snapshot to extract the Tag(s) from the
wheel_payload (the same string you pass into _parse_wheel_header), raise an
AssertionError if no Tag line is present, normalize any machine-specific
suffixes (e.g., platform triples or build IDs) from the parsed tag value(s)
before snapshotting, and return that normalized tag under "wheel":
{"root_is_purelib": root_is_purelib, "tag": parsed_and_normalized_tag} instead
of the hard-coded value. Ensure you reference the existing helpers
(_parse_wheel_header and any normalization helper or add a small normalizer) and
keep the failure behavior consistent with the existing metadata error handling.
- Around line 44-53: The function _resolve_uv currently lists the ambient "uv"
first causing PATH to be preferred over repo-local executables; change the
uv_candidates construction in _resolve_uv so local_uv_paths (from local_uv_paths
/ ".uv-tools" and ".venv") appear before the generic "uv" entry (i.e., build
uv_candidates as the local paths first, then "uv") so shutil.which checks
checkout-local executables before falling back to PATH.
🪄 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: 5f337531-1131-4fb1-84a3-1aa54904efff

📥 Commits

Reviewing files that changed from the base of the PR and between ea21118 and e97db9d.

📒 Files selected for processing (1)
  • tests/test_maturin_build.py

Comment thread tests/test_maturin_build.py

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
docs/developers-guide.md (1)

38-44: ⚠️ Potential issue | 🟡 Minor

Clarify that maturin is a hard pin, not a “minimum”.

docs/developers-guide.md labels maturin as part of “The minimum local setup”, but pyproject.toml pins it via build-system.requires = ['maturin==1.13.3']. Update the wording so it is explicitly a hard pin to 1.13.3.

Triage: type:docstyle

✏️ Suggested wording
- `maturin` 1.13.3
+ `maturin` 1.13.3 (pinned; keep this aligned with `[build-system].requires`)
🤖 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` around lines 38 - 44, Update the “The minimum local
setup” section to clarify that maturin is a hard pinned dependency: change the
line listing `maturin 1.13.3` to explicitly state that pyproject.toml hard-pins
maturin to version 1.13.3 (e.g., mention `build-system.requires =
['maturin==1.13.3']`) so readers understand this is a strict requirement rather
than a minimum version.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@docs/developers-guide.md`:
- Around line 498-503: Update the two bullet entries that currently read
`tests/ui/pass/` and `tests/ui/fail/` so they don't duplicate the fixture root
`crates/stilyagi-pyext/tests/ui/`; change them to simply `pass/` and `fail/` (or
alternatively replace the bullets with the full path
`crates/stilyagi-pyext/tests/ui/pass/` and
`crates/stilyagi-pyext/tests/ui/fail/`) so the paths correctly reference the
fixture root and no longer point one directory too deep.

In `@docs/execplans/maturin-pyo3-test-upgrade.md`:
- Around line 116-118: Change the sentence that reads "PyPI lists maturin
`1.13.3`, uploaded on 2026-05-11, as the current release. Stilyagi's `uv.lock`
currently resolves maturin `1.13.1` ..." into a retrospective note: state that
PyPI had `1.13.3` on 2026-05-11 and that, prior to pinning, `uv.lock` resolved
`maturin 1.13.1` (or explicitly prefix with "pre-pin baseline:"), and mention
that the repository now pins maturin to `1.13.3` so the live-state claim is no
longer accurate.

In `@tests/test_maturin_build.py`:
- Around line 223-229: The module-scoped fixture built_wheel currently only
calls _toolchain_available() and may build with a wrong maturin; before calling
_build_native_wheel_artifact, compare _installed_maturin_version() to
_read_expected_maturin_version(REPOSITORY_ROOT) and if they differ call
pytest.skip with a clear message about the pinned maturin mismatch so the
expensive wheel build is avoided when the active maturin version is not the
expected one.

---

Outside diff comments:
In `@docs/developers-guide.md`:
- Around line 38-44: Update the “The minimum local setup” section to clarify
that maturin is a hard pinned dependency: change the line listing `maturin
1.13.3` to explicitly state that pyproject.toml hard-pins maturin to version
1.13.3 (e.g., mention `build-system.requires = ['maturin==1.13.3']`) so readers
understand this is a strict requirement rather than a minimum version.
🪄 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: 38cd711b-4a78-4cdd-903e-32fa56900ddd

📥 Commits

Reviewing files that changed from the base of the PR and between e97db9d and a4ef01f.

⛔ Files ignored due to path filters (2)
  • Cargo.lock is excluded by !**/*.lock
  • uv.lock is excluded by !**/*.lock
📒 Files selected for processing (17)
  • Cargo.toml
  • crates/stilyagi-pyext/Cargo.toml
  • crates/stilyagi-pyext/tests/compile_tests.rs
  • crates/stilyagi-pyext/tests/ui/fail/invalid_pymodule_arg.rs
  • crates/stilyagi-pyext/tests/ui/fail/invalid_pymodule_arg.stderr
  • crates/stilyagi-pyext/tests/ui/fail/invalid_pymodule_return.rs
  • crates/stilyagi-pyext/tests/ui/fail/invalid_pymodule_return.stderr
  • crates/stilyagi-pyext/tests/ui/fail/pymodule_three_args.rs
  • crates/stilyagi-pyext/tests/ui/fail/pymodule_three_args.stderr
  • crates/stilyagi-pyext/tests/ui/pass/pyo3_detach.rs
  • crates/stilyagi-pyext/tests/ui/pass/pyo3_pymodule.rs
  • docs/contents.md
  • docs/developers-guide.md
  • docs/execplans/maturin-pyo3-test-upgrade.md
  • pyproject.toml
  • tests/__snapshots__/test_maturin_build.ambr
  • tests/test_maturin_build.py

Comment thread docs/developers-guide.md Outdated
Comment thread docs/execplans/maturin-pyo3-test-upgrade.md Outdated
Comment thread tests/test_maturin_build.py
@lodyai

lodyai Bot commented Jun 11, 2026

Copy link
Copy Markdown
Contributor Author

@coderabbitai resume

Clarify that the local maturin dependency is hard-pinned through
`pyproject.toml`, and fix the documented trybuild fixture paths so they are
relative to the stated fixture root.

Skip the shared native wheel build when the active maturin version does not
match the pinned build backend, avoiding an expensive compatibility build under
the wrong toolchain.
codescene-delta-analysis[bot]

This comment was marked as outdated.

Parse the actual `Tag` metadata from built wheels and snapshot the normalized
value instead of a fixed placeholder. Prefer checkout-local `uv` executables
before falling back to the ambient PATH.

Refresh the maturin test module guidance and move malformed-input cases into a
focused companion test file so each test module stays within the project line
limit.
codescene-delta-analysis[bot]

This comment was marked as outdated.

@leynos

leynos commented Jun 11, 2026

Copy link
Copy Markdown
Owner

@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response.

tests/test_maturin_build.py

Comment on lines +210 to +230

def _parse_wheel_header(
    wheel_payload: str, whl_path: pathlib.Path
) -> tuple[str, str, list[str]]:
    generator_match = _GENERATOR_RE.search(wheel_payload)
    if generator_match is None:
        msg = f"Could not parse maturin generator from WHEEL metadata: {whl_path}"
        raise AssertionError(msg)
    root_is_purelib: str | None = None
    tags: list[str] = []
    for line in wheel_payload.splitlines():
        if line.startswith("Root-Is-Purelib:"):
            root_is_purelib = line.removeprefix("Root-Is-Purelib: ")
        elif line.startswith("Tag:"):
            tags.append(_normalize_wheel_tag(line.removeprefix("Tag: ")))
    if root_is_purelib is None:
        msg = "wheel is missing Root-Is-Purelib metadata"
        raise AssertionError(msg)
    if not tags:
        msg = "wheel is missing Tag metadata"
        raise AssertionError(msg)
    return generator_match.group(1), root_is_purelib, sorted(tags)

❌ New issue: Complex Method
_parse_wheel_header has a cyclomatic complexity of 10, threshold = 9

@coderabbitai

This comment was marked as resolved.

@leynos

leynos commented Jun 11, 2026

Copy link
Copy Markdown
Owner

@coderabbitai resume

@coderabbitai

coderabbitai Bot commented Jun 11, 2026

Copy link
Copy Markdown
✅ Action performed

Reviews resumed.

Move Root-Is-Purelib and Tag scanning out of `_parse_wheel_header` so the
parser only validates the generator header and delegates line-field extraction.
This keeps the maturin wheel helper below the complexity threshold while
preserving the existing error paths.
codescene-delta-analysis[bot]

This comment was marked as outdated.

@leynos

leynos commented Jun 11, 2026

Copy link
Copy Markdown
Owner

@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.

Developer Documentation ⚠️ Warning The execplan contradicts the actual pins: it says uv.lock resolves maturin 1.13.1, but pyproject.toml and uv.lock pin maturin to 1.13.3. Edit docs/execplans/maturin-pyo3-test-upgrade.md to replace the stale “1.13.1” baseline with the correct 1.13.3 pin and keep the decision log/progress checklist consistent.

@coderabbitai

This comment was marked as resolved.

Update the maturin upgrade execplan so the discovery records the earlier
`1.13.1` lock resolution as historical context and points to the final exact
`1.13.3` pin introduced by this PR.
@coderabbitai

coderabbitai Bot commented Jun 11, 2026

Copy link
Copy Markdown

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
echo "=== tests/test_maturin_build.py lines 25-45 ==="
sed -n '25,45p' tests/test_maturin_build.py

echo ""
echo "=== invalid_pymodule_arg.rs ==="
cat crates/stilyagi-pyext/tests/ui/fail/invalid_pymodule_arg.rs

echo ""
echo "=== invalid_pymodule_return.rs ==="
cat crates/stilyagi-pyext/tests/ui/fail/invalid_pymodule_return.rs

echo ""
echo "=== pymodule_three_args.rs ==="
cat crates/stilyagi-pyext/tests/ui/fail/pymodule_three_args.rs

Length of output: 2891


🏁 Script executed:

#!/bin/bash
echo "=== tests/test_maturin_build.py lines 29-60 ==="
sed -n '29,60p' tests/test_maturin_build.py

Length of output: 1740



@leynos: All three checks are settled.

Check Status Notes
Unit Architecture (pathlib f-strings) ✅ Resolved _resolve_uv uses root / ".uv-tools" / uv_name with str() throughout — no f-string path construction anywhere
Architectural Complexity warning (same) ✅ Resolved Same fix covers it
Rust Compiler Lint Integrity ✅ Dismissed fn main() {} is genuinely required in these compile-fail fixtures, as confirmed during this PR; the check's premise was incorrect

No further work is required on any of these items.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant