chore: rename internal_mcp_catalog.name to slug; introduce internal_m…

[brojd]

…cp_catalog.display_name; handle duplicated names

[London-Cat]

📊 Reputation Summary

User	Rep	Pull Requests	Activity	Assigned	Core Reactions
brojd	⚡ 1856	89✅ 2🔄 9❌	32 issues, 50 comments	3	—

---

How is the score calculated? Read about it in the Reputation Bot repository 🤖

[claude]

Claude finished @brojd's task in 4m 18s —— View job

---

Code Review: Rename internal_mcp_catalog.name to slug; introduce display_name

• Read repository guidelines (CLAUDE.md)
• Analyze PR diff and understand changes
• Review database migration
• Review backend model and route changes
• Review frontend changes
• Review test changes and e2e test changes
• Check for bugs, security issues, and best practices

---

Summary

This PR cleanly decouples the internal MCP catalog's identifier (slug) from its user-facing label (displayName). The migration is well-structured (rename → add nullable column → populate → set NOT NULL → deduplicate → add unique constraint), the ensureUniqueSlug logic handles both create and update paths, and the frontend correctly derives slug from slugify(name) while keeping displayName as the raw user input.

Overall this is solid work. I have a few observations below — one potential issue (the slug/displayName fields being unused in the reauthenticate mutation), and a few minor nits.

---

Potential Issues

1. Unused slug/displayName in useReauthenticateMcpServer mutation body

In platform/frontend/src/lib/mcp-server.query.ts:230, the useReauthenticateMcpServer mutation destructures slug and displayName from the input data but only uses them in toast messages (onSuccess/onError). These fields are stripped from the body sent to the API:

const { id, slug, displayName, ...body } = data;
const response = await reauthenticateMcpServer({
  path: { id },
  body, // slug and displayName are NOT sent to the API
});

This is fine for the API contract (the server doesn't need these for reauthentication), but it means slug is extracted and immediately discarded. The toast uses displayName (line 244), which is the intended behavior. Just wanted to flag this isn't a bug — the naming is clear and the slug field is there so it's consistent with the useReinstallMcpServer pattern. No action needed.

2. ensureUniqueSlug race condition (minor)

In platform/backend/src/models/internal-mcp-catalog.ts:509-530, the ensureUniqueSlug method checks for duplicates then inserts — a classic TOCTOU pattern. If two concurrent requests try to create catalogs with the same slug, both could pass the uniqueness check before either inserts, and one would hit the DB unique constraint. The DB-level UNIQUE("slug") constraint will catch this and throw, which is fine as a safety net. However, the error message would be a raw Postgres unique violation rather than a user-friendly "slug already exists" message.

This is low-risk (requires exact same slug submitted at the exact same time) but worth noting for a potential follow-up: catch the unique constraint violation in create() and either retry with a new suffix or return a friendly error.

---

Observations & Minor Nits

3. searchByQuery uses ilike with unescaped user input

In platform/backend/src/models/internal-mcp-catalog.ts:116:

ilike(schema.internalMcpCatalogTable.slug, `%${query}%`),

The query value is interpolated directly into the LIKE pattern. Drizzle parameterizes this safely against SQL injection, but LIKE metacharacters (% and _) in user input are not escaped. A search for % would match everything; a search for _ would match any single character. This is pre-existing behavior (not introduced by this PR) but worth a future improvement to escape LIKE wildcards.

4. Migration deduplication strategy is sound

The migration at platform/backend/src/database/migrations/0175_big_outlaw_kid.sql uses ROW_NUMBER() OVER (PARTITION BY "slug" ORDER BY "created_at" ASC) to keep the oldest row unchanged and suffix newer duplicates. This is a good strategy — the oldest entry (likely the "canonical" one) retains its clean slug, and newer duplicates get the -xxxx suffix. The display_name is correctly populated from the pre-dedup slug value (step 2 runs before step 3).

5. Frontend transformFormToApiData correctly derives slug from display name

In platform/frontend/src/app/mcp/registry/_parts/mcp-catalog-form.utils.ts:20-21:

slug: slugify(values.name),
displayName: values.name,

And transformCatalogItemToFormValues correctly maps back:

name: item.displayName,  // line 311

With the special-casing for item.slug in auth method detection (line 183-186). This is clean.

6. E2e test failures look related to this PR

The 12 failing e2e tests are mostly in mcp-install.spec.ts, static-credentials-management.spec.ts, chat-auth-required.spec.ts, and dynamic-credentials.spec.ts. These tests interact heavily with catalog items and MCP server installation — exactly the areas this PR modifies. The addCustomSelfHostedCatalogItem utility now correctly finds items by item.slug instead of item.name, but the UI form fills with catalogItemName as the display name. If any test is asserting on the raw name rather than the slugified version, or if a test fixture is missing the displayName field, it could cause failures.

I'd suggest checking whether the failing tests need their expected values updated to account for the slug/displayName split, particularly for any that assert on text content rendered from the catalog item name.

7. The findBySlug method is a useful addition

platform/backend/src/models/internal-mcp-catalog.ts:248-263 — The new findBySlug method and the corresponding DELETE /api/internal_mcp_catalog/by-slug/:slug route are well-implemented. This will be useful for programmatic access where IDs aren't known.

8. Consistent use of slug for server naming

The autoReinstallServer correctly uses catalogItem.slug for K8s deployment names and tool naming prefixes (platform/backend/src/services/mcp-reinstall.ts:108,145), and the install orchestrator uses catalogItem.slug for the server name field (platform/frontend/src/lib/mcp-install-orchestrator.hook.ts:224,287,303). This is consistent with the intent that slug is the machine-stable identifier.

---

Verdict

The PR is well-structured and the approach is sound. The migration handles edge cases (deduplication, nullable→NOT NULL) correctly. The main things to verify are:

1. The 12 failing e2e tests — likely need updates for the slug/displayName split
2. The minor race condition in ensureUniqueSlug (low priority, DB constraint is a safety net)

Approving from a code quality perspective. The e2e test failures should be investigated and fixed before merging.

[github-actions]

Playwright test results

 4 failed
 599 passed
 9 flaky
 10 skipped

Details

 622 tests across 61 suites
 4 minutes, 47 seconds
 63e563e

Failed tests

chromium › ui/mcp-install.spec.ts › MCP Install › Custom remote › No auth required
chromium › ui/mcp-install.spec.ts › MCP Install › Self-hosted from catalog
chromium › ui/mcp-install.spec.ts › MCP Install › Local server with bogus image shows error, logs, and can be fixed
chromium › ui/chat-auth-required.spec.ts › Chat - Auth Required Tool › renders AuthRequiredTool when tool call fails due to missing credentials

Flaky tests

api › api/built-in-agents.spec.ts › Built-In Agents API › auto-configure triggers on individual tool assignment
api › api/knowledge-bases.spec.ts › Knowledge Bases API › Knowledge Base RBAC › member can list knowledge bases
api › api/llm-proxy/jwks-auth.spec.ts › LLM Proxy - External IdP JWKS Authentication › should authenticate with external IdP JWT and get model response
api › api/llm-proxy/token-cost-limits.spec.ts › LLMProxy-TokenCostLimits-Cohere › blocks request when profile token cost limit is exceeded
api › api/llm-proxy/token-cost-limits.spec.ts › LLMProxy-TokenCostLimits-Zhipuai › blocks request when profile token cost limit is exceeded
api › api/llm-settings.spec.ts › LLM Settings API › should read back LLM settings after update
api › api/mcp-gateway.spec.ts › MCP Gateway - External MCP Server Tests › should list internal-dev-test-server tool
api › api/mcp-gateway.spec.ts › MCP Gateway - External MCP Server Tests › should invoke internal-dev-test-server tool successfully
chromium › ui/chat-settings.spec.ts › Provider Settings - Virtual API Keys › Can create a virtual key from the Virtual API Keys tab

Skipped tests

api › api/knowledge-bases.spec.ts › Knowledge Bases API › Connector CRUD › connectors are cascade-deleted when KG is deleted
chromium › ui/mcp-install.spec.ts › MCP Install › Custom remote › Bearer Token
credentials-with-vault › ui/credentials-with-vault.ee.spec.ts › Test self-hosted MCP server with Readonly Vault › Test self-hosted MCP server with Vault - with prompt on installation
credentials-with-vault › ui/credentials-with-vault.ee.spec.ts › Test self-hosted MCP server with Readonly Vault › Test self-hosted MCP server with Vault - without prompt on installation
chromium › ui/dynamic-credentials.spec.ts › Verify tool calling using dynamic credentials
chromium › ui/static-credentials-management.spec.ts › Custom Self-hosted MCP Server - installation and static credentials management (vault disabled, prompt-on-installation disabled) › Admin
chromium › ui/static-credentials-management.spec.ts › Custom Self-hosted MCP Server - installation and static credentials management (vault disabled, prompt-on-installation disabled) › Editor
chromium › ui/static-credentials-management.spec.ts › Custom Self-hosted MCP Server - installation and static credentials management (vault disabled, prompt-on-installation disabled) › Member
chromium › ui/static-credentials-management.spec.ts › Verify Manage Credentials dialog shows correct other users credentials
chromium › ui/static-credentials-management.spec.ts › Verify tool calling using different static credentials