Ensure cargo-binstall before coverage tool installs#296
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 42 minutes Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
WalkthroughAdd an idempotent cargo-binstall install and verification step to ChangesGenerate-coverage cargo-binstall flow
Repository markdown, lint, and documentation reformatting
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 19 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (19 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![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.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
lodyai
Bot
force-pushed
the
fix/generate-coverage-binstall-order
branch
from
a1564fd to
c522b09
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
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.
Require a pre-existing `cargo-binstall` to match the pinned version before `generate-coverage` reuses it. Fall through to the pinned installer when the binary is missing or has the wrong version. Exercise the action shell step with fake commands so the fast path, mismatch path, and post-install verification all assert runtime behaviour.
Excludes the Memtrace local database directory from version control. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
- Replace git-aware markdownlint-check.sh wrapper with a direct markdownlint-cli2 invocation in the Makefile - Remove workflow_scripts/markdownlint-check.sh - Import .markdownlint-cli2.jsonc config from mapsplice (adds golden fixture ignore and enforces MD046 fenced code blocks) - Run mdformat-all and markdownlint-cli2 --fix across all Markdown files, including .github/actions/ (previously skipped by fd) - Convert indented code blocks to fenced throughout execplan docs - Fix MD056 missing table cells in upload-codescene-coverage README - Update developers-guide.md to reflect MDLINT → markdownlint-cli2 and remove the now-removed MARKDOWNLINT_BASE variable Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Record the new "Ensure cargo-binstall" step introduced in generate-coverage: - Add an Unreleased CHANGELOG bullet for the pinned cargo-binstall install. - Add a dated design decision to generate-coverage-design.md explaining why the action provisions its own pinned cargo-binstall and how both the reuse and install paths verify the version. - Add a developers-guide section describing the idempotent step, its two verification paths, and the behavioural tests that exercise them. Addresses the Developer Documentation review finding. The related "fast path bypasses version verification" and "vacuous tests" findings were already resolved by commit a5e8041 and are not re-litigated here. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
lodyai
Bot
force-pushed
the
fix/generate-coverage-binstall-order
branch
from
655f2fb to
26ebcca
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
|
@coderabbitai review |
✅ Action performedReview finished.
|
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/actions/generate-coverage/action.yml (1)
135-163: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winExport
BINSTALL_VERSIONbefore invoking the installer. The childbashonly sees exported variables; without that, the installer falls back tolatestand defeats thev1.16.6pin. Mirrorsetup-rusthere.🤖 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 135 - 163, The cargo-binstall install block is pinning BINSTALL_VERSION locally, but the child bash installer cannot see it unless it is exported. Update the installer setup in generate-coverage/action.yml so the variable is exported before calling bash "$INSTALLER_PATH", matching the setup-rust pattern, and keep the existing version-check and checksum flow intact.
🤖 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/linux-packages/README.md:
- Around line 14-24: Update the surrounding prose in the Linux packages README
to use en-GB spelling by changing descriptive text from “license” to “licence”
while keeping the table field name “license” unchanged. Locate the affected copy
in the package options table and revise only the human-readable descriptions,
not the key names or schema terms.
In @.github/actions/macos-package/README.md:
- Around line 3-4: Update the prose in the macOS package action README to use
en-GB-oxendict British spelling, specifically changing surrounding descriptive
text from “license” to “licence” while keeping identifiers, option names, and
filenames like `license-file` and `LICENSE` unchanged. Review the referenced
descriptive sections in the README and apply the spelling consistently in the
affected narrative text without altering any code-style tokens or paths.
In `@AGENTS.md`:
- Around line 115-120: Update the tool-resolution table in AGENTS.md to match
the current Makefile behavior: change the MDLINT entry to reference
markdownlint-cli2 instead of markdownlint, and remove the MARKDOWNLINT_BASE row
entirely since no diff-base variable is consumed anymore. Keep the table wording
consistent with the other tool entries and use the existing variable names in
that section to locate the change.
In `@docs/developers-guide.md`:
- Around line 99-119: The Makefile tool-resolution table no longer matches the
documented contract because the MARKDOWNLINT_BASE entry was removed and MDLINT
is now described inconsistently. Update the Makefile resolution section to
restore the MARKDOWNLINT_BASE row and ensure the MDLINT description matches the
actual fallback order used by the Makefile’s tool resolution logic. Use the
existing “Makefile Tool Resolution” table and the MDLINT/MARKDOWNLINT_BASE
symbols as the source of truth, and keep the override example aligned with that
contract.
---
Outside diff comments:
In @.github/actions/generate-coverage/action.yml:
- Around line 135-163: The cargo-binstall install block is pinning
BINSTALL_VERSION locally, but the child bash installer cannot see it unless it
is exported. Update the installer setup in generate-coverage/action.yml so the
variable is exported before calling bash "$INSTALLER_PATH", matching the
setup-rust pattern, and keep the existing version-check and checksum flow
intact.
🪄 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: 452ae7d8-2961-457a-8714-c58b1d1d051a
📒 Files selected for processing (50)
.github/actions/DEVELOPMENT.md.github/actions/determine-release-modes/README.md.github/actions/ensure-cargo-version/README.md.github/actions/export-cargo-metadata/CHANGELOG.md.github/actions/export-cargo-metadata/README.md.github/actions/generate-coverage/CHANGELOG.md.github/actions/generate-coverage/README.md.github/actions/generate-coverage/action.yml.github/actions/generate-coverage/tests/test_scripts.py.github/actions/linux-packages/README.md.github/actions/macos-package/README.md.github/actions/release-to-pypi-uv/README.md.github/actions/rust-build-release/CHANGELOG.md.github/actions/rust-build-release/README.md.github/actions/setup-rust/README.md.github/actions/stage-release-artefacts/CHANGELOG.md.github/actions/stage-release-artefacts/README.md.github/actions/upload-codescene-coverage/README.md.github/actions/upload-release-assets/CHANGELOG.md.github/actions/upload-release-assets/README.md.github/actions/validate-linux-packages/README.md.github/actions/windows-package/README.md.gitignore.markdownlint-cli2.jsoncAGENTS.mdMakefileREADME.mddocs/adr/0001-stable-manpage-path.mddocs/adr/0002-explicit-ps-module-name.mddocs/cmd-mox-users-guide.mddocs/dependabot-automerge-workflow.mddocs/developers-guide.mddocs/execplans/2-4-5-extend-architecture-enforcement-to-orchestration-code.mddocs/execplans/3-14-5-regression-coverage-for-conditional-action-dependency-manifests.mddocs/execplans/adopt-cargo-nextest-in-generate-coverage.mddocs/execplans/auto-merge-on-unstable.mddocs/execplans/binstall-metadata-audit.mddocs/execplans/cargo-manifests-input.mddocs/execplans/dependabot-auto-merge.mddocs/execplans/refactor-validate-packages-modules.mddocs/execplans/support-cranelift-codegen.mddocs/generate-coverage-design.mddocs/python-action-scripts.mddocs/python-native-command-mocking-design.mddocs/rust-build-release-pipeline.mddocs/scripting-standards.mddocs/syspath-hack-users-guide.mddocs/windows-gnullvm-build.mdrust-toy-app/README.mdworkflow_scripts/markdownlint-check.sh
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
leynos/polythene(auto-detected)
💤 Files with no reviewable changes (1)
- workflow_scripts/markdownlint-check.sh
Fix a real bug and clear valid review findings; skip stale/invalid ones. - generate-coverage/action.yml: export BINSTALL_VERSION so the child installer shell inherits the pin. Without export the installer silently falls back to releases/latest, defeating the pin and tripping post-install verification. - generate-coverage tests: add three behavioural regression tests that execute the extracted step body — the pinned version is exported to the installer, the Cargo bin dir is appended to GITHUB_PATH, and a bad installer checksum aborts before the installer runs. The fake installer now records the inherited BINSTALL_VERSION so the export is genuinely guarded (the test fails without the export). - linux-packages / macos-package READMEs: use en-GB "licence" (noun) in prose, matching the repository convention; input keys, option names, and paths (`license`, `license-file`, `include-license-panel`, `LICENSE`) are unchanged. - AGENTS.md: align the Makefile tool-resolution table with the current Makefile — MDLINT resolves to markdownlint-cli2, and the removed MARKDOWNLINT_BASE row and its override example are dropped. Skipped: the request to restore MARKDOWNLINT_BASE in developers-guide.md (that variable was deliberately removed from the Makefile; the doc already matches), and creating docs/users-guide.md (the binstall flow is internal plumbing, already covered by the action README and design notes). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Have the following now been resolved? .github/actions/generate-coverage/action.yml (1)
🤖 Detailed instructionsVerify each finding against current code. Fix only still-valid issues, skip the In @.github/actions/generate-coverage/action.yml around lines 135 - 163, The |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
sed -n '100,200p' .github/actions/generate-coverage/action.ymlLength of output: 2056 🏁 Script executed: #!/bin/bash
set -euo pipefail
sed -n '100,200p' .github/actions/generate-coverage/action.ymlLength of output: 5383
export BINSTALL_VERSION="v1.16.6"This line sits before the |
Align the generate-coverage "Ensure cargo-binstall" step with setup-rust, which main bumped to v1.19.1. The repository now uses a single cargo-binstall pin across every action and test. - action.yml: BINSTALL_VERSION v1.16.6 -> v1.19.1 and the installer-script BINSTALL_SHA256 to the matching validated digest. - tests: update the binstall test doubles (existing/installed versions and the fake installer checksum) to v1.19.1. - CHANGELOG and generate-coverage-design.md: reflect the pinned version. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
Two generate-coverage fixes for the cargo-binstall install step.
- Cache the pinned cargo-binstall binary alongside cargo-llvm-cov and
cargo-nextest so a cache hit no longer re-downloads and re-verifies the
installer on every run. The step's version check still guards a stale cached
binary, so this stays correct across pin bumps.
- Match the pinned version as a whole, space-delimited token instead of a
`grep -qF` substring, so a look-alike such as 1.19.10 no longer satisfies a
1.19.1 pin and skips the reinstall. Both the fast-path reuse check and the
post-install verification use the shared `binstall_version_matches` helper,
which handles the real bare `-V` output ("1.19.1") and the "<name> <version>"
form alike. Add a regression test for a pre-existing 1.19.10 binary.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Note the pinned cargo-binstall install/verification flow in the user-facing action README: the action provisions a specific cargo-binstall version (reusing a cached build on an exact version match, otherwise installing from a checksum-verified installer script) and verifies the resolved version before running the coverage tooling. Addresses the user-facing documentation review finding. A new docs/users-guide.md is not added: the repository's *-users-guide.md docs are subsystem-specific, and per-action user documentation lives in each action's README. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@coderabbitai Have the following now been resolved? .github/actions/generate-coverage/action.yml (2)
⚡ Proposed cache path additionpath: |
+ ~/.cargo/bin/cargo-binstall
~/.cargo/bin/cargo-llvm-cov
~/.cargo/bin/cargo-nextest
~/.cargo/registry
~/.cargo/git
target🤖 Detailed instructionsVerify each finding against current code. Fix only still-valid issues, skip the In @.github/actions/generate-coverage/action.yml around lines 108 - 120, Add the
🤖 Detailed instructionsVerify each finding against current code. Fix only still-valid issues, skip the In @.github/actions/generate-coverage/action.yml around lines 138 - 148, The |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
gh pr view 296 --json headRefName,commits --jq '.headRefName'
git fetch origin pull/296/head:pr296 2>&1 | tail -3
git checkout pr296 2>&1 | tail -3
echo "---action.yml fast path---"
sed -n '90,175p' .github/actions/generate-coverage/action.yml
echo "---tests---"
rg -n "def test_generate_coverage_binstall" .github/actions/generate-coverage/tests/test_scripts.py
echo "---docs---"
rg -n "cargo-binstall" docs/developers-guide.md docs/generate-coverage-design.md | head -20
fd -i changelog.mdLength of output: 6937
No code changes remain outstanding for these six items. On the CHANGELOG.md point: don't treat it as a defect in this PR — introducing a changelog convention repository-wide is a separate decision for you as maintainer, not a gap in this fix. If you want it tracked, open an issue for adding a root ᕦ(ò_óˇ)ᕤ |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
sed -n '1,220p' .github/actions/generate-coverage/action.ymlLength of output: 9548
Cache path (Lines 104–116): Version token matching (Lines ~138–148): The substring binstall_version_matches() {
case " $1 " in
*" ${binstall_ver} "*) return 0 ;;
*) return 1 ;;
esac
}Pad the candidate string with leading/trailing spaces and match One gap remains: I don't see a regression test that specifically exercises a pre-existing ✏️ Learnings added
|
2 participants
Summary
This branch ensures the
generate-coverageaction installs and verifiescargo-binstallbefore it installs Rust coverage tools. It prevents fresh Linux and Windows runners from failing whencargo-llvm-covinvokescargo binstall, while still reusing the binary thatsetup-rustnormally provides when that binary matches the pinned version.A follow-up review finding was valid: the first branch version reused any pre-existing
cargo-binstallwithout checking its version. This branch now verifies the existing binary with the samegrep -qFversion-match contract used after installation and falls through to the pinned installer when the version is wrong.No linked issue or execplan was identified for this regression fix.
Review walkthrough
.github/actions/generate-coverage/action.ymlto see theEnsure cargo-binstallstep run beforeInstall cargo-llvm-cov, verify an existing binary before reuse, and reinstall the pinned version on mismatch..github/actions/generate-coverage/tests/test_scripts.pyfor behavioural shell execution tests that cover the verified fast path, mismatch reinstall path, and post-install version failure.Validation
make check-fmt: passed.make typecheck: passed.make lint: passed.make test: 936 passed, 14 skipped.git diff --check: passed.Notes
The documentation-only review warning was skipped. The pinned
cargo-binstallversion and checksum were already documented for the repository'ssetup-rustinstallation path, and this branch keeps the change minimal to thegenerate-coveragefallback ordering and verification contract.