diff --git a/cli/engine/cli/main.mjs b/cli/engine/cli/main.mjs index b3edfea8d..e52998b7e 100644 --- a/cli/engine/cli/main.mjs +++ b/cli/engine/cli/main.mjs @@ -1,7 +1,8 @@ import fs from 'node:fs'; import path from 'node:path'; +import { fileURLToPath } from 'node:url'; -import { loadDesignSystemForCwd } from '../design-system.mjs'; +import { loadDesignSystemForTarget } from '../design-system.mjs'; import { RULE_SCOPES, filterByScopes } from '../registry/antipatterns.mjs'; import { createBrowserDetector, detectUrl } from '../engines/browser/detect-url.mjs'; import { detectHtml } from '../engines/static-html/detect-html.mjs'; @@ -27,6 +28,15 @@ function formatFindingSummary(count) { return `${count} anti-pattern${count === 1 ? '' : 's'} found.`; } +// Local filesystem path behind a file:// URL, or null when it can't be mapped. +function fileUrlToLocalPath(url) { + try { + return fileURLToPath(url); + } catch { + return null; + } +} + function formatFindings(findings, jsonMode) { if (jsonMode) return JSON.stringify(findings, null, 2); @@ -52,7 +62,11 @@ function formatFindings(findings, jsonMode) { // Stdin handling // --------------------------------------------------------------------------- -async function handleStdin(options = {}) { +// `optionsFor` maps a local path to scan options carrying that path's own +// project design system (or base options when null). Falls back to a plain +// object so direct/legacy callers still work. +async function handleStdin(optionsFor = () => ({})) { + const resolve = typeof optionsFor === 'function' ? optionsFor : () => optionsFor; const chunks = []; for await (const chunk of process.stdin) chunks.push(chunk); const input = Buffer.concat(chunks).toString('utf-8'); @@ -60,11 +74,12 @@ async function handleStdin(options = {}) { const parsed = JSON.parse(input); const fp = parsed?.tool_input?.file_path; if (fp && fs.existsSync(fp)) { + const options = resolve(fp); return HTML_EXTENSIONS.has(path.extname(fp).toLowerCase()) ? detectHtml(fp, options) : detectText(fs.readFileSync(fp, 'utf-8'), fp, options); } } catch { /* not JSON */ } - return detectText(input, '', options); + return detectText(input, '', resolve(null)); } @@ -199,14 +214,23 @@ async function detectCli() { process.exit(1); } const designSystemEnabled = configEnabled && !args.includes('--no-design-system') && detectionConfig.designSystem?.enabled !== false; - const designSystem = designSystemEnabled ? loadDesignSystemForCwd(process.cwd()) : null; // Inline `impeccable-disable*` waivers are part of the scanned file, so they // apply by default. `--no-config` (raw scan) and the dedicated // `--no-inline-ignores` both turn them off. const inlineIgnoresEnabled = configEnabled && !args.includes('--no-inline-ignores'); - const scanOptions = { inlineIgnores: inlineIgnoresEnabled }; - if (designSystem) scanOptions.designSystem = designSystem; - if (viewport) scanOptions.viewport = viewport; + const baseScanOptions = { inlineIgnores: inlineIgnoresEnabled }; + if (viewport) baseScanOptions.viewport = viewport; + // DESIGN.md must resolve from EACH scan target's own project root, not from + // process.cwd(): scanning project B's files from inside project A applied A's + // design rules (cross-project contamination). Resolve per target, memoized by + // resolved project root so a multi-file scan pays the read once per project. + // A target with no project marker above it gets no design system (never cwd's). + const designSystemCache = new Map(); + const scanOptionsFor = (localPath) => { + if (!designSystemEnabled || !localPath) return baseScanOptions; + const designSystem = loadDesignSystemForTarget(localPath, { cache: designSystemCache }); + return designSystem ? { ...baseScanOptions, designSystem } : baseScanOptions; + }; const targets = args.filter(a => !a.startsWith('--')); if (helpMode) { printUsage(); process.exit(0); } @@ -214,7 +238,7 @@ async function detectCli() { let allFindings = []; if (!process.stdin.isTTY && targets.length === 0) { - allFindings = await handleStdin(scanOptions); + allFindings = await handleStdin(scanOptionsFor); } else { const paths = targets.length > 0 ? targets : [process.cwd()]; // file:// URLs get the same Puppeteer-rendered pass as http(s) — the @@ -228,10 +252,17 @@ async function detectCli() { try { for (const target of paths) { if (urlRe.test(target)) { + // A file:// URL points at a local artifact, so its design system + // resolves from that file's project. A remote http(s) URL has no + // local project — it gets base options (no design system), never + // process.cwd()'s. + const urlOptions = /^file:/i.test(target) + ? scanOptionsFor(fileUrlToLocalPath(target)) + : baseScanOptions; try { const scanner = browserDetector - ? (url) => browserDetector.detectUrl(url, scanOptions) - : (url) => detectUrl(url, scanOptions); + ? (url) => browserDetector.detectUrl(url, urlOptions) + : (url) => detectUrl(url, urlOptions); allFindings.push(...await scanner(target)); } catch (e) { process.stderr.write(`Error: ${e.message}\n`); } continue; @@ -297,11 +328,14 @@ async function detectCli() { for (const file of files) { const ext = path.extname(file).toLowerCase(); + // Each file resolves its own project design system (cached by root), + // so a scan spanning sibling projects applies the right rules per file. + const fileOptions = scanOptionsFor(file); let fileFindings; if (HTML_EXTENSIONS.has(ext)) { - fileFindings = await detectHtml(file, scanOptions); + fileFindings = await detectHtml(file, fileOptions); } else { - fileFindings = detectText(fs.readFileSync(file, 'utf-8'), file, scanOptions); + fileFindings = detectText(fs.readFileSync(file, 'utf-8'), file, fileOptions); } // Annotate findings with import context const importers = importedByMap.get(file); @@ -316,10 +350,11 @@ async function detectCli() { } else if (stat.isFile()) { if (shouldIgnoreDetectionFile(resolved, process.cwd(), detectionConfig)) continue; const ext = path.extname(resolved).toLowerCase(); + const fileOptions = scanOptionsFor(resolved); if (HTML_EXTENSIONS.has(ext)) { - allFindings.push(...await detectHtml(resolved, scanOptions)); + allFindings.push(...await detectHtml(resolved, fileOptions)); } else { - allFindings.push(...detectText(fs.readFileSync(resolved, 'utf-8'), resolved, scanOptions)); + allFindings.push(...detectText(fs.readFileSync(resolved, 'utf-8'), resolved, fileOptions)); } } } diff --git a/cli/engine/design-system.mjs b/cli/engine/design-system.mjs index 874b346d7..c4d31eade 100644 --- a/cli/engine/design-system.mjs +++ b/cli/engine/design-system.mjs @@ -1,4 +1,5 @@ import fs from 'node:fs'; +import os from 'node:os'; import path from 'node:path'; import { finding } from './findings.mjs'; @@ -7,6 +8,11 @@ import { parseAnyColor, resolveLengthPx } from './rules/checks.mjs'; const DESIGN_NAMES = ['DESIGN.md', 'Design.md', 'design.md']; const FALLBACK_DIRS = ['.agents/context', 'docs']; +// Files/dirs whose presence marks a directory as a project root. Mirrors the +// walk-up semantics of skill/scripts/context.mjs (`resolveProject`), which the +// CLI can't import (separate tree). `.git` and `package.json` are the common +// boundaries; `.impeccable` is our own project marker. +const PROJECT_ROOT_MARKERS = ['.git', 'package.json', '.impeccable']; const COLOR_CHANNEL_TOLERANCE = 6; const RADIUS_TOLERANCE_PX = 0.5; const FONT_SIZE_TOLERANCE_PX = 0.5; @@ -469,6 +475,62 @@ function loadDesignSystemForCwd(cwd = process.cwd()) { }); } +// Directory to begin the project-root walk from, given a scan target that may +// be a file or a directory (and may not exist yet). +function designSystemStartDir(targetPath, cwd = process.cwd()) { + const abs = path.isAbsolute(targetPath) ? targetPath : path.resolve(cwd, targetPath); + try { + return fs.statSync(abs).isDirectory() ? abs : path.dirname(abs); + } catch { + // Nonexistent path: treat an extension-bearing leaf as a file. + return path.extname(abs) ? path.dirname(abs) : abs; + } +} + +// Walk up from `startDir` to the directory that governs the target's design +// system, mirroring skill/scripts/context.mjs's project-boundary semantics: +// +// - A directory carrying a DESIGN.md (directly or in a fallback dir) IS the +// design root — that's where the rules live. +// - A directory carrying a project marker (.git / package.json / .impeccable) +// but no DESIGN.md is a project BOUNDARY: the walk stops with no design +// system, so a sibling project never inherits a parent's or cwd's rules. +// - Reaching the home directory / filesystem root with neither means no +// design system at all — never process.cwd()'s. +// +// Returns { dir, hasDesign } for the stopping directory, or null when the walk +// runs out. This is the fix for cross-project contamination. +export function findDesignRoot(startDir) { + let dir = path.resolve(startDir); + const homeDir = path.resolve(os.homedir()); + while (true) { + if (resolveDesignMdPath(dir)) return { dir, hasDesign: true }; + if (PROJECT_ROOT_MARKERS.some((marker) => fs.existsSync(path.join(dir, marker)))) { + return { dir, hasDesign: false }; + } + if (dir === homeDir) return null; + const parent = path.dirname(dir); + if (parent === dir) return null; + dir = parent; + } +} + +// Resolve the design system that governs a specific scan target, by walking up +// from the target's own location — never process.cwd(). Scanning project B's +// files from inside project A applies B's DESIGN.md (or none), not A's. +// +// Pass a `cache` Map to memoize by resolved design root across a multi-file +// scan; a target with no design root above it resolves to null. +export function loadDesignSystemForTarget(targetPath, { cache, cwd = process.cwd() } = {}) { + const startDir = designSystemStartDir(targetPath, cwd); + const found = findDesignRoot(startDir); + const key = found ? `root:${found.dir}` : '\0none'; + if (cache && cache.has(key)) return cache.get(key); + const loaded = found?.hasDesign ? loadDesignSystemForCwd(found.dir) : null; + if (cache) cache.set(key, loaded); + return loaded; +} + function isAllowedFont(font, designSystem) { if (!font || GENERIC_FONTS.has(font)) return true; if (!designSystem?.hasFonts) return true; diff --git a/scripts/test-suites.mjs b/scripts/test-suites.mjs index ea77cbf25..365f1dea4 100644 --- a/scripts/test-suites.mjs +++ b/scripts/test-suites.mjs @@ -86,7 +86,7 @@ export const SUITES = { /^scripts\/(benchmark-detector|build-browser-detector|build-extension)\.js$/, /^site\/(pages\/detector|public\/antipattern|data\/anti-patterns-catalog\.js)/, /^tests\/design-system\.test\.mjs$/, - /^tests\/(detect-antipatterns|inline-ignores|extension-build|fixtures\/antipatterns)/, + /^tests\/(detect-antipatterns|detect-cli-design-contamination|inline-ignores|extension-build|fixtures\/antipatterns)/, ], commands: [ { @@ -104,6 +104,7 @@ export const SUITES = { 'tests/design-system.test.mjs', 'tests/detect-antipatterns-fixtures.test.mjs', 'tests/detect-antipatterns-browser.test.mjs', + 'tests/detect-cli-design-contamination.test.mjs', ], }, ], diff --git a/tests/detect-cli-design-contamination.test.mjs b/tests/detect-cli-design-contamination.test.mjs new file mode 100644 index 000000000..908a44e08 --- /dev/null +++ b/tests/detect-cli-design-contamination.test.mjs @@ -0,0 +1,132 @@ +/** + * Regression: `impeccable detect ` must resolve DESIGN.md from + * EACH scan target's own project root, not from process.cwd(). + * + * The bug (found during eval work): running detect from repo A against a file + * that lives in repo B applied A's DESIGN.md to B — cross-project contamination. + * These tests spawn the real CLI so the fix is exercised end to end. + * + * Run with: node --test tests/detect-cli-design-contamination.test.mjs + */ + +import { describe, it, before, after } from 'node:test'; +import assert from 'node:assert/strict'; +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import { spawnSync } from 'node:child_process'; +import { fileURLToPath } from 'node:url'; + +const __dirname = path.dirname(fileURLToPath(import.meta.url)); +const CLI = path.resolve(__dirname, '../cli/bin/cli.js'); + +// Verdana is a plain web-safe font: it is not in OVERUSED_FONTS and trips no +// standalone rule, so the only way it becomes a `design-system-font` finding is +// if a DESIGN.md that forbids it gets applied. +const PAGE_HTML = + '' + + '
Hi
'; + +// A DESIGN.md whose typography allows only Palatino — Verdana violates it. +const DESIGN_MD = `--- +typography: + body: + fontFamily: "Palatino, Georgia, serif" +--- +# Project A Design System +`; + +const tempRoots = []; + +function mkProject({ withDesign, withMarker = true }) { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'impeccable-detect-contam-')); + tempRoots.push(dir); + if (withMarker) fs.writeFileSync(path.join(dir, 'package.json'), '{"name":"fixture"}'); + if (withDesign) fs.writeFileSync(path.join(dir, 'DESIGN.md'), DESIGN_MD); + const page = path.join(dir, 'page.html'); + fs.writeFileSync(page, PAGE_HTML); + return { dir, page }; +} + +// Run the CLI from `cwd`; force the node binary so the HTML/jsdom path never +// runs under bun (which is unusably slow). +function runDetect(cwd, targets) { + const result = spawnSync(process.execPath, [CLI, 'detect', '--json', ...targets], { + cwd, + encoding: 'utf-8', + }); + let findings = []; + try { + findings = JSON.parse(result.stdout || '[]'); + } catch { + throw new Error(`Non-JSON CLI output.\nstdout: ${result.stdout}\nstderr: ${result.stderr}`); + } + return findings; +} + +function fontFindingsFor(findings, file) { + return findings.filter( + (f) => f.antipattern === 'design-system-font' && (!file || f.file === file), + ); +} + +let projA; +let projB; + +before(() => { + projA = mkProject({ withDesign: true }); // DESIGN.md forbids Verdana + projB = mkProject({ withDesign: false }); // its own project, no DESIGN.md +}); + +after(() => { + for (const dir of tempRoots) { + try { fs.rmSync(dir, { recursive: true, force: true }); } catch { /* best effort */ } + } +}); + +describe('detect CLI DESIGN.md resolution', () => { + it('does NOT apply cwd project A\'s DESIGN.md to project B\'s file (the contamination bug)', () => { + const findings = runDetect(projA.dir, [projB.page]); + assert.deepEqual( + fontFindingsFor(findings, projB.page).map((f) => f.ignoreValue), + [], + 'project B\'s Verdana must not be flagged by project A\'s DESIGN.md', + ); + }); + + it('still applies a project\'s own DESIGN.md to its own file (positive control)', () => { + const findings = runDetect(projA.dir, [projA.page]); + assert.ok( + fontFindingsFor(findings, projA.page).some((f) => f.ignoreValue === 'verdana'), + 'project A\'s own DESIGN.md must flag Verdana in project A\'s file', + ); + }); + + it('resolves per target when one scan spans two projects', () => { + const findings = runDetect(projA.dir, [projA.page, projB.page]); + assert.ok( + fontFindingsFor(findings, projA.page).length > 0, + 'A\'s file should be judged against A\'s DESIGN.md', + ); + assert.equal( + fontFindingsFor(findings, projB.page).length, + 0, + 'B\'s file should NOT be judged against A\'s DESIGN.md', + ); + }); + + it('falls back to no design system for a bare file with no project markers above it', () => { + // A lone file whose directory has neither .git, package.json, nor .impeccable. + const bareDir = fs.mkdtempSync(path.join(os.tmpdir(), 'impeccable-detect-bare-')); + tempRoots.push(bareDir); + const barePage = path.join(bareDir, 'page.html'); + fs.writeFileSync(barePage, PAGE_HTML); + + const findings = runDetect(projA.dir, [barePage]); + assert.equal( + fontFindingsFor(findings, barePage).length, + 0, + 'a project-less file must fall back to no design system, not cwd\'s', + ); + }); +});