Skip to content

Commit 30b8077

Browse files
anandgupta42claude
andauthored
fix: make command loading resilient to MCP/Skill failures (#36)
* fix: make command loading resilient to MCP/Skill failures Two issues could cause /discover and other server commands to not show up in the TUI: 1. In command/index.ts, if MCP.prompts() or Skill.all() threw during command state initialization, the entire state promise rejected — all default commands (init, discover, review) were lost. Now each is wrapped in try/catch so defaults are always served. 2. In sync.tsx, the non-blocking Promise.all for loading commands, LSP, MCP status etc. was fire-and-forget with no error handler. If any request failed, errors were silently swallowed and setStore("status", "complete") never ran. Now each request catches errors individually and logs a warning, allowing the others to succeed. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: address PR review feedback — add logging and comments - Add Log.Default.warn() to MCP and Skill catch blocks so failures are visible in debug logs instead of being silently swallowed - Add comment documenting MCP/Skills overwrite asymmetry (MCP can overwrite defaults by name, skills cannot due to guard clause) - Add comment noting resilience tests duplicate loading logic and may need updating if the real implementation changes Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
1 parent d8abf88 commit 30b8077

File tree

4 files changed

+456
-47
lines changed

4 files changed

+456
-47
lines changed

packages/altimate-code/src/cli/cmd/tui/context/sync.tsx

Lines changed: 18 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -399,20 +399,26 @@ export const { use: useSync, provider: SyncProvider } = createSimpleContext({
399399
})
400400
.then(() => {
401401
if (store.status !== "complete") setStore("status", "partial")
402-
// non-blocking
402+
// non-blocking — each request catches errors individually so one
403+
// failure doesn't prevent the others from populating the store.
404+
const safe = <T,>(p: Promise<T>) => p.catch((e) => {
405+
Log.Default.warn("non-blocking sync request failed", {
406+
error: e instanceof Error ? e.message : String(e),
407+
})
408+
})
403409
Promise.all([
404-
...(args.continue ? [] : [sessionListPromise.then((sessions) => setStore("session", reconcile(sessions)))]),
405-
sdk.client.command.list().then((x) => setStore("command", reconcile(x.data ?? []))),
406-
sdk.client.lsp.status().then((x) => setStore("lsp", reconcile(x.data!))),
407-
sdk.client.mcp.status().then((x) => setStore("mcp", reconcile(x.data!))),
408-
sdk.client.experimental.resource.list().then((x) => setStore("mcp_resource", reconcile(x.data ?? {}))),
409-
sdk.client.formatter.status().then((x) => setStore("formatter", reconcile(x.data!))),
410-
sdk.client.session.status().then((x) => {
410+
...(args.continue ? [] : [safe(sessionListPromise.then((sessions) => setStore("session", reconcile(sessions))))]),
411+
safe(sdk.client.command.list().then((x) => setStore("command", reconcile(x.data ?? [])))),
412+
safe(sdk.client.lsp.status().then((x) => setStore("lsp", reconcile(x.data!)))),
413+
safe(sdk.client.mcp.status().then((x) => setStore("mcp", reconcile(x.data!)))),
414+
safe(sdk.client.experimental.resource.list().then((x) => setStore("mcp_resource", reconcile(x.data ?? {})))),
415+
safe(sdk.client.formatter.status().then((x) => setStore("formatter", reconcile(x.data!)))),
416+
safe(sdk.client.session.status().then((x) => {
411417
setStore("session_status", reconcile(x.data!))
412-
}),
413-
sdk.client.provider.auth().then((x) => setStore("provider_auth", reconcile(x.data ?? {}))),
414-
sdk.client.vcs.get().then((x) => setStore("vcs", reconcile(x.data))),
415-
sdk.client.path.get().then((x) => setStore("path", reconcile(x.data!))),
418+
})),
419+
safe(sdk.client.provider.auth().then((x) => setStore("provider_auth", reconcile(x.data ?? {})))),
420+
safe(sdk.client.vcs.get().then((x) => setStore("vcs", reconcile(x.data)))),
421+
safe(sdk.client.path.get().then((x) => setStore("path", reconcile(x.data!)))),
416422
]).then(() => {
417423
setStore("status", "complete")
418424
})

packages/altimate-code/src/command/index.ts

Lines changed: 52 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import PROMPT_DISCOVER from "./template/discover.txt"
88
import PROMPT_REVIEW from "./template/review.txt"
99
import { MCP } from "../mcp"
1010
import { Skill } from "../skill"
11+
import { Log } from "../util/log"
1112

1213
export namespace Command {
1314
export const Event = {
@@ -106,46 +107,62 @@ export namespace Command {
106107
hints: hints(command.template),
107108
}
108109
}
109-
for (const [name, prompt] of Object.entries(await MCP.prompts())) {
110-
result[name] = {
111-
name,
112-
source: "mcp",
113-
description: prompt.description,
114-
get template() {
115-
// since a getter can't be async we need to manually return a promise here
116-
return new Promise<string>(async (resolve, reject) => {
117-
const template = await MCP.getPrompt(
118-
prompt.client,
119-
prompt.name,
120-
prompt.arguments
121-
? // substitute each argument with $1, $2, etc.
122-
Object.fromEntries(prompt.arguments?.map((argument, i) => [argument.name, `$${i + 1}`]))
123-
: {},
124-
).catch(reject)
125-
resolve(
126-
template?.messages
127-
.map((message) => (message.content.type === "text" ? message.content.text : ""))
128-
.join("\n") || "",
129-
)
130-
})
131-
},
132-
hints: prompt.arguments?.map((_, i) => `$${i + 1}`) ?? [],
110+
// MCP and skill loading must not prevent default commands from being served.
111+
// Wrap each in try/catch so init, discover, review are always available.
112+
// Note: MCP prompts can overwrite defaults (by name), but skills cannot
113+
// (the `if (result[skill.name]) continue` guard preserves defaults over skills).
114+
try {
115+
for (const [name, prompt] of Object.entries(await MCP.prompts())) {
116+
result[name] = {
117+
name,
118+
source: "mcp",
119+
description: prompt.description,
120+
get template() {
121+
// since a getter can't be async we need to manually return a promise here
122+
return new Promise<string>(async (resolve, reject) => {
123+
const template = await MCP.getPrompt(
124+
prompt.client,
125+
prompt.name,
126+
prompt.arguments
127+
? // substitute each argument with $1, $2, etc.
128+
Object.fromEntries(prompt.arguments?.map((argument, i) => [argument.name, `$${i + 1}`]))
129+
: {},
130+
).catch(reject)
131+
resolve(
132+
template?.messages
133+
.map((message) => (message.content.type === "text" ? message.content.text : ""))
134+
.join("\n") || "",
135+
)
136+
})
137+
},
138+
hints: prompt.arguments?.map((_, i) => `$${i + 1}`) ?? [],
139+
}
133140
}
141+
} catch (e) {
142+
Log.Default.warn("MCP prompt loading failed, continuing with defaults", {
143+
error: e instanceof Error ? e.message : String(e),
144+
})
134145
}
135146

136147
// Add skills as invokable commands
137-
for (const skill of await Skill.all()) {
138-
// Skip if a command with this name already exists
139-
if (result[skill.name]) continue
140-
result[skill.name] = {
141-
name: skill.name,
142-
description: skill.description,
143-
source: "skill",
144-
get template() {
145-
return skill.content
146-
},
147-
hints: [],
148+
try {
149+
for (const skill of await Skill.all()) {
150+
// Skip if a command with this name already exists
151+
if (result[skill.name]) continue
152+
result[skill.name] = {
153+
name: skill.name,
154+
description: skill.description,
155+
source: "skill",
156+
get template() {
157+
return skill.content
158+
},
159+
hints: [],
160+
}
148161
}
162+
} catch (e) {
163+
Log.Default.warn("Skill loading failed, continuing with defaults", {
164+
error: e instanceof Error ? e.message : String(e),
165+
})
149166
}
150167

151168
return result
Lines changed: 249 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,249 @@
1+
import { describe, test, expect } from "bun:test"
2+
import { Command } from "../../src/command/index"
3+
import { Instance } from "../../src/project/instance"
4+
import { tmpdir } from "../fixture/fixture"
5+
6+
// ---------------------------------------------------------------------------
7+
// Helpers
8+
// ---------------------------------------------------------------------------
9+
10+
async function withInstance(fn: () => Promise<void>) {
11+
await using tmp = await tmpdir({ git: true })
12+
await Instance.provide({ directory: tmp.path, fn })
13+
}
14+
15+
// ---------------------------------------------------------------------------
16+
// Tests: Default commands are always available and correctly configured
17+
// ---------------------------------------------------------------------------
18+
19+
describe("Command module", () => {
20+
describe("default commands", () => {
21+
test("init, discover, review are always present", async () => {
22+
await withInstance(async () => {
23+
const commands = await Command.list()
24+
const names = commands.map((c) => c.name)
25+
expect(names).toContain("init")
26+
expect(names).toContain("discover")
27+
expect(names).toContain("review")
28+
})
29+
})
30+
31+
test("discover command has correct metadata", async () => {
32+
await withInstance(async () => {
33+
const cmd = await Command.get("discover")
34+
expect(cmd).toBeDefined()
35+
expect(cmd.name).toBe("discover")
36+
expect(cmd.source).toBe("command")
37+
expect(cmd.description).toBe("scan data stack and set up connections")
38+
})
39+
})
40+
41+
test("discover template references project_scan tool", async () => {
42+
await withInstance(async () => {
43+
const cmd = await Command.get("discover")
44+
const template = await cmd.template
45+
expect(template).toContain("project_scan")
46+
})
47+
})
48+
49+
test("discover template has $ARGUMENTS placeholder", async () => {
50+
await withInstance(async () => {
51+
const cmd = await Command.get("discover")
52+
expect(cmd.hints).toContain("$ARGUMENTS")
53+
})
54+
})
55+
56+
test("init command has correct metadata", async () => {
57+
await withInstance(async () => {
58+
const cmd = await Command.get("init")
59+
expect(cmd).toBeDefined()
60+
expect(cmd.name).toBe("init")
61+
expect(cmd.source).toBe("command")
62+
expect(cmd.description).toBe("create/update AGENTS.md")
63+
})
64+
})
65+
66+
test("review command has subtask flag", async () => {
67+
await withInstance(async () => {
68+
const cmd = await Command.get("review")
69+
expect(cmd).toBeDefined()
70+
expect(cmd.name).toBe("review")
71+
expect(cmd.subtask).toBe(true)
72+
expect(cmd.source).toBe("command")
73+
})
74+
})
75+
76+
test("all default commands have source 'command'", async () => {
77+
await withInstance(async () => {
78+
const commands = await Command.list()
79+
const defaults = commands.filter((c) => ["init", "discover", "review"].includes(c.name))
80+
expect(defaults.length).toBe(3)
81+
for (const cmd of defaults) {
82+
expect(cmd.source).toBe("command")
83+
}
84+
})
85+
})
86+
87+
test("Command.get returns undefined for non-existent commands", async () => {
88+
await withInstance(async () => {
89+
const cmd = await Command.get("nonexistent-command-12345")
90+
expect(cmd).toBeUndefined()
91+
})
92+
})
93+
})
94+
95+
describe("user-defined commands from config", () => {
96+
test("config commands are loaded alongside defaults", async () => {
97+
await using tmp = await tmpdir({
98+
git: true,
99+
config: {
100+
command: {
101+
"my-custom": {
102+
description: "Custom command",
103+
template: "Do something custom with $1",
104+
},
105+
},
106+
},
107+
})
108+
await Instance.provide({
109+
directory: tmp.path,
110+
fn: async () => {
111+
const commands = await Command.list()
112+
const names = commands.map((c) => c.name)
113+
// Defaults still present
114+
expect(names).toContain("init")
115+
expect(names).toContain("discover")
116+
expect(names).toContain("review")
117+
// Custom command also present
118+
expect(names).toContain("my-custom")
119+
const custom = await Command.get("my-custom")
120+
expect(custom.source).toBe("command")
121+
expect(custom.description).toBe("Custom command")
122+
},
123+
})
124+
})
125+
})
126+
})
127+
128+
// ---------------------------------------------------------------------------
129+
// Tests: Resilient loading pattern (isolated from module mocks)
130+
//
131+
// These tests verify the try/catch pattern used in command/index.ts to ensure
132+
// MCP and Skill failures don't prevent default commands from being served.
133+
// ---------------------------------------------------------------------------
134+
135+
describe("Command loading resilience pattern", () => {
136+
// NOTE: These tests duplicate the loading logic from command/index.ts rather than
137+
// mocking the real MCP/Skill modules. This avoids complex module mocking but means
138+
// loadCommands() below could drift from the real implementation. If the loading
139+
// pattern in command/index.ts changes, these tests should be updated to match.
140+
async function loadCommands(opts: {
141+
mcpPrompts: () => Promise<Record<string, any>>
142+
skillAll: () => Promise<Array<{ name: string; description: string; content: string }>>
143+
}) {
144+
const result: Record<string, { name: string; source: string; description?: string }> = {
145+
init: { name: "init", source: "command", description: "create/update AGENTS.md" },
146+
discover: { name: "discover", source: "command", description: "scan data stack" },
147+
review: { name: "review", source: "command", description: "review changes" },
148+
}
149+
150+
// This matches the pattern in command/index.ts
151+
try {
152+
for (const [name, prompt] of Object.entries(await opts.mcpPrompts())) {
153+
result[name] = { name, source: "mcp", description: (prompt as any).description }
154+
}
155+
} catch {
156+
// MCP prompt loading failed — continue with default commands
157+
}
158+
159+
try {
160+
for (const skill of await opts.skillAll()) {
161+
if (result[skill.name]) continue
162+
result[skill.name] = { name: skill.name, source: "skill", description: skill.description }
163+
}
164+
} catch {
165+
// Skill loading failed — continue with default commands
166+
}
167+
168+
return Object.values(result)
169+
}
170+
171+
test("all plugins healthy — defaults + plugins present", async () => {
172+
const commands = await loadCommands({
173+
mcpPrompts: async () => ({ "mcp-cmd": { description: "MCP prompt" } }),
174+
skillAll: async () => [{ name: "my-skill", description: "A skill", content: "" }],
175+
})
176+
const names = commands.map((c) => c.name)
177+
expect(names).toContain("init")
178+
expect(names).toContain("discover")
179+
expect(names).toContain("review")
180+
expect(names).toContain("mcp-cmd")
181+
expect(names).toContain("my-skill")
182+
})
183+
184+
test("MCP throws — defaults + skills still present", async () => {
185+
const commands = await loadCommands({
186+
mcpPrompts: async () => {
187+
throw new Error("MCP server unavailable")
188+
},
189+
skillAll: async () => [{ name: "my-skill", description: "A skill", content: "" }],
190+
})
191+
const names = commands.map((c) => c.name)
192+
expect(names).toContain("init")
193+
expect(names).toContain("discover")
194+
expect(names).toContain("review")
195+
expect(names).toContain("my-skill")
196+
expect(commands.filter((c) => c.source === "mcp").length).toBe(0)
197+
})
198+
199+
test("Skills throws — defaults + MCP still present", async () => {
200+
const commands = await loadCommands({
201+
mcpPrompts: async () => ({ "mcp-cmd": { description: "MCP prompt" } }),
202+
skillAll: async () => {
203+
throw new Error("Skill discovery failed")
204+
},
205+
})
206+
const names = commands.map((c) => c.name)
207+
expect(names).toContain("init")
208+
expect(names).toContain("discover")
209+
expect(names).toContain("review")
210+
expect(names).toContain("mcp-cmd")
211+
expect(commands.filter((c) => c.source === "skill").length).toBe(0)
212+
})
213+
214+
test("BOTH throw — only defaults present", async () => {
215+
const commands = await loadCommands({
216+
mcpPrompts: async () => {
217+
throw new Error("MCP server unavailable")
218+
},
219+
skillAll: async () => {
220+
throw new Error("Skill discovery failed")
221+
},
222+
})
223+
const names = commands.map((c) => c.name)
224+
expect(names).toContain("init")
225+
expect(names).toContain("discover")
226+
expect(names).toContain("review")
227+
expect(commands.length).toBe(3)
228+
})
229+
230+
test("skill with same name as default is skipped", async () => {
231+
const commands = await loadCommands({
232+
mcpPrompts: async () => ({}),
233+
skillAll: async () => [{ name: "discover", description: "Rogue skill", content: "" }],
234+
})
235+
const discover = commands.find((c) => c.name === "discover")!
236+
expect(discover.source).toBe("command")
237+
expect(discover.description).toBe("scan data stack")
238+
})
239+
240+
test("MCP command can overwrite default (same name)", async () => {
241+
const commands = await loadCommands({
242+
mcpPrompts: async () => ({ discover: { description: "MCP discover" } }),
243+
skillAll: async () => [],
244+
})
245+
const discover = commands.find((c) => c.name === "discover")!
246+
// MCP overwrites default because it's applied after defaults
247+
expect(discover.source).toBe("mcp")
248+
})
249+
})

0 commit comments

Comments
 (0)