Compare commits

...
Author SHA1 Message Date
Paul Bakaus 52380df1ad Report unreadable detector directories
AI assistance disclosure: Codex implemented and tested this fix under maintainer direction.
2026-09-02 19:12:03 -07:00
Paul Bakaus 840965fc77 Handle unreadable detector targets
AI assistance disclosure: Codex implemented and tested this fix under maintainer direction.
2026-09-02 19:01:05 -07:00
Paul Bakaus 2001c81686 Fix local target failure exit codes
AI assistance disclosure: Codex implemented and tested this fix under maintainer direction.
2026-09-02 18:48:43 -07:00
Paul Bakaus 7437368526 Fix URL scan failure exit codes
Return exit 1 when browser setup or a URL scan fails, including partial multi-target scans, while preserving JSON findings output. Document the detector exit contract and cover isolated installs without Puppeteer.\n\nAI assistance disclosure: Codex implemented and tested this fix under maintainer direction.
2026-09-02 18:39:31 -07:00
6 changed files with 288 additions and 28 deletions
+1 -1
View File
@@ -427,7 +427,7 @@ npx impeccable ignores add-value overused-font Inter --reason "Brand font"
The detector catches 61 deterministic issues across AI slop (side-tab borders, purple gradients, bounce easing, dark glows) and general design quality (line length, cramped padding, small touch targets, skipped headings, and more). The detector catches 61 deterministic issues across AI slop (side-tab borders, purple gradients, bounce easing, dark glows) and general design quality (line length, cramped padding, small touch targets, skipped headings, and more).
Human-readable findings are diagnostics written to stderr, so redirect them with `2> findings.txt`. Use `--json` for machine-readable results on stdout. URL scans inspect the rendered DOM, computed layout, and accessible linked stylesheets; browser security still prevents reading cross-origin CSS without CORS. A clean detector run is evidence, not proof of visual or accessibility quality: it does not replace inspecting the rendered experience across relevant viewports. Human-readable findings are diagnostics written to stderr, so redirect them with `2> findings.txt`. Use `--json` for machine-readable results on stdout. Exit `0` means the scan completed without primary findings, exit `2` means it completed with primary findings, and exit `1` means at least one requested target could not be scanned; operational failure takes precedence for a partial multi-target scan. URL scans inspect the rendered DOM, computed layout, and accessible linked stylesheets; browser security still prevents reading cross-origin CSS without CORS. A clean detector run is evidence, not proof of visual or accessibility quality: it does not replace inspecting the rendered experience across relevant viewports.
By default, `detect` respects the same `.impeccable/config.json` and `.impeccable/config.local.json` detector config as the design hook: `detector.ignoreRules`, `detector.ignoreFiles`, `detector.ignoreValues`, and `detector.designSystem.enabled`. Hook lifecycle settings such as `hook.enabled` only affect automatic hook execution. By default, `detect` respects the same `.impeccable/config.json` and `.impeccable/config.local.json` detector config as the design hook: `detector.ignoreRules`, `detector.ignoreFiles`, `detector.ignoreValues`, and `detector.designSystem.enabled`. Hook lifecycle settings such as `hook.enabled` only affect automatic hook execution.
+5 -2
View File
@@ -60,8 +60,11 @@ npx impeccable detect --fast src/
## Exit Codes ## Exit Codes
- `0`: no issues found - `0`: scan completed with no primary findings (advisories may still be listed)
- `2`: anti-patterns detected - `1`: at least one requested target could not be scanned
- `2`: scan completed with primary findings
Operational failure takes precedence when a multi-target scan is partial. In JSON mode, stdout remains a findings array and diagnostics are written to stderr.
## Options ## Options
+66 -20
View File
@@ -189,6 +189,12 @@ Output streams:
Human-readable findings go to stderr so stdout stays available for structured Human-readable findings go to stderr so stdout stays available for structured
output. Use --json for JSON on stdout, or redirect text with 2> findings.txt. output. Use --json for JSON on stdout, or redirect text with 2> findings.txt.
Exit status:
0 Scan completed with no primary findings (advisories may still be listed)
1 At least one requested target could not be scanned
2 Scan completed with primary findings
Operational failure takes precedence when a multi-target scan is partial.
Project config: Project config:
Respects .impeccable/config.json and .impeccable/config.local.json detector Respects .impeccable/config.json and .impeccable/config.local.json detector
settings: detector.ignoreRules, detector.ignoreFiles, detector.ignoreValues, settings: detector.ignoreRules, detector.ignoreFiles, detector.ignoreValues,
@@ -309,6 +315,11 @@ async function detectCli() {
if (helpMode) { printUsage(); process.exit(0); } if (helpMode) { printUsage(); process.exit(0); }
let allFindings = []; let allFindings = [];
let hadOperationalFailure = false;
const reportLocalScanFailure = (target, error) => {
hadOperationalFailure = true;
process.stderr.write(`Error: cannot scan ${target}: ${error.message}\n`);
};
if (!process.stdin.isTTY && targets.length === 0) { if (!process.stdin.isTTY && targets.length === 0) {
allFindings = await handleStdin(scanOptionsFor); allFindings = await handleStdin(scanOptionsFor);
@@ -319,11 +330,22 @@ async function detectCli() {
// browser-grade scan of a local artifact can pass file:///abs/path.html // browser-grade scan of a local artifact can pass file:///abs/path.html
// instead of the bare path (which stays on the static engine). // instead of the bare path (which stays on the static engine).
const urlTargetCount = paths.filter(target => URL_TARGET_RE.test(target)).length; const urlTargetCount = paths.filter(target => URL_TARGET_RE.test(target)).length;
const browserDetector = urlTargetCount > 1 ? await createBrowserDetector() : null; let browserDetector = null;
let browserSetupFailed = false;
if (urlTargetCount > 1) {
try {
browserDetector = await createBrowserDetector();
} catch (e) {
browserSetupFailed = true;
hadOperationalFailure = true;
process.stderr.write(`Error: ${e.message}\n`);
}
}
try { try {
for (const target of paths) { for (const target of paths) {
if (URL_TARGET_RE.test(target)) { if (URL_TARGET_RE.test(target)) {
if (browserSetupFailed) continue;
// A file:// URL points at a local artifact, so its design system // 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 // resolves from that file's project. A remote http(s) URL has no
// local project — it gets base options (no design system), never // local project — it gets base options (no design system), never
@@ -336,14 +358,21 @@ async function detectCli() {
? (url) => browserDetector.detectUrl(url, urlOptions) ? (url) => browserDetector.detectUrl(url, urlOptions)
: (url) => detectUrl(url, urlOptions); : (url) => detectUrl(url, urlOptions);
allFindings.push(...await scanner(target)); allFindings.push(...await scanner(target));
} catch (e) { process.stderr.write(`Error: ${e.message}\n`); } } catch (e) {
hadOperationalFailure = true;
process.stderr.write(`Error: ${e.message}\n`);
}
continue; continue;
} }
const resolved = path.resolve(target); const resolved = path.resolve(target);
let stat; let stat;
try { stat = fs.statSync(resolved); } try { stat = fs.statSync(resolved); }
catch { process.stderr.write(`Warning: cannot access ${target}\n`); continue; } catch {
hadOperationalFailure = true;
process.stderr.write(`Warning: cannot access ${target}\n`);
continue;
}
if (stat.isDirectory()) { if (stat.isDirectory()) {
// Check for framework dev server config (skip in JSON/quiet modes to avoid polluting output) // Check for framework dev server config (skip in JSON/quiet modes to avoid polluting output)
@@ -372,7 +401,7 @@ async function detectCli() {
} }
} }
const files = walkDir(resolved) const files = walkDir(resolved, reportLocalScanFailure)
.filter(file => !shouldIgnoreDetectionFile(file, process.cwd(), detectionConfig)); .filter(file => !shouldIgnoreDetectionFile(file, process.cwd(), detectionConfig));
const htmlCount = files.filter(f => HTML_EXTENSIONS.has(path.extname(f).toLowerCase())).length; const htmlCount = files.filter(f => HTML_EXTENSIONS.has(path.extname(f).toLowerCase())).length;
@@ -388,7 +417,11 @@ async function detectCli() {
} }
// Build import graph for multi-file awareness // Build import graph for multi-file awareness
const graph = buildImportGraph(files); const unreadableFiles = new Set();
const graph = buildImportGraph(files, (file, error) => {
unreadableFiles.add(file);
reportLocalScanFailure(file, error);
});
// Build reverse map: file -> set of files that import it // Build reverse map: file -> set of files that import it
const importedByMap = new Map(); const importedByMap = new Map();
for (const [importer, imports] of graph) { for (const [importer, imports] of graph) {
@@ -399,24 +432,33 @@ async function detectCli() {
} }
for (const file of files) { for (const file of files) {
// Each file resolves its own project design system (cached by root), if (unreadableFiles.has(file)) continue;
// so a scan spanning sibling projects applies the right rules per file. try {
const fileOptions = scanOptionsFor(file); // Each file resolves its own project design system (cached by root),
const fileFindings = await detectLocalFile(file, fileOptions); // so a scan spanning sibling projects applies the right rules per file.
// Annotate findings with import context const fileOptions = scanOptionsFor(file);
const importers = importedByMap.get(file); const fileFindings = await detectLocalFile(file, fileOptions);
if (importers && importers.size > 0) { // Annotate findings with import context
const importerNames = [...importers].map(f => path.basename(f)); const importers = importedByMap.get(file);
for (const f of fileFindings) { if (importers && importers.size > 0) {
f.importedBy = importerNames; const importerNames = [...importers].map(f => path.basename(f));
for (const f of fileFindings) {
f.importedBy = importerNames;
}
} }
allFindings.push(...fileFindings);
} catch (error) {
reportLocalScanFailure(file, error);
} }
allFindings.push(...fileFindings);
} }
} else if (stat.isFile()) { } else if (stat.isFile()) {
if (shouldIgnoreDetectionFile(resolved, process.cwd(), detectionConfig)) continue; if (shouldIgnoreDetectionFile(resolved, process.cwd(), detectionConfig)) continue;
const fileOptions = scanOptionsFor(resolved); try {
allFindings.push(...await detectLocalFile(resolved, fileOptions)); const fileOptions = scanOptionsFor(resolved);
allFindings.push(...await detectLocalFile(resolved, fileOptions));
} catch (error) {
reportLocalScanFailure(target, error);
}
} }
} }
} finally { } finally {
@@ -433,6 +475,10 @@ async function detectCli() {
// advisory-only scan still prints its notes but exits 0 (a clean pass), so // advisory-only scan still prints its notes but exits 0 (a clean pass), so
// advisory rules never break CI or block automation. // advisory rules never break CI or block automation.
const { primary, advisory } = partitionAdvisory(allFindings); const { primary, advisory } = partitionAdvisory(allFindings);
// Exit 1 means at least one requested scan could not complete. It takes
// precedence over exit 2 because findings from the remaining targets do not
// turn a partial scan into a complete one.
const exitCode = hadOperationalFailure ? 1 : (primary.length > 0 ? 2 : 0);
if (allFindings.length > 0) { if (allFindings.length > 0) {
if (jsonMode) process.stdout.write(formatFindings(allFindings, true) + '\n'); if (jsonMode) process.stdout.write(formatFindings(allFindings, true) + '\n');
@@ -443,10 +489,10 @@ async function detectCli() {
} }
} }
else process.stderr.write(formatFindings(allFindings, false) + '\n'); else process.stderr.write(formatFindings(allFindings, false) + '\n');
process.exit(primary.length > 0 ? 2 : 0); process.exit(exitCode);
} }
if (jsonMode) process.stdout.write('[]\n'); if (jsonMode) process.stdout.write('[]\n');
process.exit(0); process.exit(exitCode);
} }
export { formatFindings, handleStdin, confirm, printUsage, detectCli }; export { formatFindings, handleStdin, confirm, printUsage, detectCli };
+17 -5
View File
@@ -46,15 +46,20 @@ const IMPORT_SPECIFIER_PATTERNS = [
/@(?:use|forward)\s+['"]([^'"]+)['"]/g, /@(?:use|forward)\s+['"]([^'"]+)['"]/g,
]; ];
function walkDir(dir) { function walkDir(dir, onReadError = null) {
const files = []; const files = [];
let entries; let entries;
try { entries = fs.readdirSync(dir, { withFileTypes: true }); } catch { return files; } try {
entries = fs.readdirSync(dir, { withFileTypes: true });
} catch (error) {
if (onReadError) onReadError(dir, error);
return files;
}
for (const entry of entries) { for (const entry of entries) {
if (SKIP_DIRS.has(entry.name)) continue; if (SKIP_DIRS.has(entry.name)) continue;
if (entry.isDirectory() && entry.name.startsWith('.') && !HIDDEN_SOURCE_DIRS.has(entry.name)) continue; if (entry.isDirectory() && entry.name.startsWith('.') && !HIDDEN_SOURCE_DIRS.has(entry.name)) continue;
const full = path.join(dir, entry.name); const full = path.join(dir, entry.name);
if (entry.isDirectory()) files.push(...walkDir(full)); if (entry.isDirectory()) files.push(...walkDir(full, onReadError));
else if (hasScannableExtension(entry.name)) files.push(full); else if (hasScannableExtension(entry.name)) files.push(full);
} }
return files; return files;
@@ -81,12 +86,19 @@ function resolveImport(specifier, fromDir, fileSet) {
return null; return null;
} }
function buildImportGraph(files) { function buildImportGraph(files, onReadError = null) {
const fileSet = new Set(files); const fileSet = new Set(files);
const graph = new Map(); const graph = new Map();
for (const file of files) { for (const file of files) {
const content = fs.readFileSync(file, 'utf-8'); let content;
try {
content = fs.readFileSync(file, 'utf-8');
} catch (error) {
if (!onReadError) throw error;
onReadError(file, error);
continue;
}
const dir = path.dirname(file); const dir = path.dirname(file);
const imports = new Set(); const imports = new Set();
+32
View File
@@ -2648,6 +2648,21 @@ describe('walkDir', () => {
expect(walkDir('/nonexistent/path/12345')).toHaveLength(0); expect(walkDir('/nonexistent/path/12345')).toHaveLength(0);
}); });
test('reports directory traversal errors to an optional handler', () => {
const parent = fs.mkdtempSync(path.join(os.tmpdir(), 'impeccable-walk-error-'));
try {
const missing = path.join(parent, 'missing');
const errors = [];
expect(walkDir(missing, (dir, error) => {
errors.push({ dir, code: error.code });
})).toHaveLength(0);
expect(errors).toEqual([{ dir: missing, code: 'ENOENT' }]);
} finally {
fs.rmSync(parent, { recursive: true, force: true });
}
});
// Issue #303: when impeccable (or any agent tool) is installed into a // Issue #303: when impeccable (or any agent tool) is installed into a
// project's .claude/.cursor/etc. tree, a root scan descended into the // project's .claude/.cursor/etc. tree, a root scan descended into the
// vendored skill code and reported the detector's own example strings as // vendored skill code and reported the detector's own example strings as
@@ -3684,6 +3699,23 @@ describe('buildImportGraph', () => {
expect(imp).toContain(MF); expect(imp).toContain(MF);
} }
}); });
test('reports unreadable files and continues building the graph', async () => {
await withStaticFixture({
'readable.css': '@import "./missing.css";\n',
}, ({ dir }) => {
const readable = path.join(dir, 'readable.css');
const missing = path.join(dir, 'missing.css');
const errors = [];
const graph = buildImportGraph([missing, readable], (file, error) => {
errors.push({ file, code: error.code });
});
expect(errors).toEqual([{ file: missing, code: 'ENOENT' }]);
expect(graph.get(readable)).toEqual(new Set([missing]));
expect(graph.has(missing)).toBe(false);
});
});
}); });
describe('resolveImport', () => { describe('resolveImport', () => {
+167
View File
@@ -1,7 +1,14 @@
import { describe, test, expect, afterEach } from 'bun:test'; import { describe, test, expect, afterEach } from 'bun:test';
import { spawnSync } from 'node:child_process';
import fs from 'node:fs';
import http from 'node:http'; import http from 'node:http';
import os from 'node:os';
import path from 'node:path';
import { fileURLToPath } from 'node:url';
import { launchBrowser, detectUrl, splitScanUrl } from '../cli/engine/engines/browser/detect-url.mjs'; import { launchBrowser, detectUrl, splitScanUrl } from '../cli/engine/engines/browser/detect-url.mjs';
const ROOT = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '..');
// launchBrowser prefers the system-installed Chrome on Windows to dodge the // launchBrowser prefers the system-installed Chrome on Windows to dodge the
// bundled-Chrome GPU crash-loop (issue #372), and keeps the pinned bundled // bundled-Chrome GPU crash-loop (issue #372), and keeps the pinned bundled
// build everywhere else. The function takes the puppeteer module as a // build everywhere else. The function takes the puppeteer module as a
@@ -38,6 +45,50 @@ function makePuppeteer({ failChannel = false } = {}) {
}; };
} }
function runWithoutPuppeteer(args, files = {}, { nodeArgs = [] } = {}) {
const isolatedRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'impeccable-no-puppeteer-'));
try {
const cliRoot = path.join(isolatedRoot, 'cli');
fs.mkdirSync(cliRoot, { recursive: true });
fs.cpSync(path.join(ROOT, 'cli', 'engine'), path.join(cliRoot, 'engine'), { recursive: true });
fs.cpSync(path.join(ROOT, 'cli', 'lib'), path.join(cliRoot, 'lib'), { recursive: true });
for (const [relativePath, contents] of Object.entries(files)) {
const filePath = path.join(isolatedRoot, relativePath);
fs.mkdirSync(path.dirname(filePath), { recursive: true });
fs.writeFileSync(filePath, contents);
}
return spawnSync(
'node',
[...nodeArgs, path.join(cliRoot, 'engine', 'detect-antipatterns.mjs'), '--json', ...args],
{ cwd: isolatedRoot, encoding: 'utf8' },
);
} finally {
fs.rmSync(isolatedRoot, { recursive: true, force: true });
}
}
const DENY_FS_PRELOAD = `
const fs = require('node:fs');
const originalReadFileSync = fs.readFileSync;
const originalReaddirSync = fs.readdirSync;
fs.readFileSync = function (file, ...args) {
if (String(file).endsWith('unreadable.css')) {
const error = new Error('simulated EACCES');
error.code = 'EACCES';
throw error;
}
return originalReadFileSync.call(this, file, ...args);
};
fs.readdirSync = function (dir, ...args) {
if (String(dir).endsWith('unreadable-dir')) {
const error = new Error('simulated directory EACCES');
error.code = 'EACCES';
throw error;
}
return originalReaddirSync.call(this, dir, ...args);
};
`;
describe('launchBrowser', () => { describe('launchBrowser', () => {
test('Windows: prefers system Chrome via channel:chrome', async () => { test('Windows: prefers system Chrome via channel:chrome', async () => {
setPlatform('win32'); setPlatform('win32');
@@ -81,6 +132,122 @@ describe('launchBrowser', () => {
}); });
}); });
describe('detect CLI browser failures', () => {
test('exits 1 with valid empty JSON when Puppeteer is unavailable', () => {
const result = runWithoutPuppeteer(['https://example.com']);
expect(result.status).toBe(1);
expect(result.stdout).toBe('[]\n');
expect(result.stderr).toContain('puppeteer is required for URL scanning');
});
test('reports a shared multi-URL setup failure once and exits 1', () => {
const result = runWithoutPuppeteer(['https://example.com', 'https://example.org']);
expect(result.status).toBe(1);
expect(result.stdout).toBe('[]\n');
expect(result.stderr.match(/puppeteer is required for URL scanning/g)).toHaveLength(1);
});
test('operational failure takes precedence over findings from another target', () => {
const result = runWithoutPuppeteer(
['https://example.com', 'page.css'],
{ 'page.css': '.hero { animation: bounce 1s linear infinite; }\n' },
);
const findings = JSON.parse(result.stdout);
expect(result.status).toBe(1);
expect(findings.some(finding => finding.antipattern === 'bounce-easing')).toBe(true);
expect(result.stderr).toContain('puppeteer is required for URL scanning');
});
test('exits 1 when an explicitly requested local target cannot be accessed', () => {
const result = runWithoutPuppeteer(['missing.css']);
expect(result.status).toBe(1);
expect(result.stdout).toBe('[]\n');
expect(result.stderr).toContain('Warning: cannot access missing.css');
});
test('missing local target takes precedence over findings from another target', () => {
const result = runWithoutPuppeteer(
['missing.css', 'page.css'],
{ 'page.css': '.hero { animation: bounce 1s linear infinite; }\n' },
);
const findings = JSON.parse(result.stdout);
expect(result.status).toBe(1);
expect(findings.some(finding => finding.antipattern === 'bounce-easing')).toBe(true);
expect(result.stderr).toContain('Warning: cannot access missing.css');
});
test('unreadable local file exits 1 with valid empty JSON and no stack trace', () => {
const result = runWithoutPuppeteer(
['unreadable.css'],
{
'deny-fs.cjs': DENY_FS_PRELOAD,
'unreadable.css': '.hero { color: red; }\n',
},
{ nodeArgs: ['--require=./deny-fs.cjs'] },
);
expect(result.status).toBe(1);
expect(result.stdout).toBe('[]\n');
expect(result.stderr).toContain('Error: cannot scan unreadable.css: simulated EACCES');
expect(result.stderr).not.toContain('at detectLocalFile');
});
test('unreadable directory file preserves findings from readable siblings', () => {
const result = runWithoutPuppeteer(
['styles'],
{
'deny-fs.cjs': DENY_FS_PRELOAD,
'styles/unreadable.css': '.hero { color: red; }\n',
'styles/page.css': '.hero { animation: bounce 1s linear infinite; }\n',
},
{ nodeArgs: ['--require=./deny-fs.cjs'] },
);
const findings = JSON.parse(result.stdout);
expect(result.status).toBe(1);
expect(findings.some(finding => finding.antipattern === 'bounce-easing')).toBe(true);
expect(result.stderr.match(/simulated EACCES/g)).toHaveLength(1);
});
test('unreadable local directory exits 1 with valid empty JSON', () => {
const result = runWithoutPuppeteer(
['unreadable-dir'],
{
'deny-fs.cjs': DENY_FS_PRELOAD,
'unreadable-dir/page.css': '.hero { color: red; }\n',
},
{ nodeArgs: ['--require=./deny-fs.cjs'] },
);
expect(result.status).toBe(1);
expect(result.stdout).toBe('[]\n');
expect(result.stderr).toContain('Error: cannot scan');
expect(result.stderr).toContain('unreadable-dir: simulated directory EACCES');
});
test('unreadable nested directory preserves findings from readable siblings', () => {
const result = runWithoutPuppeteer(
['project'],
{
'deny-fs.cjs': DENY_FS_PRELOAD,
'project/unreadable-dir/hidden.css': '.hero { color: red; }\n',
'project/page.css': '.hero { animation: bounce 1s linear infinite; }\n',
},
{ nodeArgs: ['--require=./deny-fs.cjs'] },
);
const findings = JSON.parse(result.stdout);
expect(result.status).toBe(1);
expect(findings.some(finding => finding.antipattern === 'bounce-easing')).toBe(true);
expect(result.stderr.match(/simulated directory EACCES/g)).toHaveLength(1);
});
});
describe('splitScanUrl', () => { describe('splitScanUrl', () => {
test('strips http(s) userinfo and returns credentials', () => { test('strips http(s) userinfo and returns credentials', () => {
expect(splitScanUrl('https://user:pass@example.com')).toEqual({ expect(splitScanUrl('https://user:pass@example.com')).toEqual({