fix(spm): track artifact bundle options in lock identity#10160
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds target-specific option resolution and lockfile generation for the SPM backend. It refactors SpmOptions to accept an optional PlatformTarget for retrieving configuration values like provider, api_url, and artifactbundle settings, and implements resolve_lockfile_options on SPMBackend. The reviewer suggested normalizing the provider name to lowercase to ensure case-insensitive consistency.
Greptile SummaryThis PR fixes stale-lock reuse for SPM by including
Confidence Score: 5/5Safe to merge; the lock-identity fix is well-scoped and the new error propagation path in All changed code paths are covered by the new and updated unit tests. The No files require special attention. Important Files Changed
Reviews (5): Last reviewed commit: "fix(spm): stop locking invalid artifact ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
📝 WalkthroughWalkthroughThe PR makes SPM backend option resolution target-aware to prevent host-specific fallback during lockfile generation for alternate platforms. Documentation is clarified, a new public ChangesSPM Target-Aware Option Resolution
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
risu729
changed the title
This comment has been minimized.
This comment has been minimized.
This comment was marked as outdated.
This comment was marked as outdated.
![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
🤖 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 `@src/backend/spm.rs`:
- Around line 105-113: The code currently treats Err(_) from
artifactbundle_mode(Some(target)) the same as Auto and drops the option, which
hides invalid inputs; change the Err(_) branch so lock identity reflects invalid
input instead of collapsing to Auto: either propagate the error (return Err with
the artifactbundle error) from the function that builds opts, or insert an
explicit marker into opts (e.g., opts.insert("artifactbundle".to_string(),
format!("invalid:{}", raw_input))) so the invalid value is preserved in the lock
identity; locate the call to artifactbundle_mode and the opts map in the current
function to implement one of these two fixes.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f972e393-6fcc-4cff-b2ba-435d151b4ea9
📒 Files selected for processing (2)
src/backend/mod.rssrc/backend/spm.rs
| match self.artifactbundle_mode(Some(target)) { | ||
| Ok(ArtifactBundleMode::Required) => { | ||
| opts.insert("artifactbundle".to_string(), "true".to_string()); | ||
| } | ||
| Ok(ArtifactBundleMode::SourceOnly) => { | ||
| opts.insert("artifactbundle".to_string(), "false".to_string()); | ||
| } | ||
| Ok(ArtifactBundleMode::Auto) | Err(_) => {} | ||
| } |
There was a problem hiding this comment.
Do not collapse invalid artifactbundle input to Auto in lock identity.
At Line 112, Err(_) is ignored, so invalid values produce the same lock options as unset/auto. That can hide bad config and cause lock identity mismatches across runs.
Suggested direction
- match self.artifactbundle_mode(Some(target)) {
+ match self.artifactbundle_mode(Some(target)) {
Ok(ArtifactBundleMode::Required) => {
opts.insert("artifactbundle".to_string(), "true".to_string());
}
Ok(ArtifactBundleMode::SourceOnly) => {
opts.insert("artifactbundle".to_string(), "false".to_string());
}
- Ok(ArtifactBundleMode::Auto) | Err(_) => {}
+ Ok(ArtifactBundleMode::Auto) => {}
+ Err(err) => {
+ // Avoid silently collapsing invalid user input into Auto identity.
+ // Prefer propagating this via a Result-returning API if feasible.
+ opts.insert("artifactbundle".to_string(), "__invalid__".to_string());
+ debug!("invalid artifactbundle option for lockfile options: {err}");
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| match self.artifactbundle_mode(Some(target)) { | |
| Ok(ArtifactBundleMode::Required) => { | |
| opts.insert("artifactbundle".to_string(), "true".to_string()); | |
| } | |
| Ok(ArtifactBundleMode::SourceOnly) => { | |
| opts.insert("artifactbundle".to_string(), "false".to_string()); | |
| } | |
| Ok(ArtifactBundleMode::Auto) | Err(_) => {} | |
| } | |
| match self.artifactbundle_mode(Some(target)) { | |
| Ok(ArtifactBundleMode::Required) => { | |
| opts.insert("artifactbundle".to_string(), "true".to_string()); | |
| } | |
| Ok(ArtifactBundleMode::SourceOnly) => { | |
| opts.insert("artifactbundle".to_string(), "false".to_string()); | |
| } | |
| Ok(ArtifactBundleMode::Auto) => {} | |
| Err(err) => { | |
| // Avoid silently collapsing invalid user input into Auto identity. | |
| // Prefer propagating this via a Result-returning API if feasible. | |
| opts.insert("artifactbundle".to_string(), "__invalid__".to_string()); | |
| debug!("invalid artifactbundle option for lockfile options: {err}"); | |
| } | |
| } |
🤖 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 `@src/backend/spm.rs` around lines 105 - 113, The code currently treats Err(_)
from artifactbundle_mode(Some(target)) the same as Auto and drops the option,
which hides invalid inputs; change the Err(_) branch so lock identity reflects
invalid input instead of collapsing to Auto: either propagate the error (return
Err with the artifactbundle error) from the function that builds opts, or insert
an explicit marker into opts (e.g., opts.insert("artifactbundle".to_string(),
format!("invalid:{}", raw_input))) so the invalid value is preserved in the lock
identity; locate the call to artifactbundle_mode and the opts map in the current
function to implement one of these two fixes.
2 participants
Summary
filter_binsout of lock identity while still treating it as current install-manifest stateOption behavior
provider: part of lock identity because it chooses the release API/provider used to resolve artifacts. Values are canonicalized to lowercase, and the defaultgithubis omitted from the lock key.api_url: part of lock identity because self-hosted or alternate API endpoints can resolve different releases/assets. Trailing slashes are trimmed so equivalent URLs do not regenerate lock entries.artifactbundle: part of lock identity becausetrue,false, and auto mode select different install paths.truerequires an artifact bundle,falseforces source builds, and auto mode can use artifacts when available.artifactbundle_asset: part of lock identity because it pins the exact artifact bundle asset name to select from a release.filter_bins: not part of lock identity because it only changes which built executables are exposed after the same source/artifact install. It remains an install-manifest option so changing config overrides stale cached layout state.Verification
cargo fmt --checkcargo test test_lockfile_optionscargo test test_resolve_artifactbundle_modecargo test test_lockfile_options_include_artifact_inputs_not_filter_binsSummary by CodeRabbit
Documentation
New Features