Skip to content

feat(bootstrap): add package manager plugin support - #11023

Merged
jdx merged 6 commits into
mainfrom
codex/package-plugin-core
Jul 16, 2026
Merged

jdx merged 6 commits into
mainfrom
codex/package-plugin-core

Conversation

@jdx

@jdx jdx commented Jul 15, 2026 •

Copy link
Copy Markdown
Owner

Summary

  • add PluginType::Package and Lua package installed/install/upgrade hooks to the vfox runtime
  • adapt installed package plugins to SystemPackageManager, including metadata-driven requirements, OS filtering, opaque version matching, and upgrade fallback
  • discover plugin managers alongside built-ins while preserving built-in precedence and rejecting install-time name collisions
  • inject shims and global toolset bin paths into package hook subprocesses

Validation

  • mise run lint-fix
  • mise run test (1,703 unit tests plus E2E task)
  • mise --cd crates/vfox run test (91 passed, 2 ignored)
  • focused package-plugin E2E coverage is included in the stacked follow-up PR

Follow-up

A stacked PR adds [bootstrap.plugins], two-phase bootstrap orchestration, generated CLI/schema artifacts, end-to-end coverage, and documentation.

Out of scope (v1)

  • uninstall/prune of removed package entries (PackageUninstall remains a future extension)
  • registry shorthands for package plugins
  • Windows E2E coverage beyond keeping the vfox code path enabled

Note

Medium Risk
Touches system package orchestration and plugin classification across bootstrap, doctor, and install paths; incorrect availability or hook env could skip or mis-run package installs, but built-in precedence and collision guards limit blast radius.

Overview
Adds package manager plugins as a first-class PluginType::Package, wired through new vfox Lua hooks (PackageInstalled, PackageInstall, PackageUpgrade) and Rust↔Lua types for package lists, dry-run, and opaque version strings.

Plugins that expose hooks/package_install.lua and hooks/package_installed.lua are discovered as managers alongside built-ins via PackagePluginManager, which implements SystemPackageManager using mise.plugin.toml [package-manager] (required binaries, OS filter, version-pin support). Hook subprocesses get PATH from shims plus the global toolset; built-in manager names win on collisions and install is rejected if a package plugin shadows a built-in.

unavailable_reason_async replaces sync is_available() checks in bootstrap, doctor, install hints, system driver/status/use, and system-deps remediation so plugin managers can resolve host tools asynchronously. Package plugins are explicitly not tool backends in BackendArg resolution.

Reviewed by Cursor Bugbot for commit f88dfea. Bugbot is set up for automated code reviews on this repo. Configure here.

Summary by CodeRabbit

  • New Features

    • Added Lua-backed package hook entry points for querying installed packages and triggering install/upgrade actions.
    • Introduced a [package-manager] configuration section for package-manager plugins (requirements, version-pin support, optional platform constraints).
    • Package plugins are now recognized as package managers via package: identifiers, including hook loading and install/upgrade dispatch.
  • Bug Fixes

    • Prevented package plugins from colliding with built-in package manager names.
    • Improved package-manager availability diagnostics and skipping across system status, install, bootstrap, system-use, and dependency remediation by using async unavailability reasoning.

@coderabbitai

coderabbitai Bot commented Jul 15, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds package-plugin support through typed Lua hooks, plugin discovery and configuration, package state/actions, and integration with the system package-manager registry.

Changes

Package plugin support

