diff --git a/open-sse/mcp-server/__tests__/essentialTools.test.ts b/open-sse/mcp-server/__tests__/essentialTools.test.ts index 88353eed92..1e6fd76c93 100644 --- a/open-sse/mcp-server/__tests__/essentialTools.test.ts +++ b/open-sse/mcp-server/__tests__/essentialTools.test.ts @@ -296,3 +296,108 @@ describe("omniroute_web_search handler (via MCP dispatch)", () => { expect(result.isError).toBe(true); }); }); + +// ── omniroute_get_health: handler dispatch tests ────────────────────────────── +// These tests use InMemoryTransport + Client to exercise the actual registered +// handler (not mockFetch directly), so they catch the real bug the original +// mock-only tests above (lines 39-56) could never catch: process.uptime() +// returns a *number*, and a naive toString() guard silently discards it. + +describe("omniroute_get_health handler (via MCP dispatch)", () => { + let client: Client; + + beforeEach(async () => { + mockFetch.mockReset(); + + const [clientTransport, serverTransport] = InMemoryTransport.createLinkedPair(); + const server = createMcpServer(); + await server.connect(serverTransport); + client = new Client({ name: "test-client", version: "1.0.0" }); + await client.connect(clientTransport); + }); + + afterEach(async () => { + await client.close(); + }); + + function mockHealthSources(opts: { + health?: unknown; + healthError?: Error; + resilience?: unknown; + resilienceError?: Error; + rateLimits?: unknown; + rateLimitsError?: Error; + }) { + mockFetch.mockImplementation(async (url: string) => { + if (url.includes("/api/monitoring/health")) { + if (opts.healthError) throw opts.healthError; + return { ok: true, json: async () => opts.health ?? {} }; + } + if (url.includes("/api/resilience")) { + if (opts.resilienceError) throw opts.resilienceError; + return { ok: true, json: async () => opts.resilience ?? {} }; + } + if (url.includes("/api/rate-limits")) { + if (opts.rateLimitsError) throw opts.rateLimitsError; + return { ok: true, json: async () => opts.rateLimits ?? {} }; + } + throw new Error(`unexpected fetch: ${url}`); + }); + } + + it("should render a real numeric uptime as a string, not fall back to unknown", async () => { + mockHealthSources({ + health: { + uptime: 4731.9817064, + version: "3.8.50", + memoryUsage: { heapUsed: 746337096, heapTotal: 765358080 }, + }, + resilience: { circuitBreakers: [] }, + rateLimits: { limits: [] }, + }); + + const result = await client.callTool({ name: "omniroute_get_health", arguments: {} }); + + expect(result.isError).toBeFalsy(); + const content = result.content as Array<{ type: string; text: string }>; + const data = JSON.parse(content[0].text); + expect(data.uptime).toBe("4731.9817064"); + expect(data.version).toBe("3.8.50"); + }); + + it("should surface a degraded entry when one source fetch fails, instead of silently faking success", async () => { + mockHealthSources({ + health: { uptime: 100, version: "3.8.50" }, + resilience: { circuitBreakers: [] }, + rateLimitsError: new Error("connect ECONNREFUSED 127.0.0.1:20128"), + }); + + const result = await client.callTool({ name: "omniroute_get_health", arguments: {} }); + + expect(result.isError).toBeFalsy(); + const content = result.content as Array<{ type: string; text: string }>; + const data = JSON.parse(content[0].text); + // The two healthy sources still come through untouched. + expect(data.uptime).toBe("100"); + expect(data.rateLimits).toEqual([]); + // But the failure is visible instead of being indistinguishable from "no rate limits". + expect(Array.isArray(data.degraded)).toBe(true); + expect(data.degraded).toHaveLength(1); + expect(data.degraded[0].source).toBe("rateLimits"); + expect(data.degraded[0].error).toContain("ECONNREFUSED"); + }); + + it("should omit degraded entirely when every source succeeds", async () => { + mockHealthSources({ + health: { uptime: 1, version: "3.8.50" }, + resilience: { circuitBreakers: [] }, + rateLimits: { limits: [] }, + }); + + const result = await client.callTool({ name: "omniroute_get_health", arguments: {} }); + + const content = result.content as Array<{ type: string; text: string }>; + const data = JSON.parse(content[0].text); + expect(data.degraded).toBeUndefined(); + }); +}); diff --git a/open-sse/mcp-server/schemas/tools.ts b/open-sse/mcp-server/schemas/tools.ts index 4db6a850dc..07b0b8db77 100644 --- a/open-sse/mcp-server/schemas/tools.ts +++ b/open-sse/mcp-server/schemas/tools.ts @@ -68,12 +68,20 @@ export const getHealthOutput = z.object({ provider: z.string(), }) .optional(), + degraded: z + .array( + z.object({ + source: z.enum(["health", "resilience", "rateLimits"]), + error: z.string(), + }) + ) + .optional(), }); export const getHealthTool: McpToolDefinition = { name: "omniroute_get_health", description: - "Returns the current health status of OmniRoute including uptime, memory usage, circuit breaker states for all providers, rate limit status, and cache statistics.", + "Returns the current health status of OmniRoute including uptime, memory usage, circuit breaker states for all providers, rate limit status, and cache statistics. If an underlying source (health/resilience/rate-limits) could not be reached, it is listed in `degraded` instead of being silently reported as empty/zero.", inputSchema: getHealthInput, outputSchema: getHealthOutput, scopes: ["read:health"], diff --git a/open-sse/mcp-server/server.ts b/open-sse/mcp-server/server.ts index d61b654f3c..48366ce405 100644 --- a/open-sse/mcp-server/server.ts +++ b/open-sse/mcp-server/server.ts @@ -270,6 +270,15 @@ function withScopeEnforcement( }; } +// process.uptime() (the source of health.uptime) returns a number, not a string; +// the shared toString() helper only passes through actual strings, so a naive +// toString(health.uptime, "unknown") silently discarded every real uptime value. +function toUptimeString(value: unknown): string { + if (typeof value === "string") return value; + if (typeof value === "number" && Number.isFinite(value)) return String(value); + return "unknown"; +} + async function handleGetHealth() { const start = Date.now(); try { @@ -287,8 +296,25 @@ async function handleGetHealth() { const resilienceCircuitBreakers = toArray(resilience.circuitBreakers); const rateLimitEntries = toArray(rateLimits.limits); + // Surface fetch failures instead of letting Promise.allSettled's {} fallback + // masquerade as genuine zero/empty data (indistinguishable "no data" vs. + // "couldn't reach the source" was the actual root confusion this fixes). + const degradedSources: Array<{ source: string; settled: PromiseSettledResult }> = [ + { source: "health", settled: healthRaw }, + { source: "resilience", settled: resilienceRaw }, + { source: "rateLimits", settled: rateLimitsRaw }, + ]; + const degraded = degradedSources + .filter(({ settled }) => settled.status === "rejected") + .map(({ source, settled }) => ({ + source, + error: sanitizeErrorMessage( + settled.status === "rejected" ? (settled as PromiseRejectedResult).reason : undefined + ), + })); + const result = { - uptime: toString(health.uptime, "unknown"), + uptime: toUptimeString(health.uptime), version: toString(health.version, "unknown"), memoryUsage: { heapUsed: toNumber(memoryUsageRaw.heapUsed, 0), @@ -310,6 +336,7 @@ async function handleGetHealth() { provider: toString(toRecord(health.cryptography).provider, "unknown"), } : undefined, + degraded: degraded.length > 0 ? degraded : undefined, }; await logToolCall("omniroute_get_health", {}, result, Date.now() - start, true);