fix: make command loading resilient to MCP/Skill failures by anandgupta42 · Pull Request #36 · AltimateAI/altimate-code

anandgupta42 · 2026-03-04T23:01:23Z

Summary

Wrap MCP prompt loading and Skill loading in individual try/catch blocks in command/index.ts so that default commands (init, discover, review) are always served even when plugins fail
Add safe() error wrapper around each non-blocking sync request in sync.tsx so one failing request doesn't silently prevent all others (including commands) from populating the TUI store
Add 22 tests covering command resilience, default command properties, and the safe wrapper pattern

Root Cause

When MCP.prompts() or Skill.all() threw during command state initialization, the entire state promise rejected — all default commands including /discover disappeared from the TUI. Additionally, the non-blocking Promise.all in sync.tsx was fire-and-forget with no error handler, so failures were silently swallowed and setStore("status", "complete") never ran.

Changes

File	Change
`src/command/index.ts`	Wrap MCP and Skill loading in try/catch
`src/cli/cmd/tui/context/sync.tsx`	Add safe() wrapper for individual error handling
`test/command/command-resilience.test.ts`	15 tests for command defaults, properties, and resilience pattern
`test/command/sync-resilience.test.ts`	7 tests for the safe() wrapper pattern

Test plan

All 1388 existing tests pass
22 new tests pass covering:
- Default commands (init, discover, review) always present
- Discover command has correct metadata and template
- Resilience pattern: MCP failure, Skill failure, both failures
- Skills can't overwrite default commands
- safe() wrapper: individual failures, all failures, non-Error rejections
- Contrasts old behavior (Promise.all rejects) vs new (always resolves)

🤖 Generated with Claude Code

jontsai

LGTM

Clean resilience fix. The root cause is well-identified and the fix is minimal:

✓ safe() wrapper in sync.tsx — elegant pattern, each request fails independently
✓ try/catch around MCP.prompts() and Skill.all() — default commands always available
✓ Good test coverage (22 new tests) including the failure-mode contrasts

Ship it! 🚢

kulvirgit · 2026-03-04T23:44:27Z

Multi-Model Code Review — 8/8 Models

Verdict: APPROVE (with suggestions)

Severity	Count
Critical	0
Major	1
Minor	3
Nit	2

Major Issues

1. Silent catch blocks lose diagnostic information — command/index.ts:138-140, 157-159

The catch blocks for MCP and Skill loading swallow all errors without logging. When MCP or Skills fail silently, debugging is impossible — operators have no visibility into why commands aren't appearing.

Fix:

} catch (e) {
  Log.Default.warn("MCP prompt loading failed, continuing with defaults", {
    error: e instanceof Error ? e.message : String(e),
  })
}

Same for the Skill catch block at line 157.

Flagged by 7/8 models

Minor Issues

2. Status transitions to "complete" even when all non-blocking requests fail — sync.tsx:422-424

The .then(() => setStore("status", "complete")) runs unconditionally after Promise.all with safe() wrappers. If all 10 non-blocking requests fail, the store shows "complete" with empty data. Consider tracking failures and setting status to "partial" instead.

Flagged by 4/8 models

3. MCP commands can silently overwrite default commands — command/index.ts:112-137

MCP prompts are applied after defaults and can overwrite init, discover, or review. Skills have a guard (if (result[skill.name]) continue) but MCP does not. Either add the same guard for MCP, or log a warning when an MCP prompt shadows a default.

Flagged by 3/8 models

4. Generic log message in safe() lacks request identification — sync.tsx:404-408

All failed requests log the same "non-blocking sync request failed" message. Consider adding a label parameter so failures are distinguishable in logs.

Flagged by 3/8 models

Nits

5. Non-null assertions (x.data!) inside safe() wrappers — sync.tsx:412-421

Pre-existing pattern, but x.data ?? defaultValue would be more defensive inside safe() wrappers.

6. Pre-existing async Promise executor anti-pattern — command/index.ts:119

new Promise<string>(async (resolve, reject) => ...) where .catch(reject) followed by resolve() can call both. Not introduced by this PR — noting for future cleanup.

Positive Observations

Clean separation of concerns — MCP and Skill loading wrapped independently (7/8)
Excellent safe() pattern — generic helper captures error handling cleanly (6/8)
Thorough test coverage — 22 tests across 2 files covering all failure modes (8/8)
Minimal blast radius — surgical fix, only wraps what needs protection (5/8)

Reviewed by 8 models: Claude, Codex, Gemini, Kimi, Grok, MiniMax, GLM-5, Qwen

setu-altimateai

PR #36 Review Tour Guide

Summary

Clean resilience fix for a real bug: when MCP.prompts() or Skill.all() threw during command state initialization, the entire state promise rejected and default commands (/init, /discover, /review) disappeared from the TUI. Additionally, the fire-and-forget Promise.all in sync.tsx had no error handling, so setStore("status", "complete") never ran on failure.

File Order

#	File	+/-	Purpose
1	`src/command/index.ts`	+45/-35	Core fix — wrap MCP/Skill loading in try/catch
2	`src/cli/cmd/tui/context/sync.tsx`	+18/-12	Core fix — safe() wrapper for non-blocking requests
3	`test/command/command-resilience.test.ts`	+246	Tests for command defaults and resilience pattern
4	`test/command/sync-resilience.test.ts`	+137	Tests for safe() wrapper pattern

Review Assessment

