From fc4fab749f82458f7c57f69014b6183153375d44 Mon Sep 17 00:00:00 2001 From: Jordan Kail <13952435+jckail@users.noreply.github.com> Date: Fri, 2 Oct 2026 14:29:00 -0700 Subject: [PATCH] fix(hosts): preserve malformed settings and require explicit retraction --- src/claude/init.ts | 14 +++--- src/hosts/retract.ts | 5 +- test/graft-preservation.test.ts | 89 +++++++++++++++++++++++++++++++++ 3 files changed, 100 insertions(+), 8 deletions(-) create mode 100644 test/graft-preservation.test.ts diff --git a/src/claude/init.ts b/src/claude/init.ts index 111c0f926..f2f820ef7 100644 --- a/src/claude/init.ts +++ b/src/claude/init.ts @@ -1,4 +1,4 @@ -import { mkdirSync, writeFileSync, readFileSync, chmodSync } from 'node:fs'; +import { mkdirSync, writeFileSync, chmodSync } from 'node:fs'; import { join, dirname } from 'node:path'; import { homedir } from 'node:os'; import { execFileSync } from 'node:child_process'; @@ -10,6 +10,7 @@ import { claudeDistDir } from './paths.js'; import { mergeJsonKey, serverEntry, type McpWrite } from '../hosts/mcp-config.js'; import { hasGraftIndex } from '../graph/root.js'; import type { PlannedWrite } from '../hosts/plan.js'; +import { readJsonObject } from '../hosts/config-write.js'; /** * The files `runInit` writes — pure, no writes, so `--dry-run` and the picker @@ -67,12 +68,13 @@ export function runInit( // Same list `--dry-run` and the picker report, so the two can't drift apart. const [settings, statusline, hooks, skill, mcpTarget] = claudeTargets(dir).map((t) => t.path); - mkdirSync(dirname(statusline), { recursive: true }); - const settingsPath = settings; - let existing: Record = {}; - try { existing = JSON.parse(readFileSync(settingsPath, 'utf8')); } catch { /* none/invalid → start fresh */ } - const { merged, warnings } = mergeGraftSettings(existing, { statusline: opts.statusline }); + const loaded = readJsonObject(settingsPath); + if (loaded === 'unparseable') { + throw new Error(`Cannot initialize Graft: ${settingsPath} is not a readable JSON object; existing settings left unchanged.`); + } + const { merged, warnings } = mergeGraftSettings(loaded.root, { statusline: opts.statusline }); + mkdirSync(dirname(statusline), { recursive: true }); writeFileSync(settingsPath, `${JSON.stringify(merged, null, 2)}\n`); const sl = statusline; diff --git a/src/hosts/retract.ts b/src/hosts/retract.ts index 801c5aa97..07c180cfd 100644 --- a/src/hosts/retract.ts +++ b/src/hosts/retract.ts @@ -484,12 +484,13 @@ export function planRetract(repo: string, opts: RetractOpts = {}): Retraction[] } /** - * Remove graft's contribution from every target. Reports every target it + * Report graft's contribution by default; remove it only with `apply: true`. + * Reports every target it * considered, including the ones that were already clean, so a caller can show * either the full sweep or just what changed. */ export function runRetract(repo: string, opts: RetractOpts = {}): Retraction[] { - const apply = opts.apply !== false; + const apply = opts.apply === true; return targets(repo, opts).map(({ run, ...t }) => ({ ...t, action: run(apply) })); } diff --git a/test/graft-preservation.test.ts b/test/graft-preservation.test.ts new file mode 100644 index 000000000..3a01187f9 --- /dev/null +++ b/test/graft-preservation.test.ts @@ -0,0 +1,89 @@ +import { test, after } from 'node:test'; +import assert from 'node:assert/strict'; +import { mkdtempSync, mkdirSync, readFileSync, writeFileSync, readdirSync, existsSync, rmSync } from 'node:fs'; +import { join } from 'node:path'; +import { tmpdir } from 'node:os'; +import { runInit } from '../src/claude/init.js'; +import { runRetract } from '../src/hosts/retract.js'; + +// Never probe an installed package or start a missing-graph build/provider. +process.env.GRAFT_MCP_NPX = '1'; +const owned: string[] = []; +function fresh(): string { + const dir = mkdtempSync(join(tmpdir(), 'graft-preservation-')); + owned.push(dir); + return dir; +} +after(() => { for (const dir of owned) rmSync(dir, { recursive: true, force: true }); }); + +for (const body of ['{ malformed', 'null', '[]', '[{"model":"user"}]', '42', '"user"', 'false']) { + test(`invalid project settings ${body} stay byte-identical before any other installation write`, () => { + const repo = fresh(); + const home = fresh(); + mkdirSync(join(repo, '.claude')); + const settings = join(repo, '.claude', 'settings.json'); + writeFileSync(settings, body); + assert.throws(() => runInit(repo, { build: false, global: false, home }), /not a readable JSON object/); + assert.equal(readFileSync(settings, 'utf8'), body); + assert.deepEqual(readdirSync(join(repo, '.claude')), ['settings.json']); + assert.deepEqual(readdirSync(home), []); + assert.equal(existsSync(join(repo, '.mcp.json')), false); + assert.equal(existsSync(join(repo, 'graft')), false); + }); +} + +test('missing project settings create normal repo wiring without home or graph writes', () => { + const repo = fresh(); + const home = fresh(); + const result = runInit(repo, { build: false, global: false, home }); + const settings = JSON.parse(readFileSync(result.settingsPath, 'utf8')); + assert.ok(settings.hooks.Stop); + assert.ok(existsSync(join(repo, '.mcp.json'))); + assert.equal(result.built, false); + assert.deepEqual(result.global, []); + assert.deepEqual(readdirSync(home), []); + assert.equal(existsSync(join(repo, 'graft')), false); +}); + +test('valid project settings preserve foreign settings and ordinary foreign handlers', () => { + const repo = fresh(); + const home = fresh(); + mkdirSync(join(repo, '.claude')); + const path = join(repo, '.claude', 'settings.json'); + const foreign = { matcher: 'Bash', hooks: [{ type: 'command', command: 'inert-foreign-sentinel' }] }; + writeFileSync(path, JSON.stringify({ model: 'user-choice', statusLine: { command: 'inert-user-bar' }, + permissions: { deny: ['Bash(inert-deny:*)'], allow: ['Bash(inert-allow:*)'] }, hooks: { Stop: [foreign] } })); + const result = runInit(repo, { build: false, global: false, home }); + const settings = JSON.parse(readFileSync(path, 'utf8')); + assert.equal(settings.model, 'user-choice'); + assert.equal(settings.statusLine.command, 'inert-user-bar'); + assert.deepEqual(settings.permissions.deny, ['Bash(inert-deny:*)']); + assert.ok(settings.permissions.allow.includes('Bash(inert-allow:*)')); + assert.deepEqual(settings.hooks.Stop[0], foreign); + assert.ok(result.warnings.length); + assert.deepEqual(readdirSync(home), []); +}); + +for (const apply of [undefined, false, true]) { + test(`retract apply=${String(apply)} mutates only on explicit true`, () => { + const repo = fresh(); + const home = fresh(); + const body = '\nowned guidance\n\n'; + const path = join(repo, 'AGENTS.md'); + writeFileSync(path, body); + mkdirSync(join(home, '.codex')); + const config = join(home, '.codex', 'config.toml'); + const toml = '[mcp_servers.graft]\ncommand = "inert-not-executed"\n'; + writeFileSync(config, toml); + const opts = apply === undefined ? { home } : { home, apply }; + const reports = runRetract(repo, opts); + assert.ok(reports.some((r) => r.path === path && r.action === 'deleted')); + if (apply === true) { + assert.equal(existsSync(path), false); + assert.equal(existsSync(config), false); + } else { + assert.equal(readFileSync(path, 'utf8'), body); + assert.equal(readFileSync(config, 'utf8'), toml); + } + }); +}