Merge pull request #471 from pbakaus/hook-skip-outside-project

fix: skip design-hook scans for files outside the resolved project root
2026-09-13 06:36:26 +03:00 · 2026-08-02 20:07:31 -07:00
parent 8cf362b6fe 62a2026afc
commit ae2be34fdc
3 changed files with 175 additions and 12 deletions
@@ -23,6 +23,7 @@ import {
  designSystemOptions,
  filterFindings,
  isNativePlatform,
+  isScanTargetInsideProject,
  loadDetector,
  matchConfiguredExtension,
  matchesAnyGlob,
@@ -161,7 +162,7 @@ function replaceOnce(original, oldString, newString) {
 }

 function readExistingProjectFile(filePath, cwd) {
-  if (!isInsideProject(filePath, cwd)) return null;
+  if (!isScanTargetInsideProject(filePath, cwd)) return null;
  if (SENSITIVE_PATH.test(filePath) || GENERATED_PATH.test(filePath)) return null;
  try {
    const stat = fs.statSync(filePath);
@@ -232,7 +233,7 @@ function shellCopiedFileContent(command, cwd) {
  const source = shellCopyPaths(command)?.source;
  if (!source) return '';
  const sourcePath = path.isAbsolute(source) ? source : path.resolve(cwd, source);
-  if (!isInsideProject(sourcePath, cwd)) return '';
+  if (!isScanTargetInsideProject(sourcePath, cwd)) return '';
  if (SENSITIVE_PATH.test(sourcePath) || GENERATED_PATH.test(sourcePath)) return '';
  try {
    const stat = fs.statSync(sourcePath);
@@ -328,15 +329,6 @@ function relativePath(filePath, cwd) {
  }
 }

-function isInsideProject(filePath, cwd) {
-  try {
-    const rel = path.relative(cwd, filePath);
-    return rel === '' || (!rel.startsWith('..') && !path.isAbsolute(rel));
-  } catch {
-    return false;
-  }
-}
-
 // The static HTML engine reads its input from disk, but preToolUse only has
 // the proposed content. Stage it in a temp file so html-engine targets get the
 // same DOM-structural rules pre-write that runHook applies post-edit.
@@ -414,7 +406,7 @@ async function main() {
  };

  if (!filePath) return allow({ ...audit, skipped: 'no-file-path', durationMs: Date.now() - started });
-  if (!isInsideProject(filePath, cwd)) return allow({ ...audit, skipped: 'outside-project', durationMs: Date.now() - started });
+  if (!isScanTargetInsideProject(filePath, cwd)) return allow({ ...audit, skipped: 'outside-project', durationMs: Date.now() - started });
  if (SENSITIVE_PATH.test(filePath)) return allow({ ...audit, skipped: 'sensitive', durationMs: Date.now() - started });
  if (GENERATED_PATH.test(filePath)) return allow({ ...audit, skipped: 'generated', durationMs: Date.now() - started });

@@ -1335,6 +1335,51 @@ function isInsideProject(filePath, projectCwd) {
  }
 }

+// Resolve a path to its canonical (symlink-free) form. When the path does
+// not exist yet — the before-edit hook gates proposed Writes — canonicalize
+// the nearest existing ancestor and re-append the remainder, so a new file
+// under a symlinked root still compares equal to its canonical project.
+// Memoized: the hook runs as a fresh process per tool event, so the cache
+// amounts to once-per-event work — the scan loops re-check the same project
+// root for every target file. The cap only matters to long-lived importers
+// like the test runner.
+const canonicalPathCache = new Map();
+const CANONICAL_PATH_CACHE_MAX = 1024;
+
+function canonicalPath(p) {
+  const resolved = path.resolve(p);
+  if (canonicalPathCache.has(resolved)) return canonicalPathCache.get(resolved);
+  let canonical = resolved;
+  let dir = resolved;
+  const tail = [];
+  while (true) {
+    try {
+      canonical = tail.length ? path.join(fs.realpathSync(dir), ...tail) : fs.realpathSync(dir);
+      break;
+    } catch { /* keep climbing */ }
+    const parent = path.dirname(dir);
+    if (parent === dir) break;
+    tail.unshift(path.basename(dir));
+    dir = parent;
+  }
+  if (canonicalPathCache.size >= CANONICAL_PATH_CACHE_MAX) canonicalPathCache.clear();
+  canonicalPathCache.set(resolved, canonical);
+  return canonical;
+}
+
+// Containment gate shared by the before-edit hook and both scan passes. A
+// session routinely touches files that belong to no project or to a
+// different one — harness scratchpad dirs under the system temp root,
+// sibling checkouts, one-off throwaway HTML — and findings against those are
+// judged with THIS project's config and DESIGN.md palette, which is never
+// right. Skip them (audit reason: outside-project). Paths are canonicalized
+// first so a symlinked root (macOS /tmp -> /private/tmp) doesn't split the
+// comparison.
+export function isScanTargetInsideProject(filePath, projectCwd) {
+  if (!filePath || !projectCwd) return false;
+  return isInsideProject(canonicalPath(filePath), canonicalPath(projectCwd));
+}
+
 export function parseStaticStyleImports(content, fromFile, projectCwd) {
  if (!content || typeof content !== 'string') return [];
  const dir = path.dirname(fromFile);
@@ -1693,6 +1738,10 @@ export async function runHook({ stdinJson, env = {}, cwd = process.cwd(), now =
        lastSkip = 'file-missing';
        continue;
      }
+      if (!isScanTargetInsideProject(filePath, projectCwd)) {
+        lastSkip = 'outside-project';
+        continue;
+      }

      const maxFileBytes = config.limits?.maxFileBytes ?? DEFAULT_CONFIG.limits.maxFileBytes;
      if (maxFileBytes > 0) {
@@ -2023,6 +2072,10 @@ export async function runStopHook({ stdinJson, env = {}, cwd = process.cwd(), no
      const relForMatch = relativize(filePath, projectCwd);
      if (matchesAnyGlob(relForMatch, config.ignoreFiles) || matchesAnyGlob(filePath, config.ignoreFiles)) continue;
      if (!fs.existsSync(filePath)) continue;
+      // Caches written before this gate existed can still hold out-of-project
+      // paths, so the Stop pass re-checks containment rather than trusting
+      // the per-edit pass to have filtered them.
+      if (!isScanTargetInsideProject(filePath, projectCwd)) continue;

      scanned += 1;
      let content = '';
@@ -60,6 +60,7 @@ import {
  resolveProjectPlatform,
  isNativePlatform,
  normalizeIgnoreValueEntries,
+  isScanTargetInsideProject,
 } from '../skill/scripts/hook-lib.mjs';
 import { normalizeIgnoreValueEntries as normalizeIgnoreValueEntriesCli } from '../cli/lib/impeccable-config.mjs';
 import { detectHtml, detectText } from '../cli/engine/detect-antipatterns.mjs';
@@ -158,6 +159,57 @@ describe('SENSITIVE_PATH / GENERATED_PATH', () => {
  });
 });

+describe('isScanTargetInsideProject()', () => {
+  let root;
+  beforeEach(() => { root = mkTmp(); });
+  afterEach(() => fs.rmSync(root, { recursive: true, force: true }));
+
+  it('accepts files under the project root and the root itself', () => {
+    const file = path.join(root, 'src', 'Card.tsx');
+    fs.mkdirSync(path.dirname(file), { recursive: true });
+    fs.writeFileSync(file, 'noop');
+    assert.equal(isScanTargetInsideProject(file, root), true);
+    assert.equal(isScanTargetInsideProject(root, root), true);
+  });
+
+  it('rejects siblings, temp scratchpads, and empty inputs', () => {
+    const scratch = mkTmp();
+    try {
+      const outside = path.join(scratch, 'landing.html');
+      fs.writeFileSync(outside, '<h1>x</h1>');
+      assert.equal(isScanTargetInsideProject(outside, root), false);
+      assert.equal(isScanTargetInsideProject('', root), false);
+      assert.equal(isScanTargetInsideProject(outside, ''), false);
+    } finally {
+      fs.rmSync(scratch, { recursive: true, force: true });
+    }
+  });
+
+  it('treats symlinked and canonical forms of the same tree as one project', () => {
+    const real = path.join(root, 'real');
+    const link = path.join(root, 'link');
+    fs.mkdirSync(path.join(real, 'src'), { recursive: true });
+    fs.symlinkSync(real, link);
+    const file = path.join(real, 'src', 'Card.tsx');
+    fs.writeFileSync(file, 'noop');
+    assert.equal(isScanTargetInsideProject(file, link), true);
+    assert.equal(isScanTargetInsideProject(path.join(link, 'src', 'Card.tsx'), real), true);
+  });
+
+  it('classifies not-yet-written files by their nearest existing ancestor', () => {
+    // The before-edit hook gates proposed Writes, so the target often does
+    // not exist. Canonicalization must climb to an existing ancestor rather
+    // than bail, or a new file under a symlinked root would read as outside.
+    const real = path.join(root, 'real');
+    const link = path.join(root, 'link');
+    fs.mkdirSync(real, { recursive: true });
+    fs.symlinkSync(real, link);
+    assert.equal(isScanTargetInsideProject(path.join(link, 'src', 'New.tsx'), real), true);
+    assert.equal(isScanTargetInsideProject(path.join(real, 'deep', 'New.tsx'), link), true);
+    assert.equal(isScanTargetInsideProject(path.join(root, 'elsewhere', 'New.tsx'), real), false);
+  });
+});
+
 describe('readConfig()', () => {
  let cwd;
  beforeEach(() => { cwd = mkTmp(); });
@@ -1538,6 +1590,49 @@ rounded:
    assert.equal(r.audit.skipped, 'sensitive');
  });

+  it('rejects files outside the project, like harness scratchpads', async () => {
+    // Session cwd is a real project; the touched file is a throwaway HTML in
+    // a temp dir elsewhere. Findings against it would be judged with this
+    // project's config and DESIGN.md, so the scan must skip it entirely.
+    fs.writeFileSync(path.join(cwd, 'package.json'), '{"name":"proj"}');
+    const scratch = mkTmp();
+    try {
+      const file = path.join(scratch, 'id-test', 'landing.html');
+      fs.mkdirSync(path.dirname(file), { recursive: true });
+      fs.writeFileSync(file, '<h1>throwaway</h1>');
+      const det = fakeDetector([finding('side-tab', 1)]);
+      const r = await runHook({ stdinJson: JSON.stringify(eventFor(file)), env: {}, cwd, detector: det });
+      assert.equal(r.stdout, '');
+      assert.equal(r.audit.skipped, 'outside-project');
+      assert.ok(!fs.existsSync(path.join(cwd, '.impeccable')), 'out-of-project edit must not dirty the cache');
+    } finally {
+      fs.rmSync(scratch, { recursive: true, force: true });
+    }
+  });
+
+  it('still scans a file whose project root is reached through a symlinked cwd', async () => {
+    // macOS /tmp -> /private/tmp style: the session cwd is a symlink to the
+    // project while the tool reports the canonical file path. Containment
+    // compares canonical paths, so this is inside, not outside.
+    const real = path.join(cwd, 'realproj');
+    const link = path.join(cwd, 'proj-link');
+    fs.mkdirSync(path.join(real, 'src'), { recursive: true });
+    fs.writeFileSync(path.join(real, 'package.json'), '{"name":"proj"}');
+    fs.symlinkSync(real, link);
+    const file = path.join(real, 'src', 'Card.tsx');
+    fs.writeFileSync(file, 'noop');
+    const event = {
+      session_id: 'sym-sid',
+      cwd: link,
+      hook_event_name: 'PostToolUse',
+      tool_name: 'Edit',
+      tool_input: { file_path: file },
+    };
+    const r = await runHook({ stdinJson: JSON.stringify(event), env: {}, cwd: link, detector: fakeDetector([]) });
+    assert.notEqual(r.audit.skipped, 'outside-project');
+    assert.match(r.stdout, /No deterministic design-quality issues found/);
+  });
+
  it('rejects extensions outside the allowlist', async () => {
    const file = writeFixture('docs/README.md', 'noop');
    const r = await runHook({ stdinJson: JSON.stringify(eventFor(file)), env: {}, cwd });
@@ -3348,6 +3443,29 @@ describe('runStopHook()', () => {
    assert.equal(r.audit.skipped, 'no-touched-files');
  });

+  it('skips out-of-project files even when an older cache still lists them', async () => {
+    // Caches written before the containment gate can hold scratchpad paths.
+    // The deep pass re-checks containment instead of trusting the per-edit
+    // pass to have filtered them.
+    const sid = 'stop-outside';
+    const scratch = fs.mkdtempSync(path.join(os.tmpdir(), 'impeccable-scratch-'));
+    try {
+      const outside = path.join(scratch, 'landing.html');
+      fs.writeFileSync(outside, '<h1>throwaway</h1>');
+      persistCache(cwd, {
+        version: 1,
+        sessions: { [sid]: { updatedAt: Date.now(), files: { [outside]: { editCount: 1, findings: [] } } } },
+      });
+      const det = fakeDetector([finding('side-tab', 7)]);
+      const stop = await runStopHook({ stdinJson: JSON.stringify(stopEvent(sid)), env: {}, cwd, detector: det });
+      assert.equal(stop.stdout, '');
+      assert.equal(stop.audit.skipped, 'stop-clean');
+      assert.equal(stop.audit.scannedFiles, 0);
+    } finally {
+      fs.rmSync(scratch, { recursive: true, force: true });
+    }
+  });
+
  it('a second Stop fire is silent: deep-pass findings are remembered', async () => {
    const sid = 'stop-twice';
    const file = write('src/Card.tsx', 'noop');