Risk level: Low — defensive error handling only, no behavioral changes on happy path
Confidence: High

Considerations

MCP can overwrite defaults but Skills can't — this is by design (MCP loop runs before Skills loop with if (result[skill.name]) continue), but it means an MCP prompt named "discover" would shadow the built-in. Worth noting in inline comment.
Bare catch {} blocks — the command/index.ts catch blocks swallow errors silently. The sync.tsx safe() logs warnings, which is better. Consider adding logging to the command/index.ts catches too for debuggability.
Test approach — the resilience tests in command-resilience.test.ts duplicate the loading logic rather than mocking the real module. This is pragmatic (avoids complex module mocks) but means the tests could drift from the real implementation. Acceptable tradeoff.

Verdict

LGTM — well-scoped fix with thorough test coverage. The root cause analysis in the PR description is excellent.

setu-altimateai · 2026-03-05T00:08:52Z

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

-        description: prompt.description,
-        get template() {
-          // since a getter can't be async we need to manually return a promise here
-          return new Promise<string>(async (resolve, reject) => {


praise: Clean pattern — wrapping MCP and Skill loading separately means one can fail independently of the other. Default commands are always served regardless.

setu-altimateai · 2026-03-05T00:08:52Z

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

+              const template = await MCP.getPrompt(
+                prompt.client,
+                prompt.name,
+                prompt.arguments


suggestion: The bare catch {} blocks silently swallow errors. Consider adding a Log.Default.warn() here (like you did with safe() in sync.tsx) so MCP/Skill failures are at least visible in debug logs.

} catch (e) { Log.Default.warn("MCP prompt loading failed", { error: e instanceof Error ? e.message : String(e), }) }

setu-altimateai · 2026-03-05T00:08:52Z

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

+            })
+          },
+          hints: prompt.arguments?.map((_, i) => `$${i + 1}`) ?? [],
+        }


thought: Note that MCP prompts can overwrite default commands (same name = overwrite), but Skills can't (the if (result[skill.name]) continue guard). This asymmetry is probably intentional but worth a brief comment for future readers.

setu-altimateai · 2026-03-05T00:08:52Z

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

+              error: e instanceof Error ? e.message : String(e),
+            })
+          })
          Promise.all([


praise: Nice safe() wrapper — elegant and minimal. The generic <T,> preserves type info, and logging the error message (with instanceof Error check) is the right level of detail for non-blocking failures.

setu-altimateai · 2026-03-05T00:08:52Z

packages/altimate-code/test/command/command-resilience.test.ts

+        config: {
+          command: {
+            "my-custom": {
+              description: "Custom command",


nit: These resilience tests duplicate the loading logic from command/index.ts rather than testing the real module with mocked MCP/Skill dependencies. This is pragmatic (avoids complex module mocking) but means the test's loadCommands() could drift from the real implementation over time. A comment noting this tradeoff would help future maintainers.

setu-altimateai · 2026-03-05T00:08:52Z

packages/altimate-code/test/command/sync-resilience.test.ts

+    warnings.length = 0
+    let statusSet = false
+
+    await Promise.all([


praise: Great test — explicitly contrasts old behavior (Promise.all rejects, status never set) vs new behavior (safe() catches, status always reaches "complete"). This makes the bug fix crystal clear.

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>

- 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>

* 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>

jontsai approved these changes Mar 4, 2026

View reviewed changes

setu-altimateai bot approved these changes Mar 5, 2026

View reviewed changes

anandgupta42 and others added 2 commits March 4, 2026 17:01

anandgupta42 force-pushed the fix/command-loading-resilience branch from a0c8b53 to c8c1fe1 Compare March 5, 2026 01:02

anandgupta42 merged commit 30b8077 into main Mar 5, 2026
4 checks passed

kulvirgit mentioned this pull request Mar 5, 2026

feat: comprehensive telemetry instrumentation #39

Merged

6 tasks

kulvirgit deleted the fix/command-loading-resilience branch March 10, 2026 21:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: make command loading resilient to MCP/Skill failures#36

fix: make command loading resilient to MCP/Skill failures#36
anandgupta42 merged 2 commits intomainfrom
fix/command-loading-resilience

anandgupta42 commented Mar 4, 2026

Uh oh!

jontsai left a comment

Uh oh!

kulvirgit commented Mar 4, 2026

Uh oh!

setu-altimateai bot left a comment

Uh oh!

setu-altimateai bot Mar 5, 2026

Uh oh!

setu-altimateai bot Mar 5, 2026

Uh oh!

setu-altimateai bot Mar 5, 2026

Uh oh!

setu-altimateai bot Mar 5, 2026

Uh oh!

setu-altimateai bot Mar 5, 2026

Uh oh!

setu-altimateai bot Mar 5, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

anandgupta42 commented Mar 4, 2026

Summary

Root Cause

Changes

Test plan

Uh oh!

jontsai left a comment

Choose a reason for hiding this comment

Uh oh!

kulvirgit commented Mar 4, 2026

Multi-Model Code Review — 8/8 Models

Major Issues

Minor Issues

Nits

Positive Observations

Uh oh!

setu-altimateai bot left a comment

Choose a reason for hiding this comment

PR #36 Review Tour Guide

Summary

File Order

Review Assessment

Considerations

Verdict

Uh oh!

setu-altimateai bot Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

setu-altimateai bot Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

setu-altimateai bot Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

setu-altimateai bot Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

setu-altimateai bot Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

setu-altimateai bot Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants