diff --git a/src/hosts/mcp-config.ts b/src/hosts/mcp-config.ts index cf53817b..c56acc13 100644 --- a/src/hosts/mcp-config.ts +++ b/src/hosts/mcp-config.ts @@ -112,24 +112,81 @@ function tomlHeader(name: Brand): string { return `[mcp_servers.${name}]`; } +type TomlStringQuote = '"' | "'"; + +/** Carry multiline string context across lines without interpreting their text as tables. */ +function nextTomlMultilineQuote(line: string, carried: TomlStringQuote | null): TomlStringQuote | null { + let quote = carried; + let multiline = carried !== null; + for (let i = 0; i < line.length; i++) { + const char = line[i]; + if (quote !== null) { + if (quote === '"' && char === '\\') { + i++; // Basic strings escape the next character; literal strings do not. + } else if (char === quote) { + if (!multiline) { + quote = null; + } else if (line.startsWith(quote.repeat(3), i)) { + const closing = quote; + quote = null; + multiline = false; + i += 2; + // Four/five closing quotes include one/two literal quotes, not a new string. + while (line[i + 1] === closing) i++; + } + } + } else { + if (char === '#') break; + if (char === '"' || char === "'") { + quote = char; + multiline = line.startsWith(char.repeat(3), i); + if (multiline) i += 2; + } + } + } + return multiline ? quote : null; +} + /** - * Remove the `[mcp_servers.graft]` table (or `name`'s) from a TOML config, returning the rest. + * Remove every canonical table owned by `name` from a TOML config, returning the rest. * * Line-based on purpose: a real parse-and-reserialize would reformat the user's - * whole file. The table runs from its header to the next `[`-header or EOF, which - * is exactly the shape {@link upsertCodexToml} appends. Exported so the writer and + * whole file. An owned table is `[mcp_servers.]` itself or any dotted + * subtable below it (`[mcp_servers..*]`, which TOML scopes under the + * parent) — wherever it appears in the file, contiguous or not, and even when + * the parent is gone and only an orphaned subtable remains. Anything else + * starting a new table (`[mcp_servers.other]`, `[mcp_servers.graft2]`, + * `[other.graft]`) is foreign and preserved. Exported so the writer and * `retract.ts` can never disagree about what "graft's section" means. */ export function stripTomlSection(text: string, name: Brand = 'graft'): { rest: string; found: boolean } { - const lines = text.split('\n'); - const header = tomlHeader(name); - const start = lines.findIndex((l) => l.trim() === header); - if (start === -1) return { rest: text, found: false }; - let end = start + 1; - while (end < lines.length && !lines[end].trimStart().startsWith('[')) end++; - const rest = [...lines.slice(0, start), ...lines.slice(end)] - .join('\n') - .replace(/\n{3,}/g, '\n\n') + const lines = text.split(/(?<=\n)/); + const ownedHeader = tomlHeader(name); + // The trailing dot keeps similarly-named tables (`[mcp_servers.graft2]`, + // `[other.graft]`) out of the family. + const familyPrefix = `${ownedHeader.slice(0, -1)}.`; + const isFamilyHeader = (line: string): boolean => { + const trimmed = line.trimStart(); + if (!trimmed.startsWith('[')) return false; + const header = trimmed.split(/\s+/)[0]; + return header === ownedHeader || header.startsWith(familyPrefix); + }; + let multiline: TomlStringQuote | null = null; + let found = false; + let inOwned = false; + const kept: string[] = []; + for (const line of lines) { + if (multiline === null && line.trimStart().startsWith('[')) { + inOwned = isFamilyHeader(line); + found ||= inOwned; + } + if (!inOwned) kept.push(line); + multiline = nextTomlMultilineQuote(line, multiline); + } + if (!found) return { rest: text, found: false }; + // Interior blank lines can be multiline string data; preserve them verbatim. + const rest = kept + .join('') .replace(/^\n+/, ''); return { rest, found: true }; } diff --git a/test/hosts-mcp-config.test.ts b/test/hosts-mcp-config.test.ts index f6596670..af08e3fc 100644 --- a/test/hosts-mcp-config.test.ts +++ b/test/hosts-mcp-config.test.ts @@ -7,10 +7,42 @@ process.env.GRAFT_MCP_NPX = '1'; import { mkdtempSync, mkdirSync, readFileSync, writeFileSync, existsSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; -import { registerMcpConfigs, serverEntry } from '../src/hosts/mcp-config.js'; +import { registerMcpConfigs, serverEntry, stripTomlSection } from '../src/hosts/mcp-config.js'; function fresh(): string { return mkdtempSync(join(tmpdir(), 'graft-mcpcfg-')); } +for (const name of ['graft', 'trail'] as const) { + for (const quote of ['"', "'"]) { + test(`${name} removal preserves multiline ${quote} data and the other brand`, () => { + const other = name === 'graft' ? 'trail' : 'graft'; + const delimiter = quote.repeat(3); + const foreign = `[mcp_servers.${other}.env]\r\nSCRIPT = ${delimiter}\r\n[mcp_servers.${name}]\r\n\r\n\r\n[mcp_servers.${name}.env]\r\n${delimiter}\r\nKEEP = "yes"\r\n`; + const similar = `[mcp_servers.${name}2]\r\ncommand = "foreign"\r\n`; + const owned = `[mcp_servers.${name}]\r\nNOTE = ${delimiter}\r\n[mcp_servers.foreign]\r\n${delimiter}\r\n`; + const text = owned + foreign + `[mcp_servers.${name}.env]\r\nOWNED = "remove"\r\n` + similar; + assert.deepEqual(stripTomlSection(text, name), { found: true, rest: foreign + similar }); + assert.deepEqual(stripTomlSection(foreign + similar, name), { found: false, rest: foreign + similar }); + }); + } +} + +test('Trail registration migrates both real table families but preserves multiline lookalikes', () => { + const repo = fresh(); const home = fresh(); + mkdirSync(join(home, '.codex'), { recursive: true }); + const path = join(home, '.codex', 'config.toml'); + const foreign = '[mcp_servers.other.env]\nSCRIPT = """\n[mcp_servers.graft]\n\n\n[mcp_servers.trail.env]\n"""\nKEEP = "yes"\n'; + writeFileSync(path, '[mcp_servers.graft]\ncommand = "old"\n' + foreign + + '[mcp_servers.trail.env]\nOLD_TRAIL = "remove"\n[mcp_servers.graft.env]\nOLD_GRAFT = "remove"\n'); + const writes = registerMcpConfigs(repo, ['agents'], { home, name: 'trail' }); + assert.deepEqual(writes.map((w) => w.action), ['updated']); + const result = readFileSync(path, 'utf8'); + assert.ok(result.startsWith(foreign), 'foreign multiline data is byte-identical'); + assert.doesNotMatch(result, /OLD_TRAIL|OLD_GRAFT|command = "old"/); + assert.match(result, /^\[mcp_servers\.trail\]$/m); + assert.deepEqual(registerMcpConfigs(repo, ['agents'], { home, name: 'trail' }).map((w) => w.action), ['unchanged']); + assert.equal(readFileSync(path, 'utf8'), result); +}); + test('cursor/gemini/kiro get repo-local JSON entries', () => { const repo = fresh(); const home = fresh(); const w = registerMcpConfigs(repo, ['cursor', 'gemini', 'kiro'], { home }); @@ -110,3 +142,188 @@ test('GRAFT_MCP_NPX overrides an installed binary', () => { process.env.GRAFT_MCP_NPX = '1'; assert.equal(serverEntry({ onPath: true }).command, 'npx', 'the escape hatch wins'); }); + +// --------------------------------------------------------------------------- +// Issue #546: dotted subtables belong to the parent table +// --------------------------------------------------------------------------- + +test('stripTomlSection removes dotted subtables of [mcp_servers.graft]', () => { + const text = [ + '[mcp_servers.graft]', + 'command = "graft"', + '', + '[mcp_servers.graft.env]', + 'DO_NOT_TRACK = "1"', + '', + '[mcp_servers.other]', + 'command = "x"', + '', + ].join('\n'); + const { rest, found } = stripTomlSection(text); + assert.equal(found, true); + assert.ok(!rest.includes('[mcp_servers.graft]'), 'parent table gone'); + assert.ok(!rest.includes('[mcp_servers.graft.env]'), 'env subtable gone'); + assert.ok(!rest.includes('DO_NOT_TRACK'), 'subtable content gone'); + assert.ok(rest.includes('[mcp_servers.other]'), 'foreign table preserved'); +}); + +test('stripTomlSection removes deeper dotted subtables', () => { + const text = [ + '[mcp_servers.graft]', + 'command = "graft"', + '', + '[mcp_servers.graft.env.extra]', + 'X = "1"', + '', + ].join('\n'); + const { rest } = stripTomlSection(text); + assert.ok(!rest.includes('mcp_servers.graft'), 'deep subtable gone'); +}); + +test('stripTomlSection keeps similarly-named foreign tables', () => { + for (const header of ['[mcp_servers.graft2]', '[other.graft]', '[graft]']) { + const text = [`[mcp_servers.graft]\ncommand = "graft"\n`, `${header}\ncommand = "y"\n`].join('\n'); + const { rest, found } = stripTomlSection(text); + assert.equal(found, true); + assert.ok(!rest.includes('[mcp_servers.graft]\n'), 'own table gone'); + assert.ok(rest.includes(header), `${header} preserved`); + } +}); + +test('stripTomlSection leaves files without graft untouched', () => { + const text = '[mcp_servers.other]\ncommand = "x"\n'; + const { rest, found } = stripTomlSection(text); + assert.equal(found, false); + assert.equal(rest, text, 'byte-identical'); +}); + +test('stripTomlSection removes non-contiguous family tables', () => { + const text = [ + '[mcp_servers.other]', + 'command = "x"', + '', + '[mcp_servers.graft]', + 'command = "graft"', + '', + '[mcp_servers.graft2]', + 'command = "y"', + '', + '[mcp_servers.graft.env]', + 'DO_NOT_TRACK = "1"', + '', + '[mcp_servers.graft.foo.bar]', + 'Z = "2"', + '', + '[other.graft]', + 'command = "z"', + '', + ].join('\n'); + const { rest, found } = stripTomlSection(text); + assert.equal(found, true); + assert.ok(!rest.includes('mcp_servers.graft]'), 'no graft residue'); + assert.ok(!rest.includes('mcp_servers.graft.'), 'no graft subtable residue'); + assert.ok(!rest.includes('DO_NOT_TRACK'), 'subtable content gone'); + assert.ok(rest.includes('[mcp_servers.other]'), 'other preserved'); + assert.ok(rest.includes('[mcp_servers.graft2]'), 'graft2 preserved'); + assert.ok(rest.includes('[other.graft]'), 'other.graft preserved'); +}); + +test('stripTomlSection removes orphan-only subtable without parent', () => { + const text = [ + '[mcp_servers.other]', + 'command = "x"', + '', + '[mcp_servers.graft.env]', + 'DO_NOT_TRACK = "1"', + '', + ].join('\n'); + const { rest, found } = stripTomlSection(text); + assert.equal(found, true); + assert.ok(!rest.includes('mcp_servers.graft'), 'orphan gone'); + assert.ok(rest.includes('[mcp_servers.other]'), 'foreign preserved'); +}); + +for (const quote of ['"', "'"]) { + const delimiter = quote.repeat(3); + test(`stripTomlSection preserves foreign multiline ${quote} strings`, () => { + const foreign = [ + '[mcp_servers.other.env]', + `SCRIPT = ${delimiter}`, + '[mcp_servers.graft]', + '', '', '', + '[mcp_servers.graft.env]', + 'this is string content', + delimiter, + 'KEEP = "2"', + '', + ].join('\n'); + const { rest, found } = stripTomlSection('[mcp_servers.graft]\ncommand = "graft"\n' + foreign); + assert.equal(found, true); + assert.equal(rest, foreign, 'the foreign string and following key survive byte-for-byte'); + assert.deepEqual(stripTomlSection(foreign), { rest: foreign, found: false }, + 'table-looking string content is not an owned table'); + }); + + test(`stripTomlSection removes owned multiline ${quote} strings completely`, () => { + const foreign = '[mcp_servers.other]\ncommand = "other"\n'; + const text = [ + '[mcp_servers.graft]', + `NOTE = ${delimiter}`, + '[mcp_servers.other]', + 'still owned string content', + delimiter, + 'command = "graft"', + foreign, + ].join('\n'); + assert.deepEqual(stripTomlSection(text), { rest: foreign, found: true }); + }); + + for (const closingLength of [4, 5]) { + test(`stripTomlSection recognizes a table after ${closingLength} closing ${quote} quotes`, () => { + const foreign = `[mcp_servers.other.env]\nNOTE = ${delimiter}content${quote.repeat(closingLength)}\n`; + assert.deepEqual(stripTomlSection(foreign + '[mcp_servers.graft]\ncommand = "graft"\n'), + { rest: foreign, found: true }); + }); + } +} + +test('stripTomlSection ignores delimiters in comments and single-line strings', () => { + const foreign = [ + '[mcp_servers.other.env]', + '# """ and \'\'\' are comments', + 'A = "\'\'\'" # """', + 'B = \'"""\'', + 'C = "escaped \\\" quotation"', + '', + ].join('\n'); + assert.deepEqual(stripTomlSection(foreign + '[mcp_servers.graft.env]\nX = "1"\n'), + { rest: foreign, found: true }); +}); + +test('stripTomlSection respects escaped multiline basic-string quotes', () => { + const foreign = [ + '[mcp_servers.other.env]', + 'SCRIPT = """', + 'an escaped delimiter: \\"""', + '[mcp_servers.graft.env]', + 'still string content', + '"""', + 'KEEP = "2"', + '', + ].join('\n'); + assert.deepEqual(stripTomlSection(foreign + '[mcp_servers.graft]\ncommand = "graft"\n'), + { rest: foreign, found: true }); +}); + +test('Codex registration preserves foreign multiline strings across re-registration', () => { + const repo = fresh(); const home = fresh(); + const foreign = '[mcp_servers.other.env]\nSCRIPT = \'\'\'\n[mcp_servers.graft.env]\ntext\n\'\'\'\nKEEP = "2"\n'; + mkdirSync(join(home, '.codex'), { recursive: true }); + const path = join(home, '.codex', 'config.toml'); + writeFileSync(path, '[mcp_servers.graft]\ncommand = "old"\n' + foreign); + assert.deepEqual(registerMcpConfigs(repo, ['agents'], { home }).map((w) => w.action), ['updated']); + const registered = readFileSync(path, 'utf8'); + assert.ok(registered.startsWith(foreign), 'foreign configuration survives the writer'); + assert.deepEqual(registerMcpConfigs(repo, ['agents'], { home }).map((w) => w.action), ['unchanged']); + assert.equal(readFileSync(path, 'utf8'), registered); +}); diff --git a/test/hosts-retract.test.ts b/test/hosts-retract.test.ts index dee31b8e..7fc4eddb 100644 --- a/test/hosts-retract.test.ts +++ b/test/hosts-retract.test.ts @@ -32,6 +32,16 @@ function byPath(rs: ReturnType): Map { const BLOCK = '\n## Graft — old text\nstale guidance\n'; +test('uninstall removes both TOML table families without consuming foreign multiline content', () => { + const repo = fresh(); const home = fresh(); + const foreign = '[mcp_servers.other.env]\nSCRIPT = \'\'\'\n[mcp_servers.trail]\n\n\n[mcp_servers.graft.env]\n\'\'\'\nKEEP = "yes"\n'; + const path = write(home, '.codex/config.toml', '[mcp_servers.trail]\ncommand = "trail"\n' + + foreign + '[mcp_servers.graft.env]\nOLD = "remove"\n[mcp_servers.trail.env]\nOLD = "remove"\n'); + const result = byPath(runRetract(repo, { apply: true, home })); + assert.equal(result.get(path), 'removed'); + assert.equal(readFileSync(path, 'utf8'), foreign); +}); + // -------------------------------------------------------------------------- // instruction files // -------------------------------------------------------------------------- @@ -335,3 +345,40 @@ test('hosts sharing one path produce a single retraction, not one each', () => { const rs = runRetract(d, { global: false }).filter((r) => r.path === agents); assert.equal(rs.length, 1, `AGENTS.md should be queued once, got ${rs.length}`); }); + +// --------------------------------------------------------------------------- +// Issue #546: retract removes dotted subtables, not just the parent table +// --------------------------------------------------------------------------- + +test('[mcp_servers.graft.env] orphan does not survive retract', () => { + const d = fresh(); + const toml = write(d, join('.grok', 'config.toml'), + '[mcp_servers.graft]\ncommand = "npx"\nargs = ["-y","@nanonets/graft","mcp"]\n\n[mcp_servers.graft.env]\nDO_NOT_TRACK = "1"\n\n[mcp_servers.keepme]\ncommand = "y"\n'); + runRetract(d, { apply: true, global: false }); + const text = readFileSync(toml, 'utf8'); + assert.ok(!text.includes('mcp_servers.graft'), 'no graft residue of any depth'); + assert.ok(text.includes('[mcp_servers.keepme]'), 'foreign table preserved'); +}); + +test('orphan-only graft subtable is removed by retract', () => { + const d = fresh(); + const toml = write(d, join('.grok', 'config.toml'), + '[mcp_servers.other]\ncommand = "x"\n\n[mcp_servers.graft.env]\nDO_NOT_TRACK = "1"\n'); + runRetract(d, { apply: true, global: false }); + const text = readFileSync(toml, 'utf8'); + assert.ok(!text.includes('mcp_servers.graft'), 'orphan gone via uninstall path'); + assert.ok(text.includes('[mcp_servers.other]'), 'foreign preserved'); +}); + +test('Codex retract preserves table-looking text inside a foreign multiline string', () => { + const repo = fresh(); const home = fresh(); + const foreign = '[mcp_servers.other.env]\nSCRIPT = \'\'\'\n[mcp_servers.graft.env]\ntext\n\'\'\'\nKEEP = "2"\n'; + const toml = write(home, join('.codex', 'config.toml'), + '[mcp_servers.graft]\ncommand = "graft"\n' + foreign); + const result = byPath(runRetract(repo, { apply: true, home })); + assert.equal(result.get(toml), 'removed'); + assert.equal(readFileSync(toml, 'utf8'), foreign); + const again = byPath(runRetract(repo, { apply: true, home })); + assert.equal(again.get(toml), 'absent', 'string content does not trigger another removal'); + assert.equal(readFileSync(toml, 'utf8'), foreign); +});