Remove dead IPC-timeout constructors on CargoMetadataError (#98) - #113
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
💤 Files with no reviewable changes (1)
OverviewThis PR removes dead IPC-timeout constructors from the ChangesCode deletions
Centralisation of error messages
Validation hardening
Test improvements
Documentation updates
Snapshot capture
Outcomes
WalkthroughCentralise CMOX_IPC_TIMEOUT error messages as constants in ChangesIPC Timeout Message Consolidation
Suggested labels Issue
🚥 Pre-merge checks | ✅ 20✅ Passed checks (20 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📋 Issue PlannerBuilt with CodeRabbit's Coding Plans for faster development and fewer bugs. View plan used: ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Reviewer's GuideCentralizes canonical cmd-mox IPC-timeout error messages in the cmd_mox_runner, tightens timeout validation (including explicit NaN rejection), adds property-based and snapshot tests around timeout resolution, removes dead CargoMetadataError IPC-timeout helpers and their tests, and updates docs to describe the new behavior and single source of truth for the messages. File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
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.
leynos
marked this pull request as ready for review
June 9, 2026 22:17
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
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: 23111f2468
ℹ️ 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".
Owner
Author
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Owner
Author
|
@coderabbitai Have the following now been resolved? Annul any requirements that violate the en-GB-oxendict spelling (-ize / -yse / -our) conventions (for example a request to replace "normalize" with "normalise" or "artefact" with "artifact"), or where the requirement unnecessarily increases cyclomatic complexity. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== lading/testing/cmd_mox_runner.py ==="
cat lading/testing/cmd_mox_runner.py
echo ""
echo "=== tests/unit/test_cmd_mox_integration.py ==="
cat tests/unit/test_cmd_mox_integration.pyLength of output: 16765
✅ Overall Comment 1 — infinities treated as valid timeoutsResolved. if not math.isfinite(timeout) or timeout <= 0:
raise CmdMoxError(NON_POSITIVE_IPC_TIMEOUT_MESSAGE)NaN, ±∞, and overflow-to-infinity values (e.g. ✅ Comment 2 — oracle accepted positive infinityResolved. The oracle condition is now ✅ Comment 1 — property test only checked membership, not per-class mappingResolved. The property test now asserts the specific canonical message per input class:
The dedicated parametrised test
|
Owner
Author
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Owner
Author
|
@coderabbitai Have the following now been resolved? Annul any requirements that violate the en-GB-oxendict spelling (-ize / -yse / -our) conventions (for example a request to replace "normalize" with "normalise" or "artefact" with "artifact"), or where the requirement unnecessarily increases cyclomatic complexity. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== tests/unit/test_cmd_mox_integration.py ==="
cat tests/unit/test_cmd_mox_integration.py
echo ""
echo "=== lading/testing/cmd_mox_runner.py ==="
cat lading/testing/cmd_mox_runner.pyLength of output: 18693
✅ Overall Comment 1 — infinities treated as validResolved. ✅ Individual Comment 2 — oracle accepted positive infinityResolved. ✅ Individual Comment 1 — property test only checked membership, not per-class mappingResolved. ✅ Overall Comment 2 — property test re-derived parsing logicResolved. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Owner
Author
|
@coderabbitai Have the following failed checks now been resolved? If further work is required, please provide an AI agent prompt for the remaining work to be done to address these failures. Do not treat warnings as optional or aspirational. Where a change is out of scope for this PR, propose a GitHub issue unless one exists already. ❌ Failed checks (1 error, 1 warning)
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== lading/testing/cmd_mox_runner.py ==="
cat lading/testing/cmd_mox_runner.py
echo ""
echo "=== lading/workspace/metadata.py ==="
cat lading/workspace/metadata.pyLength of output: 17862
✅ Module-Level Documentation — resolvedBoth modules now carry comprehensive multi-paragraph docstrings.
✅ Observability — resolved
_LOGGER.warning(
"Rejecting CMOX_IPC_TIMEOUT=%r: value is not a number", raw_timeout
)_LOGGER.warning(
"Rejecting CMOX_IPC_TIMEOUT=%r: %s is not a finite positive number",
raw_timeout,
timeout,
) |
Owner
Author
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
CargoMetadataError.invalid_ipc_timeout() and non_positive_ipc_timeout() were referenced only by their own unit test, duplicated the cmd-mox timeout messages verbatim, and placed an IPC concern on a cargo-metadata error class. Delete the two classmethods and define the messages once as constants in lading.testing.cmd_mox_runner, the module that owns IPC-timeout validation. While here, reject NaN timeouts explicitly: NaN compares false against everything, so the previous `timeout <= 0` guard let it slip through. Add a Hypothesis property test driving _resolve_cmd_mox_timeout across None, floats, and arbitrary strings, plus syrupy snapshots pinning the two canonical messages. Closes #98
Record where the IPC-timeout error messages live and how they behave, as requested in review. The cmd-mox usage guide now describes CMOX_IPC_TIMEOUT validation and names INVALID_IPC_TIMEOUT_MESSAGE and NON_POSITIVE_IPC_TIMEOUT_MESSAGE as the single source of truth in lading/testing/cmd_mox_runner.py, noting the syrupy snapshot that pins them. The developers guide cross-references this from the command-runners section. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Treat infinite CMOX_IPC_TIMEOUT values as invalid. float() returns positive infinity for "inf", "Infinity", and overflowing literals such as "1e400"; the old `math.isnan(timeout) or timeout <= 0` guard let infinity through because it is positive, so the socket layer later crashed with OverflowError instead of the documented CmdMoxError. Resolution now requires a finite value via `not math.isfinite(timeout)`, routing NaN and both infinities through the non-positive message. Strengthen the tests so the canonical messages are tied to the implementation rather than merely snapshotted: - Add parametrised example tests pinning each input class to its specific message (unparseable -> INVALID_IPC_TIMEOUT_MESSAGE; zero/negative/NaN/ infinite -> NON_POSITIVE_IPC_TIMEOUT_MESSAGE), including "inf", "Infinity", and "1e400". This locks the mapping so the two strings cannot be silently swapped, and makes the snapshot test non-vacuous by asserting the runner actually raises with those constants. - Fix the property-test oracle to reject infinity with `math.isfinite(parsed)` and assert the specific expected message per input class instead of accepting either. Document infinity (and overflowing literals) among the rejected values in the cmd-mox usage guide. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The domain property test previously re-derived its expectations by calling float() and math.isfinite() on each generated value, mirroring the resolver's own parsing. Replace it with composite Hypothesis strategies that construct each input class directly and attach the expected outcome to the input: - finite positive floats (rendered via repr) that resolve to a timeout, - consonant-only strings drawn from an alphabet that excludes every letter in "inf"/"infinity"/"nan" and the exponent marker, so they are guaranteed unparseable and map to INVALID_IPC_TIMEOUT_MESSAGE, - zero, negative, NaN, and infinite values that map to NON_POSITIVE_IPC_TIMEOUT_MESSAGE, - None mapping to the default. The new class-based test asserts the contract directly with a flat two-branch body, lowering cyclomatic complexity. A separate totality property over arbitrary input asserts the postcondition (a finite positive result or a CmdMoxError) without restating the parsing rules, guarding against a regression that returns an unusable timeout for some unforeseen value. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address the module-documentation and observability review checks. Expand the module docstrings for the cmd-mox runner and the cargo-metadata gateway so each explains its purpose, the helpers it owns, and its relationship to the shared CommandRunner protocol and the sibling adapters, rather than a one-line summary. Note that cmd_mox_runner is the test-time counterpart of subprocess_runner and the canonical home of the IPC-timeout messages, and that metadata routes cargo metadata through the protocol so use_command_runner can swap in the cmd-mox adapter. Add diagnostic logging to _resolve_cmd_mox_timeout: each rejection now emits a warning naming the rejected raw CMOX_IPC_TIMEOUT value and the rule it failed (not a number, or not a finite positive number), so a misconfigured timeout can be traced without reading the source. Extend the rejection test to assert the warning is emitted with the raw value. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
lodyai
Bot
force-pushed
the
issue-98-remove-dead-ipc-timeout-constructors
branch
from
June 10, 2026 12:37
6c3f5c4 to
684179d
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #98
CargoMetadataError.invalid_ipc_timeout()/non_positive_ipc_timeout()— dead code referenced only by their own test, duplicating cmd-mox messages on the wrong domain class.lading/testing/cmd_mox_runner.py(INVALID_IPC_TIMEOUT_MESSAGE,NON_POSITIVE_IPC_TIMEOUT_MESSAGE);_resolve_cmd_mox_timeoutreferences them.timeout <= 0guard and were returned as valid; they now raiseCmdMoxErrorwith the canonical non-positive message.Testing
None→ default; finite positive → that value; everything else (non-numeric, zero, negative, NaN) →CmdMoxErrorwith one of the two canonical messages.tests/unit/test_metadata_helpers.py.make check-fmt,make lint,make typecheck, andmake test(556 passed) all green.🤖 Generated with Claude Code
Summary by Sourcery
Consolidate and canonicalize cmd-mox IPC-timeout error handling while tightening timeout validation and tests.
Bug Fixes:
Enhancements:
Documentation:
Tests: