feat: MCP client and BYOK local mode (Harness Phase 02)#14
Conversation
CLI sends discovered MCP tool definitions in the chat submit body; the server deserializes them into dynamicTool records alongside built-in contracts without running MCP on the server, preserving Phase 11 local-only execution. Co-authored-by: Cursor <cursoragent@cursor.com>
Introduce mcp.json merge loader, stdio/HTTP/SSE transports, McpManager lifecycle with hot-reload, mcp__ tool naming, and executeMcpToolCall with TUI write approval. Co-authored-by: Cursor <cursoragent@cursor.com>
Support mocode --local with keys.json storage, /keys wizard triggers, per-project local session JSON, LocalChatTransport in-process streamText, and BYOK system prompt. Co-authored-by: Cursor <cursoragent@cursor.com>
Add /mcp and /keys slash commands, MCP/BYOK paths in use-chat, session mount MCP init, keys setup gate, and dialog integrations for MCP management and approvals. Co-authored-by: Cursor <cursoragent@cursor.com>
Add AGENTS.md and DESIGN.md as CLI UI source of truth and extend agent-permissions with MCP write approval behavior aligned with bash TUI gates. Co-authored-by: Cursor <cursoragent@cursor.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
railway-app
Bot
temporarily deployed
to
lavish-courage / MoCode-TUI-pr-14
|
Warning Review limit reached
More reviews will be available in 43 minutes and 28 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughAdds design-system guidance, local BYOK state, MCP config and approval plumbing, and CLI/server chat wiring for local sessions and MCP tool schemas. ChangesBYOK and MCP Flow
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
🚅 Deployed to the MoCode-TUI-pr-14 environment in lavish-courage
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/server/src/routes/chat.ts (1)
67-85: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winEnforce the MCP tool namespace at the request boundary.
mcpToolsis client-controlled, butname: z.string()accepts any tool id. Combined with the later spread merge, a payload like{ name: "bash" }can shadow a built-in contract fromgetToolContracts(mode), and the CLI will never execute that dynamic non-MCP tool.Proposed fix
/** Wire payload for one MCP tool schema — mirrors CLI SerializedMcpTool (no execute fn). */ const mcpToolSchema = z.object({ - name: z.string(), + name: z.string().regex(/^mcp__[^_].+__.+$/, "Invalid MCP tool name"), description: z.string().optional(), inputSchema: z.unknown().optional(), });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/server/src/routes/chat.ts` around lines 67 - 85, The submitSchema mcpTools validation currently allows any name, which lets client input collide with built-in tool contracts later merged in chat.ts. Tighten the mcpToolSchema/name check so only MCP-namespaced tool ids are accepted at the request boundary, and reject or filter any non-MCP names before they reach getToolContracts(mode) and the spread merge. Use the existing submitSchema and mcpToolSchema symbols to keep the fix localized.
🟠 Major comments (24)
packages/cli/src/screens/new-session.tsx-49-52 (1)
49-52: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDon't default unknown model ids to Anthropic.
newSessionStateSchemaaccepts any string, but this branch silently maps an unknownstate.modelto Anthropic. In local mode that opens the wrong keys flow and still carries an unsupported model id into the session screen.Proposed fix
if (isLocalMode()) { - const provider = findSupportedChatModel(state.model)?.provider ?? "anthropic"; + const model = findSupportedChatModel(state.model); + if (!model) { + throw new Error(`Unsupported chat model: ${state.model}`); + } + const provider = model.provider; if (!hasRequiredKeys(provider)) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/screens/new-session.tsx` around lines 49 - 52, The local-mode flow in new-session.tsx is defaulting unknown state.model values to anthropic via findSupportedChatModel(...)?provider ?? "anthropic", which can open the wrong keys wizard and pass an unsupported model into the session. Update the branch around isLocalMode() to first validate the model with findSupportedChatModel, only derive provider when the model is recognized, and otherwise handle the unknown model explicitly instead of falling back to Anthropic; keep the fix localized to the newSession flow and openKeysWizardIfNeeded call site.packages/cli/src/lib/local-sessions.ts-56-64 (1)
56-64: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winOnly treat a missing index file as empty.
This catch-all turns malformed JSON and permission failures into
{ sessions: [] }.createLocalSession()then writes that empty view back out, which drops every existing entry fromsessions-index.json.Proposed fix
function readIndex(projectDir: string): SessionsIndex { const indexPath = join(projectDir, "sessions-index.json"); try { const data = readFileSync(indexPath, "utf-8"); const parsed = JSON.parse(data) as SessionsIndex; return { sessions: Array.isArray(parsed.sessions) ? parsed.sessions : [] }; - } catch { + } catch (error) { + if ((error as NodeJS.ErrnoException).code === "ENOENT") { + return { sessions: [] }; + } + throw error; - return { sessions: [] }; } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/lib/local-sessions.ts` around lines 56 - 64, The readIndex() helper is swallowing all read/parse failures and treating them as an empty session list, which can overwrite a bad or inaccessible sessions-index.json in createLocalSession(). Update readIndex() to only return { sessions: [] } when the index file is missing, and rethrow or surface other errors such as malformed JSON or permission issues so existing data is not silently lost.packages/cli/src/lib/local-sessions.ts-40-47 (1)
40-47: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winUse a collision-free project directory key.
cwd.replace(/\//g, "-")is not injective (/foo/barand/foo-barboth normalize to-foo-bar), and it also leaves Windows drive/separator characters intact. Different projects can therefore share one storage directory, or fail to create one at all.Proposed fix
-import { randomUUID } from "node:crypto"; +import { createHash, randomUUID } from "node:crypto"; @@ -import { join } from "node:path"; +import { join, resolve } from "node:path"; @@ export function normalizeProjectPath(cwd: string): string { - return cwd.replace(/\//g, "-"); + return createHash("sha256").update(resolve(cwd)).digest("hex"); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/lib/local-sessions.ts` around lines 40 - 47, The project directory key generated by normalizeProjectPath is collision-prone and platform-specific because it only replaces forward slashes, so update normalizeProjectPath and its use in resolveProjectDir to produce a stable, collision-free key from the full cwd. Use a deterministic encoding or hash of the absolute path that safely handles path separators, drive letters, and special characters, so different cwd values cannot map to the same projectsDir subdirectory.packages/cli/src/lib/session-navigation.ts-14-14 (1)
14-14: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
localis still ineffective whilesessionremains required.
packages/cli/src/screens/session.tsxonly consultsprefetched?.localafter the early return forprefetched?.session. Since this schema still requiressession, a state like{ local: true }never parses, and a parsed state always hassession, so the new flag never actually drives the local-session branch. Makesessionoptional here or model the state as a union solocalcan stand on its own.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/lib/session-navigation.ts` at line 14, The session-navigation schema still makes local unusable because `session` is required, so the `local` flag can never independently reach the local-session branch in `session.tsx`. Update the schema around `local` and `session` in `session-navigation.ts` so `session` is optional or the state is represented as a union, then ensure the parsed result can be `{ local: true }` without requiring `session`.packages/cli/src/lib/keys.ts-63-67 (1)
63-67: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRe-apply restrictive modes for existing key files/directories.
saveKeys()only sets0o700/0o600when creating the path, so an existing~/.mocodeorkeys.jsonwith broader permissions stays readable. AddchmodSync(dir, 0o700)andchmodSync(keysFile, 0o600)after writing.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/lib/keys.ts` around lines 63 - 67, In saveKeys() within keys.ts, the restrictive permissions are only applied on creation, so existing ~/.mocode directories or keys.json files may keep broader modes. Update the save flow to re-apply permissions after the write by calling chmodSync on both the dir and keysFile, using 0o700 for the directory and 0o600 for the key file, so existing paths are tightened as well.packages/cli/src/lib/keys-wizard-trigger.test.ts-10-29 (1)
10-29: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winKeep these tests out of the real home directory.
They currently write to
~/.mocode-test-keys-wizardand then recursively delete it, which can remove pre-existing local data and makes concurrent runs fight over the same path. Use a unique temp directory per test run instead.Proposed fix
-import { existsSync, mkdirSync, rmSync } from "node:fs"; -import { homedir } from "node:os"; +import { mkdtempSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; import { join } from "node:path"; @@ -const TEST_DIR = join(homedir(), ".mocode-test-keys-wizard"); +let testDir: string; @@ beforeEach(() => { setLocalMode(false); - if (!existsSync(TEST_DIR)) { - mkdirSync(TEST_DIR, { recursive: true, mode: 0o700 }); - } + testDir = mkdtempSync(join(tmpdir(), "mocode-test-keys-wizard-")); }); @@ afterEach(() => { setLocalMode(false); - if (existsSync(TEST_DIR)) { - rmSync(TEST_DIR, { recursive: true }); - } + rmSync(testDir, { recursive: true, force: true }); });Then replace each
TEST_DIRuse withtestDir.Also applies to: 51-63
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/lib/keys-wizard-trigger.test.ts` around lines 10 - 29, The tests in shouldAutoOpenKeysWizard are using a fixed directory under the real home folder, which can clobber local data and collide across runs; update the setup/teardown in keys-wizard-trigger.test.ts to create a unique per-run temporary directory instead of TEST_DIR. Use that temp path consistently wherever the test currently references TEST_DIR (including the later uses mentioned in the review) so the suite isolates its filesystem writes and cleanup.packages/cli/src/components/dialogs/mcp-approval-dialog.tsx-32-53 (1)
32-53: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winBring the approval buttons onto the DESIGN token system.
Lines 37-50 hardcode padding plus
"black","white", and"gray", so this dialog is bypassing the shared terminal theme contract. Use semantic theme colors and DESIGN spacing tokens here instead of local literals. As per coding guidelines, "Use theme-driven colors by resolving semantic tokens throughuseTheme()" and "Use onlyDESIGN.mdtokens such as{color.*},{glyph.*},{space.*}, and{size.*}." Based on learnings,DESIGN.mdtakes precedence when UI specs conflict.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/components/dialogs/mcp-approval-dialog.tsx` around lines 32 - 53, The ActionButton in mcp-approval-dialog is bypassing the shared DESIGN token system by hardcoding spacing and terminal colors. Update this component to use semantic theme colors resolved through useTheme() instead of literal black/white/gray, and replace the local padding value with the appropriate DESIGN spacing token. Keep the change localized to ActionButton so the approval buttons follow the terminal theme contract.Sources: Coding guidelines, Learnings
packages/cli/src/components/dialogs/mcp-dialog.tsx-24-27 (1)
24-27: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winMove
/mcpsizing and colors to DESIGN tokens before this lands.This dialog currently hardcodes width/padding constants plus
"black","white", and"gray"text colors, which bypasses the shared terminal design contract and makes future theme/mode changes harder to propagate. Pull these values from the theme/DESIGN token set instead of local literals. As per coding guidelines, "Use theme-driven colors by resolving semantic tokens throughuseTheme()" and "Use onlyDESIGN.mdtokens such as{color.*},{glyph.*},{space.*}, and{size.*}." Based on learnings,DESIGN.mdtakes precedence when UI specs conflict.Also applies to: 96-97, 251-267
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/components/dialogs/mcp-dialog.tsx` around lines 24 - 27, The MCP dialog is using local size constants and hardcoded text colors instead of shared design tokens. Update `McpDialog` to source width/padding/row sizing from `DESIGN.md` token values and resolve text colors through `useTheme()` semantic tokens rather than literal black/white/gray, including the color usages in the dialog rendering and item rows. Keep the existing `MCP_ROW_HEIGHT`, `MCP_LIST_MAX_ITEMS`, `DIALOG_PADDING_X`, and `DIALOG_MAX_WIDTH` behavior aligned with the theme-driven token set while removing any direct literals.Sources: Coding guidelines, Learnings
packages/cli/src/components/dialogs/mcp-approval-dialog.tsx-67-99 (1)
67-99: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winCap and scroll the serialized tool input.
requestMcpApproval()can hand this dialog arbitrarily large MCP arguments, but Lines 67-99 render the full pretty-printed JSON in a plain<text>block with no width or height cap. A large payload will expand the body and can push the action rows off-screen, which breaks the approval flow. As per coding guidelines, "Follow the dialog shell plus Width & Overflow rules: cap before truncating, avoid phantom scrollbars." Based on learnings,DESIGN.mdtakes precedence when UI specs conflict.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/components/dialogs/mcp-approval-dialog.tsx` around lines 67 - 99, The MCP approval dialog is rendering unbounded serialized input, which can make the action buttons disappear off-screen; update `McpApprovalDialog` so the `formattedInput` display is capped and scrollable instead of expanding the dialog. In the component that uses `requestMcpApproval()`, wrap the pretty-printed JSON in a container with explicit width/height constraints and overflow handling, and keep the action rows visible below the capped content. Use the existing `formattedInput`, `actions`, and `useKeyboard` flow as the location to adjust the layout without changing approval behavior.Sources: Coding guidelines, Learnings
packages/cli/src/components/dialogs/keys-wizard-dialog.tsx-46-68 (1)
46-68: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winReplace hardcoded TUI colors and spacing with DESIGN tokens.
Lines 52-65, 145-173, and 185-208 hardcode layout values plus
"black","white", and"gray", so this new dialog bypasses the shared terminal theme contract. Resolve semantic colors from the theme and move spacing/sizing to DESIGN tokens before using them here. As per coding guidelines, "Use theme-driven colors by resolving semantic tokens throughuseTheme()" and "Use onlyDESIGN.mdtokens such as{color.*},{glyph.*},{space.*}, and{size.*}." Based on learnings,DESIGN.mdtakes precedence when UI specs conflict.Also applies to: 145-173, 185-208
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/components/dialogs/keys-wizard-dialog.tsx` around lines 46 - 68, The new dialog in keys-wizard-dialog.tsx is bypassing the shared theme/design system by hardcoding TUI colors and layout values in ActionButton and the other dialog sections. Update the relevant components to resolve semantic colors through useTheme() instead of using literal "black", "white", and "gray", and replace fixed padding/spacing/sizing with DESIGN.md tokens. Apply the same theme/token-driven cleanup across ActionButton and the other dialog blocks referenced in this file so the whole dialog follows the shared terminal contract.Sources: Coding guidelines, Learnings
packages/cli/src/components/dialogs/keys-wizard-dialog.tsx-97-114 (1)
97-114: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winHandle
saveKeys()failures instead of letting the dialog throw.At Line 111,
saveKeys(nextKeys)can throw on validation or filesystem errors (packages/cli/src/lib/keys.ts:54-68). Right now that exception escapes the save action, so a permission or disk failure can turn/keysinto an unhandled TUI error instead of preserving the draft and showing feedback.Possible fix
+import { useToast } from "../../providers/toast"; ... export function KeysWizardDialogContent() { const { isTopLayer } = useKeyboardLayer(); + const { show } = useToast(); const [view, setView] = useState<WizardView>("list"); ... const handleSaveKey = useCallback(() => { if (!editingProvider) { return; } @@ - saveKeys(nextKeys); - setKeys(nextKeys); - handleBack(); - }, [keys, editingProvider, draftKey, handleBack]); + try { + saveKeys(nextKeys); + setKeys(nextKeys); + handleBack(); + } catch (error) { + show({ + variant: "error", + message: error instanceof Error ? error.message : "Failed to save API key", + }); + } + }, [keys, editingProvider, draftKey, handleBack, show]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/components/dialogs/keys-wizard-dialog.tsx` around lines 97 - 114, `handleSaveKey` in `keys-wizard-dialog.tsx` should not let `saveKeys(nextKeys)` exceptions escape and crash the TUI; wrap the save call in error handling, and only update local state or navigate back when the save succeeds. On failure, keep the current draft/editing state intact and surface the error through the dialog’s existing feedback path if available, using the `saveKeys` and `handleSaveKey` symbols to locate the change.packages/cli/src/mcp/heuristics.ts-17-18 (1)
17-18: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winValidate the full
mcp__<server>__<tool>shape before routing.Line 17 currently accepts any
mcp__*string.packages/cli/src/hooks/use-chat.ts:154andpackages/cli/src/lib/mcp-tool-call.ts:62-129use that as the gate beforeparseMcpToolName(...), so a malformed model-emitted name likemcp__badwill throw and abort MCP handling instead of being ignored.Proposed fix
export function isMcpToolName(name: string): boolean { - return name.startsWith(MCP_PREFIX); + if (!name.startsWith(MCP_PREFIX)) { + return false; + } + + const rest = name.slice(MCP_PREFIX.length); + const separator = rest.indexOf("__"); + return separator > 0 && separator < rest.length - 2; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/mcp/heuristics.ts` around lines 17 - 18, The MCP name gate in isMcpToolName is too broad and currently accepts any mcp__* string, which can let malformed names reach parseMcpToolName and throw. Tighten the validation in packages/cli/src/mcp/heuristics.ts so it only returns true for the full mcp__<server>__<tool> shape, and keep use-chat and mcp-tool-call relying on that helper as the pre-check before routing.packages/cli/src/mcp/manager.ts-315-319 (1)
315-319: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAwait the in-flight connect instead of returning early.
ensureConnected()awaitsconnectServer(), but Line 316 returns immediately when the same server is already connecting. A tool call that lands during startup/reconnect can then fall through to Line 214 and fail with"not connected"even though the original connect succeeds moments later.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/mcp/manager.ts` around lines 315 - 319, The connect flow in connectServer() returns too early when this.connecting already contains the server name, which can let ensureConnected() continue before the original connection finishes. Update connectServer() in McpManager to await the existing in-flight connection attempt instead of returning immediately, so concurrent calls share the same promise and tool calls don’t fall through to the “not connected” path. Use the existing connecting state and the ensureConnected()/connectServer() flow to locate and fix the race.packages/cli/src/mcp/manager.ts-328-334 (1)
328-334: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve last-known tool schemas across reconnect attempts.
Line 333 resets
toolsto[]before the reconnect finishes. That breaks the stable-schema behavior promised bygetRegisteredTools()and drops MCP tools from model registration during transient disconnects.Suggested fix
const entry: ManagedServer = { client: null, config, status: McpConnectionStatus.PENDING, reconnectAttempts: previous?.reconnectAttempts ?? 0, - tools: [], + tools: previous?.tools ?? [], };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/mcp/manager.ts` around lines 328 - 334, The ManagedServer initialization in manager.ts is clearing tools too early during reconnects, which drops the last-known MCP tool schemas before the new connection succeeds. Update the entry setup in the reconnect path to preserve any existing tool list from previous rather than always resetting tools to an empty array, so getRegisteredTools() continues to return stable schemas while the server is reconnecting.packages/cli/src/mcp/integration.test.ts-18-40 (1)
18-40: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMake this integration test opt-in or hermetic.
The guard only proves that
npxexists. Line 39 still relies onnpx -y@modelcontextprotocol/server-filesystem`` fetching a package at runtime, so this test can fail in CI for network/npm reasons unrelated to the change.Suggested fix
test("connectAll discovers tools from filesystem MCP server", async () => { - if (!(await isNpxAvailable())) { + if (!process.env.RUN_MCP_INTEGRATION_TESTS || !(await isNpxAvailable())) { console.warn("SKIP: npx unavailable — MCP stdio integration test skipped"); return; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/mcp/integration.test.ts` around lines 18 - 40, The MCP integration test is still non-hermetic because connectAll relies on npx fetching `@modelcontextprotocol/server-filesystem` at runtime even though it only checks isNpxAvailable(). Update connectAll in integration.test.ts to be opt-in or fully local by removing the network-dependent package fetch, using a preinstalled/local test server fixture, or gating the test behind an explicit environment flag so CI does not depend on npm availability.packages/cli/src/mcp/manager.ts-343-349 (1)
343-349: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRetry remote servers when tool discovery fails too.
If
client.listTools()fails afterclient.connect()succeeds, this branch closes the client and returns before the outercatchcan callscheduleReconnect(). HTTP/SSE servers then stay failed until a manual reconnect.Suggested fix
try { ({ tools } = await client.listTools()); } catch (discoveryError) { await client.close().catch(() => {}); entry.status = McpConnectionStatus.FAILED; entry.error = sanitizeErrorMessage(discoveryError); + if (config.transport === McpTransport.HTTP || config.transport === McpTransport.SSE) { + this.scheduleReconnect(name); + } return; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/mcp/manager.ts` around lines 343 - 349, Tool discovery failures in the McpConnection flow currently bypass reconnect handling by returning from the `client.listTools()` catch block after closing the client. Update the `connect`/discovery path in `packages/cli/src/mcp/manager.ts` so `scheduleReconnect()` still runs for HTTP/SSE remote servers when `listTools()` throws, using the existing `entry`, `client`, and `McpConnectionStatus.FAILED` handling without short-circuiting the outer reconnect logic.packages/cli/src/mcp/config-schema.ts-20-35 (1)
20-35: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReject malformed remote URLs during config parsing.
Line 23 and Line 32 only require a non-empty string, but
packages/cli/src/mcp/transports.tsimmediately doesnew URL(entry.url). That turns a config typo into a runtime failure/reconnect loop instead of the fail-fastsafeParseerror thatpackages/cli/src/mcp/config.tsis set up to surface.Suggested fix
const mcpServerHttpSchema = z.object({ enabled: z.boolean().default(true), transport: z.literal("http"), - url: z.string().min(1), + url: z.string().refine((value) => { + try { + new URL(value); + return true; + } catch { + return false; + } + }, "Invalid MCP server URL"), headers: z.record(z.string(), z.string()).optional(), timeoutMs: z.number().positive().default(60000), tools: z.record(z.string(), mcpToolOverrideSchema).optional(), }); const mcpServerSseSchema = z.object({ enabled: z.boolean().default(true), transport: z.literal("sse"), - url: z.string().min(1), + url: z.string().refine((value) => { + try { + new URL(value); + return true; + } catch { + return false; + } + }, "Invalid MCP server URL"), headers: z.record(z.string(), z.string()).optional(), timeoutMs: z.number().positive().default(60000), tools: z.record(z.string(), mcpToolOverrideSchema).optional(), });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/mcp/config-schema.ts` around lines 20 - 35, The HTTP and SSE MCP schemas currently only require a non-empty string for url, but transports.ts later constructs a URL object from it, so malformed URLs slip through parsing and fail at runtime. Update mcpServerHttpSchema and mcpServerSseSchema in config-schema.ts to validate url as a proper URL format during safeParse, so config.ts can surface the error early instead of letting transports.ts hit a reconnect loop.packages/cli/src/mcp/config.ts-36-43 (1)
36-43: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winOnly treat missing files as empty config.
This
catchalso hides malformed JSON and permission errors, so a brokenmcp.jsonis reinterpreted as{}. Because this loader feeds MCP connection setup and tool discovery, one bad edit silently drops servers/tools instead of surfacing the config problem.Proposed fix
function readMcpJsonFile(path: string): Record<string, unknown> { try { const raw = readFileSync(path, "utf-8"); const parsed = JSON.parse(raw) as Record<string, unknown>; return parsed; - } catch { - return {}; + } catch (error) { + if ( + typeof error === "object" && + error !== null && + "code" in error && + error.code === "ENOENT" + ) { + return {}; + } + throw error; } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/mcp/config.ts` around lines 36 - 43, The readMcpJsonFile helper is swallowing every error and returning an empty config, which hides malformed JSON and permission issues. Update readMcpJsonFile so it only treats a missing file as empty, and rethrow or surface any other error from readFileSync/JSON.parse with context. Keep the fix localized to readMcpJsonFile in config.ts so MCP setup and tool discovery fail loudly on broken mcp.json.packages/cli/src/mcp/watcher.ts-69-75 (1)
69-75: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winHandle reload failures in the debounce callback.
void reloadMcp(...)can reject fromdisconnectAll()orconnectAll(), which leaves this path with an unhandled rejection and no fallback. Add a.catch(...)here and surface the error somehow.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/mcp/watcher.ts` around lines 69 - 75, The debounce callback in watcher.ts is firing reloadMcp without handling promise rejection, so failures from reloadMcp/disconnectAll/connectAll can become unhandled. Update the setTimer callback around debounceTimer/debounceOnReload to attach a catch handler to the reloadMcp call, and surface the error through the existing logging or error-reporting path used in this module. Keep the current debounce state reset logic intact while ensuring reload failures are observed and reported.packages/cli/src/mcp/watcher.ts-87-97 (1)
87-97: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winScope reloads to the config path and handle deletions.
When the watch falls back to the parent directory, ignore events for other files so unrelated
.mocodewrites don’t retrigger MCP reloads. Also listen forunlinkso removingmcp.jsonreloads the config.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/mcp/watcher.ts` around lines 87 - 97, The watcher in watcher.ts reloads too broadly when resolveWatchTarget falls back to the parent directory, so unrelated .mocode file events can retrigger MCP reloads. Update the event handler around resolveWatchTarget, scheduleReload, and activeWatchers to ignore change/add events unless they match the config file path being watched, and add an unlink listener so deleting mcp.json also triggers scheduleReload. Keep the fix localized to the existing watch loop over paths.global and paths.project.packages/cli/src/hooks/use-chat.ts-154-158 (1)
154-158: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDon't drop
Mcp__...dynamic calls beforeexecuteMcpToolCall().
packages/cli/src/lib/mcp-tool-call.tsalready normalizes a mixed-caseMcp__prefix, but this guard only recognizes lowercase viaisMcpToolName(). A dynamic tool call likeMcp__filesystem__read_filewill return early here and never reach the normalization path.Suggested fix
- const isMcpCall = isMcpToolName(toolCall.toolName); + const isMcpCall = + isMcpToolName(toolCall.toolName) || + toolCall.toolName.toLowerCase().startsWith("mcp__");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/hooks/use-chat.ts` around lines 154 - 158, The dynamic-tool guard in use-chat should not return early for mixed-case MCP tool calls, because executeMcpToolCall depends on reaching the normalization path in mcp-tool-call. Update the isMcpToolName/toolCall.dynamic check so Mcp__... names are treated as MCP calls even when the prefix casing differs, ensuring only unrelated dynamic tools are skipped and MCP calls still flow through executeMcpToolCall.packages/cli/src/hooks/use-chat.ts-242-246 (1)
242-246: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winOnly suppress the expected missing-session case.
updateLocalSession()also writes the session file and index. Catching everything here turns real I/O failures into silent local-session data loss.Suggested fix
- } catch { - // Session file may not exist yet during initial mount. + } catch (error) { + if ( + error instanceof Error && + error.message.startsWith("Local session not found:") + ) { + return; + } + console.error("Failed to persist local session", { sessionId, error }); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/hooks/use-chat.ts` around lines 242 - 246, The try/catch around updateLocalSession in useChat is swallowing all failures, not just the expected missing-session case. Narrow the catch logic so only the initial-mount “session file does not exist yet” path is ignored, and let other I/O errors from updateLocalSession surface or be handled explicitly. Use the updateLocalSession call in useChat as the place to distinguish the missing-session case from real write/index failures.packages/cli/src/lib/local-chat-transport.ts-111-119 (1)
111-119: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winBuild the MCP prompt from the filtered toolset.
buildMergedToolSet()already removes disallowed MCP tools for the active mode, butmcpToolNamesis taken from the unfiltered dynamic set. In PLAN mode this can advertise write tools as available even though they are absent fromtools, which pushes the model toward impossible MCP calls.Suggested fix
const mcpDynamicTools = buildMcpDynamicToolsFromManager( this.opts.getMcpManager(), config, ); const tools = buildMergedToolSet(mode, mcpDynamicTools, config); - const mcpToolNames = Object.keys(mcpDynamicTools).filter(isMcpToolName); + const mcpToolNames = Object.keys(tools).filter(isMcpToolName);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/lib/local-chat-transport.ts` around lines 111 - 119, The MCP prompt is built from the unfiltered dynamic tool map, so disallowed tools can still be advertised even after buildMergedToolSet() removes them. Update the local-chat-transport flow to derive mcpToolNames from the filtered tools returned by buildMergedToolSet() (or an equivalent filtered MCP-only list) before calling buildSystemPrompt(), so the prompt matches the actual available toolset in this mode.packages/cli/src/lib/local-chat-transport.test.ts-28-36 (1)
28-36: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMock
getRegisteredTools()here.
LocalChatTransportreachesbuildMcpDynamicToolsFromManager(), and that helper iteratesmanager.getRegisteredTools(). This stub only providesgetDiscoveredTools(), so the firstsendMessages()call will throw beforestreamTextMockis exercised.Suggested fix
function createMockManager(): McpManager { return { - getDiscoveredTools: () => [ + getRegisteredTools: () => [ { serverName: "filesystem", tools: [{ name: "read_file", description: "Read file", inputSchema: {} }], }, ],🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/lib/local-chat-transport.test.ts` around lines 28 - 36, The test mock for McpManager is incomplete: LocalChatTransport now flows through buildMcpDynamicToolsFromManager(), which calls getRegisteredTools() during sendMessages(). Update createMockManager() to stub getRegisteredTools() with a matching registered-tools shape in addition to getDiscoveredTools(), so the first sendMessages() call can proceed to streamTextMock instead of throwing.
🟡 Minor comments (4)
packages/cli/src/utils/list-scroll-nav.ts-6-13 (1)
6-13: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winGuard the new
itemRowHeightinput before using it.
itemRowHeightis new here, but0or a negative value now gives invalid viewport math and bogus scroll targets. Clamp or early-return before computingviewportRows/item bounds.Proposed fix
export function scrollIndexIntoView( scrollbox: { scrollTop: number; scrollTo: (position: number) => void }, index: number, pageSize: number, itemRowHeight = 1, ): void { - if (pageSize <= 0) return; + if (pageSize <= 0 || itemRowHeight <= 0) return;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/utils/list-scroll-nav.ts` around lines 6 - 13, The new itemRowHeight input in listScrollNav can produce invalid scroll math when it is 0 or negative, so add a guard in listScrollNav before calculating viewportRows, itemTop, and itemBottom. Use the existing pageSize check as a model and either early-return or clamp itemRowHeight to a positive value so the scroll target calculations remain valid.packages/cli/src/components/dialogs/mcp-dialog.tsx-273-276 (1)
273-276: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winClamp the busy footer to one line.
The idle footer is fixed text, but the busy branch prints raw server names with no truncation. A couple of long names will wrap this into multiple lines and violate the single-status-line footer rule for dialog shells. As per coding guidelines, "In CLI UI layouts, keep the interface bottom-anchored" and "Follow the dialog shell plus Width & Overflow rules: cap before truncating." Based on learnings,
DESIGN.mdtakes precedence when UI specs conflict.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/components/dialogs/mcp-dialog.tsx` around lines 273 - 276, The busy-state footer in mcp-dialog.tsx can wrap to multiple lines because it renders raw busyServers names in the status text. Update the footer rendering in the dialog shell to keep the busy branch to a single line by capping the displayed server list and truncating/ellipsizing it before it can overflow, while preserving the existing idle footer text and the busyServers.size condition used in the TextAttributes.DIM block.Sources: Coding guidelines, Learnings
packages/cli/src/mcp/transports.test.ts-26-35 (1)
26-35: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThe env-merge test never checks the merge.
Lines 29-34 only prove the current process has
PATH, so this still passes ifentry.envis dropped completely. Please assert thatMOCODE_TEST_ENVreaches the constructed transport, or extract the merge step into a helper with a deterministic unit test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/mcp/transports.test.ts` around lines 26 - 35, The stdio env merge test in createTransport is not actually verifying that entry.env is preserved, so update the test to assert that MOCODE_TEST_ENV appears on the constructed stdio transport rather than only checking process.env.PATH. Use the createTransport and baseStdio symbols to locate the merge path, and if the env merge is buried inside transport construction, consider extracting that merge logic into a helper that can be unit tested deterministically.packages/cli/src/mcp/transports.test.ts-21-24 (1)
21-24: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winStrengthen the stdio env test. The current assertion only checks
process.env.PATH, so it never exercisesentry.envand won’t catch a merge regression. Assert a custom key fromentry.envinstead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/mcp/transports.test.ts` around lines 21 - 24, The stdio environment test is only asserting process.env.PATH, so it does not verify that entry.env is merged into the transport environment. Update the assertion in the stdio transport test to check a custom key provided through entry.env, using the transport setup around the stdio transport creation and its env handling, so a merge regression would fail the test.
🧹 Nitpick comments (1)
packages/cli/src/components/keys-setup-gate.tsx (1)
16-28: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
openKeysWizardIfNeeded()here.This reimplements the same
shouldAutoOpenKeysWizard()+"API Keys"dialog contract that already lives inpackages/cli/src/lib/keys-wizard-trigger.ts:31-44. Calling the helper here keeps the cold-start gate and any future trigger paths from drifting.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/components/keys-setup-gate.tsx` around lines 16 - 28, The keys setup gate is duplicating the auto-open logic instead of using the shared trigger flow. Update the effect in KeysSetupGate to call openKeysWizardIfNeeded() from keys-wizard-trigger rather than rechecking shouldAutoOpenKeysWizard() and opening the dialog directly, so the “API Keys” contract stays centralized and consistent across trigger paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/server/src/routes/chat.ts`:
- Around line 67-85: The submitSchema mcpTools validation currently allows any
name, which lets client input collide with built-in tool contracts later merged
in chat.ts. Tighten the mcpToolSchema/name check so only MCP-namespaced tool ids
are accepted at the request boundary, and reject or filter any non-MCP names
before they reach getToolContracts(mode) and the spread merge. Use the existing
submitSchema and mcpToolSchema symbols to keep the fix localized.
---
Major comments:
In `@packages/cli/src/components/dialogs/keys-wizard-dialog.tsx`:
- Around line 46-68: The new dialog in keys-wizard-dialog.tsx is bypassing the
shared theme/design system by hardcoding TUI colors and layout values in
ActionButton and the other dialog sections. Update the relevant components to
resolve semantic colors through useTheme() instead of using literal "black",
"white", and "gray", and replace fixed padding/spacing/sizing with DESIGN.md
tokens. Apply the same theme/token-driven cleanup across ActionButton and the
other dialog blocks referenced in this file so the whole dialog follows the
shared terminal contract.
- Around line 97-114: `handleSaveKey` in `keys-wizard-dialog.tsx` should not let
`saveKeys(nextKeys)` exceptions escape and crash the TUI; wrap the save call in
error handling, and only update local state or navigate back when the save
succeeds. On failure, keep the current draft/editing state intact and surface
the error through the dialog’s existing feedback path if available, using the
`saveKeys` and `handleSaveKey` symbols to locate the change.
In `@packages/cli/src/components/dialogs/mcp-approval-dialog.tsx`:
- Around line 32-53: The ActionButton in mcp-approval-dialog is bypassing the
shared DESIGN token system by hardcoding spacing and terminal colors. Update
this component to use semantic theme colors resolved through useTheme() instead
of literal black/white/gray, and replace the local padding value with the
appropriate DESIGN spacing token. Keep the change localized to ActionButton so
the approval buttons follow the terminal theme contract.
- Around line 67-99: The MCP approval dialog is rendering unbounded serialized
input, which can make the action buttons disappear off-screen; update
`McpApprovalDialog` so the `formattedInput` display is capped and scrollable
instead of expanding the dialog. In the component that uses
`requestMcpApproval()`, wrap the pretty-printed JSON in a container with
explicit width/height constraints and overflow handling, and keep the action
rows visible below the capped content. Use the existing `formattedInput`,
`actions`, and `useKeyboard` flow as the location to adjust the layout without
changing approval behavior.
In `@packages/cli/src/components/dialogs/mcp-dialog.tsx`:
- Around line 24-27: The MCP dialog is using local size constants and hardcoded
text colors instead of shared design tokens. Update `McpDialog` to source
width/padding/row sizing from `DESIGN.md` token values and resolve text colors
through `useTheme()` semantic tokens rather than literal black/white/gray,
including the color usages in the dialog rendering and item rows. Keep the
existing `MCP_ROW_HEIGHT`, `MCP_LIST_MAX_ITEMS`, `DIALOG_PADDING_X`, and
`DIALOG_MAX_WIDTH` behavior aligned with the theme-driven token set while
removing any direct literals.
In `@packages/cli/src/hooks/use-chat.ts`:
- Around line 154-158: The dynamic-tool guard in use-chat should not return
early for mixed-case MCP tool calls, because executeMcpToolCall depends on
reaching the normalization path in mcp-tool-call. Update the
isMcpToolName/toolCall.dynamic check so Mcp__... names are treated as MCP calls
even when the prefix casing differs, ensuring only unrelated dynamic tools are
skipped and MCP calls still flow through executeMcpToolCall.
- Around line 242-246: The try/catch around updateLocalSession in useChat is
swallowing all failures, not just the expected missing-session case. Narrow the
catch logic so only the initial-mount “session file does not exist yet” path is
ignored, and let other I/O errors from updateLocalSession surface or be handled
explicitly. Use the updateLocalSession call in useChat as the place to
distinguish the missing-session case from real write/index failures.
In `@packages/cli/src/lib/keys-wizard-trigger.test.ts`:
- Around line 10-29: The tests in shouldAutoOpenKeysWizard are using a fixed
directory under the real home folder, which can clobber local data and collide
across runs; update the setup/teardown in keys-wizard-trigger.test.ts to create
a unique per-run temporary directory instead of TEST_DIR. Use that temp path
consistently wherever the test currently references TEST_DIR (including the
later uses mentioned in the review) so the suite isolates its filesystem writes
and cleanup.
In `@packages/cli/src/lib/keys.ts`:
- Around line 63-67: In saveKeys() within keys.ts, the restrictive permissions
are only applied on creation, so existing ~/.mocode directories or keys.json
files may keep broader modes. Update the save flow to re-apply permissions after
the write by calling chmodSync on both the dir and keysFile, using 0o700 for the
directory and 0o600 for the key file, so existing paths are tightened as well.
In `@packages/cli/src/lib/local-chat-transport.test.ts`:
- Around line 28-36: The test mock for McpManager is incomplete:
LocalChatTransport now flows through buildMcpDynamicToolsFromManager(), which
calls getRegisteredTools() during sendMessages(). Update createMockManager() to
stub getRegisteredTools() with a matching registered-tools shape in addition to
getDiscoveredTools(), so the first sendMessages() call can proceed to
streamTextMock instead of throwing.
In `@packages/cli/src/lib/local-chat-transport.ts`:
- Around line 111-119: The MCP prompt is built from the unfiltered dynamic tool
map, so disallowed tools can still be advertised even after buildMergedToolSet()
removes them. Update the local-chat-transport flow to derive mcpToolNames from
the filtered tools returned by buildMergedToolSet() (or an equivalent filtered
MCP-only list) before calling buildSystemPrompt(), so the prompt matches the
actual available toolset in this mode.
In `@packages/cli/src/lib/local-sessions.ts`:
- Around line 56-64: The readIndex() helper is swallowing all read/parse
failures and treating them as an empty session list, which can overwrite a bad
or inaccessible sessions-index.json in createLocalSession(). Update readIndex()
to only return { sessions: [] } when the index file is missing, and rethrow or
surface other errors such as malformed JSON or permission issues so existing
data is not silently lost.
- Around line 40-47: The project directory key generated by normalizeProjectPath
is collision-prone and platform-specific because it only replaces forward
slashes, so update normalizeProjectPath and its use in resolveProjectDir to
produce a stable, collision-free key from the full cwd. Use a deterministic
encoding or hash of the absolute path that safely handles path separators, drive
letters, and special characters, so different cwd values cannot map to the same
projectsDir subdirectory.
In `@packages/cli/src/lib/session-navigation.ts`:
- Line 14: The session-navigation schema still makes local unusable because
`session` is required, so the `local` flag can never independently reach the
local-session branch in `session.tsx`. Update the schema around `local` and
`session` in `session-navigation.ts` so `session` is optional or the state is
represented as a union, then ensure the parsed result can be `{ local: true }`
without requiring `session`.
In `@packages/cli/src/mcp/config-schema.ts`:
- Around line 20-35: The HTTP and SSE MCP schemas currently only require a
non-empty string for url, but transports.ts later constructs a URL object from
it, so malformed URLs slip through parsing and fail at runtime. Update
mcpServerHttpSchema and mcpServerSseSchema in config-schema.ts to validate url
as a proper URL format during safeParse, so config.ts can surface the error
early instead of letting transports.ts hit a reconnect loop.
In `@packages/cli/src/mcp/config.ts`:
- Around line 36-43: The readMcpJsonFile helper is swallowing every error and
returning an empty config, which hides malformed JSON and permission issues.
Update readMcpJsonFile so it only treats a missing file as empty, and rethrow or
surface any other error from readFileSync/JSON.parse with context. Keep the fix
localized to readMcpJsonFile in config.ts so MCP setup and tool discovery fail
loudly on broken mcp.json.
In `@packages/cli/src/mcp/heuristics.ts`:
- Around line 17-18: The MCP name gate in isMcpToolName is too broad and
currently accepts any mcp__* string, which can let malformed names reach
parseMcpToolName and throw. Tighten the validation in
packages/cli/src/mcp/heuristics.ts so it only returns true for the full
mcp__<server>__<tool> shape, and keep use-chat and mcp-tool-call relying on that
helper as the pre-check before routing.
In `@packages/cli/src/mcp/integration.test.ts`:
- Around line 18-40: The MCP integration test is still non-hermetic because
connectAll relies on npx fetching `@modelcontextprotocol/server-filesystem` at
runtime even though it only checks isNpxAvailable(). Update connectAll in
integration.test.ts to be opt-in or fully local by removing the
network-dependent package fetch, using a preinstalled/local test server fixture,
or gating the test behind an explicit environment flag so CI does not depend on
npm availability.
In `@packages/cli/src/mcp/manager.ts`:
- Around line 315-319: The connect flow in connectServer() returns too early
when this.connecting already contains the server name, which can let
ensureConnected() continue before the original connection finishes. Update
connectServer() in McpManager to await the existing in-flight connection attempt
instead of returning immediately, so concurrent calls share the same promise and
tool calls don’t fall through to the “not connected” path. Use the existing
connecting state and the ensureConnected()/connectServer() flow to locate and
fix the race.
- Around line 328-334: The ManagedServer initialization in manager.ts is
clearing tools too early during reconnects, which drops the last-known MCP tool
schemas before the new connection succeeds. Update the entry setup in the
reconnect path to preserve any existing tool list from previous rather than
always resetting tools to an empty array, so getRegisteredTools() continues to
return stable schemas while the server is reconnecting.
- Around line 343-349: Tool discovery failures in the McpConnection flow
currently bypass reconnect handling by returning from the `client.listTools()`
catch block after closing the client. Update the `connect`/discovery path in
`packages/cli/src/mcp/manager.ts` so `scheduleReconnect()` still runs for
HTTP/SSE remote servers when `listTools()` throws, using the existing `entry`,
`client`, and `McpConnectionStatus.FAILED` handling without short-circuiting the
outer reconnect logic.
In `@packages/cli/src/mcp/watcher.ts`:
- Around line 69-75: The debounce callback in watcher.ts is firing reloadMcp
without handling promise rejection, so failures from
reloadMcp/disconnectAll/connectAll can become unhandled. Update the setTimer
callback around debounceTimer/debounceOnReload to attach a catch handler to the
reloadMcp call, and surface the error through the existing logging or
error-reporting path used in this module. Keep the current debounce state reset
logic intact while ensuring reload failures are observed and reported.
- Around line 87-97: The watcher in watcher.ts reloads too broadly when
resolveWatchTarget falls back to the parent directory, so unrelated .mocode file
events can retrigger MCP reloads. Update the event handler around
resolveWatchTarget, scheduleReload, and activeWatchers to ignore change/add
events unless they match the config file path being watched, and add an unlink
listener so deleting mcp.json also triggers scheduleReload. Keep the fix
localized to the existing watch loop over paths.global and paths.project.
In `@packages/cli/src/screens/new-session.tsx`:
- Around line 49-52: The local-mode flow in new-session.tsx is defaulting
unknown state.model values to anthropic via findSupportedChatModel(...)?provider
?? "anthropic", which can open the wrong keys wizard and pass an unsupported
model into the session. Update the branch around isLocalMode() to first validate
the model with findSupportedChatModel, only derive provider when the model is
recognized, and otherwise handle the unknown model explicitly instead of falling
back to Anthropic; keep the fix localized to the newSession flow and
openKeysWizardIfNeeded call site.
---
Minor comments:
In `@packages/cli/src/components/dialogs/mcp-dialog.tsx`:
- Around line 273-276: The busy-state footer in mcp-dialog.tsx can wrap to
multiple lines because it renders raw busyServers names in the status text.
Update the footer rendering in the dialog shell to keep the busy branch to a
single line by capping the displayed server list and truncating/ellipsizing it
before it can overflow, while preserving the existing idle footer text and the
busyServers.size condition used in the TextAttributes.DIM block.
In `@packages/cli/src/mcp/transports.test.ts`:
- Around line 26-35: The stdio env merge test in createTransport is not actually
verifying that entry.env is preserved, so update the test to assert that
MOCODE_TEST_ENV appears on the constructed stdio transport rather than only
checking process.env.PATH. Use the createTransport and baseStdio symbols to
locate the merge path, and if the env merge is buried inside transport
construction, consider extracting that merge logic into a helper that can be
unit tested deterministically.
- Around line 21-24: The stdio environment test is only asserting
process.env.PATH, so it does not verify that entry.env is merged into the
transport environment. Update the assertion in the stdio transport test to check
a custom key provided through entry.env, using the transport setup around the
stdio transport creation and its env handling, so a merge regression would fail
the test.
In `@packages/cli/src/utils/list-scroll-nav.ts`:
- Around line 6-13: The new itemRowHeight input in listScrollNav can produce
invalid scroll math when it is 0 or negative, so add a guard in listScrollNav
before calculating viewportRows, itemTop, and itemBottom. Use the existing
pageSize check as a model and either early-return or clamp itemRowHeight to a
positive value so the scroll target calculations remain valid.
---
Nitpick comments:
In `@packages/cli/src/components/keys-setup-gate.tsx`:
- Around line 16-28: The keys setup gate is duplicating the auto-open logic
instead of using the shared trigger flow. Update the effect in KeysSetupGate to
call openKeysWizardIfNeeded() from keys-wizard-trigger rather than rechecking
shouldAutoOpenKeysWizard() and opening the dialog directly, so the “API Keys”
contract stays centralized and consistent across trigger paths.
…rrors Address PR review findings: MCP connect races, config I/O guards, mixed-case tool routing, keys chmod, and AI SDK error masking so BYOK users see real API failures. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/cli/src/mcp/heuristics.test.ts (1)
20-77: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd coverage for fully-qualified MCP tool names.
These suites only exercise bare names like
write_file, but the runtime contract in this PR registers tools asmcp__<server>__<tool>. A regression in prefix parsing would still pass here while breaking real read-only/approval decisions.Proposed test additions
describe("isMcpReadOnlyTool", () => { + test("accepts fully-qualified read-only MCP tool names", () => { + expect(isMcpReadOnlyTool("mcp__filesystem__read_file")).toBe(true); + }); + + test("accepts fully-qualified write MCP tool names", () => { + expect(isMcpReadOnlyTool("mcp__filesystem__write_file")).toBe(false); + }); + const readOnlyTools = [ { name: "get_foo", tool: "get_foo" }, { name: "list_items", tool: "list_items" }, @@ describe("requiresMcpWriteApproval", () => { + test("fully-qualified write MCP tools require approval", () => { + expect(requiresMcpWriteApproval("mcp__filesystem__write_file", new Set())).toBe(true); + }); + + test("fully-qualified read-only MCP tools do not require approval", () => { + expect(requiresMcpWriteApproval("mcp__filesystem__read_file", new Set())).toBe(false); + }); + const writeTools = [ { name: "write_file", tool: "write_file" }, { name: "delete_item", tool: "delete_item" },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/mcp/heuristics.test.ts` around lines 20 - 77, The current tests for isMcpReadOnlyTool and requiresMcpWriteApproval only cover bare tool names, so they miss regressions in handling the real mcp__<server>__<tool> format. Add assertions in packages/cli/src/mcp/heuristics.test.ts that exercise fully-qualified MCP tool names for both read-only and write-approval paths, including cases that should still be detected as read-only/read-required and cases that should not, so prefix parsing is validated against the runtime contract.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/cli/src/lib/stream-error.ts`:
- Around line 6-12: The stream-error normalization in the helper that handles
error messages returns an empty string for cases like throw "" or new Error(),
which can leave the TUI blank; update the logic in stream-error.ts so the
derived message from the string/Error/String(error) path falls back to "Unknown
error" whenever it is empty. Keep the fix localized to the existing
error-to-string helper so all callers automatically get the non-blank message.
---
Nitpick comments:
In `@packages/cli/src/mcp/heuristics.test.ts`:
- Around line 20-77: The current tests for isMcpReadOnlyTool and
requiresMcpWriteApproval only cover bare tool names, so they miss regressions in
handling the real mcp__<server>__<tool> format. Add assertions in
packages/cli/src/mcp/heuristics.test.ts that exercise fully-qualified MCP tool
names for both read-only and write-approval paths, including cases that should
still be detected as read-only/read-required and cases that should not, so
prefix parsing is validated against the runtime contract.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 58fce955-9e32-4a23-a4c7-f77616417f91
📒 Files selected for processing (20)
packages/cli/src/components/dialogs/keys-wizard-dialog.tsxpackages/cli/src/hooks/use-chat.tspackages/cli/src/lib/keys-wizard-trigger.test.tspackages/cli/src/lib/keys.tspackages/cli/src/lib/local-chat-transport.test.tspackages/cli/src/lib/local-chat-transport.tspackages/cli/src/lib/local-sessions.tspackages/cli/src/lib/mcp-tool-call.tspackages/cli/src/lib/stream-error.test.tspackages/cli/src/lib/stream-error.tspackages/cli/src/mcp/config-schema.tspackages/cli/src/mcp/config.tspackages/cli/src/mcp/heuristics.test.tspackages/cli/src/mcp/heuristics.tspackages/cli/src/mcp/integration.test.tspackages/cli/src/mcp/manager.tspackages/cli/src/mcp/watcher.test.tspackages/cli/src/mcp/watcher.tspackages/cli/src/screens/new-session.tsxpackages/server/src/routes/chat.ts
✅ Files skipped from review due to trivial changes (1)
- packages/cli/src/lib/stream-error.test.ts
🚧 Files skipped from review as they are similar to previous changes (17)
- packages/cli/src/screens/new-session.tsx
- packages/cli/src/mcp/config-schema.ts
- packages/cli/src/mcp/watcher.test.ts
- packages/cli/src/mcp/watcher.ts
- packages/cli/src/mcp/integration.test.ts
- packages/cli/src/components/dialogs/keys-wizard-dialog.tsx
- packages/cli/src/lib/local-sessions.ts
- packages/cli/src/lib/keys-wizard-trigger.test.ts
- packages/cli/src/mcp/heuristics.ts
- packages/cli/src/lib/mcp-tool-call.ts
- packages/cli/src/hooks/use-chat.ts
- packages/cli/src/lib/keys.ts
- packages/cli/src/lib/local-chat-transport.ts
- packages/cli/src/mcp/config.ts
- packages/cli/src/lib/local-chat-transport.test.ts
- packages/cli/src/mcp/manager.ts
- packages/server/src/routes/chat.ts
| if (typeof error === "string") { | ||
| return error; | ||
| } | ||
| if (error instanceof Error) { | ||
| return error.message; | ||
| } | ||
| return String(error); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Avoid returning a blank error message.
throw "" or new Error() currently normalizes to an empty string, so the TUI can end up showing no error text at all. Fall back to "Unknown error" when the derived message is empty.
Proposed fix
export function formatChatStreamError(error: unknown): string {
if (error == null) {
return "Unknown error";
}
if (typeof error === "string") {
- return error;
+ return error.trim() || "Unknown error";
}
if (error instanceof Error) {
- return error.message;
+ return error.message.trim() || error.name || "Unknown error";
}
- return String(error);
+ return String(error).trim() || "Unknown error";
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (typeof error === "string") { | |
| return error; | |
| } | |
| if (error instanceof Error) { | |
| return error.message; | |
| } | |
| return String(error); | |
| if (typeof error === "string") { | |
| return error.trim() || "Unknown error"; | |
| } | |
| if (error instanceof Error) { | |
| return error.message.trim() || error.name || "Unknown error"; | |
| } | |
| return String(error).trim() || "Unknown error"; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/cli/src/lib/stream-error.ts` around lines 6 - 12, The stream-error
normalization in the helper that handles error messages returns an empty string
for cases like throw "" or new Error(), which can leave the TUI blank; update
the logic in stream-error.ts so the derived message from the
string/Error/String(error) path falls back to "Unknown error" whenever it is
empty. Keep the fix localized to the existing error-to-string helper so all
callers automatically get the non-blank message.
Handle invalid mcp.json during tool execution and SaaS submit without hanging the chat loop, and normalize only the mcp__ prefix for mixed-case models. Co-authored-by: Cursor <cursoragent@cursor.com>
1 participant
Summary
mcp.jsonunion merge (global + project), stdio/HTTP/SSE transports,McpManagerlifecycle with chokidar hot-reload, andmcp__<server>__<tool>dynamicTool registration (HARNESS-04/05).deserializeMcpToolsToDynamic(Phase 11 local execution preserved).mocode --local:keys.json+/keyswizard, per-project local sessions,LocalChatTransportin-processstreamText, and optional OAuth skip (HARNESS-06)./mcpruntime dialog (status, reconnect, enabled toggle), MCP write TUI approval (mirrors bash gate), and MCP routing in BYOK system prompt.AGENTS.md/DESIGN.mdas terminal UI standards; extenddocs/agent-permissions.mdfor MCP.Commits
feat(shared,server): merge MCP tool schemas in SaaS chat routefeat(cli): add MCP client stack and write approval gatefeat(cli): add BYOK --local mode with keys and local sessionsfeat(cli): wire MCP and BYOK into chat, sessions, and TUI dialogsdocs: add terminal UI standards and MCP permission notesTest plan
bun test packages/cli/src/mcp/bun test packages/cli/src/lib/mcp-approval.test.ts packages/cli/src/hooks/use-chat-mcp.test.tsbun test packages/cli/src/lib/local-*.test.ts packages/cli/src/lib/keys.test.ts~/.mocode/mcp.jsonfilesystem server →/mcpshows connected → agent invokesmcp__*read tool without approvalbun run dev:cli -- --localwith~/.mocode/keys.json→ new session → chat streams locally without server HTTPMcpApprovalDialog; Reject skips executionllama-3.3-70b-versatilerecommended for MCP tool calling)Notes
doc/harness-phase-02-mcp-byok-notes.md(gitignoreddoc/per repo convention)..mocode/project MCP config,tmp/test fixtures.Made with Cursor
Summary by CodeRabbit
New Features
Bug Fixes