From 0f628a81741b0c112683d615d09e2be5b4069fd7 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 23 Aug 2026 13:13:04 -0400 Subject: [PATCH] fix: address Copilot review on the _meta widening MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Widen the three `_meta` annotations the bulk pass missed, all of them return types rather than identifier declarations: `mergeMeta`, `aggregateListParams`, and the `tools/call` params object. The second was unsound in a way callers could see — it advertised `{ _meta: Record }` while the runtime carried nested JSON. - `normalizeStoredMetadata`: keep an explicit `null` from a legacy pair instead of folding it into `""` (only a missing `value` defaults now), and store through `Object.defineProperty` so a legacy `__proto__` key becomes an own property rather than hitting the prototype setter. - Drop the unjustified `as unknown as` from the migration test; the legacy fixture is untyped and cast once where it crosses into the typed parser. - The TUI settings test asserted only that the app mounted, so it could not have caught a pair array being forwarded as numeric `_meta` keys. The fake client now captures its constructor options and the test asserts the nested object arrives intact. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh Signed-off-by: cliffhall --- clients/tui/__tests__/App.test.tsx | 30 +++++++++++++---- .../web/src/test/core/mcp/serverList.test.ts | 33 +++++++++++++++---- core/mcp/inspectorClient.ts | 6 ++-- core/mcp/serverList.ts | 14 +++++++- 4 files changed, 67 insertions(+), 16 deletions(-) diff --git a/clients/tui/__tests__/App.test.tsx b/clients/tui/__tests__/App.test.tsx index 3e749dff..13a13e08 100644 --- a/clients/tui/__tests__/App.test.tsx +++ b/clients/tui/__tests__/App.test.tsx @@ -102,7 +102,13 @@ const h = vi.hoisted(() => { // handler for the event (the common single-server case). type EventEntry = { client: unknown; fn: (event: unknown) => void }; const clientEvents = new Map>(); - const clientInstances: Array<{ cfg?: { type?: string; url?: string } }> = []; + const clientInstances: Array<{ + cfg?: { type?: string; url?: string }; + // The options the mount effect built for this server. Captured so a test + // can assert what was actually handed to the client — `defaultMetadata` + // in particular, which is otherwise invisible to a render-only assertion. + opts?: { defaultMetadata?: unknown }; + }> = []; const fireClientEvent = (event: string, detail?: unknown) => { clientEvents.get(event)?.forEach((e) => e.fn({ detail })); }; @@ -127,8 +133,13 @@ const h = vi.hoisted(() => { } class FakeClient { cfg: { type?: string; url?: string } | undefined; - constructor(config?: { type?: string; url?: string }) { + opts: { defaultMetadata?: unknown } | undefined; + constructor( + config?: { type?: string; url?: string }, + opts?: { defaultMetadata?: unknown }, + ) { this.cfg = config; + this.opts = opts; clientInstances.push(this); } // Derive the transport type from the server config the client was built @@ -393,10 +404,10 @@ function httpWithSettings(): Record { config: { type: "streamable-http", url: "http://x" }, settings: { requestTimeout: 5000, - metadata: [ - { key: "team", value: "alpha" }, - { key: " ", value: "ignored" }, - ], + // Object-shaped with a nested value (#1910) — the shape the TUI now + // forwards verbatim. As a pair array this would have reached the wire + // as numeric `_meta` keys. + metadata: { team: "alpha", trace: { id: "abc", hops: [1, 2] } }, oauthClientId: "cid", oauthClientSecret: "secret", oauthScopes: "read write", @@ -1114,6 +1125,13 @@ describe("App (input handling, focus, effects)", () => { it("builds a client with saved settings (metadata, oauth, timeout)", async () => { const r = await mount(httpWithSettings()); await expectFrame(r, "MCP Servers"); + // Not just "it mounted": the saved metadata must reach the client as the + // same object, nesting intact (#1910). + const web = h.clientInstances.find((c) => c.cfg?.url === "http://x"); + expect(web?.opts?.defaultMetadata).toEqual({ + team: "alpha", + trace: { id: "abc", hops: [1, 2] }, + }); }); it("passes top-level oauth client credentials into an http client", async () => { diff --git a/clients/web/src/test/core/mcp/serverList.test.ts b/clients/web/src/test/core/mcp/serverList.test.ts index 13559751..2ad70e4c 100644 --- a/clients/web/src/test/core/mcp/serverList.test.ts +++ b/clients/web/src/test/core/mcp/serverList.test.ts @@ -1424,6 +1424,26 @@ describe("normalizeStoredMetadata (#1910)", () => { ).toEqual({ kept: "yes" }); }); + it("preserves an explicit null in a legacy pair rather than blanking it", () => { + // `null` is a legal `_meta` value, so the migration must not fold it into + // `""` — only a genuinely absent `value` defaults. + expect(normalizeStoredMetadata([{ key: "z", value: null }])).toEqual({ + z: null, + }); + }); + + it("stores a legacy `__proto__` key as an own property", () => { + // Plain assignment would hit the prototype setter: the entry would vanish + // (and an object value would mutate the prototype) instead of being stored. + const out = normalizeStoredMetadata([ + { key: "__proto__", value: { polluted: true } }, + ]); + expect(Object.hasOwn(out, "__proto__")).toBe(true); + expect(out["__proto__"]).toEqual({ polluted: true }); + expect(Object.getPrototypeOf(out)).toBe(Object.prototype); + expect(({} as Record).polluted).toBeUndefined(); + }); + it("reads a legacy pair with no value as an empty string", () => { expect(normalizeStoredMetadata([{ key: "k" }])).toEqual({ k: "" }); }); @@ -1460,18 +1480,19 @@ describe("normalizeStoredMetadata (#1910)", () => { it("migrates a legacy file to the object shape on the next write", () => { // Read side accepts the pair array; write side emits only the object, so // one round-trip is the migration. - const entries = mcpConfigToServerEntries({ + // The legacy on-disk shape no longer matches `MCPConfig` — that is what + // the test is about — so the fixture is untyped and cast once where it + // crosses into the typed parser, the same boundary a real file crosses. + const legacyFile: unknown = { mcpServers: { legacy: { type: "streamable-http", url: "https://x.test/mcp", - metadata: [{ key: "tenant", value: "acme" }] as unknown as Record< - string, - never - >, + metadata: [{ key: "tenant", value: "acme" }], }, }, - }); + }; + const entries = mcpConfigToServerEntries(legacyFile as MCPConfig); expect(entries[0]?.settings?.metadata).toEqual({ tenant: "acme" }); const round = serverEntriesToMcpConfig(entries); expect(round.mcpServers.legacy?.metadata).toEqual({ tenant: "acme" }); diff --git a/core/mcp/inspectorClient.ts b/core/mcp/inspectorClient.ts index c4ad06db..f3febd33 100644 --- a/core/mcp/inspectorClient.ts +++ b/core/mcp/inspectorClient.ts @@ -1037,7 +1037,7 @@ export class InspectorClient extends InspectorClientEventTarget { */ private mergeMeta( callMetadata?: RequestMetadata, - ): Record | undefined { + ): RequestMetadata | undefined { const defaults = this.defaultMetadata; // Modern-era per-request log level (#1629): stamp the opt-in `_meta` key on // every request so the server emits `notifications/message` on this @@ -1132,7 +1132,7 @@ export class InspectorClient extends InspectorClientEventTarget { */ private aggregateListParams( metadata?: RequestMetadata, - ): { _meta: Record } | undefined { + ): { _meta: RequestMetadata } | undefined { const effectiveMeta = this.mergeMeta(metadata); return effectiveMeta ? { _meta: effectiveMeta } : undefined; } @@ -4279,7 +4279,7 @@ export class InspectorClient extends InspectorClientEventTarget { const callParams: { name: string; arguments: Record; - _meta?: Record; + _meta?: RequestMetadata; task?: { ttl: number }; } = { name: tool.name, diff --git a/core/mcp/serverList.ts b/core/mcp/serverList.ts index 5f4b10bc..405bbca2 100644 --- a/core/mcp/serverList.ts +++ b/core/mcp/serverList.ts @@ -234,7 +234,19 @@ export function normalizeStoredMetadata( } const { key, value } = entry as { key: string; value?: JsonValue }; if (key.trim() === "") continue; - out[key] = value ?? ""; + // `??`, not `||`: `null` is a legal `_meta` value and must survive the + // migration as `null`. Only a genuinely absent `value` becomes `""`. + // + // `defineProperty` rather than `out[key] = …` because a legacy key of + // `__proto__` would otherwise hit the prototype setter — silently + // dropping the entry (or, for an object value, mutating the prototype) + // instead of storing an own property under that name. + Object.defineProperty(out, key, { + value: value === undefined ? "" : value, + writable: true, + enumerable: true, + configurable: true, + }); } return out; }