feat(resources): add managed resource command foundation#244
Merged
Conversation
|
Caution Review failedPull request was closed or merged during review No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (13)
📒 Files selected for processing (7)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a ManagedResourceCommands layer (install/update/list/uninstall), command-domain types/errors and accessors, installer reuse and staging cleanup, a manifest-cache latest-only refresh, DB removal-intent columns/migration, public re-exports, and deterministic integration tests with scripted fakes and artifact fixtures. ChangesManaged Resource Commands API
Sequence Diagram(s)sequenceDiagram
participant Caller
participant ManagedResourceCommands
participant ArtifactManifestCache
participant ResourceHttpClient
participant ArtifactInstaller
participant ResourceAdapter
participant Database
Caller->>ManagedResourceCommands: install(selector, client, adapter)
ManagedResourceCommands->>ArtifactManifestCache: refresh / refresh_latest(manifest_url)
ArtifactManifestCache->>ResourceHttpClient: get_text(manifest_url)
ResourceHttpClient-->>ArtifactManifestCache: manifest JSON
ArtifactManifestCache-->>ManagedResourceCommands: ArtifactManifest
ManagedResourceCommands->>ManagedResourceCommands: select_latest(ArtifactManifest, platform)
ManagedResourceCommands->>ArtifactInstaller: install_existing_release / install(artifact)
ArtifactInstaller->>ResourceAdapter: validate_installation(install_root)
ArtifactInstaller-->>ManagedResourceCommands: ArtifactInstall
ManagedResourceCommands->>Database: record desired/installed state
Caller->>ManagedResourceCommands: uninstall(resource, track, options)
ManagedResourceCommands->>Database: read usage_count / write desired-removed
ManagedResourceCommands-->>Caller: ManagedResourceRemovalIntent
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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 docstrings
🧪 Generate unit tests (beta)
Comment |
![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
🧹 Nitpick comments (2)
crates/resources/tests/managed_resource_commands.rs (1)
159-176: ⚡ Quick winPrefer imported type names over fully-qualified type paths in test signatures.
Import these once at the top (
ManagedResourceInstall,ManagedResourceUpdate,ManagedResourceTrackRecord) and use short type names in signatures for consistency.As per coding guidelines, "PREFER top-level imports over local imports or fully qualified names in Rust".
Also applies to: 191-202
🤖 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 `@crates/resources/tests/managed_resource_commands.rs` around lines 159 - 176, Replace fully-qualified type paths in the test signatures with top-level imports: add use statements for ManagedResourceInstall, ManagedResourceUpdate, and ManagedResourceTrackRecord at the top of the file, then update the impl blocks and function signatures (impl InstallSummary for resources::ManagedResourceInstall, impl InstallSummary for resources::ManagedResourceUpdate, and fn install_summary(install: &resources::ManagedResourceInstall, ...) ) to use the short names ManagedResourceInstall, ManagedResourceUpdate, and ManagedResourceTrackRecord respectively so signatures are consistent with the project guideline.crates/resources/src/command.rs (1)
2-20: ⚡ Quick winPrefer top-level imports instead of fully-qualified names here.
Use imports for
StateErrorandArtifactInstallerto match the Rust style used in nearby code.Suggested diff
-use state::{Database, ManagedResourceDesiredState, ManagedResourceTrackRecord, PvPaths}; +use state::{Database, ManagedResourceDesiredState, ManagedResourceTrackRecord, PvPaths, StateError}; ... use crate::{ - ArtifactDownloader, ArtifactManifestCache, ArtifactManifestSource, ArtifactVersion, + ArtifactDownloader, ArtifactInstaller, ArtifactManifestCache, ArtifactManifestSource, ArtifactVersion, ResourceAdapter, ResourceName, ResourcesError, TargetPlatform, TrackName, TrackSelector, }; ... - State(#[from] state::StateError), + State(#[from] StateError), ... - let install = crate::ArtifactInstaller::new(self.paths.resources()).install( + let install = ArtifactInstaller::new(self.paths.resources()).install(As per coding guidelines, "PREFER top-level imports over local imports or fully qualified names in Rust".
Also applies to: 92-92
🤖 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 `@crates/resources/src/command.rs` around lines 2 - 20, The code uses fully-qualified names (state::StateError and likely ArtifactInstaller) instead of top-level imports; add `use state::StateError;` and `use <crate_path>::ArtifactInstaller;` (replace <crate_path> with the actual module path where ArtifactInstaller lives) at the top of crates/resources/src/command.rs and update occurrences (e.g., the ManagedResourceCommandError::State variant and any ArtifactInstaller references) to use the imported names rather than `state::StateError` or fully-qualified paths so the file follows the project's top-level import style.
🤖 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 `@crates/resources/tests/managed_resource_commands.rs`:
- Around line 259-275: The mocked/scripted client's download implementation
currently maps writer errors to ResourcesError::HttpRequestFailed; update the
error mapping so write_all errors are returned as
ResourcesError::DownloadWriteFailed (include the url and the source.to_string()
as the reason) while keeping the existing pop_front error behavior for missing
byte_responses; locate the download method (working with byte_responses and
write_all) and replace the map_err construction that builds HttpRequestFailed
with one that constructs DownloadWriteFailed.
---
Nitpick comments:
In `@crates/resources/src/command.rs`:
- Around line 2-20: The code uses fully-qualified names (state::StateError and
likely ArtifactInstaller) instead of top-level imports; add `use
state::StateError;` and `use <crate_path>::ArtifactInstaller;` (replace
<crate_path> with the actual module path where ArtifactInstaller lives) at the
top of crates/resources/src/command.rs and update occurrences (e.g., the
ManagedResourceCommandError::State variant and any ArtifactInstaller references)
to use the imported names rather than `state::StateError` or fully-qualified
paths so the file follows the project's top-level import style.
In `@crates/resources/tests/managed_resource_commands.rs`:
- Around line 159-176: Replace fully-qualified type paths in the test signatures
with top-level imports: add use statements for ManagedResourceInstall,
ManagedResourceUpdate, and ManagedResourceTrackRecord at the top of the file,
then update the impl blocks and function signatures (impl InstallSummary for
resources::ManagedResourceInstall, impl InstallSummary for
resources::ManagedResourceUpdate, and fn install_summary(install:
&resources::ManagedResourceInstall, ...) ) to use the short names
ManagedResourceInstall, ManagedResourceUpdate, and ManagedResourceTrackRecord
respectively so signatures are consistent with the project guideline.
🪄 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: CHILL
Plan: Pro Plus
Run ID: 309b7623-05c9-47b9-a7a9-61b32c72f62a
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.lockcrates/resources/tests/snapshots/managed_resource_commands__managed_resource_commands_install_update_list_and_uninstall_fake_adapter.snapis excluded by!**/*.snapcrates/resources/tests/snapshots/managed_resource_commands__managed_resource_commands_keep_installed_state_when_update_validation_fails.snapis excluded by!**/*.snap
📒 Files selected for processing (4)
crates/resources/Cargo.tomlcrates/resources/src/command.rscrates/resources/src/lib.rscrates/resources/tests/managed_resource_commands.rs
Contributor
Author
|
@codex review |
![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: ac49a4f38d
ℹ️ 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".
|
|
||
| for record in installed_tracks { | ||
| let track = TrackName::new(record.track)?; | ||
| installs.push(self.install(adapter, TrackSelector::Track(track), client)?); |
There was a problem hiding this comment.
When the update path delegates to install, it inherits ArtifactManifestCache::refresh's HTTP-error fallback to the cached manifest. In the offline or transient-network-failure case where a cached manifest exists, pv update can therefore succeed against stale metadata or reinstall the cached latest artifact instead of reporting that it could not check the current manifest, contrary to the design requirement that update refreshes the artifact manifest every time. Use an update-specific manifest refresh path that does not fall back to cache, or make the fallback behavior explicit.
Useful? React with 👍 / 👎.
Require update commands to fetch a fresh artifact manifest, reuse existing valid releases before downloading artifacts, and make uninstall intent explicit with prune/force options plus invalid-track guards. Add integration snapshots for stale manifest rejection, idempotent installs, uninstall state durability, reserved alias rejection, and not-installed tracks.
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 `@crates/resources/src/install.rs`:
- Around line 114-129: The current call to adapter.validate_installation(...)
(the reuse path in install()) must not propagate errors; instead, catch
validation failures and treat the reused release as corrupt so install() falls
back to a fresh download. Replace the direct call to
adapter.validate_installation(&release_path)? with a match or if let Err(err)
branch that logs/debugs the validation failure and returns Ok(None) (or
otherwise signals "no reuse") so prune_old_releases, update_current_pointer, and
the ArtifactInstall::new return are skipped; only proceed to pruning/updating
and returning Some(ArtifactInstall::new(...)) when validate_installation
succeeds.
🪄 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: CHILL
Plan: Pro Plus
Run ID: ab30372b-7595-4464-802e-19c19ce533d0
⛔ Files ignored due to path filters (11)
crates/resources/tests/snapshots/artifact_cache_download__manifest_cache_refresh_latest_does_not_fall_back_to_cached_manifest.snapis excluded by!**/*.snapcrates/resources/tests/snapshots/managed_resource_commands__managed_resource_commands_install_update_list_and_uninstall_fake_adapter.snapis excluded by!**/*.snapcrates/resources/tests/snapshots/managed_resource_commands__managed_resource_commands_install_uses_existing_release_without_downloading_again.snapis excluded by!**/*.snapcrates/resources/tests/snapshots/managed_resource_commands__managed_resource_commands_keep_installed_state_when_update_validation_fails.snapis excluded by!**/*.snapcrates/resources/tests/snapshots/managed_resource_commands__managed_resource_commands_report_revoked_latest_fallback.snapis excluded by!**/*.snapcrates/resources/tests/snapshots/managed_resource_commands__managed_resource_commands_uninstall_records_prune_and_force_intent.snapis excluded by!**/*.snapcrates/resources/tests/snapshots/managed_resource_commands__managed_resource_commands_uninstall_rejects_latest_alias_as_concrete_track.snapis excluded by!**/*.snapcrates/resources/tests/snapshots/managed_resource_commands__managed_resource_commands_uninstall_rejects_tracks_that_are_not_installed.snapis excluded by!**/*.snapcrates/resources/tests/snapshots/managed_resource_commands__managed_resource_commands_update_all_installed_tracks_from_one_manifest_refresh.snapis excluded by!**/*.snapcrates/resources/tests/snapshots/managed_resource_commands__managed_resource_commands_update_requires_fresh_manifest_after_cache_exists.snapis excluded by!**/*.snapcrates/resources/tests/snapshots/managed_resource_commands__managed_resource_commands_update_without_installed_tracks_does_not_refresh_manifest.snapis excluded by!**/*.snap
📒 Files selected for processing (6)
crates/resources/src/cache.rscrates/resources/src/command.rscrates/resources/src/install.rscrates/resources/src/lib.rscrates/resources/tests/artifact_cache_download.rscrates/resources/tests/managed_resource_commands.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/resources/src/lib.rs
Comment on lines
+114
to
+129
| let previous_release = current_release_name(adapter.resource_name(), ¤t_path)?; | ||
| adapter.validate_installation(&release_path)?; | ||
| prune_old_releases( | ||
| &releases_dir, | ||
| artifact.artifact_version(), | ||
| previous_release.as_deref(), | ||
| )?; | ||
| update_current_pointer(&track_dir, artifact.artifact_version())?; | ||
|
|
||
| Ok(Some(ArtifactInstall::new( | ||
| adapter.resource_name().clone(), | ||
| track.clone(), | ||
| artifact.artifact_version().clone(), | ||
| release_path, | ||
| current_path, | ||
| ))) |
There was a problem hiding this comment.
Don't let a corrupt reused release block reinstall.
Line 115 currently propagates validate_installation failures out of the reuse path. If releases/<version> exists but is stale/corrupt, install() aborts instead of falling back to a fresh download, so the command cannot self-heal from the exact case this optimization should tolerate.
Suggested fix
let previous_release = current_release_name(adapter.resource_name(), ¤t_path)?;
- adapter.validate_installation(&release_path)?;
+ if adapter.validate_installation(&release_path).is_err() {
+ fs::remove_dir_all_if_exists(&release_path)?;
+ return Ok(None);
+ }
prune_old_releases(
&releases_dir,
artifact.artifact_version(),
previous_release.as_deref(),
)?;📝 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.
Suggested change
| let previous_release = current_release_name(adapter.resource_name(), ¤t_path)?; | |
| adapter.validate_installation(&release_path)?; | |
| prune_old_releases( | |
| &releases_dir, | |
| artifact.artifact_version(), | |
| previous_release.as_deref(), | |
| )?; | |
| update_current_pointer(&track_dir, artifact.artifact_version())?; | |
| Ok(Some(ArtifactInstall::new( | |
| adapter.resource_name().clone(), | |
| track.clone(), | |
| artifact.artifact_version().clone(), | |
| release_path, | |
| current_path, | |
| ))) | |
| let previous_release = current_release_name(adapter.resource_name(), ¤t_path)?; | |
| if adapter.validate_installation(&release_path).is_err() { | |
| fs::remove_dir_all_if_exists(&release_path)?; | |
| return Ok(None); | |
| } | |
| prune_old_releases( | |
| &releases_dir, | |
| artifact.artifact_version(), | |
| previous_release.as_deref(), | |
| )?; | |
| update_current_pointer(&track_dir, artifact.artifact_version())?; | |
| Ok(Some(ArtifactInstall::new( | |
| adapter.resource_name().clone(), | |
| track.clone(), | |
| artifact.artifact_version().clone(), | |
| release_path, | |
| current_path, | |
| ))) |
🤖 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 `@crates/resources/src/install.rs` around lines 114 - 129, The current call to
adapter.validate_installation(...) (the reuse path in install()) must not
propagate errors; instead, catch validation failures and treat the reused
release as corrupt so install() falls back to a fresh download. Replace the
direct call to adapter.validate_installation(&release_path)? with a match or if
let Err(err) branch that logs/debugs the validation failure and returns Ok(None)
(or otherwise signals "no reuse") so prune_old_releases, update_current_pointer,
and the ArtifactInstall::new return are skipped; only proceed to
pruning/updating and returning Some(ArtifactInstall::new(...)) when
validate_installation succeeds.
|
Actionable comments posted: 0 |
This was referenced Jun 6, 2026
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
latestas a concrete track, and reject tracks that are not installedScope
Test plan
Summary by CodeRabbit
New Features
Tests
Chores