Skip to content

fix(mcp): make workspace config the canonical store for the disable toggle#114

Merged
juacker merged 6 commits into
mainfrom
fix/persist-mcp-disabled-state
Jul 22, 2026
Merged

fix(mcp): make workspace config the canonical store for the disable toggle#114
juacker merged 6 commits into
mainfrom
fix/persist-mcp-disabled-state

Conversation

@juacker

@juacker juacker commented Jul 22, 2026

Copy link
Copy Markdown
Collaborator

Problem

The MCP disable toggle in the workspace context bar lived only in React state: it was never sent to the backend, so runs still connected 'disabled' servers, and the toggle silently reset on app restart.

Design (reworked after review discussion)

The per-workspace .clai/config.json is the canonical store for a general workspace's MCP selection — SessionContext is not extended and only ever receives the derived enabled list.

  • WorkspaceConfig gains workspace-level disabledMcpServers (by-name McpRef, serde default → old configs parse unchanged). The manager row's selectedMcpServers keeps meaning the effective enabled set, so scheduled runs and Workspace Settings are untouched.
  • workspace_update_session_mcp sanitizes the request (enabled/disabled kept disjoint), writes the enabled list to the session and both lists to the config; the disabled list persists even with no manager row.
  • desired_workspace_context derives the enabled set from config for general workspaces; manager-less workspaces keep the session's list minus config-disabled ids; the legacy session fallback applies only when a manager exists and the config records nothing (pre-2026-05-19 sessions predate the config mirror).
  • WorkspaceSnapshot exposes disabledMcpServerIds with enabled-wins disjointness enforced at the read boundary; the bar rebuilds attached = enabled ∪ disabled, prefers the config-derived list over a stale session list, and rolls back the optimistic update on backend rejection.
  • MCP rename/delete sweeps refresh the workspace-level disabled refs. refresh_mcp_refs_after_rename also fixes a pre-existing bug where renaming a server dropped agent selections (old-name refs resolved against the post-rename config and discarded).
  • workspace_update_agent re-enables servers re-selected for the manager by dropping them from the disabled list.

Testing

  • 798 Rust tests pass (8 new: context precedence ×4, sanitizer ×2, rename-refresh ×2, config back-compat)
  • 169 vitest tests pass (7 new WorkspaceContextBar tests incl. optimistic rollback + config-vs-session precedence)
  • fmt / clippy / tsc / eslint clean
  • 3 independent review rounds (2 reviewers); all 7 validated findings fixed with regression tests; final verdicts production_quality (Minimax) after fixes

…oggle

The context-bar MCP disable toggle lived only in React state: it was
never sent to the backend, so runs still connected 'disabled' servers
and the toggle silently reset on app restart.

- WorkspaceConfig gains workspace-level disabled_mcp_servers (McpRef,
  serde default so old configs parse unchanged). The manager row's
  selected_mcp_servers keeps meaning the effective enabled set, so
  scheduled runs and Workspace Settings are untouched.
- SessionContext is NOT extended: sessions only ever receive the
  derived enabled list.
- workspace_update_session_mcp sanitizes the request (enabled and
  disabled kept disjoint), writes the enabled list to the session and
  both lists to the workspace config; the disabled list persists even
  when no manager row exists.
- desired_workspace_context derives the enabled set from the config
  for general workspaces: manager-less workspaces keep the session's
  list minus config-disabled ids; the legacy session fallback applies
  only when a manager exists and the config records nothing.
- WorkspaceSnapshot exposes disabled_mcp_server_ids with enabled-wins
  disjointness enforced at the read boundary; the bar rebuilds
  attached = enabled + disabled from the snapshot, prefers the
  config-derived list over a stale session list, and rolls back the
  optimistic update if the backend rejects it.
- MCP rename/delete sweeps now refresh the workspace-level disabled
  refs; refresh_mcp_refs_after_rename also fixes a pre-existing bug
  where renaming a server dropped agent selections (old-name refs
  were resolved against the post-rename config and discarded).
- workspace_update_agent re-enables servers re-selected for the
  manager by dropping them from the disabled list.
@juacker
juacker force-pushed the fix/persist-mcp-disabled-state branch from 1a8dae5 to 4be48eb Compare July 22, 2026 18:30
@juacker juacker changed the title fix(mcp): persist the context-bar MCP disable toggle fix(mcp): make workspace config the canonical store for the disable toggle Jul 22, 2026
juacker added 2 commits July 22, 2026 20:44
Renames no longer require sweeping every workspace config; display names resolve from AppConfig at read time. Legacy name-only refs are dropped on load (users re-attach the server).
@juacker

juacker commented Jul 22, 2026

Copy link
Copy Markdown
Collaborator Author

Follow-up commits per review discussion:

c321a0d — refactor(mcp): store workspace MCP refs by server id

  • workspace_config::McpRef now stores the AppConfig server id instead of the name; display names resolve from AppConfig at read time.
  • Renames no longer sweep every workspace config: refresh_mcp_refs_after_rename + sweep_workspace_agent_mcp_renames deleted (both call sites: update_mcp_server, finish_mcp_oauth_login). Net −165 lines.
  • Backwards compat (intentional policy): legacy { "name": ... } refs are dropped on workspace_config::load() — users re-attach those servers. New test load_drops_legacy_name_only_mcp_refs covers this.
  • Delete sweep simplified to retain() on ref ids.

Scope note (verified): the context-bar disable toggle only affects the manager/session — workspace_update_session_mcp rewrites only the manager row; member agents' selected_mcp_servers (runner.rs:99, workspace_tasks.rs:617) are untouched and never filtered by disabled_mcp_servers.

Verification: cargo test 797/797, fmt clean, tsc clean, WorkspaceContextBar vitest 7/7. Two independent reviews: production_quality (minor doc-comment finding fixed in d2a62de).

Replaces the workspace-level disabled_mcp_servers list (introduced earlier
on this branch, never shipped) with a disabled flag on McpRef: the manager
agent's selectedMcpServers now records every attached server, and
{"id": ..., "disabled": true} marks a context-bar toggle-off. Absent key
means enabled, so member agents and existing configs are untouched.

- descriptor/sessions/scheduled runs consume enabled refs only (new
  enabled_mcp_ids helper); Settings surfaces keep showing all attached refs
- Settings saves preserve the toggle via merge_mcp_selection (attachment
  and enablement are orthogonal)
- drops the disjointness invariant maintained in three places
- drops WorkspaceSnapshot.selectedMcpServerNames: the FE never read it and
  already resolves names from ids
@juacker

juacker commented Jul 22, 2026

Copy link
Copy Markdown
Collaborator Author

New commit 78117ef — refactor(mcp): record the disable toggle per-ref on the manager row (user-directed design):

  • WorkspaceConfig.disabled_mcp_servers (workspace-level list, never shipped) is gone. McpRef gains "disabled": true — absent/false = enabled — recorded only on the manager row's selectedMcpServers, exactly the JSON shape proposed in review.
  • Sessions + scheduled runs consume enabled refs only (enabled_mcp_ids); Settings surfaces keep all attached refs so the edit form round-trips toggled-off servers; merge_mcp_selection preserves the toggle across Settings saves.
  • Dropped dead WorkspaceSnapshot.selectedMcpServerNames (FE never read it; names resolve from ids client-side). Bindings regenerated.
  • Verification: cargo 800/800, fmt clean, tsc clean, context-bar vitest 7/7. Dual independent review: one production_quality, one flagged a pre-existing edge (below).

Open design question (pre-dates this commit, present since 4be48eb): the legacy fallback in desired_workspace_context keeps an existing session's MCP list when the manager row records zero refs, so a Settings remove-all doesn't clear a live session's MCP until the context bar is touched. Options: (a) delete the fallback — config strictly canonical, old session-only selections lost once on upgrade; (b) keep + document; (c) keep, but have workspace_update_agent also rewrite the session's MCP ids. Awaiting maintainer decision before a follow-up commit.

juacker added 2 commits July 22, 2026 22:43
With the workspace config as the canonical store, a manager row with zero
MCP refs now means zero MCP servers. The legacy fallback that kept a
session's pre-mirror list alive is removed: a Settings remove-all clears
live sessions immediately, and session-only selections from before the
config mirror are dropped once (re-attach to migrate), matching the
migration policy used when name-based refs were dropped.
The context bar duplicated the removed backend fallback: with both config
partitions empty it displayed the session's MCP list and would re-persist
it on the next edit, resurrecting servers a Settings remove-all had just
cleared. It now falls back to the session list only for manager-less
workspaces (defaultWorkspaceAgentId null), matching the backend rule.

WorkspaceSettingsModal also dispatches workspace-settings-changed after a
successful save (and agent delete) so the self-loading bar refetches its
snapshot instead of holding stale local state.
@juacker

juacker commented Jul 22, 2026

Copy link
Copy Markdown
Collaborator Author

Two new commits per the option-(a) decision (config strictly canonical):

58976fe — refactor(mcp): make an empty manager MCP selection authoritative
Deletes the backend legacy fallback in desired_workspace_context: a manager row with zero MCP refs now means zero MCP servers. Settings remove-all clears live sessions immediately; session-only selections predating the config mirror are dropped once (re-attach to migrate). The no-manager-row branch is kept — there the config structurally can't record a selection.

530400e — fix(mcp): mirror the authoritative empty selection in the context bar
Round-3 review caught the same fallback duplicated client-side: the context bar showed the session's stale list when both config partitions were empty and would re-persist it on the next edit. It now falls back only when defaultWorkspaceAgentId is null (manager-less). WorkspaceSettingsModal additionally dispatches workspace-settings-changed after save/agent-delete so the self-loading bar refetches instead of holding stale local state.

Verification: cargo 800/800, vitest 9/9 (3 new/updated context-bar tests), tsc + eslint clean. Independent review round 4: production_quality (round 3 needs_work findings both fixed and re-verified).

@juacker
juacker marked this pull request as ready for review July 22, 2026 21:15
@juacker
juacker merged commit 5f1d2a6 into main Jul 22, 2026
2 checks passed
@juacker
juacker deleted the fix/persist-mcp-disabled-state branch July 22, 2026 21:17
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