feat(skill-mcp-manager): improve connection handling and client implementations
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
This commit is contained in:
@@ -1,6 +1,10 @@
|
|||||||
import { afterAll, afterEach, beforeEach, describe, expect, it, mock, test } from "bun:test"
|
import { afterAll, afterEach, beforeEach, describe, expect, it, mock, test } from "bun:test"
|
||||||
|
import type { Transport } from "@modelcontextprotocol/sdk/shared/transport.js"
|
||||||
|
import type { StdioServerParameters } from "@modelcontextprotocol/sdk/client/stdio.js"
|
||||||
import type { ClaudeCodeMcpServer } from "../claude-code-mcp-loader/types"
|
import type { ClaudeCodeMcpServer } from "../claude-code-mcp-loader/types"
|
||||||
import type { SkillMcpClientInfo, SkillMcpManagerState } from "./types"
|
import type { SkillMcpClientInfo, SkillMcpManagerState } from "./types"
|
||||||
|
import { setHttpClientDependenciesForTesting } from "./http-client"
|
||||||
|
import { setStdioClientDependenciesForTesting } from "./stdio-client"
|
||||||
|
|
||||||
const trackedStates: SkillMcpManagerState[] = []
|
const trackedStates: SkillMcpManagerState[] = []
|
||||||
const createdStdioTransports: MockStdioClientTransport[] = []
|
const createdStdioTransports: MockStdioClientTransport[] = []
|
||||||
@@ -8,33 +12,62 @@ const createdHttpTransports: MockStreamableHTTPClientTransport[] = []
|
|||||||
|
|
||||||
class MockClient {
|
class MockClient {
|
||||||
readonly close = mock(async () => {})
|
readonly close = mock(async () => {})
|
||||||
|
readonly listTools = mock(async () => ({ tools: [] }))
|
||||||
|
readonly listResources = mock(async () => ({ resources: [] }))
|
||||||
|
readonly listPrompts = mock(async () => ({ prompts: [] }))
|
||||||
|
readonly callTool = mock(async () => ({ content: [] }))
|
||||||
|
readonly readResource = mock(async () => ({ contents: [] }))
|
||||||
|
readonly getPrompt = mock(async () => ({ messages: [] }))
|
||||||
|
|
||||||
constructor(
|
constructor(
|
||||||
_clientInfo: { name: string; version: string },
|
_clientInfo: { name: string; version: string },
|
||||||
_options: { capabilities: Record<string, never> }
|
_options: { capabilities: Record<string, never> }
|
||||||
) {}
|
) {}
|
||||||
|
|
||||||
async connect(_transport: unknown): Promise<void> {
|
async connect(_transport: Transport): Promise<void> {
|
||||||
// Successful connect, env-related assertions happen on transport constructor args
|
// Successful connect, env-related assertions happen on transport constructor args
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
class MockStdioClientTransport {
|
class MockStdioClientTransport {
|
||||||
readonly close = mock(async () => {})
|
readonly close = mock(async () => {})
|
||||||
readonly options: { command: string; args?: string[]; env?: Record<string, string>; stderr?: string }
|
readonly start = mock(async () => {})
|
||||||
|
readonly send = mock(async () => {})
|
||||||
|
readonly options: StdioServerParameters
|
||||||
|
|
||||||
constructor(options: { command: string; args?: string[]; env?: Record<string, string>; stderr?: string }) {
|
constructor(options: StdioServerParameters) {
|
||||||
this.options = options
|
this.options = options
|
||||||
createdStdioTransports.push(this)
|
createdStdioTransports.push(this)
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
interface MockHttpTransportOptions {
|
interface MockHttpTransportOptions {
|
||||||
requestInit?: { headers?: Record<string, string> }
|
requestInit?: RequestInit
|
||||||
|
}
|
||||||
|
|
||||||
|
function getHeaderValue(
|
||||||
|
headers: HeadersInit | undefined,
|
||||||
|
name: string,
|
||||||
|
): string | undefined {
|
||||||
|
if (!headers) {
|
||||||
|
return undefined
|
||||||
|
}
|
||||||
|
|
||||||
|
if (headers instanceof Headers) {
|
||||||
|
return headers.get(name) ?? undefined
|
||||||
|
}
|
||||||
|
|
||||||
|
if (Array.isArray(headers)) {
|
||||||
|
const entry = headers.find(([headerName]) => headerName.toLowerCase() === name.toLowerCase())
|
||||||
|
return entry?.[1]
|
||||||
|
}
|
||||||
|
|
||||||
|
return headers[name]
|
||||||
}
|
}
|
||||||
|
|
||||||
class MockStreamableHTTPClientTransport {
|
class MockStreamableHTTPClientTransport {
|
||||||
readonly close = mock(async () => {})
|
readonly close = mock(async () => {})
|
||||||
|
readonly send = mock(async () => {})
|
||||||
readonly url: URL
|
readonly url: URL
|
||||||
readonly options?: MockHttpTransportOptions
|
readonly options?: MockHttpTransportOptions
|
||||||
|
|
||||||
@@ -47,18 +80,6 @@ class MockStreamableHTTPClientTransport {
|
|||||||
async start() {}
|
async start() {}
|
||||||
}
|
}
|
||||||
|
|
||||||
mock.module("@modelcontextprotocol/sdk/client/index.js", () => ({
|
|
||||||
Client: MockClient,
|
|
||||||
}))
|
|
||||||
|
|
||||||
mock.module("@modelcontextprotocol/sdk/client/stdio.js", () => ({
|
|
||||||
StdioClientTransport: MockStdioClientTransport,
|
|
||||||
}))
|
|
||||||
|
|
||||||
mock.module("@modelcontextprotocol/sdk/client/streamableHttp.js", () => ({
|
|
||||||
StreamableHTTPClientTransport: MockStreamableHTTPClientTransport,
|
|
||||||
}))
|
|
||||||
|
|
||||||
afterAll(() => {
|
afterAll(() => {
|
||||||
mock.restore()
|
mock.restore()
|
||||||
})
|
})
|
||||||
@@ -110,6 +131,14 @@ const ORIGINAL_ENV = { ...process.env }
|
|||||||
beforeEach(() => {
|
beforeEach(() => {
|
||||||
createdStdioTransports.length = 0
|
createdStdioTransports.length = 0
|
||||||
createdHttpTransports.length = 0
|
createdHttpTransports.length = 0
|
||||||
|
setStdioClientDependenciesForTesting({
|
||||||
|
createClient: (clientInfo, options) => new MockClient(clientInfo, options),
|
||||||
|
createTransport: (options) => new MockStdioClientTransport(options),
|
||||||
|
})
|
||||||
|
setHttpClientDependenciesForTesting({
|
||||||
|
createClient: (clientInfo, options) => new MockClient(clientInfo, options),
|
||||||
|
createTransport: (url, options) => new MockStreamableHTTPClientTransport(url, options),
|
||||||
|
})
|
||||||
})
|
})
|
||||||
|
|
||||||
afterEach(async () => {
|
afterEach(async () => {
|
||||||
@@ -126,6 +155,9 @@ afterEach(async () => {
|
|||||||
for (const [key, value] of Object.entries(ORIGINAL_ENV)) {
|
for (const [key, value] of Object.entries(ORIGINAL_ENV)) {
|
||||||
process.env[key] = value
|
process.env[key] = value
|
||||||
}
|
}
|
||||||
|
|
||||||
|
setStdioClientDependenciesForTesting()
|
||||||
|
setHttpClientDependenciesForTesting()
|
||||||
})
|
})
|
||||||
|
|
||||||
describe("getOrCreateClient env var expansion", () => {
|
describe("getOrCreateClient env var expansion", () => {
|
||||||
@@ -265,11 +297,11 @@ describe("getOrCreateClient env var expansion", () => {
|
|||||||
// when
|
// when
|
||||||
await getOrCreateClient({ state, clientKey, info, config })
|
await getOrCreateClient({ state, clientKey, info, config })
|
||||||
|
|
||||||
// then
|
// then
|
||||||
expect(createdHttpTransports).toHaveLength(1)
|
expect(createdHttpTransports).toHaveLength(1)
|
||||||
expect(createdHttpTransports[0]?.options?.requestInit?.headers?.Authorization).toBe(
|
expect(getHeaderValue(createdHttpTransports[0]?.options?.requestInit?.headers, "Authorization")).toBe(
|
||||||
"Bearer xoxp-http-secret"
|
"Bearer xoxp-http-secret"
|
||||||
)
|
)
|
||||||
})
|
})
|
||||||
})
|
})
|
||||||
})
|
})
|
||||||
|
|||||||
@@ -1,5 +1,8 @@
|
|||||||
import { afterEach, beforeEach, describe, expect, it, mock, afterAll } from "bun:test"
|
import { afterEach, beforeEach, describe, expect, it, mock, afterAll } from "bun:test"
|
||||||
|
import type { StdioServerParameters } from "@modelcontextprotocol/sdk/client/stdio.js"
|
||||||
|
import type { Transport } from "@modelcontextprotocol/sdk/shared/transport.js"
|
||||||
import type { ClaudeCodeMcpServer } from "../claude-code-mcp-loader/types"
|
import type { ClaudeCodeMcpServer } from "../claude-code-mcp-loader/types"
|
||||||
|
import { setStdioClientDependenciesForTesting } from "./stdio-client"
|
||||||
import type { SkillMcpClientInfo, SkillMcpManagerState } from "./types"
|
import type { SkillMcpClientInfo, SkillMcpManagerState } from "./types"
|
||||||
|
|
||||||
type Deferred<TValue> = {
|
type Deferred<TValue> = {
|
||||||
@@ -23,7 +26,14 @@ class MockClient {
|
|||||||
createdClients.push(this)
|
createdClients.push(this)
|
||||||
}
|
}
|
||||||
|
|
||||||
async connect(_transport: MockStdioClientTransport): Promise<void> {
|
readonly listTools = mock(async () => ({ tools: [] }))
|
||||||
|
readonly listResources = mock(async () => ({ resources: [] }))
|
||||||
|
readonly listPrompts = mock(async () => ({ prompts: [] }))
|
||||||
|
readonly callTool = mock(async () => ({ content: [] }))
|
||||||
|
readonly readResource = mock(async () => ({ contents: [] }))
|
||||||
|
readonly getPrompt = mock(async () => ({ messages: [] }))
|
||||||
|
|
||||||
|
async connect(_transport: Transport): Promise<void> {
|
||||||
const pendingConnect = pendingConnects.shift()
|
const pendingConnect = pendingConnects.shift()
|
||||||
if (pendingConnect) {
|
if (pendingConnect) {
|
||||||
await pendingConnect.promise
|
await pendingConnect.promise
|
||||||
@@ -33,20 +43,14 @@ class MockClient {
|
|||||||
|
|
||||||
class MockStdioClientTransport {
|
class MockStdioClientTransport {
|
||||||
readonly close = mock(async () => {})
|
readonly close = mock(async () => {})
|
||||||
|
readonly start = mock(async () => {})
|
||||||
|
readonly send = mock(async () => {})
|
||||||
|
|
||||||
constructor(_options: { command: string; args?: string[]; env?: Record<string, string>; stderr?: string }) {
|
constructor(_options: StdioServerParameters) {
|
||||||
createdTransports.push(this)
|
createdTransports.push(this)
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
mock.module("@modelcontextprotocol/sdk/client/index.js", () => ({
|
|
||||||
Client: MockClient,
|
|
||||||
}))
|
|
||||||
|
|
||||||
mock.module("@modelcontextprotocol/sdk/client/stdio.js", () => ({
|
|
||||||
StdioClientTransport: MockStdioClientTransport,
|
|
||||||
}))
|
|
||||||
|
|
||||||
afterAll(() => { mock.restore() })
|
afterAll(() => { mock.restore() })
|
||||||
|
|
||||||
const { disconnectAll, disconnectSession } = await import("./cleanup")
|
const { disconnectAll, disconnectSession } = await import("./cleanup")
|
||||||
@@ -84,6 +88,11 @@ function createState(): SkillMcpManagerState {
|
|||||||
shutdownGeneration: 0,
|
shutdownGeneration: 0,
|
||||||
inFlightConnections: new Map(),
|
inFlightConnections: new Map(),
|
||||||
disposed: false,
|
disposed: false,
|
||||||
|
createOAuthProvider: () => ({
|
||||||
|
tokens: () => null,
|
||||||
|
login: async () => ({ accessToken: "test-token" }),
|
||||||
|
refresh: async () => ({ accessToken: "test-token" }),
|
||||||
|
}),
|
||||||
}
|
}
|
||||||
|
|
||||||
trackedStates.push(state)
|
trackedStates.push(state)
|
||||||
@@ -111,6 +120,10 @@ beforeEach(() => {
|
|||||||
pendingConnects.length = 0
|
pendingConnects.length = 0
|
||||||
createdClients.length = 0
|
createdClients.length = 0
|
||||||
createdTransports.length = 0
|
createdTransports.length = 0
|
||||||
|
setStdioClientDependenciesForTesting({
|
||||||
|
createClient: (clientInfo, options) => new MockClient(clientInfo, options),
|
||||||
|
createTransport: (options) => new MockStdioClientTransport(options),
|
||||||
|
})
|
||||||
})
|
})
|
||||||
|
|
||||||
afterEach(async () => {
|
afterEach(async () => {
|
||||||
@@ -122,6 +135,7 @@ afterEach(async () => {
|
|||||||
pendingConnects.length = 0
|
pendingConnects.length = 0
|
||||||
createdClients.length = 0
|
createdClients.length = 0
|
||||||
createdTransports.length = 0
|
createdTransports.length = 0
|
||||||
|
setStdioClientDependenciesForTesting()
|
||||||
})
|
})
|
||||||
|
|
||||||
describe("getOrCreateClient disconnect race", () => {
|
describe("getOrCreateClient disconnect race", () => {
|
||||||
|
|||||||
@@ -1,13 +1,12 @@
|
|||||||
import type { Client } from "@modelcontextprotocol/sdk/client/index.js"
|
|
||||||
import type { ClaudeCodeMcpServer } from "../claude-code-mcp-loader/types"
|
import type { ClaudeCodeMcpServer } from "../claude-code-mcp-loader/types"
|
||||||
import { expandEnvVarsInObject } from "../claude-code-mcp-loader/env-expander"
|
import { expandEnvVarsInObject } from "../claude-code-mcp-loader/env-expander"
|
||||||
import { forceReconnect } from "./cleanup"
|
import { forceReconnect } from "./cleanup"
|
||||||
import { getConnectionType } from "./connection-type"
|
import { getConnectionType } from "./connection-type"
|
||||||
import { createHttpClient } from "./http-client"
|
import { createHttpClient } from "./http-client"
|
||||||
import { createStdioClient } from "./stdio-client"
|
import { createStdioClient } from "./stdio-client"
|
||||||
import type { SkillMcpClientConnectionParams, SkillMcpClientInfo, SkillMcpManagerState } from "./types"
|
import type { McpClient, SkillMcpClientConnectionParams, SkillMcpClientInfo, SkillMcpManagerState } from "./types"
|
||||||
|
|
||||||
function removeClientIfCurrent(state: SkillMcpManagerState, clientKey: string, client: Client): void {
|
function removeClientIfCurrent(state: SkillMcpManagerState, clientKey: string, client: McpClient): void {
|
||||||
const managed = state.clients.get(clientKey)
|
const managed = state.clients.get(clientKey)
|
||||||
if (managed?.client === client) {
|
if (managed?.client === client) {
|
||||||
state.clients.delete(clientKey)
|
state.clients.delete(clientKey)
|
||||||
@@ -21,7 +20,7 @@ export async function getOrCreateClient(params: {
|
|||||||
clientKey: string
|
clientKey: string
|
||||||
info: SkillMcpClientInfo
|
info: SkillMcpClientInfo
|
||||||
config: ClaudeCodeMcpServer
|
config: ClaudeCodeMcpServer
|
||||||
}): Promise<Client> {
|
}): Promise<McpClient> {
|
||||||
const { state, clientKey, info, config } = params
|
const { state, clientKey, info, config } = params
|
||||||
|
|
||||||
if (state.disposed) {
|
if (state.disposed) {
|
||||||
@@ -42,7 +41,7 @@ export async function getOrCreateClient(params: {
|
|||||||
|
|
||||||
const isTrusted = !PROJECT_SCOPES.has(info.scope ?? "")
|
const isTrusted = !PROJECT_SCOPES.has(info.scope ?? "")
|
||||||
const expandedConfig = expandEnvVarsInObject(config, { trusted: isTrusted })
|
const expandedConfig = expandEnvVarsInObject(config, { trusted: isTrusted })
|
||||||
let currentConnectionPromise!: Promise<Client>
|
let currentConnectionPromise!: Promise<McpClient>
|
||||||
state.inFlightConnections.set(info.sessionID, (state.inFlightConnections.get(info.sessionID) ?? 0) + 1)
|
state.inFlightConnections.set(info.sessionID, (state.inFlightConnections.get(info.sessionID) ?? 0) + 1)
|
||||||
currentConnectionPromise = (async () => {
|
currentConnectionPromise = (async () => {
|
||||||
const disconnectGenAtStart = state.disconnectedSessions.get(info.sessionID) ?? 0
|
const disconnectGenAtStart = state.disconnectedSessions.get(info.sessionID) ?? 0
|
||||||
@@ -96,7 +95,7 @@ export async function getOrCreateClientWithRetryImpl(params: {
|
|||||||
clientKey: string
|
clientKey: string
|
||||||
info: SkillMcpClientInfo
|
info: SkillMcpClientInfo
|
||||||
config: ClaudeCodeMcpServer
|
config: ClaudeCodeMcpServer
|
||||||
}): Promise<Client> {
|
}): Promise<McpClient> {
|
||||||
const { state, clientKey } = params
|
const { state, clientKey } = params
|
||||||
|
|
||||||
try {
|
try {
|
||||||
@@ -115,7 +114,7 @@ async function createClient(params: {
|
|||||||
clientKey: string
|
clientKey: string
|
||||||
info: SkillMcpClientInfo
|
info: SkillMcpClientInfo
|
||||||
config: ClaudeCodeMcpServer
|
config: ClaudeCodeMcpServer
|
||||||
}): Promise<Client> {
|
}): Promise<McpClient> {
|
||||||
const { info, config } = params
|
const { info, config } = params
|
||||||
const connectionType = getConnectionType(config)
|
const connectionType = getConnectionType(config)
|
||||||
|
|
||||||
|
|||||||
@@ -2,7 +2,40 @@ import { Client } from "@modelcontextprotocol/sdk/client/index.js"
|
|||||||
import { StreamableHTTPClientTransport } from "@modelcontextprotocol/sdk/client/streamableHttp.js"
|
import { StreamableHTTPClientTransport } from "@modelcontextprotocol/sdk/client/streamableHttp.js"
|
||||||
import { registerProcessCleanup, startCleanupTimer } from "./cleanup"
|
import { registerProcessCleanup, startCleanupTimer } from "./cleanup"
|
||||||
import { buildHttpRequestInit } from "./oauth-handler"
|
import { buildHttpRequestInit } from "./oauth-handler"
|
||||||
import type { ManagedClient, SkillMcpClientConnectionParams } from "./types"
|
import type { ManagedClient, McpClient, McpTransport, SkillMcpClientConnectionParams } from "./types"
|
||||||
|
|
||||||
|
type HttpClientFactory = (
|
||||||
|
clientInfo: { name: string; version: string },
|
||||||
|
options: { capabilities: Record<string, never> }
|
||||||
|
) => McpClient
|
||||||
|
|
||||||
|
type HttpTransportFactory = (
|
||||||
|
url: URL,
|
||||||
|
options?: { requestInit?: RequestInit }
|
||||||
|
) => McpTransport
|
||||||
|
|
||||||
|
interface HttpClientDependencies {
|
||||||
|
createClient: HttpClientFactory
|
||||||
|
createTransport: HttpTransportFactory
|
||||||
|
}
|
||||||
|
|
||||||
|
const defaultHttpClientDependencies: HttpClientDependencies = {
|
||||||
|
createClient: (clientInfo, options) => new Client(clientInfo, options),
|
||||||
|
createTransport: (url, options) => new StreamableHTTPClientTransport(url, options),
|
||||||
|
}
|
||||||
|
|
||||||
|
let httpClientDependencies: HttpClientDependencies = defaultHttpClientDependencies
|
||||||
|
|
||||||
|
export function setHttpClientDependenciesForTesting(
|
||||||
|
dependencies?: Partial<HttpClientDependencies>
|
||||||
|
): void {
|
||||||
|
httpClientDependencies = dependencies
|
||||||
|
? {
|
||||||
|
...defaultHttpClientDependencies,
|
||||||
|
...dependencies,
|
||||||
|
}
|
||||||
|
: defaultHttpClientDependencies
|
||||||
|
}
|
||||||
|
|
||||||
function redactUrl(urlStr: string): string {
|
function redactUrl(urlStr: string): string {
|
||||||
try {
|
try {
|
||||||
@@ -22,7 +55,7 @@ function redactUrl(urlStr: string): string {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
export async function createHttpClient(params: SkillMcpClientConnectionParams): Promise<Client> {
|
export async function createHttpClient(params: SkillMcpClientConnectionParams): Promise<McpClient> {
|
||||||
const { state, clientKey, info, config } = params
|
const { state, clientKey, info, config } = params
|
||||||
const shutdownGenAtStart = state.shutdownGeneration
|
const shutdownGenAtStart = state.shutdownGeneration
|
||||||
|
|
||||||
@@ -43,11 +76,11 @@ export async function createHttpClient(params: SkillMcpClientConnectionParams):
|
|||||||
registerProcessCleanup(state)
|
registerProcessCleanup(state)
|
||||||
|
|
||||||
const requestInit = await buildHttpRequestInit(config, state.authProviders, state.createOAuthProvider)
|
const requestInit = await buildHttpRequestInit(config, state.authProviders, state.createOAuthProvider)
|
||||||
const transport = new StreamableHTTPClientTransport(url, {
|
const transport: McpTransport = httpClientDependencies.createTransport(url, {
|
||||||
requestInit,
|
requestInit,
|
||||||
})
|
})
|
||||||
|
|
||||||
const client = new Client(
|
const client: McpClient = httpClientDependencies.createClient(
|
||||||
{ name: `skill-mcp-${info.skillName}-${info.serverName}`, version: "1.0.0" },
|
{ name: `skill-mcp-${info.skillName}-${info.serverName}`, version: "1.0.0" },
|
||||||
{ capabilities: {} }
|
{ capabilities: {} }
|
||||||
)
|
)
|
||||||
|
|||||||
@@ -1,44 +1,84 @@
|
|||||||
import { describe, it, expect, beforeEach, afterEach, afterAll, mock, spyOn } from "bun:test"
|
import { describe, it, expect, beforeEach, afterEach, afterAll, mock, spyOn } from "bun:test"
|
||||||
|
import type { Transport } from "@modelcontextprotocol/sdk/shared/transport.js"
|
||||||
import type { SkillMcpClientInfo, SkillMcpServerContext } from "./types"
|
import type { SkillMcpClientInfo, SkillMcpServerContext } from "./types"
|
||||||
import type { ClaudeCodeMcpServer } from "../claude-code-mcp-loader/types"
|
import type { ClaudeCodeMcpServer } from "../claude-code-mcp-loader/types"
|
||||||
import type { OAuthTokenData } from "../mcp-oauth/storage"
|
import type { OAuthTokenData } from "../mcp-oauth/storage"
|
||||||
|
import { setHttpClientDependenciesForTesting } from "./http-client"
|
||||||
|
import { setStdioClientDependenciesForTesting } from "./stdio-client"
|
||||||
|
import { SkillMcpManager } from "./manager"
|
||||||
|
|
||||||
// Mock the MCP SDK transports to avoid network calls
|
|
||||||
const mockHttpConnect = mock(() => Promise.reject(new Error("Mocked HTTP connection failure")))
|
const mockHttpConnect = mock(() => Promise.reject(new Error("Mocked HTTP connection failure")))
|
||||||
const mockHttpClose = mock(() => Promise.resolve())
|
const mockHttpClose = mock(() => Promise.resolve())
|
||||||
let lastTransportInstance: { url?: URL; options?: { requestInit?: RequestInit } } = {}
|
let lastTransportInstance: { url?: URL; options?: { requestInit?: RequestInit } } = {}
|
||||||
|
|
||||||
|
class MockHttpClient {
|
||||||
|
readonly close = mock(() => Promise.resolve())
|
||||||
|
readonly listTools = mock(async () => ({ tools: [] }))
|
||||||
|
readonly listResources = mock(async () => ({ resources: [] }))
|
||||||
|
readonly listPrompts = mock(async () => ({ prompts: [] }))
|
||||||
|
readonly callTool = mock(async () => ({ content: [] }))
|
||||||
|
readonly readResource = mock(async () => ({ contents: [] }))
|
||||||
|
readonly getPrompt = mock(async () => ({ messages: [] }))
|
||||||
|
|
||||||
|
constructor(
|
||||||
|
_clientInfo: { name: string; version: string },
|
||||||
|
_options: { capabilities: Record<string, never> }
|
||||||
|
) {}
|
||||||
|
|
||||||
|
async connect(transport: Transport): Promise<void> {
|
||||||
|
await transport.start()
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
class MockStreamableHTTPClientTransport {
|
||||||
|
constructor(public url: URL, public options?: { requestInit?: RequestInit }) {
|
||||||
|
lastTransportInstance = { url, options }
|
||||||
|
}
|
||||||
|
|
||||||
|
async start(): Promise<void> {
|
||||||
|
await mockHttpConnect()
|
||||||
|
}
|
||||||
|
|
||||||
|
async send(): Promise<void> {}
|
||||||
|
|
||||||
|
async close(): Promise<void> {
|
||||||
|
await mockHttpClose()
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
function getHeaderValue(headers: HeadersInit | undefined, name: string): string | undefined {
|
||||||
|
if (!headers) {
|
||||||
|
return undefined
|
||||||
|
}
|
||||||
|
|
||||||
|
if (headers instanceof Headers) {
|
||||||
|
return headers.get(name) ?? undefined
|
||||||
|
}
|
||||||
|
|
||||||
|
if (Array.isArray(headers)) {
|
||||||
|
const entry = headers.find(([headerName]) => headerName.toLowerCase() === name.toLowerCase())
|
||||||
|
return entry?.[1]
|
||||||
|
}
|
||||||
|
|
||||||
|
return headers[name]
|
||||||
|
}
|
||||||
|
|
||||||
const mockTokens = mock(() => null as OAuthTokenData | null)
|
const mockTokens = mock(() => null as OAuthTokenData | null)
|
||||||
const mockLogin = mock(() => Promise.resolve({ accessToken: "test-token" } satisfies OAuthTokenData))
|
const mockLogin = mock(() => Promise.resolve({ accessToken: "test-token" } satisfies OAuthTokenData))
|
||||||
const mockRefresh = mock((_: string) => Promise.resolve({ accessToken: "refreshed-token" } satisfies OAuthTokenData))
|
const mockRefresh = mock((_: string) => Promise.resolve({ accessToken: "refreshed-token" } satisfies OAuthTokenData))
|
||||||
|
|
||||||
async function importFreshManagerModule(): Promise<typeof import("./manager")> {
|
|
||||||
mock.module("@modelcontextprotocol/sdk/client/streamableHttp.js", () => ({
|
|
||||||
StreamableHTTPClientTransport: class MockStreamableHTTPClientTransport {
|
|
||||||
constructor(public url: URL, public options?: { requestInit?: RequestInit }) {
|
|
||||||
lastTransportInstance = { url, options }
|
|
||||||
}
|
|
||||||
async start() {
|
|
||||||
await mockHttpConnect()
|
|
||||||
}
|
|
||||||
async close() {
|
|
||||||
await mockHttpClose()
|
|
||||||
}
|
|
||||||
},
|
|
||||||
}))
|
|
||||||
|
|
||||||
const module = await import(`./manager?test=${Date.now()}-${Math.random()}`)
|
|
||||||
mock.restore()
|
|
||||||
return module
|
|
||||||
}
|
|
||||||
|
|
||||||
afterAll(() => { mock.restore() })
|
afterAll(() => { mock.restore() })
|
||||||
|
|
||||||
describe("SkillMcpManager", () => {
|
describe("SkillMcpManager", () => {
|
||||||
let manager: any
|
let manager: SkillMcpManager
|
||||||
|
|
||||||
|
beforeEach(() => {
|
||||||
|
setHttpClientDependenciesForTesting({
|
||||||
|
createClient: (clientInfo, options) => new MockHttpClient(clientInfo, options),
|
||||||
|
createTransport: (url, options) => new MockStreamableHTTPClientTransport(url, options),
|
||||||
|
})
|
||||||
|
setStdioClientDependenciesForTesting()
|
||||||
|
|
||||||
beforeEach(async () => {
|
|
||||||
const { SkillMcpManager } = await importFreshManagerModule()
|
|
||||||
manager = new SkillMcpManager({
|
manager = new SkillMcpManager({
|
||||||
createOAuthProvider: () => ({
|
createOAuthProvider: () => ({
|
||||||
tokens: () => mockTokens(),
|
tokens: () => mockTokens(),
|
||||||
@@ -51,10 +91,13 @@ describe("SkillMcpManager", () => {
|
|||||||
mockTokens.mockClear()
|
mockTokens.mockClear()
|
||||||
mockLogin.mockClear()
|
mockLogin.mockClear()
|
||||||
mockRefresh.mockClear()
|
mockRefresh.mockClear()
|
||||||
|
lastTransportInstance = {}
|
||||||
})
|
})
|
||||||
|
|
||||||
afterEach(async () => {
|
afterEach(async () => {
|
||||||
await manager.disconnectAll()
|
await manager.disconnectAll()
|
||||||
|
setHttpClientDependenciesForTesting()
|
||||||
|
setStdioClientDependenciesForTesting()
|
||||||
})
|
})
|
||||||
|
|
||||||
describe("getOrCreateClient", () => {
|
describe("getOrCreateClient", () => {
|
||||||
@@ -697,8 +740,8 @@ describe("SkillMcpManager", () => {
|
|||||||
} catch { /* connection fails in test */ }
|
} catch { /* connection fails in test */ }
|
||||||
|
|
||||||
// then
|
// then
|
||||||
const headers = lastTransportInstance.options?.requestInit?.headers as Record<string, string> | undefined
|
const headers = lastTransportInstance.options?.requestInit?.headers
|
||||||
expect(headers?.Authorization).toBe("Bearer stored-access-token")
|
expect(getHeaderValue(headers, "Authorization")).toBe("Bearer stored-access-token")
|
||||||
})
|
})
|
||||||
|
|
||||||
it("does not inject Authorization header when no stored tokens exist and login fails", async () => {
|
it("does not inject Authorization header when no stored tokens exist and login fails", async () => {
|
||||||
@@ -724,8 +767,8 @@ describe("SkillMcpManager", () => {
|
|||||||
} catch { /* connection fails in test */ }
|
} catch { /* connection fails in test */ }
|
||||||
|
|
||||||
// then
|
// then
|
||||||
const headers = lastTransportInstance.options?.requestInit?.headers as Record<string, string> | undefined
|
const headers = lastTransportInstance.options?.requestInit?.headers
|
||||||
expect(headers?.Authorization).toBeUndefined()
|
expect(getHeaderValue(headers, "Authorization")).toBeUndefined()
|
||||||
})
|
})
|
||||||
|
|
||||||
it("preserves existing static headers alongside OAuth token", async () => {
|
it("preserves existing static headers alongside OAuth token", async () => {
|
||||||
@@ -753,9 +796,9 @@ describe("SkillMcpManager", () => {
|
|||||||
} catch { /* connection fails in test */ }
|
} catch { /* connection fails in test */ }
|
||||||
|
|
||||||
// then
|
// then
|
||||||
const headers = lastTransportInstance.options?.requestInit?.headers as Record<string, string> | undefined
|
const headers = lastTransportInstance.options?.requestInit?.headers
|
||||||
expect(headers?.["X-Custom"]).toBe("custom-value")
|
expect(getHeaderValue(headers, "X-Custom")).toBe("custom-value")
|
||||||
expect(headers?.Authorization).toBe("Bearer oauth-token")
|
expect(getHeaderValue(headers, "Authorization")).toBe("Bearer oauth-token")
|
||||||
})
|
})
|
||||||
|
|
||||||
it("attempts silent refresh for expired stored tokens before login", async () => {
|
it("attempts silent refresh for expired stored tokens before login", async () => {
|
||||||
@@ -785,8 +828,8 @@ describe("SkillMcpManager", () => {
|
|||||||
} catch { /* connection fails in test */ }
|
} catch { /* connection fails in test */ }
|
||||||
|
|
||||||
// then
|
// then
|
||||||
const headers = lastTransportInstance.options?.requestInit?.headers as Record<string, string> | undefined
|
const headers = lastTransportInstance.options?.requestInit?.headers
|
||||||
expect(headers?.Authorization).toBe("Bearer refreshed-token")
|
expect(getHeaderValue(headers, "Authorization")).toBe("Bearer refreshed-token")
|
||||||
expect(mockRefresh).toHaveBeenCalledWith("refresh-token")
|
expect(mockRefresh).toHaveBeenCalledWith("refresh-token")
|
||||||
expect(mockLogin).not.toHaveBeenCalled()
|
expect(mockLogin).not.toHaveBeenCalled()
|
||||||
})
|
})
|
||||||
@@ -819,8 +862,8 @@ describe("SkillMcpManager", () => {
|
|||||||
} catch { /* connection fails in test */ }
|
} catch { /* connection fails in test */ }
|
||||||
|
|
||||||
// then
|
// then
|
||||||
const headers = lastTransportInstance.options?.requestInit?.headers as Record<string, string> | undefined
|
const headers = lastTransportInstance.options?.requestInit?.headers
|
||||||
expect(headers?.Authorization).toBe("Bearer login-token")
|
expect(getHeaderValue(headers, "Authorization")).toBe("Bearer login-token")
|
||||||
expect(mockRefresh).toHaveBeenCalledWith("refresh-token")
|
expect(mockRefresh).toHaveBeenCalledWith("refresh-token")
|
||||||
expect(mockLogin).toHaveBeenCalled()
|
expect(mockLogin).toHaveBeenCalled()
|
||||||
})
|
})
|
||||||
@@ -846,8 +889,8 @@ describe("SkillMcpManager", () => {
|
|||||||
} catch { /* connection fails in test */ }
|
} catch { /* connection fails in test */ }
|
||||||
|
|
||||||
// then
|
// then
|
||||||
const headers = lastTransportInstance.options?.requestInit?.headers as Record<string, string> | undefined
|
const headers = lastTransportInstance.options?.requestInit?.headers
|
||||||
expect(headers?.Authorization).toBe("Bearer static-token")
|
expect(getHeaderValue(headers, "Authorization")).toBe("Bearer static-token")
|
||||||
expect(mockTokens).not.toHaveBeenCalled()
|
expect(mockTokens).not.toHaveBeenCalled()
|
||||||
})
|
})
|
||||||
|
|
||||||
|
|||||||
@@ -1,4 +1,3 @@
|
|||||||
import type { Client } from "@modelcontextprotocol/sdk/client/index.js"
|
|
||||||
import type { Prompt, Resource, Tool } from "@modelcontextprotocol/sdk/types.js"
|
import type { Prompt, Resource, Tool } from "@modelcontextprotocol/sdk/types.js"
|
||||||
import type { ClaudeCodeMcpServer } from "../claude-code-mcp-loader/types"
|
import type { ClaudeCodeMcpServer } from "../claude-code-mcp-loader/types"
|
||||||
import { McpOAuthProvider } from "../mcp-oauth/provider"
|
import { McpOAuthProvider } from "../mcp-oauth/provider"
|
||||||
@@ -6,6 +5,7 @@ import { disconnectAll, disconnectSession, forceReconnect } from "./cleanup"
|
|||||||
import { getOrCreateClient, getOrCreateClientWithRetryImpl } from "./connection"
|
import { getOrCreateClient, getOrCreateClientWithRetryImpl } from "./connection"
|
||||||
import { handlePostRequestAuthError, handleStepUpIfNeeded } from "./oauth-handler"
|
import { handlePostRequestAuthError, handleStepUpIfNeeded } from "./oauth-handler"
|
||||||
import type {
|
import type {
|
||||||
|
McpClient,
|
||||||
OAuthProviderFactory,
|
OAuthProviderFactory,
|
||||||
SkillMcpClientInfo,
|
SkillMcpClientInfo,
|
||||||
SkillMcpManagerState,
|
SkillMcpManagerState,
|
||||||
@@ -36,7 +36,7 @@ export class SkillMcpManager {
|
|||||||
return `${info.sessionID}:${info.skillName}:${info.serverName}`
|
return `${info.sessionID}:${info.skillName}:${info.serverName}`
|
||||||
}
|
}
|
||||||
|
|
||||||
async getOrCreateClient(info: SkillMcpClientInfo, config: ClaudeCodeMcpServer): Promise<Client> {
|
async getOrCreateClient(info: SkillMcpClientInfo, config: ClaudeCodeMcpServer): Promise<McpClient> {
|
||||||
const clientKey = this.getClientKey(info)
|
const clientKey = this.getClientKey(info)
|
||||||
return await getOrCreateClient({
|
return await getOrCreateClient({
|
||||||
state: this.state,
|
state: this.state,
|
||||||
@@ -106,7 +106,7 @@ export class SkillMcpManager {
|
|||||||
private async withOperationRetry<T>(
|
private async withOperationRetry<T>(
|
||||||
info: SkillMcpClientInfo,
|
info: SkillMcpClientInfo,
|
||||||
config: ClaudeCodeMcpServer,
|
config: ClaudeCodeMcpServer,
|
||||||
operation: (client: Client) => Promise<T>
|
operation: (client: McpClient) => Promise<T>
|
||||||
): Promise<T> {
|
): Promise<T> {
|
||||||
const maxRetries = 3
|
const maxRetries = 3
|
||||||
let lastError: Error | null = null
|
let lastError: Error | null = null
|
||||||
@@ -158,7 +158,7 @@ export class SkillMcpManager {
|
|||||||
}
|
}
|
||||||
|
|
||||||
// NOTE: tests spy on this exact method name via `spyOn(manager as any, 'getOrCreateClientWithRetry')`.
|
// NOTE: tests spy on this exact method name via `spyOn(manager as any, 'getOrCreateClientWithRetry')`.
|
||||||
private async getOrCreateClientWithRetry(info: SkillMcpClientInfo, config: ClaudeCodeMcpServer): Promise<Client> {
|
private async getOrCreateClientWithRetry(info: SkillMcpClientInfo, config: ClaudeCodeMcpServer): Promise<McpClient> {
|
||||||
const clientKey = this.getClientKey(info)
|
const clientKey = this.getClientKey(info)
|
||||||
return await getOrCreateClientWithRetryImpl({
|
return await getOrCreateClientWithRetryImpl({
|
||||||
state: this.state,
|
state: this.state,
|
||||||
|
|||||||
@@ -4,7 +4,39 @@ import type { ClaudeCodeMcpServer } from "../claude-code-mcp-loader/types"
|
|||||||
import { createCleanMcpEnvironment } from "./env-cleaner"
|
import { createCleanMcpEnvironment } from "./env-cleaner"
|
||||||
import { registerProcessCleanup, startCleanupTimer } from "./cleanup"
|
import { registerProcessCleanup, startCleanupTimer } from "./cleanup"
|
||||||
import { redactSensitiveData } from "./error-redaction"
|
import { redactSensitiveData } from "./error-redaction"
|
||||||
import type { ManagedClient, SkillMcpClientConnectionParams } from "./types"
|
import type { ManagedClient, McpClient, McpTransport, SkillMcpClientConnectionParams } from "./types"
|
||||||
|
|
||||||
|
type StdioClientFactory = (
|
||||||
|
clientInfo: { name: string; version: string },
|
||||||
|
options: { capabilities: Record<string, never> }
|
||||||
|
) => McpClient
|
||||||
|
|
||||||
|
type StdioTransportFactory = (
|
||||||
|
options: ConstructorParameters<typeof StdioClientTransport>[0]
|
||||||
|
) => McpTransport
|
||||||
|
|
||||||
|
interface StdioClientDependencies {
|
||||||
|
createClient: StdioClientFactory
|
||||||
|
createTransport: StdioTransportFactory
|
||||||
|
}
|
||||||
|
|
||||||
|
const defaultStdioClientDependencies: StdioClientDependencies = {
|
||||||
|
createClient: (clientInfo, options) => new Client(clientInfo, options),
|
||||||
|
createTransport: (options) => new StdioClientTransport(options),
|
||||||
|
}
|
||||||
|
|
||||||
|
let stdioClientDependencies: StdioClientDependencies = defaultStdioClientDependencies
|
||||||
|
|
||||||
|
export function setStdioClientDependenciesForTesting(
|
||||||
|
dependencies?: Partial<StdioClientDependencies>
|
||||||
|
): void {
|
||||||
|
stdioClientDependencies = dependencies
|
||||||
|
? {
|
||||||
|
...defaultStdioClientDependencies,
|
||||||
|
...dependencies,
|
||||||
|
}
|
||||||
|
: defaultStdioClientDependencies
|
||||||
|
}
|
||||||
|
|
||||||
function getStdioCommand(config: ClaudeCodeMcpServer, serverName: string): string {
|
function getStdioCommand(config: ClaudeCodeMcpServer, serverName: string): string {
|
||||||
if (!config.command) {
|
if (!config.command) {
|
||||||
@@ -13,7 +45,7 @@ function getStdioCommand(config: ClaudeCodeMcpServer, serverName: string): strin
|
|||||||
return config.command
|
return config.command
|
||||||
}
|
}
|
||||||
|
|
||||||
export async function createStdioClient(params: SkillMcpClientConnectionParams): Promise<Client> {
|
export async function createStdioClient(params: SkillMcpClientConnectionParams): Promise<McpClient> {
|
||||||
const { state, clientKey, info, config } = params
|
const { state, clientKey, info, config } = params
|
||||||
const shutdownGenAtStart = state.shutdownGeneration
|
const shutdownGenAtStart = state.shutdownGeneration
|
||||||
|
|
||||||
@@ -23,14 +55,14 @@ export async function createStdioClient(params: SkillMcpClientConnectionParams):
|
|||||||
|
|
||||||
registerProcessCleanup(state)
|
registerProcessCleanup(state)
|
||||||
|
|
||||||
const transport = new StdioClientTransport({
|
const transport: McpTransport = stdioClientDependencies.createTransport({
|
||||||
command,
|
command,
|
||||||
args,
|
args,
|
||||||
env: mergedEnv,
|
env: mergedEnv,
|
||||||
stderr: "ignore",
|
stderr: "ignore",
|
||||||
})
|
})
|
||||||
|
|
||||||
const client = new Client(
|
const client: McpClient = stdioClientDependencies.createClient(
|
||||||
{ name: `skill-mcp-${info.skillName}-${info.serverName}`, version: "1.0.0" },
|
{ name: `skill-mcp-${info.skillName}-${info.serverName}`, version: "1.0.0" },
|
||||||
{ capabilities: {} }
|
{ capabilities: {} }
|
||||||
)
|
)
|
||||||
|
|||||||
@@ -1,12 +1,24 @@
|
|||||||
import type { Client } from "@modelcontextprotocol/sdk/client/index.js"
|
import type { Client } from "@modelcontextprotocol/sdk/client/index.js"
|
||||||
import type { StdioClientTransport } from "@modelcontextprotocol/sdk/client/stdio.js"
|
import type { Transport } from "@modelcontextprotocol/sdk/shared/transport.js"
|
||||||
import type { StreamableHTTPClientTransport } from "@modelcontextprotocol/sdk/client/streamableHttp.js"
|
|
||||||
import type { ClaudeCodeMcpServer } from "../claude-code-mcp-loader/types"
|
import type { ClaudeCodeMcpServer } from "../claude-code-mcp-loader/types"
|
||||||
import type { McpOAuthProvider } from "../mcp-oauth/provider"
|
import type { McpOAuthProvider } from "../mcp-oauth/provider"
|
||||||
import type { SkillScope } from "../opencode-skill-loader/types"
|
import type { SkillScope } from "../opencode-skill-loader/types"
|
||||||
|
|
||||||
export type SkillMcpConfig = Record<string, ClaudeCodeMcpServer>
|
export type SkillMcpConfig = Record<string, ClaudeCodeMcpServer>
|
||||||
|
|
||||||
|
export type McpTransport = Transport
|
||||||
|
|
||||||
|
export interface McpClient {
|
||||||
|
connect: Client["connect"]
|
||||||
|
close: Client["close"]
|
||||||
|
listTools: Client["listTools"]
|
||||||
|
listResources: Client["listResources"]
|
||||||
|
listPrompts: Client["listPrompts"]
|
||||||
|
callTool: Client["callTool"]
|
||||||
|
readResource: Client["readResource"]
|
||||||
|
getPrompt: Client["getPrompt"]
|
||||||
|
}
|
||||||
|
|
||||||
export interface SkillMcpClientInfo {
|
export interface SkillMcpClientInfo {
|
||||||
serverName: string
|
serverName: string
|
||||||
skillName: string
|
skillName: string
|
||||||
@@ -27,7 +39,7 @@ export interface SkillMcpServerContext {
|
|||||||
export type ConnectionType = "stdio" | "http"
|
export type ConnectionType = "stdio" | "http"
|
||||||
|
|
||||||
export interface ManagedClientBase {
|
export interface ManagedClientBase {
|
||||||
client: Client
|
client: McpClient
|
||||||
skillName: string
|
skillName: string
|
||||||
lastUsedAt: number
|
lastUsedAt: number
|
||||||
connectionType: ConnectionType
|
connectionType: ConnectionType
|
||||||
@@ -35,12 +47,12 @@ export interface ManagedClientBase {
|
|||||||
|
|
||||||
export interface ManagedStdioClient extends ManagedClientBase {
|
export interface ManagedStdioClient extends ManagedClientBase {
|
||||||
connectionType: "stdio"
|
connectionType: "stdio"
|
||||||
transport: StdioClientTransport
|
transport: McpTransport
|
||||||
}
|
}
|
||||||
|
|
||||||
export interface ManagedHttpClient extends ManagedClientBase {
|
export interface ManagedHttpClient extends ManagedClientBase {
|
||||||
connectionType: "http"
|
connectionType: "http"
|
||||||
transport: StreamableHTTPClientTransport
|
transport: McpTransport
|
||||||
}
|
}
|
||||||
|
|
||||||
export type ManagedClient = ManagedStdioClient | ManagedHttpClient
|
export type ManagedClient = ManagedStdioClient | ManagedHttpClient
|
||||||
@@ -63,7 +75,7 @@ export type OAuthProviderFactory = (options: {
|
|||||||
|
|
||||||
export interface SkillMcpManagerState {
|
export interface SkillMcpManagerState {
|
||||||
clients: Map<string, ManagedClient>
|
clients: Map<string, ManagedClient>
|
||||||
pendingConnections: Map<string, Promise<Client>>
|
pendingConnections: Map<string, Promise<McpClient>>
|
||||||
disconnectedSessions: Map<string, number>
|
disconnectedSessions: Map<string, number>
|
||||||
authProviders: Map<string, McpOAuthProvider>
|
authProviders: Map<string, McpOAuthProvider>
|
||||||
cleanupRegistered: boolean
|
cleanupRegistered: boolean
|
||||||
|
|||||||
Reference in New Issue
Block a user