From 331540ddec18dae74e61c9fc4598e050f5624f71 Mon Sep 17 00:00:00 2001 From: Paul Bakaus Date: Sat, 18 Jul 2026 16:06:06 -0700 Subject: [PATCH] Scope a single rule to a file with ignore-value "*" --file (#379) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Scope a single rule to a file with ignore-value "*" --file `ignore-file ` was the only file-scoped escape the hook offered, and it is far blunter than most findings justify: it silences every rule for that path forever, including rules not written yet. A real UI surface with one noisy rule had no proportionate option. Add a file scope to `ignore-value`, so one rule can be turned off in matching files while staying active everywhere else: hooks ignore-value design-system-font-size "*" --file "src/widget.js" - Refuse a bare `"*"` with no `--file`. Suppressing a rule project-wide is `ignore-rule`'s job, and the error says so. - Reject unknown `--flags` instead of folding them into the value. `ignore-value overused-font Inter --shard` stored the value "inter --shard", matched no finding, and reported success. - Key dedup on the file scope too. The same rule/value legitimately appears more than once with different scopes; the old rule+value key silently overwrote the earlier entry. - Keep normalizer key order (rule, value, files, createdAt, reason) in step across both copies. Normalizing runs on every write, so emitting a different order than what is on disk rewrites untouched entries. - Lead with the narrow form in the hook's directive footer and hooks.md; `ignore-file` is now documented as the whole-file-out-of-scope case. Dogfoods it on skill/scripts/live-browser.js, where all 32 findings are design-system-font-size: the overlay is injected over arbitrary host pages and builds a self-contained UI, so DESIGN.md's ramp does not describe it. The other rules stay live for that file. Assisted-by: Claude Code * Show the file scope in hooks status, and stop the wildcard error misdirecting Two findings from Cursor. status formatted every ignore value as rule=value and dropped files. Now that the primary hooks path writes file-scoped `"*"` entries, that rendered `design-system-font-size=*` — which reads as exactly the project-wide wildcard this command refuses, the opposite of what is on disk. Print the scope, matching the `rule=value [files]` shape `impeccable ignores list` already uses. This repo's own config already carries several scoped wildcards written through the CLI path, so status has been under-reporting them. The bare-wildcard refusal always pointed at `ignore-rule `. For overused-font that command refuses on its own without --all-values, so the guidance handed the user a second error. Name the flag for that rule. Assisted-by: Claude Code * Refuse an empty --file glob, and store multi-file scopes in canonical order Two Copilot findings, both the silent-no-op class this PR exists to remove. An empty glob was dropped by filter(Boolean). So `ignore-value overused-font Inter --file=` reported "Added overused-font=inter" and wrote an entry with no files: the user asked to scope a rule to one file and silently got the project-wide suppression instead — broader than what they asked for, reported as success. Refuse an empty or whitespace glob on every form (--file, --file=, --files, --files=) in both the hook-admin and CLI paths. Multi-file scopes were deduped but not ordered, and the dedup key compares the files array, so `--file b.css --file a.css` stored a second entry distinct from `--file a.css --file b.css`. Sort at parse so storage is canonical, and sort inside the key so entries already on disk in another order still compare equal. Assisted-by: Claude Code * Sort files in every dedup key, not just two of the four My previous commit sorted the file scope at parse time and inside ignoreValueFilesKey, and stopped there. Cursor pointed out ignoreValueKey (CLI) and ignoreValueEntryKey (hook-admin) still joined `files` in stored order, so add/remove dedup missed any on-disk scope whose glob order differed from the sorted argv form: a re-add duplicated the entry and a remove silently failed. Four functions hash `files`; I had fixed two. All four sort now. The remaining `files.join(', ')` call sites are display, not keys. Verified against a config seeded in non-sorted order, as an older client would have written it: the re-add updates the existing entry rather than duplicating it, and remove-value finds it. Test covers that shape. Assisted-by: Claude Code * Refuse a following flag as a --file glob Cursor again, same class as the last two. requireGlob checked non-empty but not whether the argv it consumed was itself a flag, so `ignore-value design-system-font-size "*" --file --reason "why"` took `--reason` as the scope, left "why" to fold into the value, stored value="* why" files=["--reason"], and reported success. Garbage, announced as done. Refuse a glob starting with `--`, in both the hook-admin and CLI paths. Assisted-by: Claude Code --- .impeccable/config.json | 9 ++ cli/bin/commands/ignores.mjs | 33 ++++-- cli/lib/impeccable-config.mjs | 14 ++- skill/reference/hooks.md | 17 ++- skill/scripts/hook-admin.mjs | 76 +++++++++++-- skill/scripts/hook-lib.mjs | 24 +++-- tests/hook.test.mjs | 194 ++++++++++++++++++++++++++++++++++ 7 files changed, 335 insertions(+), 32 deletions(-) diff --git a/.impeccable/config.json b/.impeccable/config.json index 45ca0e911..b2df55f0d 100644 --- a/.impeccable/config.json +++ b/.impeccable/config.json @@ -71,6 +71,15 @@ ], "createdAt": "2026-06-15T23:37:38.170Z", "reason": "Generic slop card intentionally uses Inter for the before-state comparison" + }, + { + "rule": "design-system-font-size", + "value": "*", + "files": [ + "skill/scripts/live-browser.js" + ], + "createdAt": "2026-07-17T00:00:00.000Z", + "reason": "Live overlay chrome is injected over arbitrary host pages and builds a self-contained UI with its own small type scale; DESIGN.md's ramp describes the impeccable website, not this widget" } ] }, diff --git a/cli/bin/commands/ignores.mjs b/cli/bin/commands/ignores.mjs index 9eb4696ed..dc1d6a376 100644 --- a/cli/bin/commands/ignores.mjs +++ b/cli/bin/commands/ignores.mjs @@ -78,6 +78,20 @@ function parseScope(args, { allowAll = false } = {}) { return { local, all, rest }; } +// An empty glob used to be dropped by filter(Boolean), so `--file=` reported +// success and wrote an entry with no files: the user asked to scope a rule to one +// file and silently got the project-wide suppression instead. Refuse it. +function requireGlob(raw, flag) { + const glob = String(raw ?? '').trim(); + if (!glob) throw new Error(`${flag} requires a non-empty glob`); + // A following flag is not a glob. `--file --reason "why"` consumed `--reason` + // as the scope and left the reason text to fold into the value, storing + // value="* why" files=["--reason"] and reporting success. Same silent-no-op + // class as an unknown flag folding into the value; refuse it the same way. + if (glob.startsWith('--')) throw new Error(`${flag} requires a glob, got the flag ${glob}`); + return glob; +} + function parseValueArgs(args, { allowUnscopedWildcard = false } = {}) { const positionals = []; const files = []; @@ -93,11 +107,11 @@ function parseValueArgs(args, { allowUnscopedWildcard = false } = {}) { reason = arg.slice('--reason='.length).trim(); } else if (arg === '--file' || arg === '--files') { if (i + 1 >= args.length) throw new Error(`${arg} requires a glob`); - files.push(String(args[++i]).trim()); + files.push(requireGlob(args[++i], arg)); } else if (arg.startsWith('--file=')) { - files.push(arg.slice('--file='.length).trim()); + files.push(requireGlob(arg.slice('--file='.length), '--file')); } else if (arg.startsWith('--files=')) { - files.push(arg.slice('--files='.length).trim()); + files.push(requireGlob(arg.slice('--files='.length), '--files')); } else if (arg.startsWith('--')) { throw new Error(`Unknown add-value flag: ${arg}`); } else { @@ -108,7 +122,9 @@ function parseValueArgs(args, { allowUnscopedWildcard = false } = {}) { const [rule, ...valueParts] = positionals; const value = normalizeIgnoreValue(valueParts.join(' ')); if (!rule || !value) throw new Error('Pass a rule id and value, e.g. impeccable ignores add-value overused-font Inter'); - const scopedFiles = Array.from(new Set(files.filter(Boolean))); + // Sorted: the dedup key compares the files array, so an unsorted scope made + // `--file b.css --file a.css` a different entry from `--file a.css --file b.css`. + const scopedFiles = Array.from(new Set(files.filter(Boolean))).sort(); if (value === '*' && scopedFiles.length === 0 && !allowUnscopedWildcard) { throw new Error('Wildcard value ignores must be scoped with --file .'); } @@ -226,12 +242,14 @@ function addValue(cwd, args) { if (parsed.reason) existing.reason = parsed.reason; if (parsed.files.length) existing.files = parsed.files; } else { + // rule, value, files, createdAt, reason — the same order the normalizers emit, + // so a fresh entry survives the next write untouched. const entry = { rule: parsed.rule, value: parsed.value, - createdAt: new Date().toISOString(), }; if (parsed.files.length) entry.files = parsed.files; + entry.createdAt = new Date().toISOString(); if (parsed.reason) entry.reason = parsed.reason; config.ignoreValues.push(entry); } @@ -299,7 +317,10 @@ function clear(cwd, args) { } function ignoreValueKey(entry) { - const files = Array.isArray(entry.files) && entry.files.length ? entry.files.join('\x1f') : ''; + // Sorted: a file scope is a set. Comparing stored order made an on-disk scope + // miss the sorted argv form, so a re-add duplicated the entry and a remove + // silently failed. Every key that hashes `files` must sort — there are four. + const files = Array.isArray(entry.files) && entry.files.length ? [...entry.files].sort().join('\x1f') : ''; return `${String(entry.rule || '').trim().toLowerCase()}\0${normalizeIgnoreValue(entry.value)}\0${files}`; } diff --git a/cli/lib/impeccable-config.mjs b/cli/lib/impeccable-config.mjs index 45c65e65a..a62de8e3c 100644 --- a/cli/lib/impeccable-config.mjs +++ b/cli/lib/impeccable-config.mjs @@ -346,12 +346,16 @@ export function normalizeIgnoreValueEntries(entries) { ...(Array.isArray(entry.files) ? entry.files.filter(v => typeof v === 'string' && v.trim()).map(v => v.trim()) : []), ]); if (files.length > 0) normalized.files = files; - if (typeof entry.reason === 'string' && entry.reason.trim()) { - normalized.reason = entry.reason.trim(); - } + // Key order is rule, value, files, createdAt, reason and must stay that way: + // normalizing runs on every write, so emitting a different order than the one + // already on disk rewrites every untouched entry and churns the diff. Keep in + // step with normalizeIgnoreValueEntries in skill/scripts/hook-lib.mjs. if (typeof entry.createdAt === 'string' && entry.createdAt.trim()) { normalized.createdAt = entry.createdAt.trim(); } + if (typeof entry.reason === 'string' && entry.reason.trim()) { + normalized.reason = entry.reason.trim(); + } out.push(normalized); } return out; @@ -369,7 +373,9 @@ function mergeIgnoreValues(existing, incoming) { } function ignoreValueFilesKey(files) { - return Array.isArray(files) && files.length > 0 ? files.join('\x1f') : ''; + // Sort before joining: a scope is a set, so an entry already on disk in another + // order must compare equal rather than dedup as two distinct entries. + return Array.isArray(files) && files.length > 0 ? [...files].sort().join('\x1f') : ''; } // Glob -> RegExp. Supports `**`, `*`, `?`, and `{a,b}` alternation. diff --git a/skill/reference/hooks.md b/skill/reference/hooks.md index b7f6033c3..606e49150 100644 --- a/skill/reference/hooks.md +++ b/skill/reference/hooks.md @@ -23,10 +23,11 @@ The first argument is the action. Defaults to `status`. | `status` | Print current state, shared/local config paths, ignored rules / files / values, env override. | | `on` | Set `enabled: true` in `.impeccable/config.json`, record local hook consent as accepted, and install/repair provider hook manifests when the skill is installed. | | `off` | Set `enabled: false` in `.impeccable/config.json`. | -| `ignore-rule ` | Append `` to `detector.ignoreRules`; for `overused-font`, requires `--all-values`. | -| `ignore-file ` | Append `` to `detector.ignoreFiles`. | +| `ignore-rule ` | Append `` to `detector.ignoreRules`; for `overused-font`, requires `--all-values`. Suppresses the rule across the whole project. | +| `ignore-file ` | Append `` to `detector.ignoreFiles`. Suppresses **every** rule for matching files. | | `ignore-value [--shared] [--reason "..."]` | Append a rule/value suppression to shared `.impeccable/config.json`. | | `ignore-value --local [--reason "..."]` | Append a private rule/value suppression to `.impeccable/config.local.json`. | +| `ignore-value "*" --file [--file ...]` | Turn one rule off in matching files only, leaving it active everywhere else. Repeat `--file`, or use `--file=` / `--files=`. A bare `"*"` with no `--file` is refused: use `ignore-rule ` if you really mean project-wide. | | `reset` | Delete the project config, dedup cache, and Cursor pending queue. | ## Flow @@ -51,7 +52,8 @@ Prefer the narrowest exception: - If the finding line shows an exact `ignore-value` command, run that command. This writes shared `.impeccable/config.json` by default. - 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 ` for the current file. +- If the finding has no value-specific command, such as `side-tab`, scope that one rule to the file: `ignore-value "*" --file `. Run `npx impeccable detect ` first to see what actually fires there. +- Reach for `ignore-file ` only when the whole file is out of scope for design review: a fixture, a generated artifact, a deliberate slop demo. It silences every rule for that file permanently, including rules that have not been written yet. A real UI surface with one noisy rule wants the file-scoped value ignore above. - Use `ignore-rule ` 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. - 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 ` (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. @@ -73,7 +75,14 @@ Example whole-rule font exception: node {{scripts_path}}/hook-admin.mjs ignore-rule overused-font --all-values --reason "User asked to ignore overused fonts generally" ``` -Example file-scoped exception: +Example one-rule-in-one-file exception, for a file that is still worth reviewing +for everything else: + +```bash +node {{scripts_path}}/hook-admin.mjs ignore-value design-system-font-size "*" --file "src/overlay/widget.js" --reason "Injected widget builds its own type scale; DESIGN.md's ramp describes the site" +``` + +Example whole-file exception, for a file that is out of scope entirely: ```bash node {{scripts_path}}/hook-admin.mjs ignore-file "src/legacy/Card.tsx" diff --git a/skill/scripts/hook-admin.mjs b/skill/scripts/hook-admin.mjs index 8b37b1f3b..12cf004b9 100644 --- a/skill/scripts/hook-admin.mjs +++ b/skill/scripts/hook-admin.mjs @@ -13,6 +13,8 @@ * node hook-admin.mjs ignore-file # append to ignoreFiles * node hook-admin.mjs ignore-value # append to shared ignoreValues * node hook-admin.mjs ignore-value --local + * node hook-admin.mjs ignore-value "*" --file # rule off in only + * node hook-admin.mjs ignore-value "*" # refused: scope it or use ignore-rule * node hook-admin.mjs reset # remove all config + cache * * Designed to be invoked by the LLM from the reference/hooks.md flow. @@ -265,7 +267,10 @@ function mergeIgnoreValueEntries(existing, incoming) { } function ignoreValueEntryKey(entry) { - const files = Array.isArray(entry.files) && entry.files.length > 0 ? entry.files.join('\x1f') : ''; + // Sorted: a file scope is a set. Comparing stored order made an on-disk scope + // miss the sorted argv form, so a re-add duplicated the entry and a remove + // silently failed. Every key that hashes `files` must sort — there are four. + const files = Array.isArray(entry.files) && entry.files.length > 0 ? [...entry.files].sort().join('\x1f') : ''; return `${entry.rule}\0${entry.value}\0${files}`; } @@ -283,7 +288,14 @@ function statusReport(cwd) { if (info.exists) return relPath; return `${relPath} (${absent})`; }; - const ignoreValues = cfg.ignoreValues.map((entry) => `${entry.rule}=${entry.value}`); + // Show the file scope. Dropping it rendered a file-scoped entry as + // `design-system-font-size=*`, which reads as the project-wide wildcard this + // command refuses — the opposite of what is on disk. Matches the + // `rule=value [files]` shape `impeccable ignores list` already prints. + const ignoreValues = cfg.ignoreValues.map((entry) => { + const scope = Array.isArray(entry.files) && entry.files.length ? ` [${entry.files.join(', ')}]` : ''; + return `${entry.rule}=${entry.value}${scope}`; + }); const lines = [ `Impeccable design hook`, @@ -532,14 +544,29 @@ function addIgnoreFile(cwd, glob) { return `Added "${glob}" to detector.ignoreFiles. Current: ${config.ignoreFiles.join(', ')}`; } +// An empty glob used to be dropped by filter(Boolean), so `--file=` reported +// success and wrote an entry with no files: the user asked to scope a rule to one +// file and silently got the project-wide suppression instead. Refuse it. +function requireGlob(raw, flag) { + const glob = String(raw ?? '').trim(); + if (!glob) throw new Error(`${flag} requires a non-empty glob`); + // A following flag is not a glob. `--file --reason "why"` consumed `--reason` + // as the scope and left the reason text to fold into the value, storing + // value="* why" files=["--reason"] and reporting success. Same silent-no-op + // class as an unknown flag folding into the value; refuse it the same way. + if (glob.startsWith('--')) throw new Error(`${flag} requires a glob, got the flag ${glob}`); + return glob; +} + function parseIgnoreValueArgs(args) { const positionals = []; + const files = []; let shared = false; let local = false; let reason = ''; for (let i = 0; i < args.length; i++) { - const arg = args[i]; + const arg = String(args[i] || ''); if (arg === '--shared') { shared = true; } else if (arg === '--local') { @@ -550,8 +577,20 @@ function parseIgnoreValueArgs(args) { chunks.push(args[++i]); } reason = chunks.join(' ').trim(); - } else if (String(arg).startsWith('--reason=')) { - reason = String(arg).slice('--reason='.length).trim(); + } else if (arg.startsWith('--reason=')) { + reason = arg.slice('--reason='.length).trim(); + } else if (arg === '--file' || arg === '--files') { + if (i + 1 >= args.length) throw new Error(`${arg} requires a glob`); + files.push(requireGlob(args[++i], arg)); + } else if (arg.startsWith('--file=')) { + files.push(requireGlob(arg.slice('--file='.length), '--file')); + } else if (arg.startsWith('--files=')) { + files.push(requireGlob(arg.slice('--files='.length), '--files')); + } else if (arg.startsWith('--')) { + // Otherwise a typo folds into the value: `ignore-value overused-font Inter + // --shard` stored the value "inter --shard", which matches no finding, and + // reported success. Matches `impeccable ignores add-value`. + throw new Error(`Unknown ignore-value flag: ${arg}`); } else { positionals.push(arg); } @@ -561,6 +600,9 @@ function parseIgnoreValueArgs(args) { return { rule: String(rule || '').trim().toLowerCase(), value: normalizeIgnoreValue(valueParts.join(' ')), + // Sorted: the dedup key compares the files array, so an unsorted scope made + // `--file b.css --file a.css` a different entry from `--file a.css --file b.css`. + files: Array.from(new Set(files.filter(Boolean))).sort(), shared, local, reason, @@ -577,10 +619,24 @@ function addIgnoreValue(cwd, args) { throw new Error('Pass only one scope flag: --shared or --local'); } + // A bare `*` would suppress the rule everywhere, which is ignore-rule's job and + // not what a finding in one file justifies. detector.ignoreValues honours a + // `files` scope, so require one — matching `impeccable ignores add-value`. + if (parsed.value === '*' && parsed.files.length === 0) { + // `ignore-rule overused-font` refuses on its own without --all-values, so + // naming the bare form here would hand the user a second error. + const projectWide = parsed.rule === 'overused-font' + ? `${IMPECCABLE_COMMAND} hooks ignore-rule ${parsed.rule} --all-values` + : `${IMPECCABLE_COMMAND} hooks ignore-rule ${parsed.rule}`; + throw new Error(`Wildcard value ignores must be scoped with --file , e.g. ${IMPECCABLE_COMMAND} hooks ignore-value design-system-font-size "*" --file "src/widget.js". To suppress the rule project-wide use ${projectWide}.`); + } + const local = parsed.local; const config = mergeDetectorConfig(readRawDetectorConfig(cwd, { local })); - const key = `${parsed.rule}\0${parsed.value}`; - const existing = config.ignoreValues.find((entry) => `${entry.rule}\0${entry.value}` === key); + // Key on the file scope too: the same rule/value legitimately appears more than + // once with different scopes, and a rule+value-only key overwrote them. + const key = ignoreValueEntryKey({ rule: parsed.rule, value: parsed.value, files: parsed.files }); + const existing = config.ignoreValues.find((entry) => ignoreValueEntryKey(entry) === key); if (existing) { if (parsed.reason) existing.reason = parsed.reason; @@ -588,15 +644,17 @@ function addIgnoreValue(cwd, args) { const entry = { rule: parsed.rule, value: parsed.value, - createdAt: new Date().toISOString(), }; + if (parsed.files.length) entry.files = parsed.files; + entry.createdAt = new Date().toISOString(); if (parsed.reason) entry.reason = parsed.reason; config.ignoreValues.push(entry); } const target = writeDetectorConfig(cwd, config, { local }); const scope = local ? 'local detector.ignoreValues' : 'shared detector.ignoreValues'; - return `Added ${parsed.rule}=${parsed.value} to ${scope} (${path.relative(cwd, target) || target}).`; + const scopeSuffix = parsed.files.length ? ` scoped to ${parsed.files.join(', ')}` : ''; + return `Added ${parsed.rule}=${parsed.value}${scopeSuffix} to ${scope} (${path.relative(cwd, target) || target}).`; } function reset(cwd) { diff --git a/skill/scripts/hook-lib.mjs b/skill/scripts/hook-lib.mjs index 0d9722953..2893fa4f3 100644 --- a/skill/scripts/hook-lib.mjs +++ b/skill/scripts/hook-lib.mjs @@ -502,12 +502,15 @@ export function normalizeIgnoreValueEntries(entries) { ...(Array.isArray(entry.files) ? entry.files.filter(v => typeof v === 'string' && v.trim()).map(v => v.trim()) : []), ]); if (files.length > 0) normalized.files = files; - if (typeof entry.reason === 'string' && entry.reason.trim()) { - normalized.reason = entry.reason.trim(); - } + // Key order is rule, value, files, createdAt, reason and must stay that way: + // normalizing runs on every write, so emitting a different order than the one + // already on disk rewrites every untouched entry and churns the diff. if (typeof entry.createdAt === 'string' && entry.createdAt.trim()) { normalized.createdAt = entry.createdAt.trim(); } + if (typeof entry.reason === 'string' && entry.reason.trim()) { + normalized.reason = entry.reason.trim(); + } out.push(normalized); } return out; @@ -525,7 +528,9 @@ function mergeIgnoreValues(existing, incoming) { } function ignoreValueFilesKey(files) { - return Array.isArray(files) && files.length > 0 ? files.join('\x1f') : ''; + // Sort before joining: a scope is a set, so an entry already on disk in another + // order must compare equal rather than dedup as two distinct entries. + return Array.isArray(files) && files.length > 0 ? [...files].sort().join('\x1f') : ''; } export function readCache(cwd) { @@ -1465,16 +1470,17 @@ export function appendDesignSystemNote(text, scanOptions) { // raw envelope. Asking the model to surface the resolution in its // reply is the cheapest way to make the feedback loop visible. function directiveFooter(display, opts = {}) { - const ignoreFileCommand = `${IMPECCABLE_COMMAND} hooks ignore-file ${quoteCommandArg(display)}`; - const fileIgnoreGuidance = opts.grouped - ? `run \`${IMPECCABLE_COMMAND} hooks ignore-file \` for the specific file` - : `run \`${ignoreFileCommand}\``; + // Offer the rule-scoped-to-file form first. `ignore-file` silences every rule + // for the path forever, which is far more than one noisy rule on a real UI + // surface justifies, and it was previously the only option named here. + const target = opts.grouped ? '' : quoteCommandArg(display); + const fileIgnoreGuidance = `run \`${IMPECCABLE_COMMAND} hooks ignore-value "*" --file ${target}\` to scope just that rule to the file, or \`${IMPECCABLE_COMMAND} hooks ignore-file ${target}\` only when the whole file is out of scope for design review (a fixture, a generated artifact, a deliberate demo)`; return [ 'Handle these before finalizing: fix findings that are real design problems, or explicitly classify contextually intentional findings as false positives. Acknowledge what you changed or why you are leaving a finding unchanged.', '', '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, 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 \` 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_COMMAND} 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_COMMAND} 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_COMMAND} hooks ignore-rule \` only when the user asks to suppress the whole non-value-specific rule. Run ${IMPECCABLE_COMMAND} 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 \` 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_COMMAND} 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_COMMAND} hooks ignore-rule overused-font --all-values\` only when the user asks to ignore overused fonts generally. For a finding whose line shows no exact ignore-value command, such as \`side-tab\`, ${fileIgnoreGuidance}; use \`${IMPECCABLE_COMMAND} hooks ignore-rule \` only when the user asks to suppress the whole non-value-specific rule. Run ${IMPECCABLE_COMMAND} audit for the full pass.`, ].join('\n'); } diff --git a/tests/hook.test.mjs b/tests/hook.test.mjs index 3fe0000ba..0bf3317c7 100644 --- a/tests/hook.test.mjs +++ b/tests/hook.test.mjs @@ -53,7 +53,9 @@ import { extractFindingIgnoreValue, resolveProjectPlatform, isNativePlatform, + normalizeIgnoreValueEntries, } from '../skill/scripts/hook-lib.mjs'; +import { normalizeIgnoreValueEntries as normalizeIgnoreValueEntriesCli } from '../cli/lib/impeccable-config.mjs'; import { detectHtml, detectText } from '../cli/engine/detect-antipatterns.mjs'; function mkTmp() { @@ -525,6 +527,72 @@ describe('hook-admin.mjs', () => { }); } + it('refuses an empty --file glob instead of silently writing a project-wide ignore', () => { + // `--file=` was dropped by filter(Boolean), so this reported success and + // stored an entry with no files: a broader suppression than was asked for. + for (const args of [['--file='], ['--file', ''], ['--files=']]) { + assert.throws( + () => runAdmin(['ignore-value', 'overused-font', 'Inter', ...args]), + /requires a non-empty glob/, + `empty glob via ${args.join(' ')} must error`, + ); + } + // `--file --reason "why"` consumed --reason as the scope and let the reason + // text fold into the value: stored value="* why" files=["--reason"], success. + assert.throws( + () => runAdmin(['ignore-value', 'design-system-font-size', '*', '--file', '--reason', 'why']), + /requires a glob, got the flag --reason/, + 'a following flag is not a glob', + ); + assert.equal(fs.existsSync(path.join(cwd, '.impeccable', 'config.json')), false, 'nothing may be written'); + }); + + it('matches an on-disk scope whose glob order differs from the sorted argv form', () => { + // Storage is canonical now, but configs written before that are not. Every key + // that hashes `files` must sort or a re-add duplicates the entry. + fs.mkdirSync(path.join(cwd, '.impeccable'), { recursive: true }); + fs.writeFileSync(path.join(cwd, '.impeccable', 'config.json'), JSON.stringify({ + detector: { ignoreValues: [ + { rule: 'design-system-font-size', value: '*', files: ['b.css', 'a.css'], createdAt: '2026-01-01T00:00:00.000Z' }, + ] }, + })); + runAdmin(['ignore-value', 'design-system-font-size', '*', '--file', 'a.css', '--file', 'b.css', '--reason', 're-add']); + const cfg = JSON.parse(fs.readFileSync(path.join(cwd, '.impeccable', 'config.json'), 'utf-8')); + const entries = cfg.detector.ignoreValues.filter((e) => e.rule === 'design-system-font-size'); + assert.equal(entries.length, 1, 'an unsorted on-disk scope must match the sorted argv form, not duplicate'); + assert.equal(entries[0].reason, 're-add', 'the existing entry is the one updated'); + }); + + it('stores a multi-file scope in canonical order so argv order cannot duplicate it', () => { + runAdmin(['ignore-value', 'design-system-font-size', '*', '--file', 'b.css', '--file', 'a.css']); + runAdmin(['ignore-value', 'design-system-font-size', '*', '--file', 'a.css', '--file', 'b.css']); + const cfg = JSON.parse(fs.readFileSync(path.join(cwd, '.impeccable', 'config.json'), 'utf-8')); + const entries = cfg.detector.ignoreValues.filter((e) => e.rule === 'design-system-font-size'); + assert.equal(entries.length, 1, 'the same scope in a different order is one entry, not two'); + assert.deepEqual(entries[0].files, ['a.css', 'b.css']); + }); + + it('status shows the file scope of a scoped wildcard ignore', () => { + runAdmin(['ignore-value', 'design-system-font-size', '*', '--file', 'src/widget.js']); + const out = runAdmin(['status']); + // Printing `design-system-font-size=*` bare reads as the project-wide + // wildcard this command refuses, which is the opposite of what is on disk. + assert.match(out, /design-system-font-size=\*\s*\[src\/widget\.js\]/); + }); + + it('refuses a bare wildcard and names a project-wide command that actually works', () => { + assert.throws( + () => runAdmin(['ignore-value', 'design-system-font-size', '*']), + (err) => /--file/.test(String(err.stderr)) && /ignore-rule design-system-font-size\./.test(String(err.stderr)), + ); + // ignore-rule overused-font refuses on its own without --all-values, so the + // suggestion must carry the flag or it hands the user a second error. + assert.throws( + () => runAdmin(['ignore-value', 'overused-font', '*']), + (err) => /ignore-rule overused-font --all-values/.test(String(err.stderr)), + ); + }); + it('ignore-value writes shared config by default without creating local config', () => { const out = runAdmin(['ignore-value', 'overused-font', 'Inter', '--reason', 'User confirmed Inter']); assert.match(out, /overused-font=inter/); @@ -564,6 +632,132 @@ describe('hook-admin.mjs', () => { assert.match(status, /ignoreValues:\s+overused-font=inter/); }); + // detector.ignoreValues honours a `files` scope, which is the narrowest way to + // silence one noisy rule on one file. hook-admin could not write it, so the + // only reachable option was ignore-file, which silences every rule for that + // file forever. + it('ignore-value scopes a wildcard to files via --file', () => { + const out = runAdmin([ + 'ignore-value', 'design-system-font-size', '*', + '--file', 'src/overlay/widget.js', + '--reason', 'Widget builds its own type scale', + ]); + assert.match(out, /scoped to src\/overlay\/widget\.js/); + const shared = JSON.parse(fs.readFileSync(getConfigPath(cwd), 'utf-8')).detector; + assert.deepEqual(shared.ignoreValues, [{ + rule: 'design-system-font-size', + value: '*', + files: ['src/overlay/widget.js'], + createdAt: shared.ignoreValues[0].createdAt, + reason: 'Widget builds its own type scale', + }]); + }); + + it('ignore-value accepts --file=, --files= and repeated --file', () => { + runAdmin(['ignore-value', 'side-tab', '*', '--file=a.css']); + runAdmin(['ignore-value', 'side-tab', '*', '--files=b.css']); + runAdmin(['ignore-value', 'low-contrast', '*', '--file', 'c.css', '--file', 'd.css']); + const shared = JSON.parse(fs.readFileSync(getConfigPath(cwd), 'utf-8')).detector; + assert.deepEqual( + shared.ignoreValues.map(({ rule, files }) => ({ rule, files })), + [ + { rule: 'side-tab', files: ['a.css'] }, + { rule: 'side-tab', files: ['b.css'] }, + { rule: 'low-contrast', files: ['c.css', 'd.css'] }, + ], + 'each distinct file scope is its own entry; a rule+value-only key overwrote them', + ); + }); + + it('ignore-value refuses a wildcard with no file scope', () => { + assert.throws( + () => runAdmin(['ignore-value', 'design-system-font-size', '*']), + /Wildcard value ignores must be scoped with --file/, + 'a bare wildcard is ignore-rule\'s job, not a per-file waiver', + ); + assert.equal(fs.existsSync(getConfigPath(cwd)), false, 'a refused ignore must not write config'); + }); + + it('ignore-value --file requires a glob', () => { + assert.throws( + () => runAdmin(['ignore-value', 'side-tab', '*', '--file']), + /--file requires a glob/, + ); + }); + + it('ignore-value rejects an unknown flag instead of folding it into the value', () => { + // `--shard` (a typo for --shared) used to store the value "inter --shard", + // which matches nothing, while reporting a successful suppression. + assert.throws( + () => runAdmin(['ignore-value', 'overused-font', 'Inter', '--shard']), + /Unknown ignore-value flag: --shard/, + ); + assert.equal(fs.existsSync(getConfigPath(cwd)), false); + }); + + // Every write runs the entries through normalizeIgnoreValueEntries. Emitting a + // different key order than the one on disk rewrote all untouched entries. + it('an unrelated edit leaves existing ignoreValues byte-identical', () => { + fs.mkdirSync(path.join(cwd, '.impeccable'), { recursive: true }); + const seeded = { + detector: { + ignoreRules: [], + ignoreFiles: [], + ignoreValues: [ + { + rule: 'bounce-easing', + value: 'bounce-ball', + createdAt: '2026-06-15T04:15:03.164Z', + reason: 'Intentional', + }, + { + rule: 'design-system-color', + value: '*', + files: ['site/styles/demo.css'], + createdAt: '2026-06-15T23:37:38.170Z', + reason: 'Deliberate off-system demo', + }, + ], + }, + }; + fs.writeFileSync(getConfigPath(cwd), JSON.stringify(seeded, null, 2) + '\n'); + const before = JSON.parse(fs.readFileSync(getConfigPath(cwd), 'utf-8')).detector.ignoreValues; + + runAdmin(['ignore-file', 'some/other/**']); + + const after = JSON.parse(fs.readFileSync(getConfigPath(cwd), 'utf-8')).detector; + assert.deepEqual(after.ignoreFiles, ['some/other/**'], 'the intended change still lands'); + assert.equal( + JSON.stringify(after.ignoreValues), + JSON.stringify(before), + 'untouched ignoreValues must keep their exact key order, or every config diff churns', + ); + }); + + // hook-lib.mjs (skill, ships into harness dirs) and cli/lib/impeccable-config.mjs + // (CLI + Pages functions) carry independent copies of this normalizer by + // necessity. They write the same file, so a key-order drift between them makes + // the config churn depending on which tool touched it last. + it('both config normalizers emit identical entries', () => { + const input = [ + { rule: 'BOUNCE-EASING', value: 'Bounce-Ball', reason: ' r ', createdAt: '2026-01-01T00:00:00.000Z' }, + { rule: 'design-system-color', value: '*', files: [' a.css ', 'b.css', 'a.css'], createdAt: '2026-02-02T00:00:00.000Z' }, + { rule: 'side-tab', value: '*', file: 'legacy.css' }, + { rule: '', value: 'dropped' }, + ]; + assert.equal( + JSON.stringify(normalizeIgnoreValueEntries(input)), + JSON.stringify(normalizeIgnoreValueEntriesCli(input)), + 'skill/scripts/hook-lib.mjs and cli/lib/impeccable-config.mjs must agree, key order included', + ); + // And pin the canonical order itself, which is what the config on disk uses. + const full = { rule: 'side-tab', value: '*', files: ['a.css'], createdAt: '2026-01-01T00:00:00.000Z', reason: 'r' }; + assert.deepEqual( + Object.keys(normalizeIgnoreValueEntries([full])[0]), + ['rule', 'value', 'files', 'createdAt', 'reason'], + ); + }); + it('a /impeccable hooks edit preserves sibling hook fields (consent, quiet)', () => { fs.mkdirSync(path.join(cwd, '.impeccable'), { recursive: true }); // A recorded per-developer consent in the local file...