fix(install): verify cached artifacts before extract

[tonythethompson]

Summary

• Always verify the on-disk download cache against artifact_sha256 before extraction, so cache hits can no longer bypass integrity checks
• Add install integration coverage for provisional warn-once (fallback reason on new install; silent already-installed no-op) and corrupted-cache rejection
• Verified against current master after feat(client): display and gate on provisional evidence tier (P6) #115: EvidenceTier enum + unknown-tier rejection test are already present; left unchanged

Test plan

• cargo test --test install_test -- provisional corrupted full_install
• cargo test
• cargo clippy -- -D warnings
• cargo fmt --check

Made with Cursor

[sourcery-ai]

Sorry @tonythethompson, you have reached your weekly rate limit of 500000 diff characters.

Please try again later or upgrade to continue using Sourcery

[coderabbitai]

Warning

Review limit reached

@tonythethompson, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 43 minutes

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 18fe29b3-f877-4f15-801f-09b3962ebea1

📥 Commits

Reviewing files that changed from the base of the PR and between 97511cb and f201ce6.

📒 Files selected for processing (3)

• src/core/integrity.rs
• src/install/transaction.rs
• tests/install_test.rs

📝 Walkthrough

Walkthrough

The install transaction now verifies cached archives before extraction. Integration tests cover corrupted cache failures and provisional-package warning behavior across first and repeated installs.

Changes

Install validation

Layer / File(s)	Summary
Cached artifact integrity validation src/install/transaction.rs, tests/install_test.rs	Cached artifacts are checked against their declared SHA-256 before extraction. Corrupted cached content fails with an integrity error and produces no package payload.
Provisional-package warning coverage tests/install_test.rs	Integration coverage verifies the lifecycle-testing warning and fallback reason on a new install, and verifies no warning on a subsequent already-installed no-op.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

• tonythethompson/numan#115: Extends provisional-package install behavior covered by this change.

🚥 Pre-merge checks | ✅ 8

✅ Passed checks (8 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly states that cached artifacts are verified before extraction, which is the primary change.
Description check	✅ Passed	The description accurately covers cache verification, integration tests, and validation results for the changeset.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Pipeline Stage Enum Ordering	✅ Passed	The PR changes only install transaction and integration tests; repository and diff searches found no SessionWorkflowStage enum, member references, or stage comparisons.
Gpu/Cpu Runtime Boundary	✅ Passed	The PR changes only src/install/transaction.rs and tests/install_test.rs; no inference, managed requirements, main.py, or C# runtime-boundary files are modified.
Managed Host Restart Safety	✅ Passed	The patch changes only install/cache code and integration tests; none of the four managed-host components or restart/lease paths are modified or present.

✨ Finishing Touches 💡 1

🛠️ Fix failing CI checks 💡

• Create stacked PR
• Commit on current branch

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/install-cache-integrity-and-provisional-tests

✨ Simplify code

• Create PR with simplified code
• Commit simplified code in branch fix/install-cache-integrity-and-provisional-tests

Warning

Review ran into problems

🔥 Problems

Linked repositories: Public OSS repositories can only analyze public repositories installed in this organization. Analyzed tonythethompson/QuickShell, tonythethompson/numan, tonythethompson/dependency-chain-substrate, skipped Trackdubllc/Trackdub.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[qodo-code-review]

PR Summary by Qodo

Verify cached install artifacts before extraction

🐞 Bug fix 🧪 Tests 🕐 40+ Minutes

AI Description

• Always re-verify cached download artifacts against artifact_sha256 before extraction.
• Prevent corrupted cache hits from bypassing integrity checks and reaching extraction.
• Add install integration tests for provisional warn-once behavior and corrupted-cache rejection.

Diagram

graph TD
  T[["install integration tests"]] --> I["install_package()"] --> C[("download cache file")] --> V["integrity::verify_and_report"] --> X["extract/install payload"]
  T --> P{"provisional?"} --> W["warn once on new install"]
  subgraph Legend
    direction LR
    _t[["Test"]] ~~~ _p["Process"] ~~~ _db[("Cache") ] ~~~ _d{"Decision"}
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Persist a verified-marker for cache entries

• ➕ Avoids re-hashing large artifacts on every cache hit
• ➕ Keeps security guarantee if marker is bound to expected SHA and file metadata
• ➖ Needs careful invalidation (mtime/size changes) and atomic writes
• ➖ More state/complexity than always-verifying

2. Verify only at extraction boundary via streaming hash

• ➕ Single verification point regardless of cache/download source
• ➕ Potentially avoids hashing twice (download + cache) if pipelined
• ➖ Requires refactoring extraction path to be hash-aware/streaming
• ➖ Harder to keep clean separation between integrity and unpacking

3. Use OS-level immutable cache storage (read-only)

• ➕ Reduces risk of post-download tampering
• ➕ Can make “trust cache” story simpler operationally
• ➖ Not portable or reliable across environments
• ➖ Still benefits from cryptographic verification as a defense-in-depth layer

Recommendation: Keep the PR’s approach (always re-verify the on-disk cached artifact before extraction). It’s the simplest change that closes the integrity bypass, and the added integration tests validate both the provisional warning semantics and that corrupted cache content fails before any extraction side effects.

Files changed (2) +232 / -1

Bug fix (1) +5 / -0

transaction.rsRe-verify cached artifact integrity before extraction +5/-0

Re-verify cached artifact integrity before extraction

• Adds an integrity verification step on the promoted on-disk cache file when 'artifact_sha256' is available. This ensures cache hits cannot bypass 'integrity::verify_and_report' and prevents corrupted cache entries from reaching extraction.

src/install/transaction.rs

Tests (1) +227 / -1

install_test.rsAdd integration tests for provisional warn-once and corrupted cache +227/-1

Add integration tests for provisional warn-once and corrupted cache

• Introduces an integration test that runs installs in a child process to assert the provisional warning is emitted only on a new install, not on an already-installed no-op. Adds coverage that plants a corrupted cache entry and asserts install fails integrity checks and does not extract any package payload.

tests/install_test.rs

[greptile-apps]

Greptile Summary

The PR verifies cached artifacts against their registry-provided SHA-256 before extraction and redownloads corrupt cache entries.

• Reuses the observed digest after successful verification to avoid hashing verified artifacts twice.
• Adds integration coverage for corrupt-cache repair and provisional install warning behavior.
• Adds unit coverage for the digest-returning integrity API.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename	Overview
src/core/integrity.rs	Updates integrity verification to return the successfully observed digest and adds matching success and failure tests.
src/install/transaction.rs	Verifies cache hits before extraction, repairs mismatched entries through a fresh verified download, and reuses the resulting digest.
tests/install_test.rs	Adds integration tests for corrupted-cache recovery and one-time provisional install warnings.

_{Reviews (2): Last reviewed commit: "fix(install): self-heal corrupt cache an..." | Re-trigger Greptile}

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

1. Corrupted cache repeats failure ✓ Resolved 🐞 Bug ☼ Reliability

Description

On a cache hit, a SHA mismatch now aborts the install but leaves the corrupted cache file in place,
so subsequent installs will keep failing until the user manually deletes the cache entry or uses
--force. This makes cache corruption a sticky failure mode instead of self-healing via re-download.

Code

src/install/transaction.rs[R229-232]

+    // Re-check the promoted cache file so cache hits cannot bypass integrity.
+    if let Some(ref expected_sha) = artifact_sha256 {
+        integrity::verify_and_report(&cache_file, expected_sha, &lock_key)?;
+    }

Relevance

●●● Strong

Team often prefers self-healing/cleanup on corrupt state; deleting bad cache prevents sticky
failures.

PR-#106

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

install_package() only downloads when the cache file is missing (or --force), and the new
cache-hit verification calls verify_and_report() which bails on mismatch without deleting the
cache file. Therefore a corrupted cache entry will persist and repeatedly fail on subsequent
installs.

src/install/transaction.rs[206-233]
src/core/integrity.rs[43-54]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`install_package()` now verifies the promoted cache file on cache hits, but on an integrity mismatch it returns an error and leaves the corrupted cache entry in place. Because the download path is only taken when `!cache_file.exists()` or `--force`, subsequent installs will repeatedly hit the same bad cache file and fail again.

### Issue Context
This behavior is newly activated by the added cache-hit verification. The integrity helper currently only reports failure; it does not invalidate cache entries.

### Fix Focus Areas
- src/install/transaction.rs[206-233]
- src/core/integrity.rs[43-54]

### Implementation notes
- When verifying `cache_file` fails, treat it as a corrupted cache entry:
 - remove/quarantine `cache_file` (and possibly any lingering `.part`),
 - then re-download to `.part`, verify, and promote, and continue.
- Limit retry to 1 attempt to avoid loops (e.g., if the network keeps returning corrupted data).
- Consider adding context to the error if deletion/redownload also fails.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Extra full-file hash pass ✓ Resolved 🐞 Bug ➹ Performance

Description

The added cache-file integrity check introduces an additional full-file SHA-256 pass, and the code
then re-reads the file to compute payload_sha256, increasing I/O and install latency for large
artifacts. This performance cost is directly caused by adding the extra verification call on the
cache file.

Code

src/install/transaction.rs[R229-232]

+    // Re-check the promoted cache file so cache hits cannot bypass integrity.
+    if let Some(ref expected_sha) = artifact_sha256 {
+        integrity::verify_and_report(&cache_file, expected_sha, &lock_key)?;
+    }

Relevance

●● Moderate

Extra hashing/I-O is real but optimization is non-trivial and could be deprioritized vs integrity
clarity.

PR-#6

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

verify_and_report() computes a streaming SHA-256 over the file, and install_package() then reads
the whole file again to compute payload_sha256, so the new cache-hit verification adds a second
full-file digest pass.

src/core/integrity.rs[6-24]
src/install/transaction.rs[229-241]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`verify_and_report()` computes the SHA-256 of `cache_file`, and immediately after that `install_package()` reads the entire file again to compute `payload_sha256`. With the new cache-hit verification, this guarantees at least two full-file hashing passes on every install.

### Issue Context
The added cache-hit verification is important for integrity, but the digest computation can be reused to avoid duplicated I/O.

### Fix Focus Areas
- src/install/transaction.rs[229-241]
- src/core/integrity.rs[6-55]

### Implementation notes
- Option A: change `integrity::verify_and_report()` (or add a new helper) to return the computed digest (actual SHA) on success.
- Option B: compute the digest once in `install_package()` via a streaming file-hash helper, compare to `expected_sha`, and also store it into `payload_sha256` without `std::fs::read()`.
- Keep the fail-closed behavior on mismatches.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Context used

✅ Compliance rules (platform): 31 rules

✅ REVIEW.md

Review mode: ⚖️ Balanced: This changes install-time integrity behavior on a security-sensitive artifact path and adds substantial integration-test logic, but has only a few edit sites rather than the defect density warranting extended review.

Tip of the day

💡 Did you know, you can reply 'qodo' on any finding to push back, ask questions, or dig deeper

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[qodo-code-review]

Qodo Fixer

No findings are within the configured fix scope. To change which findings are fixed, adjust the setting on your Qodo configuration page.

[linear-code]

TS-185