From d28895b214d26d11cdeddf896a5cadc20326447f Mon Sep 17 00:00:00 2001 From: murashit Date: Fri, 10 Jul 2026 12:10:56 +0900 Subject: [PATCH] Scope app-server diagnostics to their owning panel and thread --- src/app-server/query/cache.ts | 9 +- src/domain/server/diagnostics.ts | 24 ++++ .../connection/server-diagnostics-actions.ts | 30 +++-- .../connection/server-metadata-actions.ts | 20 ++- .../chat/host/bundles/connection-bundle.ts | 7 +- src/features/chat/host/session-graph.ts | 4 +- tests/app-server/query-cache.test.ts | 43 ++++++ .../connection/server-actions.test.ts | 127 +++++++++++++++++- 8 files changed, 237 insertions(+), 27 deletions(-) diff --git a/src/app-server/query/cache.ts b/src/app-server/query/cache.ts index 8873f84d..56c50cbd 100644 --- a/src/app-server/query/cache.ts +++ b/src/app-server/query/cache.ts @@ -1,6 +1,12 @@ import { QueryClient, QueryObserver, type QueryObserverResult } from "@tanstack/query-core"; import type { ModelMetadata } from "../../domain/catalog/metadata"; -import { createServerDiagnostics, diagnosticProbeError, diagnosticProbeOk, diagnosticsWithProbe } from "../../domain/server/diagnostics"; +import { + createServerDiagnostics, + diagnosticProbeError, + diagnosticProbeOk, + diagnosticsWithProbe, + metadataResourceDiagnostics, +} from "../../domain/server/diagnostics"; import type { SharedServerMetadata } from "../../domain/server/metadata"; import type { Thread } from "../../domain/threads/model"; import type { AppServerClient } from "../connection/client"; @@ -390,6 +396,7 @@ function metadataWithLastKnownGood( availablePermissionProfiles: probes.permissionProfiles.status === "ok" ? metadata.availablePermissionProfiles : (previous?.availablePermissionProfiles ?? []), rateLimit: probes.rateLimits.status === "ok" ? metadata.rateLimit : (previous?.rateLimit ?? null), + serverDiagnostics: metadataResourceDiagnostics(metadata.serverDiagnostics), }); } diff --git a/src/domain/server/diagnostics.ts b/src/domain/server/diagnostics.ts index 67849ab6..e69ac29e 100644 --- a/src/domain/server/diagnostics.ts +++ b/src/domain/server/diagnostics.ts @@ -12,6 +12,8 @@ const DIAGNOSTIC_PROBE_DEFINITIONS = { mcpServers: { label: "MCP servers" }, } as const; +const METADATA_RESOURCE_PROBE_IDS = ["models", "skills", "permissionProfiles", "rateLimits"] as const; + export type DiagnosticProbeId = keyof typeof DIAGNOSTIC_PROBE_DEFINITIONS; type DiagnosticProbeStatus = "unknown" | "ok" | "failed"; @@ -64,6 +66,14 @@ export function diagnosticsWithToolInventory(diagnostics: Diagnostics, toolInven }; } +export function metadataResourceDiagnostics(diagnostics: Diagnostics): Diagnostics { + return diagnosticsWithMetadataResourceProbes(createServerDiagnostics(), diagnostics); +} + +export function diagnosticsWithMetadataResourceProbes(diagnostics: Diagnostics, metadataDiagnostics: Diagnostics): Diagnostics { + return METADATA_RESOURCE_PROBE_IDS.reduce((current, id) => diagnosticsWithProbe(current, metadataDiagnostics.probes[id]), diagnostics); +} + function createDiagnosticProbeResult(id: DiagnosticProbeId): DiagnosticProbeResult { return { id, @@ -125,6 +135,20 @@ export function upsertMcpServerStatusDiagnostics(diagnostics: Diagnostics, serve return next; } +export function replaceMcpServerStatusDiagnostics(diagnostics: Diagnostics, servers: readonly McpServerStatusSummary[]): Diagnostics { + return { + ...diagnostics, + mcpServers: servers + .map((server) => + mergeMcpServerDiagnostic( + diagnostics.mcpServers.find((item) => item.name === server.name), + mcpServerDiagnosticFromStatus(server), + ), + ) + .sort((a, b) => a.name.localeCompare(b.name)), + }; +} + export function shortDiagnosticErrorMessage(error: unknown, maxLength = 160): string { const message = error instanceof Error ? error.message : String(error); const compact = message.replace(/\s+/g, " ").trim() || "Codex app-server request failed."; diff --git a/src/features/chat/application/connection/server-diagnostics-actions.ts b/src/features/chat/application/connection/server-diagnostics-actions.ts index df9424fe..db6d5131 100644 --- a/src/features/chat/application/connection/server-diagnostics-actions.ts +++ b/src/features/chat/application/connection/server-diagnostics-actions.ts @@ -1,8 +1,9 @@ import { cloneServerDiagnostics, + diagnosticsWithMetadataResourceProbes, diagnosticsWithProbe, diagnosticsWithToolInventory, - upsertMcpServerStatusDiagnostics, + replaceMcpServerStatusDiagnostics, } from "../../../../domain/server/diagnostics"; import type { SharedServerMetadata } from "../../../../domain/server/metadata"; import type { ChatStateStore } from "../state/store"; @@ -16,18 +17,23 @@ interface RefreshServerDiagnosticsOptions { export interface ServerDiagnosticsActionsHost { stateStore: ChatStateStore; diagnosticsTransport: ServerDiagnosticsTransport; - updateAppServerMetadata: (updater: (metadata: SharedServerMetadata | null) => SharedServerMetadata | null) => SharedServerMetadata | null; appServerMetadataSnapshot: () => SharedServerMetadata | null; } export interface ServerDiagnosticsActions { refreshServerDiagnostics: (options?: RefreshServerDiagnosticsOptions) => Promise; + invalidate(): void; } export function createServerDiagnosticsActions(host: ServerDiagnosticsActionsHost): ServerDiagnosticsActions { + let generation = 0; return { refreshServerDiagnostics: async (options) => { - await refreshServerDiagnostics(host, options); + const currentGeneration = ++generation; + await refreshServerDiagnostics(host, options, () => currentGeneration === generation); + }, + invalidate: () => { + generation += 1; }, }; } @@ -35,8 +41,9 @@ export function createServerDiagnosticsActions(host: ServerDiagnosticsActionsHos async function refreshServerDiagnostics( host: ServerDiagnosticsActionsHost, options: RefreshServerDiagnosticsOptions = {}, + isCurrent: () => boolean, ): Promise { - const initialDiagnostics = currentMetadataDiagnostics(host); + const initialDiagnostics = currentPanelDiagnostics(host); const state = host.stateStore.getState(); const activeThreadId = state.activeThread.id; const metadataSnapshot = host.appServerMetadataSnapshot(); @@ -55,9 +62,9 @@ async function refreshServerDiagnostics( ...(cachedSkillsProbe !== undefined ? { cachedSkillsProbe } : {}), }; const snapshot = await host.diagnosticsTransport.readServerDiagnostics(request); - if (!snapshot) return false; + if (!snapshot || !isCurrent() || host.stateStore.getState().activeThread.id !== activeThreadId) return false; - let diagnostics = currentMetadataDiagnostics(host); + let diagnostics = currentPanelDiagnostics(host); for (const probe of snapshot.resourceProbes) { diagnostics = diagnosticsWithProbe(diagnostics, probe); } @@ -65,16 +72,15 @@ async function refreshServerDiagnostics( diagnostics = diagnosticsWithProbe(diagnostics, probe); } if (snapshot.toolInventory.mcpServerStatuses) { - diagnostics = upsertMcpServerStatusDiagnostics(diagnostics, snapshot.toolInventory.mcpServerStatuses); + diagnostics = replaceMcpServerStatusDiagnostics(diagnostics, snapshot.toolInventory.mcpServerStatuses); } diagnostics = diagnosticsWithToolInventory(diagnostics, snapshot.toolInventory.inventory); - host.updateAppServerMetadata((metadata) => (metadata ? { ...metadata, serverDiagnostics: diagnostics } : null)); host.stateStore.dispatch({ type: "connection/metadata-applied", serverDiagnostics: diagnostics }); return true; } -function currentMetadataDiagnostics(host: ServerDiagnosticsActionsHost): SharedServerMetadata["serverDiagnostics"] { - return ( - host.appServerMetadataSnapshot()?.serverDiagnostics ?? cloneServerDiagnostics(host.stateStore.getState().connection.serverDiagnostics) - ); +function currentPanelDiagnostics(host: ServerDiagnosticsActionsHost): SharedServerMetadata["serverDiagnostics"] { + const current = cloneServerDiagnostics(host.stateStore.getState().connection.serverDiagnostics); + const metadata = host.appServerMetadataSnapshot(); + return metadata ? diagnosticsWithMetadataResourceProbes(current, metadata.serverDiagnostics) : current; } diff --git a/src/features/chat/application/connection/server-metadata-actions.ts b/src/features/chat/application/connection/server-metadata-actions.ts index 7d0c0afc..c0a8a67b 100644 --- a/src/features/chat/application/connection/server-metadata-actions.ts +++ b/src/features/chat/application/connection/server-metadata-actions.ts @@ -1,4 +1,9 @@ -import { cloneServerDiagnostics, diagnosticsWithProbe, upsertMcpServerDiagnostic } from "../../../../domain/server/diagnostics"; +import { + cloneServerDiagnostics, + diagnosticsWithMetadataResourceProbes, + diagnosticsWithProbe, + upsertMcpServerDiagnostic, +} from "../../../../domain/server/diagnostics"; import type { McpServerStartupStatus } from "../../../../domain/server/mcp-status"; import type { SharedServerMetadata } from "../../../../domain/server/metadata"; import type { ChatStateStore } from "../state/store"; @@ -54,6 +59,10 @@ async function applyAppServerResourceEvent(host: ServerMetadataActionsHost, even } function applyAppServerMetadata(host: ServerMetadataActionsHost, metadata: SharedServerMetadata): void { + const serverDiagnostics = diagnosticsWithMetadataResourceProbes( + cloneServerDiagnostics(host.stateStore.getState().connection.serverDiagnostics), + metadata.serverDiagnostics, + ); host.stateStore.dispatch({ type: "connection/metadata-applied", runtimeConfig: metadata.runtimeConfig, @@ -61,7 +70,7 @@ function applyAppServerMetadata(host: ServerMetadataActionsHost, metadata: Share availableSkills: metadata.availableSkills, availablePermissionProfiles: metadata.availablePermissionProfiles, rateLimit: metadata.rateLimit, - serverDiagnostics: metadata.serverDiagnostics, + serverDiagnostics, }); } @@ -164,7 +173,6 @@ function applyMcpStartupStatusEvent( toolCount: null, message, }); - host.updateAppServerMetadata((metadata) => (metadata ? { ...metadata, serverDiagnostics: diagnostics } : null)); host.stateStore.dispatch({ type: "connection/metadata-applied", serverDiagnostics: diagnostics, @@ -172,7 +180,7 @@ function applyMcpStartupStatusEvent( } function currentMetadataDiagnostics(host: ServerMetadataActionsHost): SharedServerMetadata["serverDiagnostics"] { - return ( - host.appServerMetadataSnapshot()?.serverDiagnostics ?? cloneServerDiagnostics(host.stateStore.getState().connection.serverDiagnostics) - ); + const current = cloneServerDiagnostics(host.stateStore.getState().connection.serverDiagnostics); + const metadata = host.appServerMetadataSnapshot(); + return metadata ? diagnosticsWithMetadataResourceProbes(current, metadata.serverDiagnostics) : current; } diff --git a/src/features/chat/host/bundles/connection-bundle.ts b/src/features/chat/host/bundles/connection-bundle.ts index 13710d9c..dfd17305 100644 --- a/src/features/chat/host/bundles/connection-bundle.ts +++ b/src/features/chat/host/bundles/connection-bundle.ts @@ -57,7 +57,7 @@ export interface ChatPanelConnectionBundle { sharedStateActions: { applyAppServerMetadata: (metadata: SharedServerMetadata) => void; }; - clearServerRequestResponders: () => void; + invalidateConnectionScope: () => void; refreshSharedThreads: () => Promise; } @@ -139,7 +139,6 @@ export function createConnectionBundle( const serverDiagnostics = createServerDiagnosticsActions({ stateStore, diagnosticsTransport: appServer.serverDiagnostics, - updateAppServerMetadata: (updater) => environment.plugin.appServerQueries.updateAppServerMetadata(updater), appServerMetadataSnapshot: () => environment.plugin.appServerQueries.appServerMetadataSnapshot(), }); const refreshSharedThreads = async (): Promise => { @@ -212,6 +211,7 @@ export function createConnectionBundle( }, onExit: () => { serverRequestResponders.clear(); + serverDiagnostics.invalidate(); handleChatConnectionExit(connectionExitHost); }, }), @@ -266,8 +266,9 @@ export function createConnectionBundle( serverMetadata.applyAppServerMetadata(metadata); }, }, - clearServerRequestResponders: () => { + invalidateConnectionScope: () => { serverRequestResponders.clear(); + serverDiagnostics.invalidate(); }, refreshSharedThreads, }; diff --git a/src/features/chat/host/session-graph.ts b/src/features/chat/host/session-graph.ts index f30e2fe9..c690b600 100644 --- a/src/features/chat/host/session-graph.ts +++ b/src/features/chat/host/session-graph.ts @@ -191,7 +191,7 @@ export function createChatPanelSessionGraph(host: ChatPanelSessionGraphHost): Ch host.deferredTasks.clearDiagnostics(); }, resetConnection: () => { - connectionBundle.clearServerRequestResponders(); + connectionBundle.invalidateConnectionScope(); connection.resetConnection(); }, setStatus: (statusText, phase) => { @@ -274,7 +274,7 @@ export function createChatPanelSessionGraph(host: ChatPanelSessionGraphHost): Ch refreshSharedThreads, startNewThread: () => threadActions.navigation.startNewThread(), dispose: () => { - connectionBundle.clearServerRequestResponders(); + connectionBundle.invalidateConnectionScope(); shell.dispose(); composerController.dispose(); }, diff --git a/tests/app-server/query-cache.test.ts b/tests/app-server/query-cache.test.ts index c4dfb664..018389da 100644 --- a/tests/app-server/query-cache.test.ts +++ b/tests/app-server/query-cache.test.ts @@ -11,6 +11,8 @@ import { diagnosticProbeError, diagnosticProbeOk, diagnosticsWithProbe, + diagnosticsWithToolInventory, + upsertMcpServerDiagnostic, } from "../../src/domain/server/diagnostics"; import type { SharedServerMetadata } from "../../src/domain/server/metadata"; @@ -130,6 +132,47 @@ describe("AppServerQueryCache", () => { expect(cache.modelsSnapshot(context)).toBeNull(); }); + it("keeps connection and thread diagnostics out of shared metadata snapshots", () => { + const cache = new AppServerQueryCache(); + const context = cacheContext(); + const polluted = metadata(); + polluted.serverDiagnostics = diagnosticsWithToolInventory( + upsertMcpServerDiagnostic(polluted.serverDiagnostics, { + name: "github", + startupStatus: "ready", + authStatus: null, + toolCount: 1, + message: null, + }), + { + checkedAt: 1, + plugins: [], + pluginMarketplaceErrors: [], + pluginsError: null, + mcpServers: [], + mcpDiagnostics: [], + mcpError: null, + skills: [], + skillsError: null, + }, + ); + + cache.writeAppServerMetadata(context, polluted); + + expect(cache.appServerMetadataSnapshot(context)?.serverDiagnostics).toMatchObject({ + mcpServers: [], + toolInventory: null, + probes: { + models: { status: "ok" }, + skills: { status: "ok" }, + permissionProfiles: { status: "ok" }, + rateLimits: { status: "ok" }, + plugins: { status: "unknown" }, + mcpServers: { status: "unknown" }, + }, + }); + }); + it("does not share or store snapshots before the cache context is complete", async () => { const cache = new AppServerQueryCache(); const context = cacheContext({ codexPath: "" }); diff --git a/tests/features/chat/application/connection/server-actions.test.ts b/tests/features/chat/application/connection/server-actions.test.ts index b4f27ffb..fe6f861c 100644 --- a/tests/features/chat/application/connection/server-actions.test.ts +++ b/tests/features/chat/application/connection/server-actions.test.ts @@ -9,6 +9,8 @@ import { diagnosticProbeError, diagnosticProbeOk, diagnosticsWithProbe, + diagnosticsWithToolInventory, + upsertMcpServerDiagnostic, } from "../../../../../src/domain/server/diagnostics"; import type { McpServerStatusSummary } from "../../../../../src/domain/server/mcp-status"; import type { SharedServerMetadata } from "../../../../../src/domain/server/metadata"; @@ -18,6 +20,7 @@ import { createServerDiagnosticsActions } from "../../../../../src/features/chat import { createServerMetadataActions } from "../../../../../src/features/chat/application/connection/server-metadata-actions"; import { createChatStateStore } from "../../../../../src/features/chat/application/state/store"; import { toolInventoryDiagnosticSections } from "../../../../../src/features/chat/presentation/runtime/tool-inventory-diagnostic-sections"; +import { deferred } from "../../../../support/async"; import { chatStateFixture, chatStateWith } from "../../support/state"; describe("server metadata actions", () => { @@ -37,6 +40,60 @@ describe("server metadata actions", () => { expect(stateStore.getState().connection.availableModels.map((model) => model.model)).toEqual(["gpt-5.1"]); }); + it("preserves panel-local tool diagnostics when applying shared metadata", async () => { + const localDiagnostics = diagnosticsWithToolInventory( + upsertMcpServerDiagnostic(createServerDiagnostics(), { + name: "github", + startupStatus: "ready", + authStatus: null, + toolCount: null, + message: null, + }), + toolInventory(), + ); + const stateStore = createChatStateStore(chatStateFixture({ connection: { serverDiagnostics: localDiagnostics } })); + const metadata = serverMetadataFixture({ + serverDiagnostics: diagnosticsWithProbe(createServerDiagnostics(), diagnosticProbeOk("models", "1 model", 2)), + }); + const actions = createServerMetadataActions({ + stateStore, + metadataResourceTransport: metadataResourceTransport(), + ...metadataCacheHost(), + refreshAppServerMetadata: vi.fn().mockResolvedValue(metadata), + isStaleSharedQueryError: () => false, + }); + + await actions.refreshAppServerMetadata(); + + expect(stateStore.getState().connection.serverDiagnostics).toMatchObject({ + probes: { models: { status: "ok", summary: "1 model" } }, + mcpServers: [{ name: "github", startupStatus: "ready" }], + toolInventory: { checkedAt: 1 }, + }); + }); + + it("keeps MCP startup notifications out of shared metadata", async () => { + const stateStore = createChatStateStore(chatStateFixture()); + const cache = { current: serverMetadataFixture() as SharedServerMetadata | null }; + const actions = createServerMetadataActions({ + stateStore, + metadataResourceTransport: metadataResourceTransport(), + ...metadataCacheHost(cache), + refreshAppServerMetadata: vi.fn().mockResolvedValue(null), + isStaleSharedQueryError: () => false, + }); + + await actions.applyAppServerResourceEvent({ + type: "mcp-startup-status-updated", + name: "github", + status: "ready", + message: null, + }); + + expect(stateStore.getState().connection.serverDiagnostics.mcpServers).toMatchObject([{ name: "github", startupStatus: "ready" }]); + expect(cache.current?.serverDiagnostics.mcpServers).toEqual([]); + }); + it("ignores stale shared app-server metadata refreshes without applying state", async () => { const stateStore = createChatStateStore(chatStateFixture()); const stale = new Error("stale"); @@ -244,18 +301,15 @@ describe("server diagnostics actions", () => { it("does not apply diagnostic probes when the transport returns no snapshot", async () => { const stateStore = createChatStateStore(chatStateFixture()); - const updateAppServerMetadata = vi.fn(() => null); const diagnostics = createServerDiagnosticsActions({ stateStore, diagnosticsTransport: { readServerDiagnostics: vi.fn().mockResolvedValue(null) }, appServerMetadataSnapshot: () => null, - updateAppServerMetadata, }); await diagnostics.refreshServerDiagnostics({ appServerMetadataSnapshot: true }); expect(stateStore.getState().connection.serverDiagnostics.probes.mcpServers.status).toBe("unknown"); - expect(updateAppServerMetadata).not.toHaveBeenCalled(); }); it("refreshes tool provider snapshots with cached MCP startup diagnostics", async () => { @@ -296,6 +350,73 @@ describe("server diagnostics actions", () => { expect(sections.map((section) => section.title)).toEqual(["Plugins", "Tool providers", "Skills"]); expect(toolProviderRows.map((row) => `${row.label}: ${row.value}`)).toEqual(["github: MCP server, ready, auth oAuth, 1 tool"]); }); + + it("drops a diagnostics result when its active thread is no longer current", async () => { + const pending = deferred>(); + const stateStore = createChatStateStore(chatStateFixture({ activeThread: { id: "thread-1" } })); + const diagnostics = createServerDiagnosticsActions({ + stateStore, + diagnosticsTransport: { readServerDiagnostics: vi.fn(() => pending.promise) }, + ...metadataCacheHost(), + }); + + const refreshing = diagnostics.refreshServerDiagnostics({ appServerMetadataSnapshot: true }); + stateStore.dispatch({ type: "active-thread/cleared" }); + pending.resolve(serverDiagnosticsSnapshot({ mcpServerStatuses: [mcpServerStatus()] })); + await refreshing; + + expect(stateStore.getState().connection.serverDiagnostics.toolInventory).toBeNull(); + expect(stateStore.getState().connection.serverDiagnostics.mcpServers).toEqual([]); + }); + + it("drops a diagnostics result after its connection scope is invalidated", async () => { + const pending = deferred>(); + const stateStore = createChatStateStore(chatStateFixture({ activeThread: { id: "thread-1" } })); + const diagnostics = createServerDiagnosticsActions({ + stateStore, + diagnosticsTransport: { readServerDiagnostics: vi.fn(() => pending.promise) }, + ...metadataCacheHost(), + }); + + const refreshing = diagnostics.refreshServerDiagnostics({ appServerMetadataSnapshot: true }); + diagnostics.invalidate(); + pending.resolve(serverDiagnosticsSnapshot({ mcpServerStatuses: [mcpServerStatus()] })); + await refreshing; + + expect(stateStore.getState().connection.serverDiagnostics.toolInventory).toBeNull(); + expect(stateStore.getState().connection.serverDiagnostics.mcpServers).toEqual([]); + }); + + it("removes MCP providers missing from the latest thread-scoped inventory", async () => { + let initialDiagnostics = upsertMcpServerDiagnostic(createServerDiagnostics(), { + name: "github", + startupStatus: "ready", + authStatus: null, + toolCount: null, + message: null, + }); + initialDiagnostics = upsertMcpServerDiagnostic(initialDiagnostics, { + name: "removed-provider", + startupStatus: "ready", + authStatus: null, + toolCount: null, + message: null, + }); + const stateStore = createChatStateStore( + chatStateFixture({ activeThread: { id: "thread-1" }, connection: { serverDiagnostics: initialDiagnostics } }), + ); + const diagnostics = createServerDiagnosticsActions({ + stateStore, + diagnosticsTransport: { + readServerDiagnostics: vi.fn().mockResolvedValue(serverDiagnosticsSnapshot({ mcpServerStatuses: [mcpServerStatus()] })), + }, + ...metadataCacheHost(), + }); + + await diagnostics.refreshServerDiagnostics({ appServerMetadataSnapshot: true }); + + expect(stateStore.getState().connection.serverDiagnostics.mcpServers.map((server) => server.name)).toEqual(["github"]); + }); }); function metadataResourceTransport(overrides: Partial = {}): MetadataResourceTransport {