From b1ba495997d0bbd5a048cd9c9348a601902d85f1 Mon Sep 17 00:00:00 2001 From: Yann Roberto <1922498827@qq.com> Date: Thu, 1 Oct 2026 13:27:55 +0800 Subject: [PATCH 01/26] fix(sessions): serialize alias read-modify-write operations --- scripts/lib/install/claude-settings-lock.js | 44 ++-- scripts/lib/session-aliases.js | 189 ++++++++++-------- tests/lib/claude-settings-lock.test.js | 148 ++++++++++++++ tests/lib/helpers/session-alias-worker.js | 39 ++++ tests/lib/helpers/settings-lock-worker.js | 14 ++ tests/lib/session-aliases-concurrency.test.js | 87 ++++++++ tests/lib/session-aliases.test.js | 178 +++++++++++------ 7 files changed, 544 insertions(+), 155 deletions(-) create mode 100644 tests/lib/claude-settings-lock.test.js create mode 100644 tests/lib/helpers/session-alias-worker.js create mode 100644 tests/lib/helpers/settings-lock-worker.js create mode 100644 tests/lib/session-aliases-concurrency.test.js diff --git a/scripts/lib/install/claude-settings-lock.js b/scripts/lib/install/claude-settings-lock.js index 164e887ec..88c72580f 100644 --- a/scripts/lib/install/claude-settings-lock.js +++ b/scripts/lib/install/claude-settings-lock.js @@ -141,22 +141,36 @@ function recoverSettingsLock(lockPath, label = 'Claude settings') { } } -function acquireSettingsLock(settingsPath, { label = 'Claude settings' } = {}) { - const lockPath = `${settingsPath}.ecc.lock`; - fs.mkdirSync(path.dirname(settingsPath), { recursive: true }); - try { - return createSettingsLock(lockPath, label); - } catch (error) { - if (!error || error.code !== 'EEXIST') { - throw error; - } +function acquireSettingsLock(settingsPath, { label = 'Claude settings', timeoutMs = 0 } = {}) { + if (!Number.isFinite(timeoutMs) || timeoutMs < 0) { + throw new TypeError('Lock timeoutMs must be a finite non-negative number.'); + } + const lockPath = `${settingsPath}.ecc.lock`; + const deadline = performance.now() + timeoutMs; + const sleeper = new Int32Array(new SharedArrayBuffer(4)); + fs.mkdirSync(path.dirname(settingsPath), { recursive: true }); + while (true) { + try { + return createSettingsLock(lockPath, label); + } catch (error) { + if (!error || error.code !== 'EEXIST') throw error; + } + try { + const recovered = recoverSettingsLock(lockPath, label); + if (recovered) return recovered; + } catch (error) { + // A different writer can acquire or release between inspection and recovery. + if (!error || (error.code !== 'EEXIST' && !(timeoutMs > 0 && error.code === 'ENOENT'))) throw error; + } + const remaining = deadline - performance.now(); + if (remaining <= 0) { + throw new Error( + `Another ECC process is updating ${label}: ${settingsPath}. ` + + `If no ECC process is active, inspect and remove ${lockPath}.` + ); + } + Atomics.wait(sleeper, 0, 0, Math.min(10, remaining)); } - const recovered = recoverSettingsLock(lockPath, label); - if (recovered) return recovered; - throw new Error( - `Another ECC process is updating ${label}: ${settingsPath}. ` - + `If no ECC process is active, inspect and remove ${lockPath}.` - ); } function runWithSettingsLock(settingsPath, callback) { diff --git a/scripts/lib/session-aliases.js b/scripts/lib/session-aliases.js index bfc846315..b57664472 100644 --- a/scripts/lib/session-aliases.js +++ b/scripts/lib/session-aliases.js @@ -5,10 +5,11 @@ const fs = require('fs'); const path = require('path'); +const { writeFileAtomic } = require('./atomic-write'); +const { acquireSettingsLock } = require('./install/claude-settings-lock'); const { getClaudeDir, - ensureDir, readFile, log } = require('./utils'); @@ -80,74 +81,79 @@ function loadAliases() { } } +// Reject callback reentry instead of allowing a nested write to be overwritten +// by the outer transaction's snapshot. +const activeWrites = new Set(); + +function withAliasesLock(callback, failure) { + const aliasesPath = path.resolve(getAliasesPath()); + if (activeWrites.has(aliasesPath)) { + log('[Aliases] Cannot write aliases from inside an alias transaction'); + return failure; + } + + let release; + try { + release = acquireSettingsLock(aliasesPath, { label: 'session aliases', timeoutMs: 5000 }); + } catch (error) { + log(`[Aliases] Error locking aliases: ${error.message}`); + return failure; + } + + activeWrites.add(aliasesPath); + let result; + let primaryError; + let callbackFailed = false; + let releaseError; + try { + result = callback(); + } catch (error) { + callbackFailed = true; + primaryError = error; + } finally { + try { + release(); + } catch (error) { + releaseError = error; + } + activeWrites.delete(aliasesPath); + } + if (callbackFailed) { + if (releaseError) log(`[Aliases] Error releasing aliases lock: ${releaseError.message}`); + throw primaryError; + } + if (releaseError) { + log(`[Aliases] Error releasing aliases lock: ${releaseError.message}`); + return failure; + } + return result; +} + +function saveAliasesUnlocked(aliases) { + try { + const snapshot = { + ...aliases, + metadata: { + totalCount: Object.keys(aliases.aliases).length, + lastUpdated: new Date().toISOString() + } + }; + writeFileAtomic(getAliasesPath(), JSON.stringify(snapshot, null, 2)); + return true; + } catch (error) { + log(`[Aliases] Error saving aliases: ${error.message}`); + return false; + } +} + /** - * Save aliases to file with atomic write - * @param {object} aliases - Aliases object to save + * Atomically replace the complete snapshot. This does not merge snapshots + * loaded by callers before acquiring the lock; use the mutators for updates. + * @param {object} aliases - Complete aliases snapshot * @returns {boolean} Success status */ function saveAliases(aliases) { - const aliasesPath = getAliasesPath(); - const tempPath = aliasesPath + '.tmp'; - const backupPath = aliasesPath + '.bak'; - - try { - // Update metadata - aliases.metadata = { - totalCount: Object.keys(aliases.aliases).length, - lastUpdated: new Date().toISOString() - }; - - const content = JSON.stringify(aliases, null, 2); - - // Ensure directory exists - ensureDir(path.dirname(aliasesPath)); - - // Create backup if file exists - if (fs.existsSync(aliasesPath)) { - fs.copyFileSync(aliasesPath, backupPath); - } - - // Atomic write: write to temp file, then rename - fs.writeFileSync(tempPath, content, 'utf8'); - - // On Windows, rename fails with EEXIST if destination exists, so delete first. - // On Unix/macOS, rename(2) atomically replaces the destination — skip the - // delete to avoid an unnecessary non-atomic window between unlink and rename. - if (process.platform === 'win32' && fs.existsSync(aliasesPath)) { - fs.unlinkSync(aliasesPath); - } - fs.renameSync(tempPath, aliasesPath); - - // Remove backup on success - if (fs.existsSync(backupPath)) { - fs.unlinkSync(backupPath); - } - - return true; - } catch (err) { - log(`[Aliases] Error saving aliases: ${err.message}`); - - // Restore from backup if exists - if (fs.existsSync(backupPath)) { - try { - fs.copyFileSync(backupPath, aliasesPath); - log('[Aliases] Restored from backup'); - } catch (restoreErr) { - log(`[Aliases] Failed to restore backup: ${restoreErr.message}`); - } - } - - // Clean up temp file (best-effort) - try { - if (fs.existsSync(tempPath)) { - fs.unlinkSync(tempPath); - } - } catch { - // Non-critical: temp file will be overwritten on next save - } - - return false; - } + return withAliasesLock(() => saveAliasesUnlocked(aliases), false); } /** @@ -185,7 +191,7 @@ function resolveAlias(alias) { * @param {string} title - Optional title for the alias * @returns {object} Result with success status and message */ -function setAlias(alias, sessionPath, title = null) { +function setAliasUnlocked(alias, sessionPath, title = null) { // Validate alias name if (!alias || alias.length === 0) { return { success: false, error: 'Alias name cannot be empty' }; @@ -221,7 +227,7 @@ function setAlias(alias, sessionPath, title = null) { title: title || null }; - if (saveAliases(data)) { + if (saveAliasesUnlocked(data)) { return { success: true, isNew, @@ -278,7 +284,7 @@ function listAliases(options = {}) { * @param {string} alias - Alias name to delete * @returns {object} Result with success status */ -function deleteAlias(alias) { +function deleteAliasUnlocked(alias) { const data = loadAliases(); if (!data.aliases[alias]) { @@ -288,7 +294,7 @@ function deleteAlias(alias) { const deleted = data.aliases[alias]; delete data.aliases[alias]; - if (saveAliases(data)) { + if (saveAliasesUnlocked(data)) { return { success: true, alias, @@ -305,7 +311,7 @@ function deleteAlias(alias) { * @param {string} newAlias - New alias name * @returns {object} Result with success status */ -function renameAlias(oldAlias, newAlias) { +function renameAliasUnlocked(oldAlias, newAlias) { const data = loadAliases(); if (!data.aliases[oldAlias]) { @@ -340,7 +346,7 @@ function renameAlias(oldAlias, newAlias) { aliasData.updatedAt = new Date().toISOString(); data.aliases[newAlias] = aliasData; - if (saveAliases(data)) { + if (saveAliasesUnlocked(data)) { return { success: true, oldAlias, @@ -349,12 +355,8 @@ function renameAlias(oldAlias, newAlias) { }; } - // Restore old alias and remove new alias on failure - data.aliases[oldAlias] = aliasData; - delete data.aliases[newAlias]; - // Attempt to persist the rollback - saveAliases(data); - return { success: false, error: 'Failed to save renamed alias — rolled back to original' }; + // Atomic replacement leaves the original snapshot intact on failure. + return { success: false, error: 'Failed to save renamed alias' }; } /** @@ -379,7 +381,7 @@ function resolveSessionAlias(aliasOrId) { * @param {string|null} title - New title (string or null to clear) * @returns {object} Result with success status */ -function updateAliasTitle(alias, title) { +function updateAliasTitleUnlocked(alias, title) { if (title !== null && typeof title !== 'string') { return { success: false, error: 'Title must be a string or null' }; } @@ -393,7 +395,7 @@ function updateAliasTitle(alias, title) { data.aliases[alias].title = title || null; data.aliases[alias].updatedAt = new Date().toISOString(); - if (saveAliases(data)) { + if (saveAliasesUnlocked(data)) { return { success: true, alias, @@ -431,7 +433,7 @@ function getAliasesForSession(sessionPath) { * @param {Function} sessionExists - Function to check if session exists * @returns {object} Cleanup result */ -function cleanupAliases(sessionExists) { +function cleanupAliasesUnlocked(sessionExists) { if (typeof sessionExists !== 'function') { return { totalChecked: 0, removed: 0, removedAliases: [], error: 'sessionExists must be a function' }; } @@ -446,7 +448,7 @@ function cleanupAliases(sessionExists) { } } - if (removed.length > 0 && !saveAliases(data)) { + if (removed.length > 0 && !saveAliasesUnlocked(data)) { log('[Aliases] Failed to save after cleanup'); return { success: false, @@ -465,6 +467,31 @@ function cleanupAliases(sessionExists) { }; } +function setAlias(alias, sessionPath, title = null) { + return withAliasesLock(() => setAliasUnlocked(alias, sessionPath, title), + { success: false, error: 'Failed to save alias' }); +} + +function deleteAlias(alias) { + return withAliasesLock(() => deleteAliasUnlocked(alias), + { success: false, error: 'Failed to delete alias' }); +} + +function renameAlias(oldAlias, newAlias) { + return withAliasesLock(() => renameAliasUnlocked(oldAlias, newAlias), + { success: false, error: 'Failed to save renamed alias' }); +} + +function updateAliasTitle(alias, title) { + return withAliasesLock(() => updateAliasTitleUnlocked(alias, title), + { success: false, error: 'Failed to update alias title' }); +} + +function cleanupAliases(sessionExists) { + return withAliasesLock(() => cleanupAliasesUnlocked(sessionExists), + { success: false, totalChecked: 0, removed: 0, removedAliases: [], error: 'Failed to save after cleanup' }); +} + module.exports = { getAliasesPath, loadAliases, diff --git a/tests/lib/claude-settings-lock.test.js b/tests/lib/claude-settings-lock.test.js new file mode 100644 index 000000000..905255804 --- /dev/null +++ b/tests/lib/claude-settings-lock.test.js @@ -0,0 +1,148 @@ +'use strict'; +const assert = require('assert'); +const fs = require('fs'); +const os = require('os'); +const path = require('path'); +const { spawn } = require('child_process'); +const { acquireSettingsLock } = require('../../scripts/lib/install/claude-settings-lock'); + +async function owner(root, settingsPath) { + const child = spawn(process.execPath, [ + path.join(__dirname, 'helpers', 'settings-lock-worker.js'), root, settingsPath, + ], { env: { ...process.env, HOME: root, USERPROFILE: root, ECC_AGENT_DATA_HOME: path.join(root, '.claude') }, + stdio: ['ignore', 'ignore', 'pipe'] }); + let stderr = ''; + child.stderr.on('data', data => { stderr += data; }); + child.done = new Promise((resolve, reject) => { + child.on('error', reject); + child.on('close', (code, signal) => resolve({ code, signal, stderr })); + }); + child.done.catch(() => {}); + const deadline = Date.now() + 10000; + try { + while (!fs.existsSync(path.join(root, 'held'))) { + if (child.exitCode !== null || Date.now() > deadline) throw new Error('Owner failed: ' + stderr); + await new Promise(resolve => setTimeout(resolve, 10)); + } + return child; + } catch (error) { + child.kill('SIGKILL'); + await child.done; + throw error; + } +} +async function withOwner(callback) { + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'ecc-settings-wait-')); + const settingsPath = path.join(root, '.claude', 'session-aliases.json'); + let child; + try { + child = await owner(root, settingsPath); + await callback(root, settingsPath, child); + } finally { + if (child) { + if (child.exitCode === null && child.signalCode === null) child.kill('SIGKILL'); + await child.done; + } + fs.rmSync(root, { recursive: true, force: true }); + } +} +const cases = [ + ['default contention is fail-fast; optional timeout is bounded and leaves owner intact', () => withOwner((_root, file) => { + const before = fs.readFileSync(file + '.ecc.lock', 'utf8'); + assert.throws(() => acquireSettingsLock(file), /Another ECC process/); + const start = performance.now(); + assert.throws(() => acquireSettingsLock(file, { timeoutMs: 50 }), /Another ECC process/); + assert.ok(performance.now() - start >= 40, 'Waits for the configured deadline'); + assert.ok(performance.now() - start < 5000, 'Does not wait indefinitely'); + assert.strictEqual(fs.readFileSync(file + '.ecc.lock', 'utf8'), before); + assert.deepStrictEqual(fs.readdirSync(path.dirname(file)), ['session-aliases.json.ecc.lock']); + })], + ['waiting succeeds when the real owner releases after the first contention', () => withOwner(async (root, file, child) => { + const originalLink = fs.linkSync; + let contended = false; + fs.linkSync = function (source, target) { + try { return originalLink.apply(this, arguments); } + catch (error) { + if (target === file + '.ecc.lock' && error.code === 'EEXIST') { + contended = true; + fs.writeFileSync(path.join(root, 'release'), ''); + } + throw error; + } + }; + let release; + try { release = acquireSettingsLock(file, { timeoutMs: 2000 }); } + finally { fs.linkSync = originalLink; } + assert.ok(contended); + release(); + const result = await child.done; + assert.strictEqual(result.code, 0, result.stderr); + assert.ok(!fs.existsSync(file + '.ecc.lock')); + })], + ['a killed owner is recovered by the next alias transaction', () => withOwner(async (root, file, child) => { + child.kill('SIGKILL'); + await child.done; + const saved = { HOME: process.env.HOME, USERPROFILE: process.env.USERPROFILE, ECC_AGENT_DATA_HOME: process.env.ECC_AGENT_DATA_HOME }; + Object.assign(process.env, { HOME: root, USERPROFILE: root, ECC_AGENT_DATA_HOME: path.dirname(file) }); + try { + const aliases = require('../../scripts/lib/session-aliases'); + assert.strictEqual(aliases.setAlias('recovered', '/session').success, true); + assert.ok(aliases.loadAliases().aliases.recovered); + assert.deepStrictEqual(fs.readdirSync(path.dirname(file)), ['session-aliases.json']); + } finally { + for (const [key, value] of Object.entries(saved)) { + if (value === undefined) delete process.env[key]; else process.env[key] = value; + } + } + })], + ['non-contention errors propagate without retry', () => withOwner((_root, file) => { + const originalLink = fs.linkSync; + const expected = Object.assign(new Error('disk failure'), { code: 'EIO' }); + let attempts = 0; + fs.linkSync = () => { attempts++; throw expected; }; + try { + assert.throws(() => acquireSettingsLock(file, { timeoutMs: 2000 }), error => error === expected); + assert.strictEqual(attempts, 1); + } finally { fs.linkSync = originalLink; } + })], + ['recovery tolerates a lock disappearing between inspection and quarantine', () => { + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'ecc-lock-disappeared-')); + const file = path.join(root, 'settings.json'); + const lock = file + '.ecc.lock'; + fs.writeFileSync(lock, '{}'); + fs.utimesSync(lock, new Date(0), new Date(0)); + const originalRename = fs.renameSync; + let raced = false; + fs.renameSync = function (source, target) { + if (source === lock && target.includes('.stale-') && !raced) { + raced = true; + fs.unlinkSync(lock); + } + return originalRename.apply(this, arguments); + }; + try { + const release = acquireSettingsLock(file, { timeoutMs: 100 }); + assert.ok(raced); + release(); + assert.deepStrictEqual(fs.readdirSync(root), []); + } finally { + fs.renameSync = originalRename; + fs.rmSync(root, { recursive: true, force: true }); + } + }], + ['invalid timeout is rejected before touching disk', () => { + for (const timeoutMs of [-1, NaN, Infinity, '1']) { + assert.throws(() => acquireSettingsLock('unused', { timeoutMs }), /timeoutMs/); + } + }], +]; +(async () => { + let passed = 0; + let failed = 0; + for (const [name, run] of cases) { + try { await run(); passed++; console.log('PASS ' + name); } + catch (error) { failed++; console.error('FAIL ' + name + ': ' + error.stack); } + } + console.log('Results: Passed: ' + passed + ', Failed: ' + failed); + process.exitCode = failed ? 1 : 0; +})(); diff --git a/tests/lib/helpers/session-alias-worker.js b/tests/lib/helpers/session-alias-worker.js new file mode 100644 index 000000000..ae9cdc7df --- /dev/null +++ b/tests/lib/helpers/session-alias-worker.js @@ -0,0 +1,39 @@ +'use strict'; +const fs = require('fs'); +const path = require('path'); +const [root, role, action] = process.argv.slice(2); +const aliasesPath = path.join(root, '.claude', 'session-aliases.json'); +const pause = new Int32Array(new SharedArrayBuffer(4)); +const mark = name => fs.writeFileSync(path.join(root, name), ''); +const readFileSync = fs.readFileSync; +const linkSync = fs.linkSync; +let paused = false; +// Announce a lock attempt OR a captured read; the parent needs no scheduling guess. +fs.linkSync = function (source, target) { + if (target === aliasesPath + '.ecc.lock') mark(role + '.attempt'); + return linkSync.apply(this, arguments); +}; +fs.readFileSync = function (file) { + const result = readFileSync.apply(this, arguments); + if (file === aliasesPath && !paused) { + paused = true; + mark(role + '.attempt'); + mark(role + '.read'); + const deadline = Date.now() + 15000; + while (!fs.existsSync(path.join(root, role + '.release'))) { + if (Date.now() > deadline) throw new Error('Read barrier timed out'); + Atomics.wait(pause, 0, 0, 10); + } + } + return result; +}; +const aliases = require('../../../scripts/lib/session-aliases'); +const actions = { + set: () => aliases.setAlias('beta', '/beta'), + delete: () => aliases.deleteAlias('existing'), + rename: () => aliases.renameAlias('existing', 'renamed'), + title: () => aliases.updateAliasTitle('existing', 'Updated'), + cleanup: () => aliases.cleanupAliases(session => session !== '/existing'), +}; +const result = role === 'alpha' ? aliases.setAlias('alpha', '/alpha') : actions[action](); +process.stdout.write(JSON.stringify(result)); diff --git a/tests/lib/helpers/settings-lock-worker.js b/tests/lib/helpers/settings-lock-worker.js new file mode 100644 index 000000000..75aad5375 --- /dev/null +++ b/tests/lib/helpers/settings-lock-worker.js @@ -0,0 +1,14 @@ +'use strict'; +const fs = require('fs'); +const path = require('path'); +const { acquireSettingsLock } = require('../../../scripts/lib/install/claude-settings-lock'); +const [root, settingsPath] = process.argv.slice(2); +const release = acquireSettingsLock(settingsPath); +fs.writeFileSync(path.join(root, 'held'), ''); +const deadline = Date.now() + 15000; +const sleeper = new Int32Array(new SharedArrayBuffer(4)); +while (!fs.existsSync(path.join(root, 'release'))) { + if (Date.now() > deadline) throw new Error('Owner release barrier timed out'); + Atomics.wait(sleeper, 0, 0, 10); +} +release(); diff --git a/tests/lib/session-aliases-concurrency.test.js b/tests/lib/session-aliases-concurrency.test.js new file mode 100644 index 000000000..dcec7b30a --- /dev/null +++ b/tests/lib/session-aliases-concurrency.test.js @@ -0,0 +1,87 @@ +'use strict'; +const assert = require('assert'); +const fs = require('fs'); +const os = require('os'); +const path = require('path'); +const { spawn } = require('child_process'); +const worker = path.join(__dirname, 'helpers', 'session-alias-worker.js'); + +async function waitFor(file, children) { + const deadline = Date.now() + 10000; + while (!fs.existsSync(file)) { + for (const child of children) { + if (child.exitCode !== null || child.signalCode !== null) throw new Error('Child exited before barrier: ' + file); + } + if (Date.now() > deadline) throw new Error('Barrier timed out: ' + file); + await new Promise(resolve => setTimeout(resolve, 10)); + } +} +function start(root, role, action) { + const child = spawn(process.execPath, [worker, root, role, action], { + env: { ...process.env, HOME: root, USERPROFILE: root, ECC_AGENT_DATA_HOME: path.join(root, '.claude') }, + stdio: ['ignore', 'pipe', 'pipe'], + }); + let stdout = ''; + let stderr = ''; + child.stdout.on('data', data => { stdout += data; }); + child.stderr.on('data', data => { stderr += data; }); + child.done = new Promise((resolve, reject) => { + child.on('error', reject); + child.on('close', (code, signal) => { + if (code !== 0) reject(new Error('Worker failed: ' + code + '/' + signal + ' ' + stderr)); + else { + try { resolve(JSON.parse(stdout)); } catch (error) { reject(error); } + } + }); + }); + child.done.catch(() => {}); + return child; +} +async function race(action) { + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'ecc-alias-race-')); + const dataDir = path.join(root, '.claude'); + fs.mkdirSync(dataDir); + const aliasesPath = path.join(dataDir, 'session-aliases.json'); + fs.writeFileSync(aliasesPath, JSON.stringify({ + version: '1.0', + aliases: { existing: { sessionPath: '/existing', createdAt: '2026-01-01', title: 'Original' } }, + metadata: {}, + })); + const children = []; + const timer = setTimeout(() => children.forEach(child => child.kill('SIGKILL')), 20000); + try { + const alpha = start(root, 'alpha', action); + children.push(alpha); + await waitFor(path.join(root, 'alpha.read'), children); + const beta = start(root, 'beta', action); + children.push(beta); + await waitFor(path.join(root, 'beta.attempt'), children); + fs.writeFileSync(path.join(root, 'alpha.release'), ''); + assert.strictEqual((await alpha.done).success, true); + fs.writeFileSync(path.join(root, 'beta.release'), ''); + assert.strictEqual((await beta.done).success, true); + const data = JSON.parse(fs.readFileSync(aliasesPath, 'utf8')); + assert.ok(data.aliases.alpha, action + ' must preserve the preceding writer'); + if (action === 'set') assert.ok(data.aliases.beta); + if (['delete', 'cleanup', 'rename'].includes(action)) assert.ok(!data.aliases.existing); + if (action === 'rename') assert.ok(data.aliases.renamed); + if (action === 'title') assert.strictEqual(data.aliases.existing.title, 'Updated'); + assert.strictEqual(data.metadata.totalCount, Object.keys(data.aliases).length); + assert.deepStrictEqual(fs.readdirSync(dataDir), ['session-aliases.json']); + } finally { + clearTimeout(timer); + for (const child of children) { + if (child.exitCode === null && child.signalCode === null) child.kill('SIGKILL'); + } + await Promise.allSettled(children.map(child => child.done)); + fs.rmSync(root, { recursive: true, force: true }); + } +} +(async () => { + let failed = 0; + for (const action of ['set', 'delete', 'rename', 'title', 'cleanup']) { + try { await race(action); console.log('PASS concurrent ' + action); } + catch (error) { failed++; console.error('FAIL concurrent ' + action + ': ' + error.message); } + } + process.exitCode = failed ? 1 : 0; +})(); diff --git a/tests/lib/session-aliases.test.js b/tests/lib/session-aliases.test.js index 0b2554cb1..f2593b033 100644 --- a/tests/lib/session-aliases.test.js +++ b/tests/lib/session-aliases.test.js @@ -18,6 +18,8 @@ const tmpHome = path.join(os.tmpdir(), `ecc-alias-test-${Date.now()}`); fs.mkdirSync(path.join(tmpHome, '.claude'), { recursive: true }); const origHome = process.env.HOME; const origUserProfile = process.env.USERPROFILE; +const origAgentDataHome = process.env.ECC_AGENT_DATA_HOME; +delete process.env.ECC_AGENT_DATA_HOME; process.env.HOME = tmpHome; process.env.USERPROFILE = tmpHome; // Windows: os.homedir() uses USERPROFILE @@ -767,18 +769,16 @@ function runTests() { assert.strictEqual(resolved.title, 'Original Title'); })) passed++; else failed++; - // ── Round 33: saveAliases backup restoration ── - console.log('\nsaveAliases backup/restore (Round 33):'); + // ── Round 33: saveAliases atomic publication ── + console.log('\nsaveAliases atomic publication (Round 33):'); - if (test('saveAliases creates backup before write and removes on success', () => { + if (test('saveAliases publishes a snapshot without leaving staging files', () => { resetAliases(); aliases.setAlias('backup-test', '/path/backup'); - // After successful save, .bak file should NOT exist - const aliasesPath = path.join(tmpHome, '.claude', 'session-aliases.json'); - const backupPath = aliasesPath + '.bak'; - assert.ok(!fs.existsSync(backupPath), 'Backup should be removed after successful save'); + const aliasesPath = aliases.getAliasesPath(); assert.ok(fs.existsSync(aliasesPath), 'Main aliases file should exist'); + assert.deepStrictEqual(fs.readdirSync(path.dirname(aliasesPath)), ['session-aliases.json']); })) passed++; else failed++; if (test('saveAliases with non-serializable data returns false and preserves existing file', () => { @@ -795,7 +795,7 @@ function runTests() { const result = aliases.saveAliases(circular); assert.strictEqual(result, false, 'Should return false'); - // The file should still have the old content (restored from backup or untouched) + // The file should still have the old content (untouched) const contentAfter = fs.readFileSync(aliasesPath, 'utf8'); assert.ok(contentAfter.includes('before-fail'), 'Original aliases data should be preserved after failed save'); @@ -843,10 +843,10 @@ function runTests() { assert.strictEqual(data.metadata.totalCount, 5, 'Metadata count should match actual aliases'); })) passed++; else failed++; - // ── Round 56: Windows platform unlink-before-rename code path ── + // ── Round 56: Windows platform atomic replacement ── console.log('\nRound 56: Windows platform atomic write path:'); - if (test('Windows platform mock: unlinks existing file before rename', () => { + if (test('Windows platform mock: replaces existing file without unlinking it', () => { resetAliases(); // First create an alias so the file exists const r1 = aliases.setAlias('win-initial', '2026-01-01-abc123-session.tmp'); @@ -854,12 +854,17 @@ function runTests() { const aliasesPath = aliases.getAliasesPath(); assert.ok(fs.existsSync(aliasesPath), 'Aliases file should exist before win32 test'); - // Mock process.platform to 'win32' to trigger the unlink-before-rename path + // Check that Windows uses replacement without deleting the published file + const originalUnlink = fs.unlinkSync; + fs.unlinkSync = function (file) { + assert.notStrictEqual(file, aliasesPath, 'Never unlink the published aliases file'); + return originalUnlink.apply(this, arguments); + }; const origPlatform = Object.getOwnPropertyDescriptor(process, 'platform'); Object.defineProperty(process, 'platform', { value: 'win32', configurable: true }); try { - // This save triggers the Windows code path: unlink existing → rename temp + // Replace the snapshot while preserving the original until publication const r2 = aliases.setAlias('win-updated', '2026-02-01-def456-session.tmp'); assert.strictEqual(r2.success, true, 'setAlias should succeed under win32 mock'); @@ -875,6 +880,7 @@ function runTests() { assert.ok(!fs.existsSync(aliasesPath + '.tmp'), 'No temp file should remain'); assert.ok(!fs.existsSync(aliasesPath + '.bak'), 'No backup file should remain'); } finally { + fs.unlinkSync = originalUnlink; // Restore original platform descriptor if (origPlatform) { Object.defineProperty(process, 'platform', origPlatform); @@ -1091,9 +1097,10 @@ function runTests() { assert.strictEqual(result.success, false, 'Should fail when save is blocked'); assert.ok(result.error.includes('Failed to save after cleanup'), `Should return cleanup save failure error, got: ${result.error}`); - assert.strictEqual(result.removed, 1, 'Should report 1 removed alias'); - assert.ok(result.removedAliases.some(a => a.name === 'remove-me'), - 'Should report remove-me in removedAliases'); + assert.strictEqual(result.removed, 0, 'No cleanup runs without the transaction lock'); + assert.deepStrictEqual(result.removedAliases, []); + assert.strictEqual(result.totalChecked, 0); + assert.ok(freshAliases.resolveAlias('remove-me'), 'Published aliases stay unchanged'); } finally { try { fs.chmodSync(isoClaudeDir, 0o755); } catch { /* best-effort */ } process.env.HOME = savedHome; @@ -1210,53 +1217,34 @@ function runTests() { resetAliases(); })) passed++; else failed++; - // ── Round 90: saveAliases backup restore double failure (inner catch restoreErr) ── - console.log('\nRound 90: saveAliases (backup restore double failure):'); - - if (test('saveAliases triggers inner restoreErr catch when both save and restore fail', () => { - // session-aliases.js lines 131-137: When saveAliases fails (outer catch), - // it tries to restore from backup. If the restore ALSO fails, the inner - // catch at line 135 logs restoreErr. No existing test creates this double-fault. - if (process.platform === 'win32' || process.getuid?.() === 0) { - console.log(' (skipped — chmod ineffective on Windows/root)'); - return; - } - const isoHome = path.join(os.tmpdir(), `ecc-r90-restore-fail-${Date.now()}`); - const claudeDir = path.join(isoHome, '.claude'); - fs.mkdirSync(claudeDir, { recursive: true }); - - // Pre-create a backup file while directory is still writable - const backupPath = path.join(claudeDir, 'session-aliases.json.bak'); - fs.writeFileSync(backupPath, JSON.stringify({ aliases: {}, version: '1.0' })); - - // Make .claude directory read-only (0o555): - // 1. writeFileSync(tempPath) → EACCES (can't create file in read-only dir) — outer catch - // 2. copyFileSync(backupPath, aliasesPath) → EACCES (can't create target) — inner catch (line 135) - fs.chmodSync(claudeDir, 0o555); - - const origH = process.env.HOME; - const origP = process.env.USERPROFILE; - process.env.HOME = isoHome; - process.env.USERPROFILE = isoHome; - + // Failed publication must preserve the original bytes, even under win32. + if (test('failed replacement preserves published aliases and removes only its staging file', () => { + resetAliases(); + aliases.setAlias('preserved', '/original'); + const aliasesPath = aliases.getAliasesPath(); + const before = fs.readFileSync(aliasesPath, 'utf8'); + const originalRename = fs.renameSync; + const staged = []; + fs.renameSync = function (source, target) { + if (target === aliasesPath) { + staged.push(source); + const error = new Error('replacement denied'); + error.code = 'EPERM'; + throw error; + } + return originalRename.apply(this, arguments); + }; try { - delete require.cache[require.resolve('../../scripts/lib/session-aliases')]; - delete require.cache[require.resolve('../../scripts/lib/utils')]; - const freshAliases = require('../../scripts/lib/session-aliases'); - - const result = freshAliases.saveAliases({ aliases: { x: 1 }, version: '1.0' }); - assert.strictEqual(result, false, 'Should return false when save fails'); - - // Backup should still exist (restore also failed, so backup was not consumed) - assert.ok(fs.existsSync(backupPath), 'Backup should still exist after double failure'); + assert.strictEqual(aliases.saveAliases({ aliases: { replacement: { sessionPath: '/new' } } }), false); + assert.strictEqual(aliases.renameAlias('preserved', 'changed').success, false); + assert.strictEqual(fs.readFileSync(aliasesPath, 'utf8'), before); + assert.strictEqual(staged.length, 2, 'Each failed operation attempts one publication'); + assert.notStrictEqual(staged[0], staged[1], 'Each write has its own staging file'); + assert.deepStrictEqual(fs.readdirSync(path.dirname(aliasesPath)), ['session-aliases.json']); } finally { - process.env.HOME = origH; - process.env.USERPROFILE = origP; - delete require.cache[require.resolve('../../scripts/lib/session-aliases')]; - delete require.cache[require.resolve('../../scripts/lib/utils')]; - try { fs.chmodSync(claudeDir, 0o755); } catch { /* best-effort */ } - fs.rmSync(isoHome, { recursive: true, force: true }); + fs.renameSync = originalRename; } + assert.strictEqual(aliases.setAlias('after-failure', '/next').success, true, 'Lock is released after failure'); })) passed++; else failed++; // ── Round 95: renameAlias with same old and new name (self-rename) ── @@ -1809,6 +1797,78 @@ function runTests() { 'Object.keys includes normal alias'); })) passed++; else failed++; + if (test('cleanup rejects reentrant writes without losing the outer transaction', () => { + resetAliases(); + aliases.setAlias('existing', '/existing'); + const nested = []; + const result = aliases.cleanupAliases(() => { + nested.push(aliases.setAlias('nested', '/nested')); + nested.push(aliases.deleteAlias('existing')); + nested.push(aliases.renameAlias('existing', 'renamed')); + nested.push(aliases.updateAliasTitle('existing', 'Nested')); + nested.push(aliases.cleanupAliases(() => true)); + nested.push(aliases.saveAliases({ aliases: {} })); + return false; + }); + assert.strictEqual(result.success, true); + assert.ok(nested.slice(0, -1).every(value => value.success === false)); + assert.strictEqual(nested.at(-1), false); + assert.deepStrictEqual(aliases.loadAliases().aliases, {}); + assert.strictEqual(aliases.setAlias('after-cleanup', '/next').success, true); + })) passed++; else failed++; + + if (test('cleanup preserves callback errors and releases its lock', () => { + resetAliases(); + aliases.setAlias('existing', '/existing'); + const primary = new Error('session lookup failed'); + assert.throws(() => aliases.cleanupAliases(() => { throw primary; }), error => error === primary); + assert.ok(aliases.resolveAlias('existing')); + assert.ok(!fs.existsSync(aliases.getAliasesPath() + '.ecc.lock')); + assert.strictEqual(aliases.setAlias('after-error', '/next').success, true); + })) passed++; else failed++; + + if (test('cleanup propagates arbitrary thrown values even if lock release also fails', () => { + resetAliases(); + aliases.setAlias('existing', '/existing'); + const lockPath = aliases.getAliasesPath() + '.ecc.lock'; + const originalRename = fs.renameSync; + const errors = [null, undefined, false, 0, Object.freeze(new Error('frozen')), + new Proxy(new Error('proxy'), { set() { throw new Error('Cannot annotate error'); } })]; + for (const failRelease of [false, true]) { + for (const primary of errors) { + let caught = false; + fs.renameSync = function (source) { + if (failRelease && source === lockPath) throw new Error('release denied'); + return originalRename.apply(this, arguments); + }; + try { + aliases.cleanupAliases(() => { throw primary; }); + } catch (error) { + caught = true; + assert.strictEqual(error, primary, 'Original thrown value is preserved'); + } finally { + fs.renameSync = originalRename; + fs.rmSync(lockPath, { force: true }); + } + assert.ok(caught, 'Callback failure must propagate'); + } + } + assert.strictEqual(aliases.setAlias('after-release-error', '/next').success, true); + })) passed++; else failed++; + + if (test('saveAliases replaces its complete snapshot without mutating the caller', () => { + resetAliases(); + aliases.setAlias('existing', '/existing'); + const snapshot = { aliases: { replacement: { sessionPath: '/replacement' } }, metadata: { caller: true } }; + const before = JSON.stringify(snapshot); + assert.strictEqual(aliases.saveAliases(snapshot), true); + assert.strictEqual(JSON.stringify(snapshot), before); + assert.deepStrictEqual(Object.keys(aliases.loadAliases().aliases), ['replacement']); + })) passed++; else failed++; + + if (origAgentDataHome === undefined) delete process.env.ECC_AGENT_DATA_HOME; + else process.env.ECC_AGENT_DATA_HOME = origAgentDataHome; + // Cleanup — restore both HOME and USERPROFILE (Windows) process.env.HOME = origHome; if (origUserProfile !== undefined) { From 184c657788d3467b2d673dcb0a92e1c64d128d74 Mon Sep 17 00:00:00 2001 From: Yann Roberto <1922498827@qq.com> Date: Thu, 1 Oct 2026 14:59:29 +0800 Subject: [PATCH 02/26] fix(sessions): preserve committed results on lock cleanup errors --- scripts/lib/install/claude-settings-lock.js | 9 +++ scripts/lib/session-aliases.d.ts | 5 +- scripts/lib/session-aliases.js | 51 +++++++++++-- tests/lib/session-aliases.test.js | 80 +++++++++++++++++++++ 4 files changed, 138 insertions(+), 7 deletions(-) diff --git a/scripts/lib/install/claude-settings-lock.js b/scripts/lib/install/claude-settings-lock.js index 88c72580f..d258d00cb 100644 --- a/scripts/lib/install/claude-settings-lock.js +++ b/scripts/lib/install/claude-settings-lock.js @@ -141,6 +141,15 @@ function recoverSettingsLock(lockPath, label = 'Claude settings') { } } +/** + * Acquire exclusive settings access, optionally waiting for another writer. + * A recovery guard (`.ecc.lock.recover`) is never removed by a contender: if its + * owner crashes during recovery, inspect and remove the orphaned guard manually + * only after confirming no ECC process is active. + * @param {string} settingsPath - Settings file protected by the lock + * @param {object} options - Diagnostic label and bounded wait duration + * @returns {Function} Release callback for the acquired lock + */ function acquireSettingsLock(settingsPath, { label = 'Claude settings', timeoutMs = 0 } = {}) { if (!Number.isFinite(timeoutMs) || timeoutMs < 0) { throw new TypeError('Lock timeoutMs must be a finite non-negative number.'); diff --git a/scripts/lib/session-aliases.d.ts b/scripts/lib/session-aliases.d.ts index 3930dc722..87d4da87d 100644 --- a/scripts/lib/session-aliases.d.ts +++ b/scripts/lib/session-aliases.d.ts @@ -84,8 +84,9 @@ export function getAliasesPath(): string; export function loadAliases(): AliasStore; /** - * Save aliases to disk with atomic write (temp file + rename). - * Creates backup before writing; restores on failure. + * Atomically replace the complete snapshot under the alias transaction lock. + * A failed publication leaves the previous snapshot intact. This does not merge + * snapshots loaded before locking; use the mutation functions for updates. */ export function saveAliases(aliases: AliasStore): boolean; diff --git a/scripts/lib/session-aliases.js b/scripts/lib/session-aliases.js index b57664472..8abe38ab8 100644 --- a/scripts/lib/session-aliases.js +++ b/scripts/lib/session-aliases.js @@ -85,6 +85,16 @@ function loadAliases() { // by the outer transaction's snapshot. const activeWrites = new Set(); +/** + * Run a synchronous alias transaction with exclusive access to its snapshot. + * Lock acquisition or reentry failure returns the caller's failure value. + * Release errors are logged without replacing the callback's result or error: + * a successfully published snapshot remains committed even if cleanup fails. + * @template T + * @param {function(): T} callback - Transaction to execute while holding the lock + * @param {T} failure - Result when the transaction cannot start + * @returns {T} The transaction result, or failure if it could not start + */ function withAliasesLock(callback, failure) { const aliasesPath = path.resolve(getAliasesPath()); if (activeWrites.has(aliasesPath)) { @@ -118,17 +128,18 @@ function withAliasesLock(callback, failure) { } activeWrites.delete(aliasesPath); } - if (callbackFailed) { - if (releaseError) log(`[Aliases] Error releasing aliases lock: ${releaseError.message}`); - throw primaryError; - } if (releaseError) { log(`[Aliases] Error releasing aliases lock: ${releaseError.message}`); - return failure; } + if (callbackFailed) throw primaryError; return result; } +/** + * Publish a complete alias snapshot while the caller holds the transaction lock. + * @param {object} aliases - Snapshot to publish without mutating the caller + * @returns {boolean} Whether atomic replacement succeeded + */ function saveAliasesUnlocked(aliases) { try { const snapshot = { @@ -467,26 +478,56 @@ function cleanupAliasesUnlocked(sessionExists) { }; } +/** + * Create or update an alias using the latest snapshot under the transaction lock. + * @param {string} alias - Alias name (alphanumeric, dash, underscore) + * @param {string} sessionPath - Session directory path + * @param {string|null} title - Optional title for the alias + * @returns {object} Result with success status and the saved alias details + */ function setAlias(alias, sessionPath, title = null) { return withAliasesLock(() => setAliasUnlocked(alias, sessionPath, title), { success: false, error: 'Failed to save alias' }); } +/** + * Delete an alias without overwriting other writers' completed changes. + * @param {string} alias - Alias name to delete + * @returns {object} Result with success status and the deleted session path + */ function deleteAlias(alias) { return withAliasesLock(() => deleteAliasUnlocked(alias), { success: false, error: 'Failed to delete alias' }); } +/** + * Rename an existing alias in one locked read-modify-write transaction. + * @param {string} oldAlias - Current alias name + * @param {string} newAlias - New unused alias name + * @returns {object} Result with success status and the renamed alias details + */ function renameAlias(oldAlias, newAlias) { return withAliasesLock(() => renameAliasUnlocked(oldAlias, newAlias), { success: false, error: 'Failed to save renamed alias' }); } +/** + * Update an existing alias title using the latest locked snapshot. + * @param {string} alias - Alias name + * @param {string|null} title - New title, or null to clear it + * @returns {object} Result with success status and the updated title + */ function updateAliasTitle(alias, title) { return withAliasesLock(() => updateAliasTitleUnlocked(alias, title), { success: false, error: 'Failed to update alias title' }); } +/** + * Remove aliases whose sessions no longer exist in a single locked transaction. + * The synchronous callback must not write aliases; its exceptions propagate. + * @param {function(string): boolean} sessionExists - Check whether a session exists + * @returns {object} Checked count, removed aliases, and any operation failure + */ function cleanupAliases(sessionExists) { return withAliasesLock(() => cleanupAliasesUnlocked(sessionExists), { success: false, totalChecked: 0, removed: 0, removedAliases: [], error: 'Failed to save after cleanup' }); diff --git a/tests/lib/session-aliases.test.js b/tests/lib/session-aliases.test.js index f2593b033..3ab5fa35c 100644 --- a/tests/lib/session-aliases.test.js +++ b/tests/lib/session-aliases.test.js @@ -1856,6 +1856,86 @@ function runTests() { assert.strictEqual(aliases.setAlias('after-release-error', '/next').success, true); })) passed++; else failed++; + if (test('successful writes keep their results when releasing the lock fails', () => { + const cases = [ + { + run: () => aliases.setAlias('added', '/added', 'Added'), + expected: { success: true, isNew: true, alias: 'added', sessionPath: '/added', title: 'Added' }, + verify: data => assert.strictEqual(data.added.sessionPath, '/added') + }, + { + run: () => aliases.deleteAlias('existing'), + expected: { success: true, alias: 'existing', deletedSessionPath: '/existing' }, + verify: data => assert.deepStrictEqual(data, {}) + }, + { + run: () => aliases.renameAlias('existing', 'renamed'), + expected: { success: true, oldAlias: 'existing', newAlias: 'renamed', sessionPath: '/existing' }, + verify: data => assert.deepStrictEqual(Object.keys(data), ['renamed']) + }, + { + run: () => aliases.updateAliasTitle('existing', 'Updated'), + expected: { success: true, alias: 'existing', title: 'Updated' }, + verify: data => assert.strictEqual(data.existing.title, 'Updated') + }, + { + run: () => aliases.cleanupAliases(() => false), + expected: { success: true, totalChecked: 1, removed: 1, + removedAliases: [{ name: 'existing', sessionPath: '/existing' }] }, + verify: data => assert.deepStrictEqual(data, {}) + }, + { + run: () => aliases.saveAliases({ aliases: { replacement: { sessionPath: '/replacement' } } }), + expected: true, + verify: data => assert.deepStrictEqual(data, { replacement: { sessionPath: '/replacement' } }) + } + ]; + for (const entry of cases) { + resetAliases(); + aliases.setAlias('existing', '/existing'); + const lockPath = aliases.getAliasesPath() + '.ecc.lock'; + const originalRename = fs.renameSync; + const originalError = console.error; + const warnings = []; + try { + fs.renameSync = function (source) { + if (source === lockPath) throw new Error('release denied'); + return originalRename.apply(this, arguments); + }; + console.error = message => warnings.push(message); + assert.deepStrictEqual(entry.run(), entry.expected); + entry.verify(aliases.loadAliases().aliases); + assert.ok(warnings.some(message => message.includes('release denied')), + 'A release failure remains visible to the caller'); + } finally { + fs.renameSync = originalRename; + console.error = originalError; + fs.rmSync(lockPath, { force: true }); + } + assert.strictEqual(aliases.setAlias('after-release-error', '/next').success, true, + 'The local reentry guard is cleared after a release failure'); + } + })) passed++; else failed++; + + if (test('a release failure preserves the original operation failure result', () => { + resetAliases(); + aliases.setAlias('existing', '/existing'); + const lockPath = aliases.getAliasesPath() + '.ecc.lock'; + const originalRename = fs.renameSync; + try { + fs.renameSync = function (source) { + if (source === lockPath) throw new Error('release denied'); + return originalRename.apply(this, arguments); + }; + assert.deepStrictEqual(aliases.deleteAlias('missing'), + { success: false, error: "Alias 'missing' not found" }); + assert.ok(aliases.resolveAlias('existing')); + } finally { + fs.renameSync = originalRename; + fs.rmSync(lockPath, { force: true }); + } + })) passed++; else failed++; + if (test('saveAliases replaces its complete snapshot without mutating the caller', () => { resetAliases(); aliases.setAlias('existing', '/existing'); From 3ebc9bd3d2de3e2754452055dc45b24301459e09 Mon Sep 17 00:00:00 2001 From: Karim Farahat <47497813+karimfarahat@users.noreply.github.com> Date: Fri, 2 Oct 2026 15:05:45 +0400 Subject: [PATCH 03/26] test: isolate HOME in state-store CLI tests The status CLI reconciles install-state discovered under $HOME. The state-store tests spawned it with the developer's real environment, so on any machine with ECC installed the real ~/.claude install was counted as an extra warning and four status/work-items assertions failed (3 attention items instead of 2). CI stays green only because runners have no install. Point HOME and USERPROFILE at an empty temp dir for spawned CLIs, cleaned up on exit. Callers can still override via options.env. Co-Authored-By: Claude Opus 5.5 --- tests/lib/state-store.test.js | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/tests/lib/state-store.test.js b/tests/lib/state-store.test.js index 80ac95ec9..bf0a26c09 100644 --- a/tests/lib/state-store.test.js +++ b/tests/lib/state-store.test.js @@ -38,12 +38,19 @@ function cleanupTempDir(dirPath) { fs.rmSync(dirPath, { recursive: true, force: true }); } +// The status CLI reconciles install-state found under $HOME, so CLI runs get an +// empty home instead of the developer's real ~/.claude. +const ISOLATED_HOME = createTempDir('ecc-state-home-'); +process.on('exit', () => cleanupTempDir(ISOLATED_HOME)); + function runNode(scriptPath, args = [], options = {}) { return spawnSync('node', [scriptPath, ...args], { encoding: 'utf8', cwd: options.cwd || process.cwd(), env: { ...process.env, + HOME: ISOLATED_HOME, + USERPROFILE: ISOLATED_HOME, ...(options.env || {}), }, }); From 5ecca8883e4acc67591d01da4246e2e0d5868146 Mon Sep 17 00:00:00 2001 From: Karim Farahat <47497813+karimfarahat@users.noreply.github.com> Date: Fri, 2 Oct 2026 15:34:17 +0400 Subject: [PATCH 04/26] test: isolate OpenCode config dirs in state-store CLI tests The OpenCode install target resolves its root from OPENCODE_CONFIG_DIR, then XDG_CONFIG_HOME/opencode, before falling back to HOME. Inheriting either variable still let a developer's real OpenCode install-state reach the spawned status CLI and fail the same four tests. Point both into the isolated home. Co-Authored-By: Claude Opus 5.5 --- tests/lib/state-store.test.js | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/tests/lib/state-store.test.js b/tests/lib/state-store.test.js index bf0a26c09..a1e2f045c 100644 --- a/tests/lib/state-store.test.js +++ b/tests/lib/state-store.test.js @@ -38,8 +38,9 @@ function cleanupTempDir(dirPath) { fs.rmSync(dirPath, { recursive: true, force: true }); } -// The status CLI reconciles install-state found under $HOME, so CLI runs get an -// empty home instead of the developer's real ~/.claude. +// The status CLI reconciles install-state found under $HOME and the OpenCode +// config dirs, so CLI runs get an empty home instead of the developer's real +// ~/.claude and ~/.config/opencode. const ISOLATED_HOME = createTempDir('ecc-state-home-'); process.on('exit', () => cleanupTempDir(ISOLATED_HOME)); @@ -51,6 +52,8 @@ function runNode(scriptPath, args = [], options = {}) { ...process.env, HOME: ISOLATED_HOME, USERPROFILE: ISOLATED_HOME, + XDG_CONFIG_HOME: path.join(ISOLATED_HOME, '.config'), + OPENCODE_CONFIG_DIR: path.join(ISOLATED_HOME, '.config', 'opencode'), ...(options.env || {}), }, }); From 00d73fbae241bbea1190f16a1f3afd7b19724de3 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 12:34:14 +0530 Subject: [PATCH 05/26] fix(hooks): honor CLAUDE_CONFIG_DIR instead of writing to ~/.claude Claude Code points every profile-local path at CLAUDE_CONFIG_DIR when it is set, but ECC hooks kept reading and writing ~/.claude. A session launched under a second profile left its bash log, cost tracker, metrics, session data and skill runs in the default profile, where the other one reads them back. Two groups, as reported: - getDefaultClaudeAgentDataHome() in scripts/lib/agent-data-home.js now follows the variable, which covers every hook reaching ~/.claude through resolveAgentDataHome(). ECC_AGENT_DATA_HOME still wins, so the documented workaround keeps working. - plan-canvas-pending, plan-canvas-sessions, mcp-health-check and post-bash-command-log hard-coded os.homedir()/.claude and now resolve the same way, matching scripts/hooks/ecc-statusline.js:68. A blank value is ignored rather than resolved, since path.resolve(' ') returns the process cwd and would scatter profile data through whatever directory the session started in. Fixes #3266 --- scripts/hooks/mcp-health-check.js | 12 ++++++- scripts/hooks/plan-canvas-pending.js | 12 ++++++- scripts/hooks/plan-canvas-sessions.js | 12 ++++++- scripts/hooks/post-bash-command-log.js | 12 ++++++- scripts/lib/agent-data-home.js | 11 +++++++ tests/lib/agent-data-home.test.js | 44 ++++++++++++++++++++++++++ 6 files changed, 99 insertions(+), 4 deletions(-) diff --git a/scripts/hooks/mcp-health-check.js b/scripts/hooks/mcp-health-check.js index 843a6ec04..adcc11540 100644 --- a/scripts/hooks/mcp-health-check.js +++ b/scripts/hooks/mcp-health-check.js @@ -47,11 +47,21 @@ 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. +function claudeConfigDir() { + const configured = process.env.CLAUDE_CONFIG_DIR; + if (configured && String(configured).trim().length > 0) { + return path.resolve(String(configured).trim()); + } + return path.join(os.homedir(), '.claude'); +} + function stateFilePath() { if (process.env.ECC_MCP_HEALTH_STATE_PATH) { return path.resolve(process.env.ECC_MCP_HEALTH_STATE_PATH); } - return path.join(os.homedir(), '.claude', 'mcp-health-cache.json'); + return path.join(claudeConfigDir(), 'mcp-health-cache.json'); } function configPaths() { diff --git a/scripts/hooks/plan-canvas-pending.js b/scripts/hooks/plan-canvas-pending.js index 9ba2a9207..4e97bddda 100644 --- a/scripts/hooks/plan-canvas-pending.js +++ b/scripts/hooks/plan-canvas-pending.js @@ -34,10 +34,20 @@ 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. +function claudeConfigDir() { + const configured = process.env.CLAUDE_CONFIG_DIR; + if (configured && String(configured).trim().length > 0) { + return path.resolve(String(configured).trim()); + } + return path.join(os.homedir(), '.claude'); +} + function stateDir() { const override = process.env.ECC_PLAN_CANVAS_STATE_DIR; if (override && override.trim()) return path.resolve(override.trim()); - return path.join(os.homedir(), '.claude', 'plan-canvas'); + return path.join(claudeConfigDir(), 'plan-canvas'); } function readState() { diff --git a/scripts/hooks/plan-canvas-sessions.js b/scripts/hooks/plan-canvas-sessions.js index 4e0799f87..009d68a69 100644 --- a/scripts/hooks/plan-canvas-sessions.js +++ b/scripts/hooks/plan-canvas-sessions.js @@ -19,10 +19,20 @@ 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. +function claudeConfigDir() { + const configured = process.env.CLAUDE_CONFIG_DIR; + if (configured && String(configured).trim().length > 0) { + return path.resolve(String(configured).trim()); + } + return path.join(os.homedir(), '.claude'); +} + function stateDir() { const override = process.env.ECC_PLAN_CANVAS_STATE_DIR; if (override && override.trim()) return path.resolve(override.trim()); - return path.join(os.homedir(), '.claude', 'plan-canvas'); + return path.join(claudeConfigDir(), 'plan-canvas'); } function openSessions() { diff --git a/scripts/hooks/post-bash-command-log.js b/scripts/hooks/post-bash-command-log.js index bdea239df..41945b115 100644 --- a/scripts/hooks/post-bash-command-log.js +++ b/scripts/hooks/post-bash-command-log.js @@ -38,6 +38,16 @@ 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. +function claudeConfigDir() { + const configured = process.env.CLAUDE_CONFIG_DIR; + if (configured && String(configured).trim().length > 0) { + return path.resolve(String(configured).trim()); + } + return path.join(os.homedir(), '.claude'); +} + function run(rawInput, mode = 'audit') { const config = MODE_CONFIG[mode]; @@ -45,7 +55,7 @@ function run(rawInput, mode = 'audit') { if (config) { const input = String(rawInput || '').trim() ? JSON.parse(String(rawInput)) : {}; const command = sanitizeCommand(input.tool_input?.command || '?'); - appendLine(path.join(os.homedir(), '.claude', config.fileName), config.format(command)); + appendLine(path.join(claudeConfigDir(), config.fileName), config.format(command)); } } catch { // Logging must never block the calling hook. diff --git a/scripts/lib/agent-data-home.js b/scripts/lib/agent-data-home.js index 7302bd572..8362c31b3 100644 --- a/scripts/lib/agent-data-home.js +++ b/scripts/lib/agent-data-home.js @@ -17,6 +17,7 @@ const path = require('path'); const { assertWithinTrustedRoot } = require('./path-safety'); const AGENT_DATA_HOME_ENV = 'ECC_AGENT_DATA_HOME'; +const CLAUDE_CONFIG_DIR_ENV = 'CLAUDE_CONFIG_DIR'; const DEFAULT_CLAUDE_DIR_NAME = '.claude'; const DEFAULT_CURSOR_ECC_DIR_SEGMENTS = ['.cursor', 'ecc']; const PROJECT_CONFIG_RELATIVE = path.join('.cursor', 'ecc-agent-data.json'); @@ -92,6 +93,15 @@ function getDefaultCursorAgentDataHome() { } function getDefaultClaudeAgentDataHome() { + // Claude Code points every profile-local path at CLAUDE_CONFIG_DIR when it is + // set, so a session launched under a second profile must not keep reading and + // writing the default ~/.claude. This moves the trusted root with it, which is + // the same trust ECC_AGENT_DATA_HOME already gets: both come from whoever + // launched the session, not from the project. + const configDir = process.env[CLAUDE_CONFIG_DIR_ENV]; + if (configDir && String(configDir).trim().length > 0) { + return expandHomePath(String(configDir).trim(), process.cwd()); + } return path.join(getHomeDirFromEnv(), DEFAULT_CLAUDE_DIR_NAME); } @@ -228,6 +238,7 @@ function getCursorSessionEnvPayload(options = {}) { module.exports = { AGENT_DATA_HOME_ENV, + CLAUDE_CONFIG_DIR_ENV, DEFAULT_CLAUDE_DIR_NAME, DEFAULT_CURSOR_ECC_DIR_SEGMENTS, PROJECT_CONFIG_RELATIVE, diff --git a/tests/lib/agent-data-home.test.js b/tests/lib/agent-data-home.test.js index f5f72fc35..8aeac883b 100644 --- a/tests/lib/agent-data-home.test.js +++ b/tests/lib/agent-data-home.test.js @@ -497,6 +497,50 @@ function runTests() { }); })) passed++; else failed++; + if (test('getDefaultClaudeAgentDataHome follows CLAUDE_CONFIG_DIR', () => { + // Claude Code points every profile-local path at CLAUDE_CONFIG_DIR. Ignoring + // it writes this profile's session data, logs and metrics into the default + // ~/.claude, where another profile reads them back. + withIsolatedCwd(() => { + withEnv({ + CLAUDE_CONFIG_DIR: path.join(os.tmpdir(), 'ecc-profile-x'), + ECC_AGENT_DATA_HOME: undefined, + }, () => { + const agentDataHome = require('../../scripts/lib/agent-data-home'); + assert.strictEqual( + agentDataHome.getDefaultClaudeAgentDataHome(), + path.join(os.tmpdir(), 'ecc-profile-x') + ); + }); + }); + })) passed++; else failed++; + + if (test('getDefaultClaudeAgentDataHome falls back to ~/.claude without the env', () => { + withIsolatedCwd(() => { + withEnv({ CLAUDE_CONFIG_DIR: undefined, ECC_AGENT_DATA_HOME: undefined }, () => { + const agentDataHome = require('../../scripts/lib/agent-data-home'); + assert.strictEqual( + agentDataHome.getDefaultClaudeAgentDataHome(), + path.join(os.homedir(), '.claude') + ); + }); + }); + })) passed++; else failed++; + + if (test('a blank CLAUDE_CONFIG_DIR is ignored rather than resolving to cwd', () => { + // An empty or whitespace value would otherwise resolve to the process cwd and + // scatter profile data through whatever directory the session started in. + withIsolatedCwd(() => { + withEnv({ CLAUDE_CONFIG_DIR: ' ', ECC_AGENT_DATA_HOME: undefined }, () => { + const agentDataHome = require('../../scripts/lib/agent-data-home'); + assert.strictEqual( + agentDataHome.getDefaultClaudeAgentDataHome(), + path.join(os.homedir(), '.claude') + ); + }); + }); + })) passed++; else failed++; + console.log(`\n=== Test Results ===\nPassed: ${passed}\nFailed: ${failed}\n`); if (failed > 0) process.exit(1); } From 14d54a928522cbd13da619f02e12765f90426166 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 12:50:23 +0530 Subject: [PATCH 06/26] test(agent-data-home): pin the trusted root that moves with CLAUDE_CONFIG_DIR getDefaultClaudeAgentDataHome() is one of the two roots resolveAllowedProjectConfigHome() accepts, so following CLAUDE_CONFIG_DIR moves a security boundary and not just a default. The case asserts the containment holds at the new location and nowhere else: a path inside the relocated root is accepted, while the old ~/.claude, a traversal out of the new root, and an arbitrary home path are all still refused. It bites. Re-adding ~/.claude as a third allowed root, which is the obvious way to get this wrong, turns the case red. --- tests/lib/agent-data-home.test.js | 53 +++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/tests/lib/agent-data-home.test.js b/tests/lib/agent-data-home.test.js index 8aeac883b..f867042ca 100644 --- a/tests/lib/agent-data-home.test.js +++ b/tests/lib/agent-data-home.test.js @@ -541,6 +541,59 @@ function runTests() { }); })) passed++; else failed++; + if (test('security: the trusted root moves with CLAUDE_CONFIG_DIR but still confines the project config', () => { + // getDefaultClaudeAgentDataHome() is not only a default: it is one of the two + // roots resolveAllowedProjectConfigHome() will accept. Following + // CLAUDE_CONFIG_DIR moves that boundary with the profile, so the containment + // it enforces has to hold at the new location and nowhere else. + const projectDir = fs.mkdtempSync(path.join(os.tmpdir(), 'ecc-agent-data-home-cfgdir-')); + const homeDir = fs.mkdtempSync(path.join(os.tmpdir(), 'ecc-agent-data-home-cfgdir-user-')); + const profileDir = path.join(homeDir, '.claude-profile-x'); + const configPath = path.join(projectDir, '.cursor', 'ecc-agent-data.json'); + fs.mkdirSync(path.dirname(configPath), { recursive: true }); + + try { + withEnv({ + ECC_AGENT_DATA_HOME: undefined, + CLAUDE_CONFIG_DIR: profileDir, + HOME: homeDir, + USERPROFILE: undefined, + }, () => { + const agentDataHome = require('../../scripts/lib/agent-data-home'); + + // Inside the relocated root: accepted. + fs.writeFileSync( + configPath, + JSON.stringify({ agentDataHome: path.join(profileDir, 'nested') }), + 'utf8' + ); + assert.strictEqual( + agentDataHome.readProjectConfigAt(configPath), + path.join(profileDir, 'nested') + ); + + // The old default is no longer a root, and neither is anything outside + // the new one. A traversal out of the relocated root stays rejected. + const outside = [ + path.join(homeDir, '.claude'), + path.join(profileDir, '..', 'escaped'), + path.join(homeDir, 'arbitrary-agent-data'), + ]; + for (const candidate of outside) { + fs.writeFileSync(configPath, JSON.stringify({ agentDataHome: candidate }), 'utf8'); + const { result, messages } = captureConsoleErrors( + () => agentDataHome.readProjectConfigAt(configPath) + ); + assert.strictEqual(result, null, `expected rejection for ${candidate}`); + assert.ok(messages.some(message => message.includes('Ignoring unsafe agent data project config'))); + } + }); + } finally { + fs.rmSync(projectDir, { recursive: true, force: true }); + fs.rmSync(homeDir, { recursive: true, force: true }); + } + })) passed++; else failed++; + console.log(`\n=== Test Results ===\nPassed: ${passed}\nFailed: ${failed}\n`); if (failed > 0) process.exit(1); } 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 07/26] 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 From 3c0297e737bfcbc40d417584380742826afd8717 Mon Sep 17 00:00:00 2001 From: basil-k-aji-dev <70605804+basil-k-aji-dev@users.noreply.github.com> Date: Sun, 4 Oct 2026 12:19:37 +0530 Subject: [PATCH 08/26] test(hooks): assert the SessionStart child exits cleanly The profile-isolation case asserts empty stdout. A hook that crashed before writing is also silent, so it would have passed. --- tests/hooks/claude-config-dir-routing.test.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/hooks/claude-config-dir-routing.test.js b/tests/hooks/claude-config-dir-routing.test.js index 1d6e78fa3..8583cd83a 100644 --- a/tests/hooks/claude-config-dir-routing.test.js +++ b/tests/hooks/claude-config-dir-routing.test.js @@ -205,6 +205,9 @@ function runTests() { k1: { file: '/work/leaked.md', status: 'open', pendingFeedback: [] }, }); const result = runHook('plan-canvas-sessions.js', { home, profile }); + // Silence is the assertion here, and a hook that crashed before writing is + // also silent, so the exit status has to be checked first. + assert.strictEqual(result.status, 0, `SessionStart failed: ${result.stderr}`); assert.strictEqual(result.stdout.trim(), ''); })); From 72d738de3718cdb92eb64185c955cdc7202e01e4 Mon Sep 17 00:00:00 2001 From: Huanyi Xie <74809554+xiehuanyi@users.noreply.github.com> Date: Tue, 6 Oct 2026 19:39:22 +0300 Subject: [PATCH 09/26] fix(proximity): observe uncommitted worktree changes --- docs/design/agent-proximity.md | 5 +++ scripts/lib/control-pane/proximity.js | 19 ++++++--- tests/lib/control-pane-proximity.test.js | 49 ++++++++++++++++++++++++ 3 files changed, 67 insertions(+), 6 deletions(-) diff --git a/docs/design/agent-proximity.md b/docs/design/agent-proximity.md index c9d3e2b96..6aed3faad 100644 --- a/docs/design/agent-proximity.md +++ b/docs/design/agent-proximity.md @@ -149,3 +149,8 @@ the renderer draws), and pairwise `links` with risk (the edges to color). escalation wired to live session diffs. - v2: cross-machine airspace over Tailscale (teammate agents enter the same space); the recorded "N agents, M humans, zero merge conflicts" demo. + +The control pane compares each worktree against the merge base of its declared base +and HEAD. Committed, staged, and unstaged tracked edits carry changed line ranges. +Untracked files that Git does not ignore participate as whole-file working sets. +Changes made only on the base branch after the worktree diverged do not participate. diff --git a/scripts/lib/control-pane/proximity.js b/scripts/lib/control-pane/proximity.js index a958ef36f..1db91c81e 100644 --- a/scripts/lib/control-pane/proximity.js +++ b/scripts/lib/control-pane/proximity.js @@ -45,8 +45,8 @@ function parseDiffRanges(diff) { return byFile; } -function runGitDiff(worktreePath, base, extraArgs) { - return execFileSync('git', ['-C', worktreePath, 'diff', ...extraArgs, `${base}...HEAD`], { +function runGit(worktreePath, args) { + return execFileSync('git', ['-C', worktreePath, ...args], { encoding: 'utf8', timeout: 5000, maxBuffer: 8 * 1024 * 1024, @@ -54,6 +54,15 @@ function runGitDiff(worktreePath, base, extraArgs) { }); } +function runGitDiff(worktreePath, base, extraArgs) { + const mergeBase = runGit(worktreePath, ['merge-base', '--', base, 'HEAD']).trim(); + return runGit(worktreePath, ['diff', ...extraArgs, mergeBase, '--']); +} + +function untrackedFiles(worktreePath) { + return runGit(worktreePath, ['ls-files', '--others', '--exclude-standard', '-z']).split('\0').filter(Boolean); +} + /** * Default working-set source: a session's worktree diff against its base, with * per-file changed line ranges. Returns [{ path, lines? }], or [] on failure so @@ -65,6 +74,7 @@ function defaultWorkingSetFor(session) { const base = wt.base || 'HEAD'; try { const ranges = parseDiffRanges(runGitDiff(wt.path, base, ['--unified=0'])); + for (const file of untrackedFiles(wt.path)) ranges.set(file, []); return [...ranges.entries()].map(([path, lines]) => (lines.length > 0 ? { path, lines } : { path })); } catch { return []; @@ -79,10 +89,7 @@ function defaultChangedFilesFor(session) { if (!wt || !wt.path) return []; const base = wt.base || 'HEAD'; try { - return runGitDiff(wt.path, base, ['--name-only']) - .split('\n') - .map(s => s.trim()) - .filter(Boolean); + return [...new Set([...runGitDiff(wt.path, base, ['--name-only', '-z']).split('\0').filter(Boolean), ...untrackedFiles(wt.path)])]; } catch { return []; } diff --git a/tests/lib/control-pane-proximity.test.js b/tests/lib/control-pane-proximity.test.js index c91603f46..790959c39 100644 --- a/tests/lib/control-pane-proximity.test.js +++ b/tests/lib/control-pane-proximity.test.js @@ -54,6 +54,55 @@ const changedFilesFor = session => 'no-worktree': [] })[session.id] || []; +test('real worktrees expose committed, staged, unstaged and untracked work before commit', () => { + const fs = require('fs'); + const os = require('os'); + const path = require('path'); + const { execFileSync } = require('child_process'); + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'ecc-proximity-worktree-')); + const repo = path.join(dir, 'repo'); + const git = (cwd, ...args) => execFileSync('git', ['-C', cwd, ...args], { stdio: 'pipe' }); + try { + fs.mkdirSync(repo); + git(repo, 'init', '-q', '-b', 'main'); + git(repo, 'config', 'user.name', 'proximity-test'); + git(repo, 'config', 'user.email', 'proximity-test@example.com'); + fs.writeFileSync(path.join(repo, 'shared.js'), 'const value = 1;\n'); + fs.writeFileSync(path.join(repo, '.gitignore'), 'ignored.js\n'); + git(repo, 'add', '.'); + git(repo, 'commit', '-qm', 'base'); + const worktrees = ['a', 'b'].map(name => { + const wt = path.join(dir, name); + git(repo, 'worktree', 'add', '-q', '-b', name, wt, 'main'); + fs.writeFileSync(path.join(wt, 'shared.js'), `const value = '${name}';\n`); + fs.writeFileSync(path.join(wt, 'new.js'), `export const name = '${name}';\n`); + fs.writeFileSync(path.join(wt, 'ignored.js'), 'ignored\n'); + return { id: name, worktree: { path: wt, base: 'main' } }; + }); + git(worktrees[0].worktree.path, 'add', 'shared.js'); + fs.writeFileSync(path.join(repo, 'upstream.js'), 'upstream only\n'); + git(repo, 'add', '.'); + git(repo, 'commit', '-qm', 'advance base'); + const assertWorkingSets = () => { + const agents = sessionsToAgents(worktrees); + assert.strictEqual(agents.length, 2); + for (const agent of agents) { + assert.deepStrictEqual(agent.files.map(f => f.path).sort(), ['new.js', 'shared.js']); + assert.deepStrictEqual(agent.files.find(f => f.path === 'shared.js').lines, [[1, 1]]); + assert.strictEqual(agent.files.find(f => f.path === 'new.js').lines, undefined); + } + const snapshot = buildProximitySnapshot(worktrees, { repoRoot: repo, graph: { adjacency: {} } }); + assert.strictEqual(snapshot.counts.agents, 2); + assert.ok(snapshot.advisories.some(a => a.level === 'resolution')); + }; + assertWorkingSets(); + git(worktrees[0].worktree.path, 'commit', '-qm', 'commit staged edit'); + assertWorkingSets(); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } +}); + test('sessionsToAgents: only worktree sessions with edits participate', () => { const agents = sessionsToAgents(sessions, { changedFilesFor }); assert.deepStrictEqual(agents.map(a => a.agentId).sort(), ['docs-bot', 'lead-hermes', 'worker-kb']); From 990e45e177ef24c8f257398b50305ddc10109c66 Mon Sep 17 00:00:00 2001 From: Huanyi Xie <74809554+xiehuanyi@users.noreply.github.com> Date: Tue, 6 Oct 2026 19:41:18 +0300 Subject: [PATCH 10/26] fix(opencode): avoid duplicate home plugin initialization --- .opencode/README.md | 5 +++++ scripts/lib/install/hook-consent.js | 19 ++++++++++++------- tests/lib/hook-consent.test.js | 4 +++- tests/lib/install-executor.test.js | 6 +++++- 4 files changed, 25 insertions(+), 9 deletions(-) diff --git a/.opencode/README.md b/.opencode/README.md index e239c1361..2d76b7d92 100644 --- a/.opencode/README.md +++ b/.opencode/README.md @@ -241,3 +241,8 @@ selection, and its subagents inherit the invoking primary agent's model. ## License MIT + +Home-target installations keep `plugins/index.ts` inert even with hooks enabled. +OpenCode automatically loads `plugins/ecc-hooks.ts`; loading the barrel as well +would initialize every ECC hook twice. The source and published package barrel +remain available for module imports. diff --git a/scripts/lib/install/hook-consent.js b/scripts/lib/install/hook-consent.js index f954c9016..4876b681e 100644 --- a/scripts/lib/install/hook-consent.js +++ b/scripts/lib/install/hook-consent.js @@ -174,10 +174,6 @@ function withoutOpenCodeHookActivation(operation) { }; } -function transformOpenCodeHookActivationOperations(operations) { - return (Array.isArray(operations) ? operations : []).map(withoutOpenCodeHookActivation); -} - function planSelectsHookRuntime(plan = {}) { return ( Array.isArray(plan.selectedModuleIds) @@ -189,17 +185,26 @@ function planSelectsHookRuntime(plan = {}) { } function disableUnselectedOpenCodeHooks(plan) { - if (plan.target !== 'opencode' || planSelectsHookRuntime(plan)) { + if (plan.target !== 'opencode') { return plan; } + const runtimeSelected = planSelectsHookRuntime(plan); + const transformOperations = operations => (Array.isArray(operations) ? operations : []).map(operation => { + // The source barrel is needed by the published package, but OpenCode scans + // plugins/index.ts and plugins/ecc-hooks.ts independently in home installs. + if (!runtimeSelected || normalizeOperationPath(operation.sourceRelativePath) === '.opencode/plugins/index.ts') { + return withoutOpenCodeHookActivation(operation); + } + return operation; + }); return { ...plan, - operations: transformOpenCodeHookActivationOperations(plan.operations), + operations: transformOperations(plan.operations), statePreview: plan.statePreview ? { ...plan.statePreview, - operations: transformOpenCodeHookActivationOperations(plan.statePreview.operations), + operations: transformOperations(plan.statePreview.operations), } : plan.statePreview, }; diff --git a/tests/lib/hook-consent.test.js b/tests/lib/hook-consent.test.js index d802c5507..9fbade56a 100644 --- a/tests/lib/hook-consent.test.js +++ b/tests/lib/hook-consent.test.js @@ -148,7 +148,9 @@ function runTests() { const pending = withHookConsent(selected, null); assert.throws(() => assertHookConsentReady(pending), /automatic hook runtime/); const enabled = withHookConsent(selected, 'enabled'); - assert.ok(enabled.operations.every(operation => operation.contentTransform === undefined)); + for (const operation of [...enabled.operations, ...enabled.statePreview.operations]) { + assert.strictEqual(operation.contentTransform, operation.sourceRelativePath.endsWith('/index.ts') ? 'opencode-disable-plugin-entrypoint' : undefined); + } assert.doesNotThrow(() => assertHookConsentReady(enabled)); const declined = withHookConsent(selected, 'declined'); assert.strictEqual(declined.operations.length, 2); diff --git a/tests/lib/install-executor.test.js b/tests/lib/install-executor.test.js index d39c14628..1df4889dd 100644 --- a/tests/lib/install-executor.test.js +++ b/tests/lib/install-executor.test.js @@ -990,9 +990,13 @@ function runTests() { for (const name of OPENCODE_ENTRYPOINTS) { assert.strictEqual( fs.readFileSync(path.join(homeDir, '.config', 'opencode', 'plugins', name), 'utf8'), - fs.readFileSync(path.join(REPO_ROOT, '.opencode', 'plugins', name), 'utf8') + name === 'index.ts' ? INERT_OPENCODE_PLUGIN : fs.readFileSync(path.join(REPO_ROOT, '.opencode', 'plugins', name), 'utf8') ); } + assert.strictEqual( + enabledPlan.statePreview.operations.find(operation => operation.sourceRelativePath.split(path.sep).join('/') === '.opencode/plugins/index.ts').contentTransform, + 'opencode-disable-plugin-entrypoint' + ); applyInstallPlanDirect(declinedPlan); const declinedState = JSON.parse(fs.readFileSync(declinedPlan.installStatePath, 'utf8')); assert.strictEqual(declinedState.request.hookConsent, 'declined'); From 3989e50ab34d1a7fd7d5631e43c5b17028efab1a Mon Sep 17 00:00:00 2001 From: Huanyi Xie <74809554+xiehuanyi@users.noreply.github.com> Date: Tue, 6 Oct 2026 20:08:48 +0300 Subject: [PATCH 11/26] fix(proximity): retain all tracked working-set paths --- scripts/lib/control-pane/proximity.js | 11 +++- tests/lib/control-pane-proximity.test.js | 75 ++++++++++++++++++++++++ 2 files changed, 85 insertions(+), 1 deletion(-) diff --git a/scripts/lib/control-pane/proximity.js b/scripts/lib/control-pane/proximity.js index 1db91c81e..e287ebee3 100644 --- a/scripts/lib/control-pane/proximity.js +++ b/scripts/lib/control-pane/proximity.js @@ -60,7 +60,12 @@ function runGitDiff(worktreePath, base, extraArgs) { } function untrackedFiles(worktreePath) { - return runGit(worktreePath, ['ls-files', '--others', '--exclude-standard', '-z']).split('\0').filter(Boolean); + try { + return runGit(worktreePath, ['ls-files', '--others', '--exclude-standard', '-z']).split('\0').filter(Boolean); + } catch { + // Keep successful tracked-file diffs when this optional probe is unavailable. + return []; + } } /** @@ -74,6 +79,10 @@ function defaultWorkingSetFor(session) { const base = wt.base || 'HEAD'; try { const ranges = parseDiffRanges(runGitDiff(wt.path, base, ['--unified=0'])); + // Deletions, binary changes and quoted paths have no usable new-side hunks. + for (const file of runGitDiff(wt.path, base, ['--name-only', '-z']).split('\0').filter(Boolean)) { + if (!ranges.has(file)) ranges.set(file, []); + } for (const file of untrackedFiles(wt.path)) ranges.set(file, []); return [...ranges.entries()].map(([path, lines]) => (lines.length > 0 ? { path, lines } : { path })); } catch { diff --git a/tests/lib/control-pane-proximity.test.js b/tests/lib/control-pane-proximity.test.js index 790959c39..042576eaf 100644 --- a/tests/lib/control-pane-proximity.test.js +++ b/tests/lib/control-pane-proximity.test.js @@ -103,6 +103,81 @@ test('real worktrees expose committed, staged, unstaged and untracked work befor } }); +test('real Git working sets retain tracked deletions and binary changes', () => { + const fs = require('fs'); + const os = require('os'); + const path = require('path'); + const { execFileSync } = require('child_process'); + const repo = fs.mkdtempSync(path.join(os.tmpdir(), 'ecc-proximity-delete-')); + const git = (...args) => execFileSync('git', ['-C', repo, ...args], { stdio: 'pipe' }); + try { + git('init', '-q', '-b', 'main'); + git('config', 'user.name', 'proximity-test'); + git('config', 'user.email', 'proximity-test@example.com'); + fs.writeFileSync(path.join(repo, 'deleted.js'), 'export const value = 1;\n'); + fs.writeFileSync(path.join(repo, 'binary.bin'), Buffer.from([0, 1, 2])); + git('add', '.'); + git('commit', '-qm', 'base'); + fs.unlinkSync(path.join(repo, 'deleted.js')); + fs.writeFileSync(path.join(repo, 'binary.bin'), Buffer.from([0, 3, 4])); + const [agent] = sessionsToAgents([{ id: 'delete', worktree: { path: repo, base: 'main' } }]); + assert.ok(agent, 'an agent with only non-text tracked changes still participates'); + assert.deepStrictEqual(agent.files.map(f => f.path).sort(), ['binary.bin', 'deleted.js']); + assert.ok(agent.files.every(f => f.lines === undefined)); + } finally { + fs.rmSync(repo, { recursive: true, force: true }); + } +}); + +test('quoted staged paths and untracked paths share identity, and untracked failures keep tracked edits', () => { + const fs = require('fs'); + const os = require('os'); + const path = require('path'); + const childProcess = require('child_process'); + const originalExec = childProcess.execFileSync; + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'ecc-proximity-quoted-')); + const repo = path.join(dir, 'repo'); + const git = (cwd, ...args) => originalExec('git', ['-C', cwd, ...args], { stdio: 'pipe' }); + const modulePath = require.resolve('../../scripts/lib/control-pane/proximity'); + const cachedModule = require.cache[modulePath]; + try { + fs.mkdirSync(repo); + git(repo, 'init', '-q', '-b', 'main'); + git(repo, 'config', 'user.name', 'proximity-test'); + git(repo, 'config', 'user.email', 'proximity-test@example.com'); + fs.writeFileSync(path.join(repo, 'README'), 'base\n'); + git(repo, 'add', '.'); + git(repo, 'commit', '-qm', 'base'); + const file = 'café"résumé.js'; + const worktrees = ['a', 'b'].map(name => { + const wt = path.join(dir, name); + git(repo, 'worktree', 'add', '-q', '-b', name, wt, 'main'); + fs.writeFileSync(path.join(wt, file), `export const value = '${name}';\n`); + return { id: name, worktree: { path: wt, base: 'main' } }; + }); + git(worktrees[0].worktree.path, 'add', file); + const snapshot = buildProximitySnapshot(worktrees, { repoRoot: repo, graph: { adjacency: {} } }); + assert.strictEqual(snapshot.counts.agents, 2); + assert.ok(snapshot.advisories.some(advisory => advisory.level === 'resolution')); + assert.ok(snapshot.agents.every(agent => agent.files.includes(file))); + for (const code of ['EACCES', 'ETIMEDOUT', 'ENOBUFS']) { + childProcess.execFileSync = (command, args, options) => { + if (args.includes('ls-files')) throw Object.assign(new Error('untracked probe unavailable'), { code }); + return originalExec(command, args, options); + }; + delete require.cache[modulePath]; + const fresh = require(modulePath); + const [tracked] = fresh.sessionsToAgents([worktrees[0]]); + assert.ok(tracked, `${code}: tracked changes must survive`); + assert.deepStrictEqual(tracked.files.map(entry => entry.path), [file]); + } + } finally { + childProcess.execFileSync = originalExec; + require.cache[modulePath] = cachedModule; + fs.rmSync(dir, { recursive: true, force: true }); + } +}); + test('sessionsToAgents: only worktree sessions with edits participate', () => { const agents = sessionsToAgents(sessions, { changedFilesFor }); assert.deepStrictEqual(agents.map(a => a.agentId).sort(), ['docs-bot', 'lead-hermes', 'worker-kb']); From 52cc44f40aaae92829be18f5c8ef11c7f05c1162 Mon Sep 17 00:00:00 2001 From: Huanyi Xie <74809554+xiehuanyi@users.noreply.github.com> Date: Tue, 6 Oct 2026 20:12:07 +0300 Subject: [PATCH 12/26] fix(proximity): isolate diff headers and preserve partial probes --- scripts/lib/control-pane/proximity.js | 14 +++++++++++--- tests/lib/control-pane-proximity.test.js | 22 +++++++++++++++++++++- 2 files changed, 32 insertions(+), 4 deletions(-) diff --git a/scripts/lib/control-pane/proximity.js b/scripts/lib/control-pane/proximity.js index e287ebee3..63de333ed 100644 --- a/scripts/lib/control-pane/proximity.js +++ b/scripts/lib/control-pane/proximity.js @@ -28,9 +28,13 @@ function parseDiffRanges(diff) { const byFile = new Map(); let current = null; for (const line of String(diff || '').split('\n')) { + if (line.startsWith('diff --git ')) { + current = null; + continue; + } const fileMatch = line.match(/^\+\+\+ b\/(.+)$/); if (fileMatch) { - const name = fileMatch[1].trim(); + const name = fileMatch[1].replace(/\t$/, ''); current = name === '/dev/null' ? null : name; if (current && !byFile.has(current)) byFile.set(current, []); continue; @@ -80,8 +84,12 @@ function defaultWorkingSetFor(session) { try { const ranges = parseDiffRanges(runGitDiff(wt.path, base, ['--unified=0'])); // Deletions, binary changes and quoted paths have no usable new-side hunks. - for (const file of runGitDiff(wt.path, base, ['--name-only', '-z']).split('\0').filter(Boolean)) { - if (!ranges.has(file)) ranges.set(file, []); + try { + for (const file of runGitDiff(wt.path, base, ['--name-only', '-z']).split('\0').filter(Boolean)) { + if (!ranges.has(file)) ranges.set(file, []); + } + } catch { + // Preserve ranges already read if the supplementary filename probe fails. } for (const file of untrackedFiles(wt.path)) ranges.set(file, []); return [...ranges.entries()].map(([path, lines]) => (lines.length > 0 ? { path, lines } : { path })); diff --git a/tests/lib/control-pane-proximity.test.js b/tests/lib/control-pane-proximity.test.js index 042576eaf..74978efaf 100644 --- a/tests/lib/control-pane-proximity.test.js +++ b/tests/lib/control-pane-proximity.test.js @@ -153,6 +153,7 @@ test('quoted staged paths and untracked paths share identity, and untracked fail const wt = path.join(dir, name); git(repo, 'worktree', 'add', '-q', '-b', name, wt, 'main'); fs.writeFileSync(path.join(wt, file), `export const value = '${name}';\n`); + if (name === 'a') fs.writeFileSync(path.join(wt, 'README'), 'changed\n'); return { id: name, worktree: { path: wt, base: 'main' } }; }); git(worktrees[0].worktree.path, 'add', file); @@ -169,7 +170,14 @@ test('quoted staged paths and untracked paths share identity, and untracked fail const fresh = require(modulePath); const [tracked] = fresh.sessionsToAgents([worktrees[0]]); assert.ok(tracked, `${code}: tracked changes must survive`); - assert.deepStrictEqual(tracked.files.map(entry => entry.path), [file]); + assert.deepStrictEqual(tracked.files.map(entry => entry.path).sort(), ['README', file].sort()); + childProcess.execFileSync = (command, args, options) => { + if (args.includes('--name-only')) throw Object.assign(new Error('tracked names unavailable'), { code }); + return originalExec(command, args, options); + }; + delete require.cache[modulePath]; + const partial = require(modulePath).sessionsToAgents([worktrees[0]]); + assert.deepStrictEqual(partial[0].files.map(entry => entry.path), ['README']); } } finally { childProcess.execFileSync = originalExec; @@ -221,6 +229,18 @@ test('parseDiffRanges: extracts new-side line ranges per file', () => { ]); }); +test('diff headers isolate quoted/deleted hunks and preserve trailing-space paths', () => { + const ranges = parseDiffRanges([ + 'diff --git a/first.js b/first.js', '+++ b/first.js', '@@ -1 +1 @@', + 'diff --git "a/café.js" "b/café.js"', '+++ "b/café.js"', '@@ -20 +20 @@', + 'diff --git a/deleted.js b/deleted.js', '+++ /dev/null', '@@ -30 +0,0 @@', + 'diff --git a/trailing b/trailing', '+++ b/trailing ', '@@ -40 +40 @@', + ].join('\n')); + assert.deepStrictEqual(ranges.get('first.js'), [[1, 1]]); + assert.deepStrictEqual(ranges.get('trailing '), [[40, 40]]); + assert.strictEqual(ranges.has('trailing'), false); +}); + test('line-range channel: same file but disjoint ranges ⇒ no resolution', () => { // Two agents in the same file, far-apart functions. workingSetFor provides ranges. const workingSetFor = s => From f7d7532d651dc6e4ff878725ce9dea86f996789b Mon Sep 17 00:00:00 2001 From: Huanyi Xie <74809554+xiehuanyi@users.noreply.github.com> Date: Tue, 6 Oct 2026 21:05:58 +0300 Subject: [PATCH 13/26] fix(proximity): decode quoted paths with precise line ranges --- scripts/lib/control-pane/proximity.js | 37 ++++++++++++++++--- tests/lib/control-pane-proximity.test.js | 46 ++++++++++++++++++++++-- 2 files changed, 76 insertions(+), 7 deletions(-) diff --git a/scripts/lib/control-pane/proximity.js b/scripts/lib/control-pane/proximity.js index 63de333ed..a4525f41e 100644 --- a/scripts/lib/control-pane/proximity.js +++ b/scripts/lib/control-pane/proximity.js @@ -15,6 +15,34 @@ const { execFileSync } = require('child_process'); const { scanAirspace, buildProximityTriggers } = require('../agent-proximity'); const { buildDependencyGraph } = require('../agent-proximity/graph'); +/** Decode Git C-style path escapes as bytes before UTF-8 decoding. */ +function decodeGitPath(header) { + const value = header.replace(/\t$/, ''); + if (!value.startsWith('"')) return value; + if (!value.endsWith('"')) return null; + const parts = []; + let literal = ''; + const escaped = { a: 7, b: 8, t: 9, n: 10, v: 11, f: 12, r: 13, '"': 34, '\\': 92 }; + for (let i = 1; i < value.length - 1; i += 1) { + const character = value[i]; + if (character !== '\\') { literal += character; continue; } + if (literal) { parts.push(Buffer.from(literal, 'utf8')); literal = ''; } + const next = value[++i]; + if (/[0-7]/.test(next || '')) { + let octal = next; + while (octal.length < 3 && /[0-7]/.test(value[i + 1] || '')) octal += value[++i]; + const byte = parseInt(octal, 8); + if (byte > 255) return null; + parts.push(Buffer.from([byte])); + } else { + if (!Object.prototype.hasOwnProperty.call(escaped, next)) return null; + parts.push(Buffer.from([escaped[next]])); + } + } + if (literal) parts.push(Buffer.from(literal, 'utf8')); + return Buffer.concat(parts).toString('utf8'); +} + /** * Parse `git diff --unified=0` output into per-file NEW-side line ranges. Hunk * headers look like `@@ -a,b +c,d @@`; we keep the +c,d (new) side so the overlap @@ -32,10 +60,9 @@ function parseDiffRanges(diff) { current = null; continue; } - const fileMatch = line.match(/^\+\+\+ b\/(.+)$/); - if (fileMatch) { - const name = fileMatch[1].replace(/\t$/, ''); - current = name === '/dev/null' ? null : name; + if (line.startsWith('+++ ')) { + const gitPath = decodeGitPath(line.slice(4)); + current = gitPath && gitPath.startsWith('b/') ? gitPath.slice(2) : null; if (current && !byFile.has(current)) byFile.set(current, []); continue; } @@ -60,7 +87,7 @@ function runGit(worktreePath, args) { function runGitDiff(worktreePath, base, extraArgs) { const mergeBase = runGit(worktreePath, ['merge-base', '--', base, 'HEAD']).trim(); - return runGit(worktreePath, ['diff', ...extraArgs, mergeBase, '--']); + return runGit(worktreePath, ['-c', 'core.quotePath=false', 'diff', ...extraArgs, mergeBase, '--']); } function untrackedFiles(worktreePath) { diff --git a/tests/lib/control-pane-proximity.test.js b/tests/lib/control-pane-proximity.test.js index 74978efaf..1104214bc 100644 --- a/tests/lib/control-pane-proximity.test.js +++ b/tests/lib/control-pane-proximity.test.js @@ -148,7 +148,7 @@ test('quoted staged paths and untracked paths share identity, and untracked fail fs.writeFileSync(path.join(repo, 'README'), 'base\n'); git(repo, 'add', '.'); git(repo, 'commit', '-qm', 'base'); - const file = 'café"résumé.js'; + const file = process.platform === 'win32' ? 'café-résumé.js' : 'café"résumé.js'; const worktrees = ['a', 'b'].map(name => { const wt = path.join(dir, name); git(repo, 'worktree', 'add', '-q', '-b', name, wt, 'main'); @@ -177,7 +177,7 @@ test('quoted staged paths and untracked paths share identity, and untracked fail }; delete require.cache[modulePath]; const partial = require(modulePath).sessionsToAgents([worktrees[0]]); - assert.deepStrictEqual(partial[0].files.map(entry => entry.path), ['README']); + assert.deepStrictEqual(partial[0].files.map(entry => entry.path).sort(), ['README', file].sort()); } } finally { childProcess.execFileSync = originalExec; @@ -241,6 +241,48 @@ test('diff headers isolate quoted/deleted hunks and preserve trailing-space path assert.strictEqual(ranges.has('trailing'), false); }); +test('quoted Git headers decode UTF-8 octal bytes, quotes and control escapes into exact paths', () => { + const ranges = parseDiffRanges([ + 'diff --git "a/café.js" "b/café.js"', + '+++ "b/caf\\303\\251\\"r\\303\\251sum\\303\\251\\tname.js"', '@@ -4,2 +4,2 @@', + 'diff --git a/other.js b/other.js', '+++ b/other.js', '@@ -8 +8 @@', + ].join('\n')); + assert.deepStrictEqual(ranges.get('café"résumé\tname.js'), [[4, 5]]); + assert.deepStrictEqual(ranges.get('other.js'), [[8, 8]]); +}); + +test('real quoted filenames retain disjoint hunk ranges rather than whole-file collisions', () => { + const fs = require('fs'); + const os = require('os'); + const path = require('path'); + const { execFileSync } = require('child_process'); + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'ecc-proximity-quoted-lines-')); + const repo = path.join(dir, 'repo'); + const git = (cwd, ...args) => execFileSync('git', ['-C', cwd, ...args], { stdio: 'pipe' }); + const file = process.platform === 'win32' ? 'café-résumé.js' : 'café"résumé\tname.js'; + try { + fs.mkdirSync(repo); + git(repo, 'init', '-q', '-b', 'main'); + git(repo, 'config', 'user.name', 'proximity-test'); + git(repo, 'config', 'user.email', 'proximity-test@example.com'); + const original = Array.from({ length: 40 }, (_, i) => `const line${i} = ${i};`); + fs.writeFileSync(path.join(repo, file), `${original.join('\n')}\n`); + git(repo, 'add', '.'); git(repo, 'commit', '-qm', 'base'); + const sessions = ['a', 'b'].map((name, index) => { + const wt = path.join(dir, name); + git(repo, 'worktree', 'add', '-q', '-b', name, wt, 'main'); + const lines = original.map((line, i) => i === index * 39 ? `${line} // edit` : line); + fs.writeFileSync(path.join(wt, file), `${lines.join('\n')}\n`); + return { id: name, worktree: { path: wt, base: 'main' } }; + }); + const agents = sessionsToAgents(sessions); + assert.deepStrictEqual(agents[0].files, [{ weight: 1, path: file, lines: [[1, 1]] }]); + assert.deepStrictEqual(agents[1].files, [{ weight: 1, path: file, lines: [[40, 40]] }]); + const snapshot = buildProximitySnapshot(sessions, { repoRoot: repo, graph: { adjacency: {} } }); + assert.ok(!snapshot.advisories.some(advisory => advisory.level === 'resolution')); + } finally { fs.rmSync(dir, { recursive: true, force: true }); } +}); + test('line-range channel: same file but disjoint ranges ⇒ no resolution', () => { // Two agents in the same file, far-apart functions. workingSetFor provides ranges. const workingSetFor = s => From a3f00af6d282a5c5cb74a9c0a11d22ec4e3142cc Mon Sep 17 00:00:00 2001 From: agammann <161159040+agammann@users.noreply.github.com> Date: Tue, 6 Oct 2026 23:10:48 -0700 Subject: [PATCH 14/26] fix(hooks): report nested TypeScript errors on Windows --- scripts/hooks/stop-format-typecheck.js | 12 ++++++++++-- tests/hooks/stop-format-typecheck.test.js | 8 +++++++- 2 files changed, 17 insertions(+), 3 deletions(-) diff --git a/scripts/hooks/stop-format-typecheck.js b/scripts/hooks/stop-format-typecheck.js index 8ae580db5..bfca2fcd2 100644 --- a/scripts/hooks/stop-format-typecheck.js +++ b/scripts/hooks/stop-format-typecheck.js @@ -111,6 +111,14 @@ function findTsConfigDir(filePath) { return null; } +function diagnosticMatchesFile(line, paths) { + const normalizedLine = line.replace(/\\/g, '/'); + for (const candidate of paths) { + if (normalizedLine.includes(candidate.replace(/\\/g, '/'))) return true; + } + return false; +} + function typecheckBatch(tsConfigDir, editedFiles, timeoutMs) { const isWin = process.platform === 'win32'; const npxBin = isWin ? 'npx.cmd' : 'npx'; @@ -147,7 +155,7 @@ function typecheckBatch(tsConfigDir, editedFiles, timeoutMs) { const relPath = path.relative(tsConfigDir, filePath); const candidates = new Set([filePath, relPath]); const relevantLines = lines - .filter(line => { for (const c of candidates) { if (line.includes(c)) return true; } return false; }) + .filter(line => diagnosticMatchesFile(line, candidates)) .slice(0, 10); if (relevantLines.length > 0) { process.stderr.write(`[Hook] TypeScript errors in ${path.basename(filePath)}:\n`); @@ -248,4 +256,4 @@ if (require.main === module) { }); } -module.exports = { run, parseAccumulator, isPluginClonePath }; +module.exports = { run, parseAccumulator, isPluginClonePath, diagnosticMatchesFile }; diff --git a/tests/hooks/stop-format-typecheck.test.js b/tests/hooks/stop-format-typecheck.test.js index 564bae693..98c0cfaf7 100644 --- a/tests/hooks/stop-format-typecheck.test.js +++ b/tests/hooks/stop-format-typecheck.test.js @@ -13,7 +13,7 @@ const os = require('os'); const path = require('path'); const accumulator = require('../../scripts/hooks/post-edit-accumulator'); -const { parseAccumulator, isPluginClonePath } = require('../../scripts/hooks/stop-format-typecheck'); +const { parseAccumulator, isPluginClonePath, diagnosticMatchesFile } = require('../../scripts/hooks/stop-format-typecheck'); function test(name, fn) { try { @@ -203,6 +203,12 @@ if (test('parseAccumulator ignores blank lines and trims whitespace', () => { assert.deepStrictEqual(result, ['/tmp/a.ts', '/tmp/b.ts']); })) passed++; else failed++; +if (test('matches forward-slash TypeScript paths against Windows edited paths', () => { + const candidates = new Set(['C:\\project\\src\\broken.ts', 'src\\broken.ts']); + assert.strictEqual(diagnosticMatchesFile('src/broken.ts(1,14): error TS2322', candidates), true); + assert.strictEqual(diagnosticMatchesFile('src/other.ts(1,14): error TS2322', candidates), false); +})) passed++; else failed++; + if (test('stop hook clears accumulator after processing duplicates', () => { cleanAccumFile(); fs.writeFileSync(getAccumFile(), '/nonexistent/x.ts\n/nonexistent/x.ts\n/nonexistent/y.ts\n', 'utf8'); From 1cf47ab0e5e68b1a48fb95329c372db1dbbc1353 Mon Sep 17 00:00:00 2001 From: Yann Roberto <1922498827@qq.com> Date: Wed, 7 Oct 2026 14:33:30 +0800 Subject: [PATCH 15/26] fix(hooks): run Windows formatter shims in quality gate --- scripts/hooks/quality-gate.js | 9 ++- tests/hooks/quality-gate.test.js | 102 +++++++++++++++++++++++++++++++ 2 files changed, 108 insertions(+), 3 deletions(-) diff --git a/scripts/hooks/quality-gate.js b/scripts/hooks/quality-gate.js index 37373b87c..dd5f5a0fb 100755 --- a/scripts/hooks/quality-gate.js +++ b/scripts/hooks/quality-gate.js @@ -17,6 +17,7 @@ const fs = require('fs'); const path = require('path'); const { spawnSync } = require('child_process'); +const { getLinterInvocation } = require('./pre-bash-commit-quality'); const { findProjectRoot, detectFormatter, resolveFormatterBin } = require('../lib/resolve-formatter'); @@ -31,10 +32,12 @@ const MAX_STDIN = 1024 * 1024; * @returns {import('child_process').SpawnSyncReturns} */ function exec(command, args, cwd = process.cwd()) { - return spawnSync(command, args, { + // Reuse the Windows shim runner so .cmd formatters receive literal paths. + const invocation = getLinterInvocation(command, args); + return spawnSync(invocation.command, invocation.args, { + ...invocation.options, cwd, - encoding: 'utf8', - env: process.env, + env: invocation.options.env || process.env, timeout: 15000 }); } diff --git a/tests/hooks/quality-gate.test.js b/tests/hooks/quality-gate.test.js index cd3a05c4f..dfc75f1b4 100644 --- a/tests/hooks/quality-gate.test.js +++ b/tests/hooks/quality-gate.test.js @@ -8,8 +8,10 @@ const assert = require('assert'); const path = require('path'); const os = require('os'); const fs = require('fs'); +const { spawnSync } = require('child_process'); const qualityGate = require('../../scripts/hooks/quality-gate'); +const hookPath = path.resolve(__dirname, '../../scripts/hooks/quality-gate.js'); function test(name, fn) { try { @@ -155,5 +157,105 @@ if (test('returns original input for existing file with no formatter configured' } })) passed++; else failed++; +// Exercise real executable shims: .cmd on Windows, executable shell scripts elsewhere. + +function withFormatterFixture(formatter, fallback, fn) { + const root = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), 'quality-gate project '))); + try { + const srcDir = path.join(root, 'source files'); + const binDir = path.join(root, fallback ? 'runner bin' : 'node_modules/.bin'); + fs.mkdirSync(srcDir, { recursive: true }); + fs.mkdirSync(binDir, { recursive: true }); + fs.writeFileSync(path.join(root, 'package.json'), '{}'); + fs.writeFileSync(path.join(root, formatter === 'biome' ? 'biome.json' : '.prettierrc'), '{}'); + const fileName = 'edited %ECC_TEST_LITERAL% !ECC_TEST_LITERAL! & file'; + const filePath = path.join(srcDir, fileName + (formatter === 'biome' ? '.json' : '.js')); + const original = formatter === 'biome' ? '{"value":1}\n' : 'const value=1;\n'; + fs.writeFileSync(filePath, original); + const capturePath = path.join(root, 'invocation.json'); + const runnerPath = path.join(root, 'fake formatter.js'); + fs.writeFileSync(runnerPath, [ + "const fs = require('fs');", + "const path = require('path');", + 'const args = process.argv.slice(2);', + 'fs.writeFileSync(process.env.ECC_TEST_CAPTURE, JSON.stringify({ args, cwd: process.cwd() }));', + "if (args.includes('--write')) fs.appendFileSync(args.find(arg => path.isAbsolute(arg)), 'formatted\\n');", + 'process.exit(Number(process.env.ECC_TEST_EXIT || 0));' + ].join('\n')); + const binName = fallback ? 'npx' : formatter; + const shimPath = path.join(binDir, binName + (process.platform === 'win32' ? '.cmd' : '')); + const quoteSh = value => "'" + value.replace(/'/g, "'\\''") + "'"; + const shim = process.platform === 'win32' + ? `@echo off\r\n"${process.execPath}" "${runnerPath}" %*\r\n` + : `#!/bin/sh\nexec ${quoteSh(process.execPath)} ${quoteSh(runnerPath)} "$@"\n`; + fs.writeFileSync(shimPath, shim, { mode: 0o755 }); + const env = Object.fromEntries(Object.entries(process.env).filter(([key]) => key.toUpperCase() !== 'PATH')); + Object.assign(env, { + PATH: binDir, + CLAUDE_PACKAGE_MANAGER: 'npm', + ECC_QUALITY_GATE_FIX: 'false', + ECC_QUALITY_GATE_STRICT: 'true', + ECC_TEST_CAPTURE: capturePath, + ECC_TEST_LITERAL: 'must-not-expand', + ECC_TEST_EXIT: '0' + }); + const input = JSON.stringify({ tool_input: { file_path: filePath } }); + const runHook = overrides => spawnSync(process.execPath, [hookPath], { + cwd: srcDir, input, encoding: 'utf8', timeout: 20000, + env: { ...env, ...overrides } + }); + fn({ root, filePath, original, capturePath, input, runHook }); + } finally { + fs.rmSync(root, { recursive: true, force: true }); + } +} + +function assertInvocation(fixture, result, expectedArgs) { + assert.ifError(result.error); + assert.strictEqual(result.status, 0, result.stderr); + assert.strictEqual(result.stdout, fixture.input, 'Hook must preserve its input'); + assert.ok(fs.existsSync(fixture.capturePath), 'Formatter shim was not executed'); + assert.deepStrictEqual(JSON.parse(fs.readFileSync(fixture.capturePath, 'utf8')), { + args: expectedArgs, cwd: fixture.root + }); +} + +console.log('\nReal formatter subprocesses:'); +for (const formatter of ['prettier', 'biome']) { + for (const fix of [false, true]) { + if (test(`runs local ${formatter} in ${fix ? 'fix' : 'check'} mode with literal spaces and shell metacharacters`, () => { + withFormatterFixture(formatter, false, fixture => { + const result = fixture.runHook({ ECC_QUALITY_GATE_FIX: String(fix) }); + const args = formatter === 'biome' + ? ['check', fixture.filePath, ...(fix ? ['--write'] : [])] + : [fix ? '--write' : '--check', fixture.filePath]; + assertInvocation(fixture, result, args); + assert.strictEqual(result.stderr, '', 'Successful formatter should not warn'); + assert.strictEqual(fs.readFileSync(fixture.filePath, 'utf8'), fixture.original + (fix ? 'formatted\n' : '')); + }); + })) passed++; else failed++; + } + if (test(`runs ${formatter} through the package runner when no local formatter exists`, () => { + withFormatterFixture(formatter, true, fixture => { + const args = formatter === 'biome' + ? ['@biomejs/biome', 'check', fixture.filePath] + : ['prettier', '--check', fixture.filePath]; + const result = fixture.runHook(); + assertInvocation(fixture, result, args); + assert.strictEqual(result.stderr, ''); + assert.strictEqual(fs.readFileSync(fixture.filePath, 'utf8'), fixture.original); + }); + })) passed++; else failed++; +} + +if (test('reports a real formatter failure in strict mode while preserving hook input', () => { + withFormatterFixture('prettier', false, fixture => { + const result = fixture.runHook({ ECC_TEST_EXIT: '2' }); + assertInvocation(fixture, result, ['--check', fixture.filePath]); + assert.ok(result.stderr.includes(`[QualityGate] Prettier check failed for ${fixture.filePath}`)); + assert.strictEqual(fs.readFileSync(fixture.filePath, 'utf8'), fixture.original); + }); +})) passed++; else failed++; + console.log(`\nResults: Passed: ${passed}, Failed: ${failed}`); process.exit(failed > 0 ? 1 : 0); From b4943a5e4ba7cc16780a31742c1c8ae888e115f5 Mon Sep 17 00:00:00 2001 From: YogurDungeon Date: Wed, 7 Oct 2026 01:26:58 -0500 Subject: [PATCH 16/26] fix: record skill runs under OpenCode and report unmeasured skills honestly MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Under OpenCode the skill-health dashboard had no writer and no honest fallback, so it reported every discovered skill as healthy (#2463). Root cause: `post:skill:track` is only wired for Claude Code, where `posttooluse-dispatcher.js` fans the event out to `skill-run-tracker.js`. The OpenCode plugin re-implements the harness events instead of reusing that dispatcher, so nothing ever wrote `skill-runs.jsonl` — and the tracker's matcher only accepted the exact Claude Code tool name `Skill`, while OpenCode calls the tool `skill`, so even wiring it would have dropped every event. Commands that locate ECC through the inlined resolver in `scripts/lib/resolve-ecc-root.js` also fell back to `~/.claude`, which does not contain ECC, so they could not find their scripts. Changes: - `.opencode/plugins/ecc-hooks.ts`: dispatch `post:skill:track` from `tool.execute.after`. The tracker module is imported lazily and behind try/catch, matching the existing `changed-files-store` pattern: this file is OpenCode's startup entry point, so a static import failure would crash the session before any hook runs (#2530). The module is reused rather than reimplemented, keeping its identifier validation, length bounds and "never persist prompt text" guarantees. - `scripts/hooks/skill-run-tracker.js`: match the tool name case-insensitively so `Skill` and `skill` are both accepted. - `scripts/lib/skill-evolution/health.js`: split the summary into measured/healthy/declining/unmeasured instead of deriving `healthy = total - declining`, which turned "no telemetry" into "everything is fine" for every skill. - `scripts/lib/skill-evolution/dashboard.js`: show the breakdown and warn explicitly when skills have no recorded runs, since absence of telemetry is not a health signal. Under OpenCode the resolver also needs `CLAUDE_PLUGIN_ROOT`, which only the plugin can supply: `shell.env` exports it when a root is found, without replacing a root the user set or one an earlier plugin already put in `output.env`. Root discovery is per capability and probes required runtime files — never optional content. Probing for `skills/continuous-learning-v2` (the `workflow-quality` module) rejected supported modular installs that ship the plugin and tracker without that optional module, disabling telemetry and the root export for exactly those users. Also fixes an incidental bug: the compiled plugin bundle is ESM, where `__dirname` is undefined, so the ECC version lookup silently fell back to its default. The module directory is now derived from `import.meta.url`. Tests: `tests/opencode-plugin-hooks.test.js` covers the telemetry dispatch (skill tool recorded, other tools ignored, minimal profile respected), a modular install without the optional skills module, the root export and both override paths; `tests/hooks/skill-run-tracker.test.js` covers the case-insensitive matcher; `tests/lib/skill-evolution.test.js` and `tests/lib/skill-dashboard.test.js` cover the unmeasured bucket, the warning and the aged-out-coverage wording. Refs #2463, #2530 --- .opencode/plugins/ecc-hooks.ts | 196 +++++++++++++- scripts/hooks/skill-run-tracker.js | 41 ++- scripts/lib/skill-evolution/dashboard.js | 33 ++- scripts/lib/skill-evolution/health.js | 25 +- tests/hooks/skill-run-tracker.test.js | 53 +++- tests/lib/skill-dashboard.test.js | 173 +++++++++++++ tests/lib/skill-evolution.test.js | 56 +++- tests/opencode-plugin-hooks.test.js | 310 +++++++++++++++++++++++ 8 files changed, 865 insertions(+), 22 deletions(-) diff --git a/.opencode/plugins/ecc-hooks.ts b/.opencode/plugins/ecc-hooks.ts index e350adad6..86eaa7cdf 100644 --- a/.opencode/plugins/ecc-hooks.ts +++ b/.opencode/plugins/ecc-hooks.ts @@ -16,9 +16,26 @@ import type { PluginInput } from "@opencode-ai/plugin" import * as fs from "fs" import * as path from "path" +import { fileURLToPath, pathToFileURL } from "url" import changedFilesTool from "../tools/changed-files.ts" import dependencyAnalyzerTool from "../tools/dependency-analyzer.ts" +/** + * Directory of this module. + * + * The plugin is ESM: tsc compiles it to .opencode/dist/plugins/*.js and OpenCode + * loads the .ts directly, so `__dirname` is not defined there. Reading it used to + * throw inside a try/catch and silently degrade the reported ECC version to the + * default. `import.meta.url` is the ESM source of truth; the `__dirname` check is + * kept first only so a CJS consumer of this module still resolves correctly. + */ +function resolvePluginDir(): string { + if (typeof __dirname === "string" && __dirname) return __dirname + return path.dirname(fileURLToPath(import.meta.url)) +} + +const PLUGIN_DIR = resolvePluginDir() + /** * Type definitions for better type safety */ @@ -51,17 +68,84 @@ interface TodoEvent { } /** - * Read ECC version from package.json - * Falls back to a default if package.json cannot be read + * Artifacts that identify an ECC root, one per capability this plugin needs. + * + * These are required runtime files, never optional content: `skills/` belongs to + * the optional `workflow-quality` module, so probing for a skill rejected + * supported modular installs (e.g. `platform-configs` + `hooks-runtime` without + * `workflow-quality`) that still ship both the plugin and the tracker — silently + * disabling skill telemetry and the shell root export for those users. + * + * Each probe is the exact artifact the caller is about to use, so detection is + * layout detection rather than an authenticity control: a directory only + * qualifies because it contains what we are going to read. */ -function getECCVersion(): string { - try { - const packageJsonPath = path.resolve(__dirname, "../../package.json") - const packageJson = JSON.parse(fs.readFileSync(packageJsonPath, "utf-8")) - return packageJson.version || "2.0.0" - } catch { - return "2.0.0" +const ECC_ROOT_PROBES = { + // scripts/lib/utils.js is the shared library every scripts/ module requires. + root: [path.join("scripts", "lib", "utils.js")], + // What the skill-run telemetry hook imports at runtime. + skillTracker: [path.join("scripts", "hooks", "skill-run-tracker.js")], + // What the resolver embedded in commands requires once they read + // CLAUDE_PLUGIN_ROOT, and the version lookup for shell.env. + resolver: [path.join("scripts", "lib", "resolve-ecc-root.js")], +} as const + +/** + * Walk upwards from `startDir` looking for an ancestor that contains every + * relative path in `probes`. + * + * The plugin ships in three layouts — `/plugins` (installed), `/dist/plugins` + * (compiled) and `/.opencode/plugins` (repo checkout) — so the number of + * levels back to the root is not fixed. Probing beats a hardcoded `..`: a wrong + * root would make every command resolve against a directory that has no scripts. + * + * The walk is bounded to 5 levels so it cannot wander into unrelated ancestors. + * + * Returns null when no ancestor qualifies, so callers can leave the capability + * disabled instead of advertising a root that does not hold the artifact. + */ +function findEccRootDir(startDir: string, probes: readonly string[]): string | null { + let current = startDir + for (let depth = 0; depth < 5; depth += 1) { + if (probes.every((probe) => fs.existsSync(path.join(current, probe)))) { + return current + } + const parent = path.dirname(current) + if (parent === current) break + current = parent } + return null +} + +/** + * Read the ECC version from package.json. + * + * Candidates are tried in order — the resolved ECC root first, then the two + * plausible parent depths of the plugin — because the layout differs between an + * installed root (`/plugins`), the compiled bundle (`/dist/plugins`) + * and a repo checkout (`/.opencode/plugins`). Falls back to a default if + * no package.json can be read. + */ +function getECCVersion(eccRootDir?: string | null): string { + const candidates = [ + eccRootDir, + path.resolve(PLUGIN_DIR, ".."), + path.resolve(PLUGIN_DIR, "..", ".."), + ] + + for (const candidate of candidates) { + if (!candidate) continue + try { + const packageJson = JSON.parse(fs.readFileSync(path.join(candidate, "package.json"), "utf-8")) + if (typeof packageJson.version === "string" && packageJson.version) { + return packageJson.version + } + } catch { + // Try the next candidate. + } + } + + return "2.0.0" } type ECCHooksPluginFn = (input: PluginInput) => Promise> @@ -135,6 +219,60 @@ export const ECCHooksPlugin: ECCHooksPluginFn = async ({ .catch(() => {}) } + // Claude Code routes post:skill:track through + // scripts/hooks/posttooluse-dispatcher.js, which OpenCode never invokes. + // Without a caller here, nothing ever writes skill-runs.jsonl and + // `skills-health.js --dashboard` has no data to report on any skill (#2463). + // + // The tracker module is reused rather than reimplemented so its identifier + // validation, length bounds and "never persist prompt text" guarantees stay + // the ones that were written and audited for the Claude Code path. + // + // Loaded lazily, for the same reason as the store above: this file is + // OpenCode's startup entry point, and a static import of a missing module + // crashes the plugin -- and with it the whole session -- before any hook can + // run (#2530). A runtime file URL also keeps tsc from resolving a relative + // specifier that exists in no compiled layout. + type SkillRunTrackerModule = { run?: (payload: unknown) => void } + let recordSkillRun: ((payload: unknown) => void) | undefined + // Detection is scoped to the tracker itself, so a modular install that omits + // optional content still gets telemetry. + const trackerRoot = findEccRootDir(PLUGIN_DIR, ECC_ROOT_PROBES.skillTracker) + // The root advertised to commands is a different question: those commands + // require the resolver, so a root without it would break them again. + const eccRoot = + findEccRootDir(PLUGIN_DIR, ECC_ROOT_PROBES.resolver) ?? findEccRootDir(PLUGIN_DIR, ECC_ROOT_PROBES.root) + try { + const trackerPath = trackerRoot + ? path.join(trackerRoot, "scripts", "hooks", "skill-run-tracker.js") + : null + if (!trackerPath || !fs.existsSync(trackerPath)) { + throw new Error("skill-run-tracker.js not found") + } + // CJS module loaded from ESM: run() is on the namespace in Node/Bun, but a + // transpiled default export must stay supported. + const loaded = (await import(pathToFileURL(trackerPath).href)) as SkillRunTrackerModule & { + default?: SkillRunTrackerModule + } + const entry = typeof loaded.run === "function" ? loaded : loaded.default + if (!entry || typeof entry.run !== "function") { + throw new Error("skill-run-tracker.js exposes no run()") + } + recordSkillRun = entry.run + } catch { + // Same deferred-log rationale as the store above: guarantee that a + // telemetry failure can never escape this catch and abort startup. + Promise.resolve() + .then(() => + log( + "warn", + "[ECC] skill-run telemetry disabled: could not load scripts/hooks/skill-run-tracker.js. " + + "Run `ecc repair --target opencode` to restore the missing files. Other ECC hooks are unaffected." + ) + ) + .catch(() => {}) + } + const normalizeProfile = (value: string | undefined): HookProfile => { if (value === "minimal" || value === "strict") return value return "standard" @@ -264,6 +402,25 @@ export const ECCHooksPlugin: ECCHooksPluginFn = async ({ ) { log("info", "[ECC] PR created - check GitHub Actions status") } + + // Skill telemetry (#2463). OpenCode's tool is named `skill`, so the + // harness-side name is matched here (case-insensitively) and the payload + // is handed over in the shape the tracker already understands. + if ( + hookEnabled("post:skill:track", ["standard", "strict"]) && + typeof input.tool === "string" && + input.tool.trim().toLowerCase() === "skill" + ) { + try { + recordSkillRun?.({ + tool_name: "Skill", + tool_input: input.args ?? {}, + tool_response: output, + }) + } catch { + // Telemetry is best-effort; never let it affect the tool result. + } + } }, /** @@ -483,7 +640,7 @@ export const ECCHooksPlugin: ECCHooksPluginFn = async ({ */ "shell.env": async (_input: { cwd: string }, output: { env: Record }) => { const env: Record = { - ECC_VERSION: getECCVersion(), + ECC_VERSION: getECCVersion(eccRoot), ECC_PLUGIN: "true", ECC_HOOK_PROFILE: currentProfile, ECC_DISABLED_HOOKS: process.env.ECC_DISABLED_HOOKS || "", @@ -523,6 +680,25 @@ export const ECCHooksPlugin: ECCHooksPluginFn = async ({ env.PRIMARY_LANGUAGE = detected[0] } + // resolve-ecc-root.js only probes ~/.claude and Claude Code's plugin + // cache, so under OpenCode it fell back to ~/.claude and every command + // carrying the inlined resolver failed to find its scripts. That resolver + // reads CLAUDE_PLUGIN_ROOT first, so exporting the root here fixes them + // through the mechanism ECC already provides -- rather than editing the + // command files that embed the locator. + // + // Only set when a root actually exists, and never replace a root someone + // else already chose: neither the process environment (set by the user) + // nor `output.env` (set by an earlier plugin). The final merge below + // overwrites output.env, so checking it only in `env` would clobber it. + const rootAlreadySet = Boolean( + (process.env.CLAUDE_PLUGIN_ROOT || "").trim() || + (output.env?.CLAUDE_PLUGIN_ROOT || "").trim() + ) + if (eccRoot && !rootAlreadySet) { + env.CLAUDE_PLUGIN_ROOT = eccRoot + } + // OpenCode reads the supplied output object and ignores callback return values. output.env = { ...output.env, ...env } }, diff --git a/scripts/hooks/skill-run-tracker.js b/scripts/hooks/skill-run-tracker.js index eef54b859..a306245d5 100644 --- a/scripts/hooks/skill-run-tracker.js +++ b/scripts/hooks/skill-run-tracker.js @@ -16,8 +16,9 @@ * skill id is an identifier, not free text, so anything that does not look like * one is dropped rather than written through. * - * Best-effort: never blocks tool execution. Runs under the PostToolUse - * dispatcher, which owns stdin, pass-through, and exit codes. + * Best-effort: never blocks tool execution. Under Claude Code it runs via the + * PostToolUse dispatcher, which owns stdin, pass-through, and exit codes; under + * OpenCode the ecc-hooks plugin calls run() directly with an equivalent payload. * * Cross-platform (Windows, macOS, Linux); CommonJS. */ @@ -102,6 +103,28 @@ function deriveOutcome(payload) { return 'success'; } +const SKILL_TOOL_NAME = 'skill'; + +/** + * Report whether a hook payload describes a skill tool invocation. + * + * Claude Code names the tool `Skill`; OpenCode and other harnesses name it + * `skill`. Comparing the raw string meant a case difference silently disabled + * the whole write side again — the hook ran, dropped every event, and the + * dashboard kept reporting unmeasured skills with no hint why. + * + * @param {{tool_name?: unknown} | null} payload + * @returns {boolean} + */ +function isSkillToolCall(payload) { + if (!payload || typeof payload !== 'object') { + return false; + } + + const toolName = payload.tool_name; + return typeof toolName === 'string' && toolName.trim().toLowerCase() === SKILL_TOOL_NAME; +} + function buildRecord(payload) { const skillId = extractSkillId(payload.tool_input); if (!skillId) { @@ -130,12 +153,22 @@ function buildRecord(payload) { }; } +/** + * Record a skill invocation from a hook payload. + * + * Accepts the raw JSON string Claude Code's dispatcher reads from stdin, or an + * already-parsed payload for in-process callers such as the OpenCode plugin. + * Non-skill tools and unparsable input are ignored. + * + * @param {string | object | null | undefined} rawInput + * @returns {void} + */ function run(rawInput) { try { const payload = typeof rawInput === 'string' ? (rawInput.trim() ? JSON.parse(rawInput) : {}) : rawInput; - if (payload && typeof payload === 'object' && payload.tool_name === 'Skill') { + if (isSkillToolCall(payload)) { const record = buildRecord(payload); if (record) { recordSkillExecution(record); @@ -146,4 +179,4 @@ function run(rawInput) { } } -module.exports = { buildRecord, deriveOutcome, extractSkillId, run }; +module.exports = { buildRecord, deriveOutcome, extractSkillId, isSkillToolCall, run }; diff --git a/scripts/lib/skill-evolution/dashboard.js b/scripts/lib/skill-evolution/dashboard.js index 7049707bd..edd98ce74 100644 --- a/scripts/lib/skill-evolution/dashboard.js +++ b/scripts/lib/skill-evolution/dashboard.js @@ -328,6 +328,18 @@ function renderVersionTimelinePanel(skillsById, options = {}) { }; } +/** + * Render the full skill-health dashboard: header, coverage warning and panels. + * + * Coverage is stated explicitly because the 30-day window can empty out while + * runs are still retained, so "unmeasured" must not be reported as "never + * recorded" (#2463). + * + * @param {object} [options] + * @param {string} [options.now] ISO timestamp used as the evaluation instant + * @param {string} [options.panel] render only this panel + * @returns {{text: string, data: object}} + */ function renderDashboard(options = {}) { const now = options.now || new Date().toISOString(); const nowMs = Date.parse(now); @@ -359,12 +371,31 @@ function renderDashboard(options = {}) { const header = [ 'ECC Skill Health Dashboard', `Generated: ${now}`, - `Skills: ${summary.total_skills} total, ${summary.healthy_skills} healthy, ${summary.declining_skills} declining`, + `Skills: ${summary.total_skills} total, ${summary.measured_skills} measured, ${summary.healthy_skills} healthy, ${summary.declining_skills} declining, ${summary.unmeasured_skills} unmeasured`, '', ]; textParts.push(header.join('\n')); + if (summary.unmeasured_skills > 0) { + // The window can empty out while runs are still retained, so "unmeasured" + // must not be reported as "never recorded" (#2463). Three distinct states, + // because "no recent runs" alone does not say which one applies. + const recentRuns = health.filterRecordsWithinDays(records, nowMs, 30).length; + const coverageNote = records.length === 0 + ? 'Nothing has ever been recorded, so the panels below are not a signal about skill health.' + : recentRuns === 0 + ? `All ${records.length} retained run${records.length === 1 ? '' : 's'} are older than the 30-day window: coverage aged out rather than never existing. The panels below still show that history.` + : `Coverage is partial: ${recentRuns} recent run${recentRuns === 1 ? '' : 's'} inside the window, ${records.length - recentRuns} older one${records.length - recentRuns === 1 ? '' : 's'} retained outside it. Only the measured skills can be judged.`; + + textParts.push([ + `WARNING: ${summary.unmeasured_skills} of ${summary.total_skills} skills have no recorded run in the last 30 days.`, + 'This dashboard can say nothing about them: no telemetry is an absence of evidence,', + 'not a healthy result. Their success rate reads "n/a", which means unknown.', + coverageNote, + ].join('\n')); + } + if (selectedPanel) { const result = panelRenderers[selectedPanel](); panels[selectedPanel] = result.data; diff --git a/scripts/lib/skill-evolution/health.js b/scripts/lib/skill-evolution/health.js index 309f16094..c57e23354 100644 --- a/scripts/lib/skill-evolution/health.js +++ b/scripts/lib/skill-evolution/health.js @@ -26,15 +26,36 @@ function formatRate(value) { return `${Math.round(value * 100)}%`; } +/** + * Summarise a health report into counting buckets. + * + * A skill with no runs in the window is not healthy, it is unmeasured: the + * trend helpers return 'stable' when both rates are null, so deriving health + * from "not declining" turned a missing telemetry writer into a green + * dashboard across every skill (#2463). A declining skill always has runs, so + * the buckets partition the total. + * + * @param {{skills: Array<{declining?: boolean, run_count_30d?: number}>}} report + * @returns {{total_skills: number, measured_skills: number, healthy_skills: number, declining_skills: number, unmeasured_skills: number}} + */ function summarizeHealthReport(report) { const totalSkills = report.skills.length; + // A skill with no runs in the window is not healthy, it is unmeasured. + // Folding it into "healthy" turned a missing telemetry writer into a green + // dashboard across every skill (#2463) — false confidence that never triggers + // an alert. A declining skill always has runs, so the buckets partition the + // total. + const unmeasuredSkills = report.skills.filter(skill => !skill.run_count_30d).length; const decliningSkills = report.skills.filter(skill => skill.declining).length; - const healthySkills = totalSkills - decliningSkills; + const measuredSkills = totalSkills - unmeasuredSkills; + const healthySkills = measuredSkills - decliningSkills; return { total_skills: totalSkills, + measured_skills: measuredSkills, healthy_skills: healthySkills, declining_skills: decliningSkills, + unmeasured_skills: unmeasuredSkills, }; } @@ -230,7 +251,7 @@ function formatHealthReport(report, options = {}) { const lines = [ 'ECC skill health', `Generated: ${report.generated_at}`, - `Skills: ${summary.total_skills} total, ${summary.healthy_skills} healthy, ${summary.declining_skills} declining`, + `Skills: ${summary.total_skills} total, ${summary.measured_skills} measured, ${summary.healthy_skills} healthy, ${summary.declining_skills} declining, ${summary.unmeasured_skills} unmeasured`, '', 'skill version 7d 30d trend pending last run', '--------------------------------------------------------------------------', diff --git a/tests/hooks/skill-run-tracker.test.js b/tests/hooks/skill-run-tracker.test.js index d3e63e247..f836ab247 100644 --- a/tests/hooks/skill-run-tracker.test.js +++ b/tests/hooks/skill-run-tracker.test.js @@ -15,7 +15,7 @@ const fs = require('fs'); const os = require('os'); const path = require('path'); -const { buildRecord, deriveOutcome, extractSkillId, run } = require('../../scripts/hooks/skill-run-tracker'); +const { buildRecord, deriveOutcome, extractSkillId, isSkillToolCall, run } = require('../../scripts/hooks/skill-run-tracker'); const { readHooksConfig } = require('../../scripts/lib/hooks-config'); const { MAX_RUN_RECORDS, @@ -152,6 +152,57 @@ test('run ignores non-Skill tools and malformed input without throwing', () => { assert.doesNotThrow(() => run('')); }); +// ── tool-name tolerance ─────────────────────────────────────────────────────── +// OpenCode names the tool `skill`. A case-sensitive matcher meant the hook ran, +// dropped every event, and the dashboard kept reporting "unmeasured" with no +// indication that a writer existed at all. + +test('isSkillToolCall accepts both harness spellings and rejects everything else', () => { + assert.strictEqual(isSkillToolCall({ tool_name: 'Skill' }), true); + assert.strictEqual(isSkillToolCall({ tool_name: 'skill' }), true); + assert.strictEqual(isSkillToolCall({ tool_name: ' SKILL ' }), true); + assert.strictEqual(isSkillToolCall({ tool_name: 'Bash' }), false); + assert.strictEqual(isSkillToolCall({ tool_name: '' }), false); + assert.strictEqual(isSkillToolCall({ tool_name: 42 }), false); + assert.strictEqual(isSkillToolCall({}), false); + assert.strictEqual(isSkillToolCall(null), false); +}); + +test('run records a run for a lowercase skill tool name', () => { + withTempHome(homeDir => { + const previousHome = process.env.HOME; + const previousUserProfile = process.env.USERPROFILE; + process.env.HOME = homeDir; + process.env.USERPROFILE = homeDir; + try { + run(payload({ tool_name: 'skill' })); + + const records = readSkillExecutionRecords({ homeDir }); + assert.strictEqual(records.length, 1); + assert.strictEqual(records[0].skill_id, 'code-review'); + } finally { + if (previousHome === undefined) delete process.env.HOME; else process.env.HOME = previousHome; + if (previousUserProfile === undefined) delete process.env.USERPROFILE; else process.env.USERPROFILE = previousUserProfile; + } + }); +}); + +test('run still records nothing for a non-skill tool', () => { + withTempHome(homeDir => { + const previousHome = process.env.HOME; + const previousUserProfile = process.env.USERPROFILE; + process.env.HOME = homeDir; + process.env.USERPROFILE = homeDir; + try { + run(payload({ tool_name: 'bash' })); + assert.strictEqual(readSkillExecutionRecords({ homeDir }).length, 0); + } finally { + if (previousHome === undefined) delete process.env.HOME; else process.env.HOME = previousHome; + if (previousUserProfile === undefined) delete process.env.USERPROFILE; else process.env.USERPROFILE = previousUserProfile; + } + }); +}); + // ── JSONL sink bounds ──────────────────────────────────────────────────────── test('recordSkillExecution writes the sink owner-only', function () { diff --git a/tests/lib/skill-dashboard.test.js b/tests/lib/skill-dashboard.test.js index f90052e85..d1f29d0df 100644 --- a/tests/lib/skill-dashboard.test.js +++ b/tests/lib/skill-dashboard.test.js @@ -358,6 +358,179 @@ function runTests() { }, /Unknown panel/); })) passed++; else failed++; + // A panel that reports every never-used skill as healthy is worse than an + // empty one: it looks like evidence and never raises an alert (#2463). + if (test('renderDashboard warns that unmeasured skills say nothing about health', () => { + const result = dashboard.renderDashboard({ + skillsRoot, + learnedRoot, + importedRoot, + homeDir, + runsFilePath: path.join(homeDir, '.claude', 'state', 'no-such-runs.jsonl'), + now, + warnThreshold: 0.1, + }); + + const summary = result.data.summary; + assert.ok(summary.unmeasured_skills > 0, 'expected unmeasured skills'); + assert.ok( + result.text.includes('WARNING'), + 'expected an explicit warning when skills have no telemetry' + ); + assert.match(result.text, /unmeasured/); + assert.strictEqual( + summary.healthy_skills, + 0, + 'skills without runs must not be counted as healthy' + ); + })) passed++; else failed++; + + // Isolated roots so the full-coverage case is guaranteed rather than + // conditional: every discovered skill needs a run inside the 30-day window. + if (test('renderDashboard omits the warning once every skill is measured', () => { + const coveredRoot = createTempDir('ecc-dashboard-covered-'); + const coveredLearned = createTempDir('ecc-dashboard-covered-'); + const coveredImported = createTempDir('ecc-dashboard-covered-'); + const coveredRuns = path.join(homeDir, '.claude', 'state', 'covered-skill-runs.jsonl'); + try { + for (const name of ['covered-a', 'covered-b']) { + createSkill(coveredRoot, name, `# ${name}\n`); + } + appendJsonl(coveredRuns, ['covered-a', 'covered-b'].map(skill_id => ({ + skill_id, + skill_version: 'v1', + task_description: `Skill invocation: ${skill_id}`, + outcome: 'success', + recorded_at: now, + }))); + + const result = dashboard.renderDashboard({ + skillsRoot: coveredRoot, + learnedRoot: coveredLearned, + importedRoot: coveredImported, + homeDir, + runsFilePath: coveredRuns, + now, + warnThreshold: 0.1, + }); + + assert.strictEqual(result.data.summary.unmeasured_skills, 0); + assert.strictEqual(result.data.summary.measured_skills, 2); + assert.ok( + !result.text.includes('WARNING'), + 'no warning expected when every skill has a run in the window' + ); + assert.strictEqual( + result.data.summary.total_skills, + result.data.summary.measured_skills + result.data.summary.unmeasured_skills, + 'measured and unmeasured must partition the total' + ); + } finally { + cleanupTempDir(coveredRoot); + cleanupTempDir(coveredLearned); + cleanupTempDir(coveredImported); + } + })) passed++; else failed++; + + // The 30-day window can empty out while runs are still retained. Saying + // "nothing has ever been recorded" then is factually wrong. + if (test('the unmeasured warning distinguishes aged-out coverage from no history', () => { + const agedRuns = path.join(homeDir, '.claude', 'state', 'aged-skill-runs.jsonl'); + const agedOut = new Date(Date.parse(now) - (60 * 24 * 60 * 60 * 1000)).toISOString(); + appendJsonl(agedRuns, [{ + skill_id: 'alpha', + skill_version: 'v1', + task_description: 'Skill invocation: alpha', + outcome: 'failure', + recorded_at: agedOut, + }]); + + const result = dashboard.renderDashboard({ + skillsRoot, + learnedRoot, + importedRoot, + homeDir, + runsFilePath: agedRuns, + now, + warnThreshold: 0.1, + }); + + assert.ok(result.data.summary.unmeasured_skills > 0, 'expected unmeasured skills'); + assert.ok(result.text.includes('WARNING'), 'expected the warning'); + assert.ok( + result.text.includes('aged out'), + 'retained history outside the window must be described as aged out' + ); + assert.ok( + !result.text.includes('Nothing has ever been recorded'), + 'must not claim there is no history when runs are retained' + ); + })) passed++; else failed++; + + // Mixed coverage is the common case: some skills used recently, others not. +// It must not be described as aged out, nor as having no history at all. + if (test('the unmeasured warning describes partial coverage as partial', () => { + const mixedRuns = path.join(homeDir, '.claude', 'state', 'mixed-skill-runs.jsonl'); + appendJsonl(mixedRuns, [ + { + skill_id: 'alpha', + skill_version: 'v1', + task_description: 'Skill invocation: alpha', + outcome: 'success', + recorded_at: now, + }, + { + skill_id: 'beta', + skill_version: 'v1', + task_description: 'Skill invocation: beta', + outcome: 'success', + recorded_at: new Date(Date.parse(now) - (60 * 24 * 60 * 60 * 1000)).toISOString(), + }, + ]); + + const result = dashboard.renderDashboard({ + skillsRoot, + learnedRoot, + importedRoot, + homeDir, + runsFilePath: mixedRuns, + now, + warnThreshold: 0.1, + }); + + assert.ok(result.text.includes('WARNING'), 'expected the warning'); + assert.ok( + result.text.includes('Coverage is partial'), + 'mixed recent and aged runs must be reported as partial coverage' + ); + assert.ok( + !result.text.includes('aged out'), + 'coverage is not aged out while recent runs exist' + ); + assert.ok( + !result.text.includes('Nothing has ever been recorded'), + 'runs are retained, so history is not missing' + ); + })) passed++; else failed++; + + // With no runs at all the stronger statement is the accurate one. + if (test('the unmeasured warning says nothing was ever recorded when there is no history', () => { + const result = dashboard.renderDashboard({ + skillsRoot, + learnedRoot, + importedRoot, + homeDir, + runsFilePath: path.join(homeDir, '.claude', 'state', 'definitely-absent.jsonl'), + now, + warnThreshold: 0.1, + }); + + assert.ok( + result.text.includes('Nothing has ever been recorded'), + 'an empty record set must be reported as missing telemetry' + ); + })) passed++; else failed++; + console.log('\nCLI integration:'); if (test('CLI --dashboard --json returns valid JSON with all panels', () => { diff --git a/tests/lib/skill-evolution.test.js b/tests/lib/skill-evolution.test.js index bfff437bf..f2dd4e6ec 100644 --- a/tests/lib/skill-evolution.test.js +++ b/tests/lib/skill-evolution.test.js @@ -559,19 +559,67 @@ function runTests() { const summary = health.summarizeHealthReport(report); assert.deepStrictEqual(summary, { total_skills: 6, - healthy_skills: 5, + measured_skills: 2, + healthy_skills: 1, declining_skills: 1, + unmeasured_skills: 4, }); const human = health.formatHealthReport(report, { json: false }); assert.match(human, /alpha/); assert.match(human, /worsening/); - assert.match( - human, - new RegExp(`Skills: ${summary.total_skills} total, ${summary.healthy_skills} healthy, ${summary.declining_skills} declining`) + // Built from values rather than interpolated into a RegExp: the numbers are + // internally generated, but a literal comparison needs no pattern engine. + assert.ok( + human.includes( + `Skills: ${summary.total_skills} total, ${summary.measured_skills} measured, ` + + `${summary.healthy_skills} healthy, ${summary.declining_skills} declining, ` + + `${summary.unmeasured_skills} unmeasured` + ), + 'summary line must report measured and unmeasured counts' ); })) passed++; else failed++; + if (test('does not report unmeasured skills as healthy (#2463)', () => { + // A runs file that does not exist: skills are still discovered, so this + // is exactly the "discovered but never executed" state. + const emptyReport = health.collectSkillHealth({ + repoRoot, + homeDir, + runsFilePath: path.join(homeDir, '.claude', 'state', 'no-such-runs.jsonl'), + now, + warnThreshold: 0.1, + }); + + const summary = health.summarizeHealthReport(emptyReport); + assert.ok(summary.total_skills > 0, 'expected discovered skills'); + assert.strictEqual(summary.total_skills, summary.unmeasured_skills); + assert.strictEqual(summary.measured_skills, 0); + assert.strictEqual(summary.declining_skills, 0); + assert.strictEqual( + summary.healthy_skills, + 0, + 'a skill with no runs is unmeasured, never healthy' + ); + + // Healthy + declining must account for every measured skill, so a partial + // coverage state cannot over-report. + const partial = health.summarizeHealthReport({ + skills: [ + { skill_id: 'a', run_count_30d: 4, declining: false }, + { skill_id: 'b', run_count_30d: 2, declining: true }, + { skill_id: 'c', run_count_30d: 0, declining: false }, + ], + }); + assert.deepStrictEqual(partial, { + total_skills: 3, + measured_skills: 2, + healthy_skills: 1, + declining_skills: 1, + unmeasured_skills: 1, + }); + })) passed++; else failed++; + if (test('treats an unsnapshotted SKILL.md as v1 and orders last_run by actual time', () => { const gammaSkillDir = createSkill(skillsRoot, 'gamma', '# Gamma v1\n'); const offsetRunsFile = path.join(homeDir, '.claude', 'state', 'offset-skill-runs.jsonl'); diff --git a/tests/opencode-plugin-hooks.test.js b/tests/opencode-plugin-hooks.test.js index a6c3ed2ae..e327f6ebf 100644 --- a/tests/opencode-plugin-hooks.test.js +++ b/tests/opencode-plugin-hooks.test.js @@ -79,6 +79,90 @@ async function withTempProject(files, fn) { } } +/** + * Build a fake ECC installation root and copy the compiled plugin into it. + * + * Reproduces a *modular* install: the runtime scripts the plugin needs are + * present, but `skills/` is absent because that directory belongs to the optional + * `workflow-quality` module. `options.withSkills` adds it back so the same helper + * can prove the difference is what matters. + * + * @param {{withSkills?: boolean}} [options] + * @returns {string} path to the fake installation root + */ +function createModularInstallRoot(options = {}) { + const root = fs.mkdtempSync(path.join(os.tmpdir(), "ecc-opencode-install-")) + const distSource = path.join(__dirname, "..", ".opencode", "dist") + const repoRoot = path.join(__dirname, "..") + + for (const dir of ["plugins", "tools"]) { + fs.cpSync(path.join(distSource, dir), path.join(root, dir), { recursive: true }) + } + + // The runtime files an install without `workflow-quality` still ships. Copied + // whole rather than file-by-file: scripts/lib has transitive requires (utils.js + // alone pulls in agent-data-home), and a partial copy would fail the import for + // reasons unrelated to what this test is about. + for (const dir of ["hooks", "lib"]) { + fs.cpSync(path.join(repoRoot, "scripts", dir), path.join(root, "scripts", dir), { + recursive: true, + }) + } + + fs.writeFileSync( + path.join(root, "package.json"), + JSON.stringify({ name: "ecc-universal", version: "0.0.0-test" }) + ) + + // The plugin's tools import @opencode-ai/plugin, which resolves through + // node_modules — a real installation has it, an isolated temp dir would not. + fs.symlinkSync(path.join(repoRoot, "node_modules"), path.join(root, "node_modules"), "junction") + + if (options.withSkills) { + fs.mkdirSync(path.join(root, "skills", "continuous-learning-v2"), { recursive: true }) + fs.writeFileSync(path.join(root, "skills", "continuous-learning-v2", "SKILL.md"), "# sentinel\n") + } + + return root +} + +/** Load the compiled plugin from a fake install root instead of the repo dist. */ +async function loadPluginFrom(root) { + return import(pathToFileURL(path.join(root, "plugins", "ecc-hooks.js")).href) +} + +/** + * Restore environment variables saved with Object.fromEntries of name -> value. + * + * @param {Record} saved + * @returns {void} + */ +function restoreEnv(saved) { + for (const [key, value] of Object.entries(saved)) { + if (value === undefined) delete process.env[key] + else process.env[key] = value + } +} + +/** + * Read the skill-runs sink the tracker wrote under `homeDir`. + * + * The tracker resolves the sink from the *current* home, so the telemetry tests + * move HOME/USERPROFILE to a temp dir and read from there. + * + * @param {string} homeDir + * @returns {Array>} + */ +function readRecordedRuns(homeDir) { + const runsFilePath = path.join(homeDir, ".claude", "state", "skill-runs.jsonl") + if (!fs.existsSync(runsFilePath)) return [] + return fs + .readFileSync(runsFilePath, "utf8") + .split("\n") + .filter(Boolean) + .map((line) => JSON.parse(line)) +} + async function main() { console.log("\n=== Testing OpenCode plugin hooks ===\n") @@ -381,6 +465,232 @@ async function main() { } ), ], + [ + "skill telemetry records a run through the shared tracker (#2463)", + async () => withTempProject([], async (projectDir) => { + const homeDir = fs.mkdtempSync(path.join(os.tmpdir(), "ecc-opencode-skill-home-")) + const previousHome = process.env.HOME + const previousUserProfile = process.env.USERPROFILE + process.env.HOME = homeDir + process.env.USERPROFILE = homeDir + try { + const client = createClient() + const $ = createFailingShell() + const hooks = await ECCHooksPlugin({ client, $, directory: projectDir }) + + assert.ok( + !client.logs.some( + (entry) => + entry.level === "warn" && entry.message.includes("skill-run telemetry disabled") + ), + "Did not expect telemetry to be disabled when scripts/hooks/skill-run-tracker.js is present" + ) + + // OpenCode names the tool `skill`; the id arrives on `name`. + await hooks["tool.execute.after"]({ tool: "skill", args: { name: "code-review" } }, {}) + + const records = readRecordedRuns(homeDir) + + assert.strictEqual(records.length, 1, "Expected exactly one recorded skill run") + assert.strictEqual(records[0].skill_id, "code-review") + assert.strictEqual(records[0].outcome, "success") + } finally { + restoreEnv({ HOME: previousHome, USERPROFILE: previousUserProfile }) + fs.rmSync(homeDir, { recursive: true, force: true }) + } + }), + ], + [ + "skill telemetry records nothing for other tools", + async () => withTempProject([], async (projectDir) => { + const homeDir = fs.mkdtempSync(path.join(os.tmpdir(), "ecc-opencode-skill-home-")) + const previousHome = process.env.HOME + const previousUserProfile = process.env.USERPROFILE + process.env.HOME = homeDir + process.env.USERPROFILE = homeDir + try { + const client = createClient() + const $ = createFailingShell() + const hooks = await ECCHooksPlugin({ client, $, directory: projectDir }) + + await hooks["tool.execute.after"]({ tool: "edit", args: { path: "src/a.ts" } }, {}) + await hooks["tool.execute.after"]({ tool: "bash", args: { command: "ls" } }, {}) + + assert.strictEqual(readRecordedRuns(homeDir).length, 0) + } finally { + restoreEnv({ HOME: previousHome, USERPROFILE: previousUserProfile }) + fs.rmSync(homeDir, { recursive: true, force: true }) + } + }), + ], + [ + "skill telemetry respects the minimal hook profile", + async () => withTempProject([], async (projectDir) => { + const homeDir = fs.mkdtempSync(path.join(os.tmpdir(), "ecc-opencode-skill-home-")) + const previousHome = process.env.HOME + const previousUserProfile = process.env.USERPROFILE + const previousProfile = process.env.ECC_HOOK_PROFILE + process.env.HOME = homeDir + process.env.USERPROFILE = homeDir + process.env.ECC_HOOK_PROFILE = "minimal" + try { + const client = createClient() + const $ = createFailingShell() + const hooks = await ECCHooksPlugin({ client, $, directory: projectDir }) + + await hooks["tool.execute.after"]({ tool: "skill", args: { name: "code-review" } }, {}) + + assert.strictEqual(readRecordedRuns(homeDir).length, 0) + } finally { + restoreEnv({ + HOME: previousHome, + USERPROFILE: previousUserProfile, + ECC_HOOK_PROFILE: previousProfile, + }) + fs.rmSync(homeDir, { recursive: true, force: true }) + } + }), + ], + [ + "shell.env exports CLAUDE_PLUGIN_ROOT so commands can resolve the ECC root", + async () => withTempProject([], async (projectDir) => { + const previousRoot = process.env.CLAUDE_PLUGIN_ROOT + delete process.env.CLAUDE_PLUGIN_ROOT + try { + const client = createClient() + const $ = createFailingShell() + const hooks = await ECCHooksPlugin({ client, $, directory: projectDir }) + + const output = { env: {} } + await hooks["shell.env"]({ cwd: projectDir }, output) + + assert.deepStrictEqual($.calls, [], `Unexpected shell probes: ${$.calls.join(", ")}`) + assert.ok(output.env.CLAUDE_PLUGIN_ROOT, "Expected CLAUDE_PLUGIN_ROOT to be exported") + + // The resolver consumes this variable verbatim, so it must be a + // directory that actually carries the ECC script tree. + const root = output.env.CLAUDE_PLUGIN_ROOT + assert.ok( + fs.existsSync(path.join(root, "scripts", "lib", "resolve-ecc-root.js")), + `Expected ${root} to contain scripts/lib/resolve-ecc-root.js` + ) + } finally { + restoreEnv({ CLAUDE_PLUGIN_ROOT: previousRoot }) + } + }), + ], + [ + "shell.env does not override a CLAUDE_PLUGIN_ROOT the user set", + async () => withTempProject([], async (projectDir) => { + const explicitRoot = path.join(projectDir, "explicit-root") + const previousRoot = process.env.CLAUDE_PLUGIN_ROOT + process.env.CLAUDE_PLUGIN_ROOT = explicitRoot + try { + const client = createClient() + const $ = createFailingShell() + const hooks = await ECCHooksPlugin({ client, $, directory: projectDir }) + + const output = { env: {} } + await hooks["shell.env"]({ cwd: projectDir }, output) + + assert.strictEqual( + output.env.CLAUDE_PLUGIN_ROOT, + undefined, + "the plugin must not inject its own root over one the user set" + ) + assert.strictEqual( + process.env.CLAUDE_PLUGIN_ROOT, + explicitRoot, + "the user's value must survive untouched" + ) + } finally { + restoreEnv({ CLAUDE_PLUGIN_ROOT: previousRoot }) + } + }), + ], + [ + "shell.env does not clobber a root another plugin already put in output.env", + async () => withTempProject([], async (projectDir) => { + const previousRoot = process.env.CLAUDE_PLUGIN_ROOT + delete process.env.CLAUDE_PLUGIN_ROOT + try { + const client = createClient() + const $ = createFailingShell() + const hooks = await ECCHooksPlugin({ client, $, directory: projectDir }) + + // An earlier plugin chose a root. The final merge below spreads our env + // over output.env, so a guard that only checked process.env would + // replace this value. + const output = { env: { CLAUDE_PLUGIN_ROOT: path.join(projectDir, "other-plugin-root") } } + await hooks["shell.env"]({ cwd: projectDir }, output) + + assert.strictEqual( + output.env.CLAUDE_PLUGIN_ROOT, + path.join(projectDir, "other-plugin-root"), + "an existing output.env root must be preserved" + ) + } finally { + restoreEnv({ CLAUDE_PLUGIN_ROOT: previousRoot }) + } + }), + ], + [ + "telemetry and root export work in a modular install without optional skills", + async () => { + // Regression: root detection probed skills/continuous-learning-v2, which + // belongs to the optional workflow-quality module. An install with + // platform-configs + hooks-runtime but without workflow-quality still + // ships the plugin and the tracker, and it lost both fixes. + const root = createModularInstallRoot() + const projectDir = fs.mkdtempSync(path.join(os.tmpdir(), "ecc-opencode-modular-")) + const homeDir = fs.mkdtempSync(path.join(os.tmpdir(), "ecc-opencode-skill-home-")) + const previousHome = process.env.HOME + const previousUserProfile = process.env.USERPROFILE + const previousRoot = process.env.CLAUDE_PLUGIN_ROOT + process.env.HOME = homeDir + process.env.USERPROFILE = homeDir + delete process.env.CLAUDE_PLUGIN_ROOT + try { + assert.ok( + !fs.existsSync(path.join(root, "skills")), + "the fixture must not contain skills/, or it would not test the modular case" + ) + + const { ECCHooksPlugin: modularPlugin } = await loadPluginFrom(root) + const client = createClient() + const hooks = await modularPlugin({ + client, + $: createFailingShell(), + directory: projectDir, + }) + + assert.ok( + !client.logs.some( + (entry) => + entry.level === "warn" && entry.message.includes("skill-run telemetry disabled") + ), + "telemetry must stay enabled when the optional skills module is absent" + ) + + await hooks["tool.execute.after"]({ tool: "skill", args: { name: "code-review" } }, {}) + assert.strictEqual(readRecordedRuns(homeDir).length, 1, "expected the run to be recorded") + + const output = { env: {} } + await hooks["shell.env"]({ cwd: projectDir }, output) + assert.strictEqual(output.env.CLAUDE_PLUGIN_ROOT, root) + assert.strictEqual(output.env.ECC_VERSION, "0.0.0-test") + } finally { + restoreEnv({ + HOME: previousHome, + USERPROFILE: previousUserProfile, + CLAUDE_PLUGIN_ROOT: previousRoot, + }) + fs.rmSync(root, { recursive: true, force: true }) + fs.rmSync(projectDir, { recursive: true, force: true }) + fs.rmSync(homeDir, { recursive: true, force: true }) + } + }, + ], ] let passed = 0 From 27d58e589a4ebac8482da9a08855239eeff0f2d8 Mon Sep 17 00:00:00 2001 From: serhattayaz-ai <322430760+serhattayaz-ai@users.noreply.github.com> Date: Thu, 8 Oct 2026 16:10:35 +0300 Subject: [PATCH 17/26] fix(install): strip ecc: plugin namespace from manual Claude installs The ecc@ecc plugin namespaces agents, commands and skills as `ecc:`. A manual install (install-apply --target claude|claude-project) places the same files at bare names under ~/.claude, so plugin-prefixed references in copied markdown point at agents and commands that do not exist in that layout (e.g. subagent_type "ecc:planner", frontmatter agent: ecc:...). - Add plugin-namespace.js with stripPluginNamespace(), which removes the `ecc:` prefix before an identifier and leaves `ecc@ecc`, URLs and words ending in "ecc" alone. - Apply it to markdown copy operations when the target is claude or claude-project. Non-markdown files and other targets are unchanged. - Reword the agents rule so it describes both install modes instead of stating that agents only ship with the plugin. - Cover the transform with tests/lib/install-plugin-namespace.test.js. --- rules/common/agents.md | 4 +- scripts/lib/install/apply.js | 12 ++- scripts/lib/install/plugin-namespace.js | 19 +++++ tests/lib/install-plugin-namespace.test.js | 86 ++++++++++++++++++++++ 4 files changed, 117 insertions(+), 4 deletions(-) create mode 100644 scripts/lib/install/plugin-namespace.js create mode 100644 tests/lib/install-plugin-namespace.test.js diff --git a/rules/common/agents.md b/rules/common/agents.md index 14d9b9005..98b53e79f 100644 --- a/rules/common/agents.md +++ b/rules/common/agents.md @@ -2,8 +2,8 @@ ## Available Agents -ECC agents ship with the `ecc@ecc` plugin, not in `~/.claude/agents/`. -They are invoked through the Agent tool with a plugin-scoped `subagent_type`: +ECC agents come either from the `ecc@ecc` plugin (plugin-scoped names, `ecc:`) or from a manual install into `~/.claude/agents/` (bare names, ``). Use the name that matches the install you have; the manual installer strips the `ecc:` prefix from copied files. +They are invoked through the Agent tool with that `subagent_type`: ```text Agent(subagent_type: "ecc:planner", prompt: "...") diff --git a/scripts/lib/install/apply.js b/scripts/lib/install/apply.js index 209e53784..2f378fc7b 100644 --- a/scripts/lib/install/apply.js +++ b/scripts/lib/install/apply.js @@ -51,6 +51,11 @@ const { prepareExcludedPathsReconciliation, } = require('./excluded-paths-reconciliation'); const { buildInstallIndex, rewriteRelativeLinks } = require('./link-rewrite'); +const { stripPluginNamespace } = require('./plugin-namespace'); + +// Manual installs place agents, commands and skills at bare names; the +// ecc@ecc plugin namespaces them as `ecc:`. +const MANUAL_CLAUDE_TARGETS = new Set(['claude', 'claude-project']); const { adaptAntigravityAgent } = require('./antigravity-agent'); function isMarkdownPath(filePath) { @@ -874,13 +879,16 @@ function applyInstallPlanLocked(plan, dependencies = {}, settingsLockHeld = fals && operation.sourceRelativePath && isMarkdownPath(operation.destinationPath) ); - if (operation.kind === 'copy-file' && (operation.contentTransform || needsLinkRewrite)) { + const stripNamespace = MANUAL_CLAUDE_TARGETS.has(plan.adapter?.target) + && isMarkdownPath(operation.destinationPath); + if (operation.kind === 'copy-file' && (operation.contentTransform || needsLinkRewrite || stripNamespace)) { const transformed = transformInstallContent( operation, fs.readFileSync(operation.sourcePath, 'utf8') ); + const namespaced = stripNamespace ? stripPluginNamespace(transformed) : transformed; const installedContent = needsLinkRewrite - ? rewriteRelativeLinks(transformed, { + ? rewriteRelativeLinks(namespaced, { sourceRel: operation.sourceRelativePath, index: linkIndex, }) diff --git a/scripts/lib/install/plugin-namespace.js b/scripts/lib/install/plugin-namespace.js new file mode 100644 index 000000000..8a9022d6d --- /dev/null +++ b/scripts/lib/install/plugin-namespace.js @@ -0,0 +1,19 @@ +'use strict'; + +// The ecc@ecc plugin exposes agents, commands and skills under an `ecc:` +// namespace (`ecc:planner`, `/ecc:plan`). A manual install +// (install-apply --target claude|claude-project) places the same files at bare +// names, so plugin-prefixed references in copied markdown must drop the prefix +// or they point at agents and commands that do not exist in that layout. +// +// The leading guard keeps `ecc@ecc` and words that merely end in "ecc" (e.g. +// "decc:") untouched; the lookahead requires a real identifier after the colon. +const PLUGIN_NAMESPACE_PREFIX = /(^|[^A-Za-z0-9_@.-])ecc:(?=[a-z][a-z0-9-]*)/gm; + +function stripPluginNamespace(content) { + return String(content).replace(PLUGIN_NAMESPACE_PREFIX, '$1'); +} + +module.exports = { + stripPluginNamespace, +}; diff --git a/tests/lib/install-plugin-namespace.test.js b/tests/lib/install-plugin-namespace.test.js new file mode 100644 index 000000000..7a0d2d15e --- /dev/null +++ b/tests/lib/install-plugin-namespace.test.js @@ -0,0 +1,86 @@ +/** + * Tests for scripts/lib/install/plugin-namespace.js — manual Claude installs + * place agents, commands and skills at bare names, so plugin-prefixed + * `ecc:` references in copied markdown must lose the prefix. + */ + +'use strict'; + +const assert = require('assert'); + +const { stripPluginNamespace } = require('../../scripts/lib/install/plugin-namespace'); + +function test(name, fn) { + try { + fn(); + console.log(` PASS ${name}`); + return true; + } catch (error) { + console.log(` FAIL ${name}`); + console.log(` Error: ${error.message}`); + console.error(error.stack || error); + return false; + } +} + +function runTests() { + console.log('Running install plugin-namespace tests...\n'); + let passed = 0; + let failed = 0; + const record = ok => (ok ? passed++ : failed++); + + record(test('strips the prefix from agent names in code spans, bold and tables', () => { + const input = [ + '| ecc:planner | Planning |', + '1. Use **ecc:code-reviewer** agent', + 'Use `ecc:tdd-guide` here', + ].join('\n'); + assert.strictEqual(stripPluginNamespace(input), [ + '| planner | Planning |', + '1. Use **code-reviewer** agent', + 'Use `tdd-guide` here', + ].join('\n')); + })); + + record(test('strips the prefix from agent frontmatter and tool-call arguments', () => { + assert.strictEqual( + stripPluginNamespace('agent: ecc:security-reviewer'), + 'agent: security-reviewer' + ); + assert.strictEqual( + stripPluginNamespace('Agent(subagent_type: "ecc:planner", prompt: "...")'), + 'Agent(subagent_type: "planner", prompt: "...")' + ); + })); + + record(test('strips the prefix from slash commands', () => { + assert.strictEqual(stripPluginNamespace('see `/ecc:ecc-guide`'), 'see `/ecc-guide`'); + assert.strictEqual(stripPluginNamespace('/ecc:plan "Add auth"'), '/plan "Add auth"'); + })); + + record(test('leaves the plugin identifier ecc@ecc untouched', () => { + const input = '`/plugin install ecc@ecc` and `ecc@ecc`'; + assert.strictEqual(stripPluginNamespace(input), input); + })); + + record(test('leaves words that only end in "ecc" untouched', () => { + const input = 'the decc:thing and tecc:value'; + assert.strictEqual(stripPluginNamespace(input), input); + })); + + record(test('does not touch a colon with no identifier after it', () => { + const input = 'the `ecc:` prefix in plugin mode'; + assert.strictEqual(stripPluginNamespace(input), input); + })); + + record(test('returns content with no prefix unchanged', () => { + const input = '# Plain markdown\n\nNo namespace here.\n'; + assert.strictEqual(stripPluginNamespace(input), input); + })); + + console.log(`\nResults: Passed: ${passed}, Failed: ${failed}`); + return failed === 0; +} + +const ok = runTests(); +process.exit(ok ? 0 : 1); From bb95734f4e4bec9d9aad265c72426bf5591452d4 Mon Sep 17 00:00:00 2001 From: serhattayaz-ai <322430760+serhattayaz-ai@users.noreply.github.com> Date: Thu, 8 Oct 2026 16:19:57 +0300 Subject: [PATCH 18/26] fix(install): keep namespace stripping when link rewriting is skipped --- scripts/lib/install/apply.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/lib/install/apply.js b/scripts/lib/install/apply.js index 2f378fc7b..7e6164273 100644 --- a/scripts/lib/install/apply.js +++ b/scripts/lib/install/apply.js @@ -892,7 +892,7 @@ function applyInstallPlanLocked(plan, dependencies = {}, settingsLockHeld = fals sourceRel: operation.sourceRelativePath, index: linkIndex, }) - : transformed; + : namespaced; const writeOptions = getOpenCodeActivationWriteOptions(operation, activationSnapshot); if (writeOptions.expectedContent) { writeFileNoFollow(operation.destinationPath, installedContent, { From ab606759d98f07b8c9465a2057e32f2ba74cd5ce Mon Sep 17 00:00:00 2001 From: serhattayaz-ai <322430760+serhattayaz-ai@users.noreply.github.com> Date: Thu, 8 Oct 2026 16:33:23 +0300 Subject: [PATCH 19/26] test(install): cover namespace stripping in the Claude manual install --- tests/lib/install-executor.test.js | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests/lib/install-executor.test.js b/tests/lib/install-executor.test.js index d39c14628..4f3aba628 100644 --- a/tests/lib/install-executor.test.js +++ b/tests/lib/install-executor.test.js @@ -161,6 +161,7 @@ function writeManifestSourceFixture(root) { writeFile(root, path.join('src', 'stray.pyd'), 'ignored\n'); writeFile(root, path.join('src', 'nested', 'ecc-install-state.json'), '{}\n'); writeFile(root, path.join('rules', 'common', 'coding-style.md'), '# Common\n'); + writeFile(root, path.join('rules', 'common', 'agents.md'), 'Use the `ecc:planner` agent.\n'); writeFile(root, path.join('skills', 'demo', 'SKILL.md'), '# Demo\n'); writeFile(root, 'standalone.txt', 'standalone\n'); writeFile(root, path.join('runtime', 'ecc', 'install-state.json'), '{}\n'); @@ -683,6 +684,11 @@ function runTests() { assert.ok(fs.existsSync(path.join(homeDir, '.claude', 'src', 'app.js'))); assert.ok(fs.existsSync(path.join(homeDir, '.claude', 'standalone.txt'))); assert.ok(fs.existsSync(path.join(homeDir, '.claude', 'plugin.json'))); + const installedAgentsRule = fs.readFileSync( + path.join(homeDir, '.claude', 'rules', 'ecc', 'common', 'agents.md'), + 'utf8' + ); + assert.strictEqual(installedAgentsRule, 'Use the `planner` agent.\n'); const state = JSON.parse(fs.readFileSync(path.join(homeDir, '.claude', 'ecc', 'install-state.json'), 'utf8')); assert.strictEqual(state.request.profile, 'minimal'); assert.deepStrictEqual(state.resolution.selectedModules, ['fixture-core']); From 684c9711731d71f053d473e2879e23e6d1bf7a89 Mon Sep 17 00:00:00 2001 From: Eneko Ruiz <196690101+eneekoruiz@users.noreply.github.com> Date: Thu, 8 Oct 2026 23:46:56 +0200 Subject: [PATCH 20/26] fix(hooks): resolve session IDs from hook payloads --- scripts/hooks/post-edit-accumulator.js | 16 +- scripts/hooks/session-end-marker.js | 5 +- scripts/hooks/session-start.js | 12 +- scripts/hooks/stop-format-typecheck.js | 11 +- scripts/lib/hook-session.js | 25 ++ .../hooks/observer-session-lifecycle.test.js | 227 ++++++++++++++++++ tests/hooks/stop-format-typecheck.test.js | 92 +++++++ tests/lib/hook-session.test.js | 85 +++++++ tests/scripts/npm-publish-surface.test.js | 1 + 9 files changed, 457 insertions(+), 17 deletions(-) create mode 100644 scripts/lib/hook-session.js create mode 100644 tests/hooks/observer-session-lifecycle.test.js create mode 100644 tests/lib/hook-session.test.js diff --git a/scripts/hooks/post-edit-accumulator.js b/scripts/hooks/post-edit-accumulator.js index c626b2dd8..2bdafba9d 100644 --- a/scripts/hooks/post-edit-accumulator.js +++ b/scripts/hooks/post-edit-accumulator.js @@ -18,15 +18,16 @@ const crypto = require('crypto'); const fs = require('fs'); const os = require('os'); const path = require('path'); +const { resolveHookSessionId } = require('../lib/hook-session'); const MAX_STDIN = 1024 * 1024; -function getAccumFile() { +function getAccumFile(rawSessionId) { const raw = - process.env.CLAUDE_SESSION_ID || + rawSessionId || crypto.createHash('sha1').update(process.cwd()).digest('hex').slice(0, 12); // Strip path separators and traversal sequences so the value is safe to embed - // directly in a filename regardless of what CLAUDE_SESSION_ID contains. + // directly in a filename regardless of the hook session ID source. const sessionId = raw.replace(/[^a-zA-Z0-9_-]/g, '_').slice(0, 64); return path.join(os.tmpdir(), `ecc-edited-${sessionId}.txt`); } @@ -37,9 +38,9 @@ function getAccumFile() { */ const JS_TS_EXT = /\.(ts|tsx|js|jsx)$/; -function appendPath(filePath) { +function appendPath(filePath, sessionId) { if (filePath && JS_TS_EXT.test(filePath)) { - fs.appendFileSync(getAccumFile(), filePath + '\n', 'utf8'); + fs.appendFileSync(getAccumFile(sessionId), filePath + '\n', 'utf8'); } } @@ -50,12 +51,13 @@ function appendPath(filePath) { function run(rawInput) { try { const input = JSON.parse(rawInput); + const sessionId = resolveHookSessionId(rawInput); // Edit / Write: single file_path - appendPath(input.tool_input?.file_path); + appendPath(input.tool_input?.file_path, sessionId); // MultiEdit: array of edits, each with its own file_path const edits = input.tool_input?.edits; if (Array.isArray(edits)) { - for (const edit of edits) appendPath(edit?.file_path); + for (const edit of edits) appendPath(edit?.file_path, sessionId); } } catch { // Invalid input — pass through diff --git a/scripts/hooks/session-end-marker.js b/scripts/hooks/session-end-marker.js index 7b27a8eb3..5a432ee4c 100755 --- a/scripts/hooks/session-end-marker.js +++ b/scripts/hooks/session-end-marker.js @@ -13,6 +13,7 @@ const { stopObserverForContext, resolveSessionId } = require('../lib/observer-sessions'); +const { resolveHookSessionId } = require('../lib/hook-session'); function log(message) { process.stderr.write(`[SessionEnd] ${message}\n`); @@ -20,10 +21,10 @@ function log(message) { function run(rawInput) { const output = rawInput || ''; - const sessionId = resolveSessionId(); + const sessionId = resolveSessionId(resolveHookSessionId(output)); if (!sessionId) { - log('No CLAUDE_SESSION_ID available; skipping observer cleanup'); + log('No session ID available; skipping observer cleanup'); return output; } diff --git a/scripts/hooks/session-start.js b/scripts/hooks/session-start.js index a7462c551..528b6fcd8 100644 --- a/scripts/hooks/session-start.js +++ b/scripts/hooks/session-start.js @@ -23,6 +23,7 @@ const { log } = require('../lib/utils'); const { resolveProjectContext, writeSessionLease, resolveSessionId, getHomunculusDir } = require('../lib/observer-sessions'); +const { resolveHookSessionId } = require('../lib/hook-session'); const { getPackageManager, getSelectionPrompt } = require('../lib/package-manager'); const { listAliases } = require('../lib/session-aliases'); const { detectProjectType } = require('../lib/project-detect'); @@ -181,6 +182,10 @@ function getSessionStartMode(rawInput) { return SESSION_START_MODE_INVALID; } + if (!payload || typeof payload !== 'object' || Array.isArray(payload)) { + return SESSION_START_MODE_SKIP; + } + const supportedModes = new Set(['startup', 'resume', 'clear', 'compact']); const hookName = typeof payload.hookName === 'string' ? payload.hookName.trim() : ''; if (hookName.startsWith('SessionStart:')) { @@ -649,7 +654,8 @@ async function main() { const maxContextChars = getSessionStartMaxContextChars(); const explicitContextDisabled = isSessionStartContextDisabled(); const shouldInjectContext = !explicitContextDisabled && maxContextChars !== 0; - const sessionStartMode = getSessionStartMode(fs.readFileSync(0, 'utf8')); + const rawInput = fs.readFileSync(0, 'utf8'); + const sessionStartMode = getSessionStartMode(rawInput); // Ensure directories exist ensureDir(sessionsDir); @@ -665,7 +671,7 @@ async function main() { } } - const observerSessionId = resolveSessionId(); + const observerSessionId = resolveSessionId(resolveHookSessionId(rawInput)); if (observerSessionId) { writeSessionLease(observerContext, observerSessionId, { hook: 'SessionStart', @@ -673,7 +679,7 @@ async function main() { }); log(`[SessionStart] Registered observer lease for ${observerSessionId}`); } else { - log('[SessionStart] No CLAUDE_SESSION_ID available; skipping observer lease registration'); + log('[SessionStart] No session ID available; skipping observer lease registration'); } if (explicitContextDisabled) { diff --git a/scripts/hooks/stop-format-typecheck.js b/scripts/hooks/stop-format-typecheck.js index 8ae580db5..870af23af 100644 --- a/scripts/hooks/stop-format-typecheck.js +++ b/scripts/hooks/stop-format-typecheck.js @@ -23,6 +23,7 @@ const os = require('os'); const path = require('path'); const { findProjectRoot, detectFormatter, resolveFormatterBin } = require('../lib/resolve-formatter'); +const { resolveHookSessionId } = require('../lib/hook-session'); const MAX_STDIN = 1024 * 1024; // Total ms budget reserved for all batches (leaves headroom below the 300s Stop timeout) @@ -60,9 +61,9 @@ function isPluginClonePath(filePath, cwd = process.cwd(), homeDir = os.homedir() }); } -function getAccumFile() { +function getAccumFile(rawSessionId) { const raw = - process.env.CLAUDE_SESSION_ID || + rawSessionId || crypto.createHash('sha1').update(process.cwd()).digest('hex').slice(0, 12); const sessionId = raw.replace(/[^a-zA-Z0-9_-]/g, '_').slice(0, 64); return path.join(os.tmpdir(), `ecc-edited-${sessionId}.txt`); @@ -156,8 +157,8 @@ function typecheckBatch(tsConfigDir, editedFiles, timeoutMs) { } } -function main() { - const accumFile = getAccumFile(); +function main(sessionId) { + const accumFile = getAccumFile(sessionId); let raw; try { @@ -212,7 +213,7 @@ function main() { */ function run(rawInput) { try { - main(); + main(resolveHookSessionId(rawInput)); } catch (err) { process.stderr.write(`[Hook] stop-format-typecheck error: ${err.message}\n`); } diff --git a/scripts/lib/hook-session.js b/scripts/lib/hook-session.js new file mode 100644 index 000000000..ad53d08f1 --- /dev/null +++ b/scripts/lib/hook-session.js @@ -0,0 +1,25 @@ +'use strict'; + +/** + * Resolve a raw hook session ID from stdin before legacy environment fallbacks. + * Callers must retain their own filename sanitization at the storage boundary. + * @param {string} rawInput - Hook stdin JSON. + * @param {object} env - Environment variables; defaults to the current process. + * @returns {string} A nonblank session ID, or an empty string when unavailable. + */ +function resolveHookSessionId(rawInput, env = process.env) { + let sessionId; + try { + const input = JSON.parse(rawInput); + if (input && typeof input === 'object' && !Array.isArray(input)) { + sessionId = input.session_id; + } + } catch { + // Empty or malformed hook input can still use an environment session ID. + } + + return [sessionId, env.ECC_SESSION_ID, env.CLAUDE_SESSION_ID] + .find(value => typeof value === 'string' && value.trim().length > 0) || ''; +} + +module.exports = { resolveHookSessionId }; diff --git a/tests/hooks/observer-session-lifecycle.test.js b/tests/hooks/observer-session-lifecycle.test.js new file mode 100644 index 000000000..b9a0d1568 --- /dev/null +++ b/tests/hooks/observer-session-lifecycle.test.js @@ -0,0 +1,227 @@ +const assert = require('assert'); +const fs = require('fs'); +const os = require('os'); +const path = require('path'); +const vm = require('vm'); +const { createRequire } = require('module'); +const { spawnSync } = require('child_process'); + +const hooksDir = path.resolve(__dirname, '../../scripts/hooks'); +let passed = 0; +let failed = 0; + +function test(name, fn) { + try { + fn(); + console.log(` ✓ ${name}`); + passed += 1; + } catch (error) { + console.log(` ✗ ${name}`); + console.log(` ${error.message}`); + failed += 1; + } +} + +function withFixture(fn, extraEnv = {}) { + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'ecc-observer-lifecycle-')); + const project = path.join(root, 'project'); + const store = path.join(root, 'homunculus'); + const observerDir = path.join(store, 'projects', 'fixture-project'); + fs.mkdirSync(project, { recursive: true }); + fs.mkdirSync(observerDir, { recursive: true }); + fs.writeFileSync(path.join(store, 'projects.json'), JSON.stringify({ + fixture: { id: 'fixture-project', root: project } + })); + const overrides = { + HOME: root, + USERPROFILE: root, + ECC_AGENT_DATA_HOME: path.join(root, 'agent-data'), + CLAUDE_CONFIG_DIR: path.join(root, 'claude-config'), + CLV2_HOMUNCULUS_DIR: store, + XDG_DATA_HOME: path.join(root, 'xdg'), + CLAUDE_PROJECT_DIR: project, + CLAUDE_PACKAGE_MANAGER: 'npm', + ECC_SESSION_START_CONTEXT: 'off', + ECC_SESSION_ID: undefined, + CLAUDE_SESSION_ID: undefined, + ...extraEnv + }; + for (const key of ['GIT_DIR', 'GIT_WORK_TREE', 'GIT_INDEX_FILE', 'GIT_COMMON_DIR', 'GIT_PREFIX']) { + overrides[key] = undefined; + } + const env = { ...process.env, ...overrides }; + for (const [key, value] of Object.entries(env)) { + if (value === undefined) delete env[key]; + } + try { + fn({ project, observerDir, overrides, env }); + } finally { + fs.rmSync(root, { recursive: true, force: true }); + } +} + +function runHook(name, input, fixture) { + const result = spawnSync(process.execPath, [path.join(hooksDir, name)], { + cwd: fixture.project, + env: fixture.env, + input, + encoding: 'utf8', + timeout: 10000 + }); + assert.ifError(result.error); + assert.strictEqual(result.status, 0, result.stderr); + return result; +} + +function leaseFile(fixture, sessionId) { + return path.join(fixture.observerDir, '.observer-sessions', `${sessionId}.json`); +} + +function seedLease(fixture, sessionId) { + const filename = leaseFile(fixture, sessionId); + fs.mkdirSync(path.dirname(filename), { recursive: true }); + fs.writeFileSync(filename, JSON.stringify({ sessionId })); +} + +function assertStartOutput(result) { + assert.deepStrictEqual(JSON.parse(result.stdout), { + hookSpecificOutput: { hookEventName: 'SessionStart', additionalContext: '' } + }); + assert.ok(!result.stderr.includes('[SessionStart] Error:'), result.stderr); +} + +function withEnv(overrides, fn) { + const previous = Object.fromEntries(Object.keys(overrides).map(key => [key, process.env[key]])); + for (const [key, value] of Object.entries(overrides)) { + if (value === undefined) delete process.env[key]; + else process.env[key] = value; + } + try { + return fn(); + } finally { + for (const [key, value] of Object.entries(previous)) { + if (value === undefined) delete process.env[key]; + else process.env[key] = value; + } + } +} + +function loadEndMarkerWithStubbedStop(stops) { + const filename = path.join(hooksDir, 'session-end-marker.js'); + const localRequire = createRequire(filename); + const module = { exports: {} }; + const requireStub = id => id === '../lib/observer-sessions' + ? { ...localRequire(id), stopObserverForContext: context => { stops.push(context.projectId); return true; } } + : localRequire(id); + vm.runInNewContext(fs.readFileSync(filename, 'utf8'), { + require: requireStub, module, + process: { stderr: { write() {} }, env: process.env } + }, { filename }); + return module.exports; +} + +console.log('\n=== Observer session lifecycle tests ===\n'); + +test('SessionStart creates separate observer leases from stdin without environment IDs', () => { + withFixture(fixture => { + for (const sessionId of ['payload-one', 'payload-two']) { + assertStartOutput(runHook('session-start.js', JSON.stringify({ + session_id: sessionId, hook_event_name: 'SessionStart', source: 'startup' + }), fixture)); + const lease = JSON.parse(fs.readFileSync(leaseFile(fixture, sessionId), 'utf8')); + assert.strictEqual(lease.sessionId, sessionId); + assert.strictEqual(lease.hook, 'SessionStart'); + assert.strictEqual(lease.projectRoot, fixture.project); + } + assert.strictEqual(fs.readdirSync(path.dirname(leaseFile(fixture, 'payload-one'))).length, 2); + }); +}); + +test('SessionEnd CLI removes the payload lease, retains others, then cleans up the final observer', () => { + withFixture(fixture => { + seedLease(fixture, 'first'); + seedLease(fixture, 'second'); + const pidFile = path.join(fixture.observerDir, '.observer.pid'); + // PID 1 is rejected before process.kill, so this fixture never signals any process. + fs.writeFileSync(pidFile, '1'); + for (const sessionId of ['first', 'second']) { + const input = ` {"session_id":"${sessionId}","hook_event_name":"SessionEnd"}\n`; + assert.strictEqual(runHook('session-end-marker.js', input, fixture).stdout, input); + assert.ok(!fs.existsSync(leaseFile(fixture, sessionId))); + assert.strictEqual(fs.existsSync(pidFile), sessionId === 'first'); + if (sessionId === 'first') assert.ok(fs.existsSync(leaseFile(fixture, 'second'))); + } + }); +}); + +test('in-process SessionEnd stops the observer only after the final payload lease ends', () => { + withFixture(fixture => withEnv(fixture.overrides, () => { + seedLease(fixture, 'first'); + seedLease(fixture, 'second'); + const stops = []; + const { run } = loadEndMarkerWithStubbedStop(stops); + const first = '{"session_id":"first"}\n'; + assert.strictEqual(run(first), first); + assert.ok(!fs.existsSync(leaseFile(fixture, 'first'))); + assert.ok(fs.existsSync(leaseFile(fixture, 'second'))); + assert.deepStrictEqual(stops, []); + const second = '{"session_id":"second"}\n'; + assert.strictEqual(run(second), second); + assert.ok(!fs.existsSync(leaseFile(fixture, 'second'))); + assert.deepStrictEqual(stops, ['fixture-project']); + })); +}); + +for (const envKey of ['ECC_SESSION_ID', 'CLAUDE_SESSION_ID']) { + test(`both lifecycle hooks retain ${envKey} fallback for malformed stdin`, () => { + withFixture(fixture => { + const input = '{malformed payload\n'; + assertStartOutput(runHook('session-start.js', input, fixture)); + assert.ok(fs.existsSync(leaseFile(fixture, 'env-session'))); + assert.strictEqual(runHook('session-end-marker.js', input, fixture).stdout, input); + assert.ok(!fs.existsSync(leaseFile(fixture, 'env-session'))); + }, { [envKey]: 'env-session' }); + }); +} + +test('payload ID overrides environment IDs in both lifecycle hooks', () => { + withFixture(fixture => { + const input = '{"session_id":"payload-session","hook_event_name":"SessionStart","source":"startup"}'; + assertStartOutput(runHook('session-start.js', input, fixture)); + assert.ok(fs.existsSync(leaseFile(fixture, 'payload-session'))); + assert.ok(!fs.existsSync(leaseFile(fixture, 'ecc-session'))); + assert.ok(!fs.existsSync(leaseFile(fixture, 'claude-session'))); + seedLease(fixture, 'ecc-session'); + assert.strictEqual(runHook('session-end-marker.js', input, fixture).stdout, input); + assert.ok(!fs.existsSync(leaseFile(fixture, 'payload-session'))); + assert.ok(fs.existsSync(leaseFile(fixture, 'ecc-session'))); + }, { ECC_SESSION_ID: 'ecc-session', CLAUDE_SESSION_ID: 'claude-session' }); +}); + +test('observer lease filenames sanitize path characters and Windows reserved IDs', () => { + withFixture(fixture => { + for (const sessionId of ['../session\\alpha:*?', 'CON']) { + const input = JSON.stringify({ session_id: sessionId, hook_event_name: 'SessionStart', source: 'startup' }); + assertStartOutput(runHook('session-start.js', input, fixture)); + const leaseDir = path.dirname(leaseFile(fixture, 'unused')); + const entries = fs.readdirSync(leaseDir); + assert.strictEqual(entries.length, 1); + assert.match(entries[0], sessionId === 'CON' ? /^CON-[a-f0-9]{6}\.json$/ : /^session-alpha\.json$/); + assert.strictEqual(runHook('session-end-marker.js', input, fixture).stdout, input); + assert.deepStrictEqual(fs.readdirSync(leaseDir), []); + } + }); +}); + +test('null JSON is harmless without session IDs and SessionEnd preserves the raw payload', () => { + withFixture(fixture => { + const input = 'null\n'; + assertStartOutput(runHook('session-start.js', input, fixture)); + assert.strictEqual(runHook('session-end-marker.js', input, fixture).stdout, input); + assert.ok(!fs.existsSync(path.dirname(leaseFile(fixture, 'unused')))); + }); +}); + +console.log(`\nPassed: ${passed}`); +console.log(`Failed: ${failed}`); +process.exit(failed > 0 ? 1 : 0); diff --git a/tests/hooks/stop-format-typecheck.test.js b/tests/hooks/stop-format-typecheck.test.js index 564bae693..c1b1a5135 100644 --- a/tests/hooks/stop-format-typecheck.test.js +++ b/tests/hooks/stop-format-typecheck.test.js @@ -33,7 +33,9 @@ let failed = 0; // Use a unique session ID for tests so we don't pollute real sessions const TEST_SESSION_ID = `test-${Date.now()}`; const origSessionId = process.env.CLAUDE_SESSION_ID; +const origEccSessionId = process.env.ECC_SESSION_ID; process.env.CLAUDE_SESSION_ID = TEST_SESSION_ID; +delete process.env.ECC_SESSION_ID; function getAccumFile() { return path.join(os.tmpdir(), `ecc-edited-${TEST_SESSION_ID}.txt`); @@ -233,6 +235,91 @@ if (test('stop hook passes stdin through unchanged', () => { assert.strictEqual(result.toString(), input); })) passed++; else failed++; +console.log('\nsession-scoped accumulator regression (#3460)'); + +function withPayloadSessions(fn) { + const sessionA = `${TEST_SESSION_ID}-payload-a`; + const sessionB = `${TEST_SESSION_ID}-payload-b`; + const fileA = path.join(os.tmpdir(), `ecc-edited-${sessionA}.txt`); + const fileB = path.join(os.tmpdir(), `ecc-edited-${sessionB}.txt`); + try { + cleanAccumFile(); + fn({ sessionA, sessionB, fileA, fileB }); + } finally { + for (const file of [fileA, fileB]) fs.rmSync(file, { force: true }); + cleanAccumFile(); + } +} + +if (test('stdin session IDs isolate edits from sibling sessions in the same cwd', () => { + withPayloadSessions(({ sessionA, sessionB, fileA, fileB }) => { + accumulator.run(JSON.stringify({ session_id: sessionA, tool_input: { file_path: '/nonexistent/a.ts' } })); + accumulator.run(JSON.stringify({ session_id: sessionB, tool_input: { file_path: '/nonexistent/b.ts' } })); + assert.strictEqual(fs.readFileSync(fileA, 'utf8'), '/nonexistent/a.ts\n'); + assert.strictEqual(fs.readFileSync(fileB, 'utf8'), '/nonexistent/b.ts\n'); + assert.ok(!fs.existsSync(getAccumFile()), 'stdin must override the environment session'); + }); +})) passed++; else failed++; + +if (test('Stop consumes only its stdin session accumulator and preserves pass-through', () => { + withPayloadSessions(({ sessionA, sessionB, fileA, fileB }) => { + fs.writeFileSync(fileA, '/nonexistent/a.ts\n'); + fs.writeFileSync(fileB, '/nonexistent/b.ts\n'); + const stopHook = require('../../scripts/hooks/stop-format-typecheck'); + const inputA = JSON.stringify({ session_id: sessionA, stop_reason: 'end_turn' }); + assert.strictEqual(stopHook.run(inputA), inputA); + assert.ok(!fs.existsSync(fileA), 'completed session accumulator must be removed'); + assert.strictEqual(fs.readFileSync(fileB, 'utf8'), '/nonexistent/b.ts\n'); + stopHook.run(JSON.stringify({ session_id: sessionB })); + assert.ok(!fs.existsSync(fileB), 'second session should consume its own accumulator'); + }); +})) passed++; else failed++; + +if (test('MultiEdit uses the same stdin session accumulator for every edit', () => { + withPayloadSessions(({ sessionA, fileA }) => { + accumulator.run(JSON.stringify({ + session_id: sessionA, + tool_input: { edits: [{ file_path: '/nonexistent/a.ts' }, { file_path: '/nonexistent/b.js' }] } + })); + assert.strictEqual(fs.readFileSync(fileA, 'utf8'), '/nonexistent/a.ts\n/nonexistent/b.js\n'); + }); +})) passed++; else failed++; + +if (test('ECC_SESSION_ID precedes CLAUDE_SESSION_ID while preserving legacy filenames', () => { + const rawId = `${TEST_SESSION_ID}/ecc.with spaces`; + const safeId = rawId.replace(/[^a-zA-Z0-9_-]/g, '_').slice(0, 64); + const file = path.join(os.tmpdir(), `ecc-edited-${safeId}.txt`); + process.env.ECC_SESSION_ID = rawId; + try { + cleanAccumFile(); + accumulator.run(JSON.stringify({ tool_input: { file_path: '/nonexistent/ecc.ts' } })); + assert.strictEqual(fs.readFileSync(file, 'utf8'), '/nonexistent/ecc.ts\n'); + assert.ok(!fs.existsSync(getAccumFile())); + const stopHook = require('../../scripts/hooks/stop-format-typecheck'); + assert.strictEqual(stopHook.run('malformed JSON'), 'malformed JSON'); + assert.ok(!fs.existsSync(file), 'malformed Stop input must retain the environment fallback'); + } finally { + delete process.env.ECC_SESSION_ID; + fs.rmSync(file, { force: true }); + cleanAccumFile(); + } +})) passed++; else failed++; + +if (test('stdin IDs keep the existing filename sanitizer and length limit', () => { + const rawId = `${TEST_SESSION_ID}/../nested\\session ${'x'.repeat(100)}`; + const safeId = rawId.replace(/[^a-zA-Z0-9_-]/g, '_').slice(0, 64); + const file = path.join(os.tmpdir(), `ecc-edited-${safeId}.txt`); + try { + accumulator.run(JSON.stringify({ session_id: rawId, tool_input: { file_path: '/nonexistent/safe.ts' } })); + assert.strictEqual(fs.readFileSync(file, 'utf8'), '/nonexistent/safe.ts\n'); + require('../../scripts/hooks/stop-format-typecheck').run(JSON.stringify({ session_id: rawId })); + assert.ok(!fs.existsSync(file)); + } finally { + fs.rmSync(file, { force: true }); + cleanAccumFile(); + } +})) passed++; else failed++; + // --- Plugin and marketplace clones are read, not owned: never format them --- const FAKE_HOME = path.join(path.sep, 'home', 'someone'); @@ -279,6 +366,11 @@ if (origSessionId === undefined) { } else { process.env.CLAUDE_SESSION_ID = origSessionId; } +if (origEccSessionId === undefined) { + delete process.env.ECC_SESSION_ID; +} else { + process.env.ECC_SESSION_ID = origEccSessionId; +} console.log(`\n=== Test Results ===`); console.log(`Passed: ${passed}`); diff --git a/tests/lib/hook-session.test.js b/tests/lib/hook-session.test.js new file mode 100644 index 000000000..13bc66a35 --- /dev/null +++ b/tests/lib/hook-session.test.js @@ -0,0 +1,85 @@ +const assert = require('assert'); +const { resolveHookSessionId } = require('../../scripts/lib/hook-session'); + +let passed = 0; +let failed = 0; + +function test(name, fn) { + try { + fn(); + console.log(` ✓ ${name}`); + passed += 1; + } catch (error) { + console.log(` ✗ ${name}`); + console.log(` ${error.message}`); + failed += 1; + } +} + +console.log('\n=== Hook session ID tests ===\n'); + +test('payload session_id takes precedence over both environment IDs', () => { + assert.strictEqual(resolveHookSessionId('{"session_id":"payload-session"}', { + ECC_SESSION_ID: 'ecc-session', CLAUDE_SESSION_ID: 'claude-session' + }), 'payload-session'); +}); + +test('ECC_SESSION_ID takes precedence over CLAUDE_SESSION_ID', () => { + assert.strictEqual(resolveHookSessionId('{}', { + ECC_SESSION_ID: 'ecc-session', CLAUDE_SESSION_ID: 'claude-session' + }), 'ecc-session'); +}); + +test('CLAUDE_SESSION_ID remains the legacy fallback', () => { + assert.strictEqual(resolveHookSessionId('', { CLAUDE_SESSION_ID: 'legacy-session' }), 'legacy-session'); +}); + +test('malformed and empty input safely fall back to the environment', () => { + for (const input of ['', ' ', '{bad json', undefined, null]) { + assert.strictEqual(resolveHookSessionId(input, { ECC_SESSION_ID: 'fallback' }), 'fallback'); + } +}); + +test('nonobject JSON does not supply a session ID', () => { + for (const input of ['null', '[]', '[{"session_id":"nested"}]', '42', 'true', '"session"']) { + assert.strictEqual(resolveHookSessionId(input, { CLAUDE_SESSION_ID: 'fallback' }), 'fallback'); + } +}); + +test('invalid payload ID types and blank IDs fall back safely', () => { + for (const sessionId of [null, 42, true, {}, [], '', ' \t\n']) { + assert.strictEqual(resolveHookSessionId(JSON.stringify({ session_id: sessionId }), { + ECC_SESSION_ID: 'fallback' + }), 'fallback'); + } +}); + +test('invalid ECC environment IDs fall back to the Claude environment ID', () => { + for (const sessionId of [undefined, null, 42, true, {}, [], '', ' \t\n']) { + assert.strictEqual(resolveHookSessionId('{}', { + ECC_SESSION_ID: sessionId, CLAUDE_SESSION_ID: 'legacy-session' + }), 'legacy-session'); + } +}); + +test('missing or invalid IDs return an empty string', () => { + assert.strictEqual(resolveHookSessionId('{}', {}), ''); + assert.strictEqual(resolveHookSessionId('{"session_id":false}', { + ECC_SESSION_ID: ' ', CLAUDE_SESSION_ID: null + }), ''); +}); + +test('only the top-level session_id field is accepted', () => { + assert.strictEqual(resolveHookSessionId('{"nested":{"session_id":"nested"},"sessionId":"camel"}', {}), ''); +}); + +test('raw IDs remain intact for each caller to apply its filename sanitizer', () => { + for (const sessionId of ['../session\\alpha:*?', 'CON', 'session:one', ' session-one ', 'sesión']) { + assert.strictEqual(resolveHookSessionId(JSON.stringify({ session_id: sessionId }), {}), sessionId); + assert.strictEqual(resolveHookSessionId('', { ECC_SESSION_ID: sessionId }), sessionId); + } +}); + +console.log(`\nPassed: ${passed}`); +console.log(`Failed: ${failed}`); +process.exit(failed > 0 ? 1 : 0); diff --git a/tests/scripts/npm-publish-surface.test.js b/tests/scripts/npm-publish-surface.test.js index 02ca28943..65bc80029 100644 --- a/tests/scripts/npm-publish-surface.test.js +++ b/tests/scripts/npm-publish-surface.test.js @@ -221,6 +221,7 @@ function main() { "scripts/lib/nasiko-release.js", "scripts/lib/memory-vault-format.js", "scripts/lib/memory-vault.js", + "scripts/lib/hook-session.js", "scripts/discussion-audit.js", "scripts/operator-readiness-dashboard.js", "scripts/preview-pack-smoke.js", From 20cd70e06d16d1013a3f3fb102d8aa3a675664bc Mon Sep 17 00:00:00 2001 From: montjeffrey Date: Fri, 9 Oct 2026 14:32:52 -0400 Subject: [PATCH 21/26] test: stop Windows developer tooling from leaking into setup, tar, and audit tests On a Windows machine with Claude Code installed natively, Git for Windows, and the ECC plugin installed, `node tests/run-all.js` on main fails 44 tests in 7 files. Linux CI never sees them. Three of the four causes are test isolation gaps, fixed here (43 tests): - claude-plugin-setup, claude-scope-migration, setup (38): fixtures put a fake claude.cmd first on PATH, but on Windows spawning a bare `claude` resolves only .com/.exe, so a native ~/.local/bin/claude.exe ran instead of the fake. Fixture PATHs now drop entries holding claude.exe/claude.com. - ecc-universal-bin, eval-harness-package (4): tar received an absolute C:\ archive path, which Git Bash's GNU tar reads as a host:path remote spec ("Cannot connect to C: resolve failed"). Extract from inside the destination with a relative, forward-slash archive name, which both GNU tar and bsdtar accept. - harness-audit (1): the cache-layout test overrode HOME only, while findPluginInstall also reads USERPROFILE and os.homedir(), so the real installed plugin won over the fixture. Override USERPROFILE too. The two helpers live in tests/lib/helpers/windows-test-env.js with a platform-independent test, so Linux CI holds both invariants too. The 44th failure, plugin-hook-bootstrap's no-bash test, is a production bug rather than a test leak: System32\bash.exe launches an installed WSL distro, which passes the bash probe. It gets a separate fix. --- tests/lib/claude-plugin-setup.test.js | 3 +- tests/lib/claude-scope-migration.test.js | 3 +- tests/lib/helpers/windows-test-env.js | 38 +++++++++++++ tests/lib/windows-test-env.test.js | 62 ++++++++++++++++++++++ tests/scripts/ecc-universal-bin.test.js | 5 +- tests/scripts/eval-harness-package.test.js | 3 +- tests/scripts/harness-audit.test.js | 9 ++++ tests/scripts/setup.test.js | 5 +- 8 files changed, 121 insertions(+), 7 deletions(-) create mode 100644 tests/lib/helpers/windows-test-env.js create mode 100644 tests/lib/windows-test-env.test.js diff --git a/tests/lib/claude-plugin-setup.test.js b/tests/lib/claude-plugin-setup.test.js index cdda9f5f0..f3715d3c5 100644 --- a/tests/lib/claude-plugin-setup.test.js +++ b/tests/lib/claude-plugin-setup.test.js @@ -4,6 +4,7 @@ const assert = require('assert'); const fs = require('fs'); const os = require('os'); const path = require('path'); +const { withoutShadowingClaude } = require('./helpers/windows-test-env'); const repoRoot = path.join(__dirname, '..', '..'); const fakeClaudeScript = path.join(repoRoot, 'tests', 'fixtures', 'fake-claude-plugin.js'); @@ -87,7 +88,7 @@ function withFixture(initialState, fn) { process.chdir(fixture.projectRoot); process.env.HOME = fixture.homeDir; process.env.USERPROFILE = fixture.homeDir; - process.env.PATH = `${fixture.binDir}${path.delimiter}${previous.PATH || ''}`; + process.env.PATH = `${fixture.binDir}${path.delimiter}${withoutShadowingClaude(previous.PATH)}`; process.env.CLAUDE_CONFIG_DIR = fixture.configDir; process.env.ECC_TEST_CLAUDE_STATE = fixture.statePath; process.env.ECC_TEST_CLAUDE_CALLS = fixture.callsPath; diff --git a/tests/lib/claude-scope-migration.test.js b/tests/lib/claude-scope-migration.test.js index 91ce940ba..c3ab6606f 100644 --- a/tests/lib/claude-scope-migration.test.js +++ b/tests/lib/claude-scope-migration.test.js @@ -4,6 +4,7 @@ const assert = require('assert'); const fs = require('fs'); const os = require('os'); const path = require('path'); +const { withoutShadowingClaude } = require('./helpers/windows-test-env'); const repoRoot = path.join(__dirname, '..', '..'); const fakeClaudeScript = path.join(repoRoot, 'tests', 'fixtures', 'fake-claude-plugin.js'); @@ -100,7 +101,7 @@ function withFixture(initialState, fn) { process.chdir(fixture.projectRoot); process.env.HOME = fixture.homeDir; process.env.USERPROFILE = fixture.homeDir; - process.env.PATH = `${fixture.binDir}${path.delimiter}${previous.PATH || ''}`; + process.env.PATH = `${fixture.binDir}${path.delimiter}${withoutShadowingClaude(previous.PATH)}`; process.env.CLAUDE_CONFIG_DIR = fixture.configDir; process.env.ECC_TEST_CLAUDE_STATE = fixture.statePath; process.env.ECC_TEST_CLAUDE_CALLS = fixture.callsPath; diff --git a/tests/lib/helpers/windows-test-env.js b/tests/lib/helpers/windows-test-env.js new file mode 100644 index 000000000..9ccfeefa6 --- /dev/null +++ b/tests/lib/helpers/windows-test-env.js @@ -0,0 +1,38 @@ +/** + * Keep tests independent of tools installed on a Windows developer machine. + * + * Linux CI never sees these leaks, so they surface only as local failures. + */ + +'use strict'; + +const fs = require('fs'); +const path = require('path'); + +// On Windows, spawning a bare `claude` resolves only .com/.exe on PATH, never +// .cmd. A natively installed claude.exe therefore wins over a test's fake +// claude.cmd even when the fake's directory comes first on PATH. +const SHADOWING_CLAUDE_BINARIES = ['claude.com', 'claude.exe']; + +/** + * Drop PATH entries holding a real Claude binary that would shadow a fake + * `claude.cmd` launcher. A no-op off Windows, where PATH order is honoured. + */ +function withoutShadowingClaude(pathValue, platform = process.platform) { + if (platform !== 'win32') return pathValue || ''; + return String(pathValue || '') + .split(path.win32.delimiter) + .filter(dir => dir && !SHADOWING_CLAUDE_BINARIES.some(name => fs.existsSync(path.join(dir, name)))) + .join(path.win32.delimiter); +} + +/** + * Name an archive relative to the directory tar runs in, with forward slashes. + * GNU tar, which Git for Windows puts first on PATH, reads an archive argument + * containing a colon (C:\...) as a host:path remote spec; bsdtar does not. + */ +function toRelativeTarPath(cwd, archivePath) { + return path.relative(cwd, archivePath).split(path.sep).join('/'); +} + +module.exports = { withoutShadowingClaude, toRelativeTarPath }; diff --git a/tests/lib/windows-test-env.test.js b/tests/lib/windows-test-env.test.js new file mode 100644 index 000000000..be888271f --- /dev/null +++ b/tests/lib/windows-test-env.test.js @@ -0,0 +1,62 @@ +/** + * Tests for tests/lib/helpers/windows-test-env.js + * + * Run with: node tests/lib/windows-test-env.test.js + */ + +'use strict'; + +const assert = require('assert'); +const fs = require('fs'); +const os = require('os'); +const path = require('path'); + +const { test, banner, summary } = require('./helpers/mini-test-runner'); +const { withoutShadowingClaude, toRelativeTarPath } = require('./helpers/windows-test-env'); + +banner('windows-test-env helper'); + +let passed = 0; +let failed = 0; + +const root = fs.mkdtempSync(path.join(os.tmpdir(), 'windows-test-env-')); +const exeDir = path.join(root, 'native-install'); +const comDir = path.join(root, 'com-install'); +const cmdDir = path.join(root, 'npm-global'); +const plainDir = path.join(root, 'tools'); +for (const dir of [exeDir, comDir, cmdDir, plainDir]) fs.mkdirSync(dir); +fs.writeFileSync(path.join(exeDir, 'claude.exe'), ''); +fs.writeFileSync(path.join(comDir, 'claude.com'), ''); +// A .cmd shim cannot shadow a fake .cmd placed earlier on PATH, so it stays. +fs.writeFileSync(path.join(cmdDir, 'claude.cmd'), ''); + +try { + if (test('drops PATH entries holding claude.exe or claude.com on Windows', () => { + const pathValue = [plainDir, exeDir, cmdDir, comDir].join(';'); + assert.strictEqual(withoutShadowingClaude(pathValue, 'win32'), [plainDir, cmdDir].join(';')); + })) passed++; else failed++; + + if (test('drops empty entries and tolerates a missing PATH on Windows', () => { + assert.strictEqual(withoutShadowingClaude(`;${plainDir};;`, 'win32'), plainDir); + assert.strictEqual(withoutShadowingClaude(undefined, 'win32'), ''); + })) passed++; else failed++; + + if (test('leaves PATH untouched off Windows', () => { + const pathValue = [plainDir, exeDir].join(':'); + assert.strictEqual(withoutShadowingClaude(pathValue, 'linux'), pathValue); + assert.strictEqual(withoutShadowingClaude(undefined, 'darwin'), ''); + })) passed++; else failed++; + + if (test('names an archive relatively with forward slashes and no drive letter', () => { + const cwd = path.join(root, 'extract', 'node_modules'); + const archive = path.join(root, 'pkg-1.0.0.tgz'); + const relative = toRelativeTarPath(cwd, archive); + assert.ok(!relative.includes(':'), `tar reads a colon as host:path: ${relative}`); + assert.ok(!relative.includes('\\'), `expected forward slashes: ${relative}`); + assert.strictEqual(path.resolve(cwd, relative), archive); + })) passed++; else failed++; +} finally { + fs.rmSync(root, { recursive: true, force: true }); +} + +summary(passed, failed); diff --git a/tests/scripts/ecc-universal-bin.test.js b/tests/scripts/ecc-universal-bin.test.js index c4336d26a..27f508e95 100644 --- a/tests/scripts/ecc-universal-bin.test.js +++ b/tests/scripts/ecc-universal-bin.test.js @@ -12,6 +12,7 @@ const os = require('os'); const path = require('path'); const { spawnSync } = require('child_process'); const { getNpmPackEntry } = require('../lib/npm-pack-output'); +const { toRelativeTarPath } = require('../lib/helpers/windows-test-env'); const repoRoot = path.join(__dirname, '..', '..'); const packageJson = JSON.parse( @@ -170,8 +171,8 @@ function prepareLocalPackedProject(packageManager) { }); } fs.mkdirSync(modulesDirectory, { recursive: true }); - run('tar', ['-xzf', fixture.archivePath, '-C', modulesDirectory], { - cwd: projectDirectory, + run('tar', ['-xzf', toRelativeTarPath(modulesDirectory, fixture.archivePath)], { + cwd: modulesDirectory, timeout: archiveExtractionTimeoutMs, }); fs.renameSync(extractedDirectory, packageDirectory); diff --git a/tests/scripts/eval-harness-package.test.js b/tests/scripts/eval-harness-package.test.js index b29adf69d..5d413ca8c 100644 --- a/tests/scripts/eval-harness-package.test.js +++ b/tests/scripts/eval-harness-package.test.js @@ -8,6 +8,7 @@ const path = require('path'); const { spawnSync } = require('child_process'); const { getNpmPackEntry } = require('../lib/npm-pack-output'); const { test, tempDir, cleanup, finish, runNpm } = require('../lib/eval-harness/helpers'); +const { toRelativeTarPath } = require('../lib/helpers/windows-test-env'); const repo = path.resolve(__dirname, '../..'); const work = tempDir('package smoke'); @@ -61,7 +62,7 @@ try { assert.ok(archive, 'packing must succeed before extracting'); const extract = path.join(work, 'extracted'); fs.mkdirSync(extract); - const unpack = command('tar', ['-xzf', archive, '-C', extract]); + const unpack = command('tar', ['-xzf', toRelativeTarPath(extract, archive)], { cwd: extract }); assert.strictEqual(unpack.status, 0, unpack.error?.message || unpack.stderr); const installed = path.join(extract, 'package'); assert.ok(!fs.existsSync(path.join(installed, 'node_modules'))); diff --git a/tests/scripts/harness-audit.test.js b/tests/scripts/harness-audit.test.js index c0087475d..c7572777c 100644 --- a/tests/scripts/harness-audit.test.js +++ b/tests/scripts/harness-audit.test.js @@ -635,8 +635,12 @@ function runTests() { ); } + // findPluginInstall also reads USERPROFILE and os.homedir() (USERPROFILE + // on Windows), so isolating HOME alone lets a real install win. const originalHome = process.env.HOME; + const originalUserProfile = process.env.USERPROFILE; process.env.HOME = homeDir; + process.env.USERPROFILE = homeDir; try { const found = findPluginInstall(projectRoot); assert.ok(found); @@ -647,6 +651,11 @@ function runTests() { } else { process.env.HOME = originalHome; } + if (originalUserProfile === undefined) { + delete process.env.USERPROFILE; + } else { + process.env.USERPROFILE = originalUserProfile; + } } } finally { cleanup(homeDir); diff --git a/tests/scripts/setup.test.js b/tests/scripts/setup.test.js index babc6d946..715d383c2 100644 --- a/tests/scripts/setup.test.js +++ b/tests/scripts/setup.test.js @@ -5,6 +5,7 @@ const fs = require('fs'); const os = require('os'); const path = require('path'); const { spawnSync } = require('child_process'); +const { withoutShadowingClaude } = require('../lib/helpers/windows-test-env'); const repoRoot = path.join(__dirname, '..', '..'); const setupScript = path.join(repoRoot, 'scripts', 'setup.js'); const eccScript = path.join(repoRoot, 'scripts', 'ecc.js'); @@ -62,7 +63,7 @@ function runSetup(fixture, args, options = {}) { HOME: fixture.homeDir, USERPROFILE: fixture.homeDir, CLAUDE_CONFIG_DIR: fixture.configDir, - PATH: options.path || `${fixture.binDir}${path.delimiter}${process.env.PATH || ''}`, + PATH: options.path || `${fixture.binDir}${path.delimiter}${withoutShadowingClaude(process.env.PATH)}`, ECC_TEST_CLAUDE_STATE: fixture.statePath, ECC_TEST_CLAUDE_CALLS: fixture.callsPath, ...options.env, @@ -147,7 +148,7 @@ function runInteractiveEccSetup(fixture, options = {}) { HOME: fixture.homeDir, USERPROFILE: fixture.homeDir, CLAUDE_CONFIG_DIR: fixture.configDir, - PATH: `${fixture.binDir}${path.delimiter}${process.env.PATH || ''}`, + PATH: `${fixture.binDir}${path.delimiter}${withoutShadowingClaude(process.env.PATH)}`, ECC_TEST_CLAUDE_STATE: fixture.statePath, ECC_TEST_CLAUDE_CALLS: fixture.callsPath, }, From c6b2c55b870219a72e015b5afd081ede813aac4a Mon Sep 17 00:00:00 2001 From: montjeffrey Date: Fri, 9 Oct 2026 15:56:14 -0400 Subject: [PATCH 22/26] fix(hooks): never run .sh hooks through the WSL bash launcher on Windows plugin-hook-bootstrap (shell mode) and observe-runner probed a bare `bash.exe`. On Windows that resolves to the first match on PATH, and %SystemRoot%\System32 normally sits ahead of Git's bin directory, so the pick was System32\bash.exe: the WSL launcher. The existing guard (require a clean probe exit) only catches it when no distro is installed. With one installed, the probe passes inside Linux, then the hook script is handed over by a Windows path (C:\...) or a Git Bash path (/c/...), neither of which exists in WSL. The hook exits 127 instead of running, and observe-runner's continuous-learning observation fails on every call. Resolve bash candidates on Windows to absolute paths from PATH, skipping bash.exe in System32, SysWOW64, Sysnative, and the WindowsApps alias directory, so a native bash later on PATH (Git's) is used. With none available, the existing "no bash binary found" skip applies. An explicit BASH override still wins, and other platforms keep bare names. The shared resolver lives in scripts/lib/windows-bash.js with a platform-independent test. A Windows integration test puts System32 ahead of Git's bash and requires the .sh hook to actually run. Upstream's bootstrap fails that scenario with exit 127 ("/bin/bash: C:Users...hook.sh: No such file or directory"); this one prints the hook output. --- scripts/hooks/observe-runner.js | 5 +- scripts/hooks/plugin-hook-bootstrap.js | 8 +- scripts/lib/windows-bash.js | 87 ++++++++++++++++ tests/hooks/plugin-hook-bootstrap.test.js | 38 +++++++ tests/lib/windows-bash.test.js | 115 ++++++++++++++++++++++ 5 files changed, 250 insertions(+), 3 deletions(-) create mode 100644 scripts/lib/windows-bash.js create mode 100644 tests/lib/windows-bash.test.js diff --git a/scripts/hooks/observe-runner.js b/scripts/hooks/observe-runner.js index c046d75f0..5213eb1e2 100644 --- a/scripts/hooks/observe-runner.js +++ b/scripts/hooks/observe-runner.js @@ -4,6 +4,7 @@ const fs = require('fs'); const path = require('path'); const { spawnSync } = require('child_process'); +const { resolveWindowsBashCandidates } = require('../lib/windows-bash'); const OBSERVE_RELATIVE_PATH = path.join('skills', 'continuous-learning-v2', 'hooks', 'observe.sh'); const DEFAULT_TIMEOUT_MS = 9000; @@ -51,7 +52,9 @@ function findShellBinary() { } if (process.platform === 'win32') { - candidates.push('bash.exe', 'bash', 'sh'); + // Absolute paths, skipping WSL launchers: WSL cannot open the /c/... path + // toShellPath() hands the shell, so observe.sh would fail on every call. + candidates.push(...resolveWindowsBashCandidates(['bash.exe', 'bash', 'sh'])); } else { candidates.push('bash', 'sh'); } diff --git a/scripts/hooks/plugin-hook-bootstrap.js b/scripts/hooks/plugin-hook-bootstrap.js index 233e32980..e8738dcc4 100644 --- a/scripts/hooks/plugin-hook-bootstrap.js +++ b/scripts/hooks/plugin-hook-bootstrap.js @@ -5,6 +5,7 @@ const path = require('path'); const { spawnSync } = require('child_process'); const { ensureAgentDataHomeEnv } = require('../lib/agent-data-home'); const { normalizePluginRootForPlatform } = require('../lib/resolve-ecc-root'); +const { resolveWindowsBashCandidates } = require('../lib/windows-bash'); const { readStdinRaw: readBoundedStdin, resolveMaxStdin } = require('./hook-input'); const SHELL_PROBE_TIMEOUT_MS = 2000; @@ -108,7 +109,8 @@ function findShellBinary() { // bash.exe / conhost.exe processes the way MSYS2/Git Bash does. // Note: PowerShell is only suitable for .ps1 scripts; callers that need // to run .sh scripts (e.g. observe-runner.js) must not use this function. - candidates.push('pwsh.exe', 'powershell.exe', 'bash.exe', 'bash'); + // Bash is named by absolute path so a WSL launcher earlier on PATH is skipped. + candidates.push('pwsh.exe', 'powershell.exe', ...resolveWindowsBashCandidates(['bash.exe', 'bash'])); } else { candidates.push('bash', 'sh'); } @@ -142,7 +144,9 @@ function findBashBinary() { if (process.env.BASH && process.env.BASH.trim() && !isPowerShellBin(process.env.BASH.trim())) { candidates.push(process.env.BASH.trim()); } - candidates.push('bash.exe', 'bash'); + // On Windows these resolve to absolute paths that exclude WSL launchers; + // elsewhere they stay bare names. + candidates.push(...resolveWindowsBashCandidates(['bash.exe', 'bash'])); for (const candidate of candidates) { const probe = spawnSync(candidate, ['-c', ':'], { diff --git a/scripts/lib/windows-bash.js b/scripts/lib/windows-bash.js new file mode 100644 index 000000000..3c4e3ddd9 --- /dev/null +++ b/scripts/lib/windows-bash.js @@ -0,0 +1,87 @@ +'use strict'; + +/** + * Resolve bash on Windows without landing on a WSL launcher. + * + * %SystemRoot%\System32\bash.exe and the %LOCALAPPDATA%\Microsoft\WindowsApps + * alias start a WSL distro. With a distro installed they pass a `bash -c :` + * probe, then cannot open a hook script named by a Windows (C:\...) or Git + * Bash (/c/...) path, so the hook fails (exit 127) instead of being skipped. + * Spawning a bare `bash.exe` takes the first match on PATH, and System32 is + * normally ahead of Git's bin directory. + * + * Tests live at tests/lib/windows-bash.test.js. + */ + +const fs = require('fs'); +const path = require('path'); + +const WSL_LAUNCHER_SYSTEM_DIRS = ['System32', 'SysWOW64', 'Sysnative']; + +// Windows environment names are case-insensitive; a plain env object is not. +function readEnv(env, name) { + if (env[name] !== undefined) return env[name]; + const key = Object.keys(env).find(candidate => candidate.toUpperCase() === name.toUpperCase()); + return key === undefined ? undefined : env[key]; +} + +function normalizeDir(dir) { + return path.resolve(dir).replace(/[\\/]+$/, '').toLowerCase(); +} + +function wslLauncherDirs(env) { + const systemRoot = readEnv(env, 'SystemRoot') || readEnv(env, 'windir') || 'C:\\Windows'; + const dirs = WSL_LAUNCHER_SYSTEM_DIRS.map(name => path.join(systemRoot, name)); + const localAppData = readEnv(env, 'LOCALAPPDATA'); + if (localAppData) dirs.push(path.join(localAppData, 'Microsoft', 'WindowsApps')); + return new Set(dirs.map(normalizeDir)); +} + +/** + * True when filePath is a bash.exe that starts WSL rather than a native bash. + */ +function isWslBashLauncher(filePath, env = process.env) { + return path.basename(filePath).toLowerCase() === 'bash.exe' + && wslLauncherDirs(env).has(normalizeDir(path.dirname(filePath))); +} + +function isFile(filePath) { + try { + return fs.statSync(filePath).isFile(); + } catch { + return false; + } +} + +/** + * Absolute paths of `names` found on PATH, minus WSL bash launchers. + * + * Earlier names win over later ones, then PATH order decides, so `bash` is + * preferred to `sh` wherever each sits. A name without an extension matches + * .com/.exe, as a bare spawn would. Off Windows the names are returned as + * given, leaving lookup to the platform. + */ +function resolveWindowsBashCandidates(names, options = {}) { + const env = options.env || process.env; + const platform = options.platform || process.platform; + if (platform !== 'win32') return [...names]; + + const dirs = String(readEnv(env, 'PATH') || '').split(';').filter(Boolean); + const found = []; + const seen = new Set(); + for (const name of names) { + const fileNames = path.extname(name) ? [name] : [`${name}.com`, `${name}.exe`]; + for (const dir of dirs) { + for (const fileName of fileNames) { + const candidate = path.join(dir, fileName); + const key = candidate.toLowerCase(); + if (seen.has(key) || !isFile(candidate) || isWslBashLauncher(candidate, env)) continue; + seen.add(key); + found.push(candidate); + } + } + } + return found; +} + +module.exports = { isWslBashLauncher, resolveWindowsBashCandidates }; diff --git a/tests/hooks/plugin-hook-bootstrap.test.js b/tests/hooks/plugin-hook-bootstrap.test.js index e1ec6529d..3b3153efb 100644 --- a/tests/hooks/plugin-hook-bootstrap.test.js +++ b/tests/hooks/plugin-hook-bootstrap.test.js @@ -14,6 +14,7 @@ const SCRIPT = path.join(__dirname, '..', '..', 'scripts', 'hooks', 'plugin-hook const LIFECYCLE_SCRIPT = path.join(__dirname, '..', '..', 'scripts', 'hooks', 'lifecycle-hook-bootstrap.js'); const { normalizePluginRootForPlatform, withComparisonInput } = require(SCRIPT); const { resolveTimeout } = require(LIFECYCLE_SCRIPT); +const { resolveWindowsBashCandidates } = require('../../scripts/lib/windows-bash'); function createTempDir() { return fs.mkdtempSync(path.join(os.tmpdir(), 'plugin-hook-bootstrap-')); @@ -464,6 +465,43 @@ process.exit(7); cleanup(root); } })) passed++; else failed++; + + const [nativeBash] = resolveWindowsBashCandidates(['bash.exe']); + if (nativeBash && process.env.SystemRoot) { + if (test('shell mode runs .sh with native bash when the System32 WSL launcher is earlier on PATH', () => { + const root = createTempDir(); + try { + writeFile(root, path.join('scripts', 'hook.sh'), [ + 'input=$(cat)', + 'printf "sh:%s:%s" "$1" "$input"', + '', + ].join('\n')); + + // A bare `bash.exe` resolves to System32\bash.exe here, which starts + // WSL and cannot open the Windows script path (exit 127). + const result = run(['shell', path.join('scripts', 'hook.sh'), 'arg'], { + root, + input: 'payload', + env: { + BASH: '', + PATH: [ + `${process.env.SystemRoot}\\System32\\WindowsPowerShell\\v1.0`, + `${process.env.SystemRoot}\\System32`, + path.dirname(nativeBash), + ].join(';'), + }, + }); + + assert.strictEqual(result.status, 0, result.stderr); + assert.strictEqual(result.stdout, 'sh:arg:payload'); + } finally { + cleanup(root); + } + })) passed++; else failed++; + } else { + skipped += 1; + console.log(' SKIP 1 Windows WSL-launcher test: no native bash.exe on PATH'); + } } } diff --git a/tests/lib/windows-bash.test.js b/tests/lib/windows-bash.test.js new file mode 100644 index 000000000..ae1cd7315 --- /dev/null +++ b/tests/lib/windows-bash.test.js @@ -0,0 +1,115 @@ +/** + * Tests for scripts/lib/windows-bash.js + * + * Platform-independent: the Windows lookup is driven by an injected platform + * and env over a temp tree laid out like a Windows install. + * + * Run with: node tests/lib/windows-bash.test.js + */ + +'use strict'; + +const assert = require('assert'); +const fs = require('fs'); +const os = require('os'); +const path = require('path'); + +const { isWslBashLauncher, resolveWindowsBashCandidates } = require('../../scripts/lib/windows-bash'); + +let passed = 0; +let failed = 0; + +function test(name, fn) { + try { + fn(); + console.log(` ✓ ${name}`); + passed++; + } catch (error) { + console.log(` ✗ ${name}`); + console.log(` Error: ${error.message}`); + failed++; + } +} + +function touch(dir, name) { + fs.mkdirSync(dir, { recursive: true }); + const filePath = path.join(dir, name); + fs.writeFileSync(filePath, ''); + return filePath; +} + +console.log('\n=== Testing windows-bash ===\n'); + +const root = fs.mkdtempSync(path.join(os.tmpdir(), 'windows-bash-')); +const windowsDir = path.join(root, 'Windows'); +const localAppData = path.join(root, 'Local'); +const system32 = path.join(windowsDir, 'System32'); +const windowsApps = path.join(localAppData, 'Microsoft', 'WindowsApps'); +const gitBin = path.join(root, 'Git', 'usr', 'bin'); +const toolsBin = path.join(root, 'tools'); + +touch(system32, 'bash.exe'); +touch(windowsApps, 'bash.exe'); +const gitBash = touch(gitBin, 'bash.exe'); +const gitSh = touch(gitBin, 'sh.exe'); +const toolsSh = touch(toolsBin, 'sh.exe'); + +// The WSL launchers sit ahead of Git on PATH, as on a default install. +const env = { + PATH: [system32, windowsApps, toolsBin, gitBin].join(';'), + SystemRoot: windowsDir, + LOCALAPPDATA: localAppData, +}; +const win32 = { env, platform: 'win32' }; + +try { + test('skips System32 and WindowsApps launchers for a native bash later on PATH', () => { + assert.deepStrictEqual(resolveWindowsBashCandidates(['bash.exe', 'bash'], win32), [gitBash]); + }); + + test('prefers earlier names over PATH order, so bash beats an earlier sh', () => { + assert.deepStrictEqual( + resolveWindowsBashCandidates(['bash.exe', 'bash', 'sh'], win32), + [gitBash, toolsSh, gitSh] + ); + }); + + test('reads Windows environment names case-insensitively', () => { + const mixedCaseEnv = { + Path: env.PATH, + SYSTEMROOT: windowsDir, + LocalAppData: localAppData, + }; + assert.deepStrictEqual( + resolveWindowsBashCandidates(['bash'], { env: mixedCaseEnv, platform: 'win32' }), + [gitBash] + ); + }); + + test('returns nothing when only WSL launchers or no PATH are present', () => { + const launchersOnly = { ...env, PATH: [system32, windowsApps].join(';') }; + assert.deepStrictEqual(resolveWindowsBashCandidates(['bash'], { env: launchersOnly, platform: 'win32' }), []); + assert.deepStrictEqual(resolveWindowsBashCandidates(['bash'], { env: {}, platform: 'win32' }), []); + }); + + test('leaves names untouched off Windows', () => { + assert.deepStrictEqual(resolveWindowsBashCandidates(['bash', 'sh'], { env, platform: 'linux' }), ['bash', 'sh']); + }); + + test('recognises launchers in System32, SysWOW64, and Sysnative regardless of case', () => { + for (const dir of ['SYSTEM32', 'SysWOW64', 'sysnative']) { + assert.ok(isWslBashLauncher(path.join(windowsDir, dir, 'BASH.EXE'), env), dir); + } + assert.ok(isWslBashLauncher(path.join(windowsApps, 'bash.exe'), env)); + }); + + test('does not flag a native bash or a non-bash binary in System32', () => { + assert.ok(!isWslBashLauncher(gitBash, env)); + assert.ok(!isWslBashLauncher(path.join(system32, 'sh.exe'), env)); + }); +} finally { + fs.rmSync(root, { recursive: true, force: true }); +} + +console.log(`\nResults: Passed: ${passed}, Failed: ${failed}`); +process.exit(failed > 0 ? 1 : 0); From 9f9ea5e35bc763af30590c598bef73c6b03e0b24 Mon Sep 17 00:00:00 2001 From: Eneko Ruiz <196690101+eneekoruiz@users.noreply.github.com> Date: Fri, 9 Oct 2026 22:33:51 +0200 Subject: [PATCH 23/26] fix(codex): fail closed when pre-commit cannot find ripgrep --- scripts/codex-git-hooks/pre-commit | 5 +++ tests/scripts/codex-hooks.test.js | 60 ++++++++++++++++++++++++++++++ 2 files changed, 65 insertions(+) diff --git a/scripts/codex-git-hooks/pre-commit b/scripts/codex-git-hooks/pre-commit index b4c608c76..77cad4aad 100644 --- a/scripts/codex-git-hooks/pre-commit +++ b/scripts/codex-git-hooks/pre-commit @@ -36,6 +36,11 @@ scan_added_lines() { return 0 fi + if ! command -v rg >/dev/null 2>&1; then + printf '[ECC pre-commit] ripgrep (rg) is required for secret scanning; install it and ensure it is on PATH when Git runs the hook.\n' >&2 + exit 1 + fi + if hits="$(printf '%s\n' "$added_lines" | rg -n --pcre2 "$regex" 2>/dev/null)"; then printf '\n[ECC pre-commit] Potential secret detected (%s) in %s\n' "$name" "$file" >&2 printf '%s\n' "$hits" | head -n 3 >&2 diff --git a/tests/scripts/codex-hooks.test.js b/tests/scripts/codex-hooks.test.js index 7f9df083b..4af11550f 100644 --- a/tests/scripts/codex-hooks.test.js +++ b/tests/scripts/codex-hooks.test.js @@ -11,6 +11,7 @@ const TOML = require('@iarna/toml'); const repoRoot = path.join(__dirname, '..', '..'); const installScript = path.join(repoRoot, 'scripts', 'codex', 'install-global-git-hooks.sh'); +const preCommitHook = path.join(repoRoot, 'scripts', 'codex-git-hooks', 'pre-commit'); const prePushHook = path.join(repoRoot, 'scripts', 'codex-git-hooks', 'pre-push'); const pluginCacheCheckScript = path.join(repoRoot, 'scripts', 'codex', 'check-plugin-cache.js'); const mergeCodexConfigScript = path.join(repoRoot, 'scripts', 'codex', 'merge-codex-config.js'); @@ -164,6 +165,65 @@ if ( passed++; else failed++; +function runHermeticPreCommit({ content = 'safe text\n', filename = 'sample.txt', scanner = false, env = {} } = {}) { + const tempDir = createTempDir('codex-pre-commit-'); + try { + const bashEnv = path.join(tempDir, 'bash-env'); + const binDir = path.join(tempDir, 'bin'); + fs.mkdirSync(binDir); + const hookEnv = makeHermeticCodexEnv(tempDir, path.join(tempDir, '.codex'), { + PATH: process.env.PATH, + GIT_CONFIG_NOSYSTEM: '1', + MSYS_NO_PATHCONV: '1', + ECC_SKIP_GIT_HOOKS: '0', + ECC_SKIP_PRECOMMIT: '0', + ...env, + }); + const initialized = spawnSync('git', ['init', '--quiet'], { cwd: tempDir, env: hookEnv }); + assert.strictEqual(initialized.status, 0, initialized.stderr?.toString()); + if (content !== null) { + fs.writeFileSync(path.join(tempDir, filename), content); + const staged = spawnSync('git', ['add', '--', filename], { cwd: tempDir, env: hookEnv }); + assert.strictEqual(staged.status, 0, staged.stderr?.toString()); + } + // Keep real Git/awk available, while hiding every installed ripgrep binary. + fs.writeFileSync(bashEnv, ` +git_path="$(command -v git)" +awk_path="$(command -v awk)" +git() { "$git_path" "$@"; } +awk() { "$awk_path" "$@"; } +unset -f rg +PATH="${toBashPath(binDir)}" +${scanner ? 'rg() { return 1; }' : ''} +`); + return runBash(preCommitHook, { + env: { ...hookEnv, BASH_ENV: toBashPath(bashEnv) }, + cwd: tempDir, + preservePath: false, + }); + } finally { + cleanup(tempDir); + } +} + +for (const [name, options, status, diagnostic] of [ + ['blocks safe staged text when ripgrep is missing', {}, 1, /ripgrep.*required/i], + ['blocks a staged token when ripgrep is missing', { content: `token = "ghp_${'a'.repeat(36)}"\n` }, 1, /ripgrep.*required/i], + ['allows a successful scan with no findings', { scanner: true }, 0, null], + ['allows an empty index without ripgrep', { content: null }, 0, null], + ['allows excluded lockfiles without ripgrep', { filename: 'yarn.lock' }, 0, null], + ['preserves the pre-commit bypass warning', { env: { ECC_SKIP_PRECOMMIT: '1' } }, 0, /hook bypassed via env/], + ['preserves the global bypass warning', { env: { ECC_SKIP_GIT_HOOKS: '1' } }, 0, /hook bypassed via env/], +]) { + if (test(`pre-commit ${name}`, () => { + const result = runHermeticPreCommit(options); + assert.strictEqual(result.status, status, `${result.stdout}\n${result.stderr}`); + if (diagnostic) assert.match(result.stderr, diagnostic); + else assert.strictEqual(result.stderr, ''); + })) passed++; + else failed++; +} + function runHermeticPrePush({ failScript = null, includeCorepack = true, From 71bf78796fa1412209eca6032db51adca32e316c Mon Sep 17 00:00:00 2001 From: Tivonsico <173276029+Tivonsico@users.noreply.github.com> Date: Sun, 11 Oct 2026 00:15:12 +0800 Subject: [PATCH 24/26] fix: keep session summaries within the lifecycle deadline --- scripts/hooks/lifecycle-hook-bootstrap.js | 6 +- scripts/hooks/run-with-flags.js | 20 ++++- scripts/hooks/session-end.js | 10 ++- scripts/lib/llm-summary.js | 20 ++++- tests/hooks/session-end.test.js | 103 +++++++++++++++++++++- tests/lib/llm-summary.test.js | 52 +++++++++++ 6 files changed, 199 insertions(+), 12 deletions(-) diff --git a/scripts/hooks/lifecycle-hook-bootstrap.js b/scripts/hooks/lifecycle-hook-bootstrap.js index d0fab3c0a..629e031c9 100644 --- a/scripts/hooks/lifecycle-hook-bootstrap.js +++ b/scripts/hooks/lifecycle-hook-bootstrap.js @@ -80,6 +80,7 @@ async function main() { writeStderr(`[Hook] lifecycle stdin exceeded ${maxStdin} bytes; forwarded a bounded prefix`); } + const timeout = resolveTimeout(timeoutValue); const result = spawnSync( process.execPath, [runner, hookId, relScriptPath, profilesCsv || 'minimal,standard,strict'], @@ -92,10 +93,11 @@ async function main() { ECC_PLUGIN_ROOT: resolvedRoot, ECC_HOOK_INPUT_MAX_BYTES: String(maxStdin), ECC_HOOK_INPUT_TRUNCATED_UPSTREAM: truncated ? '1' : '0', - ECC_HOOK_CONTEXT_JSON: JSON.stringify(hookContext) + ECC_HOOK_CONTEXT_JSON: JSON.stringify(hookContext), + ECC_HOOK_DEADLINE_MS: String(Date.now() + timeout) }, cwd: process.cwd(), - timeout: resolveTimeout(timeoutValue), + timeout, maxBuffer: 16 * 1024 * 1024, windowsHide: true } diff --git a/scripts/hooks/run-with-flags.js b/scripts/hooks/run-with-flags.js index 19f0d1b9a..00c666308 100755 --- a/scripts/hooks/run-with-flags.js +++ b/scripts/hooks/run-with-flags.js @@ -299,6 +299,21 @@ async function main() { } // Legacy path: spawn a child Node process for hooks without run() export + // Direct runner invocations also need a deadline. Preserve an earlier + // bootstrap deadline instead of granting another full 30 seconds. + const inheritedDeadline = Number(process.env.ECC_HOOK_DEADLINE_MS); + const deadline = Math.min( + Date.now() + 30000, + Number.isSafeInteger(inheritedDeadline) && inheritedDeadline > 0 + ? inheritedDeadline + : Infinity + ); + const remaining = deadline - Date.now(); + if (remaining <= 0) { + writeStderr(`[Hook] lifecycle time budget exhausted for ${hookId}; skipping legacy hook`); + exitWithStdout('', 0); + return; + } const result = spawnSync(process.execPath, [scriptPath], { input: raw, encoding: 'utf8', @@ -308,10 +323,11 @@ async function main() { ECC_PLUGIN_ROOT: pluginRoot, ECC_HOOK_ID: hookId, ECC_HOOK_INPUT_TRUNCATED: truncated ? '1' : '0', - ECC_HOOK_INPUT_MAX_BYTES: String(MAX_STDIN) + ECC_HOOK_INPUT_MAX_BYTES: String(MAX_STDIN), + ECC_HOOK_DEADLINE_MS: String(deadline) }, cwd: process.cwd(), - timeout: 30000 + timeout: remaining }); const legacyStdout = sanitizeEcho(resolveLegacySpawnStdout(result)); diff --git a/scripts/hooks/session-end.js b/scripts/hooks/session-end.js index 5c31a8e0b..c7431eea7 100644 --- a/scripts/hooks/session-end.js +++ b/scripts/hooks/session-end.js @@ -257,10 +257,16 @@ async function main() { const isPeriodicTurn = summary.totalMessages > 0 && summary.totalMessages % safeInterval === 0; if (isContextLow || isPeriodicTurn) { log(`[SessionEnd] LLM summary triggered (context: ${contextPct ?? 'unknown'}%, messages: ${summary.totalMessages})`); - llmSummary = generateSessionSummary(transcriptPath); + let skipped = false; + llmSummary = generateSessionSummary(transcriptPath, { + onSkip: reason => { + skipped = true; + log(`[SessionEnd] LLM summary skipped (${reason}); falling back to mechanical extraction`); + } + }); if (llmSummary) { log('[SessionEnd] LLM summary generated successfully'); - } else { + } else if (!skipped) { log('[SessionEnd] LLM summary failed; falling back to mechanical extraction'); } } diff --git a/scripts/lib/llm-summary.js b/scripts/lib/llm-summary.js index b53fabd89..fb9394173 100644 --- a/scripts/lib/llm-summary.js +++ b/scripts/lib/llm-summary.js @@ -18,6 +18,8 @@ const fs = require('fs'); const MAX_TRANSCRIPT_CHARS = 7000; const MAX_TURNS = 25; const LLM_TIMEOUT_MS = 90000; +// Leave the Stop hook time to persist its mechanical fallback after a timeout. +const HOOK_FINISH_RESERVE_MS = 1000; function getLLMModel() { return process.env.ECC_LLM_SUMMARY_MODEL || 'haiku'; @@ -109,12 +111,26 @@ function getContextRemainingPct(transcriptPath) { * Generate a session summary using `claude -p`. * Returns the summary string, or null on failure or when recursion guard is active. */ -function generateSessionSummary(transcriptPath) { +function generateSessionSummary(transcriptPath, { onSkip = () => {} } = {}) { if (process.env.ECC_SKIP_LLM_SUMMARY) return null; const conversation = extractConversationText(transcriptPath); if (!conversation) return null; + let timeout = LLM_TIMEOUT_MS; + if (process.env.ECC_HOOK_DEADLINE_MS !== undefined) { + const deadline = Number(process.env.ECC_HOOK_DEADLINE_MS); + if (!Number.isSafeInteger(deadline) || deadline <= 0) { + onSkip('invalid lifecycle deadline'); + return null; + } + timeout = Math.min(timeout, deadline - Date.now() - HOOK_FINISH_RESERVE_MS); + if (timeout <= 0) { + onSkip('insufficient lifecycle time budget'); + return null; + } + } + const prompt = [ 'Below is a conversation log from a Claude Code coding session.', 'Create a summary to help the next session quickly understand the context.', @@ -159,7 +175,7 @@ function generateSessionSummary(transcriptPath) { ECC_SKIP_LLM_SUMMARY: '1', ECC_LLM_SUMMARY_SUBPROCESS: '1' }, - timeout: LLM_TIMEOUT_MS, + timeout, shell: process.platform === 'win32' }); diff --git a/tests/hooks/session-end.test.js b/tests/hooks/session-end.test.js index 05e74eeac..f6010f97e 100644 --- a/tests/hooks/session-end.test.js +++ b/tests/hooks/session-end.test.js @@ -37,11 +37,19 @@ function countOccurrences(haystack, needle) { return n; } +function isolatedHomeEnv(home) { + return { + ...process.env, HOME: home, USERPROFILE: home, + ECC_AGENT_DATA_HOME: path.join(home, '.claude'), + CLAUDE_CONFIG_DIR: path.join(home, '.claude') + }; +} + function runHook(home, transcript, env = {}) { - return spawnSync('node', [script], { + return spawnSync(process.execPath, [script], { encoding: 'utf8', input: transcript ? JSON.stringify({ transcript_path: transcript }) : '', - env: { ...process.env, HOME: home, USERPROFILE: home, CLAUDE_SESSION_ID: '', ...env }, + env: { ...isolatedHomeEnv(home), CLAUDE_SESSION_ID: '', ...env }, timeout: 10000, }); } @@ -96,7 +104,7 @@ function runTests() { const res = spawnSync('node', [script], { encoding: 'utf8', input: JSON.stringify({ transcript_path: transcript }), - env: { ...process.env, HOME: home, USERPROFILE: home, CLAUDE_SESSION_ID: '' }, + env: { ...isolatedHomeEnv(home), CLAUDE_SESSION_ID: '' }, timeout: 10000, }); assert.strictEqual(res.status || 0, 0, `hook exited ${res.status}: ${res.stderr}`); @@ -225,7 +233,7 @@ function runTests() { const res = spawnSync('node', [script], { encoding: 'utf8', input: '{not-json', - env: { ...process.env, HOME: home, USERPROFILE: home, CLAUDE_SESSION_ID: 'fallback-session-12345678', CLAUDE_TRANSCRIPT_PATH: '' }, + env: { ...isolatedHomeEnv(home), CLAUDE_SESSION_ID: 'fallback-session-12345678', CLAUDE_TRANSCRIPT_PATH: '' }, timeout: 10000, }); assert.strictEqual(res.status || 0, 0, `hook exited ${res.status}: ${res.stderr}`); @@ -237,6 +245,93 @@ function runTests() { } }) ? passed++ : failed++); + (test('persists mechanical resume state when the LLM budget is exhausted', () => { + const home = fs.mkdtempSync(path.join(os.tmpdir(), 'ecc-session-budget-')); + try { + const uuid = '12345678-1234-4234-8234-123456789abc'; + const transcript = path.join(home, `${uuid}.jsonl`); + fs.writeFileSync(transcript, JSON.stringify({ type: 'user', content: 'Keep this task resumable' }) + '\n'); + const bin = path.join(home, 'empty-bin'); + fs.mkdirSync(bin); + const res = runHook(home, transcript, { + ECC_LLM_SUMMARY_INTERVAL: '1', ECC_HOOK_DEADLINE_MS: '1', + ECC_SKIP_LLM_SUMMARY: '', ECC_LLM_SUMMARY_SUBPROCESS: '', PATH: bin + }); + assert.strictEqual(res.status, 0, res.stderr); + assert.match(res.stderr, /LLM summary skipped.*insufficient lifecycle time budget/); + assert.doesNotMatch(res.stderr, /LLM summary failed/); + assert.match(fs.readFileSync(sessionFileFor(home, uuid), 'utf8'), /Keep this task resumable/); + } finally { + fs.rmSync(home, { recursive: true, force: true }); + } + }) ? passed++ : failed++); + + for (const inherited of ['', String(Date.now() + 300000), '1']) { + (test(`direct legacy runner bounds the deadline ${inherited || '(unset)'}`, () => { + const home = fs.mkdtempSync(path.join(os.tmpdir(), 'ecc-runner-budget-')); + try { + const relative = 'legacy.js'; + fs.writeFileSync(path.join(home, relative), "process.stdout.write(JSON.stringify({deadline:Number(process.env.ECC_HOOK_DEADLINE_MS),now:Date.now()}));\n"); + const runner = path.resolve(__dirname, '../../scripts/hooks/run-with-flags.js'); + const res = spawnSync(process.execPath, [runner, 'session:stop:session-end', relative, 'standard'], { + input: '{}', encoding: 'utf8', timeout: 5000, + env: { + ...isolatedHomeEnv(home), CLAUDE_PLUGIN_ROOT: home, + ECC_HOOK_PROFILE: 'standard', ECC_DISABLED_HOOKS: '', ECC_HOOK_DRY_RUN: '', + ECC_HOOK_DEADLINE_MS: inherited + } + }); + assert.strictEqual(res.status, 0, res.stderr); + if (inherited === '1') { + assert.strictEqual(res.stdout, ''); + assert.match(res.stderr, /time budget exhausted/); + } else { + const result = JSON.parse(res.stdout); + assert.ok(result.deadline > result.now); + assert.ok(result.deadline - result.now <= 30000, 'Inherited far-future deadlines must not extend the runner budget'); + } + } finally { + fs.rmSync(home, { recursive: true, force: true }); + } + }) ? passed++ : failed++); + } + + // A local stand-in exercises the actual bootstrap -> runner -> Stop hook -> + // child timeout chain without invoking Claude or using authentication. + if (process.platform !== 'win32') { + (test('times out a slow summarizer before lifecycle expiry and saves fallback', () => { + const home = fs.mkdtempSync(path.join(os.tmpdir(), 'ecc-session-budget-')); + try { + const uuid = '12345678-1234-4234-8234-123456789abc'; + const transcript = path.join(home, `${uuid}.jsonl`); + const bin = path.join(home, 'bin'); + fs.mkdirSync(bin); + fs.writeFileSync(transcript, JSON.stringify({ type: 'user', content: 'Persist after a slow summary' }) + '\n'); + fs.writeFileSync(path.join(bin, 'claude'), `#!${process.execPath}\nrequire('fs').writeFileSync(${JSON.stringify(path.join(home, 'started'))}, 'started');\nsetTimeout(() => console.log('late summary'), 5000);\n`, { mode: 0o755 }); + const root = path.resolve(__dirname, '../..'); + const bootstrap = path.join(root, 'scripts/hooks/lifecycle-hook-bootstrap.js'); + const res = spawnSync(process.execPath, [bootstrap, 'session:stop:session-end', 'scripts/hooks/session-end.js', 'minimal,standard,strict', '2500'], { + encoding: 'utf8', input: JSON.stringify({ transcript_path: transcript }), + env: { + ...isolatedHomeEnv(home), PATH: bin, + CLAUDE_PLUGIN_ROOT: root, ECC_HOOK_PROFILE: 'standard', + ECC_LLM_SUMMARY_INTERVAL: '1', ECC_SKIP_LLM_SUMMARY: '', + ECC_LLM_SUMMARY_SUBPROCESS: '', ECC_HOOK_DEADLINE_MS: '1', + ECC_DISABLED_HOOKS: '', ECC_HOOK_DRY_RUN: '' + }, + timeout: 7000 + }); + assert.strictEqual(res.status, 0, res.stderr); + assert.ok(fs.existsSync(path.join(home, 'started')), 'Fresh bootstrap must replace an inherited expired deadline'); + assert.match(res.stderr, /LLM summary failed; falling back/); + assert.doesNotMatch(res.stderr, /lifecycle runner failed/); + assert.match(fs.readFileSync(sessionFileFor(home, uuid), 'utf8'), /Persist after a slow summary/); + } finally { + fs.rmSync(home, { recursive: true, force: true }); + } + }) ? passed++ : failed++); + } + console.log(`\nResults: Passed: ${passed}, Failed: ${failed}`); process.exit(failed > 0 ? 1 : 0); } diff --git a/tests/lib/llm-summary.test.js b/tests/lib/llm-summary.test.js index 1705fe499..db1b8f65a 100644 --- a/tests/lib/llm-summary.test.js +++ b/tests/lib/llm-summary.test.js @@ -201,6 +201,58 @@ test('marks the spawned summarizer so its Stop hook cannot create resume state', }); // --- Results --- +// Execute the real helper with a controlled clock and child-process boundary. +// These cases never invoke Claude or use the developer's authentication. +function summaryProbe(deadline, now = 100000, spawnResult = { status: 0, stdout: 'summary' }) { + const vm = require('vm'); + const source = fs.readFileSync(path.join(__dirname, '../../scripts/lib/llm-summary.js'), 'utf8'); + const calls = []; + const env = deadline === undefined ? {} : { ECC_HOOK_DEADLINE_MS: String(deadline) }; + const sandbox = { + module: { exports: {} }, + process: { env, platform: process.platform }, + Date: { now: () => now }, + require: name => name === 'child_process' + ? { spawnSync: (...args) => { calls.push(args); return spawnResult; } } + : require(name) + }; + vm.runInNewContext(source, sandbox, { filename: path.join(__dirname, '../../scripts/lib/llm-summary.js') }); + const skipped = []; + const result = sandbox.module.exports.generateSessionSummary( + writeTranscript([userEntry('Preserve mechanical resume state')]), + { onSkip: reason => skipped.push(reason) } + ); + return { calls, skipped, result }; +} + +test('limits summarizer to remaining hook budget minus persistence reserve', () => { + const probe = summaryProbe(105000); + assert.strictEqual(probe.result, 'summary'); + assert.strictEqual(probe.calls[0][2].timeout, 4000); + assert.strictEqual(probe.skipped.length, 0); +}); + +test('retains the standalone timeout and caps longer inherited budgets', () => { + assert.strictEqual(summaryProbe(undefined).calls[0][2].timeout, 90000); + assert.strictEqual(summaryProbe(300000).calls[0][2].timeout, 90000); +}); + +for (const deadline of [99999, 100000, 100999, 101000, '', -1, 'invalid', 'Infinity', '1.5', Number.MAX_SAFE_INTEGER + 1]) { + test(`skips summarizer with exhausted or invalid deadline ${deadline}`, () => { + const probe = summaryProbe(deadline); + assert.strictEqual(probe.result, null); + assert.strictEqual(probe.calls.length, 0, 'No child should start without a safe budget'); + assert.strictEqual(probe.skipped.length, 1, 'Skip must be distinguishable from Claude failure'); + }); +} + +test('keeps process failures distinct from deliberate budget skips', () => { + const probe = summaryProbe(105000, 100000, { status: 1, stdout: '' }); + assert.strictEqual(probe.result, null); + assert.strictEqual(probe.calls.length, 1); + assert.strictEqual(probe.skipped.length, 0); +}); + console.log('\n=== Test Results ==='); console.log(`Passed: ${passed}`); console.log(`Failed: ${failed}`); From 1ad235fdfe1ec35d4fea4c6d2a70a6f4abe2a180 Mon Sep 17 00:00:00 2001 From: Tivonsico <173276029+Tivonsico@users.noreply.github.com> Date: Sun, 11 Oct 2026 00:36:27 +0800 Subject: [PATCH 25/26] fix: report explicitly disabled summaries as skipped --- scripts/lib/llm-summary.js | 5 ++++- tests/hooks/session-end.test.js | 21 +++++++++++++++++++++ tests/lib/llm-summary.test.js | 15 +++++++++++++++ 3 files changed, 40 insertions(+), 1 deletion(-) diff --git a/scripts/lib/llm-summary.js b/scripts/lib/llm-summary.js index fb9394173..2473b3797 100644 --- a/scripts/lib/llm-summary.js +++ b/scripts/lib/llm-summary.js @@ -112,7 +112,10 @@ function getContextRemainingPct(transcriptPath) { * Returns the summary string, or null on failure or when recursion guard is active. */ function generateSessionSummary(transcriptPath, { onSkip = () => {} } = {}) { - if (process.env.ECC_SKIP_LLM_SUMMARY) return null; + if (process.env.ECC_SKIP_LLM_SUMMARY) { + onSkip('ECC_SKIP_LLM_SUMMARY'); + return null; + } const conversation = extractConversationText(transcriptPath); if (!conversation) return null; diff --git a/tests/hooks/session-end.test.js b/tests/hooks/session-end.test.js index f6010f97e..5b745705c 100644 --- a/tests/hooks/session-end.test.js +++ b/tests/hooks/session-end.test.js @@ -266,6 +266,27 @@ function runTests() { } }) ? passed++ : failed++); + (test('logs an explicit LLM disable as skipped and persists mechanical resume state', () => { + const home = fs.mkdtempSync(path.join(os.tmpdir(), 'ecc-session-skip-')); + try { + const uuid = '12345678-1234-4234-8234-123456789abc'; + const transcript = path.join(home, `${uuid}.jsonl`); + const bin = path.join(home, 'empty-bin'); + fs.mkdirSync(bin); + fs.writeFileSync(transcript, JSON.stringify({ type: 'user', content: 'Save context with LLM disabled' }) + '\n'); + const res = runHook(home, transcript, { + ECC_LLM_SUMMARY_INTERVAL: '1', ECC_SKIP_LLM_SUMMARY: '1', + ECC_LLM_SUMMARY_SUBPROCESS: '', PATH: bin + }); + assert.strictEqual(res.status, 0, res.stderr); + assert.match(res.stderr, /LLM summary skipped.*ECC_SKIP_LLM_SUMMARY/); + assert.doesNotMatch(res.stderr, /LLM summary failed/); + assert.match(fs.readFileSync(sessionFileFor(home, uuid), 'utf8'), /Save context with LLM disabled/); + } finally { + fs.rmSync(home, { recursive: true, force: true }); + } + }) ? passed++ : failed++); + for (const inherited of ['', String(Date.now() + 300000), '1']) { (test(`direct legacy runner bounds the deadline ${inherited || '(unset)'}`, () => { const home = fs.mkdtempSync(path.join(os.tmpdir(), 'ecc-runner-budget-')); diff --git a/tests/lib/llm-summary.test.js b/tests/lib/llm-summary.test.js index db1b8f65a..f836d20c4 100644 --- a/tests/lib/llm-summary.test.js +++ b/tests/lib/llm-summary.test.js @@ -185,6 +185,21 @@ test('returns null when ECC_SKIP_LLM_SUMMARY is set', () => { else delete process.env.ECC_SKIP_LLM_SUMMARY; }); +test('reports the explicit recursion guard as a deliberate skip', () => { + const orig = process.env.ECC_SKIP_LLM_SUMMARY; + process.env.ECC_SKIP_LLM_SUMMARY = '1'; + try { + const skipped = []; + assert.strictEqual(generateSessionSummary('/nonexistent.jsonl', { + onSkip: reason => skipped.push(reason) + }), null); + assert.deepStrictEqual(skipped, ['ECC_SKIP_LLM_SUMMARY']); + } finally { + if (orig !== undefined) process.env.ECC_SKIP_LLM_SUMMARY = orig; + else delete process.env.ECC_SKIP_LLM_SUMMARY; + } +}); + test('returns null for missing transcript (no conversation to summarize)', () => { const orig = process.env.ECC_SKIP_LLM_SUMMARY; delete process.env.ECC_SKIP_LLM_SUMMARY; From bb15240c9c015063517dd4b8e07ed4060bf2bd13 Mon Sep 17 00:00:00 2001 From: haelyra <49814733+haelyra@users.noreply.github.com> Date: Sat, 10 Oct 2026 17:42:43 -0400 Subject: [PATCH 26/26] fix(install): preserve native Claude names through repair receipts --- scripts/lib/install/apply.js | 13 +--- scripts/lib/install/content-transform.js | 4 + scripts/lib/install/plan.js | 12 ++- tests/scripts/manual-claude-namespace.test.js | 77 +++++++++++++++++++ 4 files changed, 95 insertions(+), 11 deletions(-) create mode 100644 tests/scripts/manual-claude-namespace.test.js diff --git a/scripts/lib/install/apply.js b/scripts/lib/install/apply.js index 2fa320cfe..ce06bacb1 100644 --- a/scripts/lib/install/apply.js +++ b/scripts/lib/install/apply.js @@ -52,10 +52,6 @@ const { } = require('./excluded-paths-reconciliation'); const { buildInstallIndex, rewriteRelativeLinks } = require('./link-rewrite'); const { transformInstallContent } = require('./content-transform'); -const { stripPluginNamespace } = require('./plugin-namespace'); - -// Manual Claude targets expose bare component names. -const MANUAL_CLAUDE_TARGETS = new Set(['claude', 'claude-project']); function isMarkdownPath(filePath) { return /\.(md|mdx|markdown)$/i.test(String(filePath || '')); @@ -862,20 +858,17 @@ function applyInstallPlanLocked(plan, dependencies = {}, settingsLockHeld = fals && operation.sourceRelativePath && isMarkdownPath(operation.destinationPath) ); - const stripNamespace = MANUAL_CLAUDE_TARGETS.has(plan.adapter?.target) - && isMarkdownPath(operation.destinationPath); - if (operation.kind === 'copy-file' && (operation.contentTransform || needsLinkRewrite || stripNamespace)) { + if (operation.kind === 'copy-file' && (operation.contentTransform || needsLinkRewrite)) { const transformed = transformInstallContent( operation, fs.readFileSync(operation.sourcePath, 'utf8') ); - const namespaced = stripNamespace ? stripPluginNamespace(transformed) : transformed; const installedContent = needsLinkRewrite - ? rewriteRelativeLinks(namespaced, { + ? rewriteRelativeLinks(transformed, { sourceRel: operation.sourceRelativePath, index: linkIndex, }) - : namespaced; + : transformed; const writeOptions = getOpenCodeActivationWriteOptions(operation, activationSnapshot); if (writeOptions.expectedContent) { writeFileNoFollow(operation.destinationPath, installedContent, { diff --git a/scripts/lib/install/content-transform.js b/scripts/lib/install/content-transform.js index d27e54c02..a2c48a883 100644 --- a/scripts/lib/install/content-transform.js +++ b/scripts/lib/install/content-transform.js @@ -1,5 +1,6 @@ 'use strict'; +const { stripPluginNamespace } = require('./plugin-namespace'); const { adaptAntigravityAgent } = require('./antigravity-agent'); const { adaptCopilotAgent } = require('./copilot-agent'); const { disableOpenCodeHookPluginRegistration, getDisabledOpenCodePluginContent } = require('./hook-consent'); @@ -16,6 +17,9 @@ function transformInstallContent(operation, content) { if (!operation.contentTransform) { return content; } + if (operation.contentTransform === 'claude-manual-plugin-namespace') { + return stripPluginNamespace(content); + } if (operation.contentTransform === 'antigravity-agent-frontmatter') { return adaptAntigravityAgent(content, operation.sourceRelativePath); } diff --git a/scripts/lib/install/plan.js b/scripts/lib/install/plan.js index ca3ba47b6..d8a8e93ff 100644 --- a/scripts/lib/install/plan.js +++ b/scripts/lib/install/plan.js @@ -287,7 +287,17 @@ function createManifestInstallPlan(options = {}) { const adapter = getInstallTargetAdapter(target); const materializedOperations = plan.operations.flatMap(operation => ( materializeScaffoldOperation(sourceRoot, operation) - )); + )).map(operation => { + // Persist this projection so install, doctor and repair compare the same + // native component names rather than restoring plugin-only references. + if (['claude', 'claude-project'].includes(target) + && operation.kind === 'copy-file' + && /\.(md|mdx|markdown)$/i.test(operation.destinationPath) + && !operation.contentTransform) { + return { ...operation, contentTransform: 'claude-manual-plugin-namespace' }; + } + return operation; + }); const ruleLanguages = Array.isArray(options.ruleLanguages) ? [...options.ruleLanguages] : []; const operations = dedupeCopyFileOperations( options.legacyMode && target === 'antigravity' diff --git a/tests/scripts/manual-claude-namespace.test.js b/tests/scripts/manual-claude-namespace.test.js new file mode 100644 index 000000000..5ec2101bd --- /dev/null +++ b/tests/scripts/manual-claude-namespace.test.js @@ -0,0 +1,77 @@ +'use strict'; + +const assert = require('assert'); +const fs = require('fs'); +const os = require('os'); +const path = require('path'); +const { spawnSync } = require('child_process'); + +const cli = path.resolve(__dirname, '../../scripts/ecc.js'); +let passed = 0; +let failed = 0; + +for (const target of ['claude', 'claude-project']) { + const root = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), 'ecc-native-names-'))); + const home = path.join(root, 'home'); + const project = path.join(root, 'project'); + fs.mkdirSync(home); + fs.mkdirSync(project); + const env = { + ...process.env, + HOME: home, + USERPROFILE: home, + CLAUDE_CONFIG_DIR: path.join(home, '.claude'), + ECC_AGENT_DATA_HOME: path.join(home, '.claude'), + XDG_CONFIG_HOME: path.join(home, '.config'), + OPENCODE_CONFIG_DIR: path.join(home, '.config/opencode'), + CODEX_HOME: path.join(home, '.codex'), + ECC_DRY_RUN: '0', + }; + const installRoot = path.join(target === 'claude' ? home : project, '.claude'); + const rule = path.join(installRoot, 'rules/ecc/common/agents.md'); + const run = args => { + const result = spawnSync(process.execPath, [cli, ...args], { + cwd: project, env, encoding: 'utf8', timeout: 45000, + }); + assert.ifError(result.error); + assert.strictEqual(result.status, 0, result.stderr || result.stdout); + return result; + }; + const assertNativeNames = () => { + assert.match(fs.readFileSync(rule, 'utf8'), /Agent\(subagent_type: "planner"/); + assert.doesNotMatch(fs.readFileSync(rule, 'utf8'), /Agent\(subagent_type: "ecc:planner"/); + assert.ok(fs.existsSync(path.join(installRoot, 'agents/planner.md'))); + assert.ok(fs.existsSync(path.join(installRoot, 'skills/tdd-workflow/SKILL.md'))); + }; + try { + const installArgs = ['install', '--target', target, '--modules', 'rules-core', + '--with', 'agent:planner', '--with', 'skill:tdd-workflow', '--no-hooks']; + run(installArgs); + assertNativeNames(); + const report = JSON.parse(run(['doctor', '--target', target, '--json']).stdout); + assert.strictEqual(report.summary.errorCount, 0); + assert.strictEqual(report.summary.warningCount, 0); + fs.appendFileSync(rule, '\nlocal drift\n'); + run(['repair', '--target', target, '--json']); + assertNativeNames(); + assert.doesNotMatch(fs.readFileSync(rule, 'utf8'), /local drift/); + run(installArgs); + assertNativeNames(); + const userFile = path.join(installRoot, 'user-note.txt'); + fs.writeFileSync(userFile, 'keep this user file\n'); + run(['uninstall', '--target', target, '--json']); + assert.ok(!fs.existsSync(rule)); + assert.strictEqual(fs.readFileSync(userFile, 'utf8'), 'keep this user file\n'); + passed++; + console.log(` PASS ${target} install, doctor, repair, reinstall and uninstall preserve native names`); + } catch (error) { + failed++; + console.error(` FAIL ${target}: ${error.stack}`); + } finally { + fs.rmSync(root, { recursive: true, force: true }); + } +} + +console.log(`Passed: ${passed}`); +console.log(`Failed: ${failed}`); +process.exitCode = failed ? 1 : 0;