diff --git a/src/application/ClientRegistration.ts b/src/application/ClientRegistration.ts index 0d9e4d6841..ec38c6d7c1 100644 --- a/src/application/ClientRegistration.ts +++ b/src/application/ClientRegistration.ts @@ -21,22 +21,36 @@ const tomlTimeoutSchema = z.union([ z.bigint().positive(), ]); -/** Common launcher facts, without assigning a shared meaning to client policy. */ -const registrationSchema = z +const launcherSchema = z .object({ command: z.string().min(1), args: z.array(z.string()).default([]), - startup_timeout_sec: tomlTimeoutSchema.optional(), - tool_timeout_sec: tomlTimeoutSchema.optional(), - startupTimeoutMs: z.number().positive().optional(), - toolTimeoutMs: z.number().positive().optional(), - type: z.string().optional(), - transport: z.string().optional(), - serverName: z.string().optional(), - disabled: z.boolean().optional(), - enabled: z.boolean().optional(), }) .passthrough(); +const opencodeLauncherSchema = z + .object({ + type: z.literal("local"), + command: z.array(z.string()).min(1), + }) + .passthrough(); +const normalizeOpencodeLauncher = ( + entry: z.output, +): z.output => { + const [command = "", ...args] = entry.command; + return { ...entry, command, args }; +}; + +const registrationSchema = launcherSchema.extend({ + startup_timeout_sec: tomlTimeoutSchema.optional(), + tool_timeout_sec: tomlTimeoutSchema.optional(), + startupTimeoutMs: z.number().positive().optional(), + toolTimeoutMs: z.number().positive().optional(), + type: z.string().optional(), + transport: z.string().optional(), + serverName: z.string().optional(), + disabled: z.boolean().optional(), + enabled: z.boolean().optional(), +}); export type ClientRegistration = z.output; @@ -76,6 +90,22 @@ const registrationError = (error: z.ZodError): ConfigurationError => })), }); +/** Launcher identity remains usable when unrelated readiness settings are invalid. */ +export const parseClientRegistrationLauncher = ( + value: unknown, + client: SetupClient, +): Result<{ command: string; args: string[] }, ConfigurationError> => { + const parsed = + client.format === "opencode" + ? opencodeLauncherSchema + .transform(normalizeOpencodeLauncher) + .safeParse(value) + : launcherSchema.safeParse(value); + return parsed.success + ? ok({ command: parsed.data.command, args: parsed.data.args }) + : err(registrationError(parsed.error)); +}; + /** Policy belongs to the client dialect, and is validated before refresh writes. */ const validateClientPolicy = ( value: unknown, @@ -109,17 +139,14 @@ export const parseClientRegistration = ( : err(registrationError(parsed.error)); } if (client.format === "opencode") { - const entry = z - .object({ - type: z.literal("local"), - command: z.array(z.string()).min(1), + const entry = opencodeLauncherSchema + .extend({ environment: z.record(z.string(), z.string()).optional(), }) - .passthrough() + .transform(normalizeOpencodeLauncher) .safeParse(value); if (!entry.success) return err(registrationError(entry.error)); - const [command = "", ...args] = entry.data.command; - value = { ...entry.data, command, args }; + value = entry.data; } const parsed = registrationSchema.safeParse(value); if (!parsed.success) return err(registrationError(parsed.error)); @@ -182,7 +209,7 @@ export const refreshClientRegistration = ( const existing = source.success ? source.data : {}; const policy = validateClientPolicy(existing, client); if (!policy.ok) return policy; - const prior = parseClientRegistration(existing, client); + const prior = parseClientRegistrationLauncher(existing, client); const priorCommand = prior.ok ? [prior.value.command, ...prior.value.args] : undefined; diff --git a/src/application/Uninstall.ts b/src/application/Uninstall.ts index a950313657..9db087b5d7 100644 --- a/src/application/Uninstall.ts +++ b/src/application/Uninstall.ts @@ -22,7 +22,7 @@ import { defaultKunaCacheDirectory } from "../config/kunaCacheDirectory.js"; import { linuxHopperReleaseCacheDirectory } from "./LinuxHopperCacheDirectory.js"; import { ClientConfigurationPublication } from "./ClientConfigurationPublication.js"; -import { z } from "zod"; +import { parseClientRegistrationLauncher } from "./ClientRegistration.js"; import { PRODUCT_IDENTITY } from "../identity.js"; import { @@ -734,21 +734,9 @@ const resolveUninstallConfigPath = async ( /** Ownership rule for persistent MCP registrations created by REA setup. */ const isOwnedRegistration = (value: unknown, client: SetupClient): boolean => { - if (client.format === "opencode") { - const parsed = z - .object({ type: z.literal("local"), command: z.array(z.string()).min(1) }) - .passthrough() - .safeParse(value); - if (!parsed.success) return false; - const command = parsed.data.command; - return ( - classifyClientRegistrationCommand(command, CURRENT_ENTRYPOINT_PATH) === - "owned" - ); - } - const parsed = registrationSchema.safeParse(value); - if (!parsed.success) return false; - const { command, args } = parsed.data; + const parsed = parseClientRegistrationLauncher(value, client); + if (!parsed.ok) return false; + const { command, args } = parsed.value; return ( classifyClientRegistrationCommand( [command, ...args], @@ -1183,8 +1171,5 @@ const uninstallStatusPriority = (status: UninstallItem["status"]): number => { } } }; -const registrationSchema = z - .object({ command: z.string(), args: z.array(z.string()) }) - .passthrough(); const isMissing = (cause: unknown): boolean => cause instanceof Error && "code" in cause && cause.code === "ENOENT"; diff --git a/tests/boundary/cli/cliSetup.test.ts b/tests/boundary/cli/cliSetup.test.ts index 15cea67f50..00e45c5b17 100644 --- a/tests/boundary/cli/cliSetup.test.ts +++ b/tests/boundary/cli/cliSetup.test.ts @@ -9,14 +9,16 @@ import { symlink, writeFile, } from "node:fs/promises"; -import { dirname, join } from "node:path"; +import { dirname, join, resolve } from "node:path"; import { promisify } from "node:util"; import { spawn } from "@lydell/node-pty"; import { describe, expect, it } from "vitest"; import { z } from "zod"; +import { parse as parseToml } from "smol-toml"; import { createTestTempDirectory } from "../../fixtures/temporaryDirectory.js"; +import { cliTest } from "../../support/cli/cliFixture.js"; const execute = promisify(execFile); const decisionMarker = "REA_SETUP_DECISION:"; @@ -176,6 +178,107 @@ describe("TTY setup dry run", () => { }); }); +cliTest.skipIf(process.platform === "win32")( + "repairs a Codex timeout without replacing an equivalent caller-selected launcher", + async ({ cli }) => { + const home = await createTestTempDirectory("rea-codex-alias-repair-"); + const codexHome = join(home, ".codex"); + const config = join(codexHome, "config.toml"); + const backup = `${config}.rea.backup`; + const alias = join(home, "rea-launcher-alias.mjs"); + await mkdir(codexHome); + await symlink(resolve("scripts/rea.mjs"), alias, "file"); + const original = [ + "# Caller-owned Codex approval policy", + 'approval_policy = "on-request"', + 'sandbox_mode = "read-only"', + "", + "[mcp_servers.rea]", + `command = ${JSON.stringify(process.execPath)}`, + `args = ${JSON.stringify([alias, "mcp"])}`, + "startup_timeout_sec = 0", + "tool_timeout_sec = 900", + 'default_tools_approval_mode = "prompt"', + "", + "[mcp_servers.rea.tools.open_binary]", + 'approval_mode = "approve"', + "output_token_limit = 2048", + "", + ].join("\n"); + await writeFile(config, original); + const setup = (approval: "--dry-run" | "--yes") => + cli.run({ + arguments: [ + "setup", + "--client", + "codex", + "--no-skill", + approval, + "--json", + ], + cwd: home, + environment: { + HOME: home, + USERPROFILE: home, + CODEX_HOME: codexHome, + XDG_CONFIG_HOME: join(home, ".config"), + XDG_DATA_HOME: join(home, ".local", "share"), + XDG_CACHE_HOME: join(home, ".cache"), + }, + timeoutMs: 20_000, + }); + + const planned = await setup("--dry-run"); + expect(planned.exitCode, planned.stderr).toBe(0); + expect(planned.json).toMatchObject({ + status: "planned", + plannedActions: [ + expect.objectContaining({ + id: "configure_client:codex", + operation: "update", + target: config, + backupPath: backup, + }), + ], + }); + expect(await readFile(config, "utf8")).toBe(original); + await expect(access(backup)).rejects.toMatchObject({ code: "ENOENT" }); + + const applied = await setup("--yes"); + expect(applied.exitCode, applied.stderr).toBe(0); + expect(applied.json).toMatchObject({ status: "ready" }); + const repaired = await readFile(config, "utf8"); + expect(parseToml(repaired)).toMatchObject({ + approval_policy: "on-request", + sandbox_mode: "read-only", + mcp_servers: { + rea: { + command: process.execPath, + args: [alias, "mcp"], + startup_timeout_sec: 30, + tool_timeout_sec: 900, + default_tools_approval_mode: "prompt", + tools: { + open_binary: { approval_mode: "approve", output_token_limit: 2048 }, + }, + }, + }, + }); + expect(await readFile(backup, "utf8")).toBe(original); + expect((await lstat(alias)).isSymbolicLink()).toBe(true); + + const repeated = await setup("--yes"); + expect(repeated.exitCode, repeated.stderr).toBe(0); + expect(repeated.json).toMatchObject({ + status: "ready", + plannedActions: [], + appliedActions: [], + }); + expect(await readFile(config, "utf8")).toBe(repaired); + expect(await readFile(backup, "utf8")).toBe(original); + }, +); + describe("interactive setup journey", () => { it("shows detected and absent clients without preselecting detected clients", async () => { const result = await runJourney( diff --git a/tests/boundary/filesystem/clientRegistrationOwnership.test.ts b/tests/boundary/filesystem/clientRegistrationOwnership.test.ts new file mode 100644 index 0000000000..efc85ad9aa --- /dev/null +++ b/tests/boundary/filesystem/clientRegistrationOwnership.test.ts @@ -0,0 +1,155 @@ +import { mkdir, readFile, readdir, writeFile } from "node:fs/promises"; +import { dirname, join } from "node:path"; + +import { parse as parseToml } from "smol-toml"; +import { expect, it } from "vitest"; + +import { readClientRegistrationStatuses } from "../../../src/application/ClientRegistrationStatus.js"; +import { systemUninstallHost } from "../../../src/application/Uninstall.js"; +import { PRODUCT_IDENTITY } from "../../../src/identity.js"; +import { createTestTempDirectory } from "../../fixtures/temporaryDirectory.js"; + +const codexSource = (invalid: string): string => + [ + 'caller_root = "keep"', + "[mcp_servers.rea]", + `command = "${PRODUCT_IDENTITY.cliBinary}"`, + 'args = ["mcp"]', + invalid, + "[mcp_servers.peer]", + 'command = "peer"', + "args = []", + "", + ].join("\n"); + +it.each([ + { + client: "codex", + path: ".codex/config.toml", + invalid: "invalid tool policy", + source: codexSource('enabled_tools = "not-an-array"'), + parse: (text: string): unknown => parseToml(text), + remaining: { + caller_root: "keep", + mcp_servers: { peer: { command: "peer", args: [] } }, + }, + }, + { + client: "codex", + path: ".codex/config.toml", + invalid: "a nonpositive startup timeout", + source: codexSource("startup_timeout_sec = 0"), + parse: (text: string): unknown => parseToml(text), + remaining: { + caller_root: "keep", + mcp_servers: { peer: { command: "peer", args: [] } }, + }, + }, + { + client: "opencode", + path: ".config/opencode/opencode.json", + invalid: "a non-string environment value", + source: JSON.stringify({ + caller_root: "keep", + mcp: { + servers: { + rea: { + type: "local", + command: [PRODUCT_IDENTITY.cliBinary, "mcp"], + environment: { CALLER_SETTING: 42 }, + }, + peer: { type: "local", command: ["peer"] }, + }, + }, + }), + parse: (text: string): unknown => JSON.parse(text), + remaining: { + caller_root: "keep", + mcp: { servers: { peer: { type: "local", command: ["peer"] } } }, + }, + }, +])( + "removes the owned $client registration despite $invalid", + async (sample) => { + const home = await createTestTempDirectory("rea-registration-ownership-"); + const configPath = join(home, sample.path); + const environment = { + CODEX_HOME: join(home, ".codex"), + XDG_CONFIG_HOME: join(home, ".config"), + }; + await mkdir(dirname(configPath), { recursive: true }); + await writeFile(configPath, sample.source); + const statuses = await readClientRegistrationStatuses(home, undefined, { + environment, + }); + expect( + statuses.find(({ client }) => client === sample.client), + ).toMatchObject({ + state: "invalid", + }); + const host = systemUninstallHost(home, undefined, environment); + try { + const client = (await host.clients()).find( + (client) => + client.name === sample.client && client.configPath === configPath, + ); + if (client === undefined) + throw new Error("Expected the configured client"); + expect(await host.removeClient(client)).toMatchObject({ + name: sample.client, + status: "removed", + }); + expect(sample.parse(await readFile(configPath, "utf8"))).toEqual( + sample.remaining, + ); + expect(await readFile(`${configPath}.rea.backup`, "utf8")).toBe( + sample.source, + ); + } finally { + expect(await host.close?.()).toBeUndefined(); + } + }, +); + +it("retains an OpenCode remote registration with a REA-like command", async () => { + const home = await createTestTempDirectory("rea-registration-remote-owner-"); + const configPath = join(home, ".config/opencode/opencode.json"); + const environment = { XDG_CONFIG_HOME: join(home, ".config") }; + const source = JSON.stringify({ + caller_root: "keep", + mcp: { + servers: { + rea: { + type: "remote", + url: "https://caller.example/mcp", + command: [PRODUCT_IDENTITY.cliBinary, "mcp"], + }, + peer: { type: "local", command: ["peer"] }, + }, + }, + }); + await mkdir(dirname(configPath), { recursive: true }); + await writeFile(configPath, source); + const statuses = await readClientRegistrationStatuses(home, undefined, { + environment, + }); + expect(statuses.find(({ client }) => client === "opencode")).toMatchObject({ + state: "invalid", + }); + const host = systemUninstallHost(home, undefined, environment); + try { + const client = (await host.clients()).find( + (client) => + client.name === "opencode" && client.configPath === configPath, + ); + if (client === undefined) throw new Error("Expected the configured client"); + expect(await host.removeClient(client)).toMatchObject({ + name: "opencode", + status: "retained", + }); + expect(await readFile(configPath, "utf8")).toBe(source); + expect(await readdir(dirname(configPath))).toEqual(["opencode.json"]); + } finally { + expect(await host.close?.()).toBeUndefined(); + } +}); diff --git a/tests/boundary/filesystem/setupConfig.test.ts b/tests/boundary/filesystem/setupConfig.test.ts index 91bbff9301..ea21eec1b3 100644 --- a/tests/boundary/filesystem/setupConfig.test.ts +++ b/tests/boundary/filesystem/setupConfig.test.ts @@ -9,6 +9,7 @@ import { } from "node:fs/promises"; import { join } from "node:path"; import { describe, expect, it } from "vitest"; +import { parse as parseToml } from "smol-toml"; import { createTestTempDirectory } from "../../fixtures/temporaryDirectory.js"; @@ -69,6 +70,57 @@ it.skipIf(process.platform === "win32")( }, ); +it.skipIf(process.platform === "win32")( + "repairs a Codex timeout without replacing an equivalent caller launcher alias", + async () => { + const home = await createTestTempDirectory( + "rea-config-timeout-launcher-alias-", + ); + const real = join(home, "rea-current"); + const alias = join(home, "rea-alias"); + await writeFile(real, "current launcher\n"); + await symlink(real, alias); + await mkdir(join(home, ".codex")); + const configPath = join(home, ".codex", "config.toml"); + const original = [ + "[mcp_servers.rea]", + `command = ${JSON.stringify(alias)}`, + 'args = ["mcp"]', + "startup_timeout_sec = 0", + "tool_timeout_sec = 450", + "future_policy = { retain = true }", + "", + "[mcp_servers.rea.env]", + 'CALLER_SETTING = "keep exactly"', + "", + ].join("\n"); + await writeFile(configPath, original); + const client = { name: "codex", format: "toml", configPath } as const; + expect( + await configureClientConfiguration(client, {}, [real, "mcp"]), + ).toMatchObject({ status: "configured" }); + const configured = await readFile(configPath, "utf8"); + expect(parseToml(configured)).toMatchObject({ + mcp_servers: { + rea: { + command: alias, + args: ["mcp"], + startup_timeout_sec: 30, + tool_timeout_sec: 450, + future_policy: { retain: true }, + env: { CALLER_SETTING: "keep exactly" }, + }, + }, + }); + expect(await readFile(`${configPath}.rea.backup`, "utf8")).toBe(original); + expect( + await configureClientConfiguration(client, {}, [real, "mcp"]), + ).toEqual({ status: "unchanged" }); + expect(await readFile(configPath, "utf8")).toBe(configured); + expect(await readFile(`${configPath}.rea.backup`, "utf8")).toBe(original); + }, +); + describe("JSON client configuration transaction: writing configuration through safe paths", () => { it.skipIf(typeof process.getuid !== "function")( "rejects a symlink target not owned by the current user",