Skip to content

fix: respect disable_backends for explicitly declared backend-prefixed tools#8791

Closed
palootcenas-outreach wants to merge 2 commits into
jdx:mainfrom
palootcenas-outreach:fix/disable-backends-explicit-tools
Closed

fix: respect disable_backends for explicitly declared backend-prefixed tools#8791
palootcenas-outreach wants to merge 2 commits into
jdx:mainfrom
palootcenas-outreach:fix/disable-backends-explicit-tools

Conversation

@palootcenas-outreach

Copy link
Copy Markdown
Contributor

Summary

disable_backends is bypassed when tools are declared with explicit backend prefixes (e.g. go:golang.org/x/tools/cmd/goimports or aqua:FiloSottile/age) in config files. This PR fixes get() in src/backend/mod.rs to check both disable_backends and disable_tools/enable_tools before creating new backends on demand.

The Problem

load_tools() correctly filters out disabled backends from the TOOLS map:

tools.retain(|backend| {
    !Settings::get()
        .disable_backends
        .contains(&backend.get_type().to_string())
});

But get() — called during config resolution for explicitly declared tools — finds no entry in the map and silently re-creates the backend without any checks:

pub fn get(ba: &BackendArg) -> Option<ABackend> {
    // ...
    } else if let Some(backend) = arg_to_backend(ba.clone()) {
        // ← creates backend regardless of disable_backends
    }
}

Reproduction

# mise.toml
[tools]
"go:golang.org/x/tools/cmd/goimports" = "latest"
MISE_DISABLE_BACKENDS=go mise install  # Still tries to install go: tools

Why existing tests don't catch this

e2e/backend/test_disable_backends only tests aqua with the short-name tool age (resolved through the registry). Short-name tools go through the load_tools() path and ARE correctly filtered. The bug only manifests with explicit backend-prefixed declarations.

The Fix

Added disable_backends and tool_enabled checks to get() before creating new backends:

pub fn get(ba: &BackendArg) -> Option<ABackend> {
    let mut tools = TOOLS.lock().unwrap();
    let tools_ = tools.as_ref().unwrap();
    if let Some(backend) = tools_.get(&ba.short) {
        Some(backend.clone())
    } else if Settings::get()
        .disable_backends
        .contains(&ba.backend_type().to_string())
    {
        None
    } else if !tool_enabled(
        &Settings::get().enable_tools(),
        &Settings::get().disable_tools(),
        &ba.short,
    ) {
        None
    } else if let Some(backend) = arg_to_backend(ba.clone()) {
        // ... insert and return
    }
}

Also extended the e2e test to cover explicit backend-prefixed tools.

Related

…d tools

When a config file declares a tool with an explicit backend prefix
(e.g. `go:golang.org/x/tools/cmd/goimports` or
`aqua:FiloSottile/age`), the `get()` function in
`src/backend/mod.rs` would bypass the `disable_backends` filter
and re-create the backend on demand via `arg_to_backend()`.

This happened because `load_tools()` correctly filters out disabled
backends from the TOOLS map, but `get()` — called during config
resolution — would find no entry and silently re-create the backend
without checking `disable_backends` or `disable_tools`.

The existing e2e test only covered short-name tools resolved through
the registry (e.g. `age` → aqua), which go through the
`load_tools()` path and ARE correctly filtered.

This commit:
- Adds `disable_backends` and `tool_enabled` checks to `get()`
  before creating new backends
- Extends the e2e test to cover explicit backend-prefixed tools

Discussion: jdx#8789

@gemini-code-assist gemini-code-assist 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.

Code Review

This pull request ensures that the disable_backends setting correctly filters tools with explicit backend prefixes in configuration files. It updates the backend retrieval logic in src/backend/mod.rs to check for disabled backends and tools before instantiation and adds an end-to-end test to verify this behavior. I have no feedback to provide.

@greptile-apps

greptile-apps Bot commented Mar 26, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR fixes a gap in the disable_backends feature: tools declared with an explicit backend prefix (e.g. "aqua:FiloSottile/age" or "go:golang.org/x/tools/cmd/goimports") in config files bypassed the disable_backends filter because get() would call arg_to_backend() unconditionally when the tool was absent from the pre-filtered TOOLS map. The fix inserts the same two guards that load_tools() already uses — disable_backends and tool_enabled — before the arg_to_backend() call, making both code paths consistent.

  • Core fix (src/backend/mod.rs): two early-return None branches added to get() — one for disable_backends, one for tool_enabled — mirror the retain-filters in load_tools().
  • Test extension (e2e/backend/test_disable_backends): a new block writes a .mise.toml with "aqua:FiloSottile/age" = "latest", runs MISE_DISABLE_BACKENDS=aqua mise install, and asserts the tool was not installed, directly exercising the fixed path.
  • Minor observation: the tool_enabled call passes ba.short (e.g. "aqua:FiloSottile/age") as-is; when enable_tools is non-empty and contains only the short alias "age", the explicitly-prefixed form will not match and the tool will be silently disabled — though this is consistent with how load_tools() handles install-state entries and the disable_tools direction works correctly.

Confidence Score: 5/5

Safe to merge; the fix closes a real bypass and the only remaining finding is a P2 edge-case with enable_tools that is consistent with existing behaviour in load_tools().

The change is minimal and targeted, the logic exactly mirrors what load_tools() already does, and the e2e test directly exercises the new path. The one open comment is a P2 observation about enable_tools behaviour with explicit prefixes — not a new regression and not blocking.

No files require special attention.

Important Files Changed

Filename Overview
src/backend/mod.rs Adds disable_backends and tool_enabled guards to get() before arg_to_backend(), mirroring load_tools() logic; minor concern that tool_enabled uses the raw prefixed ba.short rather than normalising to the short name.
e2e/backend/test_disable_backends Extends the e2e test to cover an explicit aqua:-prefixed tool in a .mise.toml config; correctly asserts the tool is not installed when the aqua backend is disabled.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["backend::get(ba)"] --> B{ba.short in\nTOOLS map?}
    B -- yes --> C[return Some clone]
    B -- no --> D{"disable_backends\ncontains ba.backend_type()?"}
    D -- yes --> E[return None ✅ NEW]
    D -- no --> F{"tool_enabled\n(enable_tools, disable_tools, ba.short)?"}
    F -- false --> G[return None ✅ NEW]
    F -- true --> H{arg_to_backend\nreturns Some?}
    H -- yes --> I[insert into TOOLS\nreturn Some backend]
    H -- no --> J[return None]
Loading

Reviews (2): Last reviewed commit: "Merge branch 'main' into fix/disable-bac..." | Re-trigger Greptile

Comment thread src/backend/mod.rs
Comment on lines +223 to +233
} else if Settings::get()
.disable_backends
.contains(&ba.backend_type().to_string())
{
None
} else if !tool_enabled(
&Settings::get().enable_tools(),
&Settings::get().disable_tools(),
&ba.short,
) {
None

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 Multiple Settings::get() calls while holding the lock

Settings::get() is called three times in this block — once for disable_backends, once for enable_tools(), and once for disable_tools(). Each call acquires the BASE_SETTINGS read-lock and clones the Arc. Capturing it once avoids redundant reference-count churn and makes the code easier to follow (a single consistent snapshot of settings for the whole check):

Suggested change
} else if Settings::get()
.disable_backends
.contains(&ba.backend_type().to_string())
{
None
} else if !tool_enabled(
&Settings::get().enable_tools(),
&Settings::get().disable_tools(),
&ba.short,
) {
None
} else if {
let settings = Settings::get();
settings
.disable_backends
.contains(&ba.backend_type().to_string())
|| !tool_enabled(
&settings.enable_tools(),
&settings.disable_tools(),
&ba.short,
)
} {
None

This is a minor style point only — the current code is functionally correct.

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!

@jdx jdx marked this pull request as draft April 4, 2026 16:02
@jdx jdx closed this Apr 27, 2026
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.

2 participants