mirror of
https://github.com/pbakaus/impeccable.git
synced 2026-09-11 21:57:14 +03:00
* Add inline, in-file ignore comments for the detector (issue #283) Complement config ignores with eslint-disable-style waivers that live where they apply and travel with the file when it leaves the repo. The motivating case is a generated/exported standalone document that legitimately uses a first-party brand typeface (on the overused-font list) and is later scanned without .impeccable/config.json present. Marker is comment-syntax-agnostic (works in //, /* */, <!-- -->, #, {/* */}): impeccable-disable <rule>[, <rule>...] [-- reason | : reason] whole file impeccable-disable-line <rule>... same line impeccable-disable-next-line <rule>... next line Bare directive or * means every rule; reason is optional and discarded at scan time. Behavior is suppression, for parity with config ignores. Implementation: - New pure module cli/engine/shared/inline-ignores.mjs (parser + filter, no Node deps). Static-HTML findings have no line number, so only whole-file directives apply there -- exactly the standalone-document case; the regex/text engine additionally honors the line-scoped forms. - Wired into detectText and detectHtml, gated by options.inlineIgnores. - detect CLI applies inline ignores by default; --no-inline-ignores skips just them, --no-config skips config and inline ignores together. Docs: config.md (new section), detector.md, README. skill/reference/hooks.md reversed its prior "inline comments are not supported" guidance and now points the agent to inline waivers for the travels-with-the-file case. Changelog 3.x. Tests: tests/inline-ignores.test.mjs (parser units, detectText/detectHtml integration, CLI end-to-end), registered in the detector suite. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Reconcile design hook wording with inline ignores Two hook-side fixes prompted by review of the new inline-ignore feature: 1. Clean-ack steer line. The old line ("Keep typography hierarchy, spacing rhythm, and color contrast intentional on the next change.") read as an odd non-sequitur after "No anti-patterns." Reworded the whole clean ack to say what it means: a clean scan only clears the deterministic rule set, not overall design quality, so keep following the design system and skill guidance. Now: "Design hook scanned X. No deterministic design-quality issues found. That does not mean the design is good: keep following the project design system and the impeccable skill guidance." 2. Directive footer. It still told the agent "Do not add source comments such as `impeccable: ignore`; those pollute the code and do not suppress hook findings." That is now misleading: the hook runs the same detector engine as the CLI, which honors inline `impeccable-disable` waivers, so they DO suppress hook findings (consistent with config ignores, which filterFindings already honors). Reworded to: don't silence a real finding to skip fixing it; suppress only after the user confirms intent; prefer a config ignore, and reach for an inline `impeccable-disable <rule>` comment only when the waiver must travel with a file that leaves the repo. Added a hook test asserting an inline `impeccable-disable-line` comment makes the hook scan the file clean (locks in the cross-cutting behavior), and updated the clean-ack / footer assertions to the new wording. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Address review on inline-ignores parser - Case-insensitive fast-path bail-out (Cursor): the cheap substring guard was lowercase-only while DIRECTIVE_RE has the `i` flag, so a mixed-case marker like `Impeccable-Disable` skipped parsing entirely and never suppressed. Switched the guard to `/impeccable-disable/i.test(...)`. Added a regression test. - Removed the unreachable `-->` branch from TRAILING_CLOSER_RE (Greptile): `--+>` already matches `-->` and any longer dash run. - Replaced the always-truthy lazy-match + `if (sep)` reason strip with an explicit first-separator slice (Greptile): clearer and drops the dead branch. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Align inline-ignore line numbering with the detector (CRLF/CR endings) parseInlineIgnores split lines with /\r\n|\r|\n/, but detectText numbers lines with split('\n'). On classic `\r`-only endings the two diverged, so a disable-line / disable-next-line directive could key a different line than the finding it should waive (Cursor review). Split on '\n' only, matching the detector exactly; the directive regex already excludes '\r', so a trailing '\r' on CRLF files is never captured into the rule list. Added a CRLF regression test through the real detectText. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 4.8
parent
68a15b6be4
commit
776c019041
@@ -305,6 +305,8 @@ The detector catches 44 deterministic issues across AI slop (side-tab borders, p
|
||||
|
||||
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.
|
||||
|
||||
For a waiver that should travel with one file instead of the repo config, add an inline comment in the file: `<!-- impeccable-disable overused-font: exported brand doc -->`. The marker works in any comment syntax, scopes to the whole file (or one line with `impeccable-disable-line` / `impeccable-disable-next-line`), and is bypassed by `--no-inline-ignores` or `--no-config`.
|
||||
|
||||
Full detector docs: [impeccable.style/docs/detector](https://impeccable.style/docs/detector).
|
||||
|
||||
## Supported Tools
|
||||
|
||||
+17
-2
@@ -93,7 +93,9 @@ Options:
|
||||
--quiet In text mode, only print the final findings count
|
||||
--gpt Also report GPT-specific provider tells (off by default)
|
||||
--gemini Also report Gemini-specific provider tells (off by default)
|
||||
--no-config Do not apply project config, detector ignores, or DESIGN.md
|
||||
--no-config Do not apply project config, detector ignores, inline
|
||||
ignore comments, or DESIGN.md
|
||||
--no-inline-ignores Do not honor in-file impeccable-disable* ignore comments
|
||||
--no-design-system Do not load local DESIGN.md / .impeccable/design.json context
|
||||
--help Show this help message
|
||||
|
||||
@@ -102,6 +104,14 @@ Project config:
|
||||
settings: detector.ignoreRules, detector.ignoreFiles, detector.ignoreValues,
|
||||
and detector.designSystem.enabled.
|
||||
|
||||
Inline ignores:
|
||||
In-file comments waive a finding where it lives and travel with the file:
|
||||
<!-- impeccable-disable overused-font -- exported brand doc -->
|
||||
.brand { font-family: Inter } /* impeccable-disable-line overused-font */
|
||||
// impeccable-disable-next-line bounce-easing: intentional bounce
|
||||
impeccable-disable applies to the whole file; -line / -next-line are scoped.
|
||||
List one or more rule ids (comma-separated), or omit them / use * for all.
|
||||
|
||||
Detection modes:
|
||||
HTML files Static HTML/CSS analysis (default, catches linked CSS)
|
||||
Non-HTML files Regex pattern matching (CSS, JSX, TSX, etc.)
|
||||
@@ -143,7 +153,12 @@ async function detectCli() {
|
||||
if (args.includes('--gemini')) providers.push('gemini');
|
||||
const designSystemEnabled = configEnabled && !args.includes('--no-design-system') && detectionConfig.designSystem?.enabled !== false;
|
||||
const designSystem = designSystemEnabled ? loadDesignSystemForCwd(process.cwd()) : null;
|
||||
const scanOptions = designSystem ? { providers, designSystem } : { providers };
|
||||
// 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 = { providers, inlineIgnores: inlineIgnoresEnabled };
|
||||
if (designSystem) scanOptions.designSystem = designSystem;
|
||||
const targets = args.filter(a => !a.startsWith('--'));
|
||||
|
||||
if (helpMode) { printUsage(); process.exit(0); }
|
||||
|
||||
@@ -2,6 +2,7 @@ import { GENERIC_FONTS } from '../../shared/constants.mjs';
|
||||
import { isNeutralColor } from '../../shared/color.mjs';
|
||||
import { checkSourceDesignSystem } from '../../design-system.mjs';
|
||||
import { isFullPage } from '../../shared/page.mjs';
|
||||
import { applyInlineIgnores } from '../../shared/inline-ignores.mjs';
|
||||
import { finding } from '../../findings.mjs';
|
||||
import { filterByProviders } from '../../registry/antipatterns.mjs';
|
||||
import { profileFindings, profileStep } from '../../profile/profiler.mjs';
|
||||
@@ -549,7 +550,10 @@ function detectText(content, filePath, options = {}) {
|
||||
}
|
||||
}
|
||||
|
||||
return filterByProviders(deduped, options?.providers);
|
||||
const byProvider = filterByProviders(deduped, options?.providers);
|
||||
// Inline `impeccable-disable*` waivers travel with the file; honor them unless
|
||||
// explicitly bypassed (`--no-config` / `--no-inline-ignores`).
|
||||
return options?.inlineIgnores === false ? byProvider : applyInlineIgnores(byProvider, content);
|
||||
}
|
||||
|
||||
export {
|
||||
|
||||
@@ -8,6 +8,7 @@ import {
|
||||
mergeDesignSystemFindings,
|
||||
} from '../../design-system.mjs';
|
||||
import { isFullPage } from '../../shared/page.mjs';
|
||||
import { applyInlineIgnores } from '../../shared/inline-ignores.mjs';
|
||||
import { finding } from '../../findings.mjs';
|
||||
import { profileFindings, profileStep, profileStepAsync } from '../../profile/profiler.mjs';
|
||||
import {
|
||||
@@ -223,7 +224,11 @@ async function detectHtml(filePath, options = {}) {
|
||||
}
|
||||
}
|
||||
|
||||
return filterByProviders(findings, options.providers);
|
||||
const byProvider = filterByProviders(findings, options.providers);
|
||||
// Static-HTML findings carry no line number, so only whole-file
|
||||
// `impeccable-disable` directives apply here — exactly the standalone-document
|
||||
// waiver this primitive targets. Bypassed by `--no-config` / `--no-inline-ignores`.
|
||||
return options?.inlineIgnores === false ? byProvider : applyInlineIgnores(byProvider, html);
|
||||
}
|
||||
|
||||
export { checkStaticPageTypography, STATIC_ELEMENT_RULES, detectHtml };
|
||||
|
||||
@@ -0,0 +1,148 @@
|
||||
/**
|
||||
* Inline, in-file ignore directives — eslint-disable-style waivers that live at
|
||||
* the point they apply and travel with the artifact instead of (or alongside)
|
||||
* an ignore in `.impeccable/config.json`.
|
||||
*
|
||||
* A config ignore is the right default for repo-wide policy. This complements it
|
||||
* for the one case config can't cover: a waiver that belongs to a single file and
|
||||
* needs to follow that file when it leaves the repo — a generated/exported
|
||||
* standalone document, an emailed HTML file, a snippet scanned out of context.
|
||||
*
|
||||
* Comment-syntax-agnostic: the directive is a raw token matched anywhere on a
|
||||
* line, so the same marker works across every comment style impeccable scans —
|
||||
* `//`, `/* *\/`, `<!-- -->`, `#`, `{/* *\/}`, `{# #}`. Trailing comment closers
|
||||
* are stripped before the rule list is parsed.
|
||||
*
|
||||
* Syntax (reason optional; eslint `--` or biome `:` separator):
|
||||
*
|
||||
* impeccable-disable <rule>[, <rule>...] [-- reason] whole file
|
||||
* impeccable-disable-line <rule>... [-- reason] the same line
|
||||
* impeccable-disable-next-line <rule>... [-- reason] the following line
|
||||
* impeccable-disable bare / `*` = every rule
|
||||
*
|
||||
* Examples:
|
||||
*
|
||||
* <!-- impeccable-disable overused-font -- exported brand doc, font is first-party -->
|
||||
* .brand { font-family: Inter; } /* impeccable-disable-line overused-font *\/
|
||||
* // impeccable-disable-next-line bounce-easing: intentional playful affordance
|
||||
*
|
||||
* Behavior is suppression, for parity with config ignores: a matched directive
|
||||
* drops the finding. The inline reason is self-documenting in the diff; it is not
|
||||
* required and is discarded at scan time (only used here to keep reason words out
|
||||
* of the parsed rule list).
|
||||
*/
|
||||
|
||||
const DIRECTIVE_RE = /impeccable-(disable-next-line|disable-line|disable)\b[ \t]*([^\n\r]*)/gi;
|
||||
|
||||
// Trailing comment closers, so `*/`, `*/}`, `-->`, `*}`, `#}`, `%>`, `}}` don't
|
||||
// leak into the rule list. Anchored to end-of-line; the leading `\s*` mops up the
|
||||
// space before the closer. `--+>` covers `-->` and any longer dash run.
|
||||
const TRAILING_CLOSER_RE = /\s*(?:\*\/\}?|--+>|\*\}|#\}|%>|\}\})\s*$/;
|
||||
|
||||
function normalizeRule(token) {
|
||||
return String(token || '').trim().toLowerCase();
|
||||
}
|
||||
|
||||
// Split the directive remainder into rule tokens, dropping any human reason that
|
||||
// follows an eslint-style `--` or biome-style `:` separator. Rule ids only ever
|
||||
// contain single hyphens (`overused-font`, `bounce-easing`), so `--` and `:`
|
||||
// are unambiguous separators.
|
||||
function parseRuleList(remainder) {
|
||||
let text = String(remainder || '').replace(TRAILING_CLOSER_RE, '').trim();
|
||||
// Cut off a human reason at the first `--` (eslint) or `:` (biome) separator.
|
||||
const reasonSep = text.match(/\s*(?:--+|:)\s*/);
|
||||
if (reasonSep) text = text.slice(0, reasonSep.index);
|
||||
const tokens = text.split(/[\s,]+/).map(normalizeRule).filter(Boolean);
|
||||
if (tokens.length === 0 || tokens.includes('*')) return ['*'];
|
||||
return tokens;
|
||||
}
|
||||
|
||||
function addRules(set, rules) {
|
||||
for (const rule of rules) set.add(rule);
|
||||
}
|
||||
|
||||
function getSet(map, key) {
|
||||
let set = map.get(key);
|
||||
if (!set) {
|
||||
set = new Set();
|
||||
map.set(key, set);
|
||||
}
|
||||
return set;
|
||||
}
|
||||
|
||||
/**
|
||||
* Parse every inline ignore directive in a file's raw text.
|
||||
*
|
||||
* Returns sets keyed by the 1-based line the directive *targets* so matching is a
|
||||
* direct lookup:
|
||||
* - file: rules disabled for the whole file
|
||||
* - line: line -> rules disabled on that exact line (disable-line)
|
||||
* - nextLine: line -> rules disabled on that line (disable-next-line on line-1)
|
||||
*
|
||||
* `*` in any set means "every rule".
|
||||
*/
|
||||
function parseInlineIgnores(content) {
|
||||
const result = { file: new Set(), line: new Map(), nextLine: new Map() };
|
||||
const text = typeof content === 'string' ? content : '';
|
||||
// Cheap bail-out: the substring must be present for any directive to exist.
|
||||
// Case-insensitive to match DIRECTIVE_RE's `i` flag (e.g. `Impeccable-Disable`).
|
||||
if (!/impeccable-disable/i.test(text)) return result;
|
||||
|
||||
// Split on `\n` only, exactly as detectText numbers lines, so directive line
|
||||
// keys line up with finding `line` values (incl. on `\r`-only line endings).
|
||||
// The directive regex excludes `\r`, so a trailing `\r` on `\r\n` files is
|
||||
// never captured into the rule list.
|
||||
const lines = text.split('\n');
|
||||
for (let i = 0; i < lines.length; i++) {
|
||||
DIRECTIVE_RE.lastIndex = 0;
|
||||
let m;
|
||||
while ((m = DIRECTIVE_RE.exec(lines[i])) !== null) {
|
||||
const variant = m[1].toLowerCase();
|
||||
const rules = parseRuleList(m[2]);
|
||||
if (variant === 'disable') {
|
||||
addRules(result.file, rules);
|
||||
} else if (variant === 'disable-line') {
|
||||
addRules(getSet(result.line, i + 1), rules);
|
||||
} else {
|
||||
// disable-next-line on line i+1 targets line i+2.
|
||||
addRules(getSet(result.nextLine, i + 2), rules);
|
||||
}
|
||||
}
|
||||
}
|
||||
return result;
|
||||
}
|
||||
|
||||
function setMatches(set, rule) {
|
||||
return Boolean(set) && (set.has('*') || set.has(rule));
|
||||
}
|
||||
|
||||
function isInlineIgnored(finding, directives) {
|
||||
const rule = normalizeRule(finding && finding.antipattern);
|
||||
if (!rule) return false;
|
||||
if (setMatches(directives.file, rule)) return true;
|
||||
const line = Number(finding && finding.line) || 0;
|
||||
if (line > 0) {
|
||||
if (setMatches(directives.line.get(line), rule)) return true;
|
||||
if (setMatches(directives.nextLine.get(line), rule)) return true;
|
||||
}
|
||||
return false;
|
||||
}
|
||||
|
||||
function hasDirectives(directives) {
|
||||
return directives.file.size > 0 || directives.line.size > 0 || directives.nextLine.size > 0;
|
||||
}
|
||||
|
||||
/**
|
||||
* Drop findings waived by an inline directive in the same file's source text.
|
||||
* Findings without a usable line number (e.g. static-HTML page-level findings)
|
||||
* are only matched by whole-file directives — which is the standalone-document
|
||||
* case this primitive exists for.
|
||||
*/
|
||||
function applyInlineIgnores(findings, content) {
|
||||
if (!Array.isArray(findings) || findings.length === 0) return findings;
|
||||
const directives = parseInlineIgnores(content);
|
||||
if (!hasDirectives(directives)) return findings;
|
||||
return findings.filter((finding) => !isInlineIgnored(finding, directives));
|
||||
}
|
||||
|
||||
export { parseInlineIgnores, applyInlineIgnores, isInlineIgnored };
|
||||
@@ -79,13 +79,14 @@ 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|extension-build|fixtures\/antipatterns)/,
|
||||
/^tests\/(detect-antipatterns|inline-ignores|extension-build|fixtures\/antipatterns)/,
|
||||
],
|
||||
commands: [
|
||||
{
|
||||
runner: 'bun',
|
||||
files: [
|
||||
'tests/detect-antipatterns.test.js',
|
||||
'tests/inline-ignores.test.mjs',
|
||||
'tests/lib/detector-bundle.test.js',
|
||||
],
|
||||
},
|
||||
|
||||
@@ -70,6 +70,28 @@ npx impeccable ignores add-value design-system-color "*" --file "src/demo.css"
|
||||
|
||||
That keeps one intentionally experimental file from teaching the whole project that every undocumented color is acceptable.
|
||||
|
||||
## Inline ignore comments
|
||||
|
||||
Config ignores live in `.impeccable/config.json`, which is the right home for repo-wide policy. They do not follow a file out of the repo, though. When a waiver belongs to one file and needs to travel with it (a generated or exported standalone document, an emailed HTML file, a snippet scanned out of context), put the waiver in the file itself:
|
||||
|
||||
```html
|
||||
<!-- impeccable-disable overused-font: exported brand doc, font is first-party -->
|
||||
```
|
||||
|
||||
The directive is comment-syntax-agnostic, so the same marker works in `//`, `/* */`, `<!-- -->`, `#`, and `{/* */}` comments across HTML, CSS, JSX, TSX, Vue, and Svelte. Three scopes are available:
|
||||
|
||||
```css
|
||||
/* impeccable-disable overused-font */ /* whole file */
|
||||
.brand { font-family: Inter } /* impeccable-disable-line overused-font */
|
||||
/* impeccable-disable-next-line bounce-easing */
|
||||
```
|
||||
|
||||
List one or more rule ids, comma-separated, or omit them (or use `*`) for every rule. A reason after `:` or `--` is optional and recommended; it is for the diff, and the scanner discards it. Like config ignores, a matched directive suppresses the finding.
|
||||
|
||||
Static HTML findings have no line number, so only whole-file `impeccable-disable` applies to them. That is the standalone-document case this exists for. The line-scoped forms apply to CSS, JSX, TSX, Vue, and Svelte, where findings carry a line.
|
||||
|
||||
Inline directives apply by default. `--no-inline-ignores` turns them off for one run while keeping config ignores; `--no-config` turns off config and inline ignores together.
|
||||
|
||||
## Details when the default path is not enough
|
||||
|
||||
<details class="docs-prose-details">
|
||||
|
||||
@@ -78,7 +78,13 @@ npx impeccable ignores add-value overused-font Inter --reason "Brand font"
|
||||
npx impeccable ignores add-file "src/legacy/**"
|
||||
```
|
||||
|
||||
Use [Config and ignores](/docs/config) for the full ignore workflow.
|
||||
For a waiver that should travel with one file instead of living in the repo config, drop an inline comment in the file itself:
|
||||
|
||||
```html
|
||||
<!-- impeccable-disable overused-font: exported brand doc -->
|
||||
```
|
||||
|
||||
Use [Config and ignores](/docs/config) for the full ignore workflow, including the line-scoped `impeccable-disable-line` and `impeccable-disable-next-line` forms.
|
||||
|
||||
## Details when the default path is not enough
|
||||
|
||||
@@ -96,6 +102,7 @@ Use [Config and ignores](/docs/config) for the full ignore workflow.
|
||||
<p>By default, <code>detect</code> reads <code>.impeccable/config.json</code> and <code>.impeccable/config.local.json</code>.</p>
|
||||
<p>It respects <code>detector.ignoreRules</code>, <code>detector.ignoreFiles</code>, <code>detector.ignoreValues</code>, and <code>detector.designSystem.enabled</code>.</p>
|
||||
<p>It does not respect <code>hook.enabled</code>; manual scans still run when the automatic hook is disabled.</p>
|
||||
<p>In-file <code>impeccable-disable*</code> comments are honored too, so a waiver can travel with a file. <code>--no-inline-ignores</code> skips just those; <code>--no-config</code> skips config and inline ignores together.</p>
|
||||
<p>Use <code>--no-config</code> only when you want a raw detector run with no project config, no detector ignores, and no <code>DESIGN.md</code> context.</p>
|
||||
</div>
|
||||
</details>
|
||||
|
||||
@@ -29,6 +29,7 @@ import '../styles/changelog-faq-kinpaku.css';
|
||||
<ul class="cf-items">
|
||||
<li><strong>Design hooks for GitHub Copilot.</strong> Installing the skill adds a <code>.github/hooks/impeccable.json</code> hook that runs the detector after Copilot edits a UI file and feeds the findings back as a focused design reminder. It covers both the Copilot CLI and the cloud agent, and recognizes every edit path Copilot uses, including <code>apply_patch</code>.</li>
|
||||
<li><strong>Monorepo-aware context.</strong> Open the repo root and Impeccable resolves <code>PRODUCT.md</code> and <code>DESIGN.md</code> per app: each child app uses its own context files and falls back to the root files for anything it does not define. <code>/impeccable live</code> detects multiple apps, asks which one to work on, and then runs and stores live state inside that app.</li>
|
||||
<li><strong>Inline ignore comments.</strong> Waive a detector finding with an in-file comment that travels with the file, for exported or standalone documents where <code>.impeccable/config.json</code> is not present. <code>impeccable-disable <rule></code> covers the whole file; <code>impeccable-disable-line</code> and <code>impeccable-disable-next-line</code> scope to one line. The marker works in any comment syntax, takes an optional reason, and is bypassed with <code>--no-inline-ignores</code>.</li>
|
||||
</ul>
|
||||
</article>
|
||||
|
||||
|
||||
@@ -51,7 +51,7 @@ Prefer the narrowest exception:
|
||||
- For value-specific findings such as `overused-font` and `bounce-easing`, use `ignore-value` when the user confirms the specific value. Do not use `ignore-rule overused-font` for a specific font.
|
||||
- If the finding has no value-specific command, such as `side-tab`, prefer `ignore-file <path>` for the current file.
|
||||
- Use `ignore-rule <id>` only when the user asks to suppress that whole rule across the project. For broad overused-font suppression, use `ignore-rule overused-font --all-values` only when the user asks to ignore overused fonts generally.
|
||||
- Do not add source comments such as `impeccable: ignore`; inline comments pollute code and are not a supported suppression mechanism.
|
||||
- Prefer config ignores (the commands above) by default; they keep suppressions in one reviewable place. Reach for an inline comment only when the waiver must travel with a single file that leaves the repo (a generated/exported standalone document, an emailed HTML file). The supported marker is `impeccable-disable <rule>` (whole file) or `impeccable-disable-line` / `impeccable-disable-next-line` (one line), in any comment syntax, with an optional reason after `:` or `--`. The detector honors it by default; `--no-inline-ignores` or `--no-config` bypasses it.
|
||||
|
||||
Example value-specific exception:
|
||||
|
||||
|
||||
@@ -1301,12 +1301,12 @@ export function setDetectorForTesting(impl) {
|
||||
// session" so the model knows it's a re-mind, not a new finding.
|
||||
// ────────────────────────────────────────────────────────────────────────
|
||||
|
||||
const STEER_LINE = 'Keep typography hierarchy, spacing rhythm, and color contrast intentional on the next change.';
|
||||
const STEER_LINE = 'That does not mean the design is good: keep following the project design system and the impeccable skill guidance.';
|
||||
|
||||
export function renderCleanAck(filePath, opts = {}) {
|
||||
const cwd = opts.cwd || process.cwd();
|
||||
const display = relativize(filePath, cwd);
|
||||
return `${ENVELOPE_PREFIX} Design hook scanned ${display}. No anti-patterns. ${STEER_LINE}`;
|
||||
return `${ENVELOPE_PREFIX} Design hook scanned ${display}. No deterministic design-quality issues found. ${STEER_LINE}`;
|
||||
}
|
||||
|
||||
export function renderPendingAck(filePath, knownFindings, opts = {}) {
|
||||
@@ -1362,7 +1362,7 @@ function directiveFooter(display, opts = {}) {
|
||||
'',
|
||||
'Use context judgment before editing. A finding is not automatically a defect; literal or domain-appropriate motion, intentional demos or fixtures, documentation of bad design, and user-confirmed choices can be valid as-is.',
|
||||
'',
|
||||
`Do not change intentional design just to satisfy the hook. Do not add source comments such as \`impeccable: ignore\`; those pollute the code and do not suppress hook findings. Persist hook ignores only after the user explicitly confirms the finding is intentional. Prefer the narrowest persisted exception: run the exact \`/impeccable hooks ignore-value ... --shared\` command shown next to a value-specific finding. For \`overused-font\`, use \`ignore-value\` for a specific font and use \`/impeccable hooks ignore-rule overused-font --all-values\` only when the user asks to ignore overused fonts generally. For file-specific findings without an ignore-value command, ${fileIgnoreGuidance}; use \`/impeccable hooks ignore-rule <id>\` only when the user asks to suppress the whole non-value-specific rule. Run /impeccable audit for the full pass.`,
|
||||
`Do not change intentional design just to satisfy the hook, and do not silence a real finding with an inline ignore comment to skip fixing it. Suppress a finding only after the user explicitly confirms it is intentional. Prefer a config ignore (one reviewable place, the commands below); reach for an inline \`impeccable-disable <rule>\` comment only when the waiver must travel with a file that leaves the repo, such as an exported or standalone document. Prefer the narrowest persisted exception: run the exact \`/impeccable hooks ignore-value ... --shared\` command shown next to a value-specific finding. For \`overused-font\`, use \`ignore-value\` for a specific font and use \`/impeccable hooks ignore-rule overused-font --all-values\` only when the user asks to ignore overused fonts generally. For file-specific findings without an ignore-value command, ${fileIgnoreGuidance}; use \`/impeccable hooks ignore-rule <id>\` only when the user asks to suppress the whole non-value-specific rule. Run /impeccable audit for the full pass.`,
|
||||
].join('\n');
|
||||
}
|
||||
|
||||
|
||||
+31
-11
@@ -658,8 +658,9 @@ describe('renderTemplate()', () => {
|
||||
assert.match(text, /not automatically a defect/);
|
||||
assert.match(text, /literal or domain-appropriate motion/);
|
||||
assert.match(text, /Do not change intentional design just to satisfy the hook/);
|
||||
assert.match(text, /Persist hook ignores only after the user explicitly confirms/);
|
||||
assert.match(text, /Do not add source comments such as `impeccable: ignore`/);
|
||||
assert.match(text, /Suppress a finding only after the user explicitly confirms it is intentional/);
|
||||
assert.match(text, /do not silence a real finding with an inline ignore comment/);
|
||||
assert.match(text, /inline `impeccable-disable <rule>` comment only when the waiver must travel with a file/);
|
||||
assert.match(text, /ignore-value \.\.\. --shared/);
|
||||
assert.match(text, /ignore-rule overused-font --all-values/);
|
||||
assert.match(text, /\/impeccable hooks ignore-file Card\.tsx/);
|
||||
@@ -937,8 +938,8 @@ rounded:
|
||||
const r = await runHook({ stdinJson: JSON.stringify(eventFor(file)), env: {}, cwd, detector: det });
|
||||
assert.equal(r.exitCode, 0);
|
||||
assert.ok(r.stdout.includes(ENVELOPE_PREFIX));
|
||||
assert.match(r.stdout, /No anti-patterns/);
|
||||
assert.match(r.stdout, /typography hierarchy, spacing rhythm, and color contrast/);
|
||||
assert.match(r.stdout, /No deterministic design-quality issues found/);
|
||||
assert.match(r.stdout, /keep following the project design system and the impeccable skill guidance/);
|
||||
assert.equal(r.audit.emitted, true);
|
||||
assert.equal(r.audit.kind, 'clean');
|
||||
});
|
||||
@@ -1080,7 +1081,7 @@ rounded:
|
||||
cwd,
|
||||
detector: det,
|
||||
});
|
||||
assert.match(withoutDesign.stdout, /No anti-patterns/);
|
||||
assert.match(withoutDesign.stdout, /No deterministic design-quality issues found/);
|
||||
assert.doesNotMatch(withoutDesign.stdout, /design-system-font/);
|
||||
|
||||
writeDesignMd();
|
||||
@@ -1110,7 +1111,7 @@ rounded:
|
||||
detector: designAwareDetector(),
|
||||
});
|
||||
|
||||
assert.match(r.stdout, /No anti-patterns/);
|
||||
assert.match(r.stdout, /No deterministic design-quality issues found/);
|
||||
assert.doesNotMatch(r.stdout, /design-system-font/);
|
||||
});
|
||||
|
||||
@@ -1133,7 +1134,7 @@ rounded:
|
||||
detector: designAwareDetector(),
|
||||
});
|
||||
|
||||
assert.match(r.stdout, /No anti-patterns/);
|
||||
assert.match(r.stdout, /No deterministic design-quality issues found/);
|
||||
assert.doesNotMatch(r.stdout, /design-system-font/);
|
||||
});
|
||||
|
||||
@@ -1153,7 +1154,7 @@ rounded:
|
||||
detector: det,
|
||||
});
|
||||
|
||||
assert.match(r.stdout, /No anti-patterns/);
|
||||
assert.match(r.stdout, /No deterministic design-quality issues found/);
|
||||
assert.match(r.stdout, /DESIGN\.md is newer than \.impeccable\/design\.json/);
|
||||
assert.match(r.stdout, /\/impeccable document/);
|
||||
});
|
||||
@@ -1288,10 +1289,29 @@ rounded:
|
||||
detector: { detectHtml, detectText },
|
||||
});
|
||||
assert.match(r.stdout, /Design hook findings requiring review/);
|
||||
assert.doesNotMatch(r.stdout, /No anti-patterns/);
|
||||
assert.doesNotMatch(r.stdout, /No deterministic design-quality issues found/);
|
||||
assert.ok(r.audit.findings > 0);
|
||||
});
|
||||
|
||||
it('honors an inline impeccable-disable comment so the hook scans the file clean', async () => {
|
||||
// The hook runs the same engine as `npx impeccable detect`, so an in-file
|
||||
// waiver suppresses hook findings exactly like a config ignore would.
|
||||
const flagged = writeFixture('src/Flagged.tsx', 'const css = "font-family: Inter";');
|
||||
const flaggedRun = await runHook({
|
||||
stdinJson: JSON.stringify(eventFor(flagged)), env: {}, cwd, detector: { detectHtml, detectText },
|
||||
});
|
||||
assert.match(flaggedRun.stdout, /Design hook findings requiring review/);
|
||||
assert.ok(flaggedRun.audit.findings > 0);
|
||||
|
||||
const waived = writeFixture('src/Waived.tsx',
|
||||
'const css = "font-family: Inter"; // impeccable-disable-line overused-font');
|
||||
const waivedRun = await runHook({
|
||||
stdinJson: JSON.stringify(eventFor(waived)), env: {}, cwd, detector: { detectHtml, detectText },
|
||||
});
|
||||
assert.match(waivedRun.stdout, /No deterministic design-quality issues found/);
|
||||
assert.equal(waivedRun.audit.findings, 0);
|
||||
});
|
||||
|
||||
it('malformed stdin → silent skip', async () => {
|
||||
const r = await runHook({ stdinJson: '{not json', env: {}, cwd });
|
||||
assert.equal(r.audit.skipped, 'stdin-malformed');
|
||||
@@ -1340,8 +1360,8 @@ describe('ALLOWED_EXTS', () => {
|
||||
describe('renderCleanAck() / renderPendingAck()', () => {
|
||||
it('renderCleanAck stays short and ends with the steer line', () => {
|
||||
const text = renderCleanAck('/x/src/App.jsx', { cwd: '/x' });
|
||||
assert.match(text, /^\[impeccable@1\] Design hook scanned src\/App\.jsx\. No anti-patterns\./);
|
||||
assert.match(text, /typography hierarchy, spacing rhythm, and color contrast/);
|
||||
assert.match(text, /^\[impeccable@1\] Design hook scanned src\/App\.jsx\. No deterministic design-quality issues found\./);
|
||||
assert.match(text, /keep following the project design system and the impeccable skill guidance/);
|
||||
// Budget guard: should fit comfortably under a single context-message
|
||||
// injection (~200 chars). Hard upper bound 240 chars.
|
||||
assert.ok(text.length < 240, `clean ack too long: ${text.length} chars`);
|
||||
|
||||
@@ -0,0 +1,210 @@
|
||||
import { describe, test, expect } from 'bun:test';
|
||||
import fs from 'node:fs';
|
||||
import os from 'node:os';
|
||||
import path from 'node:path';
|
||||
import { spawnSync } from 'node:child_process';
|
||||
|
||||
import {
|
||||
parseInlineIgnores,
|
||||
applyInlineIgnores,
|
||||
isInlineIgnored,
|
||||
} from '../cli/engine/shared/inline-ignores.mjs';
|
||||
import { detectText, detectHtml } from '../cli/engine/detect-antipatterns.mjs';
|
||||
|
||||
const CLI = path.resolve('cli/bin/cli.js');
|
||||
|
||||
function rules(finding) {
|
||||
return finding.antipattern;
|
||||
}
|
||||
|
||||
describe('parseInlineIgnores', () => {
|
||||
test('whole-file directive collects rules', () => {
|
||||
const d = parseInlineIgnores('/* impeccable-disable overused-font, bounce-easing */');
|
||||
expect([...d.file].sort()).toEqual(['bounce-easing', 'overused-font']);
|
||||
expect(d.line.size).toBe(0);
|
||||
expect(d.nextLine.size).toBe(0);
|
||||
});
|
||||
|
||||
test('bare directive and explicit * both mean every rule', () => {
|
||||
expect([...parseInlineIgnores('// impeccable-disable').file]).toEqual(['*']);
|
||||
expect([...parseInlineIgnores('// impeccable-disable *').file]).toEqual(['*']);
|
||||
});
|
||||
|
||||
test('disable-line targets its own line, disable-next-line targets the line below', () => {
|
||||
const content = [
|
||||
'a', // line 1
|
||||
'b /* impeccable-disable-line overused-font */', // line 2
|
||||
'// impeccable-disable-next-line side-tab', // line 3 -> targets line 4
|
||||
'd', // line 4
|
||||
].join('\n');
|
||||
const d = parseInlineIgnores(content);
|
||||
expect([...d.line.get(2)]).toEqual(['overused-font']);
|
||||
expect([...d.nextLine.get(4)]).toEqual(['side-tab']);
|
||||
});
|
||||
|
||||
test('strips eslint -- and biome : reasons from the rule list', () => {
|
||||
expect([...parseInlineIgnores('// impeccable-disable overused-font -- brand font, exported doc').file])
|
||||
.toEqual(['overused-font']);
|
||||
expect([...parseInlineIgnores('# impeccable-disable bounce-easing: intentional bounce').file])
|
||||
.toEqual(['bounce-easing']);
|
||||
});
|
||||
|
||||
test('strips trailing comment closers across syntaxes', () => {
|
||||
expect([...parseInlineIgnores('<!-- impeccable-disable overused-font -->').file]).toEqual(['overused-font']);
|
||||
expect([...parseInlineIgnores('{/* impeccable-disable overused-font */}').file]).toEqual(['overused-font']);
|
||||
expect([...parseInlineIgnores('{# impeccable-disable overused-font #}').file]).toEqual(['overused-font']);
|
||||
});
|
||||
|
||||
test('directive keyword is case-insensitive (fast-path matches the regex)', () => {
|
||||
expect([...parseInlineIgnores('// Impeccable-Disable overused-font').file]).toEqual(['overused-font']);
|
||||
expect([...parseInlineIgnores('/* IMPECCABLE-DISABLE-LINE side-tab */').line.get(1)]).toEqual(['side-tab']);
|
||||
});
|
||||
|
||||
test('no directive present is a cheap no-op', () => {
|
||||
const d = parseInlineIgnores('.a { color: red }');
|
||||
expect(d.file.size).toBe(0);
|
||||
expect(d.line.size).toBe(0);
|
||||
expect(d.nextLine.size).toBe(0);
|
||||
});
|
||||
});
|
||||
|
||||
describe('applyInlineIgnores / isInlineIgnored', () => {
|
||||
const findings = [
|
||||
{ antipattern: 'overused-font', line: 5 },
|
||||
{ antipattern: 'side-tab', line: 5 },
|
||||
{ antipattern: 'overused-font', line: 0 }, // no line (static-HTML shape)
|
||||
];
|
||||
|
||||
test('whole-file directive drops every matching finding regardless of line', () => {
|
||||
const out = applyInlineIgnores(findings, '/* impeccable-disable overused-font */');
|
||||
expect(out.map(rules)).toEqual(['side-tab']);
|
||||
});
|
||||
|
||||
test('* drops everything', () => {
|
||||
expect(applyInlineIgnores(findings, '// impeccable-disable *')).toEqual([]);
|
||||
});
|
||||
|
||||
test('line-scoped directive only affects the matching line and rule', () => {
|
||||
const content = ['', '', '', '', 'x /* impeccable-disable-line overused-font */'].join('\n');
|
||||
const out = applyInlineIgnores(findings, content);
|
||||
// the line-5 overused-font goes; side-tab on line 5 and the line-less one stay
|
||||
expect(out.map(rules).sort()).toEqual(['overused-font', 'side-tab']);
|
||||
expect(out.some((f) => f.antipattern === 'overused-font' && f.line === 5)).toBe(false);
|
||||
});
|
||||
|
||||
test('returns the input untouched when there are no directives', () => {
|
||||
const out = applyInlineIgnores(findings, '.a {}');
|
||||
expect(out).toBe(findings);
|
||||
});
|
||||
|
||||
test('isInlineIgnored never matches a line-scoped directive for a line-less finding', () => {
|
||||
const d = parseInlineIgnores('x /* impeccable-disable-line overused-font */');
|
||||
expect(isInlineIgnored({ antipattern: 'overused-font', line: 0 }, d)).toBe(false);
|
||||
});
|
||||
});
|
||||
|
||||
describe('detectText honors inline directives', () => {
|
||||
const opts = { providers: [] };
|
||||
|
||||
test('disable-line suppresses a same-line finding', () => {
|
||||
const flagged = detectText('.a { font-family: Inter; }', 'a.css', opts);
|
||||
expect(flagged.some((f) => f.antipattern === 'overused-font')).toBe(true);
|
||||
|
||||
const waived = detectText('.a { font-family: Inter; } /* impeccable-disable-line overused-font */', 'a.css', opts);
|
||||
expect(waived.some((f) => f.antipattern === 'overused-font')).toBe(false);
|
||||
});
|
||||
|
||||
test('disable-next-line suppresses the finding on the following line', () => {
|
||||
const content = '/* impeccable-disable-next-line overused-font */\n.a { font-family: Inter; }';
|
||||
const waived = detectText(content, 'a.css', opts);
|
||||
expect(waived.some((f) => f.antipattern === 'overused-font')).toBe(false);
|
||||
});
|
||||
|
||||
test('whole-file directive suppresses regardless of where the finding is', () => {
|
||||
const content = '/* impeccable-disable overused-font */\n.a {}\n.b { font-family: Inter; }';
|
||||
const waived = detectText(content, 'a.css', opts);
|
||||
expect(waived.some((f) => f.antipattern === 'overused-font')).toBe(false);
|
||||
});
|
||||
|
||||
test('inlineIgnores:false bypasses the directive', () => {
|
||||
const content = '.a { font-family: Inter; } /* impeccable-disable-line overused-font */';
|
||||
const raw = detectText(content, 'a.css', { providers: [], inlineIgnores: false });
|
||||
expect(raw.some((f) => f.antipattern === 'overused-font')).toBe(true);
|
||||
});
|
||||
|
||||
test('line keys align with detector line numbers on CRLF endings', () => {
|
||||
// detectText numbers lines with split('\n'); parseInlineIgnores must match.
|
||||
const content = '.a { font-family: Inter; }\r\n.b { font-family: Roboto; } /* impeccable-disable-line overused-font */';
|
||||
const out = detectText(content, 'a.css', opts);
|
||||
const fonts = out.filter((f) => f.antipattern === 'overused-font').map((f) => f.line);
|
||||
expect(fonts).toEqual([1]); // Inter on line 1 stays; Roboto on line 2 is waived
|
||||
});
|
||||
|
||||
test('a directive for one rule leaves other findings intact', () => {
|
||||
const content = '.a { font-family: Inter; } /* impeccable-disable-line side-tab */';
|
||||
const out = detectText(content, 'a.css', opts);
|
||||
expect(out.some((f) => f.antipattern === 'overused-font')).toBe(true);
|
||||
});
|
||||
});
|
||||
|
||||
describe('detectHtml honors whole-file directives (line-less findings)', () => {
|
||||
const page = (extra = '') => `<!DOCTYPE html><html><head>${extra}
|
||||
<style>body { font-family: Inter, sans-serif; }</style></head>
|
||||
<body><p>Some real paragraph text here for the typography pass.</p>
|
||||
<h1>Heading</h1><h2>Sub</h2></body></html>`;
|
||||
|
||||
test('overused-font fires without a directive', async () => {
|
||||
const flagged = await detectHtml(await writeTmp(page()), { providers: [] });
|
||||
expect(flagged.some((f) => f.antipattern === 'overused-font')).toBe(true);
|
||||
});
|
||||
|
||||
test('whole-file directive in an HTML comment suppresses it', async () => {
|
||||
const file = await writeTmp(page('<!-- impeccable-disable overused-font -- exported brand doc -->'));
|
||||
const waived = await detectHtml(file, { providers: [] });
|
||||
expect(waived.some((f) => f.antipattern === 'overused-font')).toBe(false);
|
||||
});
|
||||
|
||||
test('inlineIgnores:false bypasses it', async () => {
|
||||
const file = await writeTmp(page('<!-- impeccable-disable overused-font -->'));
|
||||
const raw = await detectHtml(file, { providers: [], inlineIgnores: false });
|
||||
expect(raw.some((f) => f.antipattern === 'overused-font')).toBe(true);
|
||||
});
|
||||
});
|
||||
|
||||
describe('detect CLI end-to-end', () => {
|
||||
function run(args) {
|
||||
return spawnSync(process.execPath, [CLI, 'detect', ...args], { encoding: 'utf-8' });
|
||||
}
|
||||
|
||||
test('inline directive is honored by default, --no-inline-ignores and --no-config bypass it', async () => {
|
||||
const file = await writeTmp(
|
||||
'<!DOCTYPE html><html><head><!-- impeccable-disable overused-font -->\n' +
|
||||
'<style>body { font-family: Inter, sans-serif; }</style></head>\n' +
|
||||
'<body><p>Paragraph copy for the typography analyzer to read.</p><h1>H</h1><h2>S</h2></body></html>',
|
||||
'.html',
|
||||
);
|
||||
|
||||
const honored = run([file, '--json', '--no-design-system']);
|
||||
expect(JSON.parse(honored.stdout).some((f) => f.antipattern === 'overused-font')).toBe(false);
|
||||
|
||||
const bypassed = run([file, '--json', '--no-design-system', '--no-inline-ignores']);
|
||||
expect(JSON.parse(bypassed.stdout).some((f) => f.antipattern === 'overused-font')).toBe(true);
|
||||
|
||||
const rawConfig = run([file, '--json', '--no-config']);
|
||||
expect(JSON.parse(rawConfig.stdout).some((f) => f.antipattern === 'overused-font')).toBe(true);
|
||||
});
|
||||
});
|
||||
|
||||
let tmpDir;
|
||||
async function writeTmp(content, ext = '.html') {
|
||||
if (!tmpDir) tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'impeccable-inline-'));
|
||||
const file = path.join(tmpDir, `f${Math.abs(hash(content))}${ext}`);
|
||||
fs.writeFileSync(file, content);
|
||||
return file;
|
||||
}
|
||||
|
||||
function hash(str) {
|
||||
let h = 0;
|
||||
for (let i = 0; i < str.length; i++) h = (Math.imul(31, h) + str.charCodeAt(i)) | 0;
|
||||
return h;
|
||||
}
|
||||
Reference in New Issue
Block a user