From f148496f672b502684145e36b21b75921793dc61 Mon Sep 17 00:00:00 2001 From: Paul Bakaus Date: Fri, 17 Jul 2026 15:06:08 -0700 Subject: [PATCH] Let hooks ignore-value scope a rule to files, and stop churning the config Fallout from suppressing the overlay's font-size findings: the narrowest exception detector.ignoreValues supports was unreachable from the path the hook tells the model to use, so the guidance steered to the blunt instrument instead. - hook-admin's ignore-value now takes --file / --files / --file= / --files=, matching `impeccable ignores add-value`, which already had them. Without it the only file-scoped option was ignore-file, which silences every rule for a path permanently, including rules not yet written. - A bare wildcard value is now refused with a message pointing at either --file or ignore-rule. Previously `ignore-value "*"` quietly wrote a project-wide suppression from a single file's finding. - ignore-value keyed entries on rule+value only, so a second scope for the same rule overwrote the first instead of coexisting. Key on the file scope too. - An unknown flag folded into the value: `ignore-value overused-font Inter --shard` stored "inter --shard", matched nothing, and reported success. Reject it, as the sibling command does. Config churn: normalizeIgnoreValueEntries runs on every write and emitted keys as rule, value, files, reason, createdAt while the config on disk uses createdAt before reason. Any edit therefore rewrote every untouched entry (35 churned lines for a one-line change). Pin the canonical order in both copies of the normalizer and in ignores.mjs, and add a test that the two copies cannot drift apart. Also point the hook's own footer and reference/hooks.md at the file-scoped form first, and say plainly what ignore-file costs. Prepared with AI assistance under maintainer direction. Co-Authored-By: Claude --- cli/bin/commands/ignores.mjs | 4 +- cli/lib/impeccable-config.mjs | 10 ++- skill/reference/hooks.md | 17 +++-- skill/scripts/hook-admin.mjs | 41 +++++++++-- skill/scripts/hook-lib.mjs | 20 +++--- tests/hook.test.mjs | 128 ++++++++++++++++++++++++++++++++++ 6 files changed, 197 insertions(+), 23 deletions(-) diff --git a/cli/bin/commands/ignores.mjs b/cli/bin/commands/ignores.mjs index 9eb4696ed..0d1393d0f 100644 --- a/cli/bin/commands/ignores.mjs +++ b/cli/bin/commands/ignores.mjs @@ -226,12 +226,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); } diff --git a/cli/lib/impeccable-config.mjs b/cli/lib/impeccable-config.mjs index 45c65e65a..431e4cad0 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; 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..cd40aff6b 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. @@ -534,12 +536,13 @@ function addIgnoreFile(cwd, 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 +553,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(String(args[++i]).trim()); + } else if (arg.startsWith('--file=')) { + files.push(arg.slice('--file='.length).trim()); + } else if (arg.startsWith('--files=')) { + files.push(arg.slice('--files='.length).trim()); + } 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 +576,7 @@ function parseIgnoreValueArgs(args) { return { rule: String(rule || '').trim().toLowerCase(), value: normalizeIgnoreValue(valueParts.join(' ')), + files: Array.from(new Set(files.filter(Boolean))), shared, local, reason, @@ -577,10 +593,19 @@ 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) { + 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 ${IMPECCABLE_COMMAND} hooks ignore-rule ${parsed.rule}.`); + } + 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 +613,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..21fcd6b68 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; @@ -1465,16 +1468,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..c2fa43f87 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() { @@ -564,6 +566,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...