Skip to content

feat: mirror hosted MCP configuration between toolsets and mcp_servers - #5989

Open
daviddanialy wants to merge 15 commits into
daviddanialy/mcp-servers-one-wrapper-per-toolsetfrom
daviddanialy/aim-19-mirror-hosted-mcp-config
Open

feat: mirror hosted MCP configuration between toolsets and mcp_servers#5989
daviddanialy wants to merge 15 commits into
daviddanialy/mcp-servers-one-wrapper-per-toolsetfrom
daviddanialy/aim-19-mirror-hosted-mcp-config

Conversation

@daviddanialy

@daviddanialy daviddanialy commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

AIM-19

Stacked on #5951 (the one-wrapper-per-toolset index); merge that first.

Summary

  • Every toolset write (create, update, delete, clone, issuer, variations group) projects onto the toolset's mcp_servers wrapper and primary mcp_endpoints row; every mcpServers / mcpEndpoints write on a toolset-backed server projects back onto the toolset's hosting columns. Both directions run in the caller's transaction with the toolset row locked first (hostedmcp.LockToolsets), order toolset → custom domain → server → slug advisory lock.
  • Visibility maps 4 toolset states → 3 (mcp_enabled=false is disabled; projecting back from disabled clears mcp_is_public). Endpoint slug is the toolset's mcp_slug verbatim in its scope; alias twins follow renames.
  • mcpServers.update rejects changing a hosted server's backing toolset or backend kind (CodeInvalid); mcpServers.delete tombstones endpoints then wrapper and clears the toolset's hosting columns without deleting the toolset. Second wrapper for a toolset → CodeConflict (index from mig: enforce one live mcp_servers wrapper per toolset (temporary, dropped by AIM-172) #5951).
  • Assistant attach enables MCP through the mirror (hostedmcp.EnableToolsetMCP) instead of a raw flag write. Backward visibility sync applies the OAuth-detach rule; wrapper renames are durable (name syncs only when the toolset name changed).
  • Demo seed provisions wrappers + endpoints for every hosted server, with a postflight invariant check.

Motivation

While toolsets and mcp_servers coexist, the two representations of a hosted server must stay identical or the wrapper-governed runtime (#5918) and the dashboard's toolset toggles disagree about what is serving. This is the dual-write half of the migration; the backfill that wraps existing servers follows and requires this to be deployed first.

Deviations from spec, deliberately: default-plugin attachment stays toolset-keyed until AIS-638 (the server-keyed plugin query lacks env-config support); a toolset bound to a soft-deleted custom domain projects no endpoint.

🤖 Generated with Claude Code

@daviddanialy daviddanialy added the enhancement New feature or request label Sep 2, 2026
@daviddanialy
daviddanialy requested a review from a team as a code owner September 2, 2026 19:34
@daviddanialy daviddanialy added go Pull requests that update go code enhancement New feature or request labels Sep 2, 2026
@linear-code

linear-code Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

AIM-19

@changeset-bot

changeset-bot Bot commented Sep 2, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 3052ac5

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
server Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@cubic-dev-ai

cubic-dev-ai Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Running ultrareview automatically — This cross-cutting change adds transactional dual-writes across toolsets, MCP servers, endpoints, visibility, OAuth, and deletion, with new locking and uniqueness invariants whose subtle failures could cause inconsistent serving, access-control exposure, or data loss.. I'll post findings when complete.

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

atlas migrate lint on server/migrations

Status Step Result
1 new migration file detected 20260902173039_mcp-servers-one-wrapper-per-toolset.sql
ERD and visual diff generated View Visualization
Analyze 20260902173039_mcp-servers-one-wrapper-per-toolset.sql
1 reports were found in analysis
Data dependent changes detected
Adding a unique index "mcp_servers_toolset_id_key" on table "mcp_servers" might fail in case column "toolset_id" contains duplicate entries (MF101)
Read the full linting report on Atlas Cloud

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

atlas migrate lint on server/clickhouse/migrations

Status Step Result
No migration files detected  
ERD and visual diff generated View Visualization
No issues found View Report
Read the full linting report on Atlas Cloud

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@daviddanialy
daviddanialy changed the base branch from main to daviddanialy/mcp-servers-one-wrapper-per-toolset September 2, 2026 19:38
@daviddanialy
daviddanialy requested a review from a team as a code owner September 2, 2026 19:38
daviddanialy and others added 2 commits September 2, 2026 12:41
…daviddanialy/aim-19-mirror-hosted-mcp-config
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

@cubic-dev-ai cubic-dev-ai 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.

Ultrareview completed in 16m 8s

Review completed against the latest diff

Heads up: you’re close to your included review allowance. Set a flex budget so reviews don’t pause.

Linked issue analysis

Linked issue: AIM-19: feat: mirror hosted MCP configuration between toolsets and mcp_servers

Status Acceptance criteria Notes
Toolset lifecycle and hosting-field writes project to the toolset's MCP wrapper and primary endpoint. Toolset create, update, delete, clone, issuer, and variations-group paths invoke mirror logic; the new toolset mirror handles wrapper and endpoint projection.
Wrapper and endpoint writes project visibility, authentication, variation-group, slug, and custom-domain state back onto the toolset. MCP server and endpoint services lock backing toolsets and invoke reverse projection for update, create, update, and delete operations.
Both projection directions run transactionally with the toolset locked before dependent resources. The PR adds explicit toolset locking and documents and implements the toolset → custom domain → server → slug lock order in both directions.
Visibility round-trips between the four toolset flag states and the three MCP server visibility states, clearing publicness when disabled. The shared hosted MCP package defines the visibility mapping, and mirror tests cover the round-trip behavior.
Deleting a toolset or its hosted wrapper tombstones endpoints child-first and keeps the toolset/wrapper semantics consistent. Toolset deletion and mcpServers.delete use dedicated deletion logic, emit endpoint deletion before server deletion, and clear hosting columns without deleting the toolset.
Each toolset has at most one live MCP server wrapper. The schema and migration add a partial unique index over non-deleted wrappers, with conflict handling and tests for duplicate wrappers.
Assistant attachment enables hosted MCP through the mirror rather than directly updating toolset flags. The raw EnableMCPForToolsets query was removed, attachment now uses hostedmcp.EnableToolsetMCP, and a test verifies disabled wrappers are lifted.
Demo seed data provisions matching wrappers and endpoints and verifies the hosting invariant. The demo seed was updated with hosted slugs, wrappers, endpoints, and a postflight invariant check.

Tip: instead of fixing issues one by one fix them all with cubic

Re-trigger cubic

Comment thread server/internal/assistants/service.go
Comment thread server/internal/background/custom_domain_registration.go
Comment thread server/internal/mcpservers/queries.sql
Comment thread server/internal/toolsets/set_tool_variations_group.go
Comment thread server/internal/toolsets/mirror.go
Comment thread server/internal/mcpservers/impl.go Outdated
Comment thread server/internal/demoseed/postgres.sql Outdated
Comment thread server/internal/demoseed/postgres.sql
Comment thread server/internal/assistants/impl_test.go
Comment thread server/internal/mcpservers/mirror_test.go
Every mirror address write takes the slug lock and unified availability
check; custom domains lock org-scoped; EnableToolsetMCP reconciles a stale
wrapper; setters re-check the authorized toolset id after locking and
reconcile cleared domains; issuer changes resync the derived remote
session issuer; delete tombstones children first and clears toolset
hosting before the issuer cascade.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

@cubic-dev-ai cubic-dev-ai 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.

All reported issues were addressed across 14 files (changes from recent commits).

Heads up: you’re close to your included review allowance. Set a flex budget so reviews don’t pause.

Tip: Review your code locally with the cubic CLI to iterate faster.

Fix all with cubic | Re-trigger cubic

Comment thread server/internal/toolsets/mirror.go
Comment thread server/internal/mcpendpoints/mirror.go
Comment thread server/internal/hostedmcp/hostedmcp.go
…p front

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

@cubic-dev-ai cubic-dev-ai 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.

All reported issues were addressed across 7 files (changes from recent commits).

Heads up: you’re close to your included review allowance. Set a flex budget so reviews don’t pause.

Tip: Review your code locally with the cubic CLI to iterate faster.

Fix all with cubic | Re-trigger cubic

Comment thread server/internal/mcpservers/impl.go
Comment thread server/internal/mcpendpoints/mirror_test.go
daviddanialy and others added 2 commits September 2, 2026 13:43
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…suer

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@daviddanialy

Copy link
Copy Markdown
Contributor Author

End-to-end verification

Live stack built from this PR's head (3052ac5, includes the #5951 index), all writes over HTTP, SQL only to inspect state. 124 checks PASS / 1 FAIL (pre-existing) / 4 NOT RUN (stated).

Forward (toolset writes → wrapper/endpoint): create, every update field in isolation (enable, public, private, slug rename re-keys the same endpoint id, disable), description-only and same-values updates are true no-ops (updated_at untouched, no audit rows), all four visibility states round-trip including the prod disabled+public shape, clone, issuer set and clear (derived remote_session_issuer_id cleared with a bound remote-session client), variations group, delete (endpoints tombstoned before wrapper, audit ordered), rename in both directions with a wrapper rename surviving later toolset writes, name validation.

Backward (server/endpoint writes → toolset): visibility sync incl. disabled clearing mcp_is_public; the OAuth-detach rule (disable keeps external_oauth_server_id, enabled public→private detaches); rewire and second-wrapper rejected (422/409) with a remote server as control for unchanged non-hosted behavior; mcpServers.delete clears hosting columns and keeps external_oauth_server_id, default_environment_slug, the issuer row and its sessions; full endpoint lifecycle (second endpoint, primary re-key, move between hosted servers without a 500, delete primary/last); oversized slug → 422; wrapper enable adds no second Default-plugin row.

Assistant: attaching a disabled toolset lifts the wrapper to private and the endpoint serves.

Dashboard (Playwright, real dialogs): Disable/Enable/Public/Private on the legacy toolset page and Disable/Private on the unified server page each produced paired mcp-server:update + toolset:update audit rows and the expected serving behavior; the legacy page reflects unified-page changes after reload.

Concurrency (real): 20 parallel slug renames vs 20 parallel endpoint re-keys on one server → 40/40 200, zero 5xx, zero deadlocks, final mcp_slug == primary endpoint slug; 20 parallel mcpServers.create for one toolset → exactly one 200, nineteen 409.

Audit: forward writes emit mcp-server:*/mcp-endpoint:*, backward writes emit a toolset:update twin, deletes ordered endpoint→server both ways. Log sweep: no panics, no 40P01.

FAIL (pre-existing, not a regression): toolset rename does not update plugin_servers.display_name for hosted servers — the sync keys on mcp_server_id and hosted rows stay toolset-keyed until AIS-638; main never synced them either. Tracked on AIS-638.

NOT RUN: first-toolset auto-enable (needs a fresh org), custom-domain bind (activation is reconciler-only), unified page → Public (offered only for tunneled sources by design).

Two unrelated API gaps noticed and ticketed at low priority: remoteSessionIssuers.create 500s without the *_supported arrays; a 61-char toolset name surfaces the DB CHECK as a 500.

🤖 Generated with Claude Code

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

Labels

enhancement New feature or request go Pull requests that update go code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant