Skip to content

fix(config): treat enable_tools empty as disable-all#9108

Merged
jdx merged 3 commits intojdx:mainfrom
risu729:copilot/enable-tools-empty
Apr 18, 2026
Merged

fix(config): treat enable_tools empty as disable-all#9108
jdx merged 3 commits intojdx:mainfrom
risu729:copilot/enable-tools-empty

Conversation

@risu729
Copy link
Copy Markdown
Contributor

@risu729 risu729 commented Apr 15, 2026

Summary

  • distinguish unset enable_tools from explicitly configured enable_tools
  • make explicit empty values disable all tools while unset continues to mean no filter
  • normalize configured tool names before filtering
  • document allowlist precedence over disable_tools and regenerate schema
  • add e2e coverage for empty env and empty settings list behavior

Related

Tests

  • cargo fmt --all --check
  • mise run render:schema
  • mise run test:unit test_tool_disabled
  • mise run test:unit test_normalize_tool_names
  • mise run test:e2e e2e/config/test_config_enable_tools_empty

This PR was originally generated by Copilot.

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

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 modifies the behavior of the enable_tools setting to distinguish between being unset and being explicitly set to an empty list. When unset, all tools remain enabled (unless explicitly disabled); when set to an empty list or empty string, all tools are now disabled. This change involves updating the configuration schema, settings documentation, and refactoring internal logic across the backend, CLI, and toolset modules to handle enable_tools as an Option. I have no feedback to provide.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Apr 15, 2026

Greptile Summary

This PR fixes enable_tools semantics by distinguishing between "not configured" (None → all tools enabled) and "explicitly empty" (Some(BTreeSet::new()) → all tools disabled). It changes the field from a BTreeSet<String> with a default of [] to Option<BTreeSet<String>> with optional = true, adds a normalize_tool_names helper that trims whitespace and filters empty entries (covering TOML-configured values that bypass set_by_comma), and threads the Option through tool_enabled, Toolset::is_disabled, ToolRequestSetBuilder, and the two CLI filter_enabled functions. Documentation, schema, and e2e tests are all updated.

Confidence Score: 5/5

Safe to merge — logic is correct, all call sites updated consistently, and the new semantics are covered by both unit and e2e tests.

No P0 or P1 issues found. The Option-based distinction between unset and explicit-empty is correctly threaded through every filtering site (Toolset, ToolRequestSetBuilder, backend loader, registry/search CLIs). The normalize_tool_names helper is defensive but correct — it handles TOML-configured values that bypass set_by_comma. Unit tests cover all five combinations of None/Some(empty)/Some(set) × enable/disable, and the e2e test validates the env-var and TOML-empty paths end-to-end.

No files require special attention.

Important Files Changed

Filename Overview
src/registry.rs Core logic change: tool_enabled now takes Option<&BTreeSet<T>> to distinguish unset (None) from empty allowlist (Some(empty)); unit tests cover all five key cases.
src/config/settings.rs Adds enable_tools() returning Option<BTreeSet<String>> and normalize_tool_names helper; disable_tools() also normalised; unit tests cover trimming and empty-string filtering.
src/toolset/tool_request_set.rs Builder's enable_tools field changed to Option<BTreeSet<BackendArg>>; correctly maps None through from settings.
src/toolset/mod.rs Toolset::is_disabled updated to call settings.enable_tools() and pass Option reference to tool_enabled.
src/backend/mod.rs Backend list filtering updated to use enable_tools.as_ref() — correctly suppresses all backends when the allowlist is Some(empty).
e2e/config/test_config_enable_tools_empty New e2e test covering unset, empty-string, whitespace-only, and TOML-empty enable_tools; also validates trailing-comma and space-padded comma-separated lists.
settings.toml Changed enable_tools from default = [] to optional = true; description and doc block updated to document allowlist precedence over disable_tools.
src/cli/registry.rs CLI filter_enabled updated to use enable_tools.as_ref().
src/cli/search.rs Same mechanical update as registry.rsfilter_enabled uses enable_tools.as_ref().

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Settings loaded] --> B{enable_tools field}
    B -- "not present in config\nand env var unset" --> C["None"]
    B -- "env var set to empty string\nor whitespace-only\nor TOML enable_tools = []" --> D["Some(BTreeSet::new())"]
    B -- "env var / TOML has values" --> E["Some({node, python, ...})"]

    C --> F["enable_tools() → None"]
    D --> G["normalize_tool_names(empty) → empty\nenable_tools() → Some(empty)"]
    E --> H["normalize_tool_names(set) → trimmed set\nenable_tools() → Some(set)"]

    F --> I["tool_enabled(None, disable_tools, name)\n→ !disable_tools.contains(name)"]
    G --> J["tool_enabled(Some(empty), _, name)\n→ false (all disabled)"]
    H --> K["tool_enabled(Some(set), _, name)\n→ set.contains(name)\n(disable_tools ignored)"]
Loading

Reviews (3): Last reviewed commit: "fix(config): clarify enable_tools filter..." | Re-trigger Greptile

Comment thread src/registry.rs
@risu729 risu729 marked this pull request as ready for review April 18, 2026 07:52
@jdx jdx merged commit cc374ed into jdx:main Apr 18, 2026
35 checks passed
@risu729 risu729 deleted the copilot/enable-tools-empty branch April 18, 2026 16:19
jdx pushed a commit that referenced this pull request Apr 18, 2026
### 🐛 Bug Fixes

- **(backend)** respect install_before in latest lookup by @risu729 in
[#9193](#9193)
- **(backend)** route explicit latest through stable lookup by @risu729
in [#9228](#9228)
- **(backends)** deprecate b shorthand by @risu729 in
[#9234](#9234)
- **(config)** warn for deprecated env keys by @risu729 in
[#9205](#9205)
- **(config)** treat enable_tools empty as disable-all by @risu729 in
[#9108](#9108)
- **(github)** avoid auth on release asset downloads by @risu729 in
[#9060](#9060)
- **(gitlab)** warn when glab OAuth2 token is expired by @stanhu in
[#9195](#9195)
- **(npm)** honor install_before without day drift by @risu729 in
[#9157](#9157)
- **(npm)** warn on old bun and pnpm for install_before by @risu729 in
[#9232](#9232)
- **(pipx)** honor install_before for uv and pipx installs by @risu729
in [#9190](#9190)
- **(registry)** allow shfmt on Windows by @zeitlinger in
[#9191](#9191)

### 🚜 Refactor

- **(backend)** remove unused rolling release helper by @risu729 in
[#9175](#9175)
- **(backend)** use file util for removals by @risu729 in
[#9206](#9206)

### 📚 Documentation

- **(config)** clarify always_keep_download behavior by @risu729 in
[#9235](#9235)
- **(configuration)** add rust to idiomatic version files by @jjt in
[#9233](#9233)
- **(contributing)** expand contribution guide introduction by
@marianwolf in [#9208](#9208)
- **(github)** document multiple release assets workaround by @risu729
in [#9236](#9236)

### 📦️ Dependency Updates

- update actions/setup-node action to v6 by @renovate[bot] in
[#9183](#9183)
- update dependency @types/node to v25 by @renovate[bot] in
[#9187](#9187)
- update crazy-max/ghaction-import-gpg action to v7 by @renovate[bot] in
[#9186](#9186)
- update actions/cache action to v5 by @renovate[bot] in
[#9181](#9181)
- update amannn/action-semantic-pull-request action to v6 by
@renovate[bot] in [#9184](#9184)
- update apple-actions/import-codesign-certs action to v6 by
@renovate[bot] in [#9185](#9185)
- update dependency eslint to v10 by @renovate[bot] in
[#9200](#9200)
- update dependency toml to v4 by @renovate[bot] in
[#9201](#9201)
- update rust crate reqwest to 0.13 by @renovate[bot] in
[#9171](#9171)
- update ghcr.io/jdx/mise:deb docker digest to 523d826 by @renovate[bot]
in [#9198](#9198)
- update ghcr.io/jdx/mise:alpine docker digest to 05617e0 by
@renovate[bot] in [#9196](#9196)
- update ghcr.io/jdx/mise:rpm docker digest to c1992f9 by @renovate[bot]
in [#9199](#9199)
- update ghcr.io/jdx/mise:copr docker digest to 90db6cd by
@renovate[bot] in [#9197](#9197)
- update taiki-e/install-action digest to 58e8625 by @renovate[bot] in
[#9209](#9209)
- update fedora docker tag to v45 by @renovate[bot] in
[#9213](#9213)
- update docker/setup-buildx-action action to v4 by @renovate[bot] in
[#9212](#9212)
- update docker/metadata-action action to v6 by @renovate[bot] in
[#9211](#9211)
- update docker/login-action action to v4 by @renovate[bot] in
[#9210](#9210)
- update dependency typescript to v6 by @renovate[bot] in
[#9202](#9202)
- update docker/build-push-action action to v7 by @renovate[bot] in
[#9203](#9203)
- update github artifact actions (major) by @renovate[bot] in
[#9215](#9215)
- update rust crate duct to v1 by @renovate[bot] in
[#9220](#9220)
- update rust crate demand to v2 by @renovate[bot] in
[#9219](#9219)
- update rust crate clx to v2 by @renovate[bot] in
[#9218](#9218)
- update nick-fields/retry action to v4 by @renovate[bot] in
[#9217](#9217)
- update jdx/mise-action action to v4 by @renovate[bot] in
[#9216](#9216)
- update rust crate self_update to 0.44 by @renovate[bot] in
[#9174](#9174)
- migrate eslint config to flat format for v10 compat by @jdx in
[#9222](#9222)
- update actions/checkout action to v6 by @renovate[bot] in
[#9182](#9182)
- update rust crate toml to v1 by @renovate[bot] in
[#9225](#9225)
- update rust crate versions to v7 by @renovate[bot] in
[#9226](#9226)
- update rust crate which to v8 by @renovate[bot] in
[#9227](#9227)
- update rust crate rmcp to v1 by @renovate[bot] in
[#9221](#9221)

### 📦 Registry

- add sheldon by @3w36zj6 in
[#9104](#9104)
- add pocketbase by @ranfdev in
[#9123](#9123)
- add worktrunk ([aqua:max-sixty/worktrunk,
cargo:worktrunk](https://github.com/max-sixty/worktrunk,
cargo:worktrunk))#1 by @edouardr in
[#8796](#8796)
- add dependency-check
([aqua:dependency-check/DependencyCheck](https://github.com/dependency-check/DependencyCheck))
by @kapitoshka438 in [#9204](#9204)
- add janet by @ranfdev in
[#9241](#9241)

### New Contributors

- @ranfdev made their first contribution in
[#9241](#9241)
- @jjt made their first contribution in
[#9233](#9233)
- @marianwolf made their first contribution in
[#9208](#9208)
- @edouardr made their first contribution in
[#8796](#8796)

## 📦 Aqua Registry Updates

#### New Packages (3)

- [`LargeModGames/spotatui`](https://github.com/LargeModGames/spotatui)
-
[`android-sms-gateway/cli`](https://github.com/android-sms-gateway/cli)
- [`velero-io/velero`](https://github.com/velero-io/velero)

#### Updated Packages (1)

- [`skim-rs/skim`](https://github.com/skim-rs/skim)
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