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 10 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.
| // altimate_change start — datamate entry heal, awaited before the first in-process | ||
| // request (session start connects MCP servers from the config this sync repairs). | ||
| // Errors are swallowed: a failed sync must never block the TUI. | ||
| const datamateSyncReady: Promise<unknown> = syncDatamateUrlFromVscodeMcp(process.cwd()).catch(() => {}) | ||
| // altimate_change end |
There was a problem hiding this comment.
Fixed in 80d4ad4 — trace init now awaits the heal (traceReady starts with await datamateSyncReady), so InstanceRuntime.load/Config.get() can no longer read concurrently with the non-atomic write.
Code Review SummaryStatus: 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 Summaries (9 snapshots, latest commit 7f68428)Current summary above is authoritative. Previous snapshots are kept for context only. 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 glm-5.2 · Input: 63.8K · Output: 15.7K · Cached: 564.2K 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.
3 issues found across 5 files
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/datamate-transport.ts">
<violation number="1" location="packages/opencode/src/altimate/datamate-transport.ts:137">
P2: Datamate entries using supported MCP env references are launched with the literal `${VAR}` value instead of the resolved environment value. The IDE-specific read and sync paths should apply the shared `ConfigPaths.resolveEnvVarsInString` handling before returning or persisting the environment, while preserving the existing single-pass escape semantics.</violation>
</file>
<file name="packages/opencode/src/cli/cmd/run.ts">
<violation number="1" location="packages/opencode/src/cli/cmd/run.ts:951">
P2: This adds a full-project `**/mcp.json` glob scan to the synchronous startup path of every local `run` invocation, since the sync is awaited before `bootstrap`. For a one-shot CLI this is per-invocation latency even when the user has no datamate entry — the scan runs before the function discovers there is nothing to heal. Consider guarding this so it only runs when a datamate IDE entry is actually present (or launching it concurrently with bootstrap rather than awaiting a blocking scan), so ordinary `run` invocations don't regress in startup time.</violation>
</file>
<file name="packages/opencode/src/cli/tui/worker.ts">
<violation number="1" location="packages/opencode/src/cli/tui/worker.ts:83">
P2: The datamate heal sync is now placed on the TUI worker's startup critical path: the first `rpc.fetch` (and `Server.listen` in external-server mode) awaits `datamateSyncReady`, which runs a recursive `**/mcp.json` glob across the whole project before it can short-circuit. Every TUI session pays this scan latency on the first request, even for users with no datamate/IDE entry. Consider not blocking the first request on the scan — run the sync in parallel with bootstrap and let the session's existing self-repair path pick it up, or short-circuit the sync (skip the glob) when no datamate entry/dir is present, so startup latency isn't tied to filesystem traversal.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| const args = Array.isArray(entry["args"]) ? (entry["args"] as string[]) : [] | ||
| if (cmd) { | ||
| return { type: "local", command: [cmd, ...args] } | ||
| const environment = extractSpawnEnvironment(entry["env"]) |
There was a problem hiding this comment.
P2: Datamate entries using supported MCP env references are launched with the literal ${VAR} value instead of the resolved environment value. The IDE-specific read and sync paths should apply the shared ConfigPaths.resolveEnvVarsInString handling before returning or persisting the environment, while preserving the existing single-pass escape semantics.
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 137:
<comment>Datamate entries using supported MCP env references are launched with the literal `${VAR}` value instead of the resolved environment value. The IDE-specific read and sync paths should apply the shared `ConfigPaths.resolveEnvVarsInString` handling before returning or persisting the environment, while preserving the existing single-pass escape semantics.</comment>
<file context>
@@ -108,11 +127,21 @@ export async function readDatamateTransportFromIde(
const args = Array.isArray(entry["args"]) ? (entry["args"] as string[]) : []
if (cmd) {
- return { type: "local", command: [cmd, ...args] }
+ const environment = extractSpawnEnvironment(entry["env"])
+ const updatedAt = typeof entry["updatedAt"] === "string" ? entry["updatedAt"] : undefined
+ return {
</file context>
There was a problem hiding this comment.
Acknowledged but deliberately not changed here: this is a pre-existing parity gap shared with syncDatamateUrlFromVscodeMcp, which has persisted the env block verbatim since the transport layer landed — this PR's read path just mirrors it (the shared extractSpawnEnvironment keeps them in lockstep). In practice the extension writes only literal values (ELECTRON_RUN_AS_NODE, the RPC socket path), never ${VAR} references, and persisted entries still go through config-load substitution. Unifying with resolveServerEnvVars would change the sync path's semantics too, so it belongs in its own change — parked as a follow-up.
| // re-spawned broken on every run invocation with no path to self-repair. | ||
| { | ||
| const { syncDatamateUrlFromVscodeMcp } = await import("../../altimate/datamate-transport") | ||
| await syncDatamateUrlFromVscodeMcp(process.cwd()).catch(() => {}) |
There was a problem hiding this comment.
P2: This adds a full-project **/mcp.json glob scan to the synchronous startup path of every local run invocation, since the sync is awaited before bootstrap. For a one-shot CLI this is per-invocation latency even when the user has no datamate entry — the scan runs before the function discovers there is nothing to heal. Consider guarding this so it only runs when a datamate IDE entry is actually present (or launching it concurrently with bootstrap rather than awaiting a blocking scan), so ordinary run invocations don't regress in startup time.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/cli/cmd/run.ts, line 951:
<comment>This adds a full-project `**/mcp.json` glob scan to the synchronous startup path of every local `run` invocation, since the sync is awaited before `bootstrap`. For a one-shot CLI this is per-invocation latency even when the user has no datamate entry — the scan runs before the function discovers there is nothing to heal. Consider guarding this so it only runs when a datamate IDE entry is actually present (or launching it concurrently with bootstrap rather than awaiting a blocking scan), so ordinary `run` invocations don't regress in startup time.</comment>
<file context>
@@ -942,6 +942,15 @@ You are speaking to a non-technical business executive. Follow these rules stric
+ // re-spawned broken on every run invocation with no path to self-repair.
+ {
+ const { syncDatamateUrlFromVscodeMcp } = await import("../../altimate/datamate-transport")
+ await syncDatamateUrlFromVscodeMcp(process.cwd()).catch(() => {})
+ }
+ // altimate_change end
</file context>
There was a problem hiding this comment.
Deliberate trade-off: the scan must complete before the session's MCP connect or the heal doesn't apply to the launch the user is looking at (see the sibling P1 about exactly that ordering — deferring the sync and strict ordering are mutually exclusive). The cost is identical to what altimate serve boot has paid since the sync was introduced, and the scan is the sync's inherent cost, not something this call site adds. If it shows up in real startup profiles, narrowing the scan itself (known IDE dirs first) would help every caller and is a better follow-up than special-casing this one.
| async fetch(input: { url: string; method: string; headers: Record<string, string>; body?: string }) { | ||
| // altimate_change start — no request is served until the datamate entry heal | ||
| // completes (already-resolved after the first request; effectively free thereafter). | ||
| await datamateSyncReady |
There was a problem hiding this comment.
P2: The datamate heal sync is now placed on the TUI worker's startup critical path: the first rpc.fetch (and Server.listen in external-server mode) awaits datamateSyncReady, which runs a recursive **/mcp.json glob across the whole project before it can short-circuit. Every TUI session pays this scan latency on the first request, even for users with no datamate/IDE entry. Consider not blocking the first request on the scan — run the sync in parallel with bootstrap and let the session's existing self-repair path pick it up, or short-circuit the sync (skip the glob) when no datamate entry/dir is present, so startup latency isn't tied to filesystem traversal.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/cli/tui/worker.ts, line 83:
<comment>The datamate heal sync is now placed on the TUI worker's startup critical path: the first `rpc.fetch` (and `Server.listen` in external-server mode) awaits `datamateSyncReady`, which runs a recursive `**/mcp.json` glob across the whole project before it can short-circuit. Every TUI session pays this scan latency on the first request, even for users with no datamate/IDE entry. Consider not blocking the first request on the scan — run the sync in parallel with bootstrap and let the session's existing self-repair path pick it up, or short-circuit the sync (skip the glob) when no datamate entry/dir is present, so startup latency isn't tied to filesystem traversal.</comment>
<file context>
@@ -65,6 +78,10 @@ let server: Awaited<ReturnType<typeof Server.listen>> | undefined
async fetch(input: { url: string; method: string; headers: Record<string, string>; body?: string }) {
+ // altimate_change start — no request is served until the datamate entry heal
+ // completes (already-resolved after the first request; effectively free thereafter).
+ await datamateSyncReady
+ // altimate_change end
const headers = { ...input.headers }
</file context>
There was a problem hiding this comment.
Same trade-off as the run latency comment: correctness requires heal-before-first-connect (your own P1 on this file asks for strictly tighter ordering, which 80d4ad4 implements), so the scan can't move off the critical path without reintroducing that bug. Cost matches serve's existing boot behavior; narrowing the scan itself is the right follow-up if profiling warrants it.
…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}
| }) | ||
| await MCP.connect(DATAMATE_KEY) | ||
| const existing = await readMcpEntryFromDisk(DATAMATE_KEY, configPath) | ||
| const TRANSPORT_FIELDS = new Set(["type", "command", "args", "environment", "url", "updatedAt", "enabled"]) |
There was a problem hiding this comment.
SUGGESTION: TRANSPORT_FIELDS duplicates the near-identical set already defined in syncDatamateUrlFromVscodeMcp (packages/opencode/src/altimate/datamate-transport.ts:258), differing only by enabled.
Both encode the same idea — fields re-derived from the transport rather than user-managed. If one list later grows a new transport field (e.g. headers) and the other doesn't, the two paths will silently disagree on what gets preserved. Consider exporting a shared base set (e.g. TRANSPORT_IDENTITY_FIELDS) from datamate-transport.ts and layering the per-site extra (enabled here) on top.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Applied in 1cb8fad — TRANSPORT_IDENTITY_FIELDS is now a shared exported set in datamate-transport.ts, used by the sync directly and by the add-refresh path with enabled layered on top (that path re-derives it as true).
…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 |
| ...preserved, | ||
| ...mcpConfig, | ||
| enabled: true, | ||
| ...(transport?.updatedAt ? { updatedAt: transport.updatedAt } : {}), |
There was a problem hiding this comment.
SUGGESTION: transport?.updatedAt ? { updatedAt: transport.updatedAt } : {} is now duplicated at line 303
The identical conditional spread appears in both the refresh branch (here, in refreshed) and the new-entry branch (diskEntry), both derived from the same transport. Hoist a single const updatedAtPart = transport?.updatedAt ? { updatedAt: transport.updatedAt } : {} above the if (existingNames.includes(...)) split and spread ...updatedAtPart in both objects. This mirrors the TRANSPORT_IDENTITY_FIELDS consolidation from the prior commit and keeps the two paths in lockstep if the updatedAt shape ever changes.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Applied in 37c3d2c — single updatedAtField hoisted above the branch, documented once.
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: |
| } | ||
|
|
||
| let datamateHealed = false | ||
| for (const configPath of await findAllConfigPaths(cwd, globalConfigDir)) { |
There was a problem hiding this comment.
SUGGESTION: A throw on one config file aborts healing of the rest
healEntryInFile can throw mid-loop: addMcpToConfig rejects a malformed-JSON config (it throws in config.ts:46-52), and readText would throw if the file is removed between findAllConfigPaths' existence check and the read. Because the whole function shares a single outer try/catch, a failure on the project config (iterated first) also skips healing the global entry and skips the remote-entry URL refresh below. The sibling persistMcpEnabledUnlocked (mcp/index.ts:974) wraps its own findAllConfigPaths loop in a try/catch for this reason — isolating each iteration would let one bad file fail without defeating the rest of the heal.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
2 issues found across 3 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/datamate-transport.ts">
<violation number="1" location="packages/opencode/src/altimate/datamate-transport.ts:331">
P2: Global Datamate entries in supported `altimate-code.jsonc` or legacy `config.json` files are skipped by this new healing pass, so those sessions remain stale despite the global-config fix. Including every filename used by global config loading in `findAllConfigPaths` would make the repair cover all active global entries.</violation>
<violation number="2" location="packages/opencode/src/altimate/datamate-transport.ts:334">
P2: Reloading a global-only Datamate entry reports success but leaves the running MCP client on the stale transport, because the new global repair result is reduced to a name while the reload path only rereads the project config. Returning the repaired config path(s), or updating the reload handler to read the global file too, would reconnect the repaired global entry.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| } | ||
|
|
||
| let datamateHealed = false | ||
| for (const configPath of await findAllConfigPaths(cwd, globalConfigDir)) { |
There was a problem hiding this comment.
P2: Global Datamate entries in supported altimate-code.jsonc or legacy config.json files are skipped by this new healing pass, so those sessions remain stale despite the global-config fix. Including every filename used by global config loading in findAllConfigPaths would make the repair cover all active global entries.
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 331:
<comment>Global Datamate entries in supported `altimate-code.jsonc` or legacy `config.json` files are skipped by this new healing pass, so those sessions remain stale despite the global-config fix. Including every filename used by global config loading in `findAllConfigPaths` would make the repair cover all active global entries.</comment>
<file context>
@@ -246,76 +251,87 @@ export async function syncDatamateUrlFromVscodeMcp(cwd: string): Promise<string[
+ }
+
+ let datamateHealed = false
+ for (const configPath of await findAllConfigPaths(cwd, globalConfigDir)) {
+ if (await healEntryInFile(configPath)) datamateHealed = true
}
</file context>
| for (const configPath of await findAllConfigPaths(cwd, globalConfigDir)) { | ||
| if (await healEntryInFile(configPath)) datamateHealed = true | ||
| } | ||
| if (datamateHealed) updated.push(DATAMATE_KEY) |
There was a problem hiding this comment.
P2: Reloading a global-only Datamate entry reports success but leaves the running MCP client on the stale transport, because the new global repair result is reduced to a name while the reload path only rereads the project config. Returning the repaired config path(s), or updating the reload handler to read the global file too, would reconnect the repaired global entry.
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 334:
<comment>Reloading a global-only Datamate entry reports success but leaves the running MCP client on the stale transport, because the new global repair result is reduced to a name while the reload path only rereads the project config. Returning the repaired config path(s), or updating the reload handler to read the global file too, would reconnect the repaired global entry.</comment>
<file context>
@@ -246,76 +251,87 @@ export async function syncDatamateUrlFromVscodeMcp(cwd: string): Promise<string[
+ for (const configPath of await findAllConfigPaths(cwd, globalConfigDir)) {
+ if (await healEntryInFile(configPath)) datamateHealed = true
}
+ if (datamateHealed) updated.push(DATAMATE_KEY)
}
</file context>
…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.
| // entry (non-active-IDE file) must not be selected as the sync source — | ||
| // it has no updatedAt, so the heal would silently skip while the real | ||
| // entry sits in the next file. | ||
| if (map[DATAMATE_KEY] && Object.keys(map[DATAMATE_KEY]).length > 0) { |
There was a problem hiding this comment.
SUGGESTION: The blank-tombstone predicate is duplicated at both scan sites
Object.keys(...).length === 0 (line 159) and Object.keys(...).length > 0 (here) encode the same rule — a blanked {} entry is a tombstone and must be skipped. This comment already calls them "the same tombstone rule," but the two expressions can still drift (one negated, one not). A small shared predicate such as isBlankedEntry = (e) => !e || Object.keys(e).length === 0 would name the rule once; both sites would call it — isBlankedEntry(entry) to skip (line 159) and !isBlankedEntry(map[KEY]) to select (here). This mirrors the TRANSPORT_IDENTITY_FIELDS consolidation done earlier in this PR and keeps the read/sync paths in lockstep if the tombstone shape ever changes.
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), covers every config file carrying a datamate entry (project, project subdirs, and the global~/.config/altimate-code/altimate-code.json—addsupportsscope: "global", and a stale global entry pops the file just the same), 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.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-32000 Connection closed, env-less entry persistedconnected as 'datamate', entry persistsenvironment+updatedAtUnit 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