feat: IDE-aware datamate transport, enabled-state persistence, and /mcps command#893
Conversation
|
This PR doesn't fully meet our contributing guidelines and PR template. What needs to be fixed:
Please edit this PR description to address the above within 2 hours, or it will be automatically closed. If you believe this was flagged incorrectly, please let a maintainer know. |
|
Thanks for your contribution! This PR doesn't have a linked issue. All PRs must reference an existing issue. Please:
See CONTRIBUTING.md for details. |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
E2E Verification — PASSED ✓Full pipeline tested end-to-end in the sandbox: Test setup:
Results: What was verified:
|
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
15 similar comments
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
- Add stdio transport support for datamate MCP (vs HTTP-only before) - Single-gateway mode: when .vscode/mcp.json has "datamate" key, always use it as server name — prevents duplicate tool sets from extension - syncDatamateUrlFromVscodeMcp: use updatedAt field as change signal for the "datamate" entry (works for both stdio and HTTP), URL comparison for all other remote entries - Strip ALTIMATE_EXTENSION_RPC from persisted mcp-discover configs to avoid stale socket paths across VS Code sessions - persistMcpEnabled: write enabled/disabled flag to disk on MCP connect/disconnect so it survives session restarts - Add /altimate/mcp/reload-datamate endpoint to re-sync and reconnect without full server restart - MCP.ToolsChanged subscription in prompt loop for traceability - Merge main: preserve trace consumer in serve.ts, restore exports on isAnthropicLikeModel and insertReminders
altimate-harness-bot
Bot
force-pushed
the
fix/datamate-stdio-local-transport
branch
from
a71092d to
0b6cc26
Compare
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
1 similar comment
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
/mcps result after fixAfter running
|
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
1 similar comment
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
1 similar comment
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
altimate-harness-bot
Bot
changed the title
ralphstodomingo
left a comment
ralphstodomingo
left a comment
There was a problem hiding this comment.
Reviewed the datamate transport + /mcps PR. Solid; the two concerns below are about the mcp.json scan and a sync-classification edge, the rest are nits/cleanups.
| .sort() | ||
| } catch { | ||
| log.warn("findAllMcpJsonFiles: glob scan failed", { cwd: projectRootDir }) | ||
| return [] |
There was a problem hiding this comment.
Concern: Glob.scan("**/mcp.json", { dot: true }) descends into node_modules/.git/dist and only filters them after the walk (the Glob.Options wrapper never forwards ignore to prune). This runs on every handleAdd, serve startup, and every /altimate/mcp/reload-datamate POST — a real repeated latency hit on big monorepos. The sibling mcp/discover.ts already uses a curated SOURCES list of exact relative paths; reuse that, or add ignore forwarding to toGlobOptions.
There was a problem hiding this comment.
Fixed: added ignore field to Glob.Options and wired it through to toGlobOptions in util/glob.ts. Updated findAllMcpJsonFiles to pass the exclude patterns directly to the glob walk — no more post-filter.
| } else { | ||
| // http / streamable-http / sse → remote | ||
| newEntry = { | ||
| type: "remote", |
There was a problem hiding this comment.
Concern (latent): the stdio branch is gated on type === "stdio" exactly; any datamate entry without that exact type falls into the else and writes { type: "remote", url: undefined } — corrupting a Cursor/mcpServers-shaped { command, args } entry. readDatamateTransportFromIde classifies the opposite (correct) way: command present → stdio. Latent today because the only updatedAt writer always also writes type: "stdio", but the two functions disagree. Suggest mirroring the reader and guarding the remote branch with typeof url === "string".
There was a problem hiding this comment.
Fixed: changed datamateVscode["type"] === "stdio" to "command" in datamateVscode. Now correctly identifies local/stdio transports by the presence of a command key rather than relying on an explicit type field (which VS Code omits when it defaults to stdio).
| const icon = s.status === "connected" ? "\u2713" : "\u25cb" | ||
| const label = | ||
| s.status === "failed" | ||
| ? icon + " " + s.status + " (" + (s as any).error + ")" |
There was a problem hiding this comment.
Convention: MCP.Status is a discriminated union where error exists only on failed/needs_client_registration. Here s.status === "failed" already narrows s, so (s as any).error is pure noise → use s.error. (The /mcps enable handler has the same cast in an else branch where error may be absent — there as any masks an unsound access; narrow explicitly with entry?.status === "failed" || entry?.status === "needs_client_registration".)
There was a problem hiding this comment.
Fixed: the s.status === "failed" ternary already narrows s to the failed variant — replaced (s as any).error with s.error directly. Same for the enable handler: replaced (entry as any)?.error with a proper narrowed check (entry?.status === "failed" ? entry.error : "").
| // 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" |
There was a problem hiding this comment.
Nit: this top-level export { syncDatamateUrlFromVscodeMcp } from "../../altimate/datamate-transport" has no consumers — all three real callers import from the transport module directly, and serve's own run-handler re-imports it dynamically. The "avoids duplication" comment is misleading (dedup is already achieved by the shared module). Remove the re-export.
There was a problem hiding this comment.
Fixed: removed the dead re-export. server.ts already imports syncDatamateUrlFromVscodeMcp directly from altimate/datamate-transport — nothing imports it from serve.ts.
| async function persistMcpEnabled(name: string, enabled: boolean): Promise<void> { | ||
| try { | ||
| const paths = await findAllConfigPaths(Instance.directory, Global.Path.config) | ||
| let found = false |
There was a problem hiding this comment.
Nit: persistMcpEnabled writes { ...entry, enabled } where entry comes from Config.get() (a deep merge across global/project/managed/discovery), so it re-serializes merged fields and can leak fields across config files. This is the exact failure the disconnect path in this same PR worked around via readMcpEntryFromDisk. Suggest reading the on-disk entry and writing { ...diskEntry, enabled } so only the flag changes.
There was a problem hiding this comment.
Fixed: moved readMcpEntryFromDisk to mcp/config.ts (where it logically belongs alongside the other config-file utilities) and updated persistMcpEnabled to use it instead of Config.get(). server.ts import updated to pull from the new location.
| // that is invalidated by the notification handler that publishes this event, | ||
| // 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. |
There was a problem hiding this comment.
Nit: the ToolsChanged subscriber sets toolsNeedRefresh = true, but the loop-top only logs and resets it — no refresh is gated on it (the comment confirms MCP.tools() is already invalidated elsewhere). A reader will reasonably assume it gates a refresh. Drop the flag and keep just the log, or comment that it's intentionally observability-only.
There was a problem hiding this comment.
Fixed: removed the toolsNeedRefresh flag and the loop-top reset block. The subscriber now just logs directly — the boolean was noise since the tools naturally refresh via resolveTools() on the next LLM turn as the comment already explained.
| await MCP.connect(name) | ||
| const statusMap = await MCP.status() | ||
| const entry = statusMap[name] | ||
| if (entry?.status === "connected") { |
There was a problem hiding this comment.
Nit: the two /mcps branches build a byte-identical ~20-line assistant-message block (Assistant skeleton + TextPart + updateMessage/updatePart + Bus.publish + return WithParts); only responseText varies. Extract a local respond(userMsg, responseText) helper and call it from both.
There was a problem hiding this comment.
Fixed: extracted a respond(parentID, responseText) helper that captures input and model via closure. Both /mcps branches now call it instead of inlining the 20-line block.
…disk, dedup respond()
E2E Verification — respond() fix confirmedRan Playwright E2E against sandbox using this PR's branch ( Primary fix verified ✅The
Full E2E PASSScreenshotsChat connected (altimate-code server started at port 16385+): Discovery result:
[verified: Playwright E2E ran to completion, exit code 0] |
sahrizvi
left a comment
sahrizvi
left a comment
There was a problem hiding this comment.
Code Review — fix/datamate-stdio-local-transport
🛑 Requesting changes — 1 blocker + 7 majors.
Blocker
readMcpEntryFromDisksilently stripscommandarrays andenvironmentobjects (mcp/config.ts:126), causing disk corruption on/mcps enable|disablefor local MCPs. Same pattern in two sibling sites indatamate-transport.ts— one of which (:269) silently stripsheaderson everyservestartup viasyncDatamateUrlFromVscodeMcp. Root cause:jsonc-parserNode.valueisundefinedfor arrays/objects — usegetNodeValue(node)instead.
Other concerns
/altimate/mcp/reload-datamatereturns HTTP 200 on internal errorpersistMcpEnabledread-modify-write has no file lock — realistic race between/mcps enableand the extension'sreload-datamatePOST- Glob for
**/mcp.jsonshould be constrained to explicit IDE config paths (defense in depth + perf) syncDatamateUrlFromVscodeMcpdropstimeout,oauth, and other non-target fields on rewrite/mcps enable|disabledoesn't validate the server name exists in config
Inline comments below have details, repros, and suggested fixes.
Required follow-up tests
readMcpEntryFromDiskround-trip withcommand: string[]+environment: {...}(would have caught the blocker)- Remote entry with
headers: {...}survivessyncDatamateUrlFromVscodeMcp /mcps enable <local>followed byConfig.Info.safeParseon disk state — must not throwPOST /altimate/mcp/reload-datamatereturns 500 on internal error- Concurrent
persistMcpEnabledcalls don't lose writes
| 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 |
There was a problem hiding this comment.
🔴 BLOCKER — silent data corruption on disk
prop.children[1]!.value returns undefined for jsonc-parser nodes of type array or object. Node.value is populated only for primitives (string/number/boolean/null).
Repro (verified against this branch with jsonc-parser 3.3.1):
Input: { "type": "local",
"command": ["node", "/path/to/datamate"],
"environment": { "DATAMATE_KEY": "abc" },
"enabled": false }
readMcpEntryFromDisk returns:
{ "type": "local", "enabled": false } // command + environment GONE
Three failure modes introduced by this PR:
/mcps enable <local-server>→MCP.connect→persistMcpEnabled(true)writes corrupted entry. Next session boot failsConfig.Info.safeParsebecauseMcpLocal.commandis required./mcps disable <local-server>→MCP.disconnect→persistMcpEnabled(false)— same outcome.POST /altimate/mcp/reload-datamatereads via this function then callsMCP.add(name, freshEntry);create()atmcp/index.ts:527doesconst [cmd, ...args] = mcp.commandand throwsTypeError.
The harness E2E missed this because same-session reads s.status[name] before persistMcpEnabled runs. Corruption only surfaces on next session restart for local stdio MCPs — exactly the path this PR enables.
Fix:
import { modify, applyEdits, parseTree, findNodeAtLocation, getNodeValue } from "jsonc-parser"
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") return undefined
return getNodeValue(node) as Config.Mcp
}Apply the same fix at the two sibling walkers in datamate-transport.ts (separate inline comments below).
Add a unit test pinning round-trips for local (with command + environment) and remote (with headers) entries.
There was a problem hiding this comment.
Fixed in 4a4a1751b. readMcpEntryFromDisk now uses getNodeValue(node) instead of the manual children walk, so command arrays and environment objects are preserved. Confirmed with jsonc-parser 3.3.1 that the old walk returned {type, enabled} (command/environment dropped) while getNodeValue returns the full entry. Added readMcpEntryFromDisk round-trip unit tests in test/mcp/config.test.ts, including the exact persistMcpEnabled read→{...entry, enabled}→write flow that previously corrupted local entries (the test fails without the fix). Also verified live in code-server: /mcps disable then /mcps enable round-trips with url preserved.
| for (const prop of existingNode.children) { | ||
| if (prop.type !== "property" || !prop.children) continue | ||
| const k = prop.children[0]!.value as string | ||
| if (k === "updatedAt") existingUpdatedAt = prop.children[1]!.value as string |
There was a problem hiding this comment.
Same prop.children[1]!.value pattern as mcp/config.ts:126.
Today it's safe by accident — only the scalar fields updatedAt and enabled are read here. But the pattern is fragile: any future field added to this walk that happens to be an array or object will silently return undefined.
Use getNodeValue(prop.children[1]!) (or the helper from the blocker fix) and shift this to a typed extraction. Same fix needed below at line 269.
There was a problem hiding this comment.
Fixed in 4a4a175. This walker now reads the existing datamate entry via getNodeValue (and the rebuild preserves non-scalar fields — see the :200 thread), so it's no longer reliant on only-scalar fields being read.
| const entry: Record<string, unknown> = {} | ||
| for (const prop of entryNode.children) { | ||
| if (prop.type === "property" && prop.children) { | ||
| entry[prop.children[0]!.value as string] = prop.children[1]!.value |
There was a problem hiding this comment.
Same root cause as the readMcpEntryFromDisk blocker — this one silently strips headers.
This walks an existing remote MCP entry to rehydrate it before rewriting url + updatedAt. If the entry has headers: {...} (per config.ts:654) or oauth: {...}, those fields are dropped on every sync because prop.children[1]!.value is undefined for object nodes.
Fix: use getNodeValue on the entry node:
const entry = getNodeValue(entryNode) as Record<string, unknown>There was a problem hiding this comment.
Fixed in 4a4a1751b. Replaced the children walk with const entry = getNodeValue(entryNode), so headers/oauth (and any future object fields) are preserved when rewriting url + updatedAt.
| // rewrites its MCP config. Re-reading it here keeps altimate-code.json in sync | ||
| // without requiring any user action. | ||
| const { syncDatamateUrlFromVscodeMcp } = await import("../../altimate/datamate-transport") | ||
| await syncDatamateUrlFromVscodeMcp(process.cwd()) |
There was a problem hiding this comment.
Automated data loss on serve startup.
syncDatamateUrlFromVscodeMcp runs on every serve startup — which is how the VS Code extension launches altimate-code. Because the function uses the broken walker in datamate-transport.ts:269, simply starting the extension can wipe headers, oauth, or any future object/array fields from existing datamate-code.json remote MCP entries, with no user action.
This is the same root cause as the blocker on mcp/config.ts:126. The fix there resolves this too — but it's worth noting that corruption fires automatically on startup, not just on user-driven /mcps enable|disable.
There was a problem hiding this comment.
Resolved by the getNodeValue fixes in datamate-transport.ts (the :269 walker) plus the field-preservation change in the datamate-entry rebuild (:200), both in 4a4a1751b. syncDatamateUrlFromVscodeMcp no longer drops object/array fields on serve startup. Verified: starting the extension and toggling the entry keeps url/enabled/extra fields intact.
| } catch (err) { | ||
| const error = err instanceof Error ? err.message : String(err) | ||
| log.error("reload-datamate: failed", { error }) | ||
| return c.json({ ok: false, error }) |
There was a problem hiding this comment.
HTTP 200 on internal error — VS Code extension can't tell success from failure.
return c.json({ ok: false, error }) defaults to HTTP 200. The extension consumes the JSON body to know what happened, but standard HTTP-error handling (retry, alerting, etc.) sees 200 and moves on.
Combined with the blocker — where reconnection throws for local transports — the extension will report live-reload as succeeded while the underlying reconnect crashed.
Fix:
return c.json({ ok: false, error }, 500)There was a problem hiding this comment.
Fixed in 4a4a1751b — reload-datamate now returns c.json({ ok: false, error }, 500) on the error path.
| } | ||
|
|
||
| // altimate_change start — persist enabled/disabled to disk so it survives session restarts | ||
| async function persistMcpEnabled(name: string, enabled: boolean): Promise<void> { |
There was a problem hiding this comment.
No file lock — concurrent writes race.
persistMcpEnabled does read → modify → write with no synchronization. Two concurrent callers can interleave:
- Session A reads
{enabled: false} - Session B reads
{enabled: false} - Session A writes
{enabled: true} - Session B writes
{enabled: true}based on the stale read — and if A wrote different fields too, those are lost
Concrete trigger: the VS Code extension fires POST /altimate/mcp/reload-datamate while the user types /mcps enable in the same session — both routes write through this function. addMcpToConfig is also non-atomic (read text, apply edits, write).
The codebase already has util/lock.ts (Lock) and util/flock.ts (Flock) used by plugin/install, provider/models, etc. Reuse one:
async function persistMcpEnabled(name, enabled) {
await Flock.acquire(configPath, async () => {
// existing logic
})
}There was a problem hiding this comment.
Fixed in 4a4a1751b. persistMcpEnabled is now serialized through a module-level promise chain (persistChain), so concurrent enable/disable callers can't interleave the read-modify-write.
| const cmd = typeof entry["command"] === "string" ? entry["command"] : undefined | ||
| const args = Array.isArray(entry["args"]) ? (entry["args"] as string[]) : [] | ||
| if (cmd) { | ||
| return { type: "local", command: [cmd, ...args] } |
There was a problem hiding this comment.
Defense in depth — narrow the glob to explicit IDE config paths.
readDatamateTransportFromIde reads command and args from any mcp.json found via the **/mcp.json glob (only node_modules, .git, dist, build, .pnpm are excluded) and passes them straight to StdioClientTransport at mcp/index.ts:527. The ignore list also misses target/, .next/, out/, vendor/, coverage/, .venv/, .turbo/, .cache/, __pycache__/.
This widens both the attack surface (an mcp.json in a third-party fixture or vendored package gets loaded) and the performance footprint (every handleAdd / serve startup / reload-datamate POST walks the tree).
Fix: reuse the curated SOURCES list already used by mcp/discover.ts — only IDEs we explicitly support ever write a datamate entry, so the glob is overkill. Kills two birds.
There was a problem hiding this comment.
Addressed in 4a4a1751b. Widened the ignore list for the **/mcp.json scan to also exclude target, .next, out, vendor, coverage, .venv, and .turbo, keeping the scan to source the user actually authors rather than vendored/generated trees. Kept the IDE-agnostic glob (per #599807ae4) rather than hardcoding paths; open to tightening to explicit IDE dirs if you'd prefer.
| typeof datamateVscode["command"] === "string" | ||
| ? (datamateVscode["command"] as string) | ||
| : DATAMATE_KEY | ||
| newEntry = { |
There was a problem hiding this comment.
syncDatamateUrlFromVscodeMcp overwrites the entry, losing timeout, oauth, and other fields.
This constructs newEntry from scratch using only type, command, args, env, updatedAt, and (separately) preserved enabled. Anything else on the existing altimate-code.json datamate entry — timeout, oauth, future fields — is silently dropped because addMcpToConfig does modify(path, value), which is a set, not a merge.
Fix: read the existing entry (via the corrected readMcpEntryFromDisk from the blocker fix) and spread it before overlaying new fields:
const existing = (existingNode ? getNodeValue(existingNode) : {}) as Record<string, unknown>
newEntry = {
...existing,
type: "local",
command: [cmd, ...((datamateVscode["args"] as string[]) ?? [])],
...(Object.keys(restEnv).length > 0 ? { environment: restEnv } : {}),
updatedAt: vscodeUpdatedAt,
}There was a problem hiding this comment.
Fixed in 4a4a175. The rebuild now carries forward the existing entry's non-transport fields: it spreads a preserved object (everything except type/command/args/environment/url/updatedAt) onto the new entry, so enabled, timeout, oauth, etc. survive the sync.
| let responseText: string | ||
|
|
||
| if (isEnable) { | ||
| await MCP.connect(name) |
There was a problem hiding this comment.
/mcps enable|disable doesn't validate the server name exists in config.
MCP.connect("nonexistent") at mcp/index.ts:738-741 logs an error and returns silently. The user sees "Attempted to enable MCP server **nonexistent**. Status: unknown." — there's no signal that they typo'd the name.
Fix:
const cfg = await Config.get()
if (!cfg.mcp?.[name]) {
return respond(userMsg.info.id, `MCP server **${name}** not found in config.`, model)
}Same check needed in the else branch for /mcps disable.
There was a problem hiding this comment.
Fixed in 4a4a1751b — the /mcps enable|disable handler now checks cfg.mcp[name] first and responds "MCP server not found in config. Known servers: …" instead of silently no-opping. Note: this is the server/TUI command path. In the VS Code extension the webview intercepts /mcps enable|disable client-side (separate repo), so I'll mirror this validation there too.
`/discover-and-add-mcps` found no servers (and auto-discovery added none) for non-git projects: `Instance.worktree` resolves to "/" when there is no `.git`, so the scan looked under the filesystem root and missed the workspace's `.vscode/mcp.json` (datamate). The `add` action also resolved its project-scoped config path against "/". - `mcp-discover.ts`: use `Instance.directory` (project root) instead of `Instance.worktree` for the discovery scan, persisted-name read, and the project-scoped config write target - `config.ts`: same `Instance.worktree` -> `Instance.directory` for the background auto-discovery caller - `discover.ts`: replace the hardcoded IDE paths with a recursive `**/mcp.json` glob scan rooted at the project directory (IDE-agnostic, trying both `servers` and `mcpServers` keys), keeping the non-`mcp.json` sources (`.mcp.json`, `.gemini/settings.json`, `~/.claude.json`) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… writes Resolves the review findings on the `/mcps` enable/disable persistence and datamate transport sync paths. - `mcp/config.ts`: `readMcpEntryFromDisk` now uses `getNodeValue` instead of a manual children walk. `jsonc-parser` only populates `Node.value` for primitives, so the old walk silently dropped `command` arrays and `environment`/`headers` objects — corrupting local stdio (and remote-with- headers) entries on the next write via `persistMcpEnabled`. - `altimate/datamate-transport.ts`: same `getNodeValue` fix for the two sibling walkers; preserve non-transport fields (`enabled`, `timeout`, `oauth`, …) when rebuilding the synced datamate entry; widen the `**/mcp.json` ignore list (`target`, `.next`, `out`, `vendor`, `coverage`, `.venv`, `.turbo`). - `mcp/index.ts`: serialize `persistMcpEnabled` (promise chain) so concurrent enable/disable can't interleave read-modify-write and clobber each other. - `server/server.ts`: `reload-datamate` returns HTTP 500 (not 200) on error so the extension can distinguish failure. - `session/prompt.ts`: `/mcps enable|disable` validates the server name against config and reports clearly when it's unknown (was silent). - `test/mcp/config.test.ts`: add `readMcpEntryFromDisk` round-trip tests, including the persist-enabled flow that previously corrupted local entries. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The `**/mcp.json` glob scan sorted purely alphabetically, which let `.cursor/mcp.json` override `.vscode/mcp.json` for same-named servers — a regression vs the previous fixed-source order (.vscode > .cursor > .github/copilot). Caught by the `discover-adversarial` precedence test. Order globbed files by IDE precedence first, then alphabetically, restoring first-source-wins semantics while staying IDE-agnostic for any other mcp.json. Also align the ignore list with `datamate-transport.ts`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…egrity tests) Two pre-existing marker imbalances on the branch failed the "Require-markers regression backstop" job and the `altimate_change marker integrity` tests (origin/main is balanced; these were introduced by earlier merge commits on this branch): - `cli/cmd/serve.ts` (6 start / 4 end): close the trace-import region after its import, and close the branding region after the rebranded log line (the sync-datamate region stays nested inside it). - `session/prompt.ts` (43 start / 44 end): remove a duplicate `altimate_change end` left by a merge — the MCP-ToolsChanged region is already closed before `while (true)`, and the SessionStatus region right after has its own pair. Comment-only changes; no behavior impact. Verified with `bun run script/upstream/analyze.ts --require-markers --strict` (38/38) and the marker-integrity suites (118/0). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
❌ Tests — Failures DetectedTypeScript — 15 failure(s)
Next StepPlease address the failing cases above and re-run verification. cc @app/altimate-harness-bot |
4 participants
What this does
Four related improvements shipped together:
1. IDE-aware datamate transport (
datamate.ts,datamate-transport.ts)When
handleAddruns and adatamateentry already exists in any IDEmcp.json(VS Code, Cursor, Copilot, etc.), altimate-code reuses the exact command from that file instead of building a new cloud URL config:"datamate"in extension mode; per-datamate cloud naming only in standalone/CLI modeDetection scans
**/mcp.jsonunder the project root (readDatamateTransportFromIde). Falls back to cloud config when no IDE entry is found.2. Fix: discovered servers come up enabled and connected (
mcp-discover.ts)/discover-and-add-mcpswas writingenabled: false(the discovery-time safety default) even when the user explicitly chose to add a server. Now:enabled: true+updatedAt(ISO timestamp) when the user confirms addingMCP.connect(name)immediately —/mcpsshowsconnectedin the same session without a restartALTIMATE_EXTENSION_RPCfrom the environment before persisting (socket path is session-specific)3. Persist enabled/disabled state across restarts (
mcp/index.ts)MCP.connect()andMCP.disconnect()now callpersistMcpEnabled(name, true/false), writing the flag back to whichever config file owns the entry. Enabled/disabled state survives session restarts.4.
/mcpscommand — list and toggle MCP servers (command/index.ts,session/prompt.ts)New slash command, handled in the session layer without LLM tokens:
/mcps— lists all configured MCP servers with live connection status/mcps enable <name>— connects and persistsenabled: true/mcps disable <name>— disconnects and persistsenabled: false5. Live reload on IDE mcp.json change (
server/server.ts,serve.ts)POST /altimate/mcp/reload-datamate— triggered by the VS Code extension after writingmcp.json; syncs and reconnects with the fresh entry (reads from disk to bypass the stale in-memory Config singleton)syncDatamateUrlFromVscodeMcpalso runs at serve startup to catch changes from a VS Code window restartRelated
/mcpsUI + live reload trigger)Requested by @saravmajestic via harness