chore: close quality-gate coverage gaps (#1689) (#1766)

Closes #1689. Typecheck cli/tui src, add a core/ format+lint gate, and gate core/json + core/client coverage. See PR #1766 for the full review history.

clients/web/package-lock.json

@@ -69,7 +69,7 @@
         "globals": "^17.4.0",
         "happy-dom": "^20.9.0",
         "playwright": "^1.58.2",
-        "prettier": "^3.8.1",
+        "prettier": "^3.8.4",
         "storybook": "^10.2.19",
         "tsup": "^8.5.1",
         "typescript": "~5.9.3",
@@ -8192,9 +8192,9 @@
       }
     },
     "node_modules/prettier": {
-      "version": "3.8.1",
-      "resolved": "https://registry.npmjs.org/prettier/-/prettier-3.8.1.tgz",
-      "integrity": "sha512-UOnG6LftzbdaHZcKoPFtOcCKztrQ57WkHDeRD9t/PTQtmT0NHSeWWepj6pS0z/N7+08BHFDQVUrfmfMRcZwbMg==",
+      "version": "3.8.4",
+      "resolved": "https://registry.npmjs.org/prettier/-/prettier-3.8.4.tgz",
+      "integrity": "sha512-N2MylSdi48+5N/6S5j+maeHbUSIzzZ5uOcX5Hm4QpV8Dkb1HFjfAKTKX6yNPJQD9AhcT3ifHNB66tWTTJDi11Q==",
       "dev": true,
       "license": "MIT",
       "bin": {

clients/web/src/test/core/client/config.test.ts

@@ -10,16 +10,23 @@ import {
 import {
   CLIENT_KEYCHAIN_ID,
   extractSecretsFromClientConfig,
+  hasClientPlaintextSecret,
   mergeSecretsIntoClientConfig,
 } from "@inspector/core/client/secrets.js";
 import {
+  getClientConfigFilePath,
   loadClientConfig,
   parseClientConfig,
   saveClientConfig,
 } from "@inspector/core/client/config.js";
 import {
+  CIMD_METADATA_URL_HTTPS_ERROR,
+  CIMD_METADATA_URL_INVALID_ERROR,
+  CIMD_METADATA_URL_PATH_ERROR,
   formatClientConfigLoadError,
+  getCimdClientMetadataUrlError,
   isAbsoluteHttpUrl,
+  serializeClientConfig,
 } from "@inspector/core/client/config-parse.js";
 import {
   getActiveCimdClientMetadataUrl,
@@ -210,6 +217,13 @@ describe("client config", () => {
     ).toThrow(/path/);
   });
 
+  it("getClientConfigFilePath honors a custom path and defaults to the storage dir", () => {
+    expect(getClientConfigFilePath("/custom/client.json")).toBe(
+      "/custom/client.json",
+    );
+    expect(getClientConfigFilePath()).toMatch(/client\.json$/);
+  });
+
   it("loadClientConfig returns {} when file is absent", async () => {
     tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), "client-config-"));
     const filePath = path.join(tmpDir, "client.json");
@@ -217,6 +231,76 @@ describe("client config", () => {
     expect(config).toEqual({});
   });
 
+  it("saveClientConfig/loadClientConfig round-trip without a secretStore", async () => {
+    tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), "client-config-"));
+    const filePath = path.join(tmpDir, "client.json");
+    const input = {
+      cimd: {
+        enabled: true,
+        clientMetadataUrl: "https://example.com/oauth/client.json",
+      },
+    };
+    await saveClientConfig(input, { filePath });
+    expect(await loadClientConfig({ filePath })).toEqual(input);
+  });
+
+  it("hasClientPlaintextSecret reflects the presence of an IdP clientSecret", () => {
+    expect(
+      hasClientPlaintextSecret({
+        enterpriseManagedAuth: {
+          idp: { issuer: "https://idp.example.com", clientId: "c" },
+        },
+      }),
+    ).toBe(false);
+    expect(
+      hasClientPlaintextSecret({
+        enterpriseManagedAuth: {
+          idp: {
+            issuer: "https://idp.example.com",
+            clientId: "c",
+            clientSecret: "s",
+          },
+        },
+      }),
+    ).toBe(true);
+  });
+
+  it("getActiveCimdClientMetadataUrl returns undefined for a whitespace-only URL", () => {
+    expect(
+      getActiveCimdClientMetadataUrl({
+        cimd: { enabled: true, clientMetadataUrl: "   " },
+      }),
+    ).toBeUndefined();
+  });
+
+  it("serializeClientConfig emits pretty-printed JSON", () => {
+    expect(serializeClientConfig({ cimd: { clientMetadataUrl: "x" } })).toBe(
+      JSON.stringify({ cimd: { clientMetadataUrl: "x" } }, null, 2),
+    );
+  });
+
+  describe("getCimdClientMetadataUrlError", () => {
+    it("returns undefined for an empty value or a valid https URL with a path", () => {
+      expect(getCimdClientMetadataUrlError("")).toBeUndefined();
+      expect(getCimdClientMetadataUrlError("   ")).toBeUndefined();
+      expect(
+        getCimdClientMetadataUrlError("https://example.com/oauth/client.json"),
+      ).toBeUndefined();
+    });
+
+    it("flags unparseable, non-https, and pathless URLs distinctly", () => {
+      expect(getCimdClientMetadataUrlError("not-a-url")).toBe(
+        CIMD_METADATA_URL_INVALID_ERROR,
+      );
+      expect(
+        getCimdClientMetadataUrlError("http://example.com/oauth/client.json"),
+      ).toBe(CIMD_METADATA_URL_HTTPS_ERROR);
+      expect(getCimdClientMetadataUrlError("https://example.com")).toBe(
+        CIMD_METADATA_URL_PATH_ERROR,
+      );
+    });
+  });
+
   it("formatClientConfigLoadError summarizes Zod validation failures", () => {
     try {
       parseClientConfig({

clients/web/src/test/core/client/node-persistence.test.ts

@@ -0,0 +1,179 @@
+import { describe, it, expect, afterEach } from "vitest";
+import * as fs from "node:fs/promises";
+import { existsSync, readFileSync } from "node:fs";
+import * as os from "node:os";
+import * as path from "node:path";
+import {
+  InMemorySecretStore,
+  KeychainUnavailableError,
+  SECRET_FIELD_IDP_CLIENT_SECRET,
+  type SecretStore,
+} from "@inspector/core/auth/node/secret-store.js";
+import { CLIENT_KEYCHAIN_ID } from "@inspector/core/client/secrets.js";
+import {
+  deleteClientConfigStore,
+  readClientConfigStore,
+  writeClientConfigStore,
+} from "@inspector/core/client/node-persistence.js";
+
+const configWithPlaintextSecret = {
+  enterpriseManagedAuth: {
+    idp: {
+      issuer: "https://idp.example.com",
+      clientId: "cid",
+      clientSecret: "plain",
+    },
+  },
+};
+
+describe("client node-persistence", () => {
+  let tmpDir: string;
+
+  afterEach(async () => {
+    if (tmpDir) {
+      await fs.rm(tmpDir, { recursive: true, force: true });
+    }
+  });
+
+  async function makeTmpFile(contents?: string): Promise<string> {
+    tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), "client-persist-"));
+    const filePath = path.join(tmpDir, "client.json");
+    if (contents !== undefined) {
+      await fs.writeFile(filePath, contents, "utf-8");
+    }
+    return filePath;
+  }
+
+  it("migrates a plaintext secret to an empty keychain and strips it from disk", async () => {
+    const filePath = await makeTmpFile(
+      JSON.stringify(configWithPlaintextSecret),
+    );
+    const secretStore = new InMemorySecretStore();
+
+    const loaded = await readClientConfigStore(filePath, secretStore);
+
+    // Rehydrated result still carries the secret (read back from the keychain).
+    expect(loaded.enterpriseManagedAuth?.idp.clientSecret).toBe("plain");
+    // On-disk copy is stripped.
+    expect(readFileSync(filePath, "utf-8")).not.toContain("plain");
+    // Keychain now holds it.
+    expect(
+      await secretStore.get(CLIENT_KEYCHAIN_ID, SECRET_FIELD_IDP_CLIENT_SECRET),
+    ).toBe("plain");
+  });
+
+  it("does not overwrite an existing keychain secret during migration", async () => {
+    const filePath = await makeTmpFile(
+      JSON.stringify(configWithPlaintextSecret),
+    );
+    const secretStore = new InMemorySecretStore();
+    await secretStore.set(
+      CLIENT_KEYCHAIN_ID,
+      SECRET_FIELD_IDP_CLIENT_SECRET,
+      "existing",
+    );
+
+    const loaded = await readClientConfigStore(filePath, secretStore);
+
+    // The keychain value wins over the disk plaintext.
+    expect(loaded.enterpriseManagedAuth?.idp.clientSecret).toBe("existing");
+    expect(
+      await secretStore.get(CLIENT_KEYCHAIN_ID, SECRET_FIELD_IDP_CLIENT_SECRET),
+    ).toBe("existing");
+    // Disk is still stripped.
+    expect(readFileSync(filePath, "utf-8")).not.toContain("plain");
+  });
+
+  it("keeps the plaintext secret on disk when the keychain is unavailable", async () => {
+    const filePath = await makeTmpFile(
+      JSON.stringify(configWithPlaintextSecret),
+    );
+    // A store whose writes always fail as if libsecret were missing.
+    const unavailable: SecretStore = {
+      async get() {
+        return null;
+      },
+      async set() {
+        throw new KeychainUnavailableError(new Error("no libsecret"));
+      },
+      async delete() {},
+      async deleteAllForServer() {},
+    };
+
+    const loaded = await readClientConfigStore(filePath, unavailable);
+
+    // Migration bailed → original config (with the secret) is returned and the
+    // on-disk copy is left untouched (still contains the plaintext).
+    expect(loaded.enterpriseManagedAuth?.idp.clientSecret).toBe("plain");
+    expect(readFileSync(filePath, "utf-8")).toContain("plain");
+  });
+
+  it("rethrows a non-keychain error raised during migration", async () => {
+    const filePath = await makeTmpFile(
+      JSON.stringify(configWithPlaintextSecret),
+    );
+    const boom: SecretStore = {
+      async get() {
+        return null;
+      },
+      async set() {
+        throw new Error("disk on fire");
+      },
+      async delete() {},
+      async deleteAllForServer() {},
+    };
+
+    await expect(readClientConfigStore(filePath, boom)).rejects.toThrow(
+      /disk on fire/,
+    );
+  });
+
+  it("returns {} when the client.json file is absent", async () => {
+    const filePath = await makeTmpFile();
+    expect(
+      await readClientConfigStore(filePath, new InMemorySecretStore()),
+    ).toEqual({});
+  });
+
+  it("deletes the keychain secret when writing a config without one", async () => {
+    const filePath = await makeTmpFile();
+    const secretStore = new InMemorySecretStore();
+    await secretStore.set(
+      CLIENT_KEYCHAIN_ID,
+      SECRET_FIELD_IDP_CLIENT_SECRET,
+      "stale",
+    );
+
+    await writeClientConfigStore(
+      filePath,
+      {
+        cimd: { enabled: true, clientMetadataUrl: "https://x.example/c.json" },
+      },
+      secretStore,
+    );
+
+    expect(
+      await secretStore.get(CLIENT_KEYCHAIN_ID, SECRET_FIELD_IDP_CLIENT_SECRET),
+    ).toBeNull();
+    expect(readFileSync(filePath, "utf-8")).toContain("clientMetadataUrl");
+  });
+
+  it("deleteClientConfigStore removes both the file and the keychain secret", async () => {
+    const filePath = await makeTmpFile(
+      JSON.stringify({ cimd: { enabled: false, clientMetadataUrl: "" } }),
+    );
+    const secretStore = new InMemorySecretStore();
+    await secretStore.set(
+      CLIENT_KEYCHAIN_ID,
+      SECRET_FIELD_IDP_CLIENT_SECRET,
+      "gone",
+    );
+
+    await deleteClientConfigStore(filePath, secretStore);
+
+    expect(existsSync(filePath)).toBe(false);
+    expect(
+      await secretStore.get(CLIENT_KEYCHAIN_ID, SECRET_FIELD_IDP_CLIENT_SECRET),
+    ).toBeNull();
+  });
+});

clients/web/src/test/core/client/remote.test.ts

@@ -0,0 +1,176 @@
+import { describe, it, expect, vi, afterEach } from "vitest";
+import {
+  loadClientConfigRemote,
+  saveClientConfigRemote,
+} from "@inspector/core/client/remote.js";
+import type { ClientConfig } from "@inspector/core/client/types.js";
+
+afterEach(() => {
+  vi.unstubAllGlobals();
+});
+
+const validConfig: ClientConfig = {
+  enterpriseManagedAuth: {
+    idp: { issuer: "https://idp.example.com", clientId: "cid" },
+  },
+};
+
+/**
+ * Build a minimal `Response`-like stub for the mocked fetch. The code under
+ * test only reads `.ok`, `.status`, and `.json()`, so the stub implements just
+ * those. The double cast is unavoidable here: `Response` has ~20 required
+ * members (`headers`, `body`, `arrayBuffer`, `blob`, `clone`, …), so a direct
+ * `as Response` is a TS2352 error and there is no lighter alternative than
+ * asserting through `unknown` for a deliberately-partial test double.
+ */
+function fakeResponse(init: {
+  ok: boolean;
+  status: number;
+  json?: () => Promise<unknown>;
+}): Response {
+  return {
+    ok: init.ok,
+    status: init.status,
+    json: init.json ?? (async () => ({})),
+  } as unknown as Response;
+}
+
+describe("client remote config", () => {
+  describe("loadClientConfigRemote", () => {
+    it("parses a populated store and strips the trailing slash from baseUrl", async () => {
+      const fetchFn = vi.fn(async () =>
+        fakeResponse({ ok: true, status: 200, json: async () => validConfig }),
+      );
+      const config = await loadClientConfigRemote({
+        baseUrl: "http://localhost:3000/",
+        fetchFn,
+      });
+      expect(config).toEqual(validConfig);
+      expect(fetchFn).toHaveBeenCalledWith(
+        "http://localhost:3000/api/storage/client",
+        expect.objectContaining({ method: "GET" }),
+      );
+    });
+
+    it("sends the bearer auth header when a token is provided", async () => {
+      const fetchFn = vi.fn(async () =>
+        fakeResponse({ ok: true, status: 200, json: async () => ({}) }),
+      );
+      await loadClientConfigRemote({
+        baseUrl: "http://localhost:3000",
+        authToken: "tok",
+        fetchFn,
+      });
+      expect(fetchFn).toHaveBeenCalledWith(
+        "http://localhost:3000/api/storage/client",
+        expect.objectContaining({
+          headers: { "x-mcp-remote-auth": "Bearer tok" },
+        }),
+      );
+    });
+
+    it("returns {} on a 404", async () => {
+      const fetchFn = vi.fn(async () =>
+        fakeResponse({ ok: false, status: 404 }),
+      );
+      expect(
+        await loadClientConfigRemote({ baseUrl: "http://x", fetchFn }),
+      ).toEqual({});
+    });
+
+    it("throws on a non-404 error status", async () => {
+      const fetchFn = vi.fn(async () =>
+        fakeResponse({ ok: false, status: 500 }),
+      );
+      await expect(
+        loadClientConfigRemote({ baseUrl: "http://x", fetchFn }),
+      ).rejects.toThrow(/Failed to read client config: 500/);
+    });
+
+    it("returns {} when the store is empty or not an object", async () => {
+      const emptyObj = vi.fn(async () =>
+        fakeResponse({ ok: true, status: 200, json: async () => ({}) }),
+      );
+      expect(
+        await loadClientConfigRemote({
+          baseUrl: "http://x",
+          fetchFn: emptyObj,
+        }),
+      ).toEqual({});
+
+      const nullJson = vi.fn(async () =>
+        fakeResponse({ ok: true, status: 200, json: async () => null }),
+      );
+      expect(
+        await loadClientConfigRemote({
+          baseUrl: "http://x",
+          fetchFn: nullJson,
+        }),
+      ).toEqual({});
+    });
+  });
+
+  describe("saveClientConfigRemote", () => {
+    it("POSTs the validated, serialized config with content-type and auth headers", async () => {
+      const fetchFn = vi.fn(async () =>
+        fakeResponse({ ok: true, status: 200 }),
+      );
+      await saveClientConfigRemote(validConfig, {
+        baseUrl: "http://localhost:3000/",
+        authToken: "tok",
+        fetchFn,
+      });
+      expect(fetchFn).toHaveBeenCalledWith(
+        "http://localhost:3000/api/storage/client",
+        expect.objectContaining({
+          method: "POST",
+          headers: {
+            "Content-Type": "application/json",
+            "x-mcp-remote-auth": "Bearer tok",
+          },
+          body: JSON.stringify(validConfig, null, 2),
+        }),
+      );
+    });
+
+    it("omits the auth header when no token is given", async () => {
+      const fetchFn = vi.fn(async () =>
+        fakeResponse({ ok: true, status: 200 }),
+      );
+      await saveClientConfigRemote(validConfig, {
+        baseUrl: "http://x",
+        fetchFn,
+      });
+      expect(fetchFn).toHaveBeenCalledWith(
+        "http://x/api/storage/client",
+        expect.objectContaining({
+          headers: { "Content-Type": "application/json" },
+        }),
+      );
+    });
+
+    it("throws when the write fails", async () => {
+      const fetchFn = vi.fn(async () =>
+        fakeResponse({ ok: false, status: 503 }),
+      );
+      await expect(
+        saveClientConfigRemote(validConfig, { baseUrl: "http://x", fetchFn }),
+      ).rejects.toThrow(/Failed to write client config: 503/);
+    });
+  });
+
+  describe("global fetch fallback", () => {
+    it("uses globalThis.fetch when no fetchFn is supplied", async () => {
+      const globalFetch = vi.fn(async () =>
+        fakeResponse({ ok: true, status: 200, json: async () => validConfig }),
+      );
+      vi.stubGlobal("fetch", globalFetch);
+
+      const loaded = await loadClientConfigRemote({ baseUrl: "http://x" });
+      expect(loaded).toEqual(validConfig);
+
+      await saveClientConfigRemote(validConfig, { baseUrl: "http://x" });
+      expect(globalFetch).toHaveBeenCalledTimes(2);
+    });
+  });
+});

clients/web/src/test/core/client/runner.test.ts

@@ -1,7 +1,12 @@
-import { describe, it, expect } from "vitest";
+import { describe, it, expect, afterEach, vi } from "vitest";
+import * as fs from "node:fs/promises";
+import * as os from "node:os";
+import * as path from "node:path";
+import { InMemorySecretStore } from "@inspector/core/auth/node/secret-store.js";
 import {
   buildRunnerClientAuthOptions,
   isOAuthCapableServerConfig,
+  loadRunnerClientConfig,
 } from "@inspector/core/client/runner.js";
 import type { ClientConfig } from "@inspector/core/client/types.js";
 import type { InspectorServerSettings } from "@inspector/core/mcp/types.js";
@@ -80,4 +85,82 @@ describe("runner client auth options", () => {
     expect(opts.oauth?.enterpriseManaged).toBe(true);
     expect(opts.oauth?.clientId).toBe("resource-client");
   });
+
+  it("buildRunnerClientAuthOptions returns no oauth when nothing supplies it", () => {
+    expect(buildRunnerClientAuthOptions({})).toEqual({});
+  });
+
+  it("buildRunnerClientAuthOptions wires CLI client id/secret and marks directAuthRecovery", () => {
+    const opts = buildRunnerClientAuthOptions({}, undefined, {
+      clientId: "cli-id",
+      clientSecret: "cli-secret",
+    });
+    expect(opts.oauth?.clientId).toBe("cli-id");
+    expect(opts.oauth?.clientSecret).toBe("cli-secret");
+    expect(opts.directAuthRecovery).toBe(true);
+  });
+
+  it("buildRunnerClientAuthOptions carries oauth scopes and client secret from server settings", () => {
+    const settings: InspectorServerSettings = {
+      oauthClientSecret: "resource-secret",
+      oauthScopes: "read write",
+      requestTimeout: 0,
+      connectionTimeout: 0,
+      taskTtl: 60000,
+      maxFetchRequests: 10,
+      autoRefreshOnListChanged: false,
+      metadata: [],
+      headers: [],
+      env: [],
+      roots: [],
+    };
+    const opts = buildRunnerClientAuthOptions({}, settings);
+    expect(opts.oauth?.clientSecret).toBe("resource-secret");
+    expect(opts.oauth?.scope).toBe("read write");
+  });
+});
+
+describe("loadRunnerClientConfig", () => {
+  let tmpDir: string;
+
+  afterEach(async () => {
+    if (tmpDir) {
+      await fs.rm(tmpDir, { recursive: true, force: true });
+    }
+    vi.unstubAllEnvs();
+  });
+
+  it("reads client.json from an explicit path (empty when absent)", async () => {
+    tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), "runner-client-"));
+    const filePath = path.join(tmpDir, "client.json");
+    // No injected store here → exercises the default `new KeyringSecretStore()`
+    // path (its get() tolerates keychain unavailability, returning {}).
+    expect(
+      await loadRunnerClientConfig({ clientConfigPath: filePath }),
+    ).toEqual({});
+  });
+
+  it("falls back to MCP_CLIENT_CONFIG_PATH and parses a stored config", async () => {
+    tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), "runner-client-"));
+    const filePath = path.join(tmpDir, "client.json");
+    await fs.writeFile(
+      filePath,
+      JSON.stringify({
+        cimd: {
+          enabled: true,
+          clientMetadataUrl: "https://example.com/oauth/client.json",
+        },
+      }),
+      "utf-8",
+    );
+    vi.stubEnv("MCP_CLIENT_CONFIG_PATH", filePath);
+    // Inject an in-memory store so the result never depends on the developer's
+    // real OS keychain.
+    const config = await loadRunnerClientConfig({
+      secretStore: new InMemorySecretStore(),
+    });
+    expect(config.cimd?.clientMetadataUrl).toBe(
+      "https://example.com/oauth/client.json",
+    );
+  });
 });

clients/web/src/test/core/jsonUtils.test.ts

@@ -3,15 +3,38 @@ import {
   convertParameterValue,
   convertToolParameters,
   convertPromptArguments,
+  toRecord,
 } from "@inspector/core/json/jsonUtils.js";
 import type { Tool } from "@modelcontextprotocol/client";
 
 describe("JSON Utils", () => {
+  describe("toRecord", () => {
+    it("returns the same object widened to a string-keyed record", () => {
+      const source = { a: 1, b: "two" };
+      const widened = toRecord(source);
+      expect(widened).toBe(source);
+      expect(widened.a).toBe(1);
+      expect(Object.keys(widened)).toEqual(["a", "b"]);
+    });
+  });
+
   describe("convertParameterValue", () => {
     it("should convert string to string", () => {
       expect(convertParameterValue("hello", { type: "string" })).toBe("hello");
     });
 
+    it("returns the raw value unchanged when it is empty", () => {
+      // Empty string short-circuits before any type coercion.
+      expect(convertParameterValue("", { type: "number" })).toBe("");
+    });
+
+    it("falls back to the raw string when JSON parsing fails", () => {
+      expect(convertParameterValue("{not json", { type: "object" })).toBe(
+        "{not json",
+      );
+      expect(convertParameterValue("[oops", { type: "array" })).toBe("[oops");
+    });
+
     it("should convert string to number", () => {
       expect(convertParameterValue("42", { type: "number" })).toBe(42);
       expect(convertParameterValue("3.14", { type: "number" })).toBe(3.14);

clients/web/vite.config.ts

@@ -105,7 +105,8 @@ export default defineConfig(({ command }) => {
         'src/lib/**/*.{ts,tsx}',
         'clients/web/server/**/*.{ts,tsx}',
         path.join(repoRoot, 'core/mcp/**/*.{ts,tsx}'),
-        path.join(repoRoot, 'core/json/xMcpHeader.ts'),
+        path.join(repoRoot, 'core/json/**/*.{ts,tsx}'),
+        path.join(repoRoot, 'core/client/**/*.{ts,tsx}'),
         path.join(repoRoot, 'core/react/**/*.{ts,tsx}'),
         path.join(repoRoot, 'core/auth/**/*.{ts,tsx}'),
         path.join(repoRoot, 'core/storage/**/*.{ts,tsx}'),