fix: address review comments — glob ignore, type guard, persist from disk, dedup respond()

Author: saravmajestic

packages/opencode/src/altimate/datamate-transport.ts

@@ -1,7 +1,7 @@
 import { readFile } from "fs/promises"
 import path from "path"
 import { parseTree, findNodeAtLocation } from "jsonc-parser"
-import { resolveConfigPath, addMcpToConfig } from "../mcp/config"
+import { resolveConfigPath, addMcpToConfig, readMcpEntryFromDisk } from "../mcp/config"
 import { Filesystem } from "../util/filesystem"
 import { Glob } from "../util/glob"
 import { Log } from "../util/log"
@@ -18,8 +18,6 @@ export const DATAMATE_KEY = "datamate"
  */
 const MCP_SERVERS_KEYS = ["servers", "mcpServers"] as const
 
-/** Glob patterns to exclude from mcp.json scans (large, irrelevant trees). */
-const MCP_SCAN_EXCLUDE = ["/node_modules/", "/.git/", "/dist/", "/build/", "/.pnpm/"]
 
 export type DatamateTransport =
   | { type: "remote"; url: string }
@@ -51,10 +49,9 @@ async function findAllMcpJsonFiles(projectRootDir: string): Promise<string[]> {
       cwd: projectRootDir,
       absolute: true,
       dot: true,
+      ignore: ["**/node_modules/**", "**/.git/**", "**/dist/**", "**/build/**", "**/.pnpm/**"],
     })
-    return paths
-      .filter((p) => !MCP_SCAN_EXCLUDE.some((ex) => p.includes(ex)))
-      .sort()
+    return paths.sort()
   } catch {
     log.warn("findAllMcpJsonFiles: glob scan failed", { cwd: projectRootDir })
     return []
@@ -193,7 +190,7 @@ export async function syncDatamateUrlFromVscodeMcp(cwd: string): Promise<string[
             // IDE config uses "stdio"/"http"/"streamable-http"/"sse";
             // altimate-code.json uses "local"/"remote".
             let newEntry: Record<string, unknown>
-            if (datamateVscode["type"] === "stdio") {
+            if ("command" in datamateVscode) {
               const env = datamateVscode["env"] as Record<string, string> | undefined
               const { ALTIMATE_EXTENSION_RPC: _rpc, ...restEnv } = env ?? {}
               const cmd =
@@ -300,30 +297,3 @@ export async function syncDatamateUrlFromVscodeMcp(cwd: string): Promise<string[
   return updated
 }
 
-/**
- * Read a single MCP entry directly from disk (bypasses the in-memory Config
- * singleton) so callers can get the freshly-written config without busting the
- * whole cache. Returns undefined if the entry is not found in any config file.
- */
-export async function readMcpEntryFromDisk(
-  name: string,
-  configPath: string,
-): Promise<Config.Mcp | undefined> {
-  if (!(await Filesystem.exists(configPath))) return undefined
-
-  const text = await Filesystem.readText(configPath)
-  const tree = parseTree(text)
-  if (!tree) return undefined
-
-  const node = findNodeAtLocation(tree, ["mcp", name])
-  if (!node || node.type !== "object" || !node.children) return undefined
-
-  const entry: Record<string, unknown> = {}
-  for (const prop of node.children) {
-    if (prop.type === "property" && prop.children) {
-      entry[prop.children[0]!.value as string] = prop.children[1]!.value
-    }
-  }
-
-  return entry as Config.Mcp
-}

packages/opencode/src/cli/cmd/serve.ts

@@ -4,10 +4,6 @@ import { withNetworkOptions, resolveNetworkOptions } from "../network"
 import { Flag } from "../../flag/flag"
 // altimate_change start — trace: session tracing in headless serve
 import { subscribeTraceConsumer } from "../../altimate/observability/trace-consumer"
-// datamate IDE-sync helpers extracted to shared module
-// Logic lives in altimate/datamate-transport.ts so serve.ts, server.ts, and
-// datamate.ts all import from one place (no duplication, shared DATAMATE_KEY constant).
-export { syncDatamateUrlFromVscodeMcp } from "../../altimate/datamate-transport"
 // altimate_change start — self-update on headless serve startup
 import { scheduleStartupUpgradeCheck } from "./serve-upgrade-check"
 // altimate_change end

packages/opencode/src/mcp/config.ts

@@ -101,3 +101,31 @@ export async function findAllConfigPaths(projectDir: string, globalDir: string):
   }
   return paths
 }
+
+/**
+ * Read a single MCP entry directly from a config file, bypassing the Config
+ * singleton so callers can get the freshly-written config without busting the
+ * whole cache. Returns undefined if the entry is not found in the file.
+ */
+export async function readMcpEntryFromDisk(
+  name: string,
+  configPath: string,
+): Promise<Config.Mcp | undefined> {
+  if (!(await Filesystem.exists(configPath))) return undefined
+
+  const text = await Filesystem.readText(configPath)
+  const tree = parseTree(text)
+  if (!tree) return undefined
+
+  const node = findNodeAtLocation(tree, ["mcp", name])
+  if (!node || node.type !== "object" || !node.children) return undefined
+
+  const entry: Record<string, unknown> = {}
+  for (const prop of node.children) {
+    if (prop.type === "property" && prop.children) {
+      entry[prop.children[0]!.value as string] = prop.children[1]!.value
+    }
+  }
+
+  return entry as Config.Mcp
+}

packages/opencode/src/mcp/index.ts

@@ -16,7 +16,7 @@ import z from "zod/v4"
 import { Instance } from "../project/instance"
 import { Installation } from "../installation"
 // altimate_change start — persist enabled flag
-import { findAllConfigPaths, listMcpInConfig, addMcpToConfig } from "./config"
+import { findAllConfigPaths, listMcpInConfig, addMcpToConfig, readMcpEntryFromDisk } from "./config"
 import { Global } from "../global"
 // altimate_change end
 import { withTimeout } from "@/util/timeout"
@@ -706,8 +706,7 @@ export namespace MCP {
       for (const p of paths) {
         const names = await listMcpInConfig(p)
         if (names.includes(name)) {
-          const cfg = await Config.get()
-          const entry = cfg.mcp?.[name]
+          const entry = await readMcpEntryFromDisk(name, p)
           if (entry)
             await addMcpToConfig(
               name,

packages/opencode/src/server/server.ts

@@ -33,7 +33,8 @@ import { McpRoutes } from "./routes/mcp"
 import { MCP } from "../mcp"
 // Import sync + fresh-read helpers directly from the shared transport module.
 // Using datamate-transport.ts instead of serve.ts avoids a dep on a cmd handler.
-import { syncDatamateUrlFromVscodeMcp, readMcpEntryFromDisk } from "../altimate/datamate-transport"
+import { syncDatamateUrlFromVscodeMcp } from "../altimate/datamate-transport"
+import { readMcpEntryFromDisk } from "../mcp/config"
 import { resolveConfigPath } from "../mcp/config"
 // altimate_change end
 import { FileRoutes } from "./routes/file"

packages/opencode/src/session/prompt.ts

@@ -423,22 +423,15 @@ export namespace SessionPrompt {
     // so the next resolveTools() call (once per LLM turn) naturally picks up fresh
     // tools without any extra work here. This subscription makes the session layer
     // explicitly aware of the reconnect and logs it so it is traceable in prod.
-    let toolsNeedRefresh = false
     const unsubscribeToolsChanged = Bus.subscribe(MCP.ToolsChanged, (event) => {
       log.info("MCP.ToolsChanged received — tools will refresh on next turn", {
         server: event.properties.server,
         sessionID,
       })
-      toolsNeedRefresh = true
     })
     using _unsubToolsChanged = defer(unsubscribeToolsChanged)
     // altimate_change end
     while (true) {
-      // altimate_change start — log when a ToolsChanged event was received since last turn
-      if (toolsNeedRefresh) {
-        log.info("refreshing MCP tools after ToolsChanged event", { sessionID })
-        toolsNeedRefresh = false
-      }
       // altimate_change end
       // altimate_change start — SessionStatus.set became async in v1.4.0; await so busy state flushes before LLM call
       await SessionStatus.set(sessionID, { type: "busy" })
@@ -2592,6 +2585,33 @@ NOTE: At any point in time through this workflow you should feel free to ask the
 
     // altimate_change start — /mcps enable/disable: direct handler bypasses LLM
     if (input.command === "mcps") {
+
+      // Helper: build and persist an assistant reply for a command shortcut.
+      async function respond(
+        parentID: string,
+        responseText: string,
+      ): Promise<MessageV2.WithParts> {
+        const now = Date.now()
+        const assistantMsg: MessageV2.Assistant = {
+          id: MessageID.ascending(), role: "assistant", sessionID: input.sessionID,
+          parentID, modelID: model.modelID, providerID: model.providerID,
+          mode: "builder", agent: "builder",
+          path: { cwd: Instance.directory, root: Instance.worktree },
+          cost: 0, tokens: { total: 0, input: 0, output: 0, reasoning: 0, cache: { read: 0, write: 0 } },
+          finish: "stop", time: { created: now, completed: now },
+        }
+        await Session.updateMessage(assistantMsg)
+        const textPart: MessageV2.TextPart = {
+          id: PartID.ascending(), sessionID: input.sessionID, messageID: assistantMsg.id,
+          type: "text", text: responseText, time: { start: now, end: now },
+        }
+        await Session.updatePart(textPart)
+        Bus.publish(Command.Event.Executed, {
+          name: input.command, sessionID: input.sessionID,
+          arguments: input.arguments, messageID: assistantMsg.id,
+        })
+        return { info: assistantMsg, parts: [textPart] } as MessageV2.WithParts
+      }
       const trimmed = input.arguments.trim()
 
       if (!trimmed) {
@@ -2608,7 +2628,7 @@ NOTE: At any point in time through this workflow you should feel free to ask the
             const icon = s.status === "connected" ? "\u2713" : "\u25cb"
             const label =
               s.status === "failed"
-                ? icon + " " + s.status + " (" + (s as any).error + ")"
+                ? icon + " " + s.status + " (" + s.error + ")"
                 : icon + " " + s.status
             return "| `" + srv + "` | " + label + " |"
           })
@@ -2617,26 +2637,7 @@ NOTE: At any point in time through this workflow you should feel free to ask the
           ? "MCP servers:\n\n| Server | Status |\n|---|---|\n" + rows
           : "No MCP servers configured."
 
-        const now = Date.now()
-        const assistantMsg: MessageV2.Assistant = {
-          id: MessageID.ascending(), role: "assistant", sessionID: input.sessionID,
-          parentID: userMsg.info.id, modelID: model.modelID, providerID: model.providerID,
-          mode: "builder", agent: "builder",
-          path: { cwd: Instance.directory, root: Instance.worktree },
-          cost: 0, tokens: { total: 0, input: 0, output: 0, reasoning: 0, cache: { read: 0, write: 0 } },
-          finish: "stop", time: { created: now, completed: now },
-        }
-        await Session.updateMessage(assistantMsg)
-        const textPart: MessageV2.TextPart = {
-          id: PartID.ascending(), sessionID: input.sessionID, messageID: assistantMsg.id,
-          type: "text", text: responseText, time: { start: now, end: now },
-        }
-        await Session.updatePart(textPart)
-        Bus.publish(Command.Event.Executed, {
-          name: input.command, sessionID: input.sessionID,
-          arguments: input.arguments, messageID: assistantMsg.id,
-        })
-        return { info: assistantMsg, parts: [textPart] } as MessageV2.WithParts
+        return respond(userMsg.info.id, responseText)
       }
 
       const subMatch = trimmed.match(/^(enable|disable)\s+(\S+)/)
@@ -2660,33 +2661,15 @@ NOTE: At any point in time through this workflow you should feel free to ask the
           if (entry?.status === "connected") {
             responseText = `MCP server **${name}** enabled. Status: connected.`
           } else {
-            responseText = `Attempted to enable MCP server **${name}**. Status: ${entry?.status ?? "unknown"}${(entry as any)?.error ? " — " + (entry as any).error : "."}.`
+            const errSuffix = entry?.status === "failed" ? " — " + entry.error : ""
+          responseText = `Attempted to enable MCP server **${name}**. Status: ${entry?.status ?? "unknown"}${errSuffix}.`
           }
         } else {
           await MCP.disconnect(name)
           responseText = `MCP server **${name}** disabled.`
         }
 
-        const now = Date.now()
-        const assistantMsg: MessageV2.Assistant = {
-          id: MessageID.ascending(), role: "assistant", sessionID: input.sessionID,
-          parentID: userMsg.info.id, modelID: model.modelID, providerID: model.providerID,
-          mode: "builder", agent: "builder",
-          path: { cwd: Instance.directory, root: Instance.worktree },
-          cost: 0, tokens: { total: 0, input: 0, output: 0, reasoning: 0, cache: { read: 0, write: 0 } },
-          finish: "stop", time: { created: now, completed: now },
-        }
-        await Session.updateMessage(assistantMsg)
-        const textPart: MessageV2.TextPart = {
-          id: PartID.ascending(), sessionID: input.sessionID, messageID: assistantMsg.id,
-          type: "text", text: responseText, time: { start: now, end: now },
-        }
-        await Session.updatePart(textPart)
-        Bus.publish(Command.Event.Executed, {
-          name: input.command, sessionID: input.sessionID,
-          arguments: input.arguments, messageID: assistantMsg.id,
-        })
-        return { info: assistantMsg, parts: [textPart] } as MessageV2.WithParts
+        return respond(userMsg.info.id, responseText)
       }
     }
     // altimate_change end

packages/opencode/src/util/glob.ts

@@ -8,6 +8,7 @@ export namespace Glob {
     include?: "file" | "all"
     dot?: boolean
     symlink?: boolean
+    ignore?: string | string[]
   }
 
   function toGlobOptions(options: Options): GlobOptions {
@@ -17,6 +18,7 @@ export namespace Glob {
       dot: options.dot,
       follow: options.symlink ?? false,
       nodir: options.include !== "all",
+      ignore: options.ignore,
     }
   }