[codex] fix MCP management lifecycle (#1144)
* feat(mcp): add MCP server management UI - Server CRUD: add/edit/remove with YAML/JSON Monaco editor - raw_config passthrough: zero field loss on edit/toggle - tool_details embedding: single-request card data (1+N → 1) - Auto-retry exponential backoff (2s→32s, max 5 retries) - Route safety guards (hasRoute) for dynamic sidebar - i18n: 9 languages (de/en/es/fr/ja/ko/pt/zh/zh-TW) - 19 unit tests + 8 UX browser tests - 35 files, +2933 lines * fix mcp management lifecycle --------- Co-authored-by: Crafter-feng <succeed_happu@163.com>
This commit is contained in:
ekko
@@ -711,6 +711,51 @@ assert response["ok"] is True, response
|
||||
assert captured["endpoint"] == "ipc:///tmp/worker.sock", captured
|
||||
assert captured["req"] == {"action": "chat"}, captured
|
||||
assert captured["timeout"] == 310, captured
|
||||
`)
|
||||
})
|
||||
|
||||
it('awaits MCP server shutdown without holding the MCP registry lock', () => {
|
||||
runPython(String.raw`
|
||||
${harness}
|
||||
|
||||
import asyncio
|
||||
|
||||
lock = threading.Lock()
|
||||
servers = {}
|
||||
events = []
|
||||
|
||||
class FakeMcpTask:
|
||||
async def shutdown(self):
|
||||
events.append("shutdown-started")
|
||||
acquired = lock.acquire(blocking=False)
|
||||
events.append(("lock-free-during-shutdown", acquired))
|
||||
if acquired:
|
||||
lock.release()
|
||||
await asyncio.sleep(0)
|
||||
events.append("shutdown-finished")
|
||||
|
||||
task = FakeMcpTask()
|
||||
servers["github"] = task
|
||||
|
||||
def run_on_mcp_loop(factory, timeout=30):
|
||||
events.append(("timeout", timeout))
|
||||
asyncio.run(factory())
|
||||
|
||||
result = bridge.BridgeServer._shutdown_mcp_server(
|
||||
"github",
|
||||
servers,
|
||||
lock,
|
||||
run_on_mcp_loop,
|
||||
)
|
||||
|
||||
assert result is True, result
|
||||
assert "github" not in servers, servers
|
||||
assert events == [
|
||||
("timeout", 15),
|
||||
"shutdown-started",
|
||||
("lock-free-during-shutdown", True),
|
||||
"shutdown-finished",
|
||||
], events
|
||||
`)
|
||||
})
|
||||
})
|
||||
|
||||
@@ -0,0 +1,286 @@
|
||||
import { beforeEach, describe, expect, it, vi } from 'vitest'
|
||||
|
||||
// ── Mocks ──────────────────────────────────────────────────
|
||||
const mcpListMock = vi.fn()
|
||||
const mcpAddMock = vi.fn()
|
||||
const mcpUpdateMock = vi.fn()
|
||||
const mcpRemoveMock = vi.fn()
|
||||
const mcpTestMock = vi.fn()
|
||||
const mcpToolsMock = vi.fn()
|
||||
const mcpReloadMock = vi.fn()
|
||||
|
||||
vi.mock('../../packages/server/src/services/hermes/agent-bridge/client', () => ({
|
||||
AgentBridgeClient: vi.fn().mockImplementation(() => ({
|
||||
mcpList: mcpListMock,
|
||||
mcpAdd: mcpAddMock,
|
||||
mcpUpdate: mcpUpdateMock,
|
||||
mcpRemove: mcpRemoveMock,
|
||||
mcpTest: mcpTestMock,
|
||||
mcpTools: mcpToolsMock,
|
||||
mcpReload: mcpReloadMock,
|
||||
})),
|
||||
}))
|
||||
|
||||
vi.mock('../../packages/server/src/services/logger', () => ({
|
||||
logger: { warn: vi.fn(), error: vi.fn(), info: vi.fn() },
|
||||
}))
|
||||
|
||||
// ── Helpers ────────────────────────────────────────────────
|
||||
function createCtx(overrides: Record<string, any> = {}) {
|
||||
const ctx: any = {
|
||||
state: { profile: { name: 'test-profile' } },
|
||||
request: { body: {} },
|
||||
params: {},
|
||||
query: {},
|
||||
status: 200,
|
||||
body: null,
|
||||
...overrides,
|
||||
}
|
||||
return ctx
|
||||
}
|
||||
|
||||
const SAMPLE_SERVERS_RESPONSE = {
|
||||
ok: true,
|
||||
servers: [
|
||||
{
|
||||
name: 'github',
|
||||
transport: 'stdio',
|
||||
connected: true,
|
||||
tools: 26,
|
||||
tools_registered: 3,
|
||||
tool_names: ['create_repository', 'search_repositories'],
|
||||
tool_names_registered: ['mcp_github_create_repository', 'mcp_github_search_repositories'],
|
||||
error: null,
|
||||
raw_config: {
|
||||
command: 'npx',
|
||||
args: ['-y', '@modelcontextprotocol/server-github'],
|
||||
tools: { include: ['create_repository', 'search_repositories'] },
|
||||
prompts: null,
|
||||
resources: null,
|
||||
enabled: true,
|
||||
},
|
||||
tool_details: [
|
||||
{ name: 'create_repository', description: 'Create a repo' },
|
||||
{ name: 'search_repositories', description: 'Search repos' },
|
||||
],
|
||||
},
|
||||
],
|
||||
total_tools: 3,
|
||||
}
|
||||
|
||||
const SAMPLE_TOOLS_RESPONSE = {
|
||||
ok: true,
|
||||
results: [
|
||||
{
|
||||
server: 'github',
|
||||
tools: [
|
||||
{ name: 'create_repository', description: 'Create a repo', input_schema: {} },
|
||||
{ name: 'search_repositories', description: 'Search repos', input_schema: {} },
|
||||
],
|
||||
},
|
||||
],
|
||||
}
|
||||
|
||||
// ── Tests ──────────────────────────────────────────────────
|
||||
describe('MCP Controller', () => {
|
||||
beforeEach(() => {
|
||||
vi.clearAllMocks()
|
||||
})
|
||||
|
||||
describe('listServers', () => {
|
||||
it('returns servers list from bridge', async () => {
|
||||
mcpListMock.mockResolvedValue(SAMPLE_SERVERS_RESPONSE)
|
||||
const { listServers } = await import('../../packages/server/src/controllers/hermes/mcp')
|
||||
const ctx = createCtx()
|
||||
await listServers(ctx)
|
||||
expect(ctx.body).toEqual(SAMPLE_SERVERS_RESPONSE)
|
||||
expect(mcpListMock).toHaveBeenCalledWith('test-profile')
|
||||
})
|
||||
|
||||
it('returns 503 on bridge error', async () => {
|
||||
mcpListMock.mockRejectedValue(new Error('bridge down'))
|
||||
const { listServers } = await import('../../packages/server/src/controllers/hermes/mcp')
|
||||
const ctx = createCtx()
|
||||
await listServers(ctx)
|
||||
expect(ctx.status).toBe(503)
|
||||
expect(ctx.body).toEqual({ error: 'bridge down' })
|
||||
})
|
||||
})
|
||||
|
||||
describe('addServer', () => {
|
||||
it('sends name and config to bridge', async () => {
|
||||
mcpAddMock.mockResolvedValue({ ok: true, name: 'my-server' })
|
||||
const { addServer } = await import('../../packages/server/src/controllers/hermes/mcp')
|
||||
const ctx = createCtx({ request: { body: { name: 'my-server', config: { command: 'node', args: ['srv.js'] } } } })
|
||||
await addServer(ctx)
|
||||
expect(mcpAddMock).toHaveBeenCalledWith('my-server', { command: 'node', args: ['srv.js'] }, 'test-profile')
|
||||
expect(ctx.body).toEqual({ ok: true, name: 'my-server' })
|
||||
})
|
||||
|
||||
it('returns 400 when name is missing', async () => {
|
||||
const { addServer } = await import('../../packages/server/src/controllers/hermes/mcp')
|
||||
const ctx = createCtx({ request: { body: { config: { command: 'x' } } } })
|
||||
await addServer(ctx)
|
||||
expect(ctx.status).toBe(400)
|
||||
expect(mcpAddMock).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('returns 400 when config is missing', async () => {
|
||||
const { addServer } = await import('../../packages/server/src/controllers/hermes/mcp')
|
||||
const ctx = createCtx({ request: { body: { name: 'x' } } })
|
||||
await addServer(ctx)
|
||||
expect(ctx.status).toBe(400)
|
||||
})
|
||||
})
|
||||
|
||||
describe('updateServer', () => {
|
||||
it('sends name from params and config to bridge', async () => {
|
||||
mcpUpdateMock.mockResolvedValue({ ok: true })
|
||||
const { updateServer } = await import('../../packages/server/src/controllers/hermes/mcp')
|
||||
const ctx = createCtx({
|
||||
params: { name: 'github' },
|
||||
request: { body: { config: { tools: { include: ['a', 'b'] } } } },
|
||||
})
|
||||
await updateServer(ctx)
|
||||
expect(mcpUpdateMock).toHaveBeenCalledWith('github', { tools: { include: ['a', 'b'] } }, 'test-profile')
|
||||
})
|
||||
|
||||
it('returns 400 when config is missing', async () => {
|
||||
const { updateServer } = await import('../../packages/server/src/controllers/hermes/mcp')
|
||||
const ctx = createCtx({ params: { name: 'github' }, request: { body: {} } })
|
||||
await updateServer(ctx)
|
||||
expect(ctx.status).toBe(400)
|
||||
})
|
||||
})
|
||||
|
||||
describe('removeServer', () => {
|
||||
it('sends name to bridge', async () => {
|
||||
mcpRemoveMock.mockResolvedValue({ ok: true })
|
||||
const { removeServer } = await import('../../packages/server/src/controllers/hermes/mcp')
|
||||
const ctx = createCtx({ params: { name: 'github' } })
|
||||
await removeServer(ctx)
|
||||
expect(mcpRemoveMock).toHaveBeenCalledWith('github', 'test-profile')
|
||||
})
|
||||
})
|
||||
|
||||
describe('testServer', () => {
|
||||
it('returns tool list from bridge', async () => {
|
||||
mcpTestMock.mockResolvedValue({ ok: true, tools: ['create_repository', 'search_repositories'] })
|
||||
const { testServer } = await import('../../packages/server/src/controllers/hermes/mcp')
|
||||
const ctx = createCtx({ params: { name: 'github' } })
|
||||
await testServer(ctx)
|
||||
expect(mcpTestMock).toHaveBeenCalledWith('github', 'test-profile')
|
||||
expect(ctx.body).toEqual({ ok: true, tools: ['create_repository', 'search_repositories'] })
|
||||
})
|
||||
})
|
||||
|
||||
describe('listTools', () => {
|
||||
it('returns tools without server filter', async () => {
|
||||
mcpToolsMock.mockResolvedValue(SAMPLE_TOOLS_RESPONSE)
|
||||
const { listTools } = await import('../../packages/server/src/controllers/hermes/mcp')
|
||||
const ctx = createCtx({ query: {} })
|
||||
await listTools(ctx)
|
||||
expect(mcpToolsMock).toHaveBeenCalledWith(undefined, 'test-profile')
|
||||
expect(ctx.body).toEqual(SAMPLE_TOOLS_RESPONSE)
|
||||
})
|
||||
|
||||
it('passes server filter to bridge', async () => {
|
||||
mcpToolsMock.mockResolvedValue(SAMPLE_TOOLS_RESPONSE)
|
||||
const { listTools } = await import('../../packages/server/src/controllers/hermes/mcp')
|
||||
const ctx = createCtx({ query: { server: 'github' } })
|
||||
await listTools(ctx)
|
||||
expect(mcpToolsMock).toHaveBeenCalledWith('github', 'test-profile')
|
||||
})
|
||||
|
||||
it('returns 503 on bridge error', async () => {
|
||||
mcpToolsMock.mockRejectedValue(new Error('timeout'))
|
||||
const { listTools } = await import('../../packages/server/src/controllers/hermes/mcp')
|
||||
const ctx = createCtx()
|
||||
await listTools(ctx)
|
||||
expect(ctx.status).toBe(503)
|
||||
})
|
||||
})
|
||||
|
||||
describe('reloadMcp', () => {
|
||||
it('reloads all servers when no filter', async () => {
|
||||
mcpReloadMock.mockResolvedValue({ ok: true, message: 'MCP servers reloaded' })
|
||||
const { reloadMcp } = await import('../../packages/server/src/controllers/hermes/mcp')
|
||||
const ctx = createCtx({ query: {} })
|
||||
await reloadMcp(ctx)
|
||||
expect(mcpReloadMock).toHaveBeenCalledWith(undefined, 'test-profile')
|
||||
})
|
||||
|
||||
it('reloads specific server', async () => {
|
||||
mcpReloadMock.mockResolvedValue({ ok: true, message: 'MCP servers reloaded' })
|
||||
const { reloadMcp } = await import('../../packages/server/src/controllers/hermes/mcp')
|
||||
const ctx = createCtx({ query: { server: 'github' } })
|
||||
await reloadMcp(ctx)
|
||||
expect(mcpReloadMock).toHaveBeenCalledWith('github', 'test-profile')
|
||||
})
|
||||
|
||||
it('returns 503 on bridge error', async () => {
|
||||
mcpReloadMock.mockRejectedValue(new Error('reload failed'))
|
||||
const { reloadMcp } = await import('../../packages/server/src/controllers/hermes/mcp')
|
||||
const ctx = createCtx()
|
||||
await reloadMcp(ctx)
|
||||
expect(ctx.status).toBe(503)
|
||||
})
|
||||
})
|
||||
|
||||
describe('profile handling', () => {
|
||||
it('passes undefined profile when ctx.state.profile is missing', async () => {
|
||||
mcpListMock.mockResolvedValue({ ok: true, servers: [], total_tools: 0 })
|
||||
const { listServers } = await import('../../packages/server/src/controllers/hermes/mcp')
|
||||
const ctx = createCtx({ state: {} })
|
||||
await listServers(ctx)
|
||||
expect(mcpListMock).toHaveBeenCalledWith(undefined)
|
||||
})
|
||||
|
||||
it('passes undefined profile when profile.name is empty', async () => {
|
||||
mcpListMock.mockResolvedValue({ ok: true, servers: [], total_tools: 0 })
|
||||
const { listServers } = await import('../../packages/server/src/controllers/hermes/mcp')
|
||||
const ctx = createCtx({ state: { profile: { name: '' } } })
|
||||
await listServers(ctx)
|
||||
expect(mcpListMock).toHaveBeenCalledWith(undefined)
|
||||
})
|
||||
})
|
||||
|
||||
describe('response structure', () => {
|
||||
it('mcp_list response has all required fields', async () => {
|
||||
mcpListMock.mockResolvedValue(SAMPLE_SERVERS_RESPONSE)
|
||||
const { listServers } = await import('../../packages/server/src/controllers/hermes/mcp')
|
||||
const ctx = createCtx()
|
||||
await listServers(ctx)
|
||||
const body = ctx.body as any
|
||||
expect(body.ok).toBe(true)
|
||||
expect(body.servers).toBeDefined()
|
||||
expect(body.total_tools).toBeDefined()
|
||||
const server = body.servers[0]
|
||||
expect(server).toHaveProperty('name')
|
||||
expect(server).toHaveProperty('transport')
|
||||
expect(server).toHaveProperty('connected')
|
||||
expect(server).toHaveProperty('tools')
|
||||
expect(server).toHaveProperty('tools_registered')
|
||||
expect(server).toHaveProperty('tool_names')
|
||||
expect(server).toHaveProperty('tool_names_registered')
|
||||
expect(server).toHaveProperty('raw_config')
|
||||
expect(server).toHaveProperty('tool_details')
|
||||
expect(server.raw_config).toHaveProperty('command')
|
||||
expect(server.raw_config).toHaveProperty('enabled')
|
||||
})
|
||||
|
||||
it('mcp_tools_list response has tools with name/description/schema', async () => {
|
||||
mcpToolsMock.mockResolvedValue(SAMPLE_TOOLS_RESPONSE)
|
||||
const { listTools } = await import('../../packages/server/src/controllers/hermes/mcp')
|
||||
const ctx = createCtx()
|
||||
await listTools(ctx)
|
||||
const body = ctx.body as any
|
||||
expect(body.ok).toBe(true)
|
||||
expect(body.results).toHaveLength(1)
|
||||
const tool = body.results[0].tools[0]
|
||||
expect(tool).toHaveProperty('name')
|
||||
expect(tool).toHaveProperty('description')
|
||||
expect(tool).toHaveProperty('input_schema')
|
||||
})
|
||||
})
|
||||
})
|
||||
@@ -59,6 +59,7 @@ function makeContext(state: any, commandResult: Record<string, unknown> = {
|
||||
const runQueuedItem = vi.fn()
|
||||
const bridge = {
|
||||
command: vi.fn(async () => commandResult),
|
||||
mcpReload: vi.fn(async () => ({ ok: true, message: 'MCP servers reloaded' })),
|
||||
status: vi.fn(async () => ({
|
||||
exists: true,
|
||||
running: false,
|
||||
@@ -303,4 +304,30 @@ describe('plan session command', () => {
|
||||
}),
|
||||
}))
|
||||
})
|
||||
|
||||
it('rejects MCP reload while the session is running', async () => {
|
||||
const state = { messages: [], isWorking: true, events: [], queue: [] }
|
||||
const { bridge, namespaceEmit, runQueuedItem, sessionMap, socket, nsp } = makeContext(state)
|
||||
const { handleSessionCommand, parseSessionCommand } = await import('../../packages/server/src/services/hermes/run-chat/session-command')
|
||||
const command = parseSessionCommand('/reload-mcp github')!
|
||||
|
||||
await handleSessionCommand('session-1', command, {
|
||||
nsp: nsp as any,
|
||||
socket: socket as any,
|
||||
sessionMap,
|
||||
bridge: bridge as any,
|
||||
profile: 'default',
|
||||
runQueuedItem,
|
||||
})
|
||||
|
||||
expect(bridge.mcpReload).not.toHaveBeenCalled()
|
||||
expect(runQueuedItem).not.toHaveBeenCalled()
|
||||
expect(namespaceEmit).toHaveBeenCalledWith('session.command', expect.objectContaining({
|
||||
command: 'reload-mcp',
|
||||
ok: false,
|
||||
action: 'reload-mcp',
|
||||
terminal: false,
|
||||
message: 'MCP reload can only run while the session is idle. Wait for the current run to finish or abort it first.',
|
||||
}))
|
||||
})
|
||||
})
|
||||
|
||||
Reference in New Issue
Block a user