mirror of
https://github.com/pbakaus/impeccable.git
synced 2026-09-11 21:57:14 +03:00
Resolve detect DESIGN.md from each target's project, not cwd
The detect CLI loaded DESIGN.md once from process.cwd() and applied it to every scan target. Scanning another project's files from inside a different repo therefore judged them against the wrong project's design system (cross-project contamination observed during eval work: running detect from impeccable-evals against a generated artifact elsewhere applied the evals repo's DESIGN.md). DESIGN.md now resolves by walking up from each scan target's own location to its design root: a directory carrying a DESIGN.md is the root; a directory carrying a project marker (.git / package.json / .impeccable) without a DESIGN.md is a boundary that stops the walk with no design system, so a sibling project never inherits a parent's or cwd's rules. A target with no design root above it falls back to no design system rather than cwd's. Resolution is memoized per root, so a multi-file scan reads each DESIGN.md once, and targets spanning projects each get their own. file:// URLs resolve from their path; remote http(s) URLs get no design system. Adds tests/detect-cli-design-contamination.test.mjs, which spawns the real CLI to prove B's file is not judged by A's DESIGN.md, that a project still governs its own file, that a mixed-project scan resolves per target, and that a marker-less bare file gets no design system. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Fable 5
parent
9f5bbed8b8
commit
70fdc172b8
+49
-14
@@ -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, '<stdin>', options);
|
||||
return detectText(input, '<stdin>', 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));
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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',
|
||||
],
|
||||
},
|
||||
],
|
||||
|
||||
@@ -0,0 +1,132 @@
|
||||
/**
|
||||
* Regression: `impeccable detect <file-or-dir>` 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 =
|
||||
'<!doctype html><html><head><style>.card { font-family: Verdana, sans-serif; }</style></head>' +
|
||||
'<body><div class="card">Hi</div></body></html>';
|
||||
|
||||
// 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',
|
||||
);
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user