Layer / File(s) Summary
Package hook API and Lua wiring
crates/vfox/src/hooks/*, crates/vfox/src/lib.rs, crates/vfox/src/lua_mod/hooks.rs, crates/vfox/src/vfox.rs
Adds typed package contexts and responses, Lua serialization, package hook registration, and Vfox dispatch methods for installed, install, and upgrade operations.
Package plugin identity and configuration
src/plugins/mise_plugin_toml.rs, src/plugins/mod.rs, src/plugins/vfox_plugin.rs, src/cli/args/backend_arg.rs, src/toolset/install_state.rs
Adds package plugin variants, TOML configuration, parsing and discovery, CLI handling, collision cleanup, and exclusion from tool initialization.
PackagePluginManager behavior
src/system/packages/plugin.rs
Implements environment checks, package state mapping, version mismatch handling, install/upgrade actions, availability, and version-pin support.
System package-manager registry integration
src/system/packages/*, src/system/mod.rs
Adds plugin-backed manager discovery, built-in name filtering, asynchronous availability contracts, compatible manager name signatures, and manager lookup reuse.
Async availability consumers
src/cli/bootstrap.rs, src/cli/doctor/mod.rs, src/cli/install.rs, src/cli/system/*, src/system/deps.rs, src/toolset/helpers.rs
Updates package status, diagnostics, installation hints, bootstrap reporting, dependency remediation, and system commands to await manager availability reasons.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant PackagePluginManager
  participant Vfox
  participant LuaHook
  PackagePluginManager->>Vfox: dispatch package action
  Vfox->>LuaHook: evaluate package hook
  LuaHook-->>Vfox: return package response
  Vfox-->>PackagePluginManager: map result to package status
Loading

Possibly related PRs

  • jdx/mise#10326: Adds the Vfox Rust-to-Lua package hook contexts and dispatch methods used by the package plugin manager.
  • jdx/mise#10969: Also changes BackendArg backend-type resolution logic, while this PR adds explicit package-plugin handling.

Poem

I’m a rabbit with packages, hopping in line,
Lua hooks bloom and the versions align.
Install or upgrade, the burrow takes flight,
Built-ins avoid plugins that share names outright.
Through Vfox we spring, with responses just right!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 22.06% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
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.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly matches the main change: adding package manager plugin support, especially in bootstrap and the vfox runtime.

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.

Comment thread src/system/packages/plugin.rs Resolved
@greptile-apps

greptile-apps Bot commented Jul 15, 2026 •

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR introduces PluginType::Package — a new category for vfox-based package manager plugins — and wires them into the existing SystemPackageManager pipeline via PackagePluginManager. The three new Lua hook entry-points (PackageInstalled, PackageInstall, PackageUpgrade) are exposed through the vfox crate, and all is_available() / unavailable_reason() call-sites are migrated to the new async unavailable_reason_async() so plugin managers can resolve required binaries from mise's global toolset.

  • New PackagePluginManager: discovers package plugins from install state, guards against built-in-name collisions at install time and at load time, resolves requires binaries against the global toolset path, and delegates status/install/upgrade to Lua hooks.
  • Trait change: SystemPackageManager::name() widens from &'static str to &str, and a new default-implementation unavailable_reason_async() lets plugin managers consult the async toolset PATH while built-in managers fall back to the existing sync checks.
  • Install-state skip: PluginType::Package entries are excluded from the tool install-state index so package plugins are never treated as versioned tool backends.

Confidence Score: 4/5

The new package plugin path is optional and collision-guarded, so it cannot corrupt existing workflows.

The issues flagged in earlier review rounds — particularly the permanent VersionMismatch loop when a plugin confirms installation without reporting a version, and the lock dropped before directory cleanup — are real behavioral defects in the new code paths and warrant fixing before the feature is relied upon in production configs.

src/system/packages/plugin.rs (version-mismatch with None version) and src/system/packages/mod.rs (all_managers rebuilds the full list including TOML reads on every call)

Important Files Changed

Filename Overview
src/system/packages/plugin.rs New PackagePluginManager implementation; contains the version-mismatch-with-None bug (flagged in previous thread) and drops the Lua log receiver, silently discarding hook log output
src/system/packages/mod.rs Adds plugin discovery to all_managers(); rebuilds the full manager list (including disk I/O per plugin) on every invocation — flagged in previous thread
src/plugins/vfox_plugin.rs Adds install-time collision guard; drops plugin_lock before file::remove_all, which is the lock-drop-before-cleanup race covered in the previous thread
crates/vfox/src/hooks/package.rs New Lua hook bindings for PackageInstalled/Install/Upgrade with correct Lua serialization and unit tests; looks correct
src/plugins/mod.rs Adds Package variant to PluginType/PluginEnum with consistent match arm coverage; from_plugin_path detection logic and tests look correct
src/plugins/mise_plugin_toml.rs Adds [package-manager] TOML section parsing with a test covering all three fields; looks correct
src/cli/bootstrap.rs Migrates add_package_skips to async and calls unavailable_reason_async; straightforward mechanical change
src/cli/install.rs Builds an 'available' HashSet using unavailable_reason_async() before the fingerprint/install loop; logic is correct
src/system/deps.rs pick_manager, build_requests, hint_commands made async to use unavailable_reason_async(); straightforward migration
src/toolset/install_state.rs Skips Package plugins in tool init and adds try_list_plugins() returning Option; correct
src/cli/args/backend_arg.rs Adds Package arms to match blocks; correctly bails on tool-backend use and maps Package to BackendType::Unknown

Reviews (6): Last reviewed commit: "fix(plugins): require complete package h..." | Re-trigger Greptile

Comment thread src/plugins/vfox_plugin.rs Resolved
Comment thread src/system/packages/mod.rs Outdated
Comment on lines 142 to 168
pub fn all_managers() -> Vec<Arc<dyn SystemPackageManager>> {
let mut managers = builtin_managers();
let builtins = managers
.iter()
.map(|manager| manager.name().to_string())
.collect::<std::collections::HashSet<_>>();
for (name, plugin_type) in crate::toolset::install_state::list_plugins().iter() {
if *plugin_type != crate::plugins::PluginType::Package {
continue;
}
if builtins.contains(name) {
warn!(
"package plugin '{name}' collides with a built-in package manager; ignoring plugin"
);
continue;
}
match plugin::PackagePluginManager::new(name.clone()) {
Ok(manager) => managers.push(Arc::new(manager)),
Err(err) => warn!("failed to load package plugin '{name}': {err:#}"),
}
}
managers
}

pub fn get_manager(name: &str) -> Option<Arc<dyn SystemPackageManager>> {
all_managers().into_iter().find(|m| m.name() == name)
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 Repeated disk I/O on every all_managers() call

all_managers() is synchronous and rebuilds the full manager list on every invocation, which now includes reading mise.plugin.toml from disk for each installed package plugin (inside PackagePluginManager::new). get_manager() calls all_managers() to find a single entry, meaning a lookup for any package triggers a full plugin-directory scan and TOML parse for every registered package plugin. If bootstrap has several package plugins or if get_manager / all_managers is called on a hot path (e.g. per-package install loop), this accumulates redundant I/O. Consider memoising the list — a once_cell::sync::Lazy or a short-lived cache passed through the call stack would eliminate the repeated work without changing the calling convention.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Fix in Claude Code

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (2)
crates/vfox/src/hooks/package.rs (1)

76-128: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Leverage mlua's sequence traits for cleaner serialization.

Instead of manually constructing and iterating tables for the arrays, you can implement IntoLua for PackageRequest and FromLua for InstalledPackage. mlua automatically provides IntoLua and FromLua for Vec<T>, making the context and response conversions more concise and idiomatic.

♻️ Proposed refactor
-fn packages_into_lua(packages: Vec<PackageRequest>, lua: &Lua) -> mlua::Result<Value> {
-    let list = lua.create_table()?;
-    for (index, package) in packages.into_iter().enumerate() {
-        let item = lua.create_table()?;
-        item.set("name", package.name)?;
-        item.set("version", package.version)?;
-        list.set(index + 1, item)?;
-    }
-    Ok(Value::Table(list))
-}
-
+impl IntoLua for PackageRequest {
+    fn into_lua(self, lua: &Lua) -> mlua::Result<Value> {
+        let table = lua.create_table()?;
+        table.set("name", self.name)?;
+        table.set("version", self.version)?;
+        Ok(Value::Table(table))
+    }
+}
+
 impl IntoLua for PackageInstalledContext {
     fn into_lua(self, lua: &Lua) -> mlua::Result<Value> {
         let table = lua.create_table()?;
-        table.set("packages", packages_into_lua(self.packages, lua)?)?;
+        table.set("packages", self.packages)?;
         Ok(Value::Table(table))
     }
 }
 
 impl IntoLua for PackageActionContext {
     fn into_lua(self, lua: &Lua) -> mlua::Result<Value> {
         let table = lua.create_table()?;
-        table.set("packages", packages_into_lua(self.packages, lua)?)?;
+        table.set("packages", self.packages)?;
         table.set("dry_run", self.dry_run)?;
         table.set("update", self.update)?;
         Ok(Value::Table(table))
     }
 }
 
+impl FromLua for InstalledPackage {
+    fn from_lua(value: Value, _: &Lua) -> mlua::Result<Self> {
+        let Value::Table(table) = value else {
+            return Err(LuaError::FromLuaConversionError {
+                from: value.type_name(),
+                to: "InstalledPackage".to_string(),
+                message: Some("Expected table".to_string()),
+            });
+        };
+        Ok(Self {
+            name: table.get("name")?,
+            state: table.get("state")?,
+            version: table.get("version")?,
+        })
+    }
+}
+
 impl FromLua for PackageInstalledResponse {
     fn from_lua(value: Value, _: &Lua) -> mlua::Result<Self> {
         let Value::Table(table) = value else {
             return Err(LuaError::FromLuaConversionError {
                 from: value.type_name(),
                 to: "PackageInstalledResponse".to_string(),
                 message: Some("Expected table".to_string()),
             });
         };
-        let packages = table
-            .get::<mlua::Table>("packages")?
-            .sequence_values::<mlua::Table>()
-            .map(|item| {
-                let item = item?;
-                Ok(InstalledPackage {
-                    name: item.get("name")?,
-                    state: item.get("state")?,
-                    version: item.get("version")?,
-                })
-            })
-            .collect::<mlua::Result<Vec<_>>>()?;
+        let packages = table.get("packages")?;
         Ok(Self { packages })
     }
 }
🤖 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/vfox/src/hooks/package.rs` around lines 76 - 128, Refactor the Lua
conversions by implementing IntoLua for PackageRequest and FromLua for
InstalledPackage, moving each item’s field mapping into its respective
conversion. Then replace packages_into_lua and the manual sequence iteration in
PackageInstalledResponse::from_lua with direct
Vec<PackageRequest>/Vec<InstalledPackage> table conversions, while preserving
the existing field names and error behavior.
src/system/packages/plugin.rs (1)

1-232: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

No unit tests for the new PackagePluginManager logic.

Pure helpers like platform_available(), missing_from_path(), and the state-mapping logic in installed() are new and non-trivial (opaque version matching, OS filtering, upgrade-hook fallback) but this file has no #[cfg(test)] module. Consider at least covering platform_available/missing_from_path with a temp-dir fixture and the installed() state-mapping branches (installed/missing/mismatch/invalid-state/unreturned) with a fake PackageInstalledResponse.

🤖 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/system/packages/plugin.rs` around lines 1 - 232, Add a #[cfg(test)]
module for PackagePluginManager covering platform_available and
missing_from_path with temporary PATH fixtures, plus installed state mapping
using a fake PackageInstalledResponse for installed, missing, version mismatch,
invalid-state, and unreturned-package cases. Reuse existing constructors or test
helpers where possible and keep production behavior unchanged.
🤖 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/system/packages/plugin.rs`:
- Around line 41-66: Update the availability checks around platform_available,
sync_lookup_path, and missing_from_path to construct the lookup PATH using the
hook environment’s toolset.list_paths(&config) entries in addition to the
process PATH and shims directory. Ensure is_available() and unavailable_reason()
use this same hook PATH when resolving required binaries, so tools declared in
[tools] are recognized before install() or upgrade() is skipped.

---

Nitpick comments:
In `@crates/vfox/src/hooks/package.rs`:
- Around line 76-128: Refactor the Lua conversions by implementing IntoLua for
PackageRequest and FromLua for InstalledPackage, moving each item’s field
mapping into its respective conversion. Then replace packages_into_lua and the
manual sequence iteration in PackageInstalledResponse::from_lua with direct
Vec<PackageRequest>/Vec<InstalledPackage> table conversions, while preserving
the existing field names and error behavior.

In `@src/system/packages/plugin.rs`:
- Around line 1-232: Add a #[cfg(test)] module for PackagePluginManager covering
platform_available and missing_from_path with temporary PATH fixtures, plus
installed state mapping using a fake PackageInstalledResponse for installed,
missing, version mismatch, invalid-state, and unreturned-package cases. Reuse
existing constructors or test helpers where possible and keep production
behavior unchanged.
🪄 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: Central YAML (base), Organization UI (inherited)

Review profile: CHILL

Plan: Pro Plus

Run ID: bb52afcb-d922-467b-908c-240f36907ca9

📥 Commits

Reviewing files that changed from the base of the PR and between 50d0ef3 and ab5ad73.

📒 Files selected for processing (21)
  • crates/vfox/src/hooks/mod.rs
  • crates/vfox/src/hooks/package.rs
  • crates/vfox/src/lib.rs
  • crates/vfox/src/lua_mod/hooks.rs
  • crates/vfox/src/vfox.rs
  • src/cli/args/backend_arg.rs
  • src/cli/doctor/mod.rs
  • src/plugins/mise_plugin_toml.rs
  • src/plugins/mod.rs
  • src/plugins/vfox_plugin.rs
  • src/system/packages/apk.rs
  • src/system/packages/apt.rs
  • src/system/packages/brew/cask.rs
  • src/system/packages/brew/mod.rs
  • src/system/packages/dnf.rs
  • src/system/packages/flatpak.rs
  • src/system/packages/mas.rs
  • src/system/packages/mod.rs
  • src/system/packages/pacman.rs
  • src/system/packages/plugin.rs
  • src/toolset/install_state.rs

Comment on lines +41 to +66
fn platform_available(&self) -> bool {
self.config.os.as_ref().is_none_or(|oses| {
oses.iter()
.any(|os| os == crate::config::Settings::get().os())
})
}

fn sync_lookup_path() -> Vec<PathBuf> {
let mut paths: Vec<PathBuf> = std::env::var_os("PATH")
.map(|path| split_paths(&path).collect())
.unwrap_or_default();
if !paths.iter().any(|path| path == *crate::dirs::SHIMS) {
paths.push(crate::dirs::SHIMS.to_path_buf());
}
paths
}

fn missing_from_path(&self, paths: &[PathBuf]) -> Option<&str> {
let path = join_paths(paths).ok()?;
let cwd = std::env::current_dir().ok()?;
self.config
.requires
.iter()
.find(|binary| which::which_in(binary, Some(&path), &cwd).is_err())
.map(String::as_str)
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check how is_available()/unavailable_reason() are consumed for package managers,
# to confirm whether unavailable managers are filtered out before install/upgrade runs.
rg -n -B3 -A10 'is_available\(\)' src/cli/system/driver.rs src/system/packages/mod.rs

Repository: jdx/mise

Length of output: 2223


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Map the plugin and package manager files first.
ast-grep outline src/system/packages/plugin.rs --view expanded
echo '---'
ast-grep outline src/system/packages/mod.rs --view expanded
echo '---'
rg -n -A4 -B4 'hook_env|checked_hook_env|list_paths|sync_lookup_path|missing_from_path|is_available|unavailable_reason' src/system/packages/plugin.rs src/system/packages/mod.rs src/cli/system/driver.rs

Repository: jdx/mise

Length of output: 12169


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Read the relevant slices if outlines are too coarse.
sed -n '1,220p' src/system/packages/plugin.rs

Repository: jdx/mise

Length of output: 7767


Use the hook PATH for availability checks is_available() / unavailable_reason() only inspect the raw process PATH, but the hook env also adds toolset.list_paths(&config). A package plugin whose required binary comes from [tools] can still run in install()/upgrade(), yet it gets skipped before those paths are considered.

🤖 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/system/packages/plugin.rs` around lines 41 - 66, Update the availability
checks around platform_available, sync_lookup_path, and missing_from_path to
construct the lookup PATH using the hook environment’s
toolset.list_paths(&config) entries in addition to the process PATH and shims
directory. Ensure is_available() and unavailable_reason() use this same hook
PATH when resolving required binaries, so tools declared in [tools] are
recognized before install() or upgrade() is skipped.

Comment thread src/system/packages/mod.rs Resolved
Comment thread src/system/packages/plugin.rs Resolved

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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/cli/bootstrap.rs`:
- Around line 1155-1161: Short-circuit asynchronous availability checks for
disabled package managers. In src/cli/bootstrap.rs lines 1155-1161,
src/cli/doctor/mod.rs lines 481-487 and 717-723, and src/cli/system/status.rs
lines 31-37, update each reason-selection flow to use the disabled-manager
message directly when mp.disabled is true and only await
mp.manager.unavailable_reason_async() when it is false, preserving the existing
unavailable-reason behavior otherwise.
🪄 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: Central YAML (base), Organization UI (inherited)

Review profile: CHILL

Plan: Pro Plus

Run ID: af7d5b13-1a66-497d-998c-09b33fe217d5

📥 Commits

Reviewing files that changed from the base of the PR and between ab5ad73 and 99e50f5.

📒 Files selected for processing (8)
  • src/cli/bootstrap.rs
  • src/cli/doctor/mod.rs
  • src/cli/install.rs
  • src/cli/system/driver.rs
  • src/cli/system/status.rs
  • src/cli/system/use.rs
  • src/system/packages/mod.rs
  • src/system/packages/plugin.rs
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/system/packages/mod.rs
  • src/system/packages/plugin.rs

Comment thread src/cli/bootstrap.rs Outdated
Comment on lines 1155 to 1161
let unavailable = mp.manager.unavailable_reason_async().await;
if mp.disabled || unavailable.is_some() {
let reason = if mp.disabled {
"excluded by the system_packages.managers setting".to_string()
} else {
mp.manager.unavailable_reason()
unavailable.unwrap()
};

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win

Short-circuit async availability checks for disabled package managers.

mp.manager.unavailable_reason_async().await is awaited unconditionally before checking mp.disabled. For package plugins, computing this reason involves spawning a vfox subprocess to extract the plugin's environment variables. If a package manager is disabled via settings, we shouldn't pay the cost of executing its hook or risk a failure if the plugin is broken.

Refactor these sites to lazily evaluate the async availability check only when mp.disabled is false:

  • src/cli/bootstrap.rs#L1155-L1161: defer the async call by determining the reason directly.
  • src/cli/doctor/mod.rs#L481-L487: apply the same short-circuit logic here.
  • src/cli/doctor/mod.rs#L717-L723: apply the same short-circuit logic here.
  • src/cli/system/status.rs#L31-L37: apply the same short-circuit logic here.
⚡ Proposed fix (applicable to all occurrences)
-            let unavailable = mp.manager.unavailable_reason_async().await;
-            if mp.disabled || unavailable.is_some() {
-                let reason = if mp.disabled {
-                    "excluded by the system_packages.managers setting".to_string()
-                } else {
-                    unavailable.unwrap()
-                };
+            let reason = if mp.disabled {
+                Some("excluded by the system_packages.managers setting".to_string())
+            } else {
+                mp.manager.unavailable_reason_async().await
+            };
+            if let Some(reason) = reason {
📝 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 unavailable = mp.manager.unavailable_reason_async().await;
if mp.disabled || unavailable.is_some() {
let reason = if mp.disabled {
"excluded by the system_packages.managers setting".to_string()
} else {
mp.manager.unavailable_reason()
unavailable.unwrap()
};
let reason = if mp.disabled {
Some("excluded by the system_packages.managers setting".to_string())
} else {
mp.manager.unavailable_reason_async().await
};
if let Some(reason) = reason {
📍 Affects 3 files
  • src/cli/bootstrap.rs#L1155-L1161 (this comment)
  • src/cli/doctor/mod.rs#L481-L487
  • src/cli/doctor/mod.rs#L717-L723
  • src/cli/system/status.rs#L31-L37
🤖 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/cli/bootstrap.rs` around lines 1155 - 1161, Short-circuit asynchronous
availability checks for disabled package managers. In src/cli/bootstrap.rs lines
1155-1161, src/cli/doctor/mod.rs lines 481-487 and 717-723, and
src/cli/system/status.rs lines 31-37, update each reason-selection flow to use
the disabled-manager message directly when mp.disabled is true and only await
mp.manager.unavailable_reason_async() when it is false, preserving the existing
unavailable-reason behavior otherwise.

@github-actions

github-actions Bot commented Jul 15, 2026 •

Copy link
Copy Markdown
Contributor

Hyperfine Performance

mise x -- echo

Command Mean [ms] Min [ms] Max [ms] Relative
mise-2026.7.7 x -- echo 22.9 ± 1.7 19.9 37.4 1.02 ± 0.11
mise x -- echo 22.4 ± 1.7 18.8 35.9 1.00

mise env

Command Mean [ms] Min [ms] Max [ms] Relative
mise-2026.7.7 env 22.1 ± 0.9 19.8 25.8 1.02 ± 0.09
mise env 21.7 ± 1.7 18.8 28.6 1.00

mise hook-env

Command Mean [ms] Min [ms] Max [ms] Relative
mise-2026.7.7 hook-env 23.2 ± 1.4 20.2 27.7 1.01 ± 0.11
mise hook-env 22.9 ± 2.0 19.6 35.4 1.00

mise ls

Command Mean [ms] Min [ms] Max [ms] Relative
mise-2026.7.7 ls 23.2 ± 2.5 18.4 32.5 1.14 ± 0.15
mise ls 20.4 ± 1.6 16.7 27.4 1.00
✅ Performance improvement for ls is 14%

xtasks/test/perf

Command mise-2026.7.7 mise Variance
install (cached) 191ms 190ms +0%
ls (cached) 73ms 73ms +0%
bin-paths (cached) 80ms 76ms +5%
task-ls (cached) 163ms 164ms +0%

jdx added 2 commits July 15, 2026 14:29
Entire-Checkpoint: 8164a9ce23de
Entire-Checkpoint: e0ec02b5b8d1
Comment thread src/system/packages/mod.rs Resolved

@cursor cursor 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.

Cursor Bugbot has reviewed your changes and found 2 potential issues.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 05666e8. Configure here.

Comment thread src/plugins/mod.rs Resolved
Comment thread src/plugins/mod.rs Resolved

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
src/system/deps.rs (1)

476-481: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Short-circuit the async availability check.

Check if dep.packages contains the manager's name before awaiting its availability. This avoids potentially slow async checks (such as spawning a subprocess to verify the manager) for package managers that the dependency does not even support.

⚡ Proposed optimization
-        if m.unavailable_reason_async().await.is_some() {
-            continue;
-        }
-        if dep.packages.contains_key(name) {
-            return Some(m);
-        }
+        if !dep.packages.contains_key(name) {
+            continue;
+        }
+        if m.unavailable_reason_async().await.is_some() {
+            continue;
+        }
+        return Some(m);
🤖 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/system/deps.rs` around lines 476 - 481, In the manager selection flow,
update the logic around unavailable_reason_async() so
dep.packages.contains_key(name) is checked first and managers absent from the
dependency are skipped without awaiting availability. Preserve returning Some(m)
only for supported managers that pass the availability check.
🤖 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/system/deps.rs`:
- Around line 476-481: In the manager selection flow, update the logic around
unavailable_reason_async() so dep.packages.contains_key(name) is checked first
and managers absent from the dependency are skipped without awaiting
availability. Preserve returning Some(m) only for supported managers that pass
the availability check.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Central YAML (base), Organization UI (inherited)

Review profile: CHILL

Plan: Pro Plus

Run ID: 48cd6849-f786-4dbb-89c5-56de35b5ef69

📥 Commits

Reviewing files that changed from the base of the PR and between 1e7a563 and 05666e8.

📒 Files selected for processing (2)
  • src/system/deps.rs
  • src/toolset/helpers.rs

Entire-Checkpoint: 1fbf88e7c9c2
@jdx
jdx merged commit 2137ed0 into main Jul 16, 2026
35 checks passed
@jdx
jdx deleted the codex/package-plugin-core branch July 16, 2026 03:06
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.

1 participant