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

@@ -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

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);
 };

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<Record<string, string>> = {},
+): Result<Readonly<Record<string, string>>, 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),
+      ),
+    ),
+  );
+};

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,

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."

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", "[email protected]", "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 }) => {

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",
+    });
+  },
+);

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" },
+        },
       },
     });
   });