From cbd7870159100f98c7cbd8803fdcc878b93058db Mon Sep 17 00:00:00 2001 From: 0xDarkMatter <0xDarkMatter@users.noreply.github.com> Date: Sat, 15 Aug 2026 11:19:21 +1000 Subject: [PATCH] Address review: collision-resistant slugs, os.homedir() tilde expansion - The per-project state dir key is now the readable separator-mapped slug plus an 8-hex sha256 of the resolved project path. The readable part alone is lossy (/x/my.app and /x/my-app both mapped to -x-my-app and shared hook state); the digest keeps distinct projects' cache and pending state apart while the dir name stays human-scannable. - Tilde roots now expand via os.homedir() instead of HOME/USERPROFILE with a '.' fallback. When no home dir can be determined, expansion is rejected and state falls back to the project-local default rather than anchoring under the hook process's working directory. - Tests updated to the digest-suffixed slug via a mirrored slugFor() helper, plus two new cases: colliding readable slugs get distinct state dirs, and the tilde form resolves identically to the explicit homedir-joined form. 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 | 26 +++++++++++----- tests/hook.test.mjs | 64 ++++++++++++++++++++++---------------- 2 files changed, 55 insertions(+), 35 deletions(-) diff --git a/skill/scripts/hook-lib.mjs b/skill/scripts/hook-lib.mjs index 91d3de7d9..767fe65a6 100644 --- a/skill/scripts/hook-lib.mjs +++ b/skill/scripts/hook-lib.mjs @@ -43,6 +43,7 @@ * `cli/engine/detect-antipatterns.mjs` (running from source). */ +import crypto from 'node:crypto'; import fs from 'node:fs'; import os from 'node:os'; import path from 'node:path'; @@ -220,20 +221,29 @@ export function getLocalConfigPath(cwd) { // 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; `~/` expands to -// the home dir (settings/env files hand it to Node unexpanded — same -// treatment IMPECCABLE_HOOK_LOG gets in writeAuditLog); resolving both sides -// makes the slug deterministic when callers hand in a trailing separator or -// unnormalized cwd. +// switch. Trim guards against stray whitespace in env files; `~/` (or the +// Windows `~\` spelling) expands via os.homedir(), and when no home dir can +// be determined the expansion is rejected — state falls back to the +// project-local default rather than anchoring under the hook process's cwd. +// Resolving both sides makes the slug deterministic when callers hand in a +// trailing separator or unnormalized cwd. The slug is the readable +// separator-mapped path PLUS an 8-hex sha256 of the resolved path: the +// readable part alone is lossy (`/x/my.app` and `/x/my-app` would both map +// to `-x-my-app` and share state), so the digest disambiguates while keeping +// the dir name human-scannable. function hookStateDir(cwd) { const raw = process.env.IMPECCABLE_CACHE_ROOT; let root = typeof raw === 'string' ? raw.trim() : ''; if (root.startsWith('~/') || root.startsWith('~\\') || root === '~') { - root = path.join(process.env.HOME || process.env.USERPROFILE || '.', root.slice(2)); + let home = ''; + try { home = os.homedir() || ''; } catch { home = ''; } + root = home ? path.join(home, root.slice(2)) : ''; } if (root) { - const slug = path.resolve(String(cwd)).replace(/[:\\/.]/g, '-'); - return path.join(path.resolve(root), slug); + const resolved = path.resolve(String(cwd)); + const slug = resolved.replace(/[:\\/.]/g, '-'); + const digest = crypto.createHash('sha256').update(resolved).digest('hex').slice(0, 8); + return path.join(path.resolve(root), `${slug}-${digest}`); } return path.join(cwd, '.impeccable'); } diff --git a/tests/hook.test.mjs b/tests/hook.test.mjs index 03aaf617c..4719573eb 100644 --- a/tests/hook.test.mjs +++ b/tests/hook.test.mjs @@ -9,6 +9,7 @@ import { describe, it, beforeEach, afterEach } from 'node:test'; import assert from 'node:assert/strict'; +import crypto from 'node:crypto'; import fs from 'node:fs'; import path from 'node:path'; import os from 'node:os'; @@ -450,19 +451,40 @@ describe('IMPECCABLE_CACHE_ROOT relocates hook state (issue #422)', () => { assert.equal(getCachePath(cwd), path.join(cwd, '.impeccable', 'hook.cache.json')); }); + // Mirrors hookStateDir's slug formula: readable separator-mapped path plus + // an 8-hex sha256 disambiguator. + function slugFor(p) { + const resolved = path.resolve(p); + const readable = resolved.replace(/[:\\/.]/g, '-'); + const digest = crypto.createHash('sha256').update(resolved).digest('hex').slice(0, 8); + return `${readable}-${digest}`; + } + it('relocates cache and pending under a per-project slug dir', () => { process.env.IMPECCABLE_CACHE_ROOT = cacheRoot; - const slug = String(cwd).replace(/[:\\/.]/g, '-'); - assert.equal(getCachePath(cwd), path.join(cacheRoot, slug, 'hook.cache.json')); - assert.equal(getPendingPath(cwd), path.join(cacheRoot, slug, 'hook.pending.json')); + assert.equal(getCachePath(cwd), path.join(cacheRoot, slugFor(cwd), 'hook.cache.json')); + assert.equal(getPendingPath(cwd), path.join(cacheRoot, slugFor(cwd), 'hook.pending.json')); }); - it('slug maps separators, colons, and dots to hyphens', () => { + it('slug maps separators, colons, and dots to hyphens, with a digest suffix', () => { process.env.IMPECCABLE_CACHE_ROOT = cacheRoot; 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})`); + assert.match(slugDir, /my-app-v2-[0-9a-f]{8}$/, `readable slug + 8-hex digest (got ${slugDir})`); + }); + + it('distinct projects whose readable slugs collide get distinct state dirs', () => { + process.env.IMPECCABLE_CACHE_ROOT = cacheRoot; + const dotted = path.join(cwd, 'my.app'); + const dashed = path.join(cwd, 'my-app'); + // Readable part is identical for both... + assert.equal( + path.resolve(dotted).replace(/[:\\/.]/g, '-'), + path.resolve(dashed).replace(/[:\\/.]/g, '-'), + ); + // ...but the digest keeps their hook state apart. + assert.notEqual(path.dirname(getCachePath(dotted)), path.dirname(getCachePath(dashed))); }); it('trailing separators and relative segments slug to the same dir', () => { @@ -474,8 +496,7 @@ describe('IMPECCABLE_CACHE_ROOT relocates hook state (issue #422)', () => { 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')); + assert.equal(getCachePath(cwd), path.join(cacheRoot, slugFor(cwd), 'hook.cache.json')); }); it('persistCache degrades gracefully when the cache root is unusable', () => { @@ -495,24 +516,14 @@ describe('IMPECCABLE_CACHE_ROOT relocates hook state (issue #422)', () => { assert.equal(getLocalConfigPath(cwd), path.join(cwd, '.impeccable', 'config.local.json')); }); - it('expands a leading ~/ against the home dir, like IMPECCABLE_HOOK_LOG', () => { - const savedHome = process.env.HOME; - const savedProfile = process.env.USERPROFILE; - try { - process.env.HOME = cacheRoot; - delete process.env.USERPROFILE; - process.env.IMPECCABLE_CACHE_ROOT = '~/impeccable-state'; - const slug = path.resolve(cwd).replace(/[:\\/.]/g, '-'); - assert.equal( - getCachePath(cwd), - path.join(cacheRoot, 'impeccable-state', slug, 'hook.cache.json'), - ); - } finally { - if (savedHome === undefined) delete process.env.HOME; - else process.env.HOME = savedHome; - if (savedProfile === undefined) delete process.env.USERPROFILE; - else process.env.USERPROFILE = savedProfile; - } + it('expands a leading ~/ against os.homedir()', () => { + // Property check without duplicating the expansion: the tilde form must + // resolve identically to the explicit homedir-joined form. + process.env.IMPECCABLE_CACHE_ROOT = path.join(os.homedir(), 'impeccable-state'); + const explicit = getCachePath(cwd); + process.env.IMPECCABLE_CACHE_ROOT = '~/impeccable-state'; + assert.equal(getCachePath(cwd), explicit); + assert.ok(explicit.startsWith(os.homedir()), 'anchored under the home dir'); }); it('persistCache round-trips through the redirect dir and leaves the project root clean', () => { @@ -522,8 +533,7 @@ describe('IMPECCABLE_CACHE_ROOT relocates hook state (issue #422)', () => { assert.equal(persistCache(cwd, cache), true); assert.equal(fs.existsSync(path.join(cwd, '.impeccable')), false, 'project root untouched'); - const slug = String(cwd).replace(/[:\\/.]/g, '-'); - assert.equal(fs.existsSync(path.join(cacheRoot, slug, 'hook.cache.json')), true); + assert.equal(fs.existsSync(path.join(cacheRoot, slugFor(cwd), 'hook.cache.json')), true); const reloaded = readCache(cwd); assert.equal(reloaded.sessions['sid-1'].files['/x/a.tsx'].editCount, 1);