fix: carry the IDE entry's env when wiring the datamate stdio MCP server - #1081
fix: carry the IDE entry's env when wiring the datamate stdio MCP server#1081ralphstodomingo wants to merge 12 commits into
Conversation
`datamate_manager add` reused the command + args from the IDE's `mcp.json` `datamate` entry but dropped its `env` block, both in the immediate spawn and in the entry persisted to `.altimate-code/altimate-code.json`. On desktop editors the command is the editor's Electron binary and `env` carries `ELECTRON_RUN_AS_NODE=1` — spawned without it, the editor GUI boots and opens `datamate-cli.js` as a document, the MCP client reports `-32000 Connection closed`, and the broken persisted entry re-pops the file on every subsequent session launch. - `readDatamateTransportFromIde` now returns the entry's env (minus `ALTIMATE_EXTENSION_RPC`, mirroring the sync path) and `updatedAt`; `handleAdd` carries the env into the runtime config and persists it as `environment`, plus `updatedAt` on disk so the sync recognizes the entry as current. - The sync path's inline env-strip is extracted into the shared `extractSpawnEnvironment` helper so both paths stay in lockstep. - The TUI worker and `run` now run `syncDatamateUrlFromVscodeMcp` before the first session (as `serve` already did), so entries already persisted without `environment` self-heal on the next launch.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughDatamate local transports now preserve filtered environment variables and ChangesDatamate synchronization
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant RunCommand
participant DatamateTransport
participant MCPConfig
participant DatamateGateway
RunCommand->>DatamateTransport: resolve project root
RunCommand->>MCPConfig: synchronize Datamate entry
MCPConfig->>DatamateTransport: read command, environment, and updatedAt
DatamateTransport-->>MCPConfig: return filtered transport metadata
MCPConfig->>DatamateGateway: persist refreshed entry
DatamateGateway-->>RunCommand: complete or suppress synchronization error
RunCommand->>DatamateGateway: start local session
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
github-actions
Bot
added
needs:compliance
contributor
needs:issue
and removed
needs:compliance
labels
|
Thanks for updating your PR! It now meets our contributing guidelines. 👍 |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
There was a problem hiding this comment.
Pull request overview
This PR fixes a desktop-editor regression where the IDE-provided datamate stdio MCP entry’s env (notably ELECTRON_RUN_AS_NODE=1) was dropped when wiring/persisting the server, causing Electron to boot the editor UI and open datamate-cli.js as a tab, and leading to -32000 Connection closed. It also expands the “heal from .vscode/mcp.json” sync behavior so terminal entrypoints (run/TUI worker) self-repair already-persisted broken entries, matching serve startup behavior.
Changes:
- Carry the IDE
env(minusALTIMATE_EXTENSION_RPC) andupdatedAtthroughreadDatamateTransportFromIde,datamate_manager addruntime wiring, and persisted config. - Deduplicate env-stripping logic into a shared
extractSpawnEnvironment()helper to keepaddand sync paths aligned. - Trigger
syncDatamateUrlFromVscodeMcpearlier forrunand the TUI worker so previously-broken persisted entries self-heal on next launch.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/opencode/src/altimate/datamate-transport.ts | Adds env + updatedAt propagation for IDE datamate stdio entries; factors env normalization into extractSpawnEnvironment; updates sync to use shared env extraction. |
| packages/opencode/src/altimate/tools/datamate.ts | Ensures datamate_manager add carries environment into runtime MCP config and persists environment + updatedAt to disk. |
| packages/opencode/src/cli/tui/worker.ts | Adds a boot-time datamate sync gate so the worker doesn’t serve requests / start external server mode until the heal attempt finishes. |
| packages/opencode/src/cli/cmd/run.ts | Runs the same datamate sync before bootstrapping a session to self-heal env-less persisted entries. |
| packages/opencode/test/release-validation/mcp-datamate-stdio-env.test.ts | Adds regression coverage for env carry-through, stripping rules, back-compat, and sync parity. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Code Review SummaryStatus: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (4 files)
Fix these issues in Kilo Cloud Previous Review Summaries (11 snapshots, latest commit 692417a)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 692417a)Status: No Issues Found | Recommendation: Merge Incremental review of The commit resolves the prior review's only SUGGESTION: the duplicated blank-tombstone predicate is now a single Files Reviewed (1 file)
Previous review (commit 9b16734)Status: 1 Issue Found | Recommendation: Address before merge Incremental review of The extension blanks Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (2 files)
Fix these issues in Kilo Cloud Previous review (commit 7f68428)Status: No Issues Found | Recommendation: Merge Incremental review of Test-only change: the adversarial Files Reviewed (1 file)
Previous review (commit 42311f2)Status: No Issues Found | Recommendation: Merge Incremental review of The split is correct: Files Reviewed (2 files)
Previous review (commit 6625177)Status: No Issues Found | Recommendation: Merge Files Reviewed (6 files)
Previous review (commit 33b60d8)Status: 1 Issue Found | Recommendation: Merge (1 non-blocking suggestion) Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (3 files)
The incremental commit ( Fix these issues in Kilo Cloud Previous review (commit 37c3d2c)Status: No Issues Found | Recommendation: Merge The incremental commit ( Files Reviewed (1 file)
Previous review (commit 7bcc9b6)Status: 1 Issue Found | Recommendation: Merge (non-blocking suggestion) Overview
Issue Details (click to expand)SUGGESTION
The incremental commit (
Only a minor DRY suggestion remains. Fix these issues in Kilo Cloud Files Reviewed (3 files)
Previous review (commit 1cb8fad)Status: No Issues Found | Recommendation: Merge The incremental commit ( Behavior is verified identical at both call sites:
No new issues introduced; the Files Reviewed (2 files)
Previous review (commit 80d4ad4)Status: 1 Issue Found | Recommendation: Merge (non-blocking) Overview
The incremental changes (commit Issue Details (click to expand)SUGGESTION
Files Reviewed (5 files)
Fix these issues in Kilo Cloud Previous review (commit cbf4f65)Status: No Issues Found | Recommendation: Merge The fix correctly carries the IDE Files Reviewed (5 files)
Reviewed by deepseek-v4-pro · Input: 82.9K · Output: 25.9K · Cached: 1.2M Review guidance: REVIEW.md from base branch |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 5 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…root sync scope - TUI worker: the datamate heal is now sequenced strictly before `InstanceRuntime.load`/`Config.get()` (trace init awaits it), so the config read can neither race the non-atomic write nor cache the pre-heal entry — the first session connects with the healed config. - `datamate_manager add`: the in-config-but-not-connected branch refreshes the persisted entry from the current IDE transport (preserving user-managed fields) and connects via `MCP.add`, instead of `MCP.connect` which re-reads the stale in-memory entry. - Boot heals (`run`, TUI worker) scan from the containing git project root via the new `resolveDatamateSyncRoot`, not raw cwd — a session launched from a subdirectory now finds the root IDE config and persisted entry.
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
…dd refresh Both paths encode the same idea — entry fields re-derived from the IDE transport versus user-managed fields carried forward. A single exported set keeps them from silently diverging when a new transport field is added; the add-refresh path layers `enabled` on top since it re-derives that too.
There was a problem hiding this comment.
All reported issues were addressed across 5 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…rry updatedAt for remote - The add-refresh path wrote the merged entry (preserved headers/oauth/timeout + fresh transport) to disk but connected the live client with the bare transport config, dropping authentication and connection settings for the session being connected. MCP.add now receives the same merged entry as the disk write, matching the reload-datamate endpoint. - The remote transport variant now carries updatedAt like the local one, so a remote datamate added via datamate_manager is not rewritten once by the next boot's sync purely for the missing change signal.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/opencode/src/altimate/datamate-transport.ts (1)
277-284: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winSerialize MCP config writes before this sync path.
addMcpToConfigreadsmcpConfig, then writes withmodify+Filesystem.writewithout a lock. Concurrentdatamate_manager addwrites to the same server can overwrite newer fields such asenvironment,updatedAt, or user-managedheaders/oauth/timeout. Add a per-config-path lock or update queue that covers IDE sync anddatamate_manager add, and keep the lock shared whenresolveConfigPathpoints to the same file.🤖 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/opencode/src/altimate/datamate-transport.ts` around lines 277 - 284, Serialize the read-modify-write flow in addMcpToConfig with a per-config-path lock or update queue covering both IDE synchronization and datamate_manager add operations. Ensure resolveConfigPath results sharing the same file reuse the same lock, and hold it through mcpConfig reads, modify, and Filesystem.write so newer environment, updatedAt, headers, oauth, and timeout fields are not overwritten.Source: Coding guidelines
🤖 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/opencode/src/altimate/datamate-transport.ts`:
- Around line 23-24: Update syncDatamateUrlFromVscodeMcp to compare the datamate
entry’s TRANSPORT_IDENTITY_FIELDS whenever readDatamateTransportFromIde returns
a transport without vscodeUpdatedAt, while preserving timestamp-based
synchronization when the timestamp is present. Add a regression test covering a
timestamp-less IDE transport and verifying that altimate-code.json is
synchronized.
---
Outside diff comments:
In `@packages/opencode/src/altimate/datamate-transport.ts`:
- Around line 277-284: Serialize the read-modify-write flow in addMcpToConfig
with a per-config-path lock or update queue covering both IDE synchronization
and datamate_manager add operations. Ensure resolveConfigPath results sharing
the same file reuse the same lock, and hold it through mcpConfig reads, modify,
and Filesystem.write so newer environment, updatedAt, headers, oauth, and
timeout fields are not overwritten.
🪄 Autofix
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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5cbe1fe5-948b-4030-829b-bd6729445c96
📒 Files selected for processing (3)
packages/opencode/src/altimate/datamate-transport.tspackages/opencode/src/altimate/tools/datamate.tspackages/opencode/test/release-validation/mcp-datamate-stdio-env.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/opencode/test/release-validation/mcp-datamate-stdio-env.test.ts
- packages/opencode/src/altimate/tools/datamate.ts
|
Re CodeRabbit's outside-diff finding (serialize |
Both the refresh and new-entry branches persisted the transport's updatedAt with the same conditional spread; a single `updatedAtField` above the branch keeps them from drifting, and the disk-only rationale is documented once.
datamate_manager add supports scope "global", so a broken (env-less) datamate entry can live in the global altimate-code.json. It is spawned at session start like any merged config entry — reproducing the editor-tab pop — but the boot heal only rewrote the project config, so the entry never repaired (found by the bug reporter testing the fix: no environment block appeared). syncDatamateUrlFromVscodeMcp now heals every config file carrying a datamate entry via findAllConfigPaths (project, project subdirs, global), reporting the entry once. Sync tests pass an isolated global dir so test runs never touch the developer's real config.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/opencode/src/altimate/datamate-transport.ts`:
- Around line 331-332: Resolve the Git project root once at the start of
syncDatamateUrlFromVscodeMcp, then use that root instead of cwd for both
findAllMcpJsonFiles and findAllConfigPaths. Add a direct regression test
invoking syncDatamateUrlFromVscodeMcp from a nested directory and verify
root-level configuration files are synchronized.
🪄 Autofix
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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: edd49936-8519-4d1c-ac2f-62ff387826f9
📒 Files selected for processing (3)
packages/opencode/src/altimate/datamate-transport.tspackages/opencode/test/release-validation/mcp-datamate-893.test.tspackages/opencode/test/release-validation/mcp-datamate-stdio-env.test.ts
|
Field testing by the bug reporter surfaced a second gap, fixed in 33b60d8: |
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
…al root resolution, per-file isolation, global-aware reload - CONFIG_FILENAMES now mirrors every filename the config loader merges (adds altimate-code.jsonc and legacy config.json), so entries in those files are healed/removed/listed like the rest instead of loading as live config that tooling cannot see. - syncDatamateUrlFromVscodeMcp resolves the git project root itself, so every caller (serve, reload endpoint, TUI worker, run) handles nested invocations; the worker/run callers drop their now-redundant resolution. - One malformed config file no longer aborts the multi-file heal — each file is healed independently with a logged skip on failure. - The reload-datamate endpoint reads the fresh entry from any config file the sync covers (project, subdirs, global) instead of only the project path, so a healed global-only entry actually reconnects.
|
Re cubic's four P2s on the global-heal commit — all four were valid, addressed in 6625177:
Each has a regression test (nested-heal, malformed-continue, |
There was a problem hiding this comment.
All reported issues were addressed across 6 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Tip: cubic used a learning from your PR history. Let your coding agent read cubic learnings directly with the cubic MCP.
Re-trigger cubic
The config loader merges config.json only from the global config dir; the project loader reads only altimate-code.json/.jsonc and opencode.json/.jsonc. Listing config.json in the shared filename set made project-side discovery treat any unrelated project config.json as live config — and resolveConfigPath could return it as the write target for a fresh add, persisting an entry the loader would never load. Split the sets: GLOBAL_CONFIG_FILENAMES carries config.json, project candidates do not. Regression test asserts the global legacy file heals while a project-level config.json is left byte-identical.
|
cubic's follow-up P2 was correct and is fixed in 42311f2: the config loader merges |
The adversarial guard asserted the single-path read line verbatim; the endpoint now scans every config file the heal covers. The guarded contract — stale-singleton bypass via readMcpEntryFromDisk + MCP.add — is unchanged and still asserted.
The extension blanks the datamate entry to {} (not delete) in non-active-IDE
mcp.json files, and the sorted scan can reach the blanked file first (.cursor/
sorts before .vscode/). For the transport read that shadowed the real entry
behind the bare-marker fallback; for the sync it silently skipped the heal
entirely ({} has no updatedAt). Empty entries are tombstones, not transports —
both selection loops now skip them so the active IDE's real entry wins.
|
Re-verified from scratch (2026-08-25) on a pristine code-server harness (fresh mcp.json written by the extension, no prior altimate-code config), Electron contract simulated by the fake-electron shim, every scenario a real
That last row matters for anyone re-testing in the field: the heal is one-way, so once a machine has run this build (or re-added the entry with env), the unfixed binary stops reproducing too. A reporter who "can no longer reproduce" after testing the fix build is the expected outcome, not a sign the bug vanished — the bug reproduces deterministically on any pristine 0.8.10 install. |
…cans The transport read and the sync source selection each open-coded the "missing or empty datamate entry" check. One isBlankDatamateEntry helper keeps the two scans agreeing on what a tombstone is.
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 692417ac5f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| * spawn boots the editor GUI — which opens datamate-cli.js as a document in | ||
| * the IDE — instead of running it as a Node script. | ||
| */ | ||
| function extractSpawnEnvironment(raw: unknown): Record<string, string> | undefined { |
There was a problem hiding this comment.
CRITICAL — Security: a repo-local mcp.json can rewrite the global config's command + env, automatically, at every boot
extractSpawnEnvironment copies every string-valued key from the IDE entry's env, stripping only ALTIMATE_EXTENSION_RPC. Three mechanisms compound into a new trust-boundary crossing:
- The transport source is
findAllMcpJsonFiles()—Glob.scan("**/mcp.json")over the whole project tree (line 113), not just.vscode/. Anymcp.jsonin a cloned repo is a candidate, selected by sort order. - The heal now writes the global config (line 349, via
findAllConfigPaths), so the entry it produces outlives the project. - It now runs automatically at TUI/
runboot (cli/tui/worker.ts:52,cli/cmd/run.ts:953), where before it requiredserveboot or an explicitdatamate_manager add.
Net new capability: cloning an untrusted repo and opening a session is enough for a repo-controlled file to replace the global datamate entry's command, args, and env — spawned on every subsequent session, in every project. At spawn, mcp/index.ts:559 spreads ...mcp.environment after ...process.env, so the carried env also overrides altimate-code's own host environment (NODE_OPTIONS, LD_PRELOAD, DYLD_INSERT_LIBRARIES, PATH).
To be precise about the delta: the env carry itself is not new — main already had the same one-key denylist in the sync path. What this PR adds is env carry on the read/handleAdd path, boot-time triggering, and global-config scope. The rating rests on the latter two.
Suggested fix — two parts, both needed:
const SPAWN_ENV_ALLOWLIST = new Set(["ELECTRON_RUN_AS_NODE"])
for (const [key, value] of Object.entries(raw as Record<string, unknown>)) {
if (!SPAWN_ENV_ALLOWLIST.has(key)) continue
if (typeof value === "string") env[key] = value
}Second, bind provenance before writing: only auto-heal from an mcp.json that is the known extension-written location for this workspace, and never promote a project-file-derived transport into the global config without explicit user action.
There was a problem hiding this comment.
Agreed on all three mechanisms and the rating — thank you for the precise delta. Addressed in e96f26b, both parts:
Env: extractSpawnEnvironment is now an allowlist of exactly ELECTRON_RUN_AS_NODE (the extension writes nothing else we need); NODE_OPTIONS/LD_PRELOAD/PATH/etc. are dropped regardless of the file. Test: "only ELECTRON_RUN_AS_NODE is carried…".
Provenance: the transport source is now only the two locations the extension writes — **/.vscode/mcp.json and **/.cursor/mcp.json (not **/mcp.json), and every entry altimate-code derives from an IDE file is stamped managedBy: "altimate-ide" + sourceMcpJson. The boot heal rewrites project-scope entries as before, but touches a global entry only when its stamp matches the exact IDE file in hand; hand-added and legacy global entries are never rewritten from a project file — the explicit datamate_manager add is what (re)stamps them. Tests: hand-added global survives byte-identical; global managed from a different project's file is left alone; docs/examples/mcp.json is never a source. Re-verified end-to-end in the code-server harness (clone-shaped project file can no longer reach the global config at boot).
| * a session launched from a subdirectory would otherwise scan the subtree and | ||
| * miss both the IDE config and the persisted entry it needs to repair. | ||
| */ | ||
| export async function resolveDatamateSyncRoot(directory: string): Promise<string> { |
There was a problem hiding this comment.
MAJOR — Logic Error: unbounded walk to / can heal from an unrelated project's mcp.json
Filesystem.up({ targets: [".git"], start: directory }) passes no stop, and the helper only breaks on stop === current (util/filesystem.ts:324). From a directory with no .git ancestor the walk reaches /. If any ancestor is a git repo — a ~ under dotfiles management is the common case — that ancestor becomes the "project root". findAllMcpJsonFiles then globs its entire tree and takes the first non-blank datamate entry by sort order, which can be a different project's entry. Its command and env are then written into this project's and the global config.
The fix already has in-repo precedent at config/paths.ts:46-48:
Filesystem.up({ targets: [".git"], start: directory, stop: Global.Path.home })Separately: .git is a file in worktrees and submodules, so the current code resolves to the submodule root and misses the superproject's IDE config. Neither case is covered by the new tests.
There was a problem hiding this comment.
Fixed in e96f26b — the walk passes stop: Global.Path.home, and because Filesystem.up's stop is inclusive (the stop dir is still searched), a .git found at home is explicitly rejected so a dotfiles-managed ~ is never treated as a project. On .git files: the nearest one (worktree/submodule root) wins, which matches how Project.fromDirectory derives the sandbox, so altimate-code's own notion of the project and the heal's stay consistent — kept deliberately, now documented and covered. Tests added for both: home-as-git-repo → falls back to the launch dir; .git file → nearest root.
| } | ||
|
|
||
| let datamateHealed = false | ||
| for (const configPath of await findAllConfigPaths(cwd, globalConfigDir)) { |
There was a problem hiding this comment.
MAJOR — Logic Error: the heal rewrites the global config from a project-local IDE entry
This loop writes the transport derived from this project's mcp.json into every config carrying a datamate entry, including ~/.config/altimate-code/altimate-code.json.
Concretely: a user runs datamate_manager add --scope global (remote/cloud transport, no IDE involved). They later open any project that has a .vscode/mcp.json with a desktop stdio entry. The global entry is silently replaced by that project's Electron command + env. Every later session in every other project now spawns that binary. addMcpToConfig overwrites in place, so there is no undo.
The PR description justifies this with "a stale global entry pops the file just the same", which is true — but the remedy overreaches from "refresh an entry the IDE owns" to "replace any global entry from any project file".
Suggested fix: do not gate on "has updatedAt" — that is not evidence an entry is extension-managed, and an unrelated project or an attacker can supply one. Use explicit managed-provenance metadata bound to the workspace/source, or do not auto-rewrite the global config from project files at all.
Worth a test either way: a hand-added global entry should survive a project-local heal.
There was a problem hiding this comment.
Agreed — updatedAt was a change signal, not authorization, and the remedy overreached. Fixed in e96f26b with explicit provenance: entries written from an IDE transport carry managedBy + sourceMcpJson (stamped by datamate_manager add and by the sync itself); the boot heal only rewrites a global entry whose stamp matches the exact mcp.json it is healing from, and never creates one. Your scenario — add --scope global with a remote transport, then opening a project with a desktop stdio entry — now leaves the global entry byte-identical (test: "a hand-added GLOBAL entry (no provenance) survives a project-local heal byte-identical"; plus "managed from a DIFFERENT project's mcp.json is left alone"). Trade-off made explicit in the PR body: a legacy env-less global entry from 0.8.10 no longer self-heals at boot; one explicit add restamps and heals it (verified E2E).
| * active IDE's real entry is shadowed by whichever file sorts first | ||
| * (`.cursor/` sorts before `.vscode/`). | ||
| */ | ||
| function isBlankDatamateEntry(entry: unknown): boolean { |
There was a problem hiding this comment.
MAJOR — Bug: a non-empty but invalid IDE entry is selected as authoritative and writes a remote entry with no url
isBlankDatamateEntry only rejects {}. An entry like:
{ "servers": { "datamate": { "type": "stdio", "updatedAt": "T2" } } }has a key, so it passes the blank check and wins source selection over a valid entry in a file that sorts later. It then reaches the sync's transport branch, where "command" in datamateVscode is false, so it falls to the else (remote) branch and writes:
{ "type": "remote", "url": undefined, "updatedAt": "T2" }addMcpToConfig validates JSON syntax, not the MCP schema, so the malformed entry persists and breaks config loading on the next read. The bad entry also shadows any valid datamate entry that sorts after it.
Suggested fix: validate before selecting a source — local requires a non-empty string command, remote requires a non-empty url. Reject incomplete entries and continue scanning later files. Replacing "command" in datamateVscode with a validated discriminated transport would close this at the same time.
There was a problem hiding this comment.
Fixed in e96f26b exactly as suggested — source selection now goes through a validating parseIdeTransport: remote requires a non-empty string url, local a non-empty string command; blank tombstones and incomplete entries return null and scanning continues to later files. The "command" in datamateVscode branch is gone — the sync consumes the validated discriminated transport, so a url-less remote can no longer be written. Tests: the incomplete-entry-sorts-first case (valid later file wins, persisted entry has no url key), and a lone incomplete entry writes nothing. The PR893 "marker fallback" test was updated to assert null, with the rationale in the test.
Consensus review — remaining findings (minor / nit / cross-cutting)Verdict: request changes — 1 critical, 3 major, 12 minor, 4 nits. The critical and majors are posted as inline comments on The headline fix is correct and worth landing: carrying the IDE entry's Minorm1. m2. m3. m4. The boot heal runs an unpruned m5. Persisted m6. m7. m8. m9. Non-atomic read-modify-write, now with more boot callers — m10. m11. The adversarial test asserts on source text, not behavior — m12. Nits
Missing tests
What's done well
|
…auto-rewrite a global entry from a project file Review on this PR showed the boot-time heal plus the global-config heal had turned the pre-existing "scan any mcp.json in the tree" transport source into a trust-boundary crossing: a repo-local file could replace the global datamate entry's command/args/env automatically at every session start, and the carried env overrode the host process env at spawn. - Env carry is an allowlist (ELECTRON_RUN_AS_NODE only), not a denylist. - Transport sources are only the two locations the extension writes (**/.vscode/mcp.json, **/.cursor/mcp.json), parsed through a validating parseIdeTransport: local needs a non-empty command, remote a non-empty url; blank tombstones and incomplete entries are skipped instead of winning selection (an incomplete entry used to persist as a url-less remote entry). The old bare-marker fallback is gone. - Entries derived from an IDE file carry provenance (managedBy + sourceMcpJson). The boot heal rewrites project-scope entries as before but touches a GLOBAL entry only when its stamp matches the exact IDE file in hand; hand-added and legacy global entries are left alone, and an explicit datamate_manager add is what (re)stamps them. - The project-root walk stops at the home directory, and a .git at home is not a project; .git files (worktrees/submodules) resolve to the nearest root as in Project.fromDirectory. - Config heal candidates are every directory from the launch dir up to the root (plus the global dir under the provenance rule), matching the loader's upward walk so a nested package's own config is healed too. - mcp/config exposes findProjectConfigPaths/findGlobalConfigPaths. PR893 regression tests updated to the new contract (IDE-only locations, required transport.source, incomplete entries rejected, allowlist env); new tests cover each review point.
|
Re-verified end-to-end on the trust-boundary rework (e96f26b) — pristine code-server harness, extension-written
The one behavior change vs. the previous head is deliberate and called out in the body: a legacy env-less global entry no longer self-heals at boot, because that is exactly the unsafe rewrite; one explicit |
There was a problem hiding this comment.
2 issues found across 6 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/opencode/src/altimate/tools/datamate.ts">
<violation number="1" location="packages/opencode/src/altimate/tools/datamate.ts:236">
P2: When `datamate_manager add` finds a connected global entry without provenance, this new stamp is never persisted because the connected branch returns first. Boot healing then rejects the entry, so legacy/global entries cannot be repaired; persist the stamp before returning without replacing the live client.</violation>
</file>
<file name="packages/opencode/src/altimate/datamate-transport.ts">
<violation number="1" location="packages/opencode/src/altimate/datamate-transport.ts:116">
P2: When the launch directory reaches `$HOME` through a symlink or different Windows casing, this string comparison accepts `$HOME` as the project root. The boot scan can then inspect unrelated home projects and heal their configs; canonicalize the paths before applying the home-root rejection.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| // file. The boot-time heal rewrites a GLOBAL entry only when this stamp | ||
| // matches, so an explicit `add` is what authorizes future auto-repair of | ||
| // a global-scope entry. | ||
| const provenanceFields = { managedBy: DATAMATE_PROVENANCE, sourceMcpJson: transport.source } |
There was a problem hiding this comment.
P2: When datamate_manager add finds a connected global entry without provenance, this new stamp is never persisted because the connected branch returns first. Boot healing then rejects the entry, so legacy/global entries cannot be repaired; persist the stamp before returning without replacing the live client.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/altimate/tools/datamate.ts, line 236:
<comment>When `datamate_manager add` finds a connected global entry without provenance, this new stamp is never persisted because the connected branch returns first. Boot healing then rejects the entry, so legacy/global entries cannot be repaired; persist the stamp before returning without replacing the live client.</comment>
<file context>
@@ -229,6 +229,11 @@ async function handleAdd(args: { datamate_id?: string; name?: string; scope?: "p
+ // file. The boot-time heal rewrites a GLOBAL entry only when this stamp
+ // matches, so an explicit `add` is what authorizes future auto-repair of
+ // a global-scope entry.
+ const provenanceFields = { managedBy: DATAMATE_PROVENANCE, sourceMcpJson: transport.source }
const existingNames = await listMcpInConfig(configPath)
const staleEntries = existingNames.filter(
</file context>
| await matches.return() | ||
| if (dotgit) { | ||
| const root = path.dirname(dotgit) | ||
| if (root !== home) return root |
There was a problem hiding this comment.
P2: When the launch directory reaches $HOME through a symlink or different Windows casing, this string comparison accepts $HOME as the project root. The boot scan can then inspect unrelated home projects and heal their configs; canonicalize the paths before applying the home-root rejection.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/altimate/datamate-transport.ts, line 116:
<comment>When the launch directory reaches `$HOME` through a symlink or different Windows casing, this string comparison accepts `$HOME` as the project root. The boot scan can then inspect unrelated home projects and heal their configs; canonicalize the paths before applying the home-root rejection.</comment>
<file context>
@@ -64,10 +101,20 @@ function isBlankDatamateEntry(entry: unknown): boolean {
- if (dotgit) return path.dirname(dotgit)
+ if (dotgit) {
+ const root = path.dirname(dotgit)
+ if (root !== home) return root
+ }
} catch {
</file context>
| * vscode|cursor). Anything else in a checkout is not an extension-authored | ||
| * entry and must not become a transport source. | ||
| */ | ||
| const IDE_MCP_JSON_PATTERNS = ["**/.vscode/mcp.json", "**/.cursor/mcp.json"] |
There was a problem hiding this comment.
WARNING: Narrowing the transport scan to .vscode/.cursor drops .github/copilot/mcp.json, which this tool previously discovered and still documents as supported
IDE_MCP_JSON_PATTERNS replaces the prior Glob.scan('**/mcp.json'), which also reached .github/copilot/mcp.json. The repo's own mcp/discover.ts precedence list (IDE_PRECEDENCE = ['.vscode/mcp.json', '.cursor/mcp.json', '.github/copilot/mcp.json']) and the datamate manager's doc comment (altimate/tools/datamate.ts:33 - 'Scans .vscode/mcp.json, .cursor/mcp.json, .github/copilot/mcp.json ... so this works in Cursor, Copilot, and other IDEs') both treat the Copilot location as first-class. After this change both readDatamateTransportFromIde and syncDatamateUrlFromVscodeMcp silently stop detecting a datamate entry written to .github/copilot/mcp.json, regressing Copilot users. If the exclusion is deliberate, the stale datamate.ts comment should be updated; otherwise add '**/.github/copilot/mcp.json' to the list.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Issue for this PR
Closes #1082
Type of change
What does this PR do?
Fixes the bug where
datamate-cli.jssuddenly opens as an editor tab when launching sessions, and the datamate MCP server dies with-32000 Connection closed.On desktop editors the extension-written
.vscode/mcp.jsondatamatestdio entry hascommand= the editor's Electron binary andenv: {"ELECTRON_RUN_AS_NODE": "1"}(Electron only runs the script as Node with that flag; without it, the editor GUI boots and opens the script as a document).datamate_manager addreused the entry's command + args but dropped theenvblock, both in the immediate spawn and in the entry persisted to.altimate-code/altimate-code.json— so the file popped onaddand again on every later session launch, with no self-repair in TUI/run(the healing sync only ran onserveboot).Changes:
readDatamateTransportFromIdenow returns the IDE entry's env (minusALTIMATE_EXTENSION_RPC, mirroring the sync path) andupdatedAt;handleAddcarries the env into the runtime MCP config and persists it asenvironment, plusupdatedAton disk so the sync recognizes the entry as current.extractSpawnEnvironmenthelper so the two paths stay in lockstep.runnow runsyncDatamateUrlFromVscodeMcpbefore the first session, asservealready did — entries already persisted broken in the field self-heal on the next launch. The heal is scoped to the containing git project root (resolveDatamateSyncRoot, bounded at the home directory), walks the config files from the launch directory up to that root the way the loader does (a nested package's own config is healed too), and in the worker it is sequenced strictly before config load, the first in-process request, andServer.listen, so the first session connects with the healed entry rather than a stale cached one.datamate_manager addon an existing-but-disconnected entry likewise refreshes it from the current IDE transport before connecting.ELECTRON_RUN_AS_NODEonly, since the carried env is spread over the host process env at spawn. Transport sources are only the two locations the extension writes (**/.vscode/mcp.json,**/.cursor/mcp.json), parsed through a validatingparseIdeTransport(local needs a non-emptycommand, remote a non-emptyurl; blank tombstones and incomplete entries are skipped rather than winning selection — an incomplete entry used to be persisted as a url-lessremote). Entries derived from an IDE file carry provenance (managedBy: "altimate-ide"+sourceMcpJson), and the boot heal never rewrites a global entry from a project file unless that entry's stamp matches the exact IDE file — hand-added and legacy global entries are left alone; an explicitdatamate_manager addis what (re)stamps them and is the remedy for a legacy global entry.syncDatamateUrlFromVscodeMcphas a second pass that refreshes the URL (andupdatedAt) of other remote MCP entries mirrored from the IDE config (name match, URL differs). That pass is not new behavior —serveboot has always run it — TUI/runnow just apply the same refresh consistently. Spawn/env behavior for non-datamate servers is unchanged.How did you verify your code works?
E2E in the docker code-server harness against a desktop-shaped mcp.json entry (command = an Electron-contract shim that opens its args as documents unless
ELECTRON_RUN_AS_NODE=1), driven through realrunsessions:datamate_manager add(project)-32000 Connection closed, env-less entry persistedconnected as 'datamate', entry carriesenvironment+ provenanceaddbelow)datamate_manager add --scope globalon that legacy entry, then relaunch.cursor/mcp.jsontombstone sorting firstUnit tests:
test/release-validation/mcp-datamate-stdio-env.test.tscovers the env carry (strip rule, omission when empty, back-compat bare shape, non-string filtering) and sync parity. Existingmcp-datamate-893suite unchanged and green;tsgo --noEmitclean.Screenshots / recordings
Before —
datamate_manager addpops the file open:After — same broken persisted entry, next session heals it and nothing pops:
Checklist