From 52331ab637d02df820fa5e14792b2d3e31140912 Mon Sep 17 00:00:00 2001 From: Feng Date: Sat, 10 Oct 2026 23:23:02 +0100 Subject: [PATCH] fix(setup): preserve configured MCP server environment (#1607) * fix(setup): preserve configured MCP server environment * fix(setup): validate retained server environment before writes --- docs/installation.md | 6 + src/application/ClientConfigurationFiles.ts | 12 +- .../ClientRegistrationEnvironment.ts | 54 +++++ src/application/SetupClientConfiguration.ts | 50 +++-- src/application/SetupPlan.ts | 4 +- tests/acceptance/setup/piClientSetup.test.ts | 192 ++++++++++++++++++ .../clientConfigurationEnvironment.test.ts | 181 +++++++++++++++++ .../filesystem/clientLifecycle.test.ts | 12 +- 8 files changed, 479 insertions(+), 32 deletions(-) create mode 100644 src/application/ClientRegistrationEnvironment.ts create mode 100644 tests/boundary/filesystem/clientConfigurationEnvironment.test.ts diff --git a/docs/installation.md b/docs/installation.md index a76b824a27..7a6359a9bd 100644 --- a/docs/installation.md +++ b/docs/installation.md @@ -360,6 +360,12 @@ Hopper installation or any file write. Declining or pressing Ctrl-C makes no changes. Agent configuration writes preserve unrelated entries and comments, create backups, use atomic replacement, and verify their result. Setup and uninstall retain an existing `.rea.backup` rather than replacing the first snapshot. +Setup also retains valid environment overrides in REA's existing server entry, +such as `REA_MCP_INPUT_SCHEMA_PROFILE` and `REA_MCP_MAX_RESPONSE_BYTES`. +Detected provider settings replace only matching environment keys. +Setup and doctor report a malformed REA environment map or invalid runtime +setting before setup writes files. Repair the named setting in the client +configuration; setup does not silently discard the rest of the map. After setup, REA reports which agents, analysis tools, and workflow files passed its final checks. Restart any agent named in the completion message, then begin diff --git a/src/application/ClientConfigurationFiles.ts b/src/application/ClientConfigurationFiles.ts index a3ff426e77..a1e2a93383 100644 --- a/src/application/ClientConfigurationFiles.ts +++ b/src/application/ClientConfigurationFiles.ts @@ -6,6 +6,7 @@ import { type ClientConfigurationDocument, } from "./ClientConfigurationDocument.js"; import type { SetupClient } from "./SupportedClients.js"; +import { clientRegistrationEnvironment } from "./ClientRegistrationEnvironment.js"; /** A named source failure that setup can explain without losing the bad path. */ export interface ClientConfigurationFileError { @@ -63,7 +64,16 @@ export const readClientConfigurationFiles = async ( for (const path of client.configPaths ?? [client.configPath]) { const document = await readConfigurationFile(path, client.format); if (!document.ok) return document; - if (document.value !== undefined) documents.push(document.value); + if (document.value !== undefined) { + const environment = clientRegistrationEnvironment(document.value); + if (!environment.ok) + return err({ + kind: "malformed", + path, + detail: `Configuration ${path} has an invalid REA server environment: ${environment.error.settings.map(({ setting, constraint }) => `${setting}: ${constraint}`).join("; ")}. Repair it before rerunning setup.`, + }); + documents.push(document.value); + } } return ok(documents); }; diff --git a/src/application/ClientRegistrationEnvironment.ts b/src/application/ClientRegistrationEnvironment.ts new file mode 100644 index 0000000000..0d537ba9b2 --- /dev/null +++ b/src/application/ClientRegistrationEnvironment.ts @@ -0,0 +1,54 @@ +import { z } from "zod"; + +import { parseEnvironment } from "../config/environment.js"; +import { ConfigurationError } from "../domain/configurationErrors.js"; +import { err, ok, type Result } from "../domain/result.js"; +import { + effectiveClientServer, + type ClientConfigurationDocument, +} from "./ClientConfigurationDocument.js"; +import { PRODUCT_IDENTITY } from "../identity.js"; + +/** Read explicit REA overrides without dropping peers when one value is invalid. */ +export const clientRegistrationEnvironment = ( + parsed: ClientConfigurationDocument | undefined, + overrides: Readonly> = {}, +): Result>, ConfigurationError> => { + const key = + parsed?.dialect === "opencode" || parsed?.dialect === "opencode_v2" + ? "environment" + : "env"; + const registration = + parsed === undefined + ? undefined + : effectiveClientServer(parsed, PRODUCT_IDENTITY.mcpServerKey); + const raw = + typeof registration === "object" && registration !== null + ? Reflect.get(registration, key) + : undefined; + const existing = z + .record(z.string(), z.string()) + .safeParse(raw === undefined ? {} : raw); + if (!existing.success) + return err( + new ConfigurationError("Invalid REA server environment", { + settings: existing.error.issues.map((issue) => ({ + setting: [key, ...issue.path].join("."), + constraint: + "Server environment must be an object containing only string values", + })), + }), + ); + // Validate known settings with the runtime's parser, but retain the original + // string overrides, including unknown client-specific keys and no defaults. + const environment = { ...existing.data, ...overrides }; + const validated = parseEnvironment(environment); + if (!validated.ok) return validated; + return ok( + Object.fromEntries( + Object.entries(environment).sort(([left], [right]) => + left.localeCompare(right), + ), + ), + ); +}; diff --git a/src/application/SetupClientConfiguration.ts b/src/application/SetupClientConfiguration.ts index 4ae8a5ff71..7648456dd4 100644 --- a/src/application/SetupClientConfiguration.ts +++ b/src/application/SetupClientConfiguration.ts @@ -11,12 +11,12 @@ import { serializeClientConfiguration, withClientServers, type ClientConfigurationDocument, - type ClientRegistrationDialect, } from "./ClientConfigurationDocument.js"; import { constants as fsConstants } from "node:fs"; import { copyFile, mkdir, rm, writeFile } from "node:fs/promises"; import { dirname } from "node:path"; import writeFileAtomic from "write-file-atomic"; +import { clientRegistrationEnvironment } from "./ClientRegistrationEnvironment.js"; import { PRODUCT_IDENTITY } from "../identity.js"; import { npxRegistrationCommand } from "./ClientRegistrationIdentity.js"; @@ -54,7 +54,12 @@ const configureClientDocument = async ( const policy = await readClientPolicyBlock( client, undefined, - clientConfigurationDesired(client, environment, command, client.format), + clientConfigurationDesired( + client, + environment, + command, + files.value.at(-1), + ), ); if (!policy.ok || policy.value !== undefined) return { status: "failed", reason: "readback" }; @@ -78,7 +83,7 @@ const configureClientDocument = async ( const policy = await readClientPolicyBlock( client, parsed, - clientConfigurationDesired(client, environment, command, parsed.dialect), + clientConfigurationDesired(client, environment, command, parsed), ); if (!policy.ok || policy.value !== undefined) return { status: "failed", reason: "readback" }; @@ -91,7 +96,7 @@ const configureClientDocument = async ( client, environment, command, - parsed.dialect, + parsed, ); if (registrationCurrent(parsed, desired)) return { status: "unchanged" }; const backupPath = @@ -184,12 +189,7 @@ export const clientConfigurationAligned = async ( const policy = await readClientPolicyBlock( client, parsed, - clientConfigurationDesired( - client, - providerEnvironment, - command, - parsed.dialect, - ), + clientConfigurationDesired(client, providerEnvironment, command, parsed), ); return ( policy.ok && @@ -200,7 +200,7 @@ export const clientConfigurationAligned = async ( client, providerEnvironment, command, - parsed.dialect, + parsed, ), ) ); @@ -229,7 +229,7 @@ export const inspectClientConfiguration = async ( client, providerEnvironment, command, - client.format, + files.value.at(-1), ), ); if (!policy.ok) @@ -261,12 +261,7 @@ export const inspectClientConfiguration = async ( const policy = await readClientPolicyBlock( client, parsed, - clientConfigurationDesired( - client, - providerEnvironment, - command, - parsed.dialect, - ), + clientConfigurationDesired(client, providerEnvironment, command, parsed), ); if (!policy.ok) return { status: "invalid", remediation: policy.error.detail }; @@ -276,7 +271,7 @@ export const inspectClientConfiguration = async ( client, providerEnvironment, command, - parsed.dialect, + parsed, ); if (registrationCurrent(parsed, desired)) return { status: "already_current" }; @@ -338,17 +333,20 @@ const clientConfigurationDesired = ( client: SetupClient, providerEnvironment: SetupProviderEnvironment, command: readonly string[], - format: ClientRegistrationDialect | undefined, + parsed?: ClientConfigurationDocument, ) => { - const environment = Object.fromEntries( - Object.entries(providerEnvironment).sort(([left], [right]) => - left.localeCompare(right), - ), + const format = parsed?.dialect ?? client.format ?? "json"; + // Preserve explicitly configured server settings, never the ambient process + // environment. Newly discovered provider paths win only for their own keys. + const environment = clientRegistrationEnvironment( + parsed, + providerEnvironment, ); + if (!environment.ok) throw environment.error; const registration = clientRegistrationEntry( - format ?? "json", + format, command.length === 0 ? [PRODUCT_IDENTITY.cliBinary, "mcp"] : command, - environment, + environment.value, ); return { ...registration, diff --git a/src/application/SetupPlan.ts b/src/application/SetupPlan.ts index 20f1e77a22..2871bd04b2 100644 --- a/src/application/SetupPlan.ts +++ b/src/application/SetupPlan.ts @@ -312,8 +312,8 @@ const clientConfigurationDetail = ( .map(([name, value]) => `${name}=${value}`); const environmentDetail = entries.length === 0 - ? " No additional environment variables." - : ` Environment: ${entries.join(", ")}.`; + ? " No additional provider environment variables. Existing server environment overrides are retained." + : ` Detected provider environment: ${entries.join(", ")}. Existing server environment overrides are retained; detected values replace matching keys.`; const hopperDetail = installHopper ? environment.HOPPER_LAUNCHER_PATH === undefined ? " If this plan installs Hopper, REA will add HOPPER_LAUNCHER_PATH using the launcher path reported by that installation." diff --git a/tests/acceptance/setup/piClientSetup.test.ts b/tests/acceptance/setup/piClientSetup.test.ts index bfb1d1b0a7..736e6a5088 100644 --- a/tests/acceptance/setup/piClientSetup.test.ts +++ b/tests/acceptance/setup/piClientSetup.test.ts @@ -1,11 +1,203 @@ import { mkdir, readFile, readdir, writeFile } from "node:fs/promises"; import { join, resolve } from "node:path"; +import { Client } from "@modelcontextprotocol/client"; +import { StdioClientTransport } from "@modelcontextprotocol/client/stdio"; import { expect } from "vitest"; +import { z } from "zod"; + +import { COMPACT_INPUT_SCHEMA_BUDGET_BYTES } from "../../../src/config/mcpInputSchemaProfile.js"; import { createTestTempDirectory } from "../../fixtures/temporaryDirectory.js"; import { cliTest } from "../../support/cli/cliFixture.js"; +for (const profile of ["", "bogus"]) + cliTest( + `setup and doctor refuse an invalid retained schema profile ${JSON.stringify(profile)}`, + async ({ cli }) => { + const home = await createTestTempDirectory("rea-pi-invalid-profile-cli-"); + const agent = join(home, ".pi", "agent"); + const config = join(agent, "mcp.json"); + const original = JSON.stringify({ + mcpServers: { + rea: { + type: "stdio", + command: "rea", + args: ["mcp"], + env: { REA_MCP_INPUT_SCHEMA_PROFILE: profile }, + }, + }, + }); + await mkdir(agent, { recursive: true }); + await writeFile(config, original); + const environment = { + HOME: home, + USERPROFILE: home, + HOPPER_LAUNCHER_PATH: join(home, "unconfigured-hopper"), + }; + const setup = await cli.run({ + arguments: [ + "setup", + "--client", + "pi", + "--skill=false", + "--yes", + "--json", + ], + cwd: home, + environment, + }); + expect(setup.exitCode).toBe(1); + expect(setup.json).toMatchObject({ + status: "needs_human", + plannedActions: [], + appliedActions: [], + remediation: expect.stringContaining("REA_MCP_INPUT_SCHEMA_PROFILE"), + }); + const doctor = await cli.run({ + arguments: ["doctor", "--client", "pi", "--json"], + cwd: home, + environment, + }); + expect(doctor.exitCode).toBe(1); + expect(doctor.json).toMatchObject({ + healthy: false, + identity: { + registrations: expect.arrayContaining([ + expect.objectContaining({ client: "pi", state: "invalid" }), + ]), + }, + }); + expect(await readFile(config, "utf8")).toBe(original); + await expect(readFile(`${config}.rea.backup`)).rejects.toMatchObject({ + code: "ENOENT", + }); + }, + ); + +cliTest( + "setup preserves a configured compact catalog through CLI migration and MCP startup", + async ({ cli }) => { + const home = await createTestTempDirectory("rea-pi-environment-cli-"); + const agent = join(home, ".pi", "agent"); + const config = join(agent, "mcp.json"); + const custom = { + REA_MCP_INPUT_SCHEMA_PROFILE: "compact", + REA_LOG_LEVEL: "silent", + }; + const original = JSON.stringify({ + mcpServers: { + rea: { + type: "stdio", + command: "npx", + args: ["-y", "rea-agents@1.0.0", "mcp"], + env: custom, + }, + }, + }); + await mkdir(agent, { recursive: true }); + await writeFile(config, original); + const environment = { + HOME: home, + USERPROFILE: home, + HOPPER_LAUNCHER_PATH: join(home, "unconfigured-hopper"), + }; + const arguments_ = [ + "setup", + "--client", + "pi", + "--skill=false", + "--yes", + "--json", + ]; + const planned = await cli.run({ + arguments: [...arguments_, "--dry-run"], + cwd: home, + environment, + }); + expect(planned.exitCode).toBe(0); + expect(planned.json).toMatchObject({ + plannedActions: [ + expect.objectContaining({ + id: "configure_client:pi", + detail: expect.stringContaining( + "Existing server environment overrides are retained", + ), + }), + ], + appliedActions: [], + }); + expect(await readFile(config, "utf8")).toBe(original); + const applied = await cli.run({ + arguments: arguments_, + cwd: home, + environment, + timeoutMs: 20_000, + }); + expect(applied.exitCode).toBe(0); + expect(applied.json).toMatchObject({ + status: "ready", + clients: { pi: { status: "configured" } }, + }); + const configured = await readFile(config, "utf8"); + const registration = z + .object({ + mcpServers: z.object({ + rea: z.object({ + command: z.string(), + args: z.array(z.string()), + env: z.record(z.string(), z.string()).optional(), + }), + }), + }) + .parse(JSON.parse(configured)).mcpServers.rea; + const transport = new StdioClientTransport({ + command: registration.command, + args: registration.args, + cwd: home, + env: { + PATH: process.env.PATH ?? "/usr/bin:/bin", + ...environment, + ...registration.env, + }, + stderr: "pipe", + }); + const client = new Client({ name: "setup-environment", version: "1" }); + try { + await client.connect(transport); + const { tools } = await client.listTools(); + expect(tools.length).toBeGreaterThan(0); + expect( + tools + .filter( + ({ inputSchema }) => + Buffer.byteLength(JSON.stringify(inputSchema), "utf8") > + COMPACT_INPUT_SCHEMA_BUDGET_BYTES, + ) + .map(({ name }) => name), + ).toEqual([]); + expect(await client.ping()).toEqual({}); + } finally { + await client.close(); + await transport.close(); + } + expect(registration.env).toMatchObject(custom); + expect(await readFile(`${config}.rea.backup`, "utf8")).toBe(original); + const repeated = await cli.run({ + arguments: arguments_, + cwd: home, + environment, + timeoutMs: 20_000, + }); + expect(repeated.exitCode).toBe(0); + expect(repeated.json).toMatchObject({ + plannedActions: [], + appliedActions: [], + }); + expect(await readFile(config, "utf8")).toBe(configured); + }, +); + cliTest( "Pi public CLI plans, requires approval, applies, diagnoses, repeats, and uninstalls only owned content", async ({ cli }) => { diff --git a/tests/boundary/filesystem/clientConfigurationEnvironment.test.ts b/tests/boundary/filesystem/clientConfigurationEnvironment.test.ts new file mode 100644 index 0000000000..9e0b340c5b --- /dev/null +++ b/tests/boundary/filesystem/clientConfigurationEnvironment.test.ts @@ -0,0 +1,181 @@ +import { access, mkdir, readFile, writeFile } from "node:fs/promises"; +import { join } from "node:path"; +import { expect, it } from "vitest"; + +import { + clientRegistrationEntry, + effectiveClientServer, + parseClientConfiguration, + serializeClientConfiguration, +} from "../../../src/application/ClientConfigurationDocument.js"; +import { + clientConfigurationAligned, + configureClientConfiguration, + inspectClientConfiguration, +} from "../../../src/application/SetupClientConfiguration.js"; +import { createTestTempDirectory } from "../../fixtures/temporaryDirectory.js"; +import { readClientRegistrationStatuses } from "../../../src/application/ClientRegistrationStatus.js"; + +it.each([ + ["cursor", "json", "mcpServers", "env"], + ["codex", "toml", "mcp_servers", "env"], + ["opencode", "opencode", "mcp", "environment"], + ["hermes", "hermes", "mcp_servers", "env"], +] as const)( + "preserves %s server environment through command and provider updates", + async (name, format, serversKey, environmentKey) => { + const root = await createTestTempDirectory("rea-setup-environment-"); + const configPath = join(root, "config"); + const client = { name, format, configPath }; + const custom = { + REA_MCP_INPUT_SCHEMA_PROFILE: "compact", + REA_MCP_MAX_RESPONSE_BYTES: "33554432", + REA_BROWSER_EXECUTABLE: join(root, "custom-browser"), + GHIDRA_INSTALL_DIR: join(root, "old-ghidra"), + }; + const original = serializeClientConfiguration( + { + [serversKey]: { + rea: clientRegistrationEntry(format, ["rea-old", "mcp"], custom), + other: clientRegistrationEntry(format, ["other"], { OTHER: "keep" }), + }, + }, + format, + ); + await writeFile(configPath, original); + const providers = { GHIDRA_INSTALL_DIR: join(root, "new-ghidra") }; + const command = ["rea", "mcp"]; + expect( + await inspectClientConfiguration(client, providers, command), + ).toMatchObject({ status: "update" }); + expect( + await configureClientConfiguration(client, providers, command), + ).toMatchObject({ status: "configured" }); + const configured = await readFile(configPath, "utf8"); + const parsed = parseClientConfiguration(configured, format); + expect(effectiveClientServer(parsed, "rea")).toMatchObject({ + [environmentKey]: { ...custom, ...providers }, + }); + expect(effectiveClientServer(parsed, "other")).toEqual( + effectiveClientServer( + parseClientConfiguration(original, format), + "other", + ), + ); + expect(await readFile(`${configPath}.rea.backup`, "utf8")).toBe(original); + expect(await clientConfigurationAligned(client, providers, command)).toBe( + true, + ); + expect( + await inspectClientConfiguration(client, providers, command), + ).toEqual({ status: "already_current" }); + expect( + await configureClientConfiguration(client, providers, command), + ).toEqual({ status: "unchanged" }); + expect(await readFile(configPath, "utf8")).toBe(configured); + }, +); + +it("retains legacy OpenCode environment when migrating into a native server table", async () => { + const root = await createTestTempDirectory("rea-opencode-environment-"); + const configPath = join(root, "opencode.json"); + const client = { name: "opencode", format: "opencode", configPath } as const; + const custom = { REA_MCP_INPUT_SCHEMA_PROFILE: "compact" }; + await writeFile( + configPath, + JSON.stringify({ + mcp: { + rea: clientRegistrationEntry("opencode", ["old", "mcp"], custom), + servers: { + other: clientRegistrationEntry("opencode_v2", ["other"], {}), + }, + }, + }), + ); + expect( + await configureClientConfiguration(client, {}, ["rea", "mcp"]), + ).toMatchObject({ status: "configured" }); + const parsed = parseClientConfiguration( + await readFile(configPath, "utf8"), + "opencode", + ); + expect(parsed.legacyServers).toEqual({}); + expect(parsed.servers.rea).toEqual( + clientRegistrationEntry("opencode_v2", ["rea", "mcp"], custom), + ); + expect( + await configureClientConfiguration(client, {}, ["rea", "mcp"]), + ).toEqual({ status: "unchanged" }); +}); + +it.each([ + [ + "non-string member", + { REA_MCP_INPUT_SCHEMA_PROFILE: "compact", PORT: 8080 }, + "env.PORT", + ], + [ + "boolean member", + { REA_MCP_INPUT_SCHEMA_PROFILE: "compact", FLAG: true }, + "env.FLAG", + ], + [ + "null member", + { REA_MCP_INPUT_SCHEMA_PROFILE: "compact", VALUE: null }, + "env.VALUE", + ], + ["array", [], "env"], + ["string", "not-a-map", "env"], + ["null map", null, "env"], + [ + "empty profile", + { REA_MCP_INPUT_SCHEMA_PROFILE: "" }, + "REA_MCP_INPUT_SCHEMA_PROFILE", + ], + [ + "invalid profile", + { REA_MCP_INPUT_SCHEMA_PROFILE: "bogus" }, + "REA_MCP_INPUT_SCHEMA_PROFILE", + ], + [ + "invalid response budget", + { REA_MCP_MAX_RESPONSE_BYTES: "1" }, + "REA_MCP_MAX_RESPONSE_BYTES", + ], +] as const)( + "refuses %s before changing the configuration", + async (_name, env, setting) => { + const home = await createTestTempDirectory("rea-invalid-environment-"); + const directory = join(home, ".cursor"); + const configPath = join(directory, "mcp.json"); + await mkdir(directory); + const original = JSON.stringify({ + mcpServers: { rea: { command: "rea", args: ["mcp"], env } }, + }); + await writeFile(configPath, original); + const client = { name: "cursor", format: "json", configPath } as const; + expect( + await inspectClientConfiguration(client, {}, ["rea", "mcp"]), + ).toMatchObject({ + status: "invalid", + remediation: expect.stringContaining(setting), + }); + expect( + await configureClientConfiguration(client, {}, ["rea", "mcp"]), + ).toEqual({ status: "failed", reason: "readback" }); + expect(await clientConfigurationAligned(client, {}, ["rea", "mcp"])).toBe( + false, + ); + const statuses = await readClientRegistrationStatuses(home, "rea", { + environment: {}, + }); + expect(statuses.find(({ client }) => client === "cursor")).toMatchObject({ + state: "invalid", + remediation: expect.stringContaining(setting), + }); + expect(await readFile(configPath, "utf8")).toBe(original); + await expect(access(`${configPath}.rea.backup`)).rejects.toMatchObject({ + code: "ENOENT", + }); + }, +); diff --git a/tests/boundary/filesystem/clientLifecycle.test.ts b/tests/boundary/filesystem/clientLifecycle.test.ts index 437f36480b..ae814c8855 100644 --- a/tests/boundary/filesystem/clientLifecycle.test.ts +++ b/tests/boundary/filesystem/clientLifecycle.test.ts @@ -248,7 +248,7 @@ describe("Grok Build server table edits", () => { expect(configured).toContain("share = false"); expect(configured).toContain("[mcp_servers.rea]"); expect(configured).toContain("startup_timeout_sec = 30"); - expect(configured).not.toContain('OLD = "1"'); + expect(configured).toContain('OLD = "1"'); expect(configured).not.toContain('command = "old"'); expect(parseToml(configured)).toMatchObject({ mcp_servers: { @@ -258,6 +258,7 @@ describe("Grok Build server table edits", () => { args: ["mcp"], startup_timeout_sec: 30, env: { + OLD: "1", JAVA_HOME: "/opt/jdk-21", HOPPER_LAUNCHER_PATH: "/Hopper Path", GHIDRA_INSTALL_DIR: "/opt/ghidra", @@ -507,7 +508,7 @@ describe("Grok Build text that resembles a server table", () => { const configured = await readFile(configPath, "utf8"); expect(configured).not.toContain("mcp_servers . rea"); expect(configured).not.toContain("\\u0072ea"); - expect(configured).not.toContain('OLD = "1"'); + expect(configured).toContain('OLD = "1"'); expect( configured .split("\n") @@ -515,7 +516,12 @@ describe("Grok Build text that resembles a server table", () => { ).toHaveLength(1); expect(parseToml(configured)).toMatchObject({ mcp_servers: { - rea: { command: "rea", args: ["mcp"], startup_timeout_sec: 30 }, + rea: { + command: "rea", + args: ["mcp"], + startup_timeout_sec: 30, + env: { OLD: "1" }, + }, }, }); });