refactor(http): prepare installs transactionally#10964
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe backend adds a prepared-install contract, refactors HTTP installation into preparation and execution phases, verifies lockfile contracts before persistent cache or install changes, and expands end-to-end coverage for checksum, size, URL, and cache integrity. ChangesHTTP prepared installation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Backend
participant HTTPPreparation
participant HTTPInstall
participant Lockfile
participant HTTPCache
Backend->>HTTPPreparation: prepare target and install contract
HTTPPreparation-->>Backend: PreparedHttpInstall
Backend->>HTTPInstall: install prepared artifact
HTTPInstall->>Lockfile: verify checksum and size contract
Lockfile-->>HTTPInstall: validation result
HTTPInstall->>HTTPCache: lock and populate cache after validation
HTTPInstall-->>Backend: successful installation
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/backend/http.rs (1)
994-1001: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant pre-population duplicates
verify_lock_contractand records checksum/size before verification.Lines 994-1001 set
platform_info.url/checksum/sizefrom the lock spec, butverify_lock_contract(Line 1021) unconditionally setsurl, and setschecksum/sizewheneverlock_checksum/lock_sizeare present — so this block is a no-op in every case. It's currently safe (verification always runs afterward andtvis dropped on any error), but recording the checksum/size ahead of verification here obscures the "verified-before-recorded" invariant this PR is built around, and would silently persist unverified values if the verification step were ever reordered. Consider dropping the block and lettingverify_lock_contractown enrichment.♻️ Proposed simplification
- // Carry only the prepared contract into the successful result. The - // writer sees this state only when every operation below succeeds. - let platform_info = tv.lock_platforms.entry(spec.target.clone()).or_default(); - platform_info.url = Some(url.clone()); - if let Some(checksum) = &spec.lock_checksum { - platform_info.checksum = Some(checksum.clone()); - } - if let Some(size) = spec.lock_size { - platform_info.size = Some(size); - } - let lockfile_enabled = Settings::get().lockfile_enabled();🤖 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/http.rs` around lines 994 - 1001, Remove the pre-population block that updates platform_info in the flow before verification. Let verify_lock_contract exclusively assign url, checksum, and size after successful validation, preserving the verified-before-recorded invariant and avoiding duplicate enrichment.
🤖 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.
Nitpick comments:
In `@src/backend/http.rs`:
- Around line 994-1001: Remove the pre-population block that updates
platform_info in the flow before verification. Let verify_lock_contract
exclusively assign url, checksum, and size after successful validation,
preserving the verified-before-recorded invariant and avoiding duplicate
enrichment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 3c06423e-1a3e-47f2-879c-8abf601fe901
📒 Files selected for processing (4)
e2e/backend/test_http_lock_install_verifysrc/backend/http.rssrc/backend/mod.rssrc/backend/prepared_install.rs
Greptile SummaryThis PR introduces a
Confidence Score: 4/5Safe to merge for the common HTTP-backend install flow; one edge case (tool stubs in locked mode) was exempted by the old guard in mod.rs but is no longer exempted by the new prepare_http_install check. The new transactional prepare path works correctly for the primary use cases and the E2E tests are comprehensive. However, src/backend/http.rs — specifically Important Files Changed
Reviews (2): Last reviewed commit: "fix(http): tighten prepared install hand..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
This PR currently has failing checks. If this continues for 7 days, it will be closed automatically. This is warning day 1 of 7. Please update the PR when you have a chance. Feel free to reopen or create a new PR if it is closed and you'd like to continue working on it. This comment was generated by an automated workflow. |
1 participant
Summary
PreparedInstall/SuccessfulInstallseam to backend installation, with an explicitLegacypath for every unmigrated backendThis is the first HTTP vertical slice from Migrate lock installs to prepared transactions one backend at a time. It intentionally does not introduce the final global planner, request identity, receipts, plugin bootstrap, or a new lockfile schema.
Tests
mise x cargo -- cargo test backend::http::tests::prepared_http_install_prefers_locked_artifact_contractmise run test:e2e e2e/backend/test_http_lock_install_verifymise run lint-fixThe HTTP E2E coverage verifies locked URL replay after config changes, checksum and size failure atomicity, unchanged lockfile contents, unchanged shared cache contents, and no residual install path, including a request for
latest.Summary by CodeRabbit