Export the cargo-binstall version pin#271
Merged
Merged
Conversation
Export `BINSTALL_VERSION` so the installer subprocess uses the pinned `v1.16.6` release instead of falling back to the latest release URL. Also assert the installed binary reports `1.16.6`.
Contributor
|
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 (1)
WalkthroughExport the BINSTALL_VERSION environment variable in the inline installer and add a post‑install verification that runs ChangesCargo-binstall version export and verification
Sequence DiagramsequenceDiagram
participant ActionYML as action.yml
participant Installer as installer.sh
participant Binary as cargo-binstall
ActionYML->>Installer: export BINSTALL_VERSION="v1.16.6"
Installer->>Binary: download & install release tagged by BINSTALL_VERSION
Binary->>ActionYML: cargo-binstall -V (version string)
ActionYML->>ActionYML: verify -V contains "1.16.6" or exit non‑zero
Poem
🚥 Pre-merge checks | ✅ 20✅ Passed checks (20 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
Reviewer's guide (collapsed on small PRs)Reviewer's GuideExports the pinned cargo-binstall version in the setup-rust composite GitHub Action so the installer script consumes the pinned version, and adds a smoke assertion that validates the installed cargo-binstall binary reports version 1.16.6. Sequence diagram for pinned cargo-binstall installation and smoke checksequenceDiagram
participant GitHubRunner
participant setup_rust_action
participant cargo_binstall_installer
participant cargo_binstall_binary
GitHubRunner->>setup_rust_action: run setup-rust
setup_rust_action->>setup_rust_action: export BINSTALL_VERSION=v1.16.6
setup_rust_action->>cargo_binstall_installer: bash INSTALLER_PATH (uses BINSTALL_VERSION)
cargo_binstall_installer-->>setup_rust_action: install cargo-binstall v1.16.6
setup_rust_action->>cargo_binstall_binary: cargo-binstall -V
cargo_binstall_binary-->>setup_rust_action: 1.16.6
setup_rust_action->>setup_rust_action: grep -q "1.16.6" (smoke assertion)
File-Level Changes
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.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?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/setup-rust/action.yml:
- Line 93: The version-check pipeline currently runs "cargo-binstall -V | grep
-q \"1.16.6\"" without an explicit failure message; modify the check to wrap it
in an if-failed block so that when the version doesn't match you print a clear
error like "cargo-binstall version verification failed: expected 1.16.6", output
the actual "cargo-binstall -V" to stderr, and exit 1; update the command around
the cargo-binstall -V invocation in action.yml accordingly to mirror the
checksum validation pattern.
🪄 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: 87d6b3cd-7b76-4248-aa44-aafeb9cbe754
📒 Files selected for processing (1)
.github/actions/setup-rust/action.yml
Wrap the pinned cargo-binstall version check in an explicit failure block so CI logs show the expected version and the actual installed binary version before exiting.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Explain why `BINSTALL_VERSION` must be exported for the setup-rust installer subprocess and record the cargo-binstall version pinning strategy in the developer guide.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Emit the verified cargo-binstall version on the success path and add manifest tests that lock in the exported version pin and installed-version assertion.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
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/setup-rust/tests/test_setup_rust_manifest.py:
- Around line 17-24: The helper _install_binstall_run_script currently uses
next(...) without a default so it raises StopIteration with no context if the
"Install cargo-binstall" step is missing; update the function (referencing
_install_binstall_run_script and _load_steps) to either call next(..., None) and
assert the result is not None with a clear message like "manifest missing
'Install cargo-binstall' step", or catch StopIteration and raise an
AssertionError (or ValueError) with that explicit message before proceeding to
extract run; keep the existing type check for run_script afterwards.
🪄 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: 5f24b737-5b10-40ea-9d8f-1681753ea17e
📒 Files selected for processing (2)
.github/actions/setup-rust/action.yml.github/actions/setup-rust/tests/test_setup_rust_manifest.py
Owner
Author
|
@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. Ensure that this is validated against the current version of the codegraph. If further refinement to address this finding would be deleterious, please supply a clear explanatory one to two paragraph markdown message I can paste into the CodeScene web ui's diagnostic suppression function so this diagnostic can be silenced. .github/actions/setup-rust/tests/test_setup_rust_manifest.py Comment on file return manifest["runs"]["steps"]
def _install_binstall_run_script() -> str:❌ New issue: Code Duplication |
This comment was marked as resolved.
This comment was marked as resolved.
Add a helper for retrieving step conditions and use it in the Postgres setup-rust manifest tests to remove duplicated step lookup logic.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Make setup-rust manifest helper failures explicit when expected steps or fields are missing, and tighten cargo-binstall pin checks to match complete run-script lines.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Add deterministic shell-fragment tests for the setup-rust cargo-binstall installer block so the exported version pin, pinned download URL, success logging, and mismatch failure path are covered without network access.
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.
Add focused tests for missing setup-rust manifest steps and missing helper fields so assertion branches report the expected context.
Owner
Author
|
@coderabbitai Have the following now been resolved? .github/actions/setup-rust/tests/test_setup_rust_manifest.py (1)
🧪 Proposed test additions+def test_get_step_fails_on_missing_step() -> None:
+ """_get_step should raise AssertionError with a clear message for missing steps."""
+ with pytest.raises(AssertionError, match="Missing setup-rust step: Nonexistent Step"):
+ _get_step("Nonexistent Step")
+
+
+def test_get_step_condition_fails_on_missing_condition() -> None:
+ """_get_step_condition should fail clearly when a step has no 'if' field."""
+ # This would require either mocking _get_step or using a step known to lack 'if'
+ # Inspect action.yml to find a step without an 'if' condition, or mock
+ with pytest.raises(AssertionError, match="Step has no string condition"):
+ _get_step_condition("Install Rust") # Replace with actual unconditional step
+
+
+def test_install_binstall_run_script_fails_on_missing_run() -> None:
+ """_install_binstall_run_script should fail clearly if 'run' is absent."""
+ # This would require mocking or temporarily modifying the helper
+ # Alternatively, accept that this is implicitly tested by action.yml structure
+ pass # Consider whether this test adds value given the manifest structure🤖 Detailed instructionsVerify each finding against current code. Fix only still-valid issues, skip the In @.github/actions/setup-rust/tests/test_setup_rust_manifest.py around lines 27
|
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 8675
All three use |
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 exports the
BINSTALL_VERSIONvariable in the setup-rust action so the cargo-binstall installer subprocess receives the pinnedv1.16.6release instead of falling back toreleases/latest. It also adds a smoke assertion that the installed binary reports1.16.6, proving the action did not install whatever latest resolves to at run time.No linked issue, roadmap task, or execplan was found for this branch.
Review walkthrough
Install cargo-binstallstep, the exported version pin, unchanged checksum, and installed-version assertion.Validation
make check-fmt: passed.make typecheck: passed.make lint: passed.make test: passed with719 passed, 93 skipped.https://github.com/cargo-bins/cargo-binstall/releases/download/v1.16.6/cargo-binstall-x86_64-unknown-linux-musl.tgzand the installed binary reported1.16.6.Notes
The requested
cargo binstall --version | grep -q "1.16.6"assertion was adjusted tocargo-binstall -V | grep -q "1.16.6"because cargo-binstall v1.16.6 treats--versionas a package-selection option requiring a value. The-Vflag is the supported binary-version output for this pinned release.Summary by Sourcery
Pin the cargo-binstall installer to a specific version in the setup-rust GitHub Action and verify that the installed binary matches the pinned version.
Enhancements: