Conversation
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| serverDetails, err := te.registryClient.GetServerDetails(ctx, Registry{ | ||
| ID: registry.ID, | ||
| URL: registry.Url, | ||
| URL: registry.Url.String, |
There was a problem hiding this comment.
🔴 process.go accesses nullable registry.Url.String without validity check after schema migration
The migration 20260313205018_allow-internal-registries.sql changed the mcp_registries.url column from NOT NULL to nullable. The generated Go model correspondingly changed the Url field from string to pgtype.Text. However, process.go:101 accesses registry.Url.String without first checking registry.Url.Valid. For an internal registry where url is NULL, registry.Url.Valid is false and registry.Url.String is "" (empty string). This passes an empty URL to RegistryClient.GetServerDetails() which would construct a malformed HTTP request URL like /v0.1/servers/..., causing a runtime HTTP error during deployment processing.
Where the field is accessed unsafely
At server/internal/externalmcp/process.go:99-102:
serverDetails, err := te.registryClient.GetServerDetails(ctx, Registry{
ID: registry.ID,
URL: registry.Url.String, // no .Valid check
}, ...)Contrast with impl.go:162-164 and impl.go:226 which correctly check registry.Url.Valid before accessing .String.
Prompt for agents
In server/internal/externalmcp/process.go, after line 97 (the error check for GetMCPRegistryByID), add a check for registry.Url.Valid before accessing registry.Url.String at line 101. If the URL is not valid (NULL), return an appropriate error indicating that the registry has no external URL and cannot be used for external MCP tool extraction. This matches the pattern used in impl.go at lines 162-164 and 226-227.
Was this helpful? React with 👍 or 👎 to provide feedback.
| func (s *Service) Publish(ctx context.Context, payload *gen.PublishPayload) (*types.MCPRegistry, error) { | ||
| authCtx, ok := contextvalues.GetAuthContext(ctx) | ||
| if !ok || authCtx == nil { | ||
| return nil, oops.C(oops.CodeUnauthorized) |
There was a problem hiding this comment.
🚩 Publish endpoint lacks authorization beyond basic auth check
The Publish method at impl.go:81 checks authCtx != nil but doesn't verify that the caller has permission to create registries for the organization. It uses authCtx.ActiveOrganizationID directly without checking if the user has admin/write permissions. By contrast, the old ListRegistries checked for admin status. The new Publish just trusts that any authenticated user in the org can create catalogs. This might be intentional (any org member can publish), but worth confirming the authorization model is correct for this new mutating endpoint.
Was this helpful? React with 👍 or 👎 to provide feedback.
| id uuid NOT NULL DEFAULT generate_uuidv7(), | ||
| name TEXT NOT NULL CHECK (name <> '' AND CHAR_LENGTH(name) <= 100), | ||
| url TEXT NOT NULL CHECK (url <> '' AND CHAR_LENGTH(url) <= 500), | ||
| name TEXT NOT NULL, |
There was a problem hiding this comment.
🚩 Schema migration drops CHECK constraints from mcp_registries
The migration 20260313205018_allow-internal-registries.sql drops mcp_registries_name_check and mcp_registries_url_check constraints (visible in the diff at schema.sql:1261-1262 where name no longer has CHECK (name <> '' AND CHAR_LENGTH(name) <= 100)). While the url constraint removal is necessary (url is now nullable), the name constraint removal means empty or very long names are now accepted at the database level. The Goa design still specifies MinLength(1) and MaxLength(100) for the publish payload's name, so API-level validation exists, but direct DB inserts (e.g., migrations, admin scripts) could bypass this.
Was this helpful? React with 👍 or 👎 to provide feedback.
| const results = await Promise.all( | ||
| slugsToFetch.map(async (slug) => { | ||
| const result = await client.mcpRegistries.serve({ | ||
| registrySlug: slug, | ||
| search: debouncedSearch || undefined, | ||
| cursor: cursors[slug], | ||
| }); | ||
| return { slug, ...result }; | ||
| }), | ||
| ); |
There was a problem hiding this comment.
🚩 Frontend useInfiniteServeMCPRegistry fires parallel requests to all registries on every page
The old useInfiniteListMCPCatalog made a single API call that aggregated results server-side. The new useInfiniteServeMCPRegistry at hooks.ts:84-93 fires parallel Promise.all requests to each registry slug individually. For organizations with many registries, this means N parallel HTTP requests per page load. This is a behavioral change with potential performance implications — especially on the Home page (Home.tsx:62) and Sources page (Sources.tsx:80) which call this hook on load.
Was this helpful? React with 👍 or 👎 to provide feedback.
qstearns
force-pushed
the
add-registries-ryMY
branch
from
859f608 to
bdbbb45
Compare
We wish to introduce functionality to allow organizations to share groups of servers with each other, but in order to do so, we must model some relationship between organizations. We add the peered_organizations concept underneath the mcp_registries package. The idea is that this should be a change of limited scope to support the particular enterprise use cases. The place of sub-tenancy in Gram's system remains unclear over the long-term
| if !toolset.McpSlug.Valid { | ||
| return nil, oops.E(oops.CodeUnexpected, nil, "toolset %s missing mcp_slug", toolset.Slug).Log(ctx, s.logger) | ||
| } |
There was a problem hiding this comment.
🔴 McpSlug validity check comes after McpSlug is already used, making it ineffective
In serveInternalRegistry, the check if !toolset.McpSlug.Valid at line 334 comes after toolset.McpSlug.String is already used on line 304 (in the remote URL construction) and after the meta map is built. If McpSlug is invalid, the code will have already used the zero-value empty string in the remote URL at line 305, and then the validity check at line 334 will error out — but only after doing unnecessary work. The check should be moved before line 304, right after the toolset is fetched.
Prompt for agents
In server/internal/externalmcp/impl.go, in the serveInternalRegistry method, move the McpSlug validity check (lines 334-336) to right after the search filter (after line 270), i.e. before line 272. This ensures the check occurs before toolset.McpSlug.String is used to build the remote URL on line 305 and the RegistrySpecifier on line 339. The current position at line 334 is too late - the value has already been used.
Was this helpful? React with 👍 or 👎 to provide feedback.
| -- name: SetRegistryToolsets :exec | ||
| WITH deleted AS ( | ||
| DELETE FROM mcp_registry_toolset_links WHERE registry_id = @registry_id | ||
| ) | ||
| INSERT INTO mcp_registry_toolset_links (registry_id, toolset_id) | ||
| SELECT @registry_id, unnest(@toolset_ids::uuid[]); |
There was a problem hiding this comment.
🟡 SetRegistryToolsets SQL fails on empty toolset array due to INSERT with unnest of empty array
The SetRegistryToolsets SQL query (queries.sql:22-27) uses a CTE to delete existing links and then inserts new ones via unnest(@toolset_ids::uuid[]). When toolset_ids is an empty slice (e.g., Publish is called with ToolsetIds: []string{}), unnest of an empty array produces zero rows. With PostgreSQL's CTE semantics, the DELETE in the CTE should still execute. However, the Publish endpoint at impl.go:88 creates toolsetIDs as an empty []uuid.UUID{} when no toolset IDs are provided, and this is then passed to SetRegistryToolsets. The INSERT after unnest on an empty array produces no rows, which with some PostgreSQL/pgx driver versions can cause unexpected behavior. The test TestPublishWithToolsets confirms this works with empty arrays, but the API OpenAPI spec (publishrequestbody.ts:63) has minItems: 1 for toolset_ids, meaning an empty array should be rejected at the API validation layer. The mismatch is that the Go server endpoint and the test explicitly allow empty toolset arrays, while the OpenAPI schema says minItems: 1.
Was this helpful? React with 👍 or 👎 to provide feedback.
| const registrySlugs = registries | ||
| .map((r) => r.slug) | ||
| .filter((s): s is string => s != null); |
There was a problem hiding this comment.
🟡 ImportMCPTabContent creates unstable registrySlugs array on every render, causing infinite query refetches
In ImportMCPTabContent.tsx:112-114, registrySlugs is computed using .map().filter() directly in the component body without useMemo. This creates a new array reference on every render. Since this array is used as part of the useQuery queryKey at line 117, React Query will see a new key on every render and may refetch unnecessarily. Compare with hooks.ts:56-62 which correctly uses useMemo for the same computation.
| const registrySlugs = registries | |
| .map((r) => r.slug) | |
| .filter((s): s is string => s != null); | |
| const registrySlugs = React.useMemo( | |
| () => registries | |
| .map((r) => r.slug) | |
| .filter((s): s is string => s != null), | |
| [registries], | |
| ); |
Was this helpful? React with 👍 or 👎 to provide feedback.
| registry, err := txRepo.CreateInternalRegistry(ctx, repo.CreateInternalRegistryParams{ | ||
| Name: payload.Name, | ||
| Slug: conv.ToPGText(payload.Slug), | ||
| Visibility: payload.Visibility, | ||
| OrganizationID: conv.ToPGText(authCtx.ActiveOrganizationID), | ||
| ProjectID: projectID, | ||
| }) |
There was a problem hiding this comment.
🚩 Publish endpoint does not validate slug format — allows arbitrary strings
The Publish method at impl.go:81-133 accepts the slug field directly from the payload without validating that it conforms to a URL-friendly slug pattern (e.g., lowercase alphanumeric with hyphens). The Goa design at design/externalmcp/design.go:29-30 only enforces MinLength(1) and MaxLength(100). Since slugs are used in URL paths (e.g., the serve endpoint looks up by slug), allowing arbitrary characters could cause routing issues or collisions. The database has a unique index on slug, so duplicates will error, but there's no format validation.
Was this helpful? React with 👍 or 👎 to provide feedback.
| const registrySlugs = useMemo( | ||
| () => | ||
| (registriesData?.registries ?? []) | ||
| .map((r) => r.slug) | ||
| .filter((s): s is string => s != null), | ||
| [registriesData], | ||
| ); |
There was a problem hiding this comment.
🚩 External registries without slugs won't be served by the new frontend
The new frontend (hooks.ts:58-60, ImportMCPTabContent.tsx:112-114) filters registries to only those with a non-null slug. Pre-existing external registries in the database that only have a url (and no slug) will be silently excluded from the catalog and import UIs. The ListRegistriesForOrganization query returns all registries (external and internal), but the frontend discards any without slugs. This is a behavioral regression for external registries that don't have slugs set yet. The migration at 20260313205018_allow-internal-registries.sql adds the slug column but doesn't populate it for existing rows.
Was this helpful? React with 👍 or 👎 to provide feedback.
Add Catalogs page with create dialog for publishing internal registries. Update catalog hooks for multi-registry aggregation with search/cursor. Add serverMetadata helpers supporting both com.pulsemcp/* and ai.getgram/* meta keys. Update sidebar nav and routes. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
qstearns
force-pushed
the
add-registries-ryMY
branch
from
bdbbb45 to
74ccdaf
Compare
|
|
||||||||||||||||
|
|
||||||||||||||||
1 participant
Uh oh!
There was an error while loading. Please reload this page.