diff --git a/docs/client-configuration.md b/docs/client-configuration.md index b7b3ca9d42..38af2e6d74 100644 --- a/docs/client-configuration.md +++ b/docs/client-configuration.md @@ -14,6 +14,12 @@ preserving your REA client entry. See the ## GitHub Copilot CLI +Setup preserves an existing `tools` selection in the REA registration, including +an empty list. New registrations default to `["*"]`. A restricted selection is +aligned configuration; doctor does not widen it to expose every REA tool. See +[Copilot's MCP configuration](https://docs.github.com/en/copilot/how-tos/copilot-cli/customize-copilot/add-mcp-servers) +for tool-selection settings. + For GitHub Copilot CLI 1.0.95, `gpt-4.1` model metadata can block ordinary chat with REA's full input-schema profile before sending a model request (`compaction_static_context_blocked`; [#1554](https://github.com/morluto/rea/issues/1554)). @@ -250,17 +256,29 @@ standalone CLI check. REA manages its launcher and selected provider settings in the client registration. Codex's server and per-tool approval policies are user choices. -Current limitation: doctor rejects nested `[mcp_servers.rea.tools.]` -approval tables as an invalid registration -([#3862](https://github.com/morluto/rea/issues/3862)), and setup refresh rebuilds -the REA entry without those tables or `default_tools_approval_mode` -([#3863](https://github.com/morluto/rea/issues/3863)). Preserving unrelated server -entries, valid environment overrides, and positive timeouts does not establish -preservation of these policy fields. Do not rerun setup solely to clear this -diagnostic. If a refresh is needed, back up the configuration and restore the -policy fields from that backup or the reported recovery snapshot before -restarting Codex. The per-tool table can still cause the doctor diagnostic -after restoration. +Setup patches REA's connection fields in the selected file and retains other +server settings, including `default_tools_approval_mode`, nested +`[mcp_servers.rea.tools.]` tables, tool allow/deny lists, and unrecognized +client settings. Doctor accepts Codex's per-tool policy tables. Known malformed +policy values block setup before it writes the configuration or a backup, and +the diagnostic identifies the setting to repair. See the +[Codex configuration reference](https://developers.openai.com/codex/config-reference/) +for the supported policy values. + +| Settings | Setup ownership | +| ----------------------------------------- | --------------------------------------------------------------------- | +| Launcher, transport and activation | Refreshes the REA stdio connection and enables it. | +| Explicit server environment | Preserves configured overrides and merges selected provider settings. | +| Codex positive timeouts and client policy | Retains caller choices; adds missing recommended REA timeouts. | +| Unrelated or future server settings | Retains their parsed values. | + +Dry-run applies the same policy validation and writes no files. A successful +refresh keeps an exact recovery snapshot of the original configuration; +repeating setup with the same settings performs no write. Uninstall removes +the owned REA entry, including its policy, while retaining other servers and +the recovery snapshot. A green registration check establishes known +configuration alignment; it does not prove that a particular client or fork +successfully exposes every REA tool. Codex treats an unset or empty `CODEX_HOME` as `~/.codex`. Its MCP server registration supports `startup_timeout_sec` for connection initialization and diff --git a/src/application/ClientRegistration.ts b/src/application/ClientRegistration.ts new file mode 100644 index 0000000000..b5b7fac068 --- /dev/null +++ b/src/application/ClientRegistration.ts @@ -0,0 +1,218 @@ +import { z } from "zod"; + +import { ConfigurationError } from "../domain/configurationErrors.js"; +import { err, ok, type Result } from "../domain/result.js"; +import { + MCP_STARTUP_POLICY, + recommendedMcpToolTimeoutMs, + recommendedMcpToolTimeoutSeconds, + type McpToolTimeoutInputs, +} from "../mcpStartupPolicy.js"; +import { + clientConfigurationRecordSchema, + clientRegistrationEntry, + type ClientRegistrationDialect, +} from "./ClientConfigurationDocument.js"; +import type { SetupClient } from "./SupportedClients.js"; + +const tomlTimeoutSchema = z.union([ + z.number().positive(), + z.bigint().positive(), +]); + +/** Common launcher facts, without assigning a shared meaning to client policy. */ +const registrationSchema = 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(); + +export type ClientRegistration = z.output; + +// https://developers.openai.com/codex/config-reference/#mcp_servers +const codexApprovalModeSchema = z.enum(["auto", "prompt", "writes", "approve"]); +const codexToolPolicySchema = z + .object({ + approval_mode: codexApprovalModeSchema.optional(), + output_token_limit: z + .union([z.number().int().positive(), z.bigint().positive()]) + .optional(), + }) + .passthrough(); +const codexPolicySchema = z + .object({ + default_tools_approval_mode: codexApprovalModeSchema.optional(), + enabled_tools: z.array(z.string()).optional(), + disabled_tools: z.array(z.string()).optional(), + tools: clientConfigurationRecordSchema + .superRefine((tools, context) => { + for (const [name, value] of Object.entries(tools)) { + const policy = codexToolPolicySchema.safeParse(value); + if (!policy.success) + for (const issue of policy.error.issues) + context.addIssue({ ...issue, path: [name, ...issue.path] }); + } + }) + .optional(), + }) + .passthrough(); + +const registrationError = (error: z.ZodError): ConfigurationError => + new ConfigurationError("Invalid client registration settings", { + settings: error.issues.map((issue) => ({ + setting: issue.path.join("."), + constraint: issue.message, + })), + }); + +/** Policy belongs to the client dialect, and is validated before refresh writes. */ +const validateClientPolicy = ( + value: unknown, + client: SetupClient, +): Result => { + const policy = + client.name === "codex" + ? codexPolicySchema.safeParse(value) + : client.format === "copilot_cli" + ? z.object({ tools: z.array(z.string()).optional() }).safeParse(value) + : undefined; + return policy === undefined || policy.success + ? ok(undefined) + : err(registrationError(policy.error)); +}; + +/** Normalize launcher facts once; policy fields keep their dialect-specific shape. */ +export const parseClientRegistration = ( + value: unknown, + client: SetupClient, +): Result => { + const policy = validateClientPolicy(value, client); + if (!policy.ok) return policy; + if (client.format === "dsh") { + const parsed = registrationSchema + .pick({ command: true, args: true, serverName: true, transport: true }) + .strip() + .safeParse(value); + return parsed.success + ? ok(parsed.data) + : err(registrationError(parsed.error)); + } + if (client.format === "opencode") { + const entry = z + .object({ + type: z.literal("local"), + command: z.array(z.string()).min(1), + environment: z.record(z.string(), z.string()).optional(), + }) + .passthrough() + .safeParse(value); + if (!entry.success) return err(registrationError(entry.error)); + const [command = "", ...args] = entry.data.command; + value = { ...entry.data, command, args }; + } + const parsed = registrationSchema.safeParse(value); + if (!parsed.success) return err(registrationError(parsed.error)); + const registration = parsed.data; + const transportInvalid = + ((client.format === "kimi" || client.format === "commandcode") && + registration.transport !== undefined && + registration.transport !== "stdio") || + (client.format === "commandcode" && registration.transport === undefined); + const typeInvalid = + ((client.format === "vscode" || client.format === "copilot_cli") && + registration.type !== "stdio") || + ((client.format === "kimi" || + client.format === "omp" || + client.format === "pi") && + registration.type !== undefined && + registration.type !== "stdio"); + if (transportInvalid || typeInvalid) + return err( + new ConfigurationError("Expected an stdio registration", { + settings: [ + { + setting: transportInvalid ? "transport" : "type", + constraint: "Expected stdio", + }, + ], + }), + ); + return ok(registration); +}; + +// These fields select REA's connection and activation. Everything else belongs +// to the caller/client, including future policy keys. Client-specific timeout +// defaults and preservation rules are applied below. +const managedConnectionKeys = new Set([ + "command", + "args", + "env", + "environment", + "type", + "transport", + "enabled", + "disabled", + "enable", + "serverName", + "url", + "httpUrl", +]); + +/** Patch a source fragment, retaining client-owned settings rather than rebuilding it. */ +export const refreshClientRegistration = ( + client: SetupClient, + dialect: ClientRegistrationDialect, + command: readonly string[], + environment: Readonly>, + previous: unknown, + timeoutInputs: McpToolTimeoutInputs, +): Result, ConfigurationError> => { + const source = clientConfigurationRecordSchema.safeParse(previous); + const existing = source.success ? source.data : {}; + const policy = validateClientPolicy(existing, client); + if (!policy.ok) return policy; + const registration = { + ...clientRegistrationEntry(dialect, command, environment), + ...Object.fromEntries( + Object.entries(existing).filter( + ([key]) => !managedConnectionKeys.has(key), + ), + ), + }; + if (client.name === "codex" || client.name === "grok_build") + registration.startup_timeout_sec = + (client.name === "codex" + ? positiveTimeout(existing.startup_timeout_sec) + : undefined) ?? MCP_STARTUP_POLICY.codexStartupTimeoutSeconds; + if (client.name === "codex") + registration.tool_timeout_sec = + positiveTimeout(existing.tool_timeout_sec) ?? + recommendedMcpToolTimeoutSeconds(timeoutInputs); + if (client.name === "kimi") { + registration.startupTimeoutMs = + positiveTimeout(existing.startupTimeoutMs) ?? + MCP_STARTUP_POLICY.kimiStartupTimeoutMs; + registration.toolTimeoutMs = + positiveTimeout(existing.toolTimeoutMs) ?? + recommendedMcpToolTimeoutMs(timeoutInputs); + } + return ok(registration); +}; + +const positiveTimeout = (value: unknown): number | bigint | undefined => { + // TOML keeps integers beyond the safe Number range exact as bigint. + if (typeof value === "bigint") return value > 0n ? value : undefined; + return typeof value === "number" && Number.isFinite(value) && value > 0 + ? value + : undefined; +}; diff --git a/src/application/ClientRegistrationStatus.ts b/src/application/ClientRegistrationStatus.ts index c11f5f585e..649bb27992 100644 --- a/src/application/ClientRegistrationStatus.ts +++ b/src/application/ClientRegistrationStatus.ts @@ -11,7 +11,6 @@ import { lstat } from "node:fs/promises"; import { err, ok, type Result } from "../domain/result.js"; import { isAbsolute } from "node:path"; -import { z } from "zod"; import { valid } from "semver"; import { PRODUCT_IDENTITY } from "../identity.js"; @@ -41,6 +40,11 @@ import { clientRegistrationEnvironmentDetailsWithBudget } from "./ClientRegistra import type { ProviderRequestBudgetResolver } from "./ClientRegistrationEnvironment.js"; import type { ArtifactResourceScope } from "../artifacts/ArtifactResourceScope.js"; +import { + parseClientRegistration, + type ClientRegistration, +} from "./ClientRegistration.js"; + interface ClientRegistrationStatusBase { readonly client: string; readonly config_path: string; @@ -81,29 +85,6 @@ export type UnhealthyClientRegistrationStatus = Exclude< { readonly state: "aligned" } >; -const tomlTimeoutSchema = z.union([ - z.number().positive(), - z.bigint().positive(), -]); - -const registrationSchema = z - .object({ - command: z.string().min(1), - args: z.array(z.string()).default([]), - // TOML keeps integers beyond the safe Number range exact as bigint. - 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(), - tools: z.array(z.string()).optional(), - disabled: z.boolean().optional(), - enabled: z.boolean().optional(), - }) - .passthrough(); - interface ClientRegistrationReadOptions { readonly platform?: NodeJS.Platform; readonly environment?: NodeJS.ProcessEnv; @@ -259,7 +240,18 @@ export const readClientRegistrationInspections = async ( format: "zcode_fallback", } : client; - const registration = parseRegistration(raw, effectiveClient); + const parsedRegistration = parseClientRegistration(raw, effectiveClient); + if (!parsedRegistration.ok) { + record({ + client: client.name, + config_path: configPath, + command: [], + state: "invalid", + remediation: `${parsedRegistration.error.message}: ${parsedRegistration.error.settings.map(({ setting, constraint }) => `${setting}: ${constraint}`).join("; ")}`, + }); + continue; + } + const registration = parsedRegistration.value; const registrationEnvironment = client.name === "kimi" || client.name === "codex" ? await clientRegistrationEnvironmentDetailsWithBudget( @@ -362,7 +354,7 @@ const ignoresDisabledKey = new Set(["codex", "kimi"]); const ignoresEnabledKey = new Set(["devin"]); const registrationActivation = ( - registration: z.output, + registration: ClientRegistration, client: SetupClient, parsed: ClientConfigurationDocument, ): "enabled" | "disabled" => @@ -385,7 +377,7 @@ const registrationAligned = ({ platform, kimiInputSchemaProfileSelected, }: { - readonly registration: z.output; + readonly registration: ClientRegistration; readonly client: SetupClient; readonly currentCommandPath: string | undefined; readonly platform: NodeJS.Platform; @@ -417,16 +409,6 @@ const registrationAligned = ({ ) return "stale"; if (client.name === "kimi" && !kimiInputSchemaProfileSelected) return "stale"; - if ( - (client.format === "kimi" || - client.format === "omp" || - client.format === "pi") && - registration.type !== undefined && - registration.type !== "stdio" - ) - return "stale"; - if (client.format === "vscode" && registration.type !== "stdio") - return "stale"; // A Harness registration only connects as the rea stdio server; another // server name or transport leaves the session without REA tools. if ( @@ -434,14 +416,6 @@ const registrationAligned = ({ (registration.serverName !== "rea" || registration.transport !== "stdio") ) return "stale"; - if ( - client.format === "copilot_cli" && - (registration.type !== "stdio" || - JSON.stringify(registration.tools) !== JSON.stringify(["*"])) - ) - return "stale"; - if (client.format === "commandcode" && registration.transport !== "stdio") - return "stale"; if ( (["npx", "bunx"] as const).some((runner) => { const expected = packageRunnerRegistrationCommand(runner, platform); @@ -459,56 +433,6 @@ const registrationAligned = ({ return isAbsolute(command[0] ?? "") ? "aligned" : "stale"; }; -const parseRegistration = ( - value: unknown, - client: SetupClient, -): z.output => { - if (client.format === "dsh") - return registrationSchema - .pick({ command: true, args: true, serverName: true, transport: true }) - .strip() - .parse(value); - if (client.format === "opencode") { - const entry = z - .object({ - type: z.literal("local"), - command: z.array(z.string()).min(1), - environment: z.record(z.string(), z.string()).optional(), - }) - .passthrough() - .parse(value); - const [command = "", ...args] = entry.command; - return registrationSchema.parse({ - ...entry, - command, - args, - }); - } - const registration = registrationSchema.parse(value); - if ( - client.format === "kimi" && - registration.transport !== undefined && - registration.transport !== "stdio" - ) - throw new TypeError("Expected an stdio registration"); - if ( - (client.format === "vscode" || client.format === "copilot_cli") && - registration.type !== "stdio" - ) - throw new TypeError("Expected an stdio registration"); - if (client.format === "commandcode" && registration.transport !== "stdio") - throw new TypeError("Expected an stdio registration"); - if ( - (client.format === "kimi" || - client.format === "omp" || - client.format === "pi") && - registration.type !== undefined && - registration.type !== "stdio" - ) - throw new TypeError("Expected an stdio registration"); - return registration; -}; - /** A client that rewrites its file from memory must be quit before setup. */ const remediation = (client: SetupClient): string => { if (client.quitBeforeSetup !== true) @@ -553,7 +477,7 @@ const configuredStatus = ( const timeoutAdvisory = ( client: SetupClient, - registration: z.output, + registration: ClientRegistration, timeoutInputs: McpToolTimeoutInputs, ): string | undefined => { const toolTimeout = diff --git a/src/application/SetupClientConfiguration.ts b/src/application/SetupClientConfiguration.ts index 7408f32901..a1c21d67de 100644 --- a/src/application/SetupClientConfiguration.ts +++ b/src/application/SetupClientConfiguration.ts @@ -8,7 +8,6 @@ import { import { err, ok, type Result } from "../domain/result.js"; import { ConfigurationError } from "../domain/configurationErrors.js"; import { - clientRegistrationEntry, clientConfigurationValuesEqual, clientServerListedDisabled, clientServerPath, @@ -33,14 +32,11 @@ import { type ProviderRequestBudgetResolver, } from "./ClientRegistrationEnvironment.js"; +import { refreshClientRegistration } from "./ClientRegistration.js"; + import { PRODUCT_IDENTITY } from "../identity.js"; import { snapshotEnvironment } from "../process/snapshotEnvironment.js"; import { npxRegistrationCommand } from "./ClientRegistrationIdentity.js"; -import { - MCP_STARTUP_POLICY, - recommendedMcpToolTimeoutMs, - recommendedMcpToolTimeoutSeconds, -} from "../mcpStartupPolicy.js"; import { resolveClientConfigTransactionPath } from "./ClientConfigPath.js"; import { packageReadTargetChanged } from "./MsixPackageStorage.js"; import { RegularFileCleanupFailure } from "../filesystem/OwnedRegularFile.js"; @@ -100,14 +96,20 @@ export const configureClientConfiguration = async ( ); if (transactionPath === undefined) return { status: "failed", reason: "path" }; - return withClientConfigurationTransaction(transactionPath, () => - configureClientDocument( - { client, transactionPath }, - environment, - command, - resolveProviderRequestBudget, - ), - ); + try { + return await withClientConfigurationTransaction(transactionPath, () => + configureClientDocument( + { client, transactionPath }, + environment, + command, + resolveProviderRequestBudget, + ), + ); + } catch (cause: unknown) { + if (cause instanceof ConfigurationError) + return { status: "failed", reason: "readback" }; + throw cause; + } }; const configureClientDocument = async ( @@ -452,12 +454,21 @@ export const inspectClientConfiguration = async ( command: readonly string[], resolveProviderRequestBudget?: ProviderRequestBudgetResolver, ): Promise => { - const inspection = await inspectClientDocument( - client, - providerEnvironment, - command, - resolveProviderRequestBudget, - ); + let inspection: ClientConfigurationInspection; + try { + inspection = await inspectClientDocument( + client, + providerEnvironment, + command, + resolveProviderRequestBudget, + ); + } catch (cause: unknown) { + if (!(cause instanceof ConfigurationError)) throw cause; + return { + status: "invalid", + remediation: `${cause.message}: ${cause.settings.map(({ setting, constraint }) => `${setting}: ${constraint}`).join("; ")}`, + }; + } if (inspection.status !== "create" && inspection.status !== "update") return inspection; // A change the running client would overwrite is not a persisted registration. @@ -785,59 +796,14 @@ const clientConfigurationDesired = async ( REA_MCP_INPUT_SCHEMA_PROFILE: "compact", } : environment.value; - const registration = clientRegistrationEntry( + const registration = refreshClientRegistration( + client, format, launcher, registrationEnvironment, + parsed?.servers[PRODUCT_IDENTITY.mcpServerKey], + validated.value.timeoutInputs, ); - const previous = parsed?.servers[PRODUCT_IDENTITY.mcpServerKey]; - if ( - client.name === "kimi" && - typeof previous === "object" && - previous !== null && - "enabledTools" in previous - ) - registration.enabledTools = previous.enabledTools; - return { - ...registration, - ...(client.name === "codex" || client.name === "grok_build" - ? { - startup_timeout_sec: - client.name === "codex" - ? (positiveTimeout(previous, "startup_timeout_sec") ?? - MCP_STARTUP_POLICY.codexStartupTimeoutSeconds) - : MCP_STARTUP_POLICY.codexStartupTimeoutSeconds, - } - : {}), - ...(client.name === "codex" - ? { - tool_timeout_sec: - positiveTimeout(previous, "tool_timeout_sec") ?? - recommendedMcpToolTimeoutSeconds(validated.value.timeoutInputs), - } - : {}), - ...(client.name === "kimi" - ? { - startupTimeoutMs: - positiveTimeout(previous, "startupTimeoutMs") ?? - MCP_STARTUP_POLICY.kimiStartupTimeoutMs, - toolTimeoutMs: - positiveTimeout(previous, "toolTimeoutMs") ?? - recommendedMcpToolTimeoutMs(validated.value.timeoutInputs), - } - : {}), - }; -}; - -const positiveTimeout = ( - registration: unknown, - key: string, -): number | bigint | undefined => { - if (typeof registration !== "object" || registration === null) return; - const value: unknown = Reflect.get(registration, key); - // TOML keeps integers beyond the safe Number range exact as bigint. - if (typeof value === "bigint") return value > 0n ? value : undefined; - return typeof value === "number" && Number.isFinite(value) && value > 0 - ? value - : undefined; + if (!registration.ok) throw registration.error; + return registration.value; }; diff --git a/tests/acceptance/setup/codexToolPolicySetup.test.ts b/tests/acceptance/setup/codexToolPolicySetup.test.ts new file mode 100644 index 0000000000..2fe7512f90 --- /dev/null +++ b/tests/acceptance/setup/codexToolPolicySetup.test.ts @@ -0,0 +1,104 @@ +import { mkdir, readFile, readdir, writeFile } from "node:fs/promises"; +import { dirname, join } from "node:path"; +import { parse as parseToml } from "smol-toml"; +import { expect } from "vitest"; + +import { + codexToolPolicy, + codexToolPolicySource, +} from "../../fixtures/codexToolPolicy.js"; +import { createTestTempDirectory } from "../../fixtures/temporaryDirectory.js"; +import { cliTest } from "../../support/cli/cliFixture.js"; + +cliTest( + "Codex policy survives setup preview, refresh, doctor, repeat and uninstall", + async ({ cli }) => { + const home = await createTestTempDirectory("rea-codex-tool-policy-"); + const config = join(home, ".codex/config.toml"); + const original = codexToolPolicySource("tables"); + await mkdir(dirname(config), { recursive: true }); + await writeFile(config, original); + const run = (arguments_: readonly string[]) => + cli.run({ + arguments: arguments_, + cwd: home, + environment: { + HOME: home, + USERPROFILE: home, + CODEX_HOME: dirname(config), + XDG_CONFIG_HOME: join(home, ".config"), + REA_GHIDRA_STARTUP_TIMEOUT_MS: "17000", + }, + }); + const doctorArguments = [ + "doctor", + "--client", + "codex", + "--skill=false", + "--json", + ]; + const assertAligned = (value: unknown) => + expect(value).toMatchObject({ + scope_checks: expect.arrayContaining([ + expect.objectContaining({ name: "registration:codex", ok: true }), + ]), + }); + const setupArguments = [ + "setup", + "--client", + "codex", + "--skill=false", + "--json", + ]; + const preview = await run([...setupArguments, "--dry-run"]); + expect(preview.exitCode, preview.stdout).toBe(0); + expect(preview.json).toMatchObject({ status: "planned" }); + expect(await readFile(config, "utf8")).toBe(original); + expect(await readdir(dirname(config))).toEqual(["config.toml"]); + + const applied = await run([...setupArguments, "--yes"]); + expect(applied.exitCode, applied.stdout).toBe(0); + expect(applied.json).toMatchObject({ + status: "ready", + clients: { codex: { status: "configured" } }, + }); + const configured = await readFile(config, "utf8"); + expect(configured).toContain("# retained root comment"); + expect(configured).toContain("# caller approval"); + expect( + parseToml(configured, { integersAsBigInt: "asNeeded" }), + ).toMatchObject({ + model: "caller-model", + mcp_servers: { + other: { command: "other" }, + rea: { + ...codexToolPolicy, + startup_timeout_sec: 19, + tool_timeout_sec: 999, + env: { + CALLER_SETTING: "kept exactly", + REA_GHIDRA_STARTUP_TIMEOUT_MS: "17000", + }, + }, + }, + }); + expect(await readFile(`${config}.rea.backup`, "utf8")).toBe(original); + assertAligned((await run(doctorArguments)).json); + const repeated = await run([...setupArguments, "--yes"]); + expect(repeated.exitCode, repeated.stdout).toBe(0); + expect(repeated.json).toMatchObject({ + status: "ready", + appliedActions: [], + }); + expect(await readFile(config, "utf8")).toBe(configured); + expect(await readFile(`${config}.rea.backup`, "utf8")).toBe(original); + + const removed = await run(["uninstall", "--json"]); + expect(removed.exitCode, removed.stdout).toBe(0); + expect(parseToml(await readFile(config, "utf8"))).toEqual({ + model: "caller-model", + mcp_servers: { other: { command: "other" } }, + }); + expect(await readFile(`${config}.rea.backup`, "utf8")).toBe(original); + }, +); diff --git a/tests/boundary/filesystem/clientRegistrationStatus.test.ts b/tests/boundary/filesystem/clientRegistrationStatus.test.ts index 348e4641e3..cd4c9d0001 100644 --- a/tests/boundary/filesystem/clientRegistrationStatus.test.ts +++ b/tests/boundary/filesystem/clientRegistrationStatus.test.ts @@ -482,7 +482,7 @@ describe("Node-wrapped registration policy", () => { ]); }); - it.each([{ tools: ["binary_session"] }, { tools: ["*"] }])( + it.each([{ tools: ["binary_session"] }, { tools: ["*"] }, { tools: [] }])( "checks Copilot tool selection $tools independently of launcher", async ({ tools }) => { const home = await createTestTempDirectory("rea-node-copilot-"); @@ -507,7 +507,7 @@ describe("Node-wrapped registration policy", () => { expect(statuses).toEqual([ expect.objectContaining({ client: "copilot_cli", - state: tools.includes("*") ? "aligned" : "stale", + state: "aligned", }), ]); }, diff --git a/tests/boundary/filesystem/codexClientPolicy.test.ts b/tests/boundary/filesystem/codexClientPolicy.test.ts new file mode 100644 index 0000000000..982db18549 --- /dev/null +++ b/tests/boundary/filesystem/codexClientPolicy.test.ts @@ -0,0 +1,183 @@ +import { mkdir, readFile, readdir, writeFile } from "node:fs/promises"; +import { dirname, join } from "node:path"; +import { expect, it } from "vitest"; +import { parse as parseToml } from "smol-toml"; + +import { readClientRegistrationStatuses } from "../../../src/application/ClientRegistrationStatus.js"; +import { + configureClientConfiguration, + inspectClientConfiguration, +} from "../../../src/application/SetupClientConfiguration.js"; +import { npxRegistrationCommand } from "../../../src/application/ClientRegistrationIdentity.js"; +import { PRODUCT_IDENTITY } from "../../../src/identity.js"; +import { createTestTempDirectory } from "../../fixtures/temporaryDirectory.js"; +import { + codexToolPolicy, + codexToolPolicySource, +} from "../../fixtures/codexToolPolicy.js"; + +it.each(["tables", "inline table"] as const)( + "preserves Codex %s policy and status through refresh", + async (layout) => { + const home = await createTestTempDirectory("rea-codex-policy-refresh-"); + const configPath = join(home, ".codex/config.toml"); + await mkdir(dirname(configPath)); + const original = codexToolPolicySource(layout); + await writeFile(configPath, original); + const client = { name: "codex", configPath, format: "toml" } as const; + const status = async () => + ( + await readClientRegistrationStatuses(home, undefined, { + environment: {}, + }) + ).find(({ client }) => client === "codex"); + expect(await status()).toMatchObject({ state: "aligned" }); + expect( + await inspectClientConfiguration(client, {}, npxRegistrationCommand()), + ).toEqual({ status: "already_current" }); + const overrides = { REA_MCP_INPUT_SCHEMA_PROFILE: "full" }; + expect( + await configureClientConfiguration( + client, + overrides, + npxRegistrationCommand(), + ), + ).toMatchObject({ status: "configured" }); + const configured = await readFile(configPath, "utf8"); + expect( + parseToml(configured, { integersAsBigInt: "asNeeded" }), + ).toMatchObject({ + mcp_servers: { + rea: { + ...codexToolPolicy, + startup_timeout_sec: 19, + tool_timeout_sec: 999, + env: { CALLER_SETTING: "kept exactly", ...overrides }, + }, + }, + }); + expect(await status()).toMatchObject({ state: "aligned" }); + expect( + await configureClientConfiguration( + client, + overrides, + npxRegistrationCommand(), + ), + ).toEqual({ status: "unchanged" }); + expect(await readFile(configPath, "utf8")).toBe(configured); + expect(await readFile(`${configPath}.rea.backup`, "utf8")).toBe(original); + }, +); + +it.each([ + ['tools = ["binary_session"]', "tools"], + ["tools = { binary_session = true }", "tools.binary_session"], + [ + 'tools = { binary_session = { approval_mode = "unknown" } }', + "tools.binary_session.approval_mode", + ], + [ + "tools = { binary_session = { output_token_limit = -1 } }", + "tools.binary_session.output_token_limit", + ], + ['default_tools_approval_mode = "unknown"', "default_tools_approval_mode"], + ["enabled_tools = true", "enabled_tools"], +] as const)( + "blocks malformed Codex policy %s before writes", + async (setting, field) => { + const home = await createTestTempDirectory("rea-codex-invalid-policy-"); + const configPath = join(home, ".codex/config.toml"); + await mkdir(dirname(configPath)); + const original = [ + "[mcp_servers.rea]", + 'command = "npx"', + `args = ["-y", "${PRODUCT_IDENTITY.registrationPackageSpecifier}", "mcp"]`, + "startup_timeout_sec = 30", + "tool_timeout_sec = 450", + setting, + "", + ].join("\n"); + await writeFile(configPath, original); + const client = { name: "codex", configPath, format: "toml" } as const; + const inspection = await inspectClientConfiguration( + client, + {}, + npxRegistrationCommand(), + ); + expect(inspection).toMatchObject({ + status: "invalid", + remediation: expect.stringContaining(field), + }); + expect( + await configureClientConfiguration(client, {}, npxRegistrationCommand()), + ).toEqual({ status: "failed", reason: "readback" }); + const statuses = await readClientRegistrationStatuses(home, undefined, { + environment: {}, + }); + expect(statuses.find(({ client }) => client === "codex")).toMatchObject({ + state: "invalid", + remediation: expect.stringContaining(field), + }); + expect(await readFile(configPath, "utf8")).toBe(original); + expect(await readdir(dirname(configPath))).toEqual(["config.toml"]); + }, +); + +it.each([{ tools: ["binary_session"] }, { tools: ["*"] }, { tools: [] }])( + "preserves Copilot's tool selection $tools through refresh", + async ({ tools }) => { + const home = await createTestTempDirectory("rea-copilot-tools-policy-"); + const configPath = join(home, ".copilot/mcp-config.json"); + await mkdir(dirname(configPath)); + const original = JSON.stringify({ + mcpServers: { + rea: { + command: "npx", + args: ["-y", PRODUCT_IDENTITY.registrationPackageSpecifier, "mcp"], + type: "stdio", + tools, + future_policy: { requireReview: true }, + }, + }, + }); + await writeFile(configPath, original); + const client = { + name: "copilot_cli", + configPath, + format: "copilot_cli", + } as const; + const status = async () => + ( + await readClientRegistrationStatuses(home, undefined, { + environment: {}, + }) + ).find(({ client }) => client === "copilot_cli"); + expect(await status()).toMatchObject({ state: "aligned" }); + const overrides = { REA_MCP_INPUT_SCHEMA_PROFILE: "compact" }; + expect( + await configureClientConfiguration( + client, + overrides, + npxRegistrationCommand(), + ), + ).toMatchObject({ status: "configured" }); + expect(JSON.parse(await readFile(configPath, "utf8"))).toMatchObject({ + mcpServers: { + rea: { + type: "stdio", + tools, + future_policy: { requireReview: true }, + }, + }, + }); + expect( + await configureClientConfiguration( + client, + overrides, + npxRegistrationCommand(), + ), + ).toEqual({ status: "unchanged" }); + expect(await status()).toMatchObject({ state: "aligned" }); + expect(await readFile(`${configPath}.rea.backup`, "utf8")).toBe(original); + }, +); diff --git a/tests/fixtures/codexToolPolicy.ts b/tests/fixtures/codexToolPolicy.ts new file mode 100644 index 0000000000..e62d320856 --- /dev/null +++ b/tests/fixtures/codexToolPolicy.ts @@ -0,0 +1,63 @@ +import { npxRegistrationCommand } from "../../src/application/ClientRegistrationIdentity.js"; + +export const codexToolPolicy = { + default_tools_approval_mode: "prompt", + enabled_tools: ["binary_session", "inspect_binary"], + disabled_tools: ["run_script"], + required: true, + cwd: "/caller/working-directory", + future_policy: { label: "retain this client setting" }, + tools: { + binary_session: { + approval_mode: "approve", + output_token_limit: 9007199254740993n, + }, + "inspect.binary": { approval_mode: "auto" }, + "run/script": { approval_mode: "writes" }, + ["__proto__"]: { approval_mode: "prompt", future_option: true }, + }, +}; + +/** The same client policy in native table and inline-table representations. */ +export const codexToolPolicySource = ( + layout: "tables" | "inline table", +): string => { + const [command, ...args] = npxRegistrationCommand(); + return layout === "tables" + ? [ + "# retained root comment", + 'model = "caller-model"', + "[mcp_servers.other]", + 'command = "other"', + "[mcp_servers.rea]", + `command = ${JSON.stringify(command)}`, + `args = ${JSON.stringify(args)}`, + "startup_timeout_sec = 19", + "tool_timeout_sec = 999", + 'default_tools_approval_mode = "prompt"', + 'enabled_tools = ["binary_session", "inspect_binary"]', + 'disabled_tools = ["run_script"]', + "required = true", + 'cwd = "/caller/working-directory"', + 'future_policy = { label = "retain this client setting" }', + "[mcp_servers.rea.env]", + 'CALLER_SETTING = "kept exactly"', + "[mcp_servers.rea.tools.binary_session]", + 'approval_mode = "approve" # caller approval', + "output_token_limit = 9007199254740993", + '[mcp_servers.rea.tools."inspect.binary"]', + 'approval_mode = "auto"', + '[mcp_servers.rea.tools."run/script"]', + 'approval_mode = "writes"', + "[mcp_servers.rea.tools.__proto__]", + 'approval_mode = "prompt"', + "future_option = true", + "", + ].join("\n") + : [ + "# retained root comment", + 'model = "caller-model"', + `mcp_servers = { other = { command = "other" }, rea = { command = ${JSON.stringify(command)}, args = ${JSON.stringify(args)}, startup_timeout_sec = 19, tool_timeout_sec = 999, default_tools_approval_mode = "prompt", enabled_tools = ["binary_session", "inspect_binary"], disabled_tools = ["run_script"], required = true, cwd = "/caller/working-directory", future_policy = { label = "retain this client setting" }, env = { CALLER_SETTING = "kept exactly" }, tools = { binary_session = { approval_mode = "approve", output_token_limit = 9007199254740993 }, "inspect.binary" = { approval_mode = "auto" }, "run/script" = { approval_mode = "writes" }, __proto__ = { approval_mode = "prompt", future_option = true } } } }`, + "", + ].join("\n"); +};