Ensure cargo-binstall before coverage tool installs#296
Open
leynos wants to merge 1 commit into
Open
Conversation
Install `cargo-binstall` before `cargo-llvm-cov` is installed so Rust coverage runs can use `cargo binstall` on fresh runners. Keep the step idempotent so `setup-rust` can continue to provide the binary when it is already present. Cover the action ordering and guard condition in the generate-coverage manifest tests.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
|
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:
SummaryThis pull request addresses a regression in the Changes
|
| Layer / File(s) | Summary |
|---|---|
Idempotent binstall step and PATH/version verification .github/actions/generate-coverage/action.yml |
Adds an "Ensure cargo-binstall" early-check block that exits the step if the binary is already present, computes and exports the Cargo bin directory to GITHUB_PATH, and asserts the installed version matches BINSTALL_VERSION before proceeding to install cargo-llvm-cov. |
Test helpers and new step-contract tests .github/actions/generate-coverage/tests/test_scripts.py |
Introduces _generate_coverage_steps() and _generate_coverage_step() helpers; adds test_generate_coverage_ensures_binstall_before_llvm_cov, test_generate_coverage_binstall_is_not_nextest_only, and test_generate_coverage_binstall_install_is_idempotent; refactors _python_step_env_contract() to use the shared helper. |
Suggested Reviewers
- codescene-delta-analysis
Poem
🦀
cargo-binstallknocks at the door,
Checks if it's home — no need to install more.
The PATH is exported, the version confirmed,
Idempotent habits have finally been learned.
Tests stand on guard to keep order in line —
Install once, verify twice, and all shall be fine! ✅
Caution
Pre-merge checks failed
Please resolve all errors before merging. Addressing warnings is optional.
- Ignore
❌ Failed checks (2 errors, 4 warnings)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Testing (Overall) | ❌ Error | Tests are vacuous: they only check for string presence in YAML and script text, not actual behaviour. The idempotent test would pass even if version verification were removed from the fast path, di... | Strengthen tests to verify version checking logic executes in both fast and slow paths through integration tests or mock-based behavioural assertions, not just string presence checks. |
| Unit Architecture | ❌ Error | The "Ensure cargo-binstall" step violates unit architecture: fast path bypasses version verification (asymmetric fallibility contracts), tests verify source syntax not execution semantics. | Restructure the step to invoke version verification BEFORE the early exit, ensuring both paths enforce identical validation contracts. Augment tests with integration-style execution or shell-script mocking. |
| Developer Documentation | The PR adds a new pinned tooling requirement (cargo-binstall v1.16.6) to generate-coverage but lacks documentation in CHANGELOG.md, developers-guide.md, and design documents. | Add entry to CHANGELOG.md Unreleased section, create "generate-coverage cargo-binstall Pinning" section in developers-guide.md, and add design decision to generate-coverage-design.md for the new idempotent binstall installation step. | |
| Testing (Unit And Behavioural) | The three new tests are manifest-only checks (YAML parsing and string assertions) disguised as behavioural tests. They never execute the shell script logic, verify functional boundaries, test error... | Add integration tests that execute the Ensure cargo-binstall step with actual shell invocation, testing: (1) version mismatch detection in both fast and slow paths; (2) idempotency verification via actual command invocation; (3) hash ver... | |
| Observability | Fast-path version verification is missing. The script logs when cargo-binstall exists (line 126) but exits (line 128) before performing the version check required in the slow path (lines 164-169),... | Move the version check before the fast-path exit to ensure version verification always occurs, providing observability for version mismatches in both paths. | |
| Concurrency And State | The idempotent fast path (lines 125-128) bypasses the version verification that the slow path enforces (lines 164-169). State transitions violate the version-pinning contract, and tests only check... | Move the version verification check (lines 164-169) before the fast-path exit. Verify BINSTALL_VERSION immediately after the early-exit check, before line 128. Add integration tests that exercise both paths: success with correct version,... |
✅ Passed checks (14 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The title directly describes the main change: adding a cargo-binstall installation step before coverage tool installation. |
| Description check | ✅ Passed | The description is directly related to the changeset, providing context about the cargo-binstall ordering fix and the implementation approach. |
| Docstring Coverage | ✅ Passed | Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. |
| 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. |
| User-Facing Documentation | ✅ Passed | This PR modifies only the internal implementation of the generate-coverage action (adding an "Ensure cargo-binstall" step) without changing inputs, outputs, or user-facing behaviour documented in R... |
| Module-Level Documentation | ✅ Passed | The PR introduced no new Python modules. The only module modified (test_scripts.py) retains its comprehensive module-level docstring explaining purpose, utility, and relationships to other components. |
| Testing (Property / Proof) | ✅ Passed | Property-based testing or formal proof is not applicable here. The PR introduces simple step-ordering and conditional-logic invariants in YAML and Bash, not mathematically-ranged or state-space pro... |
| Testing (Compile-Time / Ui) | ✅ Passed | Check does not apply: PR modifies a GitHub Action manifest and adds contract tests validating YAML structure (step ordering, conditional expressions, shell script content), not compile-time Rust co... |
| Domain Architecture | ✅ Passed | This PR modifies GitHub Actions YAML and related tests—inherently infrastructure code. No domain logic exists to segregate from infrastructure concerns. The changes ensure cargo-binstall is install... |
| Security And Privacy | ✅ Passed | The pull request introduces no security or privacy risks. The new "Ensure cargo-binstall" step safely downloads, verifies and executes a third-party installer with SHA256 checksum validation. All c... |
| Performance And Resource Use | ✅ Passed | The PR introduces no algorithmic regressions, unbounded loops, memory leaks, or resource growth issues. The new "Ensure cargo-binstall" step executes a linear sequence of operations (PATH check, si... |
| Architectural Complexity And Maintainability | ✅ Passed | Abstractions introduced are proportional to the problem: three modest test helpers enable three focused tests validating step ordering, condition logic, and idempotent fast-path behaviour. No specu... |
| Rust Compiler Lint Integrity | ✅ Passed | PR modifies only YAML (.github/actions/generate-coverage/action.yml) and Python (.github/actions/generate-coverage/tests/test_scripts.py) files; no Rust source code (.rs) is present, so Rust Compil... |
✨ Finishing Touches
📝 Generate docstrings
- Create stacked PR
- Commit on current branch
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Commit unit tests in branch
fix/generate-coverage-binstall-order
Comment @coderabbitai help to get the list of available commands.
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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 @.github/actions/generate-coverage/action.yml:
- Around line 121-128: The early exit in the Ensure cargo-binstall step accepts
any pre-existing cargo-binstall and exits immediately without verifying that its
version matches the pinned BINSTALL_VERSION. The version check must be performed
before the early exit succeeds. Restructure the logic to verify the version of
the existing cargo-binstall (using the same grep pattern that checks if the
version output matches the expected version string) before exiting; only exit
successfully if both the binary exists and the version matches, otherwise fall
through to the installation path to download the correct pinned 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: f8918c33-65b6-4a20-b3a8-2bacb145cf03
📒 Files selected for processing (2)
.github/actions/generate-coverage/action.yml.github/actions/generate-coverage/tests/test_scripts.py
Comment on lines
+121
to
+128
| - name: Ensure cargo-binstall | ||
| if: steps.detect.outputs.lang == 'rust' || steps.detect.outputs.lang == 'mixed' | ||
| run: uv run --script "${{ github.action_path }}/scripts/install_cargo_llvm_cov.py" | ||
| shell: bash | ||
| - name: Install cargo-binstall | ||
| if: (steps.detect.outputs.lang == 'rust' || steps.detect.outputs.lang == 'mixed') && inputs.use-cargo-nextest == 'true' | ||
| run: | | ||
| set -euo pipefail | ||
| if command -v cargo-binstall >/dev/null 2>&1; then | ||
| echo "cargo-binstall already installed: $(cargo-binstall -V)" | ||
| exit 0 | ||
| fi |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect setup-rust's pinned cargo-binstall version for drift against generate-coverage.
fd -t f 'action.yml' .github/actions/setup-rust --exec rg -n 'BINSTALL_VERSION' {}
rg -n 'BINSTALL_VERSION' .github/actions/generate-coverage/action.ymlRepository: leynos/shared-actions
Length of output: 795
🏁 Script executed:
# Inspect the slow path in generate-coverage that allegedly enforces the pin
sed -n '159,170p' .github/actions/generate-coverage/action.yml
# Check if setup-rust installs or provides cargo-binstall
sed -n '1,200p' .github/actions/setup-rust/action.yml | grep -A 5 -B 5 'cargo-binstall\|cargo.*install'Repository: leynos/shared-actions
Length of output: 3990
Move the pinned-version assertion ahead of the early exit; the fast path bypasses the version check.
The early-exit branch (lines 121–128) accepts any pre-existing cargo-binstall on PATH and exits immediately, but it never verifies the version matches BINSTALL_VERSION. The slow path (lines 164–169) enforces this: if ! "$cargo_binstall" -V | grep -qF "$binstall_ver" exits with status 1 on mismatch. This asymmetry violates the documented contract—the version check exists to fail when the installed binary drifts from the pinned release, yet the fast path silently bypasses it.
Both setup-rust and generate-coverage currently pin v1.16.6, so immediate practical risk is low. However, a runner with a pre-existing cargo-binstall (sourced from a previous workflow, system installation, or version drift in future updates) could bypass the version enforcement entirely. Restructure the fast path to verify the version before exiting.
🤖 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 @.github/actions/generate-coverage/action.yml around lines 121 - 128, The
early exit in the Ensure cargo-binstall step accepts any pre-existing
cargo-binstall and exits immediately without verifying that its version matches
the pinned BINSTALL_VERSION. The version check must be performed before the
early exit succeeds. Restructure the logic to verify the version of the existing
cargo-binstall (using the same grep pattern that checks if the version output
matches the expected version string) before exiting; only exit successfully if
both the binary exists and the version matches, otherwise fall through to the
installation path to download the correct pinned version.
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
This branch ensures the
generate-coverageaction installscargo-binstallbefore it installs Rust coverage tools. That prevents fresh Linux and Windows runners from failing whencargo-llvm-covinvokescargo binstall, while still reusing the binary thatsetup-rustnormally provides.No linked issue or execplan was identified for this regression fix.
Review walkthrough
.github/actions/generate-coverage/action.ymlto see the new idempotentEnsure cargo-binstallstep and its position beforeInstall cargo-llvm-cov..github/actions/generate-coverage/tests/test_scripts.pyfor manifest coverage of the step order, Rust/mixed condition, and existing-binary fast path.Validation
make test: 934 passed, 14 skipped.make check-fmt: passed.make typecheck: passed.make lint: passed.git diff --check: passed.Notes
The branch was renamed from
session/a758c119tofix/generate-coverage-binstall-orderbefore the pull request was created, so no PR branch rename was required.