Property tests for path/git-ref validators and backend resolver (#70, #72) - #224
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
|
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:
WalkthroughSeparate Rust availability probing from backend resolution, add public path and git-ref rejection classifiers, and introduce Hypothesis property tests covering validators, backend selection, environment parsing, documentation, and wheel contents. ChangesBackend and argument validation
Sequence Diagram(s)sequenceDiagram
participant get_stream_backend
participant _probe_rust_availability
participant _resolve_backend
get_stream_backend->>_probe_rust_availability: Probe Rust availability
_probe_rust_availability-->>get_stream_backend: Return availability state
get_stream_backend->>_resolve_backend: Resolve requested backend
_resolve_backend-->>get_stream_backend: Return Rust/Python or raise ImportError
Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 19 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (19 passed)
📋 Issue PlannerBuilt with CodeRabbit's Coding Plans for faster development and fewer bugs. View plan used: ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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.
![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: 1637734d72
ℹ️ 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".
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cuprum/_backend.py (1)
211-286: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMove the Rust probe inside the
tryblock (cuprum/_backend.py:272-283). ForcedRUSTfailures can raise from_probe_rust_availability()before theexcept ImportErrorhandler, so the "Rust stream backend requested but unavailable" warning never runs on that path. Initialiserust_available = None, call the probe inside thetry, and log both forced-backend failure paths.🤖 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/_backend.py` around lines 211 - 286, Update get_stream_backend so rust_available is initialized to None and _probe_rust_availability(requested) runs inside the existing try block before _resolve_backend. Keep the ImportError warning handler covering both probe and resolution failures, then preserve normal resolution logging and return behavior.
🤖 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_backend_resolver_property.py`:
- Around line 101-121: Update test_read_backend_env_rejects_invalid_values so
the valid branch asserts the exact expected StreamBackend member for each
normalized input, rather than only checking membership in set(StreamBackend).
Preserve the existing AUTO checks for empty and "auto", and verify every other
valid value maps to its corresponding enum member.
---
Outside diff comments:
In `@cuprum/_backend.py`:
- Around line 211-286: Update get_stream_backend so rust_available is
initialized to None and _probe_rust_availability(requested) runs inside the
existing try block before _resolve_backend. Keep the ImportError warning handler
covering both probe and resolution failures, then preserve normal resolution
logging and 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: b2101eb8-a093-4e90-9dd9-8f3b4552cb47
📒 Files selected for processing (5)
cuprum/_backend.pycuprum/builders/args.pycuprum/unittests/__snapshots__/test_maturin_build.ambrcuprum/unittests/test_args_validators_property.pycuprum/unittests/test_backend_resolver_property.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.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@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/_backend.py`:
- Around line 48-81: The private helper _parse_backend_value has an
unnecessarily elaborate public-style docstring; replace it with a concise
single-line summary. Apply the same documentation-only change to the private
helper _resolve_backend, removing its Parameters, Returns, Raises, and Examples
sections while preserving both implementations unchanged.
- Around line 298-313: Update get_stream_backend so its try/except ImportError
encloses both _probe_rust_availability and _resolve_backend, ensuring
probe-raised failures receive the same cuprum.stream_backend_unavailable warning
and structured context as resolution failures. Preserve the existing requested,
rust_available, and resolved values for logging, while handling the
probe-failure case without referencing an uninitialized rust_available value.
In `@cuprum/unittests/test_args_validators_property.py`:
- Line 55: Update every assertion in the property tests, including the
assertions around result types, accepted and rejected paths, and validator
outcomes, to include a concise diagnostic message using the assertion’s
invariant. Preserve each existing assertion condition and add messages that make
shrunk Hypothesis counterexamples identify the failed expectation.
In `@cuprum/unittests/test_backend_resolver_property.py`:
- Around line 37-119: Group the five property tests into related test classes,
such as resolver behavior and backend-value parsing, while preserving each
test’s existing assertions and Hypothesis decorators. Convert each function into
a test_ prefixed method within its appropriate class, including the existing
resolver helpers and parser symbols unchanged.
🪄 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: 1a24acdc-23ca-44ec-ae70-7133aa5378cc
📒 Files selected for processing (6)
cuprum/_backend.pycuprum/builders/args.pycuprum/unittests/__snapshots__/test_maturin_build.ambrcuprum/unittests/test_args_validators_property.pycuprum/unittests/test_backend_resolver_property.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)
| def test_classify_path_string_is_total(raw: str, *, allow_relative: bool) -> None: | ||
| """``classify_path_string`` returns a value or ``None`` but never raises.""" | ||
| result = classify_path_string(raw, allow_relative=allow_relative) | ||
| assert result is None or isinstance(result, PathRejection) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add diagnostic messages to every assertion.
Attach a failure message to each assertion so shrunk Hypothesis counterexamples identify the broken invariant immediately. As per coding guidelines, use assert …, "message" over bare asserts. As per path instructions, use assert …, "message" over bare asserts.
Also applies to: 65-65, 69-69, 75-75, 85-87, 96-100, 107-107, 115-119, 125-125, 137-138
🤖 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_args_validators_property.py` at line 55, Update every
assertion in the property tests, including the assertions around result types,
accepted and rejected paths, and validator outcomes, to include a concise
diagnostic message using the assertion’s invariant. Preserve each existing
assertion condition and add messages that make shrunk Hypothesis counterexamples
identify the failed expectation.
Sources: Coding guidelines, Path instructions
There was a problem hiding this comment.
@coderabbitai Has this now been resolved in the latest commit?
Use codegraph analysis to determine your answer.
If this comment is now resolved, please mark it as such using the API. Otherwise, please provide an AI agent prompt for the remaining work to be done to address this comment.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '== Current PR head =='
git rev-parse HEAD
echo '== Code-structure outline =='
ast-grep outline cuprum/unittests/test_args_validators_property.py --items all --type function
echo '== Assertion AST nodes =='
ast-grep run --pattern 'assert $COND, $MSG' --lang python cuprum/unittests/test_args_validators_property.py || true
echo '-- bare assertion AST nodes --'
ast-grep run --pattern 'assert $COND' --lang python cuprum/unittests/test_args_validators_property.py || true
echo '== Assertion source context =='
rg -n -C 1 '^\s*assert\b' cuprum/unittests/test_args_validators_property.py
echo '== Matching review threads =='
gh api graphql \
-F owner='leynos' \
-F name='cuprum' \
-F number=224 \
-f query='
query($owner: String!, $name: String!, $number: Int!) {
repository(owner: $owner, name: $name) {
pullRequest(number: $number) {
reviewThreads(first: 100) {
nodes {
id
isResolved
path
line
comments(first: 20) {
nodes {
author { login }
body
url
}
}
}
}
}
}
}' \
--jq '.data.repository.pullRequest.reviewThreads.nodes[]
| select(.path == "cuprum/unittests/test_args_validators_property.py")
| select(any(.comments.nodes[]; .author.login == "coderabbitai" and (.body | contains("Add diagnostic messages to every assertion."))))
| {id, isResolved, path, line, comments: [.comments.nodes[] | {author: .author.login, url}]}'Length of output: 7491
@leynos, leave this thread unresolved. Codegraph analysis on latest commit e6fd9b335ea0e228266d7b99d3419e1ba4bd576b found 18 bare assertions and no assertions with diagnostic messages in cuprum/unittests/test_args_validators_property.py, including the anchor at Line 59.
Use this AI-agent prompt:
In `cuprum/unittests/test_args_validators_property.py`, add a concise diagnostic message to every `assert` statement in the property-based tests.
Preserve every existing assertion condition and test control-flow branch. Make each message state the invariant being checked and include the relevant generated values where useful, so a shrunk Hypothesis counterexample is immediately actionable.
Cover all current assertions, including:
- classifier totality and return-type assertions;
- validator/classifier consistency and exception-message equality;
- NUL, parent-segment, whitespace, leading-dash, and non-absolute rejection-category assertions;
- valid path classification, normalization, and idempotence;
- accepted Git-ref classification and round-trip assertions.
Keep the change limited to assertion messages. Do not alter validator behaviour, strategies, or production code. Verify the final file contains no bare `assert` statements.
| @given(requested=_REQUESTED, rust_available=_AVAILABILITY) | ||
| def test_resolver_never_returns_auto( | ||
| requested: StreamBackend, | ||
| rust_available: bool | None, # noqa: FBT001 - property input, not a flag. | ||
| ) -> None: | ||
| """The resolver always collapses to a concrete backend (never ``AUTO``).""" | ||
| try: | ||
| resolved = _resolve_backend(requested, rust_available=rust_available) | ||
| except ImportError: | ||
| # Forced Rust without availability is the only permitted failure. | ||
| assert requested is StreamBackend.RUST | ||
| assert not rust_available | ||
| return | ||
| assert resolved in {StreamBackend.RUST, StreamBackend.PYTHON} | ||
|
|
||
|
|
||
| @given(rust_available=_AVAILABILITY) | ||
| def test_python_is_honoured_unconditionally( | ||
| rust_available: bool | None, # noqa: FBT001 - property input, not a flag. | ||
| ) -> None: | ||
| """Requesting ``PYTHON`` always resolves to ``PYTHON``.""" | ||
| assert ( | ||
| _resolve_backend(StreamBackend.PYTHON, rust_available=rust_available) | ||
| is StreamBackend.PYTHON | ||
| ) | ||
|
|
||
|
|
||
| @given(rust_available=_AVAILABILITY) | ||
| def test_forced_rust_respects_availability( | ||
| rust_available: bool | None, # noqa: FBT001 - property input, not a flag. | ||
| ) -> None: | ||
| """Forced ``RUST`` resolves iff available, else raises ``ImportError``.""" | ||
| if rust_available: | ||
| assert ( | ||
| _resolve_backend(StreamBackend.RUST, rust_available=rust_available) | ||
| is StreamBackend.RUST | ||
| ) | ||
| else: | ||
| with pytest.raises(ImportError, match="Rust extension is not available"): | ||
| _resolve_backend(StreamBackend.RUST, rust_available=rust_available) | ||
|
|
||
|
|
||
| @given(rust_available=_AVAILABILITY) | ||
| def test_auto_follows_availability( | ||
| rust_available: bool | None, # noqa: FBT001 - property input, not a flag. | ||
| ) -> None: | ||
| """``AUTO`` picks ``RUST`` iff available, otherwise ``PYTHON``.""" | ||
| resolved = _resolve_backend(StreamBackend.AUTO, rust_available=rust_available) | ||
| expected = StreamBackend.RUST if rust_available else StreamBackend.PYTHON | ||
| assert resolved is expected | ||
|
|
||
|
|
||
| # Environment values can never contain a NUL byte (the OS rejects them), so | ||
| # fuzz over NUL-free text. Seed the known tokens (with case/whitespace noise) | ||
| # so the accept paths are exercised alongside the garbage. | ||
| _ENV_VALUE = st.one_of( | ||
| st.text( | ||
| alphabet=st.characters(codec="ascii", exclude_characters="\x00"), | ||
| max_size=12, | ||
| ), | ||
| st.sampled_from(["", "auto", "RUST", " Python ", "\tauto\n", "turbo", "rustc"]), | ||
| ) | ||
|
|
||
|
|
||
| @given(raw=_ENV_VALUE) | ||
| def test_parse_backend_value_maps_exactly_or_rejects(raw: str) -> None: | ||
| """Parsing maps to the exact member (empty/auto -> AUTO), else raises. | ||
|
|
||
| The raw value is injected directly rather than written to ``os.environ``, | ||
| so the property never mutates global process state (avoiding the shared | ||
| environment-guard requirement in AGENTS.md and any cross-thread races). | ||
| """ | ||
| normalised = raw.strip().lower() | ||
| valid = {member.value for member in StreamBackend} | ||
| if normalised == "": | ||
| # Empty/whitespace resolves to AUTO. | ||
| assert _parse_backend_value(raw) is StreamBackend.AUTO | ||
| elif normalised in valid: | ||
| # A recognized value maps to exactly that member, never a different one. | ||
| assert _parse_backend_value(raw).value == normalised | ||
| else: | ||
| with pytest.raises(ValueError, match=_ENV_VAR): | ||
| _parse_backend_value(raw) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Group the property tests into classes.
All five tests sit as bare module-level functions. Group related assertions (e.g., resolver-behaviour tests vs. parser tests) under test_-prefixed methods inside classes.
As per path instructions, "Group related tests using classes with method names prefixed by test_."
🤖 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_backend_resolver_property.py` around lines 37 - 119,
Group the five property tests into related test classes, such as resolver
behavior and backend-value parsing, while preserving each test’s existing
assertions and Hypothesis decorators. Convert each function into a test_
prefixed method within its appropriate class, including the existing resolver
helpers and parser symbols unchanged.
Source: Path instructions
There was a problem hiding this comment.
@coderabbitai Has this now been resolved in the latest commit?
Use codegraph analysis to determine your answer.
If this comment is now resolved, please mark it as such using the API. Otherwise, please provide an AI agent prompt for the remaining work to be done to address this comment.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '== Codegraph outline =='
ast-grep outline cuprum/unittests/test_backend_resolver_property.py \
--items all --type class,function --view expanded
printf '%s\n' '== Target source range =='
sed -n '25,145p' cuprum/unittests/test_backend_resolver_property.py
printf '%s\n' '== Matching PR review thread =='
gh api graphql \
-F owner='leynos' \
-F name='cuprum' \
-F number=224 \
-f query='
query($owner: String!, $name: String!, $number: Int!) {
repository(owner: $owner, name: $name) {
pullRequest(number: $number) {
reviewThreads(first: 100) {
nodes {
id
isResolved
path
line
comments(first: 20) {
nodes {
author { login }
body
url
}
}
}
}
}
}
}' \
--jq '.data.repository.pullRequest.reviewThreads.nodes[]
| select(.path == "cuprum/unittests/test_backend_resolver_property.py")
| select(any(.comments.nodes[]; .author.login == "coderabbitai" and (.body | contains("Group the property tests into classes."))))
| {id, isResolved, path, line, comments: [.comments.nodes[] | {author: .author.login, url, body}]}'Length of output: 7431
@leynos, leave this thread unresolved. Codegraph analysis shows all five tests remain bare module-level functions (Lines 38, 54, 65, 80, and 102), with no test classes in cuprum/unittests/test_backend_resolver_property.py.
Verify the current state of `cuprum/unittests/test_backend_resolver_property.py` and make the smallest change needed to address the review finding.
Group the existing Hypothesis property tests into focused pytest classes while preserving all existing assertions, decorators, imports, strategies, and test behaviour:
1. Add a `TestBackendResolution` class containing:
- `test_resolver_never_returns_auto`
- `test_python_is_honoured_unconditionally`
- `test_forced_rust_respects_availability`
- `test_auto_follows_availability`
2. Add a `TestBackendValueParsing` class containing:
- `test_parse_backend_value_maps_exactly_or_rejects`
Keep every method `test_`-prefixed and retain the existing `@given(...)` decorators on each method. Do not change production code or alter test inputs/assertions. Run the applicable formatting, lint, type-check, and test commands after the structural change.
leynos
changed the title
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai On the "User-Facing Documentation" pre-merge check: The new |
|
Tip For best results, initiate chat on the files or code changes.
🐇 📖 ✏️ Learnings added
|
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai Resolved the "User-Facing Documentation" pre-merge check by taking the second option you offered — the classifiers are developer-facing only, so I have stopped exporting them.
|
|
Tip For best results, initiate chat on the files or code changes.
✏️ Learnings added
|
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.
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cuprum/builders/args.py (1)
93-100: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winParse Windows-style paths with Windows semantics.
PurePath(raw_value)on POSIX keeps backslashes literal, soC:\tmp\..\secretbypasses the..check; UNC paths are also not recognised as absolute. Switch toPureWindowsPathfor Windows-shaped inputs before checkingparts, and add regression coverage for drive-letter and UNC paths.🤖 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/builders/args.py` around lines 93 - 100, Update the path parsing around raw_value and _WINDOWS_ABS_PATTERN to use PureWindowsPath for Windows-shaped inputs, including drive-letter and UNC paths, before evaluating path.parts and absolute-path status; retain native PurePath handling for other inputs. Add regression coverage verifying that parent segments in Windows drive-letter and UNC paths return PathRejection.PARENT_SEGMENT and that those paths are recognised as absolute.
🤖 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.
Outside diff comments:
In `@cuprum/builders/args.py`:
- Around line 93-100: Update the path parsing around raw_value and
_WINDOWS_ABS_PATTERN to use PureWindowsPath for Windows-shaped inputs, including
drive-letter and UNC paths, before evaluating path.parts and absolute-path
status; retain native PurePath handling for other inputs. Add regression
coverage verifying that parent segments in Windows drive-letter and UNC paths
return PathRejection.PARENT_SEGMENT and that those paths are recognised as
absolute.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2937a2b5-8459-425e-9da7-32bed1344fa9
📒 Files selected for processing (5)
cuprum/_backend.pycuprum/builders/args.pycuprum/unittests/test_args_validators_property.pycuprum/unittests/test_backend.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)
|
@coderabbitai Documented the backend resolution split in These are private, internal helpers and the public |
✅ Action performedReview finished.
|
|
@coderabbitai The "Developer Documentation" pre-merge warning describes a gap that head 1. Keyword-only marker for
2. Backend seam in the design doc —
The remaining references to
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- relevant commits ---'
git log --oneline -n 8
printf '%s\n' '--- head commit and parent relationship ---'
git show --no-patch --format='commit=%H%nparents=%P%nsubject=%s' 47088d1
git merge-base --is-ancestor ff6ee30 47088d1 && printf '%s\n' 'ff6ee30 is an ancestor of 47088d1'
printf '%s\n' '--- developer guide: resolver signature and keyword-only contract ---'
git grep -n -A10 -B4 '_resolve_backend\|keyword-only\|Stream-backend resolution seam' 47088d1 -- docs/developers-guide.md
printf '%s\n' '--- design document: backend-resolution seam ---'
git grep -n -A10 -B4 '_parse_backend_value\|_probe_rust_availability\|_resolve_backend' 47088d1 -- docs/cuprum-design.md
printf '%s\n' '--- implementation signatures and cached probe references ---'
rg -n -A5 -B3 '^def (_check_rust_available|_parse_backend_value|_probe_rust_availability|_resolve_backend|get_stream_backend)\b' cuprum/_backend.py
printf '%s\n' '--- ADR: cached availability probe ---'
rg -n -A8 -B4 '_check_rust_available|cached|availability' docs/adr-005-unified-rust-availability-probe.mdLength of output: 9778
Treat 🐇 🔎 |
There was a problem hiding this comment.
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 `@docs/cuprum-design.md`:
- Around line 1872-1880: The cached-resolver bullets preceding this section
still reference the obsolete _check_rust_available() contract. Update them to
describe get_stream_backend() composing _parse_backend_value(),
_probe_rust_availability(), and _resolve_backend(), preserving the documented
precedence and single authoritative backend-resolution model.
🪄 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: 859681ea-1826-4d59-8a67-bee6b84b8e0b
📒 Files selected for processing (2)
docs/cuprum-design.mddocs/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)
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.
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.
|
@coderabbitai review |
✅ Action performedReview finished.
|
The backend precedence prose and its flowchart still led with get_stream_backend() delegating to _check_rust_available(), competing with the newer _parse_backend_value -> _probe_rust_availability -> _resolve_backend seam. Present one authoritative model: the resolution seam is primary, with _check_rust_available() described as the cached probe that _probe_rust_availability() wraps. Route the mermaid availability probe through a _probe_rust_availability() node so the diagram matches the prose and the implementation. Closes CodeRabbit's Developer Documentation pre-merge warning on #224. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@coderabbitai Addressed in
|
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- checked-out head and target commit ---'
git show --no-patch --format='HEAD=%H%nsubject=%s' HEAD
git show --no-patch --format='commit=%H%nparents=%P%nsubject=%s' e6fd9b3
printf '%s\n' '--- backend implementation map ---'
ast-grep outline cuprum/_backend.py --items all --type function --match 'get_stream_backend|probe_rust_availability|check_rust_available|resolve_backend|parse_backend_value'
rg -n -A20 -B4 '^def (get_stream_backend|_probe_rust_availability|_check_rust_available|_resolve_backend|_parse_backend_value)\b' cuprum/_backend.py
printf '%s\n' '--- developer guide: Section 13.4 and backend flowchart ---'
rg -n -A140 -B8 '^### 13\.4\b|_parse_backend_value|_probe_rust_availability|_check_rust_available|_resolve_backend' docs/developers-guide.md
printf '%s\n' '--- design guide: backend seam references ---'
rg -n -A20 -B5 '_parse_backend_value|_probe_rust_availability|_check_rust_available|_resolve_backend' docs/cuprum-design.md
printf '%s\n' '--- documentation changes in e6fd9b3 ---'
git show --format=fuller --find-renames -- docs/developers-guide.md docs/cuprum-design.md e6fd9b3Length of output: 213505
|
Refactor the argument validators and the stream-backend dispatcher to expose pure, classification-style seams, then fuzz them with Hypothesis. cuprum/builders/args.py: introduce PathRejection / GitRefRejection enums and classify_path_string / classify_git_ref helpers that report the first failing category (each enum value is the exact error message). The validators now raise from the classifier, so the rejection reason is inspectable rather than pass/fail only. Behaviour and messages are unchanged. cuprum/_backend.py: split a pure _resolve_backend(requested, rust_available) decision from the cached, environment-reading wrapper, and add _probe_rust_availability to encapsulate each mode's probe-failure policy. All existing log events (resolution debug, forced-unavailable warning, auto-probe-failed debug) are preserved. Tests: fuzz validator/classifier consistency, totality, and category coverage (NUL, parent segments, whitespace); prove the resolver never leaks AUTO, honours PYTHON, respects forced-Rust availability, and that env parsing rejects invalid values. Closes #70 Closes #72 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Respond to Codex and CodeRabbit review feedback on the validator/backend property tests: - Extract a pure `_parse_backend_value(raw)` from `_read_backend_env` and test it by injecting the raw value directly, rather than mutating `os.environ`. This removes the direct environment mutation AGENTS.md forbids and any cross-thread race (Codex P1). - Pin the exact backend mapping in the parse property (`result.value == normalised`, empty -> AUTO) so a mis-mapped member can no longer pass (CodeRabbit). - Anchor the absolute-path round-trip property with a platform-native absolute prefix; a leading "/" is drive-relative on Windows, so the test now uses a drive anchor there and asserts validity/idempotence rather than a POSIX-only "/" prefix (Codex P2). - Document the `classify_path_string` / `classify_git_ref` classifiers and their `PathRejection` / `GitRefRejection` enums in developers-guide.md, recording their intended audience and reuse policy (Codex P1). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Wrap the availability probe and resolution in one boundary handler in `get_stream_backend`, so a forced-RUST failure emits the structured `stream_backend_unavailable` warning whether the probe reports the extension unavailable OR the probe itself raises ImportError (previously the probe ran outside the handler and a probe ImportError bypassed the warning). Add `test_forced_rust_probe_import_error_logs_warning` (CodeRabbit: Unit Architecture, Observability). - Pin the specific git-ref/path rejection categories with dedicated properties: `LEADING_DASH`, `WHITESPACE`, and `NOT_ABSOLUTE`, so a regression that mis-mapped a category can no longer pass the existing "rejected" assertions (CodeRabbit: Testing). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Resolve the remaining pre-merge documentation warning by treating classify_path_string / classify_git_ref and the PathRejection / GitRefRejection enums as developer-facing helpers: drop them from cuprum/builders/args.py __all__ (whose public surface is safe_path / git_ref and their return types) while keeping them importable for in-tree use and tests. This aligns the export surface with the developers-guide documentation, so no users-guide entry is warranted (CodeRabbit). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Record the internal split of get_stream_backend into _parse_backend_value / _probe_rust_availability / _resolve_backend and the single boundary handler, in the developers guide (the appropriate home for an internal testability seam). The public contract and precedence are unchanged (CodeRabbit pre-merge Developer Documentation). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- developers-guide: show _resolve_backend(requested, *, rust_available) with the keyword-only marker, so maintainers do not call it positionally (CodeRabbit inline). - cuprum-design.md: add the internal resolution seam (_parse_backend_value / _probe_rust_availability / _resolve_backend and the single boundary handler) to the stream-backend precedence section, so the design doc reflects the current architecture (CodeRabbit pre-merge). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The spelling gate enforces Oxford (-ize) spelling; "normalisation" tripped typos.toml's normalisation→normalization mapping, failing lint-test. Use "normalization" in the _parse_backend_value bullet. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The precedence section said get_stream_backend() delegates directly to _check_rust_available(), reading as if it bypassed the new resolution seam. Match the code path (get_stream_backend -> _probe_rust_availability -> _check_rust_available) so the flow description and the seam note below it are consistent, closing CodeRabbit's design-doc pre-merge warning. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The backend precedence prose and its flowchart still led with get_stream_backend() delegating to _check_rust_available(), competing with the newer _parse_backend_value -> _probe_rust_availability -> _resolve_backend seam. Present one authoritative model: the resolution seam is primary, with _check_rust_available() described as the cached probe that _probe_rust_availability() wraps. Route the mermaid availability probe through a _probe_rust_availability() node so the diagram matches the prose and the implementation. Closes CodeRabbit's Developer Documentation pre-merge warning on #224. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
e6fd9b3 to
22f6d21
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
The post-merge Developer Documentation warning is valid: |
3 participants
Summary
Adds Hypothesis property-based coverage for the argument validators (
safe_path,git_ref) and the stream-backend resolver, backed by small refactors that expose pure, verifiable seams.Changes
cuprum/builders/args.py(#70)PathRejectionandGitRefRejectionenums whose member values are the exact error messages, andclassify_path_string/classify_git_refhelpers that return the first failing category (orNone).cuprum/_backend.py(#72)_resolve_backend(requested, rust_available)decision from the cached, environment-reading, logging wrapper, plus_probe_rust_availabilityencapsulating each mode's probe-failure policy.Tests
test_args_validators_property.py: totality, validator/classifier consistency, and category coverage (NUL,..segments, whitespace, leading dash), plus round-trip/idempotence for valid inputs.test_backend_resolver_property.py: the resolver never leaksAUTO, honoursPYTHON, respects forced-Rust availability, and env parsing rejects invalid values (only empty/autoresolve toAUTO).test_maturin_buildwheel-manifest snapshot to include the two new modules.Verification
make check-fmt lint typecheck test markdownlintall green (779 Python items, 33 Rust nextest).Closes #70
Closes #72
🤖 Generated with Claude Code