From e20b41e569178e93d2af407fdfbb1e9a1027db18 Mon Sep 17 00:00:00 2001 From: Haider Date: Mon, 31 Aug 2026 16:21:48 +0530 Subject: [PATCH 1/2] fix(mcp): scope diagnostics to the project they came from MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #1211 The four MCP diagnostic records were module-level singletons keyed by server name alone. One process serves several projects — the server resolves an instance per request from `x-opencode-directory`, and `project/instance.ts` caches those instances per directory — so a second project's discovery erased the first's answers, and two projects reusing a server name overwrote each other. `datamate` is exactly such a name: the extension sync writes it into every project. Measured against the previous commit: after A: unresolvedEnvVars('alpha') = ["VAR_A"] after B: unresolvedEnvVars('alpha') = [] ← erased shared name: unresolvedEnvVars('datamate') = ["VAR_B"] ← A's answer gone `_unresolvedEnv`, `_drift` and `_discoveredSource` are now keyed by project directory, and a discovery run clears only its own project — which keeps the staleness fix from #1121 while making the clear harmless to every other instance. The accessors take the project explicitly, so a caller cannot forget: `unresolvedEnvVars(server, projectDir)`, `configDrift(projectDir)`, `discoveredSource(server, projectDir)`. `_blankedEnv` is scoped differently, on purpose. It is keyed by config source rather than server, and threading a project through `substitute` would mean widening `loadConfig`/`loadFile` signatures in an upstream-shared file — which Marker Guard rejects, and which would carry this change into code that has nothing to do with it. Filtering at read time gives the same result: a config file living under a *different* project belongs to that project's session. Sources every instance shares — the global config dir, `OPENCODE_CONFIG_CONTENT`, a remote config URL — stay visible to all of them. The reproduction from the issue is committed as `test/mcp/diagnostics-instance-scope.test.ts` and covers sequential discovery, a shared server name, concurrent discovery, and per-project drift attribution. Mutation-tested in both halves: restoring the global clear fails the two cross-project cases, and removing the path filter fails the foreign-config case. Full opencode suite: 11744 pass, 0 fail. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_018fJ3X7pcGT4R9yzjsJnqsV --- packages/opencode/src/cli/cmd/mcp.ts | 13 +-- packages/opencode/src/config/config.ts | 3 +- packages/opencode/src/config/variable.ts | 35 +++++- packages/opencode/src/mcp/discover.ts | 91 +++++++++------- packages/opencode/src/session/prompt.ts | 10 +- .../opencode/test/config/blanked-env.test.ts | 38 ++++++- .../opencode/test/mcp/config-drift.test.ts | 35 ++++-- .../mcp/diagnostics-instance-scope.test.ts | 100 ++++++++++++++++++ packages/opencode/test/mcp/discover.test.ts | 6 +- 9 files changed, 271 insertions(+), 60 deletions(-) create mode 100644 packages/opencode/test/mcp/diagnostics-instance-scope.test.ts diff --git a/packages/opencode/src/cli/cmd/mcp.ts b/packages/opencode/src/cli/cmd/mcp.ts index 2fd3c56ea..1b4d372b1 100644 --- a/packages/opencode/src/cli/cmd/mcp.ts +++ b/packages/opencode/src/cli/cmd/mcp.ts @@ -18,6 +18,7 @@ import { McpOAuthProvider } from "../../mcp/oauth-provider" import { Config } from "@/config/config" import { ConfigMCPV1 } from "@opencode-ai/core/v1/config/mcp" import { InstanceRef } from "@/effect/instance-ref" +import { Instance } from "@/project/instance" import { InstallationVersion } from "@opencode-ai/core/installation/version" import path from "path" import { Global } from "@opencode-ai/core/global" @@ -126,11 +127,11 @@ export const McpCommand = cmd({ // altimate_change start — upstream_fix (#878/#701): config-level diagnostics, shared by every // exit of `mcp list` / `mcp status` so a config with nothing listable still reports them. -function reportConfigDiagnostics() { +function reportConfigDiagnostics(projectDir: string) { // Discovery is first-source-wins, so a server already in altimate-code.json is skipped and a // changed .vscode/mcp.json is never mentioned. The configured value still wins; this only // says the two disagree and which file to look at. - for (const { server, source, fields } of McpDiscover.configDrift()) { + for (const { server, source, fields } of McpDiscover.configDrift(projectDir)) { prompts.log.warn(`${server} differs from ${source}: ${fields.join(", ")} (config wins)`) } @@ -138,7 +139,7 @@ function reportConfigDiagnostics() { // the server and fails much later with an error naming neither. Attribution to a single server // is not available here (substitution runs on raw config text, before any structure exists), // so this is reported against the file. - for (const { source, names } of ConfigVariable.blankedEnvVars()) { + for (const { source, names } of ConfigVariable.blankedEnvVars(projectDir)) { prompts.log.warn(`${names.join(", ")} resolved to empty in ${source} (set or remove)`) } } @@ -160,7 +161,7 @@ export const McpListCommand = effectCmd({ // altimate_change start — upstream_fix (#878): drift and blank-variable warnings are about // the config, not about any one server, so they must survive the nothing-to-list exit. An // enabled-only override for a discovered server leaves this list empty while drift exists. - reportConfigDiagnostics() + reportConfigDiagnostics(Instance.directory) // altimate_change end // altimate_change start — branding regression prompts.outro("Add servers with: altimate mcp add") @@ -205,7 +206,7 @@ export const McpListCommand = effectCmd({ // altimate_change start — upstream_fix (#701): name variables that resolved to "". // A blank `${SNOWFLAKE_PASSWORD}` often connects and only fails on first real use, so // this is appended regardless of status rather than only on the failure branch. - const unresolved = McpDiscover.unresolvedEnvVars(name) + const unresolved = McpDiscover.unresolvedEnvVars(name, Instance.directory) if (unresolved.length > 0) { hint += "\n unresolved env: " + unresolved.join(", ") + " (set or remove)" } @@ -217,7 +218,7 @@ export const McpListCommand = effectCmd({ } // altimate_change start — upstream_fix (#878/#701): config-level diagnostics. - reportConfigDiagnostics() + reportConfigDiagnostics(Instance.directory) // altimate_change end prompts.outro(`${servers.length} server(s)`) diff --git a/packages/opencode/src/config/config.ts b/packages/opencode/src/config/config.ts index 4ee63401a..efe58c6b3 100644 --- a/packages/opencode/src/config/config.ts +++ b/packages/opencode/src/config/config.ts @@ -774,8 +774,9 @@ export const layer = Layer.effect( const configured = (result.mcp as Record)[name] setConfigDrift( name, - discoveredSource(name) ?? sources.join(", "), + discoveredSource(name, ctx.directory) ?? sources.join(", "), driftFields(server as Record, configured), + ctx.directory, ) } } diff --git a/packages/opencode/src/config/variable.ts b/packages/opencode/src/config/variable.ts index 415e09efc..728789e61 100644 --- a/packages/opencode/src/config/variable.ts +++ b/packages/opencode/src/config/variable.ts @@ -6,6 +6,7 @@ import { Filesystem } from "@/util/filesystem" import { InvalidError } from "@opencode-ai/core/v1/config/error" // altimate_change start — upstream_fix: restore ${VAR}/${VAR:-default}/$${VAR} config interpolation import { ConfigPaths } from "@/config/paths" +import { Global } from "@/global" // altimate_change end type ParseSource = @@ -32,8 +33,11 @@ type SubstituteInput = ParseSource & { // An unresolved bare `${VAR}` is left LITERAL above on purpose, so it stays visible and is not // recorded here. `{env:VAR}` has no such deferral — it becomes "" and the config parses clean, so // a missing `{env:SNOWFLAKE_PASSWORD}` launches an MCP server with a blank credential and fails -// later with an error naming neither the variable nor this file. Keyed by config source; the -// newest parse of a file replaces its entry so a fixed variable stops being reported. +// later with an error naming neither the variable nor this file. +// +// Keyed projectDir -> config source. One process serves several projects (the server resolves an +// instance per request from `x-opencode-directory`), and a flat source-keyed map meant +// `blankedEnvVars()` handed every session every other project's config files. const _blankedEnv = new Map>() /** Drop `src`'s record so a load starts clean; substitution then unions within that load. */ @@ -41,12 +45,35 @@ export function resetBlankedEnvVars(src: string) { _blankedEnv.delete(src) } -/** Variable names that silently became "" during config substitution, grouped by config source. */ -export function blankedEnvVars(): { source: string; names: string[] }[] { +/** + * Variable names that silently became "" during config substitution, grouped by config source. + * + * Scoped by path rather than by threading a project through `substitute`: a config file that + * lives under a *different* project belongs to that project's session, not this one. One process + * serves several projects (the server resolves an instance per request from + * `x-opencode-directory`), and an unfiltered record handed every session every other project's + * files. Sources that are not project-local — the global config dir, `OPENCODE_CONFIG_CONTENT`, + * a remote config URL — are shared by every instance and are always included. + */ +export function blankedEnvVars(projectDir: string): { source: string; names: string[] }[] { return [..._blankedEnv.entries()] + .filter(([src]) => !isForeignProjectPath(src, projectDir)) .map(([src, names]) => ({ source: src, names: [...names].sort() })) .sort((a, b) => a.source.localeCompare(b.source)) } + +/** True when `src` is an absolute path that sits outside `projectDir` and outside the config dir. */ +function isForeignProjectPath(src: string, projectDir: string): boolean { + if (!path.isAbsolute(src)) return false // OPENCODE_CONFIG_CONTENT, a URL — shared + const rel = path.relative(projectDir, src) + if (rel && !rel.startsWith("..") && !path.isAbsolute(rel)) return false // under this project + // The user-level config dir and the home directory are shared by every instance. + const shared = [Global.Path.config, os.homedir()].filter(Boolean) as string[] + return !shared.some((base) => { + const r = path.relative(base, src) + return r !== "" && !r.startsWith("..") && !path.isAbsolute(r) + }) +} // altimate_change end function source(input: ParseSource) { diff --git a/packages/opencode/src/mcp/discover.ts b/packages/opencode/src/mcp/discover.ts index e0d32dd9f..d873c6266 100644 --- a/packages/opencode/src/mcp/discover.ts +++ b/packages/opencode/src/mcp/discover.ts @@ -19,7 +19,7 @@ const log = Log.create({ service: "mcp.discover" }) // here, rather than twice. function resolveServerEnvVars( obj: Record, - context: { server: string; source: string; field: "env" | "headers" }, + context: { server: string; source: string; field: "env" | "headers"; projectDir: string }, ): Record { const out: Record = {} const stats = ConfigPaths.newEnvSubstitutionStats() @@ -39,9 +39,10 @@ function resolveServerEnvVars( // credential, failing later with something that names neither the variable nor the config // file. The log line already had the answer; nobody reads it. Recorded here so `/mcps` can // say so. Mirrors the `setDiscoveryResult` handoff below. - const seen = _unresolvedEnv.get(context.server) ?? new Set() + const b = bucket(_unresolvedEnv, context.projectDir) + const seen = b.get(context.server) ?? new Set() for (const name of stats.unresolvedNames) seen.add(name) - _unresolvedEnv.set(context.server, seen) + b.set(context.server, seen) // altimate_change end } return out @@ -49,8 +50,18 @@ function resolveServerEnvVars( // altimate_change end // altimate_change start — upstream_fix: unresolved-variable record for the user surface (#701). -/** Server name -> variable names that resolved to "" during discovery. */ -const _unresolvedEnv = new Map>() +/** projectDir -> server name -> variable names that resolved to "" during discovery. */ +const _unresolvedEnv = new Map>>() + +/** Per-project bucket, created on demand. Keyed by the directory discovery ran for. */ +function bucket(store: Map>, projectDir: string): Map { + let b = store.get(projectDir) + if (!b) { + b = new Map() + store.set(projectDir, b) + } + return b +} /** * Variable names that silently became "" for `server`, from the most recent discovery. @@ -62,16 +73,18 @@ const _unresolvedEnv = new Map>() * `unresolvedNames.length > 0` guard, so a clean run never touched it), and `/mcps` went on * telling the user to set a variable that already resolved. * - * Only the latest run's servers are present, so a daemon that discovers for a second project - * replaces the first project's entries rather than mixing the two under a shared server name. + * Scoped to `projectDir`, because one process serves more than one project: a global record + * meant a second project's discovery erased the first's answers, and two projects reusing a + * server name (`datamate`, which the extension sync writes everywhere) overwrote each other. + * Clearing now touches only the project being rediscovered. */ -export function unresolvedEnvVars(server: string): string[] { - return [...(_unresolvedEnv.get(server) ?? [])].sort() +export function unresolvedEnvVars(server: string, projectDir: string): string[] { + return [...(_unresolvedEnv.get(projectDir)?.get(server) ?? [])].sort() } -/** Drop the previous run's records. Called once per `discoverExternalMcp`. */ -function resetUnresolvedEnv() { - _unresolvedEnv.clear() +/** Drop this project's records. Called once per `discoverExternalMcp`. */ +function resetUnresolvedEnv(projectDir: string) { + _unresolvedEnv.get(projectDir)?.clear() } // altimate_change end @@ -80,7 +93,7 @@ function resetUnresolvedEnv() { // outright and a changed `.vscode/mcp.json` (a new ALTIMATE_EXTENSION_RPC port, a moved command) // is never mentioned. Overwriting the user's own config would be worse than the silence, so the // differing field names are recorded and a user surface reports them; the user decides. -const _drift = new Map() +const _drift = new Map>() /** * Fields whose difference is expected and not worth reporting. @@ -139,30 +152,32 @@ export function driftFields(discovered: Record, configured: Record< return fields.sort() } -/** Record that `server` is configured differently from what discovery found in `source`. */ -export function setConfigDrift(server: string, source: string, fields: string[]) { - if (fields.length > 0) _drift.set(server, { source, fields }) - else _drift.delete(server) +/** Record that `server` in `projectDir` is configured differently from what discovery found. */ +export function setConfigDrift(server: string, source: string, fields: string[], projectDir: string) { + const b = bucket(_drift, projectDir) + if (fields.length > 0) b.set(server, { source, fields }) + else b.delete(server) } -/** Servers whose configured definition differs from the discovered one. */ -export function configDrift(): { server: string; source: string; fields: string[] }[] { - return [..._drift.entries()] +/** Servers in `projectDir` whose configured definition differs from the discovered one. */ +export function configDrift(projectDir: string): { server: string; source: string; fields: string[] }[] { + return [...(_drift.get(projectDir)?.entries() ?? [])] .map(([server, info]) => ({ server, ...info })) .sort((a, b) => a.server.localeCompare(b.server)) } -/** Server name -> the file that actually defined it, for drift attribution. */ -const _discoveredSource = new Map() +/** projectDir -> server name -> the file that actually defined it, for drift attribution. */ +const _discoveredSource = new Map>() /** The config file a discovered server came from, or undefined if it was not discovered. */ -export function discoveredSource(server: string): string | undefined { - return _discoveredSource.get(server) +export function discoveredSource(server: string, projectDir: string): string | undefined { + return _discoveredSource.get(projectDir)?.get(server) } -/** Test seam — drift accumulates at module level. */ -export function resetConfigDrift() { - _drift.clear() +/** Test seam — clears one project's drift, or every project's when no directory is given. */ +export function resetConfigDrift(projectDir?: string) { + if (projectDir === undefined) _drift.clear() + else _drift.get(projectDir)?.clear() } // altimate_change end interface ExternalMcpSource { @@ -198,7 +213,7 @@ const SOURCES: ExternalMcpSource[] = [ function transform( entry: Record, // altimate_change start — context for env-var resolution warnings - context: { server: string; source: string }, + context: { server: string; source: string; projectDir: string }, // altimate_change end ): ConfigMCPV1.Info | undefined { // Remote server — handle both "url" and Claude Code's "type: http" format @@ -308,6 +323,7 @@ function addServersFromFile( sourceLabel: string, result: Record, contributingSources: string[], + projectDir: string, projectScoped = false, ) { if (!servers || typeof servers !== "object") return @@ -322,6 +338,7 @@ function addServersFromFile( const transformed = transform(entry as Record, { server: name, source: sourceLabel, + projectDir, }) if (transformed) { // Project-scoped servers are discovered but disabled by default for security. @@ -332,7 +349,7 @@ function addServersFromFile( result[name] = transformed // altimate_change start — upstream_fix (#878): attribute drift to the file that defined // this server, not to every file that contributed something to the run. - _discoveredSource.set(name, sourceLabel) + bucket(_discoveredSource, projectDir).set(name, sourceLabel) // altimate_change end added++ } @@ -368,6 +385,7 @@ async function discoverClaudeCode( worktree: string, result: Record, contributingSources: string[], + projectDir: string, ) { const claudeJsonPath = path.join(os.homedir(), ".claude.json") const parsed = await readJsonSafe(claudeJsonPath) @@ -382,13 +400,14 @@ async function discoverClaudeCode( `~/.claude.json (${path.basename(worktree)})`, result, contributingSources, + projectDir, ) } } // Global-level mcpServers (lower priority — project-specific already added) if (parsed.mcpServers && typeof parsed.mcpServers === "object") { - addServersFromFile(parsed.mcpServers, "~/.claude.json (global)", result, contributingSources) + addServersFromFile(parsed.mcpServers, "~/.claude.json (global)", result, contributingSources, projectDir) } } @@ -433,12 +452,12 @@ export async function discoverExternalMcp(projectDir: string): Promise<{ }> { log.info("Discovering MCP servers from external AI tool configs...") // Start from a clean slate so a variable fixed since the last run stops being reported. - resetUnresolvedEnv() + resetUnresolvedEnv(projectDir) // Same for drift: a server removed from the external config, or a reload that resolved the // difference, otherwise left a stale entry and `mcp status` reported a mismatch that no // longer existed. The setConfigDrift calls after this run repopulate it. - resetConfigDrift() - _discoveredSource.clear() + resetConfigDrift(projectDir) + _discoveredSource.get(projectDir)?.clear() const result: Record = Object.create(null) const contributingSources: string[] = [] const homedir = os.homedir() @@ -490,7 +509,7 @@ export async function discoverExternalMcp(projectDir: string): Promise<{ const parsed = await readJsonSafe(file) if (!parsed || typeof parsed !== "object") continue const label = toRel(file) || path.basename(file) - addServersFromFile(mergeServerKeys(parsed), label, result, contributingSources, true) + addServersFromFile(mergeServerKeys(parsed), label, result, contributingSources, projectDir, true) } // Non-"mcp.json" config files (not matched by the glob above), in project and/or home. @@ -510,12 +529,12 @@ export async function discoverExternalMcp(projectDir: string): Promise<{ const isProjectScoped = dir === projectDir const servers = parsed[source.key] - addServersFromFile(servers, label, result, contributingSources, isProjectScoped) + addServersFromFile(servers, label, result, contributingSources, projectDir, isProjectScoped) } } // Claude Code has a unique config structure — handle separately - await discoverClaudeCode(projectDir, result, contributingSources) + await discoverClaudeCode(projectDir, result, contributingSources, projectDir) const serverNames = Object.keys(result) if (serverNames.length > 0) { diff --git a/packages/opencode/src/session/prompt.ts b/packages/opencode/src/session/prompt.ts index 1e757bb4f..90c27ccd9 100644 --- a/packages/opencode/src/session/prompt.ts +++ b/packages/opencode/src/session/prompt.ts @@ -3087,7 +3087,11 @@ NOTE: At any point in time through this workflow you should feel free to ask the const rows = Object.entries(statusMap) .map( ([srv, s]) => - "| `" + srv + "` | " + formatMcpStatusForDisplay(srv, s, McpDiscover.unresolvedEnvVars(srv)) + " |", + "| `" + + srv + + "` | " + + formatMcpStatusForDisplay(srv, s, McpDiscover.unresolvedEnvVars(srv, Instance.directory)) + + " |", ) .join("\n") // altimate_change start — upstream_fix (#701): `/mcps` showed only the per-server @@ -3095,8 +3099,8 @@ NOTE: At any point in time through this workflow you should feel free to ask the // A server templated as `"url": "https://{env:MY_HOST}/mcp"` records against the config // file rather than the server, so it appeared in the CLI and not here — in the session // view, which is where someone is when a server will not connect. - const blanked = formatBlankedEnvForDisplay(ConfigVariable.blankedEnvVars()) - const drift = formatConfigDriftForDisplay(McpDiscover.configDrift()) + const blanked = formatBlankedEnvForDisplay(ConfigVariable.blankedEnvVars(Instance.directory)) + const drift = formatConfigDriftForDisplay(McpDiscover.configDrift(Instance.directory)) const table = rows ? "MCP servers:\n\n| Server | Status |\n|---|---|\n" + rows : "No MCP servers configured." const responseText = [table, drift, blanked].filter(Boolean).join("\n\n") // altimate_change end diff --git a/packages/opencode/test/config/blanked-env.test.ts b/packages/opencode/test/config/blanked-env.test.ts index d2d443134..772edc09f 100644 --- a/packages/opencode/test/config/blanked-env.test.ts +++ b/packages/opencode/test/config/blanked-env.test.ts @@ -8,8 +8,10 @@ const SOURCE = "/virtual/blanked-env-test/config.json" const VAR = "ALTIMATE_TEST_BLANKED_VAR" const OTHER = "ALTIMATE_TEST_BLANKED_VAR_TWO" +const PROJECT = "/virtual/blanked-env-test" + function namesFor(source: string): string[] { - return ConfigVariable.blankedEnvVars().find((e) => e.source === source)?.names ?? [] + return ConfigVariable.blankedEnvVars(PROJECT).find((e) => e.source === source)?.names ?? [] } async function substitute(text: string) { @@ -68,3 +70,37 @@ describe("blankedEnvVars", () => { }) }) // altimate_change end + +// altimate_change start — upstream_fix (#1211): one process serves several projects. +describe("blankedEnvVars project scoping", () => { + const OTHER = "/virtual/some-other-project" + + test("a config file under another project is not reported here", async () => { + // The server resolves an instance per request from `x-opencode-directory`, so two projects + // are live in one process. Project B's local config is B's session's business, not A's. + await ConfigVariable.substitute({ + text: `{"token":"{env:${VAR}}"}`, + type: "virtual", + dir: OTHER, + source: OTHER + "/altimate-code.json", + env: {}, + }) + const sources = ConfigVariable.blankedEnvVars(PROJECT).map((e) => e.source) + expect(sources).not.toContain(OTHER + "/altimate-code.json") + // ...and it is still visible to the project it belongs to. + expect(ConfigVariable.blankedEnvVars(OTHER).map((e) => e.source)).toContain(OTHER + "/altimate-code.json") + }) + + test("a non-path source stays shared, since every instance loads it", async () => { + await ConfigVariable.substitute({ + text: `{"token":"{env:${VAR}}"}`, + type: "virtual", + dir: "/virtual", + source: "OPENCODE_CONFIG_CONTENT", + env: {}, + }) + expect(ConfigVariable.blankedEnvVars(PROJECT).map((e) => e.source)).toContain("OPENCODE_CONFIG_CONTENT") + expect(ConfigVariable.blankedEnvVars(OTHER).map((e) => e.source)).toContain("OPENCODE_CONFIG_CONTENT") + }) +}) +// altimate_change end diff --git a/packages/opencode/test/mcp/config-drift.test.ts b/packages/opencode/test/mcp/config-drift.test.ts index a5821a32b..f5276ee71 100644 --- a/packages/opencode/test/mcp/config-drift.test.ts +++ b/packages/opencode/test/mcp/config-drift.test.ts @@ -34,20 +34,43 @@ describe("driftFields", () => { }) describe("configDrift record", () => { + // The record is per project now, so every call names the directory it belongs to. + const PROJECT = "/tmp/project-a" beforeEach(() => resetConfigDrift()) test("records only servers that actually differ", () => { - setConfigDrift("datamate", ".vscode/mcp.json", ["environment.ALTIMATE_EXTENSION_RPC"]) - setConfigDrift("clean", ".vscode/mcp.json", []) - expect(configDrift()).toEqual([ + setConfigDrift("datamate", ".vscode/mcp.json", ["environment.ALTIMATE_EXTENSION_RPC"], PROJECT) + setConfigDrift("clean", ".vscode/mcp.json", [], PROJECT) + expect(configDrift(PROJECT)).toEqual([ { server: "datamate", source: ".vscode/mcp.json", fields: ["environment.ALTIMATE_EXTENSION_RPC"] }, ]) }) test("a server that stops drifting is dropped from the report", () => { - setConfigDrift("datamate", ".vscode/mcp.json", ["url"]) - setConfigDrift("datamate", ".vscode/mcp.json", []) - expect(configDrift()).toEqual([]) + setConfigDrift("datamate", ".vscode/mcp.json", ["url"], PROJECT) + setConfigDrift("datamate", ".vscode/mcp.json", [], PROJECT) + expect(configDrift(PROJECT)).toEqual([]) + }) + + test("one project's drift is invisible to another", () => { + // `datamate` is written into every project by the extension sync, so a name-only record + // meant two open workspaces reported each other's drift. + const OTHER = "/tmp/project-b" + setConfigDrift("datamate", ".vscode/mcp.json", ["url"], PROJECT) + setConfigDrift("datamate", ".cursor/mcp.json", ["command"], OTHER) + + expect(configDrift(PROJECT)).toEqual([{ server: "datamate", source: ".vscode/mcp.json", fields: ["url"] }]) + expect(configDrift(OTHER)).toEqual([{ server: "datamate", source: ".cursor/mcp.json", fields: ["command"] }]) + }) + + test("clearing one project leaves the other intact", () => { + const OTHER = "/tmp/project-b" + setConfigDrift("datamate", ".vscode/mcp.json", ["url"], PROJECT) + setConfigDrift("datamate", ".cursor/mcp.json", ["command"], OTHER) + + resetConfigDrift(PROJECT) + expect(configDrift(PROJECT)).toEqual([]) + expect(configDrift(OTHER)).toHaveLength(1) }) }) // altimate_change end diff --git a/packages/opencode/test/mcp/diagnostics-instance-scope.test.ts b/packages/opencode/test/mcp/diagnostics-instance-scope.test.ts new file mode 100644 index 000000000..ed1e6764d --- /dev/null +++ b/packages/opencode/test/mcp/diagnostics-instance-scope.test.ts @@ -0,0 +1,100 @@ +import { describe, test, expect, beforeEach, afterEach, spyOn } from "bun:test" +import { mkdtemp, rm, mkdir, writeFile } from "fs/promises" +import os, { tmpdir } from "os" +import path from "path" +import { discoverExternalMcp, unresolvedEnvVars, configDrift, discoveredSource } from "../../src/mcp/discover" + +let homeDir: string +let homedirSpy: ReturnType | undefined + +const VAR_A = "ALTIMATE_TEST_SCOPE_VAR_A" +const VAR_B = "ALTIMATE_TEST_SCOPE_VAR_B" + +async function projectWith(server: string, varName: string): Promise { + const dir = await mkdtemp(path.join(tmpdir(), "mcp-scope-")) + await mkdir(path.join(dir, ".vscode"), { recursive: true }) + await writeFile( + path.join(dir, ".vscode/mcp.json"), + JSON.stringify({ servers: { [server]: { command: "node", env: { TOKEN: `{env:${varName}}` } } } }), + ) + return dir +} + +beforeEach(async () => { + homeDir = await mkdtemp(path.join(tmpdir(), "mcp-scope-home-")) + homedirSpy = spyOn(os, "homedir").mockImplementation(() => homeDir) + delete process.env[VAR_A] + delete process.env[VAR_B] +}) + +afterEach(async () => { + homedirSpy?.mockRestore() + await rm(homeDir, { recursive: true, force: true }) +}) + +describe("MCP diagnostics are project-scoped", () => { + test("a second project's discovery does not erase the first project's diagnostics", async () => { + const projectA = await projectWith("alpha", VAR_A) + const projectB = await projectWith("beta", VAR_B) + try { + await discoverExternalMcp(projectA) + expect(unresolvedEnvVars("alpha", projectA)).toContain(VAR_A) + + // A second session in the same process discovers for a different project. Project A's + // session is still open and still asking about its own servers. + await discoverExternalMcp(projectB) + + expect(unresolvedEnvVars("beta", projectB)).toContain(VAR_B) + // The failing half: a module-global record cleared per run means A's answer is gone. + expect(unresolvedEnvVars("alpha", projectA)).toContain(VAR_A) + } finally { + await rm(projectA, { recursive: true, force: true }) + await rm(projectB, { recursive: true, force: true }) + } + }) + + test("two projects reusing one server name keep separate diagnostics", async () => { + // Server names are not unique across projects — `datamate` is the obvious example. + const projectA = await projectWith("datamate", VAR_A) + const projectB = await projectWith("datamate", VAR_B) + try { + await discoverExternalMcp(projectA) + await discoverExternalMcp(projectB) + + expect(unresolvedEnvVars("datamate", projectB)).toContain(VAR_B) + expect(unresolvedEnvVars("datamate", projectB)).not.toContain(VAR_A) + expect(unresolvedEnvVars("datamate", projectA)).toContain(VAR_A) + expect(unresolvedEnvVars("datamate", projectA)).not.toContain(VAR_B) + } finally { + await rm(projectA, { recursive: true, force: true }) + await rm(projectB, { recursive: true, force: true }) + } + }) + + test("concurrent discovery does not interleave one project's clear with another's writes", async () => { + const projectA = await projectWith("alpha", VAR_A) + const projectB = await projectWith("beta", VAR_B) + try { + await Promise.all([discoverExternalMcp(projectA), discoverExternalMcp(projectB)]) + expect(unresolvedEnvVars("alpha", projectA)).toContain(VAR_A) + expect(unresolvedEnvVars("beta", projectB)).toContain(VAR_B) + } finally { + await rm(projectA, { recursive: true, force: true }) + await rm(projectB, { recursive: true, force: true }) + } + }) + + test("discoveredSource and configDrift are per project", async () => { + const projectA = await projectWith("alpha", VAR_A) + const projectB = await projectWith("beta", VAR_B) + try { + await discoverExternalMcp(projectA) + await discoverExternalMcp(projectB) + expect(discoveredSource("alpha", projectA)).toContain(".vscode/mcp.json") + expect(configDrift(projectA).every((d) => d.server !== "beta")).toBe(true) + } finally { + await rm(projectA, { recursive: true, force: true }) + await rm(projectB, { recursive: true, force: true }) + } + }) +}) diff --git a/packages/opencode/test/mcp/discover.test.ts b/packages/opencode/test/mcp/discover.test.ts index ad4e243db..3dfbdfe5f 100644 --- a/packages/opencode/test/mcp/discover.test.ts +++ b/packages/opencode/test/mcp/discover.test.ts @@ -513,7 +513,7 @@ describe("unresolvedEnvVars staleness", () => { await writeServer() await discoverExternalMcp(tempDir) - expect(unresolvedEnvVars("stale")).toContain(VAR) + expect(unresolvedEnvVars("stale", tempDir)).toContain(VAR) // The user sets the variable and discovery runs again (config reload / mcp_discover). process.env[VAR] = "now-set" @@ -521,7 +521,7 @@ describe("unresolvedEnvVars staleness", () => { // Previously this still returned [VAR]: the record only ever unioned, and the recording // site sits inside an `unresolvedNames.length > 0` guard, so a clean run never cleared it. // `/mcps` kept telling the user to set a variable that already resolved. - expect(unresolvedEnvVars("stale")).toEqual([]) + expect(unresolvedEnvVars("stale", tempDir)).toEqual([]) }) test("still reports it while it is genuinely unset", async () => { @@ -529,7 +529,7 @@ describe("unresolvedEnvVars staleness", () => { await discoverExternalMcp(tempDir) await discoverExternalMcp(tempDir) // The reset must not swallow a real, still-unresolved variable across runs. - expect(unresolvedEnvVars("stale")).toContain(VAR) + expect(unresolvedEnvVars("stale", tempDir)).toContain(VAR) }) }) // altimate_change end From 12d87d41c81dba0a84b027fe41f587f031693806 Mon Sep 17 00:00:00 2001 From: Haider Date: Mon, 31 Aug 2026 22:34:58 +0530 Subject: [PATCH 2/2] fix(mcp): declare config ownership instead of guessing it from the path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replaces the read-time path filter from the previous commit, which did not work. That filter treated any source under `$HOME` as shared, on the theory that only the global config dir lives there. Projects live under `$HOME` too, so `/Users/me/code/projB/altimate-code.json` was classified as shared and still leaked into project A's diagnostics — the exact case the change existed to prevent. It passed review and passed its own tests because those tests used `/virtual/...` fixtures, which sit outside `$HOME` and so exercised the one shape the filter handled. Ownership is now declared by whoever loads a source, since the loader always knows and the path never reliably tells you: * `SHARED_CONFIG` for sources every instance loads — the global config dir, `OPENCODE_CONFIG`, macOS managed preferences. * `ctx.directory` for project-local files, the console-managed config, and `OPENCODE_CONFIG_CONTENT`. * A source nobody declared is omitted rather than attributed to a guess. `blankedEnvVars(projectDir)` returns that project's sources plus the shared ones. The tests now use `$HOME`-based fixtures, so they fail against the version this replaces. Also from the review round: * `_unresolvedEnv`, `_drift` and `_discoveredSource` delete the per-project bucket on reset rather than emptying it, so a long-lived server does not retain one Map per directory it has ever served. * Dropped a redundant `altimate_change` marker nested inside the `/mcps` block that already covers it. * Test hygiene: `afterEach` cleanup for module-level drift, saved and restored `process.env` around the discovery tests, and a single teardown path that survives a failure while creating the second project. typecheck clean. test/config + test/mcp + test/session: 1229 pass, 0 fail. MCP CLI tests: 9 pass, 0 fail. No new formatting violations in any file. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_018fJ3X7pcGT4R9yzjsJnqsV --- packages/opencode/src/config/config.ts | 42 ++++-- packages/opencode/src/config/tui.ts | 2 +- packages/opencode/src/config/variable.ts | 60 ++++---- packages/opencode/src/mcp/discover.ts | 13 +- packages/opencode/src/session/prompt.ts | 11 +- .../opencode/test/config/blanked-env.test.ts | 139 ++++++++++-------- .../opencode/test/mcp/config-drift.test.ts | 5 +- .../mcp/diagnostics-instance-scope.test.ts | 98 ++++++------ 8 files changed, 213 insertions(+), 157 deletions(-) diff --git a/packages/opencode/src/config/config.ts b/packages/opencode/src/config/config.ts index efe58c6b3..2efb582b7 100644 --- a/packages/opencode/src/config/config.ts +++ b/packages/opencode/src/config/config.ts @@ -156,12 +156,16 @@ async function substituteWellKnownRemoteConfig(input: { dir: string source: string env: Record + // altimate_change start — upstream_fix (#701): the project this load belongs to, so the + // blanked-variable record can be attributed to it rather than guessed from the path. + projectDir: string + // altimate_change end }) { if (!isRecord(input.value) || typeof input.value.url !== "string") return undefined // altimate_change start — upstream_fix (#701): the url and every header below publish under // this same source, so clear once here and let those calls union into one record. - ConfigVariable.resetBlankedEnvVars(input.source) + ConfigVariable.resetBlankedEnvVars(input.source, input.projectDir) // altimate_change end const url = await ConfigVariable.substitute({ text: input.value.url, @@ -345,14 +349,6 @@ export const layer = Layer.effect( const loadFile = Effect.fnUntraced(function* (filepath: string, env?: Record) { yield* Effect.logInfo("loading", { path: filepath }) - // altimate_change start — upstream_fix (#701): substitution unions now, so whoever begins a - // load clears this source first. Before the empty-file return, not after: a config that is - // deleted or emptied must drop the names it recorded while it still had a `{env:VAR}`, - // otherwise `mcp list` warns about a variable that appears in no config at all. - // Deliberately NOT inside loadConfig — the well-known flow records url/header blanks under - // the same source before calling it, and a reset in there threw those names away. - ConfigVariable.resetBlankedEnvVars(filepath) - // altimate_change end const text = yield* readConfigFile(filepath) if (!text) return {} as Info return yield* loadConfig(text, { path: filepath }, env) @@ -372,6 +368,13 @@ export const layer = Layer.effect( .pipe(Effect.catch(() => Effect.void)) } } + // altimate_change start — upstream_fix (#701): declare ownership before each load, so a + // diagnostic can be attributed to a project or to every project. Clearing here also means a + // config that is deleted or emptied drops what it recorded while it still had a `{env:VAR}`. + for (const f of ["config.json", "opencode.json", "opencode.jsonc", "altimate-code.json", "altimate-code.jsonc"]) { + ConfigVariable.resetBlankedEnvVars(path.join(Global.Path.config, f), ConfigVariable.SHARED_CONFIG) + } + // altimate_change end result = mergeConfig(result, yield* loadFile(path.join(Global.Path.config, "config.json"), env)) result = mergeConfig(result, yield* loadFile(path.join(Global.Path.config, "opencode.json"), env)) result = mergeConfig(result, yield* loadFile(path.join(Global.Path.config, "opencode.jsonc"), env)) @@ -488,6 +491,9 @@ export const layer = Layer.effect( dir: url, source: wellknownURL, env: authEnv, + // altimate_change start — upstream_fix (#701): attribute this load to the project. + projectDir: ctx.directory, + // altimate_change end }), ) const fetchedConfig = remote @@ -523,12 +529,18 @@ export const layer = Layer.effect( yield* merge(Global.Path.config, global, "global") if (Flag.OPENCODE_CONFIG) { + // altimate_change start — upstream_fix (#701): a process-wide config every instance loads. + ConfigVariable.resetBlankedEnvVars(Flag.OPENCODE_CONFIG, ConfigVariable.SHARED_CONFIG) + // altimate_change end yield* merge(Flag.OPENCODE_CONFIG, yield* loadFile(Flag.OPENCODE_CONFIG, authEnv)) yield* Effect.logDebug("loaded custom config", { path: Flag.OPENCODE_CONFIG }) } if (!Flag.OPENCODE_DISABLE_PROJECT_CONFIG) { for (const file of yield* ConfigPaths.files("opencode", ctx.directory, ctx.worktree).pipe(Effect.orDie)) { + // altimate_change start — upstream_fix (#701): this file belongs to this project. + ConfigVariable.resetBlankedEnvVars(file, ctx.directory) + // altimate_change end yield* merge(file, yield* loadFile(file, authEnv), "local") } } @@ -561,6 +573,9 @@ export const layer = Layer.effect( // altimate_change end const source = path.join(dir, file) yield* Effect.logDebug(`loading config from ${source}`) + // altimate_change start — upstream_fix (#701): loaded for this instance. + ConfigVariable.resetBlankedEnvVars(source, ctx.directory) + // altimate_change end yield* merge(source, yield* loadFile(source, authEnv)) result.agent ??= {} result.mode ??= {} @@ -611,7 +626,7 @@ export const layer = Layer.effect( if (process.env.OPENCODE_CONFIG_CONTENT) { const source = "OPENCODE_CONFIG_CONTENT" // altimate_change start — upstream_fix (#701): clear before this load. - ConfigVariable.resetBlankedEnvVars(source) + ConfigVariable.resetBlankedEnvVars(source, ctx.directory) // altimate_change end const next = yield* loadConfig(process.env.OPENCODE_CONFIG_CONTENT, { dir: ctx.directory, @@ -641,7 +656,7 @@ export const layer = Layer.effect( if (Option.isSome(configOpt)) { const source = `${url}/api/config` // altimate_change start — upstream_fix (#701): clear before this load. - ConfigVariable.resetBlankedEnvVars(source) + ConfigVariable.resetBlankedEnvVars(source, ctx.directory) // altimate_change end const next = yield* loadConfig(JSON.stringify(configOpt.value), { dir: path.dirname(source), @@ -679,6 +694,9 @@ export const layer = Layer.effect( // altimate_change end const source = path.join(managedDir, file) // altimate_change start — note a managed datamate key before merging + // altimate_change start — upstream_fix (#701): MDM-deployed, machine-wide. + ConfigVariable.resetBlankedEnvVars(source, ConfigVariable.SHARED_CONFIG) + // altimate_change end const managedFile = yield* loadFile(source) if (managedFile?.mcp && DATAMATE_KEY in managedFile.mcp) managedOwnsDatamate = true yield* merge(source, managedFile, "global") @@ -690,7 +708,7 @@ export const layer = Layer.effect( const managed = yield* Effect.promise(() => ConfigManaged.readManagedPreferences()) if (managed) { // altimate_change start — upstream_fix (#701): clear before this load. - ConfigVariable.resetBlankedEnvVars(managed.source) + ConfigVariable.resetBlankedEnvVars(managed.source, ConfigVariable.SHARED_CONFIG) // altimate_change end // altimate_change start — note a managed datamate key before merging const managedPrefs = yield* loadConfig(managed.text, { diff --git a/packages/opencode/src/config/tui.ts b/packages/opencode/src/config/tui.ts index 2ae0e2f80..9e80d7aa7 100644 --- a/packages/opencode/src/config/tui.ts +++ b/packages/opencode/src/config/tui.ts @@ -107,7 +107,7 @@ const loadState = Effect.fn("TuiConfig.loadState")(function* (ctx: { directory: // altimate_change start — upstream_fix (#701): substitution unions now instead of // replacing, so every caller clears first. Without this a `{env:VAR}` in tui.json that // was later fixed kept being reported blank for the life of the process. - ConfigVariable.resetBlankedEnvVars(configFilepath) + ConfigVariable.resetBlankedEnvVars(configFilepath, ctx.directory) // altimate_change end const expanded = yield* Effect.promise(() => ConfigVariable.substitute({ text, type: "path", path: configFilepath, missing: "empty" }), diff --git a/packages/opencode/src/config/variable.ts b/packages/opencode/src/config/variable.ts index 728789e61..f92647603 100644 --- a/packages/opencode/src/config/variable.ts +++ b/packages/opencode/src/config/variable.ts @@ -6,7 +6,6 @@ import { Filesystem } from "@/util/filesystem" import { InvalidError } from "@opencode-ai/core/v1/config/error" // altimate_change start — upstream_fix: restore ${VAR}/${VAR:-default}/$${VAR} config interpolation import { ConfigPaths } from "@/config/paths" -import { Global } from "@/global" // altimate_change end type ParseSource = @@ -34,45 +33,54 @@ type SubstituteInput = ParseSource & { // recorded here. `{env:VAR}` has no such deferral — it becomes "" and the config parses clean, so // a missing `{env:SNOWFLAKE_PASSWORD}` launches an MCP server with a blank credential and fails // later with an error naming neither the variable nor this file. -// -// Keyed projectDir -> config source. One process serves several projects (the server resolves an -// instance per request from `x-opencode-directory`), and a flat source-keyed map meant -// `blankedEnvVars()` handed every session every other project's config files. const _blankedEnv = new Map>() -/** Drop `src`'s record so a load starts clean; substitution then unions within that load. */ -export function resetBlankedEnvVars(src: string) { +/** + * Who a config source belongs to: a project directory, or SHARED for one every instance loads. + * + * Declared by the loader rather than guessed from the path. An earlier attempt inferred it — + * "under this project, or under $HOME/the config dir, is mine" — which is wrong in the ordinary + * case, because projects live under $HOME: `/Users/me/code/projB/altimate-code.json` was + * classified as shared and leaked into project A's diagnostics. The loader always knows; the + * path never reliably tells you. + */ +const _sourceOwner = new Map() + +/** Marker for a config every instance in the process loads: global config, OPENCODE_CONFIG, managed. */ +export const SHARED_CONFIG = "\u0000shared" + +/** + * Drop `src`'s record so a load starts clean, and declare who it belongs to. + * + * `owner` is the project directory being loaded, or `SHARED_CONFIG`. Substitution then unions + * into the source within that load. + */ +export function resetBlankedEnvVars(src: string, owner: string) { _blankedEnv.delete(src) + _sourceOwner.set(src, owner) } /** - * Variable names that silently became "" during config substitution, grouped by config source. + * Variable names that silently became "" while loading `projectDir`, grouped by config source. * - * Scoped by path rather than by threading a project through `substitute`: a config file that - * lives under a *different* project belongs to that project's session, not this one. One process - * serves several projects (the server resolves an instance per request from - * `x-opencode-directory`), and an unfiltered record handed every session every other project's - * files. Sources that are not project-local — the global config dir, `OPENCODE_CONFIG_CONTENT`, - * a remote config URL — are shared by every instance and are always included. + * Returns this project's own sources plus the shared ones. A source whose owner was never + * declared is omitted: a diagnostic that cannot be attributed is not worth showing to the wrong + * session, and every loader in this file declares one. */ export function blankedEnvVars(projectDir: string): { source: string; names: string[] }[] { return [..._blankedEnv.entries()] - .filter(([src]) => !isForeignProjectPath(src, projectDir)) + .filter(([src]) => { + const owner = _sourceOwner.get(src) + return owner === projectDir || owner === SHARED_CONFIG + }) .map(([src, names]) => ({ source: src, names: [...names].sort() })) .sort((a, b) => a.source.localeCompare(b.source)) } -/** True when `src` is an absolute path that sits outside `projectDir` and outside the config dir. */ -function isForeignProjectPath(src: string, projectDir: string): boolean { - if (!path.isAbsolute(src)) return false // OPENCODE_CONFIG_CONTENT, a URL — shared - const rel = path.relative(projectDir, src) - if (rel && !rel.startsWith("..") && !path.isAbsolute(rel)) return false // under this project - // The user-level config dir and the home directory are shared by every instance. - const shared = [Global.Path.config, os.homedir()].filter(Boolean) as string[] - return !shared.some((base) => { - const r = path.relative(base, src) - return r !== "" && !r.startsWith("..") && !path.isAbsolute(r) - }) +/** Test seam — forget every source's ownership and recorded names. */ +export function resetAllBlankedEnvVars() { + _blankedEnv.clear() + _sourceOwner.clear() } // altimate_change end diff --git a/packages/opencode/src/mcp/discover.ts b/packages/opencode/src/mcp/discover.ts index d873c6266..27883e422 100644 --- a/packages/opencode/src/mcp/discover.ts +++ b/packages/opencode/src/mcp/discover.ts @@ -82,9 +82,14 @@ export function unresolvedEnvVars(server: string, projectDir: string): string[] return [...(_unresolvedEnv.get(projectDir)?.get(server) ?? [])].sort() } -/** Drop this project's records. Called once per `discoverExternalMcp`. */ +/** + * Drop this project's records. Called once per `discoverExternalMcp`. + * + * Deletes the bucket rather than clearing it: a long-lived server visits many directories, and + * keeping an empty Map per directory it has ever served is a slow leak with no cleanup path. + */ function resetUnresolvedEnv(projectDir: string) { - _unresolvedEnv.get(projectDir)?.clear() + _unresolvedEnv.delete(projectDir) } // altimate_change end @@ -177,7 +182,7 @@ export function discoveredSource(server: string, projectDir: string): string | u /** Test seam — clears one project's drift, or every project's when no directory is given. */ export function resetConfigDrift(projectDir?: string) { if (projectDir === undefined) _drift.clear() - else _drift.get(projectDir)?.clear() + else _drift.delete(projectDir) } // altimate_change end interface ExternalMcpSource { @@ -457,7 +462,7 @@ export async function discoverExternalMcp(projectDir: string): Promise<{ // difference, otherwise left a stale entry and `mcp status` reported a mismatch that no // longer existed. The setConfigDrift calls after this run repopulate it. resetConfigDrift(projectDir) - _discoveredSource.get(projectDir)?.clear() + _discoveredSource.delete(projectDir) const result: Record = Object.create(null) const contributingSources: string[] = [] const homedir = os.homedir() diff --git a/packages/opencode/src/session/prompt.ts b/packages/opencode/src/session/prompt.ts index 90c27ccd9..dc40c15e1 100644 --- a/packages/opencode/src/session/prompt.ts +++ b/packages/opencode/src/session/prompt.ts @@ -3094,16 +3094,15 @@ NOTE: At any point in time through this workflow you should feel free to ask the " |", ) .join("\n") - // altimate_change start — upstream_fix (#701): `/mcps` showed only the per-server - // unresolved variables from discovery, while `mcp list` also reported file-scoped blanks. - // A server templated as `"url": "https://{env:MY_HOST}/mcp"` records against the config - // file rather than the server, so it appeared in the CLI and not here — in the session - // view, which is where someone is when a server will not connect. + // upstream_fix (#701): `/mcps` showed only the per-server unresolved variables from + // discovery, while `mcp list` also reported file-scoped blanks. A server templated as + // `"url": "https://{env:MY_HOST}/mcp"` records against the config file rather than the + // server, so it appeared in the CLI and not here — in the session view, which is where + // someone is when a server will not connect. (Inside the enclosing block's markers.) const blanked = formatBlankedEnvForDisplay(ConfigVariable.blankedEnvVars(Instance.directory)) const drift = formatConfigDriftForDisplay(McpDiscover.configDrift(Instance.directory)) const table = rows ? "MCP servers:\n\n| Server | Status |\n|---|---|\n" + rows : "No MCP servers configured." const responseText = [table, drift, blanked].filter(Boolean).join("\n\n") - // altimate_change end return respond(userMsg.info.id, responseText, model) } diff --git a/packages/opencode/test/config/blanked-env.test.ts b/packages/opencode/test/config/blanked-env.test.ts index 772edc09f..3bcddeda5 100644 --- a/packages/opencode/test/config/blanked-env.test.ts +++ b/packages/opencode/test/config/blanked-env.test.ts @@ -1,28 +1,51 @@ -// altimate_change start — upstream_fix (#701): the blank-variable record had no tests at all, -// which is how three separate placement mistakes reached review. These pin the contract every -// call site has to honour: substitution UNIONS into a source, and only a reset clears it. -import { describe, expect, test, beforeEach } from "bun:test" +// altimate_change start — upstream_fix (#701/#1211): the blank-variable record had no tests at +// all, which is how three placement mistakes reached review. These pin two things: substitution +// UNIONS into a source and only a reset clears it, and a source is attributed to the project that +// declared it rather than guessed from its path. +import { describe, expect, test, beforeEach, afterEach } from "bun:test" +import os from "os" +import path from "path" import { ConfigVariable } from "@/config/variable" -const SOURCE = "/virtual/blanked-env-test/config.json" +// Deliberately under $HOME. An earlier version of this fix inferred ownership from the path and +// treated everything under $HOME as shared; tests using /virtual/... paths passed anyway and hid +// it. Real projects live under $HOME, so the fixtures do too. +const PROJECT = path.join(os.homedir(), "code", "blanked-env-project-a") +const OTHER = path.join(os.homedir(), "code", "blanked-env-project-b") +const SOURCE = path.join(PROJECT, "altimate-code.json") +const OTHER_SOURCE = path.join(OTHER, "altimate-code.json") const VAR = "ALTIMATE_TEST_BLANKED_VAR" -const OTHER = "ALTIMATE_TEST_BLANKED_VAR_TWO" - -const PROJECT = "/virtual/blanked-env-test" +const OTHER_VAR = "ALTIMATE_TEST_BLANKED_VAR_TWO" function namesFor(source: string): string[] { return ConfigVariable.blankedEnvVars(PROJECT).find((e) => e.source === source)?.names ?? [] } -async function substitute(text: string) { - return ConfigVariable.substitute({ text, type: "virtual", dir: "/virtual", source: SOURCE, env: {} }) +async function substitute(text: string, source = SOURCE) { + return ConfigVariable.substitute({ text, type: "virtual", dir: path.dirname(source), source, env: {} }) } +let priorVar: string | undefined +let priorOther: string | undefined + describe("blankedEnvVars", () => { beforeEach(() => { + // Save and restore: these are process-wide, and a parallel `bun test` must not observe a + // variable this file removed or left behind. + priorVar = process.env[VAR] + priorOther = process.env[OTHER_VAR] delete process.env[VAR] - delete process.env[OTHER] - ConfigVariable.resetBlankedEnvVars(SOURCE) + delete process.env[OTHER_VAR] + ConfigVariable.resetAllBlankedEnvVars() + ConfigVariable.resetBlankedEnvVars(SOURCE, PROJECT) + }) + + afterEach(() => { + if (priorVar === undefined) delete process.env[VAR] + else process.env[VAR] = priorVar + if (priorOther === undefined) delete process.env[OTHER_VAR] + else process.env[OTHER_VAR] = priorOther + ConfigVariable.resetAllBlankedEnvVars() }) test("records a {env:VAR} that resolved to empty", async () => { @@ -31,12 +54,11 @@ describe("blankedEnvVars", () => { }) test("unions across substitutions of one source instead of replacing", async () => { - // A remote config substitutes its url and then each header separately, all under one - // source. Replacing meant the later call erased what the earlier one found, so a blank - // credential in the url was never reported. + // A remote config substitutes its url and then each header separately, all under one source. + // Replacing meant the later call erased what the earlier one found. await substitute(`{"url":"{env:${VAR}}"}`) - await substitute(`{"header":"{env:${OTHER}}"}`) - expect(namesFor(SOURCE).sort()).toEqual([VAR, OTHER].sort()) + await substitute(`{"header":"{env:${OTHER_VAR}}"}`) + expect(namesFor(SOURCE).sort()).toEqual([VAR, OTHER_VAR].sort()) }) test("a later clean substitution does not erase an earlier finding", async () => { @@ -49,58 +71,59 @@ describe("blankedEnvVars", () => { await substitute(`{"token":"{env:${VAR}}"}`) expect(namesFor(SOURCE)).toContain(VAR) - // The user sets the variable and the file is loaded again. process.env[VAR] = "now-set" - try { - ConfigVariable.resetBlankedEnvVars(SOURCE) - await substitute(`{"token":"{env:${VAR}}"}`) - expect(namesFor(SOURCE)).toEqual([]) - } finally { - delete process.env[VAR] - } + ConfigVariable.resetBlankedEnvVars(SOURCE, PROJECT) + await substitute(`{"token":"{env:${VAR}}"}`) + expect(namesFor(SOURCE)).toEqual([]) }) test("reset alone clears, for a source that is no longer loaded at all", async () => { - // The case that motivated moving the reset above loadFile's empty-file return: a config - // that is deleted or emptied must drop what it recorded, or `mcp list` keeps warning about - // a variable that appears in no config. + // The case behind moving the reset above loadFile's empty-file return: a config that is + // deleted or emptied must drop what it recorded. await substitute(`{"token":"{env:${VAR}}"}`) - ConfigVariable.resetBlankedEnvVars(SOURCE) + ConfigVariable.resetBlankedEnvVars(SOURCE, PROJECT) expect(namesFor(SOURCE)).toEqual([]) }) }) -// altimate_change end -// altimate_change start — upstream_fix (#1211): one process serves several projects. -describe("blankedEnvVars project scoping", () => { - const OTHER = "/virtual/some-other-project" - - test("a config file under another project is not reported here", async () => { - // The server resolves an instance per request from `x-opencode-directory`, so two projects - // are live in one process. Project B's local config is B's session's business, not A's. - await ConfigVariable.substitute({ - text: `{"token":"{env:${VAR}}"}`, - type: "virtual", - dir: OTHER, - source: OTHER + "/altimate-code.json", - env: {}, - }) - const sources = ConfigVariable.blankedEnvVars(PROJECT).map((e) => e.source) - expect(sources).not.toContain(OTHER + "/altimate-code.json") - // ...and it is still visible to the project it belongs to. - expect(ConfigVariable.blankedEnvVars(OTHER).map((e) => e.source)).toContain(OTHER + "/altimate-code.json") +describe("blankedEnvVars ownership", () => { + beforeEach(() => { + priorVar = process.env[VAR] + delete process.env[VAR] + ConfigVariable.resetAllBlankedEnvVars() + }) + + afterEach(() => { + if (priorVar === undefined) delete process.env[VAR] + else process.env[VAR] = priorVar + ConfigVariable.resetAllBlankedEnvVars() + }) + + test("another project's config is not reported here, even under $HOME", async () => { + // The bug this replaces: both projects live under $HOME, so a path-based rule called B's + // config "shared" and handed it to A. + ConfigVariable.resetBlankedEnvVars(OTHER_SOURCE, OTHER) + await substitute(`{"token":"{env:${VAR}}"}`, OTHER_SOURCE) + + expect(ConfigVariable.blankedEnvVars(PROJECT).map((e) => e.source)).not.toContain(OTHER_SOURCE) + expect(ConfigVariable.blankedEnvVars(OTHER).map((e) => e.source)).toContain(OTHER_SOURCE) + }) + + test("a shared config is reported to every project", async () => { + // Global config, OPENCODE_CONFIG and managed preferences are loaded by every instance, so + // suppressing them per project would lose a real diagnostic. + const shared = path.join(os.homedir(), ".config", "altimate-code", "altimate-code.json") + ConfigVariable.resetBlankedEnvVars(shared, ConfigVariable.SHARED_CONFIG) + await substitute(`{"token":"{env:${VAR}}"}`, shared) + + expect(ConfigVariable.blankedEnvVars(PROJECT).map((e) => e.source)).toContain(shared) + expect(ConfigVariable.blankedEnvVars(OTHER).map((e) => e.source)).toContain(shared) }) - test("a non-path source stays shared, since every instance loads it", async () => { - await ConfigVariable.substitute({ - text: `{"token":"{env:${VAR}}"}`, - type: "virtual", - dir: "/virtual", - source: "OPENCODE_CONFIG_CONTENT", - env: {}, - }) - expect(ConfigVariable.blankedEnvVars(PROJECT).map((e) => e.source)).toContain("OPENCODE_CONFIG_CONTENT") - expect(ConfigVariable.blankedEnvVars(OTHER).map((e) => e.source)).toContain("OPENCODE_CONFIG_CONTENT") + test("a source nobody declared is not attributed to a guess", async () => { + await substitute(`{"token":"{env:${VAR}}"}`, path.join(os.homedir(), "stray", "config.json")) + expect(ConfigVariable.blankedEnvVars(PROJECT)).toEqual([]) + expect(ConfigVariable.blankedEnvVars(OTHER)).toEqual([]) }) }) // altimate_change end diff --git a/packages/opencode/test/mcp/config-drift.test.ts b/packages/opencode/test/mcp/config-drift.test.ts index f5276ee71..844512590 100644 --- a/packages/opencode/test/mcp/config-drift.test.ts +++ b/packages/opencode/test/mcp/config-drift.test.ts @@ -1,6 +1,6 @@ // altimate_change start — upstream_fix (#878): discovery skipped already-configured servers // without a word, so a changed .vscode/mcp.json never surfaced. These pin what counts as drift. -import { describe, expect, test, beforeEach } from "bun:test" +import { describe, expect, test, beforeEach, afterEach } from "bun:test" import { driftFields, setConfigDrift, configDrift, resetConfigDrift } from "../../src/mcp/discover" describe("driftFields", () => { @@ -37,6 +37,9 @@ describe("configDrift record", () => { // The record is per project now, so every call names the directory it belongs to. const PROJECT = "/tmp/project-a" beforeEach(() => resetConfigDrift()) + // `_drift` is module-level: the cross-project cases below deliberately leave OTHER populated, + // so clear everything afterwards rather than letting the next test in this worker observe it. + afterEach(() => resetConfigDrift()) test("records only servers that actually differ", () => { setConfigDrift("datamate", ".vscode/mcp.json", ["environment.ALTIMATE_EXTENSION_RPC"], PROJECT) diff --git a/packages/opencode/test/mcp/diagnostics-instance-scope.test.ts b/packages/opencode/test/mcp/diagnostics-instance-scope.test.ts index ed1e6764d..90e14346a 100644 --- a/packages/opencode/test/mcp/diagnostics-instance-scope.test.ts +++ b/packages/opencode/test/mcp/diagnostics-instance-scope.test.ts @@ -20,81 +20,81 @@ async function projectWith(server: string, varName: string): Promise { return dir } +let priorA: string | undefined +let priorB: string | undefined +const created: string[] = [] + +/** Create a project and register it for teardown, so a failure mid-setup still cleans up. */ +async function project(server: string, varName: string): Promise { + const dir = await projectWith(server, varName) + created.push(dir) + return dir +} + beforeEach(async () => { homeDir = await mkdtemp(path.join(tmpdir(), "mcp-scope-home-")) homedirSpy = spyOn(os, "homedir").mockImplementation(() => homeDir) + // Save rather than blindly delete: these are process-wide and a parallel run must not observe + // a variable this file removed. + priorA = process.env[VAR_A] + priorB = process.env[VAR_B] delete process.env[VAR_A] delete process.env[VAR_B] }) afterEach(async () => { homedirSpy?.mockRestore() + if (priorA === undefined) delete process.env[VAR_A] + else process.env[VAR_A] = priorA + if (priorB === undefined) delete process.env[VAR_B] + else process.env[VAR_B] = priorB await rm(homeDir, { recursive: true, force: true }) + await Promise.all(created.splice(0).map((d) => rm(d, { recursive: true, force: true }))) }) describe("MCP diagnostics are project-scoped", () => { test("a second project's discovery does not erase the first project's diagnostics", async () => { - const projectA = await projectWith("alpha", VAR_A) - const projectB = await projectWith("beta", VAR_B) - try { - await discoverExternalMcp(projectA) - expect(unresolvedEnvVars("alpha", projectA)).toContain(VAR_A) + const projectA = await project("alpha", VAR_A) + const projectB = await project("beta", VAR_B) + await discoverExternalMcp(projectA) + expect(unresolvedEnvVars("alpha", projectA)).toContain(VAR_A) - // A second session in the same process discovers for a different project. Project A's - // session is still open and still asking about its own servers. - await discoverExternalMcp(projectB) + // A second session in the same process discovers for a different project. Project A's + // session is still open and still asking about its own servers. + await discoverExternalMcp(projectB) - expect(unresolvedEnvVars("beta", projectB)).toContain(VAR_B) - // The failing half: a module-global record cleared per run means A's answer is gone. - expect(unresolvedEnvVars("alpha", projectA)).toContain(VAR_A) - } finally { - await rm(projectA, { recursive: true, force: true }) - await rm(projectB, { recursive: true, force: true }) - } + expect(unresolvedEnvVars("beta", projectB)).toContain(VAR_B) + // The failing half: a module-global record cleared per run means A's answer is gone. + expect(unresolvedEnvVars("alpha", projectA)).toContain(VAR_A) }) test("two projects reusing one server name keep separate diagnostics", async () => { // Server names are not unique across projects — `datamate` is the obvious example. - const projectA = await projectWith("datamate", VAR_A) - const projectB = await projectWith("datamate", VAR_B) - try { - await discoverExternalMcp(projectA) - await discoverExternalMcp(projectB) + const projectA = await project("datamate", VAR_A) + const projectB = await project("datamate", VAR_B) + await discoverExternalMcp(projectA) + await discoverExternalMcp(projectB) - expect(unresolvedEnvVars("datamate", projectB)).toContain(VAR_B) - expect(unresolvedEnvVars("datamate", projectB)).not.toContain(VAR_A) - expect(unresolvedEnvVars("datamate", projectA)).toContain(VAR_A) - expect(unresolvedEnvVars("datamate", projectA)).not.toContain(VAR_B) - } finally { - await rm(projectA, { recursive: true, force: true }) - await rm(projectB, { recursive: true, force: true }) - } + expect(unresolvedEnvVars("datamate", projectB)).toContain(VAR_B) + expect(unresolvedEnvVars("datamate", projectB)).not.toContain(VAR_A) + expect(unresolvedEnvVars("datamate", projectA)).toContain(VAR_A) + expect(unresolvedEnvVars("datamate", projectA)).not.toContain(VAR_B) }) test("concurrent discovery does not interleave one project's clear with another's writes", async () => { - const projectA = await projectWith("alpha", VAR_A) - const projectB = await projectWith("beta", VAR_B) - try { - await Promise.all([discoverExternalMcp(projectA), discoverExternalMcp(projectB)]) - expect(unresolvedEnvVars("alpha", projectA)).toContain(VAR_A) - expect(unresolvedEnvVars("beta", projectB)).toContain(VAR_B) - } finally { - await rm(projectA, { recursive: true, force: true }) - await rm(projectB, { recursive: true, force: true }) - } + const projectA = await project("alpha", VAR_A) + const projectB = await project("beta", VAR_B) + await Promise.all([discoverExternalMcp(projectA), discoverExternalMcp(projectB)]) + expect(unresolvedEnvVars("alpha", projectA)).toContain(VAR_A) + expect(unresolvedEnvVars("beta", projectB)).toContain(VAR_B) }) test("discoveredSource and configDrift are per project", async () => { - const projectA = await projectWith("alpha", VAR_A) - const projectB = await projectWith("beta", VAR_B) - try { - await discoverExternalMcp(projectA) - await discoverExternalMcp(projectB) - expect(discoveredSource("alpha", projectA)).toContain(".vscode/mcp.json") - expect(configDrift(projectA).every((d) => d.server !== "beta")).toBe(true) - } finally { - await rm(projectA, { recursive: true, force: true }) - await rm(projectB, { recursive: true, force: true }) - } + const projectA = await project("alpha", VAR_A) + const projectB = await project("beta", VAR_B) + await discoverExternalMcp(projectA) + await discoverExternalMcp(projectB) + expect(discoveredSource("alpha", projectA)).toContain(".vscode/mcp.json") + expect(configDrift(projectA).every((d) => d.server !== "beta")).toBe(true) }) })