diff --git a/src/application/ClientConfigurationDocument.shape.test.ts b/src/application/ClientConfigurationDocument.shape.test.ts index c975b75078..6e63867dd9 100644 --- a/src/application/ClientConfigurationDocument.shape.test.ts +++ b/src/application/ClientConfigurationDocument.shape.test.ts @@ -2,6 +2,7 @@ import { describe, expect, it } from "vitest"; import { ClientConfigurationShapeError, + clientConfigurationSyntaxName, parseClientConfiguration, shadowedClientServers, } from "./ClientConfigurationDocument.js"; @@ -27,6 +28,44 @@ describe("parseClientConfiguration shape diagnostics", () => { expect(error.message).toContain("object"); }); + // The diagnostic syntax name must describe what the reader actually admits. + it.each([ + ["json", "JSONC"], + ["vscode", "JSONC"], + ["opencode", "JSONC"], + ["kilo", "JSONC"], + ["zed", "JSONC"], + ["amp", "JSONC"], + ["copilot_cli", "JSONC"], + ["fx", "JSONC"], + ["claude_code", "JSON"], + ["pi", "JSON"], + ["kimi", "JSON"], + ["lm_studio", "JSON"], + ["zcode", "JSON"], + ["toml", "TOML"], + ["grok", "TOML"], + ["goose", "YAML"], + ["hermes", "YAML"], + ] as const)("names %s configuration syntax %s", (format, name) => { + expect(clientConfigurationSyntaxName(format)).toBe(name); + const accepts = (text: string): boolean => { + try { + parseClientConfiguration(text, format); + return true; + } catch { + return false; + } + }; + if (name === "TOML") expect(accepts("keep = 1\n")).toBe(true); + if (name === "YAML") expect(accepts("keep: 1\n")).toBe(true); + if (name === "JSON" || name === "JSONC") { + expect(accepts('{"keep":1}')).toBe(true); + expect(accepts('{"keep":1,}')).toBe(name === "JSONC"); + expect(accepts('{"keep":1 /* comment */}')).toBe(name === "JSONC"); + } + }); + it("still rejects unparsable JSON with SyntaxError", () => { expect(() => parseClientConfiguration("not-json", "json")).toThrow( SyntaxError, diff --git a/src/application/ClientConfigurationDocument.ts b/src/application/ClientConfigurationDocument.ts index 487c07d905..e3101b5280 100644 --- a/src/application/ClientConfigurationDocument.ts +++ b/src/application/ClientConfigurationDocument.ts @@ -172,57 +172,146 @@ const parseClientToml = (text: string): Record => { return parsed; }; -const parseDocument = ( - text: string, - format: ClientConfigurationFormat, -): Record => { - if (format === "toml" || format === "grok") - return requireObject(parseClientToml(text), "The configuration root"); +/** + * How a client's own loader reads its configuration file: the syntax and the + * strictness, byte-order-mark and empty-file rules REA must match so it never + * accepts or writes a document the client rejects. This is the only owner of + * those facts; the registration dialect (server table, entry shape) is separate. + */ +type ClientConfigurationReader = + | { readonly syntax: "toml" } + | { + readonly syntax: "yaml"; + readonly parse?: (text: string) => unknown; + } + | { + readonly syntax: "json"; + /** What the client's loader accepts: strict `JSON.parse` or JSONC. */ + readonly native: "json" | "jsonc"; + /** + * REA admits JSONC for a strict client only when it rewrites the whole + * file as strict JSON before the client reads it again. + */ + readonly repairsJsonc?: true; + /** Whether a leading UTF-8 byte-order mark is accepted. */ + readonly bom: "accept" | "reject"; + /** An empty (blank or BOM-only) file holds no settings, or is invalid. */ + readonly emptyFile: "no-settings" | "reject"; + }; + +const JSONC_READER = { + syntax: "json", + native: "jsonc", + bom: "accept", + emptyFile: "no-settings", +} as const satisfies ClientConfigurationReader; +const STRICT_JSON_REPAIRED_READER = { + syntax: "json", + native: "json", + repairsJsonc: true, + bom: "accept", + emptyFile: "no-settings", +} as const satisfies ClientConfigurationReader; +/** Strict `JSON.parse` without BOM tolerance; an empty file holds no settings. */ +const STRICT_JSON_READER = { + syntax: "json", + native: "json", + bom: "reject", + emptyFile: "no-settings", +} as const satisfies ClientConfigurationReader; + +const CLIENT_CONFIGURATION_READERS = { + json: JSONC_READER, + vscode: JSONC_READER, + opencode: JSONC_READER, + commandcode: JSONC_READER, + omp: JSONC_READER, + amp: JSONC_READER, + zed: JSONC_READER, + kilo: JSONC_READER, + // Copilot CLI and fx load strict JSON; admitted JSONC is rewritten. + copilot_cli: STRICT_JSON_REPAIRED_READER, + copilot_cli_project: STRICT_JSON_REPAIRED_READER, + fx: STRICT_JSON_REPAIRED_READER, + // Claude Code loads `.claude.json` with strict JSON.parse after a UTF-8 BOM, + // so comments and trailing commas it rejects must not be admitted as JSONC. + claude_code: { ...STRICT_JSON_READER, bom: "accept" }, + // Kimi, LM Studio and ZCode read their files with JSON.parse; an empty + // file holds nothing to preserve. + kimi: STRICT_JSON_READER, + lm_studio: STRICT_JSON_READER, + zcode: STRICT_JSON_READER, // Pi loads `mcp.json` with a strict `JSON.parse` and no BOM tolerance, so // comments, trailing commas, a BOM, or an empty existing file must be // rejected instead of registering into a document the client cannot load. - // Kimi and LM Studio also read mcp.json with JSON.parse; an empty file holds - // nothing to preserve. - if (format === "pi" || format === "kimi" || format === "lm_studio") { - if (format !== "pi" && text.trim().length === 0) return {}; - return requireObject(JSON.parse(text), "The configuration root"); - } + pi: { ...STRICT_JSON_READER, emptyFile: "reject" }, + zcode_fallback: { ...STRICT_JSON_READER, emptyFile: "reject" }, + toml: { syntax: "toml" }, + grok: { syntax: "toml" }, + goose: { syntax: "yaml", parse: parseGooseConfigurationYaml }, + // Hermes stores MCP servers in `mcp_servers` inside a YAML document. + hermes: { syntax: "yaml", parse: parseHermesConfigurationYaml }, + // DeepSeek Harness reads a Cordis patch list; its own parser owns the shape. + dsh: { syntax: "yaml" }, +} as const satisfies Record< + Exclude, + ClientConfigurationReader +>; + +/** The client loader's reader for one configuration format. */ +const clientConfigurationReader = ( + format: Exclude, +): ClientConfigurationReader => CLIENT_CONFIGURATION_READERS[format]; + +/** The syntax name REA's reader admits for a format, for diagnostics. */ +export const clientConfigurationSyntaxName = ( + format: Exclude, +): "JSON" | "JSONC" | "TOML" | "YAML" => { + const reader = clientConfigurationReader(format); + if (reader.syntax === "toml") return "TOML"; + if (reader.syntax === "yaml") return "YAML"; + return reader.native === "json" && reader.repairsJsonc !== true + ? "JSON" + : "JSONC"; +}; + +const parseDocument = ( + text: string, + format: Exclude, + root = "The configuration root", +): Record => { + const reader = clientConfigurationReader(format); + if (reader.syntax === "toml") + return requireObject(parseClientToml(text), root); // An empty file, which some clients create before any server is added, // holds no settings to preserve. - if (isEmptyClientConfigurationText(text)) return {}; - // Hermes stores MCP servers in `mcp_servers` inside a YAML document. - if (format === "goose") + if ( + (reader.syntax === "yaml" || reader.emptyFile === "no-settings") && + isEmptyClientConfigurationText(text) + ) + return {}; + if (reader.syntax === "yaml") { + if (reader.parse === undefined) + throw new TypeError(`${format} configuration has a dedicated parser`); + return requireObject(reader.parse(text), root); + } + if (reader.native === "json" && reader.repairsJsonc !== true) return requireObject( - parseGooseConfigurationYaml(text), - "The configuration root", - ); - if (format === "hermes") - return requireObject( - parseHermesConfigurationYaml(text), - "The configuration root", - ); - // ZCode loads nonempty native files with strict JSON.parse. - if (format === "zcode") - return requireObject(JSON.parse(text), "The configuration root"); - // Claude Code loads `.claude.json` with strict JSON.parse after a UTF-8 BOM, - // so comments and trailing commas it rejects must not be admitted as JSONC. - if (format === "claude_code") - return requireObject( - JSON.parse(text.replace(/^\uFEFF/, "")), - "The configuration root", + JSON.parse(reader.bom === "accept" ? text.replace(/^\uFEFF/, "") : text), + root, ); const errors: ParseError[] = []; // Accept a UTF-8 BOM without shifting diagnostics or editing the original text. const jsonText = text.startsWith("\uFEFF") ? ` ${text.slice(1)}` : text; - const root = parseTree(jsonText, errors, { allowTrailingComma: true }); + const tree = parseTree(jsonText, errors, { allowTrailingComma: true }); const firstError = errors[0]; if (firstError !== undefined) throw new SyntaxError( `Invalid JSON/JSONC at offset ${firstError.offset}: ${printParseErrorCode(firstError.error)}`, ); return requireObject( - root === undefined ? undefined : getNodeValue(root), - "The configuration root", + tree === undefined ? undefined : getNodeValue(tree), + root, ); }; @@ -245,7 +334,7 @@ const requireObject = ( export const parseZcodeFallbackConfiguration = ( text: string, ): ClientConfigurationDocument => { - const document = requireObject(JSON.parse(text), "ZCode fallback root"); + const document = parseDocument(text, "zcode_fallback", "ZCode fallback root"); return { document, servers: @@ -294,10 +383,10 @@ export const parseClientConfiguration = ( if (format === "zcode_fallback") return parseZcodeFallbackConfiguration(text); if (format === "dsh") return parseDshCordisDocument(text); const document = parseDocument(text, format); + const reader = clientConfigurationReader(format); const requiresRewrite: true | undefined = - (format === "copilot_cli_project" || - format === "copilot_cli" || - format === "fx") && + reader.syntax === "json" && + reader.repairsJsonc === true && strictJsonNeedsRewrite(text) ? true : undefined; @@ -1927,10 +2016,9 @@ export const serializeClientConfiguration = ( if (!edited.ok) return edited; text = edited.value; } + const reader = clientConfigurationReader(format); return ok( - format === "copilot_cli_project" || - format === "copilot_cli" || - format === "fx" + reader.syntax === "json" && reader.repairsJsonc === true ? strictJsonSyntax(text) : text, ); diff --git a/src/application/Uninstall.ts b/src/application/Uninstall.ts index b128764c1e..5392390021 100644 --- a/src/application/Uninstall.ts +++ b/src/application/Uninstall.ts @@ -6,6 +6,7 @@ import { import { ClientConfigurationShapeError, clientConfigurationServersKey, + clientConfigurationSyntaxName, parseClientConfiguration, serializeClientConfiguration, clientServerPath, @@ -888,7 +889,8 @@ const readClientConfiguration = async ( ); if (client.configPathError !== undefined) return configurationFailure(client.configPathError); - if (client.format === "unsupported") + const { format } = client; + if (format === "unsupported") return itemRead( item( client.name, @@ -910,14 +912,16 @@ const readClientConfiguration = async ( }); if (typeof original !== "string") return original; try { - const parsed = parseClientConfiguration(original, client.format); + const parsed = parseClientConfiguration(original, format); return { kind: "configuration", transactionPath, original, parsed }; } catch (cause: unknown) { return configurationFailure( cause instanceof ClientConfigurationShapeError || cause instanceof ConfigurationError ? `Configuration was not changed: ${cause.message}. Repair it, then rerun uninstall.` - : `Configuration is not valid ${client.format === "toml" || client.format === "grok" ? "TOML" : client.format === "opencode" ? "JSONC" : client.format === "hermes" || client.format === "dsh" ? "YAML" : "JSON"} and was not changed. Repair it, then rerun uninstall.`, + : format === undefined + ? "This client's configuration syntax is unknown, so its configuration was not changed." + : `Configuration is not valid ${clientConfigurationSyntaxName(format)} and was not changed. Repair it, then rerun uninstall.`, ); } }; diff --git a/tests/acceptance/setup/clientConfigurationDiagnosticsCli.test.ts b/tests/acceptance/setup/clientConfigurationDiagnosticsCli.test.ts index 5043a6624f..60dec064bf 100644 --- a/tests/acceptance/setup/clientConfigurationDiagnosticsCli.test.ts +++ b/tests/acceptance/setup/clientConfigurationDiagnosticsCli.test.ts @@ -82,9 +82,24 @@ const cases = [ client: "cursor", path: ".cursor/mcp.json", text: "not-json", - diagnostic: "Configuration is not valid JSON", + diagnostic: "Configuration is not valid JSONC and was not changed", + syntax: "JSONC", + }, + { + // Pi loads mcp.json with strict JSON.parse, so REA's reader admits JSON only. + client: "pi", + path: ".pi/agent/mcp.json", + text: '{"mcpServers":{},}', + diagnostic: "Configuration is not valid JSON and was not changed", syntax: "JSON", }, + { + client: "codex", + path: ".codex/config.toml", + text: "[mcp_servers\n", + diagnostic: "Configuration is not valid TOML and was not changed", + syntax: "TOML", + }, { client: "hermes", path: ".hermes/config.yaml", @@ -142,7 +157,7 @@ for (const scenario of cases) expect(setup.json).toMatchObject({ remediation: expect.stringMatching(/\S/u), }); - if (scenario.syntax === "JSON") + if (scenario.syntax === "JSONC") expect(setup.stdout).toContain("Invalid JSON/JSONC"); } else { expect(setup.json).toMatchObject({