fix(extensions): harden mcp diagnostics output
This commit is contained in:
parent
fd4fd135a1
commit
81c59440bf
8 changed files with 128 additions and 12 deletions
|
|
@ -12,7 +12,7 @@ import type { CliFlavor } from '@shared/types';
|
||||||
import type { InstalledMcpEntry, McpServerDiagnostic } from '@shared/types/extensions';
|
import type { InstalledMcpEntry, McpServerDiagnostic } from '@shared/types/extensions';
|
||||||
|
|
||||||
const MCP_LIST_TIMEOUT_MS = 15_000;
|
const MCP_LIST_TIMEOUT_MS = 15_000;
|
||||||
const MCP_DIAGNOSE_TIMEOUT_MS = 30_000;
|
const MCP_DIAGNOSE_TIMEOUT_MS = 60_000;
|
||||||
|
|
||||||
export interface ExtensionsRuntimeAdapter {
|
export interface ExtensionsRuntimeAdapter {
|
||||||
readonly flavor: CliFlavor;
|
readonly flavor: CliFlavor;
|
||||||
|
|
|
||||||
|
|
@ -3,6 +3,8 @@ import type { McpServerDiagnostic, McpServerHealthStatus } from '@shared/types/e
|
||||||
interface McpDiagnoseJsonEntry {
|
interface McpDiagnoseJsonEntry {
|
||||||
name?: string;
|
name?: string;
|
||||||
target?: string;
|
target?: string;
|
||||||
|
scope?: 'local' | 'user' | 'project' | 'global' | 'dynamic' | 'managed';
|
||||||
|
transport?: string;
|
||||||
status?: 'connected' | 'needs-authentication' | 'failed' | 'timeout';
|
status?: 'connected' | 'needs-authentication' | 'failed' | 'timeout';
|
||||||
statusLabel?: string;
|
statusLabel?: string;
|
||||||
}
|
}
|
||||||
|
|
@ -12,6 +14,10 @@ interface McpDiagnoseJsonPayload {
|
||||||
diagnostics?: McpDiagnoseJsonEntry[];
|
diagnostics?: McpDiagnoseJsonEntry[];
|
||||||
}
|
}
|
||||||
|
|
||||||
|
const EMBEDDED_HTTP_URL_PATTERN = /https?:\/\/[^\s"'`]+/gi;
|
||||||
|
const SENSITIVE_FLAG_VALUE_PATTERN =
|
||||||
|
/(--(?:api[-_]?key|access[-_]?token|auth[-_]?token|token|secret|password|client[-_]?secret))(?:=([^\s]+)|\s+([^\s]+))/gi;
|
||||||
|
|
||||||
function extractJsonObject<T>(raw: string): T {
|
function extractJsonObject<T>(raw: string): T {
|
||||||
const trimmed = raw.trim();
|
const trimmed = raw.trim();
|
||||||
try {
|
try {
|
||||||
|
|
@ -45,6 +51,42 @@ function parseStatusChunk(statusChunk: string): {
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
function redactHttpUrl(urlString: string): string {
|
||||||
|
try {
|
||||||
|
const parsed = new URL(urlString);
|
||||||
|
if (parsed.protocol !== 'http:' && parsed.protocol !== 'https:') {
|
||||||
|
return urlString;
|
||||||
|
}
|
||||||
|
|
||||||
|
if (!parsed.username && !parsed.password && !parsed.search && !parsed.hash) {
|
||||||
|
return urlString;
|
||||||
|
}
|
||||||
|
|
||||||
|
if (parsed.username) parsed.username = '***';
|
||||||
|
if (parsed.password) parsed.password = '***';
|
||||||
|
|
||||||
|
for (const key of new Set(parsed.searchParams.keys())) {
|
||||||
|
parsed.searchParams.set(key, 'REDACTED');
|
||||||
|
}
|
||||||
|
|
||||||
|
if (parsed.hash) {
|
||||||
|
parsed.hash = 'REDACTED';
|
||||||
|
}
|
||||||
|
|
||||||
|
return parsed.toString();
|
||||||
|
} catch {
|
||||||
|
return urlString;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
function redactDiagnosticTarget(target: string): string {
|
||||||
|
return target
|
||||||
|
.replace(EMBEDDED_HTTP_URL_PATTERN, (match) => redactHttpUrl(match))
|
||||||
|
.replace(SENSITIVE_FLAG_VALUE_PATTERN, (_match, flag: string, inlineValue?: string) =>
|
||||||
|
inlineValue ? `${flag}=REDACTED` : `${flag} REDACTED`
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
function parseDiagnosticLine(line: string, checkedAt: number): McpServerDiagnostic | null {
|
function parseDiagnosticLine(line: string, checkedAt: number): McpServerDiagnostic | null {
|
||||||
const statusSeparatorIdx = line.lastIndexOf(' - ');
|
const statusSeparatorIdx = line.lastIndexOf(' - ');
|
||||||
if (statusSeparatorIdx === -1) {
|
if (statusSeparatorIdx === -1) {
|
||||||
|
|
@ -60,7 +102,7 @@ function parseDiagnosticLine(line: string, checkedAt: number): McpServerDiagnost
|
||||||
}
|
}
|
||||||
|
|
||||||
const name = descriptor.slice(0, nameSeparatorIdx).trim();
|
const name = descriptor.slice(0, nameSeparatorIdx).trim();
|
||||||
const target = descriptor.slice(nameSeparatorIdx + 2).trim();
|
const target = redactDiagnosticTarget(descriptor.slice(nameSeparatorIdx + 2).trim());
|
||||||
if (!name || !target) {
|
if (!name || !target) {
|
||||||
return null;
|
return null;
|
||||||
}
|
}
|
||||||
|
|
@ -102,6 +144,7 @@ export function parseMcpDiagnosticsJsonOutput(output: string): McpServerDiagnost
|
||||||
return [];
|
return [];
|
||||||
}
|
}
|
||||||
|
|
||||||
|
const redactedTarget = redactDiagnosticTarget(entry.target);
|
||||||
const normalizedStatus: McpServerHealthStatus =
|
const normalizedStatus: McpServerHealthStatus =
|
||||||
entry.status === 'connected'
|
entry.status === 'connected'
|
||||||
? 'connected'
|
? 'connected'
|
||||||
|
|
@ -111,11 +154,13 @@ export function parseMcpDiagnosticsJsonOutput(output: string): McpServerDiagnost
|
||||||
? 'failed'
|
? 'failed'
|
||||||
: 'unknown';
|
: 'unknown';
|
||||||
|
|
||||||
const rawLine = `${entry.name}: ${entry.target} - ${entry.statusLabel}`;
|
const rawLine = `${entry.name}: ${redactedTarget} - ${entry.statusLabel}`;
|
||||||
return [
|
return [
|
||||||
{
|
{
|
||||||
name: entry.name,
|
name: entry.name,
|
||||||
target: entry.target,
|
target: redactedTarget,
|
||||||
|
scope: entry.scope,
|
||||||
|
transport: entry.transport,
|
||||||
status: normalizedStatus,
|
status: normalizedStatus,
|
||||||
statusLabel: entry.statusLabel,
|
statusLabel: entry.statusLabel,
|
||||||
rawLine,
|
rawLine,
|
||||||
|
|
|
||||||
|
|
@ -17,6 +17,7 @@ import { useStore } from '@renderer/store';
|
||||||
import { formatRelativeTime } from '@renderer/utils/formatters';
|
import { formatRelativeTime } from '@renderer/utils/formatters';
|
||||||
import { CLI_NOT_FOUND_MARKER } from '@shared/constants/cli';
|
import { CLI_NOT_FOUND_MARKER } from '@shared/constants/cli';
|
||||||
import {
|
import {
|
||||||
|
getMcpDiagnosticKey,
|
||||||
getPreferredMcpInstallationEntry,
|
getPreferredMcpInstallationEntry,
|
||||||
sanitizeMcpServerName,
|
sanitizeMcpServerName,
|
||||||
} from '@shared/utils/extensionNormalizers';
|
} from '@shared/utils/extensionNormalizers';
|
||||||
|
|
@ -164,7 +165,12 @@ export const McpServersPanel = ({
|
||||||
|
|
||||||
const getDiagnostic = (server: McpCatalogItem): McpServerDiagnostic | null => {
|
const getDiagnostic = (server: McpCatalogItem): McpServerDiagnostic | null => {
|
||||||
const installedEntry = getInstalledEntry(server);
|
const installedEntry = getInstalledEntry(server);
|
||||||
return installedEntry ? (mcpDiagnostics[installedEntry.name] ?? null) : null;
|
return installedEntry
|
||||||
|
? (mcpDiagnostics[getMcpDiagnosticKey(installedEntry.name, installedEntry.scope)] ??
|
||||||
|
mcpDiagnostics[getMcpDiagnosticKey(installedEntry.name)] ??
|
||||||
|
mcpDiagnostics[installedEntry.name] ??
|
||||||
|
null)
|
||||||
|
: null;
|
||||||
};
|
};
|
||||||
|
|
||||||
const allDiagnostics = useMemo(
|
const allDiagnostics = useMemo(
|
||||||
|
|
@ -250,11 +256,18 @@ export const McpServersPanel = ({
|
||||||
<div className="mcp-diagnostics-list max-h-[18.5rem] space-y-2 overflow-y-auto pr-1">
|
<div className="mcp-diagnostics-list max-h-[18.5rem] space-y-2 overflow-y-auto pr-1">
|
||||||
{allDiagnostics.map((diagnostic) => (
|
{allDiagnostics.map((diagnostic) => (
|
||||||
<div
|
<div
|
||||||
key={diagnostic.name}
|
key={getMcpDiagnosticKey(diagnostic.name, diagnostic.scope)}
|
||||||
className="flex items-start justify-between gap-3 rounded-md border border-black/10 px-3 py-2 dark:border-white/10"
|
className="flex items-start justify-between gap-3 rounded-md border border-black/10 px-3 py-2 dark:border-white/10"
|
||||||
>
|
>
|
||||||
<div className="min-w-0 flex-1">
|
<div className="min-w-0 flex-1">
|
||||||
<p className="text-sm text-text">{diagnostic.name}</p>
|
<div className="flex items-center gap-2">
|
||||||
|
<p className="text-sm text-text">{diagnostic.name}</p>
|
||||||
|
{diagnostic.scope && (
|
||||||
|
<span className="rounded-full border border-border bg-surface-raised px-1.5 py-0.5 text-[10px] uppercase tracking-wide text-text-muted">
|
||||||
|
{diagnostic.scope}
|
||||||
|
</span>
|
||||||
|
)}
|
||||||
|
</div>
|
||||||
<p
|
<p
|
||||||
className="truncate font-mono text-[11px] text-text-muted"
|
className="truncate font-mono text-[11px] text-text-muted"
|
||||||
title={diagnostic.target}
|
title={diagnostic.target}
|
||||||
|
|
@ -269,7 +282,9 @@ export const McpServersPanel = ({
|
||||||
))}
|
))}
|
||||||
</div>
|
</div>
|
||||||
) : (
|
) : (
|
||||||
<p className="text-xs text-text-muted">Waiting for `claude mcp list` results...</p>
|
<p className="text-xs text-text-muted">
|
||||||
|
Waiting for <code>{diagnosticsCommand}</code> results...
|
||||||
|
</p>
|
||||||
)}
|
)}
|
||||||
</div>
|
</div>
|
||||||
)}
|
)}
|
||||||
|
|
|
||||||
|
|
@ -6,7 +6,11 @@
|
||||||
import { api } from '@renderer/api';
|
import { api } from '@renderer/api';
|
||||||
import { CLI_NOT_FOUND_MESSAGE } from '@shared/constants/cli';
|
import { CLI_NOT_FOUND_MESSAGE } from '@shared/constants/cli';
|
||||||
import { isProjectScopedMcpScope } from '@shared/utils/mcpScopes';
|
import { isProjectScopedMcpScope } from '@shared/utils/mcpScopes';
|
||||||
import { getMcpOperationKey, getPluginOperationKey } from '@shared/utils/extensionNormalizers';
|
import {
|
||||||
|
getMcpDiagnosticKey,
|
||||||
|
getMcpOperationKey,
|
||||||
|
getPluginOperationKey,
|
||||||
|
} from '@shared/utils/extensionNormalizers';
|
||||||
|
|
||||||
import { findPaneByTabId, updatePane } from '../utils/paneHelpers';
|
import { findPaneByTabId, updatePane } from '../utils/paneHelpers';
|
||||||
|
|
||||||
|
|
@ -555,7 +559,9 @@ export const createExtensionsSlice: StateCreator<AppState, [], [], ExtensionsSli
|
||||||
const diagnostics = await mcpRegistry.diagnose(projectPath);
|
const diagnostics = await mcpRegistry.diagnose(projectPath);
|
||||||
set({
|
set({
|
||||||
mcpDiagnostics: Object.fromEntries(
|
mcpDiagnostics: Object.fromEntries(
|
||||||
diagnostics.map((entry) => [entry.name, entry] as const)
|
diagnostics.map(
|
||||||
|
(entry) => [getMcpDiagnosticKey(entry.name, entry.scope), entry] as const
|
||||||
|
)
|
||||||
),
|
),
|
||||||
mcpDiagnosticsLoading: false,
|
mcpDiagnosticsLoading: false,
|
||||||
mcpDiagnosticsLastCheckedAt: Date.now(),
|
mcpDiagnosticsLastCheckedAt: Date.now(),
|
||||||
|
|
|
||||||
|
|
@ -92,6 +92,8 @@ export type McpServerHealthStatus = 'connected' | 'needs-authentication' | 'fail
|
||||||
export interface McpServerDiagnostic {
|
export interface McpServerDiagnostic {
|
||||||
name: string;
|
name: string;
|
||||||
target: string;
|
target: string;
|
||||||
|
scope?: 'local' | 'user' | 'project' | 'global' | 'dynamic' | 'managed';
|
||||||
|
transport?: string;
|
||||||
status: McpServerHealthStatus;
|
status: McpServerHealthStatus;
|
||||||
statusLabel: string;
|
statusLabel: string;
|
||||||
rawLine: string;
|
rawLine: string;
|
||||||
|
|
|
||||||
|
|
@ -120,6 +120,14 @@ export function getMcpOperationKey(registryId: string, scope: InstallScope): str
|
||||||
return `mcp:${registryId}:${scope}`;
|
return `mcp:${registryId}:${scope}`;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Namespaced lookup key for MCP diagnostics. Scope is included when available
|
||||||
|
* so the same server name can coexist across global/project/local installs.
|
||||||
|
*/
|
||||||
|
export function getMcpDiagnosticKey(name: string, scope?: string | null): string {
|
||||||
|
return scope ? `mcp-diagnostic:${scope}:${name}` : `mcp-diagnostic:${name}`;
|
||||||
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Check whether a plugin has an installation for the selected scope.
|
* Check whether a plugin has an installation for the selected scope.
|
||||||
*/
|
*/
|
||||||
|
|
|
||||||
|
|
@ -25,7 +25,7 @@ alpic: https://mcp.alpic.ai (HTTP) - ! Needs authentication`);
|
||||||
});
|
});
|
||||||
expect(diagnostics[3]).toMatchObject({
|
expect(diagnostics[3]).toMatchObject({
|
||||||
name: 'tavily-remote-mcp',
|
name: 'tavily-remote-mcp',
|
||||||
target: 'npx -y mcp-remote https://mcp.tavily.com/mcp/?tavilyApiKey=test',
|
target: 'npx -y mcp-remote https://mcp.tavily.com/mcp/?tavilyApiKey=REDACTED',
|
||||||
status: 'failed',
|
status: 'failed',
|
||||||
statusLabel: 'Failed to connect',
|
statusLabel: 'Failed to connect',
|
||||||
});
|
});
|
||||||
|
|
@ -58,7 +58,9 @@ another log line`);
|
||||||
},
|
},
|
||||||
{
|
{
|
||||||
name: 'tavily',
|
name: 'tavily',
|
||||||
target: 'https://mcp.tavily.com/mcp',
|
target: 'https://mcp.tavily.com/mcp?token=secret',
|
||||||
|
scope: 'global',
|
||||||
|
transport: 'http',
|
||||||
status: 'timeout',
|
status: 'timeout',
|
||||||
statusLabel: 'Timed out',
|
statusLabel: 'Timed out',
|
||||||
},
|
},
|
||||||
|
|
@ -74,6 +76,9 @@ another log line`);
|
||||||
}),
|
}),
|
||||||
expect.objectContaining({
|
expect.objectContaining({
|
||||||
name: 'tavily',
|
name: 'tavily',
|
||||||
|
target: 'https://mcp.tavily.com/mcp?token=REDACTED',
|
||||||
|
scope: 'global',
|
||||||
|
transport: 'http',
|
||||||
status: 'failed',
|
status: 'failed',
|
||||||
statusLabel: 'Timed out',
|
statusLabel: 'Timed out',
|
||||||
}),
|
}),
|
||||||
|
|
|
||||||
|
|
@ -55,6 +55,7 @@ vi.mock('../../../src/renderer/api', () => ({
|
||||||
|
|
||||||
import { api } from '../../../src/renderer/api';
|
import { api } from '../../../src/renderer/api';
|
||||||
import {
|
import {
|
||||||
|
getMcpDiagnosticKey,
|
||||||
getMcpOperationKey,
|
getMcpOperationKey,
|
||||||
getPluginOperationKey,
|
getPluginOperationKey,
|
||||||
} from '../../../src/shared/utils/extensionNormalizers';
|
} from '../../../src/shared/utils/extensionNormalizers';
|
||||||
|
|
@ -833,6 +834,40 @@ describe('extensionsSlice', () => {
|
||||||
expect(api.cliInstaller!.getStatus).toHaveBeenCalled();
|
expect(api.cliInstaller!.getStatus).toHaveBeenCalled();
|
||||||
expect(store.getState().apiKeys).toEqual([]);
|
expect(store.getState().apiKeys).toEqual([]);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it('keys MCP diagnostics by scope when the same server exists in multiple scopes', async () => {
|
||||||
|
(api.mcpRegistry!.diagnose as ReturnType<typeof vi.fn>).mockResolvedValue([
|
||||||
|
{
|
||||||
|
name: 'context7',
|
||||||
|
scope: 'global',
|
||||||
|
target: 'npx -y @upstash/context7-mcp',
|
||||||
|
status: 'connected',
|
||||||
|
statusLabel: 'Connected',
|
||||||
|
rawLine: 'context7: npx -y @upstash/context7-mcp - Connected',
|
||||||
|
checkedAt: 1,
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: 'context7',
|
||||||
|
scope: 'project',
|
||||||
|
target: 'uvx context7-project',
|
||||||
|
status: 'failed',
|
||||||
|
statusLabel: 'Failed to connect',
|
||||||
|
rawLine: 'context7: uvx context7-project - Failed to connect',
|
||||||
|
checkedAt: 1,
|
||||||
|
},
|
||||||
|
]);
|
||||||
|
|
||||||
|
await store.getState().runMcpDiagnostics('/tmp/project-a');
|
||||||
|
|
||||||
|
expect(store.getState().mcpDiagnostics).toMatchObject({
|
||||||
|
[getMcpDiagnosticKey('context7', 'global')]: expect.objectContaining({
|
||||||
|
target: 'npx -y @upstash/context7-mcp',
|
||||||
|
}),
|
||||||
|
[getMcpDiagnosticKey('context7', 'project')]: expect.objectContaining({
|
||||||
|
target: 'uvx context7-project',
|
||||||
|
}),
|
||||||
|
});
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
describe('skills state hardening', () => {
|
describe('skills state hardening', () => {
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue