From cd85014b9edab9659f9251070e71ea258612026e Mon Sep 17 00:00:00 2001 From: basil-k-aji-dev <70605804+basil-k-aji-dev@users.noreply.github.com> Date: Sat, 3 Oct 2026 20:20:40 +0530 Subject: [PATCH] fix(plan-canvas): route session state through CLAUDE_CONFIG_DIR The Plan Canvas library wrote ~/.claude/plan-canvas while the SessionStart and Stop hooks read CLAUDE_CONFIG_DIR/plan-canvas. Under a second profile SessionStart missed open reviews and Stop let a turn end with feedback still queued. Expand a leading ~ in CLAUDE_CONFIG_DIR the way scripts/lib/agent-data-home.js does, so a profile written as ~/.claude-work resolves to one directory rather than two. Add hook-level coverage that spawns each hook with a distinct CLAUDE_CONFIG_DIR and no ECC_* override, plus a producer-to-consumer round trip. Eight of the nine cases fail against the previous revision. --- scripts/hooks/mcp-health-check.js | 18 +- scripts/hooks/plan-canvas-pending.js | 18 +- scripts/hooks/plan-canvas-sessions.js | 18 +- scripts/hooks/post-bash-command-log.js | 18 +- scripts/lib/plan-canvas/sessions.js | 18 +- tests/hooks/claude-config-dir-routing.test.js | 279 ++++++++++++++++++ tests/lib/agent-data-home.test.js | 4 +- 7 files changed, 351 insertions(+), 22 deletions(-) create mode 100644 tests/hooks/claude-config-dir-routing.test.js diff --git a/scripts/hooks/mcp-health-check.js b/scripts/hooks/mcp-health-check.js index adcc11540..6003033c3 100644 --- a/scripts/hooks/mcp-health-check.js +++ b/scripts/hooks/mcp-health-check.js @@ -47,14 +47,22 @@ function envNumber(name, fallback) { return Number.isFinite(value) && value >= 0 ? value : fallback; } -// Claude Code redirects every profile-local path when CLAUDE_CONFIG_DIR is set. -// Writing to ~/.claude regardless leaks this profile's data into the default one. +// Resolve the Claude profile directory the same way everywhere. +// +// Duplicated rather than imported on purpose: hooks ship in managed installs +// that carry top-level `scripts/` without `scripts/lib/` (see #3259), so a +// require into lib/ would make the hook unloadable there. `~` is expanded the +// way `scripts/lib/agent-data-home.js` expands it, so a profile written as +// `~/.claude-work` resolves to one place rather than two. function claudeConfigDir() { const configured = process.env.CLAUDE_CONFIG_DIR; - if (configured && String(configured).trim().length > 0) { - return path.resolve(String(configured).trim()); + const trimmed = configured ? String(configured).trim() : ''; + if (!trimmed) return path.join(os.homedir(), '.claude'); + if (trimmed.startsWith('~')) { + const remainder = trimmed.slice(1).replace(/^[/\\]+/, ''); + return remainder ? path.join(os.homedir(), remainder) : os.homedir(); } - return path.join(os.homedir(), '.claude'); + return path.resolve(trimmed); } function stateFilePath() { diff --git a/scripts/hooks/plan-canvas-pending.js b/scripts/hooks/plan-canvas-pending.js index 4e97bddda..b9b7dee4b 100644 --- a/scripts/hooks/plan-canvas-pending.js +++ b/scripts/hooks/plan-canvas-pending.js @@ -34,14 +34,22 @@ const path = require('path'); const SERVER_TIMEOUT_MS = 1000; const MAX_ITEMS_REPORTED = 20; -// Claude Code redirects every profile-local path when CLAUDE_CONFIG_DIR is set. -// Writing to ~/.claude regardless leaks this profile's data into the default one. +// Resolve the Claude profile directory the same way everywhere. +// +// Duplicated rather than imported on purpose: hooks ship in managed installs +// that carry top-level `scripts/` without `scripts/lib/` (see #3259), so a +// require into lib/ would make the hook unloadable there. `~` is expanded the +// way `scripts/lib/agent-data-home.js` expands it, so a profile written as +// `~/.claude-work` resolves to one place rather than two. function claudeConfigDir() { const configured = process.env.CLAUDE_CONFIG_DIR; - if (configured && String(configured).trim().length > 0) { - return path.resolve(String(configured).trim()); + const trimmed = configured ? String(configured).trim() : ''; + if (!trimmed) return path.join(os.homedir(), '.claude'); + if (trimmed.startsWith('~')) { + const remainder = trimmed.slice(1).replace(/^[/\\]+/, ''); + return remainder ? path.join(os.homedir(), remainder) : os.homedir(); } - return path.join(os.homedir(), '.claude'); + return path.resolve(trimmed); } function stateDir() { diff --git a/scripts/hooks/plan-canvas-sessions.js b/scripts/hooks/plan-canvas-sessions.js index 009d68a69..b2e4a1574 100644 --- a/scripts/hooks/plan-canvas-sessions.js +++ b/scripts/hooks/plan-canvas-sessions.js @@ -19,14 +19,22 @@ const fs = require('fs'); const path = require('path'); const os = require('os'); -// Claude Code redirects every profile-local path when CLAUDE_CONFIG_DIR is set. -// Writing to ~/.claude regardless leaks this profile's data into the default one. +// Resolve the Claude profile directory the same way everywhere. +// +// Duplicated rather than imported on purpose: hooks ship in managed installs +// that carry top-level `scripts/` without `scripts/lib/` (see #3259), so a +// require into lib/ would make the hook unloadable there. `~` is expanded the +// way `scripts/lib/agent-data-home.js` expands it, so a profile written as +// `~/.claude-work` resolves to one place rather than two. function claudeConfigDir() { const configured = process.env.CLAUDE_CONFIG_DIR; - if (configured && String(configured).trim().length > 0) { - return path.resolve(String(configured).trim()); + const trimmed = configured ? String(configured).trim() : ''; + if (!trimmed) return path.join(os.homedir(), '.claude'); + if (trimmed.startsWith('~')) { + const remainder = trimmed.slice(1).replace(/^[/\\]+/, ''); + return remainder ? path.join(os.homedir(), remainder) : os.homedir(); } - return path.join(os.homedir(), '.claude'); + return path.resolve(trimmed); } function stateDir() { diff --git a/scripts/hooks/post-bash-command-log.js b/scripts/hooks/post-bash-command-log.js index 41945b115..c57579d49 100644 --- a/scripts/hooks/post-bash-command-log.js +++ b/scripts/hooks/post-bash-command-log.js @@ -38,14 +38,22 @@ function appendLine(filePath, line) { fs.appendFileSync(filePath, `${line}\n`, 'utf8'); } -// Claude Code redirects every profile-local path when CLAUDE_CONFIG_DIR is set. -// Writing to ~/.claude regardless leaks this profile's data into the default one. +// Resolve the Claude profile directory the same way everywhere. +// +// Duplicated rather than imported on purpose: hooks ship in managed installs +// that carry top-level `scripts/` without `scripts/lib/` (see #3259), so a +// require into lib/ would make the hook unloadable there. `~` is expanded the +// way `scripts/lib/agent-data-home.js` expands it, so a profile written as +// `~/.claude-work` resolves to one place rather than two. function claudeConfigDir() { const configured = process.env.CLAUDE_CONFIG_DIR; - if (configured && String(configured).trim().length > 0) { - return path.resolve(String(configured).trim()); + const trimmed = configured ? String(configured).trim() : ''; + if (!trimmed) return path.join(os.homedir(), '.claude'); + if (trimmed.startsWith('~')) { + const remainder = trimmed.slice(1).replace(/^[/\\]+/, ''); + return remainder ? path.join(os.homedir(), remainder) : os.homedir(); } - return path.join(os.homedir(), '.claude'); + return path.resolve(trimmed); } function run(rawInput, mode = 'audit') { diff --git a/scripts/lib/plan-canvas/sessions.js b/scripts/lib/plan-canvas/sessions.js index 799cf4b36..cd254600d 100644 --- a/scripts/lib/plan-canvas/sessions.js +++ b/scripts/lib/plan-canvas/sessions.js @@ -16,10 +16,26 @@ const path = require('path'); const FEEDBACK_KINDS = new Set(['chat', 'annotation', 'verdict']); const VERDICTS = new Set(['approve', 'request-changes']); +// The producer and the SessionStart/Stop hooks must land on one directory. The +// hooks resolve CLAUDE_CONFIG_DIR (they cannot require this file: managed +// installs ship scripts/ without scripts/lib/, see #3259), so writing to +// ~/.claude here regardless would hide queued feedback from them whenever a +// session runs under a second profile. +function resolveClaudeConfigDir(env = process.env) { + const configured = env.CLAUDE_CONFIG_DIR; + const trimmed = configured ? String(configured).trim() : ''; + if (!trimmed) return path.join(os.homedir(), '.claude'); + if (trimmed.startsWith('~')) { + const remainder = trimmed.slice(1).replace(/^[/\\]+/, ''); + return remainder ? path.join(os.homedir(), remainder) : os.homedir(); + } + return path.resolve(trimmed); +} + function resolveStateDir(env = process.env) { const override = env.ECC_PLAN_CANVAS_STATE_DIR; if (override && String(override).trim()) return path.resolve(String(override).trim()); - return path.join(os.homedir(), '.claude', 'plan-canvas'); + return path.join(resolveClaudeConfigDir(env), 'plan-canvas'); } // Canonicalize so `./plan.md`, symlinks, and absolute paths all land on the diff --git a/tests/hooks/claude-config-dir-routing.test.js b/tests/hooks/claude-config-dir-routing.test.js new file mode 100644 index 000000000..1d6e78fa3 --- /dev/null +++ b/tests/hooks/claude-config-dir-routing.test.js @@ -0,0 +1,279 @@ +/** + * Tests that the hooks route profile state through CLAUDE_CONFIG_DIR + * + * Run with: node tests/hooks/claude-config-dir-routing.test.js + * + * These spawn the hooks rather than calling the shared resolver. The four hooks + * below cannot require scripts/lib/agent-data-home.js: managed installs ship a + * top-level scripts/ without scripts/lib/ (#3259), so each carries its own copy + * of the resolver. A resolver test therefore cannot show where a hook writes. + */ + +const assert = require('assert'); +const path = require('path'); +const fs = require('fs'); +const os = require('os'); +const { spawnSync } = require('child_process'); + +const hooksDir = path.join(__dirname, '..', '..', 'scripts', 'hooks'); +const libSessions = path.join(__dirname, '..', '..', 'scripts', 'lib', 'plan-canvas', 'sessions.js'); + +function test(name, fn) { + try { + fn(); + console.log(` ✓ ${name}`); + return true; + } catch (err) { + console.log(` ✗ ${name}`); + console.log(` Error: ${err.message}`); + return false; + } +} + +function makeTempDir() { + return fs.mkdtempSync(path.join(os.tmpdir(), 'config-dir-routing-')); +} + +/** + * A profile that is not the home directory, so a hook that ignored + * CLAUDE_CONFIG_DIR would write somewhere this returns. + */ +function makeProfile() { + const root = makeTempDir(); + return { + home: path.join(root, 'home'), + profile: path.join(root, 'profile', '.claude-work'), + }; +} + +// No ECC_* override is passed anywhere here: an override would route the hook +// before it reached the CLAUDE_CONFIG_DIR fallback under test. +function runHook(hookFile, { profile, home, stdin = '{}', args = [], env = {} }) { + fs.mkdirSync(home, { recursive: true }); + return spawnSync('node', [path.join(hooksDir, hookFile), ...args], { + input: stdin, + encoding: 'utf8', + env: { + ...process.env, + HOME: home, + USERPROFILE: home, + CLAUDE_CONFIG_DIR: profile, + ECC_PLAN_CANVAS_STATE_DIR: undefined, + ECC_MCP_HEALTH_STATE_PATH: undefined, + ...env, + }, + }); +} + +/** + * plan-canvas-pending.js exports run() and is required by the lifecycle hook + * bootstrap rather than spawned, so it is driven here in a child process. The + * child is what makes CLAUDE_CONFIG_DIR meaningful: requiring it in-process, + * the way tests/hooks/plan-canvas-pending-hook.test.js does with + * ECC_PLAN_CANVAS_STATE_DIR, cannot reach the fallback under test. + */ +function runPendingHook({ profile, home, payload }) { + fs.mkdirSync(home, { recursive: true }); + const result = spawnSync('node', ['-e', ` + require(${JSON.stringify(path.join(hooksDir, 'plan-canvas-pending.js'))}) + .run(${JSON.stringify(JSON.stringify(payload))}) + .then(out => process.stdout.write(out.stdout)); + `], { + encoding: 'utf8', + env: { + ...process.env, + HOME: home, + USERPROFILE: home, + CLAUDE_CONFIG_DIR: profile, + ECC_PLAN_CANVAS_STATE_DIR: undefined, + }, + }); + assert.strictEqual(result.status, 0, `pending hook failed: ${result.stderr}`); + return result.stdout; +} + +function writeSessions(stateDir, sessions) { + fs.mkdirSync(stateDir, { recursive: true }); + fs.writeFileSync(path.join(stateDir, 'sessions.json'), JSON.stringify({ sessions }, null, 2)); +} + +function runTests() { + console.log('\n🧪 Testing CLAUDE_CONFIG_DIR routing in hooks\n'); + let passed = 0; + let failed = 0; + const record = ok => { if (ok) passed++; else failed++; }; + + console.log('post-bash-command-log.js'); + + record(test('writes the audit log under CLAUDE_CONFIG_DIR', () => { + const { home, profile } = makeProfile(); + runHook('post-bash-command-log.js', { + home, + profile, + args: ['audit'], + stdin: JSON.stringify({ tool_name: 'Bash', tool_input: { command: 'echo routed' } }), + }); + const log = path.join(profile, 'bash-commands.log'); + assert.ok(fs.existsSync(log), `expected ${log} to exist`); + assert.match(fs.readFileSync(log, 'utf8'), /echo routed/); + assert.ok( + !fs.existsSync(path.join(home, '.claude', 'bash-commands.log')), + 'the log must not also land in ~/.claude' + ); + })); + + record(test('writes the cost log under CLAUDE_CONFIG_DIR', () => { + const { home, profile } = makeProfile(); + runHook('post-bash-command-log.js', { + home, + profile, + args: ['cost'], + stdin: JSON.stringify({ tool_name: 'Bash', tool_input: { command: 'echo costed' } }), + }); + assert.match(fs.readFileSync(path.join(profile, 'cost-tracker.log'), 'utf8'), /echo costed/); + })); + + record(test('falls back to ~/.claude when CLAUDE_CONFIG_DIR is unset', () => { + const { home } = makeProfile(); + fs.mkdirSync(home, { recursive: true }); + spawnSync('node', [path.join(hooksDir, 'post-bash-command-log.js'), 'audit'], { + input: JSON.stringify({ tool_name: 'Bash', tool_input: { command: 'echo default' } }), + encoding: 'utf8', + env: { ...process.env, HOME: home, USERPROFILE: home, CLAUDE_CONFIG_DIR: undefined }, + }); + assert.match( + fs.readFileSync(path.join(home, '.claude', 'bash-commands.log'), 'utf8'), + /echo default/ + ); + })); + + record(test('expands a tilde profile instead of creating a "~" directory', () => { + // path.resolve('~/.claude-work') would make ./~/.claude-work under the cwd, + // splitting the hook off from scripts/lib/agent-data-home.js, which expands. + const { home } = makeProfile(); + runHook('post-bash-command-log.js', { + home, + profile: '~/.claude-work', + args: ['audit'], + stdin: JSON.stringify({ tool_name: 'Bash', tool_input: { command: 'echo tilde' } }), + }); + assert.match( + fs.readFileSync(path.join(home, '.claude-work', 'bash-commands.log'), 'utf8'), + /echo tilde/ + ); + })); + + console.log('\nmcp-health-check.js'); + + record(test('writes the health cache under CLAUDE_CONFIG_DIR', () => { + const { home, profile } = makeProfile(); + fs.mkdirSync(home, { recursive: true }); + // Without a configured server the hook skips the probe and never writes. + // The command does not exist, so the probe fails and the result is cached. + fs.writeFileSync(path.join(home, '.claude.json'), JSON.stringify({ + mcpServers: { routing_probe: { command: 'ecc-no-such-mcp-binary', args: [] } }, + })); + runHook('mcp-health-check.js', { + home, + profile, + stdin: JSON.stringify({ tool_name: 'mcp__routing_probe__ping', tool_input: {} }), + }); + const cache = path.join(profile, 'mcp-health-cache.json'); + assert.ok(fs.existsSync(cache), `expected ${cache} to exist`); + assert.ok( + !fs.existsSync(path.join(home, '.claude', 'mcp-health-cache.json')), + 'the cache must not also land in ~/.claude' + ); + })); + + console.log('\nplan-canvas-sessions.js (SessionStart)'); + + record(test('reports open sessions recorded under CLAUDE_CONFIG_DIR', () => { + const { home, profile } = makeProfile(); + writeSessions(path.join(profile, 'plan-canvas'), { + k1: { file: '/work/plan.md', status: 'open', pendingFeedback: [] }, + }); + const result = runHook('plan-canvas-sessions.js', { home, profile }); + assert.match(result.stdout, /\/work\/plan\.md/); + })); + + record(test('does not read ~/.claude when a profile is set', () => { + const { home, profile } = makeProfile(); + // Only the home copy holds a session. A hook that ignored the profile + // would announce it; the right answer here is silence. + writeSessions(path.join(home, '.claude', 'plan-canvas'), { + k1: { file: '/work/leaked.md', status: 'open', pendingFeedback: [] }, + }); + const result = runHook('plan-canvas-sessions.js', { home, profile }); + assert.strictEqual(result.stdout.trim(), ''); + })); + + console.log('\nplan-canvas-pending.js (Stop)'); + + record(test('blocks on feedback queued under CLAUDE_CONFIG_DIR', () => { + const { home, profile } = makeProfile(); + const cwd = makeTempDir(); + const artifact = path.join(cwd, 'plan.md'); + fs.writeFileSync(artifact, '# plan\n'); + writeSessions(path.join(profile, 'plan-canvas'), { + k1: { + key: 'k1', + file: artifact, + status: 'feedback', + pendingFeedback: [{ kind: 'verdict', verdict: 'request-changes', text: 'tighten step 2' }], + }, + }); + const decision = JSON.parse(runPendingHook({ home, profile, payload: { cwd } })); + assert.strictEqual(decision.decision, 'block'); + assert.match(decision.reason, /tighten step 2/); + })); + + console.log('\nproducer to consumer'); + + record(test('a session the library records is found by both hooks', () => { + // The regression this guards: the library wrote ~/.claude/plan-canvas while + // the hooks read CLAUDE_CONFIG_DIR/plan-canvas, so under a second profile + // SessionStart missed open reviews and Stop let a turn end with feedback + // still queued. Both ends are exercised with no state-directory override. + const { home, profile } = makeProfile(); + const cwd = makeTempDir(); + const artifact = path.join(cwd, 'design.md'); + fs.writeFileSync(artifact, '# design\n'); + fs.mkdirSync(home, { recursive: true }); + + const producer = spawnSync('node', ['-e', ` + const store = require(${JSON.stringify(libSessions)}).createSessionStore(); + const { session } = store.open(${JSON.stringify(artifact)}); + store.queueFeedback(session.key, [{ kind: 'verdict', verdict: 'request-changes', text: 'needs a rollback plan' }]); + `], { + encoding: 'utf8', + env: { + ...process.env, + HOME: home, + USERPROFILE: home, + CLAUDE_CONFIG_DIR: profile, + ECC_PLAN_CANVAS_STATE_DIR: undefined, + }, + }); + assert.strictEqual(producer.status, 0, `producer failed: ${producer.stderr}`); + assert.ok( + fs.existsSync(path.join(profile, 'plan-canvas', 'sessions.json')), + 'the library must record the session under the profile' + ); + + const sessionStart = runHook('plan-canvas-sessions.js', { home, profile }); + assert.match(sessionStart.stdout, /design\.md/, 'SessionStart must see the open review'); + + const decision = JSON.parse(runPendingHook({ home, profile, payload: { cwd } })); + assert.strictEqual(decision.decision, 'block'); + assert.match(decision.reason, /needs a rollback plan/); + })); + + console.log(`\n${'='.repeat(50)}`); + console.log(`Passed: ${passed}`); + console.log(`Failed: ${failed}`); + console.log('='.repeat(50)); + if (failed > 0) process.exit(1); +} + +runTests(); diff --git a/tests/lib/agent-data-home.test.js b/tests/lib/agent-data-home.test.js index f867042ca..82a744b77 100644 --- a/tests/lib/agent-data-home.test.js +++ b/tests/lib/agent-data-home.test.js @@ -567,9 +567,11 @@ function runTests() { JSON.stringify({ agentDataHome: path.join(profileDir, 'nested') }), 'utf8' ); + // readProjectConfigAt canonicalizes, and macOS reaches os.tmpdir() + // through a symlink, so compare against the resolved home. assert.strictEqual( agentDataHome.readProjectConfigAt(configPath), - path.join(profileDir, 'nested') + path.join(fs.realpathSync(homeDir), '.claude-profile-x', 'nested') ); // The old default is no longer a root, and neither is anything outside