feat(skill-mcp-manager): enhance manager with improved test coverage
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-opencode) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
This commit is contained in:
@@ -395,6 +395,35 @@ describe("SkillMcpManager", () => {
|
||||
// then
|
||||
expect(manager.getConnectedServers()).toEqual([])
|
||||
})
|
||||
|
||||
it("unregisters signal handlers after disconnectAll", async () => {
|
||||
// given
|
||||
const info: SkillMcpClientInfo = {
|
||||
serverName: "signal-server",
|
||||
skillName: "signal-skill",
|
||||
sessionID: "session-1",
|
||||
}
|
||||
const config: ClaudeCodeMcpServer = {
|
||||
url: "https://example.com/mcp",
|
||||
}
|
||||
|
||||
const before = process.listenerCount("SIGINT")
|
||||
|
||||
// when
|
||||
try {
|
||||
await manager.getOrCreateClient(info, config)
|
||||
} catch {
|
||||
// Expected to fail connection, still registers cleanup handlers
|
||||
}
|
||||
const afterRegister = process.listenerCount("SIGINT")
|
||||
|
||||
await manager.disconnectAll()
|
||||
const afterDisconnect = process.listenerCount("SIGINT")
|
||||
|
||||
// then
|
||||
expect(afterRegister).toBe(before + 1)
|
||||
expect(afterDisconnect).toBe(before)
|
||||
})
|
||||
})
|
||||
|
||||
describe("isConnected", () => {
|
||||
|
||||
@@ -65,6 +65,7 @@ export class SkillMcpManager {
|
||||
private authProviders: Map<string, McpOAuthProvider> = new Map()
|
||||
private cleanupRegistered = false
|
||||
private cleanupInterval: ReturnType<typeof setInterval> | null = null
|
||||
private cleanupHandlers: Array<{ signal: NodeJS.Signals; listener: () => void }> = []
|
||||
private readonly IDLE_TIMEOUT = 5 * 60 * 1000
|
||||
|
||||
private getClientKey(info: SkillMcpClientInfo): string {
|
||||
@@ -119,11 +120,26 @@ export class SkillMcpManager {
|
||||
// Don't call process.exit() here - let the background-agent manager handle the final process exit.
|
||||
// Use void + catch to trigger async cleanup without awaiting it in the signal handler.
|
||||
|
||||
process.on("SIGINT", () => void cleanup().catch(() => {}))
|
||||
process.on("SIGTERM", () => void cleanup().catch(() => {}))
|
||||
if (process.platform === "win32") {
|
||||
process.on("SIGBREAK", () => void cleanup().catch(() => {}))
|
||||
const register = (signal: NodeJS.Signals) => {
|
||||
const listener = () => void cleanup().catch(() => {})
|
||||
this.cleanupHandlers.push({ signal, listener })
|
||||
process.on(signal, listener)
|
||||
}
|
||||
|
||||
register("SIGINT")
|
||||
register("SIGTERM")
|
||||
if (process.platform === "win32") {
|
||||
register("SIGBREAK")
|
||||
}
|
||||
}
|
||||
|
||||
private unregisterProcessCleanup(): void {
|
||||
if (!this.cleanupRegistered) return
|
||||
for (const { signal, listener } of this.cleanupHandlers) {
|
||||
process.off(signal, listener)
|
||||
}
|
||||
this.cleanupHandlers = []
|
||||
this.cleanupRegistered = false
|
||||
}
|
||||
|
||||
async getOrCreateClient(
|
||||
@@ -376,12 +392,23 @@ export class SkillMcpManager {
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
for (const key of keysToRemove) {
|
||||
this.pendingConnections.delete(key)
|
||||
}
|
||||
|
||||
if (this.clients.size === 0) {
|
||||
this.stopCleanupTimer()
|
||||
}
|
||||
}
|
||||
|
||||
async disconnectAll(): Promise<void> {
|
||||
this.stopCleanupTimer()
|
||||
this.unregisterProcessCleanup()
|
||||
const clients = Array.from(this.clients.values())
|
||||
this.clients.clear()
|
||||
this.pendingConnections.clear()
|
||||
this.authProviders.clear()
|
||||
for (const managed of clients) {
|
||||
try {
|
||||
await managed.client.close()
|
||||
@@ -420,6 +447,10 @@ export class SkillMcpManager {
|
||||
} catch { /* transport may already be terminated */ }
|
||||
}
|
||||
}
|
||||
|
||||
if (this.clients.size === 0) {
|
||||
this.stopCleanupTimer()
|
||||
}
|
||||
}
|
||||
|
||||
async listTools(
|
||||
|
||||
@@ -237,7 +237,7 @@ describe("sisyphus-task", () => {
|
||||
|
||||
// then proceeds without error - uses fallback chain
|
||||
expect(result).not.toContain("oh-my-opencode requires a default model")
|
||||
})
|
||||
}, { timeout: 10000 })
|
||||
|
||||
test("returns clear error when no model can be resolved", async () => {
|
||||
// given - custom category with no model, no systemDefaultModel, no available models
|
||||
|
||||
Reference in New Issue
Block a user