Skip to content

fix(web): return effective enabled state from toolset toggle endpoint - #37649

Closed
AhmetArif0 wants to merge 1 commit into
NousResearch:mainfrom
AhmetArif0:fix/toolset-toggle-false-success
Closed

fix(web): return effective enabled state from toolset toggle endpoint#37649
AhmetArif0 wants to merge 1 commit into
NousResearch:mainfrom
AhmetArif0:fix/toolset-toggle-false-success

Conversation

@AhmetArif0

Copy link
Copy Markdown
Contributor

Problem

PUT /api/tools/toolsets/{name} always responds with enabled: body.enabled
regardless of whether the write was actually applied. _save_platform_tools()
silently drops toolsets restricted to other platforms (e.g. discord_admin is
restricted to platform='discord'; the endpoint hardcodes platform='cli').
Result: the desktop Skills & Tools toggle flashes on and then reverts to off
on the next navigation — with no error surfaced to the user.

Reported in #37609.

Root cause trace

PUT /api/tools/toolsets/discord_admin  {"enabled": true}

_save_platform_tools(config, "cli", {…, "discord_admin"})
  └─ _toolset_allowed_for_platform("discord_admin", "cli")
       └─ _TOOLSET_PLATFORM_RESTRICTIONS["discord_admin"] = {"discord"}
       └─ "cli" ∉ {"discord"}  →  write silently dropped

return {"ok": True, "enabled": True}   ← false success

Fix

After saving, re-read the config and return whether the toolset is actually
present in the effective set — so the API response always reflects reality:

_save_platform_tools(config, "cli", enabled)
config_after = load_config()
effective = set(_get_platform_tools(config_after, "cli", include_default_mcp_servers=False))
return {"ok": True, "name": name, "enabled": name in effective}

_get_platform_tools is already imported in the same block.

Test plan

  • test_toggle_toolset_platform_restricted_returns_actual_state — toggling discord_admin on returns enabled: false; GET listing confirms it stayed off
  • test_toggle_toolset_enable_disable — normal (unrestricted) toggles still work and return the correct state
  • test_toggle_toolset_unknown_returns_400 — regression guard

PUT /api/tools/toolsets/{name} always responded with enabled=body.enabled
even when _save_platform_tools() silently dropped the write because the
toolset is restricted to a different platform (e.g. discord_admin is
restricted to platform='discord', so cli writes are filtered out by
_toolset_allowed_for_platform). The optimistic response caused the desktop
Skills & Tools toggle to flash enabled briefly then revert on the next GET.

After saving, re-read the config and return name in effective so the
response always reflects what was actually persisted.

Fixes NousResearch#37609.
@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/cli CLI entry point, hermes_cli/, setup wizard comp/gateway Gateway runner, session dispatch, delivery labels Jun 2, 2026
@alt-glitch alt-glitch added comp/dashboard Web dashboard / control panel UI (dashboard/, landing) and removed comp/cli CLI entry point, hermes_cli/, setup wizard comp/gateway Gateway runner, session dispatch, delivery labels Jun 26, 2026

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

Thanks for isolating the false-success path; current main still returns body.enabled after _save_platform_tools() at hermes_cli/web_server.py:13446-13447, while that save helper filters platform-restricted toolsets at hermes_cli/tools_config.py:1941-1947.

Problems

  • The proposed re-read is outside with _profile_scope(body.profile or profile). _profile_scope resets the named-profile Hermes-home override when it exits (hermes_cli/web_server.py:13201-13209), so a request for a named profile can read and return the dashboard profile's state instead.

Suggested changes

  • Keep the effective-state load_config() / _get_platform_tools() read within the existing profile scope, and add a named-profile regression test alongside the restricted-toolset test.

This is an automated hermes-sweeper review.

Comment thread hermes_cli/web_server.py
enabled.discard(name)
_save_platform_tools(config, "cli", enabled)
return {"ok": True, "name": name, "enabled": body.enabled}
config_after = load_config()

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.

This read is after the with _profile_scope(body.profile or profile) block. For a named profile, _profile_scope has already restored the prior Hermes-home override, so this can report the dashboard profile’s effective state rather than the profile just updated. Please perform the read inside the existing scope and add a named-profile regression test.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 13, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for catching this one — you were the first to flag the false-success on platform-restricted toolset toggles (#37609). We've now merged PR #65361, which takes the complementary approach: instead of reporting the honest enabled=false, the dashboard now resolves the toolset's configuration platform and persists Discord-restricted toolsets to platform_toolsets.discord, so enabling them from the web UI actually works. The toggle response now also returns the resolved platform, and the drawer shows the real scope. Closing this as superseded — appreciate the report and the fix, credited here.

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

Labels

comp/dashboard Web dashboard / control panel UI (dashboard/, landing) P2 Medium — degraded but workaround exists sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants