diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index df80382fe..f7e5b2c2c 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -18,6 +18,30 @@ entry. See `CONTRIBUTING.md` § Releases & changelog. ## [Unreleased] +### Fixed — a mistyped id no longer produces a dead-but-configured-looking public MCP binding + +- `public_mcp_key_bindings.key_id` and `agent_id` are not foreign keys — the key + records live in the secret vault and the agents in the in-process registry, not + in Postgres (`migrations/0033`) — and nothing in the application layer compensated. + A one-character typo in either id got `201 Created`, a row in the list, and a + fully-configured-**looking** binding that reached zero tools forever, visually + indistinguishable from a working one. +- The operator write path now resolves both ids against the same sources a real + request does. A `agent_id` the registry does not know is a **hard `400` + (`agent_not_found`)** with no row written — the registry is cheap and + authoritative in-process. A `key_id` that matches no vault record is a + **warning, not a rejection** (the honest interim until the key-lister UI from + #438/#439 ships): the row still saves, but the write response and every list + row carry a `key_id_unknown` warning so the operator sees it reaches nothing. +- The list endpoint annotates **pre-existing** rows too, so a binding that was + already dead — created before this shipped, or bound to an agent later deleted — + is flagged the next time the pane is opened, not only on save. The MCP Control + Center's Public API keys tab renders these warnings inline. +- Fail-honest, never fail-red: when a source cannot be read (no registry wired, a + vault that failed to load) the check returns "cannot tell" and neither rejects + the agent nor invents a warning, so a transient read failure never paints a + working install as broken. + ### Fixed — `per_user` MCP delegation was unreachable from chat - Migration `0031` made delegation explicit per MCP server and gave new servers a diff --git a/middleware/src/index.ts b/middleware/src/index.ts index ff905e492..7fc041ed8 100644 --- a/middleware/src/index.ts +++ b/middleware/src/index.ts @@ -162,7 +162,7 @@ import { type MdnsAdvertisement, } from './pairing/mdns.js'; import { publicPaths } from './auth/publicPaths.js'; -import { mountPublicMcp } from './mcp/wirePublicMcp.js'; +import { createVerifyOnlyApiKeyStore, mountPublicMcp } from './mcp/wirePublicMcp.js'; // W5-1 — the WRITE half of `public_mcp_key_bindings`. Imported for the // OPERATOR router only. `mountPublicMcp` above must never be handed this: the // internet-facing endpoint gets `createPublicMcpKeyBindingStore` (read-only) @@ -2755,6 +2755,52 @@ async function main(): Promise { // Explicit gate on those routes, independent of the `requireAuth` that // sits in front of this mount. operatorAuth, + // #571 — resolve the two ids a binding points at, so a one-character typo + // is a 400 (agent) or a warning (key) rather than a + // fully-configured-looking row that reaches zero tools forever. Both + // sources are read LIVE and from the SAME places a real request resolves + // against: `configStore` for registered agent slugs, and the verify-only + // API-key store the public MCP endpoint itself authenticates through + // (`createVerifyOnlyApiKeyStore` over the shared vault namespace). A read + // that throws or finds no source returns `undefined` — "cannot tell", + // which the router never treats as "unknown". + publicMcpBindingExistence: { + async knownAgentIds() { + // Keyed on SLUG, not the agent uuid: `agent_id` stores the + // orchestrator slug (migration `0033`, and the UI's agent picker sends + // `option.value = slug`). A caller that sends a uuid is correctly + // rejected — it is not a valid `agent_id`. + const configStore = + serviceRegistry.get('configStore'); + if (!configStore) return undefined; + try { + return new Set((await configStore.listAgents()).map((a) => a.slug)); + } catch (err) { + console.warn( + `[middleware] public-mcp binding agent lister unavailable: ${String(err)}`, + ); + return undefined; + } + }, + async knownKeyIds() { + try { + // O(keys) per call — enumerates the channel-api vault namespace and + // parses each record. Fine at operator scale (one read per list-page + // load, one per save); revisit with a cache if an install ever holds + // thousands of keys. The store is a thin read-only adapter, cheap to + // rebuild, but pinned here so the cost is one construction per call + // rather than hidden in a closure. + const keyStore = createVerifyOnlyApiKeyStore(secretVault); + const keys = await keyStore.list(); + return new Set(keys.map((k) => k.id)); + } catch (err) { + console.warn( + `[middleware] public-mcp binding key lister unavailable: ${String(err)}`, + ); + return undefined; + } + }, + }, }), ); console.log( diff --git a/middleware/src/routes/agentBuilder.ts b/middleware/src/routes/agentBuilder.ts index ea04e255c..8c811a3b9 100644 --- a/middleware/src/routes/agentBuilder.ts +++ b/middleware/src/routes/agentBuilder.ts @@ -82,6 +82,7 @@ import { } from '../services/skillVerdictLlmVerifier.js'; import { createPublicMcpBindingsRouter, + type BindingExistenceCheck, type OperatorSessionCheck, } from './publicMcpBindingsRouter.js'; import type { PublicMcpKeyBindingAdminStore } from '../mcp/publicMcpKeyBindingsAdmin.js'; @@ -165,6 +166,12 @@ export interface AgentBuilderRouterOptions { * to serve at all, rather than relying on the `requireAuth` that happens to * sit in front of this router's mount. */ readonly operatorAuth?: OperatorSessionCheck; + /** #571 — resolves whether a binding's `agent_id` / `key_id` actually exist, + * so a typo is a 400 (agent) or a warning (key) instead of a + * fully-configured-looking dead row. Forwarded verbatim to the binding + * router; absent ⇒ existence is never checked. Built in `index.ts`, the one + * place with both the registry and the API-key vault in scope. */ + readonly publicMcpBindingExistence?: BindingExistenceCheck; } interface Live { @@ -242,6 +249,9 @@ export function createAgentBuilderRouter( createPublicMcpBindingsRouter({ getStore: () => options.getPublicMcpBindingStore?.(), ...(options.operatorAuth ? { operatorAuth: options.operatorAuth } : {}), + ...(options.publicMcpBindingExistence + ? { existence: options.publicMcpBindingExistence } + : {}), }), ); diff --git a/middleware/src/routes/publicMcpBindingsRouter.ts b/middleware/src/routes/publicMcpBindingsRouter.ts index dd6831f9b..99a0adbef 100644 --- a/middleware/src/routes/publicMcpBindingsRouter.ts +++ b/middleware/src/routes/publicMcpBindingsRouter.ts @@ -25,6 +25,7 @@ import { Router, type NextFunction, type Request, type Response } from 'express'; import type { + PublicMcpKeyBindingAdminRow, PublicMcpKeyBindingAdminStore, PublicMcpKeyBindingInput, } from '../mcp/publicMcpKeyBindingsAdmin.js'; @@ -37,6 +38,53 @@ export interface OperatorSessionCheck { hasValidSession(cookieHeader: string | undefined): Promise; } +/** + * Non-fatal note attached to a binding — "this row is configured, but the + * `key_id`/`agent_id` it points at does not resolve, so it reaches nothing". + * + * The whole reason this surface exists (issue #571): a one-character typo in + * either id produced a `201 Created`, a row in the list, and a + * fully-configured-LOOKING binding that reaches zero tools forever, visually + * indistinguishable from a working one. A warning is that missing distinction — + * carried on the write response AND on every list row, so a typo made before + * this shipped is still flagged the next time an operator opens the pane. + */ +export interface BindingWarning { + readonly code: 'key_id_unknown' | 'agent_id_unknown'; + readonly message: string; +} + +/** A list/write result with its non-fatal warnings, if any. `warnings` is + * omitted rather than empty when the row resolves cleanly, so a green binding + * serializes to exactly the pre-#571 shape. */ +export type AnnotatedBinding = PublicMcpKeyBindingAdminRow & { + readonly warnings?: readonly BindingWarning[]; +}; + +/** + * Answers "does this id actually resolve" for the two ids a binding points at, + * neither of which the database can enforce: `agent_id` names an in-process + * registry slug, and `key_id` names a record in the secret vault (see + * `migrations/0033_public_mcp_keys.sql` — deliberately NOT foreign keys). + * + * Both methods return the full known-id SET rather than a per-id predicate so + * the list route pays ONE registry read and ONE vault enumeration for the whole + * page instead of one per row. A `undefined` return means "the source could not + * be read" — an older host with no registry wired, a vault that failed to load — + * and is treated as "cannot tell", never as "unknown". The asymmetry between the + * two ids is the issue's: an unknown agent is a HARD reject (the registry is + * cheap and authoritative in-process), an unknown key is only a WARNING (the key + * lister is the interim half — see the router's POST handler). + */ +export interface BindingExistenceCheck { + /** Slugs of every agent the registry currently knows, or `undefined` when the + * registry cannot be read. */ + knownAgentIds(): Promise | undefined>; + /** Ids of every API-key record in the vault, or `undefined` when the vault + * cannot be read. */ + knownKeyIds(): Promise | undefined>; +} + export interface PublicMcpBindingsRouterOptions { /** Absent ⇒ every route 503s. The store needs the graph pool; without it * there is nothing to read or write. */ @@ -44,6 +92,11 @@ export interface PublicMcpBindingsRouterOptions { /** Absent ⇒ every route 503s, BEFORE any handler runs. Never a fallback to * "unauthenticated but mounted". */ readonly operatorAuth?: OperatorSessionCheck; + /** Absent ⇒ existence is never checked (an older host, or a test that does not + * exercise it): every id is accepted and no row is annotated, exactly the + * pre-#571 behaviour. Wired, it turns a typo'd `agent_id` into a 400 and a + * typo'd `key_id` into a warning. */ + readonly existence?: BindingExistenceCheck; } export function createPublicMcpBindingsRouter( @@ -112,12 +165,58 @@ export function createPublicMcpBindingsRouter( return store; } + /** A binding whose `agent_id` names no registered agent — the write path + * rejects this, so it can only reach the list from a row created before + * #571 shipped (or on a host with no registry wired). */ + function agentUnknownWarning(agentId: string): BindingWarning { + return { + code: 'agent_id_unknown', + message: `no agent "${agentId}" is registered — this binding reaches nothing`, + }; + } + + /** A binding whose `key_id` matches no vault record. Only ever a warning: the + * key lister is the interim half (issue #571), and a key created out of band + * a moment ago must not be rejected by a stale read. */ + function keyUnknownWarning(): BindingWarning { + return { + code: 'key_id_unknown', + message: + 'no API key with this id exists — this binding reaches nothing until such a key is created', + }; + } + + /** + * Reads both known-id sets ONCE and returns a per-row annotator. + * + * A row is flagged only when the relevant set is READABLE and does not hold + * the id; an unreadable set (`undefined`) flags nothing, so "the vault failed + * to load" never masquerades as "every key is dead" and paints a working + * install red. With no `existence` wired at all, every row passes through + * untouched — the pre-#571 serialization. + */ + async function loadAnnotator(): Promise< + (row: PublicMcpKeyBindingAdminRow) => AnnotatedBinding + > { + const { existence } = options; + const [agents, keys] = existence + ? await Promise.all([existence.knownAgentIds(), existence.knownKeyIds()]) + : [undefined, undefined]; + return (row) => { + const warnings: BindingWarning[] = []; + if (agents && !agents.has(row.agentId)) warnings.push(agentUnknownWarning(row.agentId)); + if (keys && !keys.has(row.keyId)) warnings.push(keyUnknownWarning()); + return warnings.length > 0 ? { ...row, warnings } : row; + }; + } + // ── List ──────────────────────────────────────────────────────────────── router.get('/', async (_req: Request, res: Response) => { const store = storeOr503(res); if (!store) return; try { - res.json({ bindings: await store.list() }); + const annotate = await loadAnnotator(); + res.json({ bindings: (await store.list()).map(annotate) }); } catch (err) { fail(res, 'public_mcp_bindings.list_failed', err); } @@ -175,19 +274,58 @@ export function createPublicMcpBindingsRouter( ...(rawEnabled === undefined ? {} : { enabled: rawEnabled }), }; - // The reader's own rules decide. See `validateBindingInput`. + // The reader's own rules decide the SHAPE. See `validateBindingInput`. const validated = validateBindingInput(input); if (!validated.ok) { res.status(400).json({ error: 'invalid_request', ...validated.error }); return; } + // EXISTENCE, the #571 half the shape check cannot cover. A well-formed id is + // not a resolvable one: `agent_id` names an in-process registry slug and + // `key_id` a vault record, neither a foreign key the DB could enforce. + // + // The two halves diverge on purpose. The agent registry is authoritative + // in-process, so a typo'd agent is a HARD reject — better a 400 the operator + // sees now than a row that looks configured and silently reaches nothing. A + // `undefined` set means the registry could not be read; that is "cannot + // tell", never "unknown", so it does NOT reject. + // + // Both existence reads run INSIDE the same try as the upsert. The interface + // documents `knownAgentIds`/`knownKeyIds` as returning `undefined` rather + // than throwing, but a broken impl that threw here — outside the try — would + // escape `fail()` and answer with an unsanitized 500 (or hang), leaking the + // very pg/vault internals `fail()` exists to hide. + const { existence } = options; try { + if (existence) { + const agents = await existence.knownAgentIds(); + if (agents && !agents.has(validated.value.agentId)) { + res.status(400).json({ + error: 'invalid_request', + code: 'agent_not_found', + message: `no agent "${validated.value.agentId}" is registered; bind to an existing agent`, + }); + return; + } + } + const { binding, created } = await store.upsert(validated.value); + + // The key is only ever a WARNING (see `keyUnknownWarning`): the write + // still succeeds and the row is stored, but the operator is told the key + // does not resolve rather than being left to discover it when the + // integration reaches zero tools. Checked AFTER the upsert so a vault read + // failure cannot cost a legitimate save. + const keys = existence ? await existence.knownKeyIds() : undefined; + const warnings = keys && !keys.has(binding.keyId) ? [keyUnknownWarning()] : []; + // 201 only for a row that did not exist. "Created" over an existing // binding is the operator's only per-request hint that they landed on // somebody else's row — spending it on every save makes it worthless. - res.status(created ? 201 : 200).json({ binding }); + res.status(created ? 201 : 200).json({ + binding: warnings.length > 0 ? { ...binding, warnings } : binding, + }); } catch (err) { fail(res, 'public_mcp_bindings.upsert_failed', err); } diff --git a/middleware/test/publicMcp/publicMcpKeyBindingsAdmin.test.ts b/middleware/test/publicMcp/publicMcpKeyBindingsAdmin.test.ts index 9a0ab0d06..d8bc57d10 100644 --- a/middleware/test/publicMcp/publicMcpKeyBindingsAdmin.test.ts +++ b/middleware/test/publicMcp/publicMcpKeyBindingsAdmin.test.ts @@ -10,9 +10,13 @@ import { createInMemoryPublicMcpKeyBindingAdminStore, createPublicMcpKeyBindingAdminStore, validateBindingInput, + type PublicMcpKeyBindingAdminRow, type PublicMcpKeyBindingAdminStore, } from '../../src/mcp/publicMcpKeyBindingsAdmin.js'; -import { createPublicMcpBindingsRouter } from '../../src/routes/publicMcpBindingsRouter.js'; +import { + createPublicMcpBindingsRouter, + type BindingExistenceCheck, +} from '../../src/routes/publicMcpBindingsRouter.js'; import { createInMemoryPublicMcpKeyBindingStore } from '../../src/mcp/publicMcpKeyBindings.js'; /** @@ -54,6 +58,7 @@ function neverValidOperatorAuth(): { hasValidSession(): Promise } { function mountRouter(opts: { store?: PublicMcpKeyBindingAdminStore | undefined; operatorAuth?: { hasValidSession(cookie: string | undefined): Promise }; + existence?: BindingExistenceCheck; }): { server: Server; baseUrl: string } { const app = express(); app.use(express.json()); @@ -62,6 +67,7 @@ function mountRouter(opts: { createPublicMcpBindingsRouter({ getStore: () => opts.store, ...(opts.operatorAuth ? { operatorAuth: opts.operatorAuth } : {}), + ...(opts.existence ? { existence: opts.existence } : {}), }), ); const server = app.listen(0); @@ -338,6 +344,145 @@ describe('publicMcpBindingsRouter — CRUD (auth stubbed valid)', () => { }); }); +// ── #571: id existence — a typo must not look configured ───────────────────── + +/** A stub `BindingExistenceCheck`. `undefined` for either list models the + * "source could not be read" case the router must treat as cannot-tell. */ +function existenceOf( + agents: readonly string[] | undefined, + keys: readonly string[] | undefined, +): BindingExistenceCheck { + return { + async knownAgentIds() { + return agents ? new Set(agents) : undefined; + }, + async knownKeyIds() { + return keys ? new Set(keys) : undefined; + }, + }; +} + +function seededRow(keyId: string, agentId: string): PublicMcpKeyBindingAdminRow { + return { + keyId, + agentId, + readTools: [], + writeTools: [], + writeRateLimitPerMinute: 5, + enabled: true, + createdAt: '2026-01-01T00:00:00.000Z', + updatedAt: '2026-01-01T00:00:00.000Z', + }; +} + +describe('publicMcpBindingsRouter — #571 id existence (agent hard-reject, key warning)', () => { + const auth = alwaysValidOperatorAuth(); + + it('POST with an agent the registry does not know → 400 agent_not_found, and NO row', async () => { + const store = createInMemoryPublicMcpKeyBindingAdminStore(); + await withRouter( + { store, operatorAuth: auth, existence: existenceOf(['sales'], ['key-1']) }, + async (url) => { + const res = await postBinding(url, { ...VALID_INPUT, agentId: 'saels' }); + assert.equal(res.status, 400); + assert.equal(((await res.json()) as { code: string }).code, 'agent_not_found'); + assert.deepEqual(await store.list(), [], "a typo'd agent must not leave a row"); + }, + ); + }); + + it('POST with a key the vault does not hold → still created, carrying a key_id_unknown WARNING', async () => { + const store = createInMemoryPublicMcpKeyBindingAdminStore(); + await withRouter( + { store, operatorAuth: auth, existence: existenceOf(['sales'], ['some-other-key']) }, + async (url) => { + const res = await postBinding(url, VALID_INPUT); // key-1 is not in the vault set + assert.equal(res.status, 201, 'an unknown key is a warning, not a rejection'); + const { binding } = (await res.json()) as { + binding: { keyId: string; warnings?: { code: string }[] }; + }; + assert.equal(binding.keyId, 'key-1'); + assert.deepEqual((binding.warnings ?? []).map((w) => w.code), ['key_id_unknown']); + assert.equal((await store.list()).length, 1, 'the row is stored despite the warning'); + }, + ); + }); + + it('POST with BOTH ids unknown → the agent reject wins; no row, no key warning reached', async () => { + const store = createInMemoryPublicMcpKeyBindingAdminStore(); + await withRouter( + { store, operatorAuth: auth, existence: existenceOf(['sales'], ['key-1']) }, + async (url) => { + const res = await postBinding(url, { keyId: 'ghost', agentId: 'ghost' }); + assert.equal(res.status, 400); + assert.equal(((await res.json()) as { code: string }).code, 'agent_not_found'); + assert.deepEqual(await store.list(), [], 'a rejected agent must never reach the upsert'); + }, + ); + }); + + it('POST with both ids resolvable → 201 and NO warnings field (unchanged happy path)', async () => { + const store = createInMemoryPublicMcpKeyBindingAdminStore(); + await withRouter( + { store, operatorAuth: auth, existence: existenceOf(['sales'], ['key-1']) }, + async (url) => { + const res = await postBinding(url, VALID_INPUT); + assert.equal(res.status, 201); + const { binding } = (await res.json()) as { binding: Record }; + assert.equal('warnings' in binding, false, 'a clean row serializes exactly as pre-#571'); + }, + ); + }); + + it('GET annotates a pre-existing row whose ids no longer resolve — the core "indistinguishable" fix', async () => { + // Seeded directly, as if the rows were created before this shipped (or by + // hand in psql): the write path would now reject the agent, but the list + // must still flag what is already stored. + const store = createInMemoryPublicMcpKeyBindingAdminStore([ + seededRow('ghost-key', 'ghost-agent'), + seededRow('key-1', 'sales'), + ]); + await withRouter( + { store, operatorAuth: auth, existence: existenceOf(['sales'], ['key-1']) }, + async (url) => { + const { bindings } = (await (await fetch(url)).json()) as { + bindings: { keyId: string; warnings?: { code: string }[] }[]; + }; + const ghost = bindings.find((b) => b.keyId === 'ghost-key'); + assert.ok(ghost, 'the dead row must still be listed'); + assert.deepEqual( + (ghost.warnings ?? []).map((w) => w.code).sort(), + ['agent_id_unknown', 'key_id_unknown'], + ); + const healthy = bindings.find((b) => b.keyId === 'key-1'); + assert.ok(healthy); + assert.equal('warnings' in healthy, false, 'the healthy row is not annotated'); + }, + ); + }); + + it('an unreadable source (undefined sets) neither rejects the agent nor invents warnings', async () => { + const store = createInMemoryPublicMcpKeyBindingAdminStore(); + await withRouter( + { store, operatorAuth: auth, existence: existenceOf(undefined, undefined) }, + async (url) => { + const res = await postBinding(url, { ...VALID_INPUT, agentId: 'anything' }); + assert.equal(res.status, 201, 'cannot-tell must never become a rejection'); + const { binding } = (await res.json()) as { binding: Record }; + assert.equal('warnings' in binding, false, 'cannot-tell must not paint a row red'); + }, + ); + }); + + it('with no existence wired at all, every id is accepted (pre-#571 behaviour preserved)', async () => { + const store = createInMemoryPublicMcpKeyBindingAdminStore(); + await withRouter({ store, operatorAuth: auth }, async (url) => { + const res = await postBinding(url, { ...VALID_INPUT, agentId: 'whatever' }); + assert.equal(res.status, 201); + }); + }); +}); + // ── validateBindingInput: the writer/reader contract ──────────────────────── describe('validateBindingInput — the admin path cannot drift from the reader', () => { diff --git a/web-ui/app/_lib/agentBuilder.ts b/web-ui/app/_lib/agentBuilder.ts index d3160d7ac..62a57fe04 100644 --- a/web-ui/app/_lib/agentBuilder.ts +++ b/web-ui/app/_lib/agentBuilder.ts @@ -1048,6 +1048,15 @@ export async function exportSkill(id: string): Promise { * public MCP endpoint. `enabled: false` is a PARKED binding: the key reaches * nothing, but what it was configured to reach is still on the row. */ +/** #571 — a non-fatal note that a binding points at an id that does not + * resolve, so it reaches nothing despite looking configured. `code` is the + * stable, locale-independent discriminator the pane renders from; `message` is + * the server's English fallback, for API consumers and logs. */ +export interface PublicMcpKeyBindingWarning { + code: 'key_id_unknown' | 'agent_id_unknown'; + message: string; +} + export interface PublicMcpKeyBinding { keyId: string; agentId: string; @@ -1057,6 +1066,9 @@ export interface PublicMcpKeyBinding { enabled: boolean; createdAt: string; updatedAt: string; + /** #571 — present only when the key or agent this row names does not resolve. + * Absent on a clean row, so a healthy binding is unchanged from before. */ + warnings?: PublicMcpKeyBindingWarning[]; } export interface PublicMcpKeyBindingsResponse { diff --git a/web-ui/app/admin/mcp/__tests__/bindingsWarnings.test.tsx b/web-ui/app/admin/mcp/__tests__/bindingsWarnings.test.tsx new file mode 100644 index 000000000..37b5da166 --- /dev/null +++ b/web-ui/app/admin/mcp/__tests__/bindingsWarnings.test.tsx @@ -0,0 +1,104 @@ +import { screen, waitFor } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +import { renderWithIntl } from '../../../_lib/test-utils'; +import type { PublicMcpKeyBinding } from '../../../_lib/agentBuilder'; +import AdminMcpPage from '../page'; + +/** + * Issue #571 — a binding whose `key_id`/`agent_id` does not resolve is a dead + * row that LOOKS configured. The server annotates such rows with a warning; this + * pane must render that warning so a typo is visually distinguishable from a + * working binding. These tests pin the visible half of the fix: the badge + * appears when — and only when — the server flagged the row. + */ + +const { mockListBindings, mockListOrchestrators, mockListServers } = vi.hoisted(() => ({ + mockListBindings: vi.fn(), + mockListOrchestrators: vi.fn(), + mockListServers: vi.fn(), +})); + +vi.mock('../../../_lib/agentBuilder', async (importOriginal) => ({ + ...(await importOriginal()), + listPublicMcpKeyBindings: mockListBindings, + listMcpOrchestrators: mockListOrchestrators, + listMcpServers: mockListServers, +})); + +function binding(overrides: Partial): PublicMcpKeyBinding { + return { + keyId: 'key-1', + agentId: 'sales', + readTools: [], + writeTools: [], + writeRateLimitPerMinute: 5, + enabled: true, + createdAt: '2026-01-01T00:00:00.000Z', + updatedAt: '2026-01-01T00:00:00.000Z', + ...overrides, + }; +} + +beforeEach(() => { + mockListServers.mockResolvedValue({ servers: [] }); + mockListOrchestrators.mockResolvedValue({ orchestrators: [] }); + mockListBindings.mockReset(); +}); + +async function openBindingsTab(): Promise { + const user = userEvent.setup(); + renderWithIntl(); + await user.click(screen.getByRole('button', { name: 'Public API keys' })); +} + +describe('BindingsPane — #571 dead-binding warnings', () => { + it('renders the key-unknown warning on a row the server flagged', async () => { + mockListBindings.mockResolvedValue({ + bindings: [ + binding({ + keyId: 'typoed-key', + warnings: [{ code: 'key_id_unknown', message: 'server english fallback' }], + }), + ], + }); + + await openBindingsTab(); + + // Rendered from the locale catalog, not the server's English `message`. + await waitFor(() => + expect(screen.getByText(/No API key with this id exists yet/i)).toBeTruthy(), + ); + // The server's raw message is deliberately NOT shown. + expect(screen.queryByText('server english fallback')).toBeNull(); + }); + + it('renders the agent-unknown warning for a row bound to a vanished agent', async () => { + mockListBindings.mockResolvedValue({ + bindings: [ + binding({ + keyId: 'orphan', + agentId: 'deleted-agent', + warnings: [{ code: 'agent_id_unknown', message: 'x' }], + }), + ], + }); + + await openBindingsTab(); + + await waitFor(() => + expect(screen.getByText(/names an agent that is no longer registered/i)).toBeTruthy(), + ); + }); + + it('shows NO warning on a clean binding — a healthy row is unchanged', async () => { + mockListBindings.mockResolvedValue({ bindings: [binding({ keyId: 'healthy' })] }); + + await openBindingsTab(); + + await waitFor(() => expect(screen.getByText('healthy')).toBeTruthy()); + expect(screen.queryByText(/No API key with this id exists yet/i)).toBeNull(); + expect(screen.queryByText(/no longer registered/i)).toBeNull(); + }); +}); diff --git a/web-ui/app/admin/mcp/page.tsx b/web-ui/app/admin/mcp/page.tsx index 432e5719f..d0159b643 100644 --- a/web-ui/app/admin/mcp/page.tsx +++ b/web-ui/app/admin/mcp/page.tsx @@ -52,6 +52,7 @@ import { type McpServerNode, type McpTransport, type PublicMcpKeyBinding, + type PublicMcpKeyBindingWarning, type SkillVerdictSeverity, } from '../../_lib/agentBuilder'; @@ -1814,6 +1815,10 @@ function BindingsPane(): React.ReactElement { const [busy, setBusy] = useState(null); const [confirmRevoke, setConfirmRevoke] = useState(null); const [confirmRestore, setConfirmRestore] = useState(null); + // #571 — warnings the server returned for the row we just saved. A save + // succeeds even when the key/agent does not resolve, so this is the operator's + // immediate signal that the row they just created reaches nothing. + const [savedWarnings, setSavedWarnings] = useState([]); const [keyId, setKeyId] = useState(''); const [agentId, setAgentId] = useState(''); @@ -1842,14 +1847,19 @@ function BindingsPane(): React.ReactElement { async function save(): Promise { setBusy('save'); setError(null); + setSavedWarnings([]); try { - await upsertPublicMcpKeyBinding({ + const { binding } = await upsertPublicMcpKeyBinding({ keyId: keyId.trim(), agentId: agentId.trim(), readTools: parseToolList(readTools), writeTools: parseToolList(writeTools), writeRateLimitPerMinute: Number(writeRate), }); + // A typo'd key still saves (warning, not rejection) — surface it now so it + // is not mistaken for a working binding. A typo'd agent never reaches here: + // the server 400s and we land in `catch` above. + setSavedWarnings(binding.warnings ?? []); setKeyId(''); setReadTools(''); setWriteTools(''); @@ -1898,13 +1908,36 @@ function BindingsPane(): React.ReactElement { const inputCls = 'rounded-md border border-[color:var(--border)] bg-transparent px-3 py-2 text-sm outline-none focus:border-[color:var(--accent)]'; + // Deliberately still just non-empty — existence is NOT re-checked here (#571). + // The agent field is a constrained dropdown whenever the orchestrator list + // loaded, so a typo is impossible on that path; when the list is unavailable + // the field falls back to free text and there is nothing to validate against. + // The key has no client-side lister at all. So the authoritative existence + // check lives on the server (agent → 400, key → warning); mirroring a weaker + // copy of it here would only drift. const canSave = keyId.trim().length > 0 && agentId.trim().length > 0; + // Rendered from the stable `code`, never the server's English `message`, so + // the note follows the operator's locale. See `PublicMcpKeyBindingWarning`. + const warningText = (code: PublicMcpKeyBindingWarning['code']): string => + code === 'key_id_unknown' + ? t('bindings.warnings.keyUnknown') + : t('bindings.warnings.agentUnknown'); + return (

