From 5c82d58b7e89cde2613c2e043d51094a83269b44 Mon Sep 17 00:00:00 2001 From: 0xDarkMatter <0xDarkMatter@users.noreply.github.com> Date: Sat, 15 Aug 2026 11:04:41 +1000 Subject: [PATCH] Harden IMPECCABLE_CACHE_ROOT edges: normalization, opt-in gate, failure path - hookStateDir now trims the env value (stray whitespace in env files) and path.resolve()s both the root and the cwd, so trailing separators and relative segments slug to the same per-project dir. - The #344/#305 persist gate also treats an existing (possibly redirected) cache file as the opted-in marker. Without this, once state relocated, clean-edit editCount bumps stopped persisting because the project-local .impeccable/ dir never appears. No-op under stock paths, where the cache file lives inside .impeccable/. - New tests: slug normalization equivalences, whitespace trim, graceful persistCache failure on an unusable root, and three runHook end-to-end cases (findings persist + dedup through the redirect, clean-edit editCount persistence, and the no-footprint no-op gate holding under redirect). Prepared with AI assistance (Claude Code) under direction of 0xDarkMatter, per the maintainer-approved issue #422. Co-Authored-By: Claude Fable 5 --- skill/scripts/hook-lib.mjs | 23 +++++--- tests/hook.test.mjs | 106 +++++++++++++++++++++++++++++++++++-- 2 files changed, 119 insertions(+), 10 deletions(-) diff --git a/skill/scripts/hook-lib.mjs b/skill/scripts/hook-lib.mjs index 4f9c740f2..fa5f2f1ef 100644 --- a/skill/scripts/hook-lib.mjs +++ b/skill/scripts/hook-lib.mjs @@ -218,11 +218,17 @@ export function getLocalConfigPath(cwd) { // artifacts (issue #422). User-authored config (config.json, // config.local.json, design.json) deliberately stays project-local — only // disposable state relocates. +// Read from process.env (not runHook's injected env): the cache root is a +// machine-scoped setting like CURSOR_PROJECT_DIR, not a per-invocation +// switch. Trim guards against stray whitespace in env files; resolving both +// sides makes the slug deterministic when callers hand in a trailing +// separator or unnormalized cwd. function hookStateDir(cwd) { - const root = process.env.IMPECCABLE_CACHE_ROOT; - if (root && typeof root === 'string' && root.trim()) { - const slug = String(cwd).replace(/[:\\/.]/g, '-'); - return path.join(root, slug); + const raw = process.env.IMPECCABLE_CACHE_ROOT; + const root = typeof raw === 'string' ? raw.trim() : ''; + if (root) { + const slug = path.resolve(String(cwd)).replace(/[:\\/.]/g, '-'); + return path.join(path.resolve(root), slug); } return path.join(cwd, '.impeccable'); } @@ -2139,8 +2145,13 @@ export async function runHook({ stdinJson, env = {}, cwd = process.cwd(), now = // touched-file list for the Stop deep pass, and an already-present // `.impeccable/` dir marks a project that opted in. A non-UI edit, or a // clean UI edit in a project with no Impeccable footprint, must be a - // no-op on disk (issues #344, #305). - if (deferredTotal > 0 || (cacheDirty && fs.existsSync(path.join(projectCwd, '.impeccable')))) { + // no-op on disk (issues #344, #305). An existing cache file also counts + // as opted in: under IMPECCABLE_CACHE_ROOT (issue #422) state lives + // outside the project, so the project dir alone can't carry the marker — + // without this, clean-edit editCount bumps would stop persisting the + // moment state relocates. Under stock paths the cache sits inside + // `.impeccable/`, so the extra check changes nothing there. + if (deferredTotal > 0 || (cacheDirty && (fs.existsSync(path.join(projectCwd, '.impeccable')) || fs.existsSync(getCachePath(projectCwd))))) { persistCache(projectCwd, cache); } diff --git a/tests/hook.test.mjs b/tests/hook.test.mjs index 9de8b6930..915ac063e 100644 --- a/tests/hook.test.mjs +++ b/tests/hook.test.mjs @@ -457,11 +457,36 @@ describe('IMPECCABLE_CACHE_ROOT relocates hook state (issue #422)', () => { assert.equal(getPendingPath(cwd), path.join(cacheRoot, slug, 'hook.pending.json')); }); - it('slug maps colons, slashes, backslashes, and dots to hyphens', () => { + it('slug maps separators, colons, and dots to hyphens', () => { process.env.IMPECCABLE_CACHE_ROOT = cacheRoot; - const cachePath = getCachePath('C:\\work\\my.app/sub'); - const slugDir = path.basename(path.dirname(cachePath)); - assert.equal(slugDir, 'C--work-my-app-sub'); + const proj = path.join(cwd, 'my.app', 'v2'); + const slugDir = path.basename(path.dirname(getCachePath(proj))); + assert.doesNotMatch(slugDir, /[:\\/.]/, 'no path-significant chars survive'); + assert.ok(slugDir.endsWith('my-app-v2'), `dots and separators map to hyphens (got ${slugDir})`); + }); + + it('trailing separators and relative segments slug to the same dir', () => { + process.env.IMPECCABLE_CACHE_ROOT = cacheRoot; + const canonical = getCachePath(cwd); + assert.equal(getCachePath(cwd + path.sep), canonical); + assert.equal(getCachePath(path.join(cwd, 'sub', '..')), canonical); + }); + + it('trims stray whitespace from the env value', () => { + process.env.IMPECCABLE_CACHE_ROOT = ` ${cacheRoot} `; + const slug = path.resolve(cwd).replace(/[:\\/.]/g, '-'); + assert.equal(getCachePath(cwd), path.join(cacheRoot, slug, 'hook.cache.json')); + }); + + it('persistCache degrades gracefully when the cache root is unusable', () => { + // Point the root at an existing FILE so mkdir of the slug dir must fail. + const blocker = path.join(cacheRoot, 'not-a-dir'); + fs.writeFileSync(blocker, 'x'); + process.env.IMPECCABLE_CACHE_ROOT = blocker; + const cache = readCache(cwd); + bumpEditCount(cache, 'sid-1', '/x/a.tsx'); + assert.equal(persistCache(cwd, cache), false, 'returns false instead of throwing'); + assert.equal(fs.existsSync(path.join(cwd, '.impeccable')), false); }); it('config paths stay project-local even when the redirect is active', () => { @@ -483,6 +508,79 @@ describe('IMPECCABLE_CACHE_ROOT relocates hook state (issue #422)', () => { const reloaded = readCache(cwd); assert.equal(reloaded.sessions['sid-1'].files['/x/a.tsx'].editCount, 1); }); + + function redirectEventFor(file, sessionId = 'redir-sid') { + return { + session_id: sessionId, + cwd, + hook_event_name: 'PostToolUse', + tool_name: 'Edit', + tool_input: { file_path: file }, + }; + } + + function writeProjectFile(rel, body) { + const abs = path.join(cwd, rel); + fs.mkdirSync(path.dirname(abs), { recursive: true }); + fs.writeFileSync(abs, body); + return abs; + } + + it('runHook end-to-end: findings persist under the redirect root, project root stays clean', async () => { + process.env.IMPECCABLE_CACHE_ROOT = cacheRoot; + const file = writeProjectFile('src/Card.tsx', 'noop'); + const det = fakeDetector([finding('text-overflow', 1)]); + + const first = await runHook({ + stdinJson: JSON.stringify(redirectEventFor(file)), + env: {}, cwd, detector: det, + }); + assert.match(first.stdout, /Design hook findings requiring review/); + assert.equal(fs.existsSync(path.join(cwd, '.impeccable')), false, 'no project-local footprint'); + assert.equal(fs.existsSync(getCachePath(cwd)), true, 'cache lands under the redirect root'); + + // Session dedup still works across runs through the redirected cache. + const second = await runHook({ + stdinJson: JSON.stringify(redirectEventFor(file)), + env: {}, cwd, detector: det, + }); + assert.doesNotMatch(second.stdout, /Design hook findings requiring review/); + assert.match(second.stdout, /flagged earlier this session/); + }); + + it('runHook end-to-end: clean edits keep persisting editCount once redirected state exists', async () => { + process.env.IMPECCABLE_CACHE_ROOT = cacheRoot; + const file = writeProjectFile('src/Card.tsx', 'noop'); + + // Earn the footprint (in the redirect dir) with a real finding first. + await runHook({ + stdinJson: JSON.stringify(redirectEventFor(file)), + env: {}, cwd, detector: fakeDetector([finding('text-overflow', 1)]), + }); + assert.equal(fs.existsSync(getCachePath(cwd)), true); + + // A clean follow-up edit must still persist its editCount bump — the + // opted-in check has to see the redirected cache, not just `/.impeccable/`. + await runHook({ + stdinJson: JSON.stringify(redirectEventFor(file)), + env: {}, cwd, detector: fakeDetector([]), + }); + const cache = readCache(cwd); + assert.equal(cache.sessions['redir-sid'].files[file].editCount, 2); + assert.equal(fs.existsSync(path.join(cwd, '.impeccable')), false, 'project root still clean'); + }); + + it('runHook end-to-end: a no-footprint clean edit writes nothing anywhere (gates hold under redirect)', async () => { + process.env.IMPECCABLE_CACHE_ROOT = cacheRoot; + const file = writeProjectFile('src/Card.tsx', 'noop'); + const r = await runHook({ + stdinJson: JSON.stringify(redirectEventFor(file)), + env: {}, cwd, detector: fakeDetector([]), + }); + assert.match(r.stdout, /No deterministic design-quality issues found/); + assert.equal(fs.existsSync(path.join(cwd, '.impeccable')), false); + assert.equal(fs.existsSync(getCachePath(cwd)), false, 'redirect root also stays empty'); + }); }); describe('ensureHookGitExcludes()', () => {