feat: add MCP collections service, publishing, and catalog UI#2199
feat: add MCP collections service, publishing, and catalog UI#2199
Conversation
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: a40e3a5 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
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 |
🚀 Preview Environment (PR #2199)Preview URL: https://pr-2199.dev.getgram.ai
Gram Preview Bot |
| func pgText(s *string) pgtype.Text { | ||
| if s == nil || *s == "" { | ||
| return pgtype.Text{String: "", Valid: false} | ||
| } | ||
| return pgtype.Text{String: *s, Valid: true} | ||
| } |
There was a problem hiding this comment.
🟡 pgText treats empty string as NULL, preventing users from clearing collection description
The pgText helper at server/internal/collections/impl.go:358-363 returns {Valid: false} (SQL NULL) for both nil and empty string "". In the Update method (line 162), this means COALESCE(NULL, description) preserves the old value. A user who wants to clear a collection's description by sending description: "" will have their change silently ignored — the old description persists.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if registry.Namespace != "" { | ||
| specifier = fmt.Sprintf("%s/%s", registry.Namespace, t.McpSlug.String) | ||
| } | ||
| collectionRegistryIDStr := registry.ID.String() |
There was a problem hiding this comment.
🚩 ListServers uses zero-value registry struct when registry row not found
At server/internal/collections/impl.go:372-375, if GetOrganizationMcpCollectionRegistryByID returns pgx.ErrNoRows, the error is silently ignored and registry remains a zero-value OrganizationMcpCollectionRegistry struct. This means registry.Namespace will be "" (line 393 check passes) and registry.ID will be uuid.Nil, which gets serialized as "00000000-0000-0000-0000-000000000000" in the OrganizationMcpCollectionRegistryID field of the response (line 396). If a collection somehow doesn't have a registry row (which shouldn't happen in normal flow but could after a data inconsistency), clients would receive a nil-UUID registry ID rather than null, which could cause confusion.
Was this helpful? React with 👍 or 👎 to provide feedback.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Ensure each organization gets a default Registry collection and namespace when a user logs in or switches scopes, with auth tests covering the bootstrap path. Made-with: Cursor
…ns-v3 Made-with: Cursor # Conflicts: # .speakeasy/workflow.lock # client/sdk/.speakeasy/gen.lock # client/sdk/.speakeasy/gen.yaml # client/sdk/jsr.json # client/sdk/package.json # client/sdk/src/lib/config.ts # client/sdk/src/models/operations/index.ts # server/gen/http/openapi3.json
| if registry.Namespace != "" { | ||
| specifier = fmt.Sprintf("%s/%s", registry.Namespace, t.McpSlug.String) | ||
| } | ||
| collectionRegistryIDStr := registry.ID.String() |
There was a problem hiding this comment.
🔴 ListServers produces bogus nil-UUID collection registry ID when registry row is missing
In ListServers, when GetOrganizationMcpCollectionRegistryByID returns pgx.ErrNoRows, the error is silently swallowed (line 392) and the code continues with a zero-value registry struct. At line 415, registry.ID.String() produces "00000000-0000-0000-0000-000000000000", which is then set as OrganizationMcpCollectionRegistryID on every returned server. When the frontend subsequently installs these servers via a deployment, it sends this nil-UUID as the organization_mcp_collection_registry_id. The backend XOR validation passes (one ID is set), but during async deployment processing at server/internal/externalmcp/process.go:102, the code would try to use this invalid reference, and the FK constraint to organization_mcp_collection_registries would reject it. By contrast, the sibling List method at server/internal/collections/impl.go:174 treats a missing registry as a hard error, making this silent fallthrough inconsistent.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
|
||
| var AddExternalMCPForm = Type("AddExternalMCPForm", func() { | ||
| Required("registry_id", "name", "slug", "registry_server_specifier") | ||
| Required("name", "slug", "registry_server_specifier") |
There was a problem hiding this comment.
🚩 registryId made optional is a breaking change for existing API consumers
The AddExternalMCPForm type's registryId field changed from required (string) to optional (*string) in the Go types at server/gen/deployments/service.go:68-69 and correspondingly in the OpenAPI spec. The Required array in the design (server/design/deployments/design.go:352) no longer includes registry_id. Existing API consumers that always provide registry_id won't break, but any consumers relying on the response always having registry_id populated (e.g., the DeploymentExternalMCP type) will see a breaking change. The client SDK was regenerated to handle this, but external SDK users or direct API consumers may need updating.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Not relevant. We don't have any public consumers.
| const attachedToolsetIds = useMemo(() => { | ||
| const ids = new Set<string>(); | ||
| for (const toolset of allToolsets) { | ||
| if (attachedMcpSlugs.has(toolset.mcpSlug)) { | ||
| ids.add(toolset.id); | ||
| } | ||
| } | ||
| return ids; | ||
| }, [allToolsets, attachedMcpSlugs]); |
There was a problem hiding this comment.
🚩 Collection attachedToolsetIds matching uses mcpSlug which may not be unique across projects
In CollectionDetail.tsx:146-154, attachedToolsetIds is built by matching toolsets' mcpSlug against the attachedMcpSlugs set derived from server registry specifiers. If two toolsets in different projects happen to share the same mcpSlug, both will appear as 'attached' even if only one is actually in the collection. This could cause incorrect checkbox states in the edit form and incorrect diff calculations when saving (potentially detaching the wrong toolset). The mcpSlug is extracted from the last segment of registrySpecifier at line 99-100, which further increases collision risk.
Was this helpful? React with 👍 or 👎 to provide feedback.
Initialize pgtype.Text explicitly so the default Registry bootstrap passes the stricter server lint rules after the merge from main. Made-with: Cursor
Restore the MCP Details publishing section so toolsets can be attached to or detached from collections directly from the settings page. Made-with: Cursor
Reject publishing for toolsets that are not enabled MCP servers and hide those toolsets from the collections UI so published servers persist after refresh. Made-with: Cursor
| Result(MCPCollection) | ||
|
|
||
| HTTP(func() { | ||
| PATCH("/rpc/collections.update") |
There was a problem hiding this comment.
We're not patching in this operation and we generally don't have any APIs that take in patches on resources.
| PATCH("/rpc/collections.update") | |
| POST("/rpc/collections.update") |
| SelectedRemotes: mcp.SelectedRemotes, | ||
| AttachmentID: mcp.ID, | ||
| RegistryID: mcp.RegistryID, | ||
| OrganizationMcpCollectionRegistryID: mcp.OrganizationMcpCollectionRegistryID, |
There was a problem hiding this comment.
Is there a shorter name for this field? Also a little confusing to have it alongside RegistryID. Should they be named InternalRegistryID and ExternalRegistryID or something like that?
Also are they mutually exclusive? If so, should there be some logic in deployments service to ensure that only one is provided?
There was a problem hiding this comment.
Is there a shorter name for this field? Also a little confusing to have it alongside RegistryID. Should they be named InternalRegistryID and ExternalRegistryID or something like that?
I think the current naming is clear enough in context. If you don't have any strong objections, I'm happy to keep it as is for now. WDYT?
| -- name: GetOrganizationMcpCollectionByID :one | ||
| SELECT id, organization_id, name, description, slug, visibility, created_at, updated_at | ||
| FROM organization_mcp_collections | ||
| WHERE id = @id AND deleted IS FALSE; |
There was a problem hiding this comment.
All queries must be qualified with either project id or organization id. In this case case:
| WHERE id = @id AND deleted IS FALSE; | |
| WHERE | |
| id = @id | |
| AND organization_id = @organization_id | |
| AND deleted IS FALSE; |
In the application layer, you pass ActiveOrganizationID and then you can check for pgx.ErrNoRows to assert that a collection exists or not.
| description = COALESCE(sqlc.narg('description'), description), | ||
| visibility = COALESCE(sqlc.narg('visibility'), visibility), | ||
| updated_at = clock_timestamp() | ||
| WHERE id = @id AND deleted IS FALSE |
There was a problem hiding this comment.
Needs to be qualified with org id here as well (authCtx.ActiveOrganizationID in code)
| return &gen.ListServersResult{Servers: servers}, nil | ||
| } | ||
|
|
||
| func pgText(s *string) pgtype.Text { |
There was a problem hiding this comment.
Use the conv.PtrToPGTextEmpty
| } | ||
| return oops.E(oops.CodeUnexpected, err, "get collection").Log(ctx, s.logger) | ||
| } | ||
| if existing.OrganizationID != authCtx.ActiveOrganizationID { |
| if !t.McpSlug.Valid { | ||
| continue | ||
| } | ||
| remoteURL := fmt.Sprintf("%s/mcp/%s", s.serverURL.String(), t.McpSlug.String) |
There was a problem hiding this comment.
use s.serverURL.JoinPath instead of fmt.Sprintf
| } | ||
| specifier := t.McpSlug.String | ||
| if registry.Namespace != "" { | ||
| specifier = fmt.Sprintf("%s/%s", registry.Namespace, t.McpSlug.String) |
There was a problem hiding this comment.
use s.serverURL.JoinPath
| return pgtype.Text{String: *s, Valid: true} | ||
| } | ||
|
|
||
| func toMCPCollection(c repo.CreateOrganizationMcpCollectionRow, namespace string) *types.MCPCollection { |
There was a problem hiding this comment.
Consider conv.FromPGText, conv.PtrEmpty
|
|
||
| externalMCPID := r.ExternalMcpID.UUID | ||
| if externalMCPID != uuid.Nil && !seenExternalMCPs[externalMCPID] { | ||
| hasRegistry := r.ExternalMcpRegistryID.Valid && r.ExternalMcpRegistryID.UUID != uuid.Nil |
There was a problem hiding this comment.
use conv.FromNullableUUID and it should help clean up the pointer assignments below
| setPhase("configure"); | ||
| } | ||
| }, [servers]); | ||
| }, [servers, existingSpecifiers]); |
There was a problem hiding this comment.
🚩 useEffect dependency on existingSpecifiers may cause infinite re-partitioning loop
At useExternalMcpReleaseWorkflow.ts:197, the useEffect that initializes server configs now depends on [servers, existingSpecifiers]. Since existingSpecifiers is a useMemo that creates a new Set on every recomputation (line 130-138), it will have a new reference whenever latestDeployment?.externalMcps changes. After a deployment completes and useLatestDeployment refetches, this could trigger the useEffect to re-run and re-partition servers, potentially resetting the workflow state mid-flow. In practice, the deployment flow has already moved past the configure phase by then, but the re-partitioning could reset multiRemoteConfigs and serverConfigs unexpectedly. The Set reference stability depends on whether useMemo's [latestDeployment?.externalMcps] dependency changes reference between refetches.
Was this helpful? React with 👍 or 👎 to provide feedback.
This comment has been minimized.
This comment has been minimized.
| // Clean up Radix body scroll-lock on unmount (e.g. when navigating away mid-dialog) | ||
| useEffect(() => { | ||
| return () => { | ||
| document.body.style.removeProperty("pointer-events"); | ||
| }; | ||
| }, []); | ||
|
|
adaam2
left a comment
There was a problem hiding this comment.
I had a play with the preview app - everything seemed to work and its much improved. Had a quick breeze over the frontend code, and it looked 👍🏻
Approving for frontend. Waiting on Georges feedback on backend code.
| var empty projectsRepo.Project | ||
| if err != nil { | ||
| if errors.As(err, &pgErr) && pgErr.Code == pgerrcode.UniqueViolation { | ||
| if isUniqueViolation(err) { |
There was a problem hiding this comment.
This change isn't relevant to the PR right? Can we revert it along with the helper function introduced below.
| orgSlug = orgMeta.Slug | ||
| } | ||
|
|
||
| if err := s.ensureDefaultRegistryCollection(ctx, authCtx.ActiveOrganizationID, orgSlug); err != nil { |
There was a problem hiding this comment.
good idea to move here versus auth service 👍
| setPhase("configure"); | ||
| } | ||
| }, [servers]); | ||
| }, [servers, existingSpecifiers]); |
There was a problem hiding this comment.
🔴 Adding existingSpecifiers to useEffect deps causes phase reset mid-deployment
The useEffect at line 161 that partitions servers into multi/single remote now includes existingSpecifiers in its dependency array (line 197). When a deployment completes, latestDeployment is refetched, which causes existingSpecifiers to update (the newly installed servers are now in the set). This triggers the useEffect to re-run, which re-partitions servers (filtering out the just-installed ones), resets serverConfigs to empty, and unconditionally calls setPhase("configure") (line 195) — overwriting the current "complete" or "deploying" phase. This disrupts the completion UI (which shows toolset creation progress) by abruptly resetting to an empty configure phase.
Trigger scenario
- User installs servers from a collection
- Deployment starts → phase is
"deploying" - Deployment completes →
latestDeploymentrefetches →existingSpecifiersupdates - useEffect re-runs, filters out all installed servers, calls
setPhase("configure") - UI jumps from completion/deploying status to an empty configure screen
Prompt for agents
In useExternalMcpReleaseWorkflow.ts, the useEffect that partitions servers (starting around line 161) now has existingSpecifiers in its dependency array at line 197. This causes the effect to re-run when latestDeployment refetches after a successful deployment, which resets the phase and serverConfigs in the middle of active deployment or completion.
The fix should add a guard at the top of the useEffect to bail out if the workflow is in an active phase (deploying, complete, or error). For example:
if (phase === 'deploying' || phase === 'complete' || phase === 'error') return;
This ensures the effect only partitions servers during the initial setup phases (configure/selectRemotes) when existingSpecifiers changes, not during active deployment.
Was this helpful? React with 👍 or 👎 to provide feedback.
| for _, idStr := range payload.ToolsetIds { | ||
| toolsetID, parseErr := uuid.Parse(idStr) | ||
| if parseErr != nil { | ||
| continue | ||
| } | ||
| if err := s.attachServerToCollection(ctx, cr, collection.ID, toolsetID, authCtx.ActiveOrganizationID, authCtx.UserID); err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
🚩 CreateCollection does not filter non-MCP-enabled toolsets during creation
In CreateCollection, the toolsetIds array is iterated and each ID is passed to attachServerToCollection. Inside that function (impl.go:440), there's a check if !toolset.McpEnabled || !toolset.McpSlug.Valid that correctly rejects non-MCP-enabled toolsets. So invalid toolset IDs will cause the entire Create to fail with an error (line 131-133 returns the error). This means if a user provides even one non-MCP toolset ID, the whole collection creation fails. This might be intentional strictness, but could also be surprising since invalid UUIDs at line 128-129 are silently skipped while valid-but-ineligible toolsets cause a hard failure.
Was this helpful? React with 👍 or 👎 to provide feedback.
Summary
registry_idoptional across deployments/external MCP types for collection-backed serversRegistrycollection and namespace on login and org scope switchRebased onto main now that #2198 is merged.
Test plan
go test github.com/speakeasy-api/gram/server/internal/auth🤖 Generated with Claude Code