{t('bindings.intro')}

{t('bindings.keyIdHint')}

{error ?
{error}
: null} + {savedWarnings.length > 0 ? ( +
+ {savedWarnings.map((w) => ( + + {warningText(w.code)} + + ))} +
+ ) : null}
+ {b.warnings && b.warnings.length > 0 ? ( +
+ {b.warnings.map((w) => ( + + {warningText(w.code)} + + ))} +
+ ) : null}
{t('bindings.readToolsLabel')}:{' '} {b.readTools.length > 0 ? b.readTools.join(', ') : t('bindings.none')} diff --git a/web-ui/messages/de.json b/web-ui/messages/de.json index 8abcc34df..dfa371584 100644 --- a/web-ui/messages/de.json +++ b/web-ui/messages/de.json @@ -2828,7 +2828,11 @@ "restoreHint": "Erneutes Speichern ändert die Tools, hebt den Widerruf aber nicht auf. Nur Wiederherstellen macht den Key wieder nutzbar.", "restoreTitle": "Dieses Binding wiederherstellen?", "restoreBody": "Der Key {keyId} erhält sofort alles zurück, was auf dieser Zeile steht: {tools}.", - "restoreConfirm": "Wiederherstellen" + "restoreConfirm": "Wiederherstellen", + "warnings": { + "keyUnknown": "Es existiert noch kein API-Key mit dieser ID. Das Binding wurde gespeichert, erreicht aber nichts, solange kein solcher Key angelegt ist – prüfe die ID auf einen Tippfehler.", + "agentUnknown": "Dieses Binding nennt einen Agenten, der nicht mehr registriert ist, und erreicht daher nichts. Verknüpfe es mit einem existierenden Agenten." + } }, "plugins": { "intro": "Plugins, die im Manifest MCP-Zugriff deklarieren. Weise jedem gezielt die Server zu, die es erreichen darf — nichts ist implizit. Die Per-Tool-Sicherheit greift weiterhin: ein ungescanntes oder hochriskantes Tool wird zur Aufrufzeit abgelehnt.", diff --git a/web-ui/messages/en.json b/web-ui/messages/en.json index 7c86708b1..c1c9d2ee3 100644 --- a/web-ui/messages/en.json +++ b/web-ui/messages/en.json @@ -2828,7 +2828,11 @@ "restoreHint": "Saving this binding again edits its tools but leaves it revoked. Restoring is the only way to make the key work again.", "restoreTitle": "Restore this binding?", "restoreBody": "The key {keyId} immediately regains everything on this row: {tools}.", - "restoreConfirm": "Restore" + "restoreConfirm": "Restore", + "warnings": { + "keyUnknown": "No API key with this id exists yet. The binding was saved, but it reaches nothing until such a key is created — double-check the id for a typo.", + "agentUnknown": "This binding names an agent that is no longer registered, so it reaches nothing. Rebind it to an existing agent." + } }, "plugins": { "intro": "Plugins that declare MCP access in their manifest. Grant each the specific servers it may reach — nothing is ambient. Per-tool safety still applies: an unscanned or high-risk tool is refused at call time.",