Skip to content

fix: respect --home precedence when uninstalling models - #164

Closed
MuhammadAashirAslam wants to merge 1 commit into
RunanywhereAI:mainfrom
MuhammadAashirAslam:fix/uninstall-home-precedence
Closed

MuhammadAashirAslam wants to merge 1 commit into
RunanywhereAI:mainfrom
MuhammadAashirAslam:fix/uninstall-home-precedence

Conversation

@MuhammadAashirAslam

@MuhammadAashirAslam MuhammadAashirAslam commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor

Fix: respect --home when selecting models for uninstall

Summary

wally uninstall could select the model store from RUNANYWHERE_HOME even when
an explicit --home pointed elsewhere. Confirming uninstall could therefore
delete models from the wrong store.

This completes the override handling added in
PR #133.

Changes

  • Use the existing home resolver: --home takes priority over RUNANYWHERE_HOME.
  • Keep the search inside a configured home, even if it has no model directory.
    Only use kit/platform fallback discovery when neither override is set.
  • Preserve both supported layouts: RunAnywhere/Models and Models.
  • Add cross-platform regression coverage and restore test environment variables
    after each test, including on failure.

No SDK, dependency pin, confirmation, or deletion-mechanism changes.

Validation

  • Before the fix: three regression tests failed; two existing/compatibility cases passed.
  • After the fix: all five model-directory tests passed; all 12 maintenance tests
    passed on Windows with the pinned Rust 1.93.1 toolchain.
  • Full library run: 356 passed, two unrelated symlink tests failed because
    Windows denied symlink creation (OS error 1314) in cmd_models and cmd_rm.
  • Formatting, contract consistency, version consistency, and test-ledger checks passed.
  • cargo clippy --release --locked --all-targets -- -D warnings is blocked by
    three pre-existing Windows dead-code warnings in src/harness/path_reload.rs.

Targeted test command (after CMake configuration and build-environment setup):

cargo test --release --locked --lib commands::cmd_maintenance::tests

Local test linking used a temporary import library under ignored build/ for the
installed Git OpenSSL DLL. Without it, the existing Windows kit fails to link
OPENSSL_thread_stop. The original linker settings were restored after testing;
the workaround is not part of the PR.

The agent-sync check reports the checkout's existing CLAUDE.md regular-file
issue; it requires a symlink to AGENTS.md. No agent files were changed.

Full CTest, smoke/e2e, and macOS verification have not been run for this change.


Summary by cubic

Fixes wally uninstall selecting the model store from RUNANYWHERE_HOME even when an explicit --home pointed elsewhere. --home now takes precedence, and any override confines the search to that base, so confirming an uninstall can no longer delete models from the wrong store.

  • Preserves both RunAnywhere/Models and Models layouts.
  • Falls back to kit/platform discovery only when neither --home nor RUNANYWHERE_HOME is set.
  • Adds regression tests covering both layouts and the fallback behavior, and restores test environment variables after each test, including on failure.

Written for commit 544d515. Summary will update on new commits.

Review in cubic

Summary by CodeRabbit

  • Bug Fixes
    • Model-store lookup now prioritizes --home, then RUNANYWHERE_HOME, and uses the platform default only when neither is set.
    • When --home is supplied, missing model stores are reported within that location without falling back to other directories.

@MuhammadAashirAslam
MuhammadAashirAslam requested a review from a team as a code owner October 2, 2026 14:06
@coderabbitai

coderabbitai Bot commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: f1066b7a-36e4-495f-b573-9e99741cf444

📥 Commits

Reviewing files that changed from the base of the PR and between 6e349cb and 544d515.

📒 Files selected for processing (1)
  • src/commands/cmd_maintenance.rs

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.


📝 Walkthrough

Walkthrough

models_directory now prioritizes --home, then RUNANYWHERE_HOME. It uses the platform default only when neither override is set. Tests cover both supported model-store layouts and verify that lookup does not fall back to another base.

Changes

Model directory lookup

Layer / File(s) Summary
Base selection and lookup tests
src/commands/cmd_maintenance.rs
models_directory searches only the explicit home when --home is set. Otherwise, it uses RUNANYWHERE_HOME when set and the platform default only when neither override is set. Tests cover both model-store layouts, override priority, and missing-store behavior.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: siddhesh2377

Merge Risk: ⚪ Minimal · up to 544d5

Uninstall follows the requested home precedence and avoids deleting a model store from another configured base. No actionable merge-blocking risk remains.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 544d5

The change narrows model-store selection to the requested home and preserves the confirmation gate. No introduced security vulnerability was established. Low residual uncertainty remains because deletion behavior during concurrent filesystem changes and interrupted execution could not be fully compared with the previous version.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The assessed exposure is local filesystem deletion using the invoking process's authority. The changed input flow selects a model path from the home argument or process environment; configuration and installation targets are selected separately. Confinement applies to model-store base selection, not to every uninstall target.

Trust Boundaries and Controls

  • observed — Current uninstall displays selected targets before deletion and requires either interactive confirmation or an explicit yes argument. A noninteractive invocation without yes returns before deletion, and cancellation also returns before deletion. Environment-home discovery cannot substitute another model base when an explicit home is supplied.

Resilience and Maintainability Implications

  • observed — Current deletion handles failures per target and defers installation deletion until other targets have been attempted. It is pathname-based rather than transactional, so partial removal or concurrent filesystem replacement is not prevented by target-list reuse. The supplied changed ranges do not modify this mechanism; exact comparison with base behavior remains unavailable.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 55.56% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 9 functions across 1 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: giving explicit --home precedence during model uninstallation.
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.
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.


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

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

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 1 file

Re-trigger cubic

@sanchitmonga22
sanchitmonga22 marked this pull request as draft October 3, 2026 17:41
@shubhammalhotra28

Copy link
Copy Markdown
Contributor

This change looks good - can we test this @Siddhesh2377 ?

@Siddhesh2377

Copy link
Copy Markdown
Collaborator

Superseded: the --home precedence fix is cherry-picked into #168. Closing as redundant — review continues there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants