From d146d2084bfed89afd1bffdb91b45e247b4596e9 Mon Sep 17 00:00:00 2001 From: Paul Bakaus Date: Sun, 19 Jul 2026 20:40:32 -0700 Subject: [PATCH] Stop the design hook lying about findings it already reported (#391) * Stop the design hook lying about findings it already reported Three fixes, all aimed at the hook being trustworthy enough that an agent keeps reading it. 1. The session cache was append-only, so the hook lied and then went blind. `rememberFindings` unioned new keys into the remembered set and nothing ever removed them, and the pending ack took its count from that set rather than from the live scan. Fixing two of three findings produced: Still has 3 finding(s) flagged earlier this session (overused-font:1:inter, overused-font:2:roboto, overused-font:3:geist) with roboto and geist already gone. Worse, a finding that was fixed and then reintroduced was deduped against the stale memory and never re-reported, so the hook was permanently blind to that regression for the rest of the session. The cache now syncs to the complete current scan on every scan, so the count shrinks as work lands and a reintroduced finding reads as fresh. Dedup within a session still works, because it compares against the previous scan rather than against all history. A detector failure leaves the remembered set alone instead of recording an empty scan as truth. 2. The size ceiling, for generated files that do not live under dist/. `GENERATED_PATH` covered dist, build, out, .next, .cache, coverage and .min., but repos commit browser bundles and vendored detector copies next to source. The hook was reading and scanning a 215KB generated bundle, and reporting findings in it. Added `generated` as a path segment, matched with separators on both sides so authored names such as generated-utils.ts and CodeGenerator.tsx still get scanned, plus a `limits.maxFileBytes` ceiling defaulting to 128KB. In this codebase authored files top out at 86KB while the bundles start at 215KB, so the gap is comfortable. 3. The clean ack repeated on every clean edit. It carries no finding, only the standing steer that a silent hook is not a verdict on the design. That steer is worth saying, but not dozens of times per session. It now fires once per file per session and reports `clean-ack-deduped` in the audit log so suppressed noise stays visible. The pending ack is deliberately untouched: it names real unresolved work, and the comment explaining why it must repeat still holds. Verified end-to-end against the built hook: three findings, fix two and the count drops to one naming only the survivor, fix the last and it goes clean, edit again and it stays silent, reintroduce and it fires as fresh. Generated provider output is deliberately left out; the sync workflow owns it. Prepared with AI assistance (Claude Code). Co-Authored-By: Claude * Address review: three clean-ack and audit bugs in the dedupe change All three were introduced by this PR and all three are fair catches. Quiet mode spent the ack (bugbot). A clean scan marked cleanAcked and persisted it even when quiet suppressed all output, so a later non-quiet run in the same session never got the steer. The quiet decision is now hoisted above the scan loop and quiet leaves the ack unspent. Multi-file events lost the ack (copilot). The first clean target became cleanWinner unconditionally; if that file was already acked, cleanAckDeduped went true and the `!cleanWinner` guard meant a later target that had never been acked could never win. A raw apply_patch touching two files would drop the second file's ack entirely. The loop now keeps looking for a target that is actually owed an ack. audit.bytes leaked across targets (copilot). It was set when a file was skipped as too-large and never cleared, so in a multi-file event a later emitted result carried the skipped file's byte count. Cleared per iteration. The tests use a raw apply_patch payload rather than MultiEdit, because MultiEdit in this harness is single-file ({ file_path, edits: [] }) and would not have exercised the multi-target paths at all. Verified the three tests fail against the pre-fix code and pass after, so they are not passing for the wrong reason. Prepared with AI assistance (Claude Code). Co-Authored-By: Claude * Address review: font-size waivers silently did nothing Two more review findings, both real. Specific-value font-size waivers were dead config (greptile). The rule emits an ignoreValue, and the hook's own directive footer tells the agent to waive value-specific findings with `hooks ignore-value `, but `design-system-font-size` was missing from the direct-value rule set in `extractFindingIgnoreValue`. The extracted value came back empty, so any waiver naming an actual size was compared against nothing and silently dropped. Only the `*` wildcard worked, which is why the framework-viz waiver earlier in this branch appeared to function. Reproduced against the built hook: with a `0.82rem` waiver the finding still fired; it now goes clean, while a waiver naming a different size correctly still fires, so this is not over-matching. Wrong audit skip reason (bugbot). In a mixed multi-target run, an earlier UI file whose ack was already spent set `cleanAckDeduped`, and a later non-UI clean file became the winner. The tail then reported `clean-ack-deduped` when the honest reason was `non-ui-ack`. Audit-label only, no behavior change. Reordered so the winner is described first and dedupe is reported only when it is genuinely why nothing was emitted. Prepared with AI assistance (Claude Code). Co-Authored-By: Claude * Mirror the font-size waiver fix into the CLI's config reader Bugbot caught that the previous commit only fixed one of two copies. `extractFindingIgnoreValue` exists twice, in skill/scripts/hook-lib.mjs and in cli/lib/impeccable-config.mjs, and the direct-value rule list is duplicated in both. Adding design-system-font-size to the hook alone meant the same .impeccable/config.json filtered differently depending on the entry point: a size waiver was honored by the hook and ignored by `npx impeccable detect`. The two functions are otherwise byte-identical, so this restores parity rather than changing CLI behavior independently. The new test notes the duplication so the next person knows the pair has drifted once already. Prepared with AI assistance (Claude Code). Co-Authored-By: Claude * Fix the audit byte-count leak properly, not just one scan order My earlier fix cleared audit.bytes at the top of each iteration, which was wrong twice over, and bugbot caught both. The clear sat below the sensitive, generated, extension, ignore-file and file-missing continues, so a later target exiting through any of those never reached it and kept the oversized file's size while audit.file pointed somewhere else. It also only handled the bundle-scanned-first order; when the oversized file came last, the byte count was set after the emitting file had already been decided and rode along on its audit entry regardless. The root problem was keeping per-file state on the shared audit object. The size is now held in a local and attached only when the oversized skip is the run's actual outcome, so it cannot describe a file other than the one being reported. Tests cover both scan orders, an early-continue target after the skip, and the single-oversized-file case where the count should still appear. Prepared with AI assistance (Claude Code). Co-Authored-By: Claude --------- Co-authored-by: Claude --- cli/lib/impeccable-config.mjs | 1 + skill/scripts/hook-lib.mjs | 108 +++++++-- tests/hook.test.mjs | 359 ++++++++++++++++++++++++++++ tests/lib/impeccable-config.test.js | 10 + 4 files changed, 459 insertions(+), 19 deletions(-) diff --git a/cli/lib/impeccable-config.mjs b/cli/lib/impeccable-config.mjs index a62de8e3c..a0c2af6d3 100644 --- a/cli/lib/impeccable-config.mjs +++ b/cli/lib/impeccable-config.mjs @@ -500,6 +500,7 @@ export function extractFindingIgnoreValue(finding) { 'design-system-font', 'design-system-color', 'design-system-radius', + 'design-system-font-size', ]); if (!directValueRules.has(rule)) return ''; return normalizeIgnoreValue(extractFindingIgnoreValueRaw(finding, rule)); diff --git a/skill/scripts/hook-lib.mjs b/skill/scripts/hook-lib.mjs index 2893fa4f3..f90c2f4a8 100644 --- a/skill/scripts/hook-lib.mjs +++ b/skill/scripts/hook-lib.mjs @@ -70,7 +70,9 @@ export const SENSITIVE_PATH = new RegExp([ ].join('|'), 'i'); // Hard-skip regex for generated, lock, minified, and build-output paths. -export const GENERATED_PATH = /(?:\.generated\.[a-z]+$|\.d\.ts$|\.min\.[a-z]+$|[/\\]node_modules[/\\]|[/\\](?:dist|build|out|\.next|\.cache|coverage)[/\\]|[/\\]?[^/\\]+\.lock(?:\.json)?$)/i; +// `generated` is matched as a whole path segment so authored names such as +// `generated-utils.ts` or `CodeGenerator.tsx` still get scanned. +export const GENERATED_PATH = /(?:\.generated\.[a-z]+$|\.d\.ts$|\.min\.[a-z]+$|[/\\]node_modules[/\\]|[/\\]generated[/\\]|[/\\](?:dist|build|out|\.next|\.cache|coverage)[/\\]|[/\\]?[^/\\]+\.lock(?:\.json)?$)/i; export const TRUTHY = /^(1|true|yes|on)$/i; @@ -83,7 +85,12 @@ export const DEFAULT_CONFIG = Object.freeze({ ignoreFiles: [], ignoreValues: [], extensions: [], - limits: { maxFindings: 5, maxChars: 8000 }, + // maxFileBytes: not every generated artifact lives under a path we can + // recognize. Committed browser bundles and vendored detector copies sit + // next to source and run 200KB+, while genuinely authored stylesheets in + // this codebase top out under 90KB. A single file past the ceiling is a + // bundle, and findings against a bundle are never actionable. + limits: { maxFindings: 5, maxChars: 8000, maxFileBytes: 131072 }, }); export const HOOK_LOCAL_IGNORE_PATTERNS = Object.freeze([ @@ -315,6 +322,7 @@ function applyConfigSource(config, raw) { config.limits = { maxFindings: numberOr(raw.limits.maxFindings, config.limits.maxFindings), maxChars: numberOr(raw.limits.maxChars, config.limits.maxChars), + maxFileBytes: numberOr(raw.limits.maxFileBytes, config.limits.maxFileBytes), }; } return config; @@ -774,6 +782,7 @@ export function extractFindingIgnoreValue(finding) { 'design-system-font', 'design-system-color', 'design-system-radius', + 'design-system-font-size', ]); if (!directValueRules.has(rule)) return ''; return normalizeIgnoreValue(extractFindingIgnoreValueRaw(finding, rule)); @@ -850,11 +859,20 @@ export function dedupeAgainstCache(findings, cache, sessionId, filePath) { return fresh; } +// Sync the remembered set to the findings present in the scan just performed. +// +// This replaces rather than accumulates, and that is the whole point. An +// append-only set made the hook lie twice over: the pending ack counted +// history instead of the live scan, so it kept naming findings the agent had +// already fixed, and a finding that was fixed and later reintroduced was +// deduped against a stale memory and never re-reported. Forgetting what is no +// longer there is what lets the count shrink and a regression fire again. +// +// Callers must pass the complete current finding set, not just the fresh ones. export function rememberFindings(cache, sessionId, filePath, findings) { const fileEntry = ensureFile(cache, sessionId, filePath); - const known = new Set(fileEntry.findings || []); - for (const f of findings) known.add(findingCacheKey(f)); - fileEntry.findings = Array.from(known); + const keys = new Set((findings || []).map(f => findingCacheKey(f))); + fileEntry.findings = Array.from(keys); ensureSession(cache, sessionId).updatedAt = Date.now(); } @@ -1556,6 +1574,9 @@ export async function runHook({ stdinJson, env = {}, cwd = process.cwd(), now = let cleanWinner = null; const freshGroups = []; let suppressionWinner = null; + let cleanAckDeduped = false; + let skippedBytes = 0; + const quietMode = truthy(env.IMPECCABLE_HOOK_QUIET) || config.quiet === true; let detectorThrewAny = false; let lastSkip = 'no-scannable-file'; let suppressedHit = false; @@ -1591,6 +1612,17 @@ export async function runHook({ stdinJson, env = {}, cwd = process.cwd(), now = continue; } + const maxFileBytes = config.limits?.maxFileBytes ?? DEFAULT_CONFIG.limits.maxFileBytes; + if (maxFileBytes > 0) { + let size = 0; + try { size = fs.statSync(filePath).size; } catch { size = 0; } + if (size > maxFileBytes) { + skippedBytes = size; + lastSkip = 'too-large'; + continue; + } + } + if (primaryFileSet.has(filePath)) { const editCount = bumpEditCount(cache, sessionId, filePath); cacheDirty = true; @@ -1624,23 +1656,47 @@ export async function runHook({ stdinJson, env = {}, cwd = process.cwd(), now = audit.findings = (findings || []).length; audit.freshFindings = fresh.length; - if (fresh.length > 0) { - rememberFindings(cache, sessionId, filePath, fresh); - cacheDirty = true; - freshGroups.push({ filePath, findings: fresh }); - continue; - } - + // A detector failure tells us nothing about the file, so leave whatever + // was remembered alone rather than recording an empty scan as truth. if (detectorThrew) { detectorThrewAny = true; continue; } + // Sync the cache to this scan before deciding what to emit, so fixed + // findings stop being remembered and a reintroduced one reads as fresh. + rememberFindings(cache, sessionId, filePath, filtered); + cacheDirty = true; + + if (fresh.length > 0) { + freshGroups.push({ filePath, findings: fresh }); + continue; + } + if (filtered.length > 0 && !pendingWinner) { - const known = (ensureFile(cache, sessionId, filePath).findings || []).slice(); - pendingWinner = { filePath, known }; + // Count the live scan, not the session's history. + pendingWinner = { filePath, known: filtered.map(f => findingCacheKey(f)) }; } else if (filtered.length === 0 && !cleanWinner) { - cleanWinner = { filePath }; + // The clean ack carries no finding, only the standing steer that a + // silent hook is not a verdict on the design. Repeating it on every + // clean edit spends context to say nothing, so it fires once per file + // per session. The pending ack, which names real unresolved work, is + // deliberately left to repeat. + // + // Quiet mode emits nothing, so it must not consume the ack and leave a + // later non-quiet run in this session silent. + if (quietMode || !shouldEmitAckForFile(filePath, config)) { + cleanWinner = { filePath }; + } else if (ensureFile(cache, sessionId, filePath).cleanAcked) { + // Spent for this file. Remember it for the audit trail, but keep + // scanning: another target in this same event may still be owed an + // ack, and dropping out here would lose it. + cleanAckDeduped = true; + } else { + ensureFile(cache, sessionId, filePath).cleanAcked = true; + cleanWinner = { filePath }; + cleanAckDeduped = false; + } } } @@ -1683,7 +1739,7 @@ export async function runHook({ stdinJson, env = {}, cwd = process.cwd(), now = return result({ emitted: false, error: 'detector-threw', durationMs: Date.now() - started }); } - if (truthy(env.IMPECCABLE_HOOK_QUIET) || config.quiet === true) { + if (quietMode) { return result({ emitted: false, quiet: true, durationMs: Date.now() - started }); } @@ -1721,7 +1777,7 @@ export async function runHook({ stdinJson, env = {}, cwd = process.cwd(), now = }; } - if (cleanWinner && shouldEmitAckForFile(cleanWinner.filePath, config)) { + if (cleanWinner && !cleanAckDeduped && shouldEmitAckForFile(cleanWinner.filePath, config)) { const text = appendDesignSystemNote(renderCleanAck(cleanWinner.filePath, { cwd: projectCwd }), scanOptions); return { exitCode: 0, @@ -1738,15 +1794,29 @@ export async function runHook({ stdinJson, env = {}, cwd = process.cwd(), now = }; } - if (pendingWinner || cleanWinner) { + if (pendingWinner) { return result({ emitted: false, skipped: 'non-ui-ack', durationMs: Date.now() - started }); } + // Distinct from non-ui-ack so the audit log shows noise being suppressed on + // purpose rather than a file the hook could not classify. + if (cleanWinner) { + return result({ emitted: false, skipped: 'non-ui-ack', durationMs: Date.now() - started }); + } + + if (cleanAckDeduped) { + return result({ emitted: false, skipped: 'clean-ack-deduped', durationMs: Date.now() - started }); + } + if (suppressedHit) { return result({ suppressed: true, emitted: false, durationMs: Date.now() - started }); } - return result({ skipped: lastSkip, durationMs: Date.now() - started }); + return result({ + skipped: lastSkip, + ...(lastSkip === 'too-large' ? { bytes: skippedBytes } : {}), + durationMs: Date.now() - started, + }); } catch (err) { return { exitCode: 0, diff --git a/tests/hook.test.mjs b/tests/hook.test.mjs index 0bf3317c7..20b6211a8 100644 --- a/tests/hook.test.mjs +++ b/tests/hook.test.mjs @@ -126,6 +126,30 @@ describe('SENSITIVE_PATH / GENERATED_PATH', () => { assert.ok(GENERATED_PATH.test(p), `expected generated: ${p}`); } }); + + it('skips committed build output living outside dist/', () => { + // Not every generated artifact lands in dist/. Repos commit browser + // bundles and detector copies next to source, and findings against them + // are never actionable. + for (const p of [ + '/x/site/public/js/generated/counts.js', + '/x/src/generated/schema.ts', + '/x/app/generated/api.tsx', + ]) { + assert.ok(GENERATED_PATH.test(p), `expected generated: ${p}`); + } + }); + + it('does not treat authored paths that merely mention generation as generated', () => { + for (const p of [ + '/x/src/generateReport.ts', + '/x/src/generated-utils.ts', + '/x/src/components/CodeGenerator.tsx', + '/x/src/ui/regenerate-button.jsx', + ]) { + assert.ok(!GENERATED_PATH.test(p), `unexpected generated: ${p}`); + } + }); }); describe('readConfig()', () => { @@ -423,6 +447,24 @@ describe('filterFindings()', () => { assert.deepEqual(filtered.map((f) => `${f.antipattern}:${f.line}`), ['overused-font:2', 'bounce-easing:4', 'side-tab:3']); }); + it('honors a specific-value ignoreValues entry for design-system-font-size', () => { + // The rule carries an ignoreValue and the hook's own directive tells the + // agent to waive value-specific findings with `hooks ignore-value`, but + // font-size was missing from the direct-value rule set, so any waiver + // naming an actual size was filtered against an empty extracted value and + // silently did nothing. Only the `*` wildcard worked. + const findings = [ + { ...finding('design-system-font-size', 1), ignoreValue: '0.82rem' }, + { ...finding('design-system-font-size', 2), ignoreValue: '0.9rem' }, + ]; + const filtered = filterFindings(findings, '', '.css', { + ignoreRules: [], + ignoreValues: [{ rule: 'design-system-font-size', value: '0.82rem' }], + limits: DEFAULT_CONFIG.limits, + }); + assert.deepEqual(filtered.map((f) => f.ignoreValue), ['0.9rem']); + }); + it('scopes ignoreValues to file globs when files are provided', () => { const findings = [ { ...finding('design-system-color', 1, { file: '/tmp/project/site/styles/main.css' }), ignoreValue: '#8b5cf6' }, @@ -1683,6 +1725,323 @@ describe('runHook() — cache write gating (issues #344, #305)', () => { }); }); +describe('runHook() — oversized files', () => { + let cwd; + beforeEach(() => { + cwd = mkTmp(); + fs.mkdirSync(path.join(cwd, '.impeccable'), { recursive: true }); + }); + afterEach(() => fs.rmSync(cwd, { recursive: true, force: true })); + + const event = (file) => JSON.stringify({ + session_id: 'sid-1', cwd, hook_event_name: 'PostToolUse', + tool_name: 'Edit', tool_input: { file_path: file }, + }); + + it('skips a file past the size ceiling, since a huge single file is a bundle', async () => { + const file = path.join(cwd, 'bundle.js'); + fs.writeFileSync(file, `/* ${'x'.repeat(200 * 1024)} */`); + const r = await runHook({ + stdinJson: event(file), env: {}, cwd, + detector: fakeDetector([finding('side-tab', 1, { name: 'Side-tab' })]), + }); + assert.ok(!r.audit.emitted); + assert.equal(r.audit.skipped, 'too-large'); + }); + + it('still scans a large but plausibly authored stylesheet', async () => { + const file = path.join(cwd, 'main.css'); + fs.writeFileSync(file, `/* ${'x'.repeat(90 * 1024)} */`); + const r = await runHook({ + stdinJson: event(file), env: {}, cwd, + detector: fakeDetector([finding('side-tab', 1, { name: 'Side-tab' })]), + }); + assert.equal(r.audit.emitted, true); + }); + + // `bytes` describes the file that was skipped. It must never ride along on an + // audit entry whose `file` is something else, in either scan order, and must + // survive the early-continue paths that sit above the size check. + function patchEvent(...files) { + return JSON.stringify({ + session_id: `sid-${files.length}-${files[0]}`, cwd, + hook_event_name: 'PostToolUse', tool_name: 'apply_patch', + tool_input: { + command: `*** Begin Patch\n${files.map(f => `*** Update File: ${f}`).join('\n')}\n*** End Patch`, + }, + }); + } + + it('does not leak a skipped file\'s byte count when the bundle is scanned first', async () => { + const big = path.join(cwd, 'bundle.js'); + const small = path.join(cwd, 'a.css'); + fs.writeFileSync(big, `/* ${'x'.repeat(200 * 1024)} */`); + fs.writeFileSync(small, 'noop'); + const r = await runHook({ + stdinJson: patchEvent(big, small), env: {}, cwd, + detector: fakeDetector([finding('side-tab', 1, { name: 'Side-tab' })]), + }); + assert.match(r.stdout, /a\.css/); + assert.equal(r.audit.bytes, undefined, 'bytes belongs to the skipped file, not this one'); + }); + + it('does not leak a skipped file\'s byte count when the bundle is scanned last', async () => { + const small = path.join(cwd, 'a.css'); + const big = path.join(cwd, 'bundle.js'); + fs.writeFileSync(small, 'noop'); + fs.writeFileSync(big, `/* ${'x'.repeat(200 * 1024)} */`); + const r = await runHook({ + stdinJson: patchEvent(small, big), env: {}, cwd, + detector: fakeDetector([finding('side-tab', 1, { name: 'Side-tab' })]), + }); + assert.match(r.stdout, /a\.css/); + assert.equal(r.audit.bytes, undefined, 'the emitted file is not the oversized one'); + }); + + it('does not leak a byte count past an early-continue target', async () => { + // `generated` is checked before the size gate, so a later generated target + // returns without ever reaching the point where bytes would be cleared. + const big = path.join(cwd, 'bundle.js'); + const gen = path.join(cwd, 'dist', 'Card.tsx'); + fs.writeFileSync(big, `/* ${'x'.repeat(200 * 1024)} */`); + fs.mkdirSync(path.dirname(gen), { recursive: true }); + fs.writeFileSync(gen, 'noop'); + const r = await runHook({ + stdinJson: patchEvent(big, gen), env: {}, cwd, + detector: fakeDetector([finding('side-tab', 1, { name: 'Side-tab' })]), + }); + assert.ok(!r.audit.emitted); + assert.equal(r.audit.bytes, undefined, 'bytes must not describe a different file'); + }); + + it('still records the byte count when the oversized file is the outcome', async () => { + const big = path.join(cwd, 'bundle.js'); + fs.writeFileSync(big, `/* ${'x'.repeat(200 * 1024)} */`); + const r = await runHook({ + stdinJson: patchEvent(big), env: {}, cwd, + detector: fakeDetector([finding('side-tab', 1, { name: 'Side-tab' })]), + }); + assert.equal(r.audit.skipped, 'too-large'); + assert.ok(r.audit.bytes > 200 * 1024, 'the skip reason should still carry its size'); + }); + + it('honors a configured limits.maxFileBytes', async () => { + fs.writeFileSync(path.join(cwd, '.impeccable', 'config.json'), JSON.stringify({ + hook: { limits: { maxFileBytes: 1024 } }, + })); + const file = path.join(cwd, 'small.css'); + fs.writeFileSync(file, `/* ${'x'.repeat(4096)} */`); + const r = await runHook({ + stdinJson: event(file), env: {}, cwd, + detector: fakeDetector([finding('side-tab', 1, { name: 'Side-tab' })]), + }); + assert.equal(r.audit.skipped, 'too-large'); + }); +}); + +describe('runHook() — the session cache tracks the current scan', () => { + let cwd; + beforeEach(() => { + cwd = mkTmp(); + fs.mkdirSync(path.join(cwd, '.impeccable'), { recursive: true }); + }); + afterEach(() => fs.rmSync(cwd, { recursive: true, force: true })); + + function eventFor(file, sessionId = 'sid-1') { + return { + session_id: sessionId, + cwd, + hook_event_name: 'PostToolUse', + tool_name: 'Edit', + tool_input: { file_path: file }, + }; + } + + // A detector whose findings change between runs, so the cache can be + // observed as the file is progressively fixed. + function mutableDetector(initial = []) { + let current = initial; + return { + set(next) { current = next; }, + detectText: () => current.slice(), + detectHtml: () => current.slice(), + }; + } + + function fontFinding(line, value) { + return { ...finding('overused-font', line, { name: 'Overused font' }), ignoreValue: value }; + } + + const run = (file, det) => runHook({ stdinJson: JSON.stringify(eventFor(file)), env: {}, cwd, detector: det }); + + it('counts the current scan in the pending ack, not the session history', async () => { + const file = path.join(cwd, 'a.css'); + fs.writeFileSync(file, 'noop'); + const det = mutableDetector([ + fontFinding(1, 'inter'), fontFinding(2, 'roboto'), fontFinding(3, 'geist'), + ]); + + const r1 = await run(file, det); + assert.match(r1.stdout, /\(3 issue\(s\)\)/); + + // Fix two of the three. The pending ack must not keep naming them. + det.set([fontFinding(1, 'inter')]); + const r2 = await run(file, det); + assert.equal(r2.audit.kind, 'pending'); + assert.match(r2.stdout, /Still has 1 finding\(s\)/); + assert.match(r2.stdout, /overused-font:1:inter/); + assert.ok(!r2.stdout.includes('roboto'), 'must not name a finding that was fixed'); + assert.ok(!r2.stdout.includes('geist'), 'must not name a finding that was fixed'); + }); + + it('reports a reintroduced finding as fresh instead of swallowing it', async () => { + const file = path.join(cwd, 'a.css'); + fs.writeFileSync(file, 'noop'); + const det = mutableDetector([fontFinding(1, 'inter')]); + + const r1 = await run(file, det); + assert.match(r1.stdout, /Design hook findings requiring review/); + + // Fixed: the hook goes clean and must forget the finding. + det.set([]); + const r2 = await run(file, det); + assert.equal(r2.audit.kind, 'clean'); + + // Reintroduced: this is a regression and has to surface as fresh, not be + // deduped against a stale memory of the same key. + det.set([fontFinding(1, 'inter')]); + const r3 = await run(file, det); + assert.equal(r3.audit.emitted, true); + assert.match(r3.stdout, /Design hook findings requiring review/, 'a reintroduced finding must fire again'); + }); + + it('still dedupes an unchanged finding within a session', async () => { + // Guard against over-correcting: forgetting fixed findings must not turn + // every repeat edit back into a full findings dump. + const file = path.join(cwd, 'a.css'); + fs.writeFileSync(file, 'noop'); + const det = mutableDetector([fontFinding(1, 'inter')]); + + await run(file, det); + const r2 = await run(file, det); + assert.equal(r2.audit.kind, 'pending'); + assert.match(r2.stdout, /Still has 1 finding\(s\)/); + }); +}); + +describe('runHook() — clean-ack noise', () => { + let cwd; + beforeEach(() => { + cwd = mkTmp(); + fs.mkdirSync(path.join(cwd, '.impeccable'), { recursive: true }); + }); + afterEach(() => fs.rmSync(cwd, { recursive: true, force: true })); + + const event = (file, sessionId = 'sid-1') => JSON.stringify({ + session_id: sessionId, cwd, hook_event_name: 'PostToolUse', + tool_name: 'Edit', tool_input: { file_path: file }, + }); + + it('emits the clean ack once per file per session, then stays silent', async () => { + const a = path.join(cwd, 'a.css'); + const b = path.join(cwd, 'b.css'); + fs.writeFileSync(a, 'noop'); + fs.writeFileSync(b, 'noop'); + const det = fakeDetector([]); + + const r1 = await runHook({ stdinJson: event(a), env: {}, cwd, detector: det }); + assert.equal(r1.audit.kind, 'clean', 'first clean scan of a file still acks'); + + const r2 = await runHook({ stdinJson: event(a), env: {}, cwd, detector: det }); + assert.ok(!r2.audit.emitted, 'repeat clean scans of the same file stay silent'); + assert.equal(r2.audit.skipped, 'clean-ack-deduped'); + assert.equal(r2.stdout, ''); + + // A different file gets its own first ack. + const r3 = await runHook({ stdinJson: event(b), env: {}, cwd, detector: det }); + assert.equal(r3.audit.kind, 'clean'); + + // A new session starts over, since the steer is per-session context. + const r4 = await runHook({ stdinJson: event(a, 'sid-2'), env: {}, cwd, detector: det }); + assert.equal(r4.audit.kind, 'clean'); + }); + + it('picks a not-yet-acked file when an earlier target was already acked', async () => { + // A multi-file event (apply_patch, MultiEdit) must not lose the ack for a + // file the session has never acked just because an earlier target in the + // same run was already deduped. + const a = path.join(cwd, 'a.css'); + const b = path.join(cwd, 'b.css'); + fs.writeFileSync(a, 'noop'); + fs.writeFileSync(b, 'noop'); + const det = fakeDetector([]); + + // Ack a on its own first. + const r1 = await runHook({ stdinJson: event(a), env: {}, cwd, detector: det }); + assert.equal(r1.audit.kind, 'clean'); + + // Now touch a and b together. a is spent; b has never been acked. + const multi = JSON.stringify({ + session_id: 'sid-1', cwd, hook_event_name: 'PostToolUse', tool_name: 'apply_patch', + tool_input: { + command: `*** Begin Patch\n*** Update File: ${a}\n*** Update File: ${b}\n*** End Patch`, + }, + }); + const r2 = await runHook({ stdinJson: multi, env: {}, cwd, detector: det }); + assert.equal(r2.audit.kind, 'clean', 'b has never been acked and should win'); + assert.match(r2.stdout, /b\.css/); + }); + + it('reports non-ui-ack when the winner was not ack-eligible, even after a dedupe', async () => { + // Mixed multi-target run: one UI file whose ack is already spent, plus a + // non-UI file. Nothing is emitted either way, but the audit reason must + // describe the winner rather than the earlier dedupe. + const css = path.join(cwd, 'a.css'); + const ts = path.join(cwd, 'b.ts'); + fs.writeFileSync(css, 'noop'); + fs.writeFileSync(ts, 'export const a = 1;'); + const det = fakeDetector([]); + + await runHook({ stdinJson: event(css), env: {}, cwd, detector: det }); + + const multi = JSON.stringify({ + session_id: 'sid-1', cwd, hook_event_name: 'PostToolUse', tool_name: 'apply_patch', + tool_input: { + command: `*** Begin Patch\n*** Update File: ${css}\n*** Update File: ${ts}\n*** End Patch`, + }, + }); + const r = await runHook({ stdinJson: multi, env: {}, cwd, detector: det }); + assert.ok(!r.audit.emitted); + assert.equal(r.audit.skipped, 'non-ui-ack'); + }); + + it('does not spend the clean ack while quiet mode is suppressing output', async () => { + // Quiet emits nothing, so it must not consume the once-per-session ack and + // leave a later non-quiet run silent. + const file = path.join(cwd, 'a.css'); + fs.writeFileSync(file, 'noop'); + const det = fakeDetector([]); + + const quiet = await runHook({ stdinJson: event(file), env: { IMPECCABLE_HOOK_QUIET: '1' }, cwd, detector: det }); + assert.ok(!quiet.audit.emitted); + + const loud = await runHook({ stdinJson: event(file), env: {}, cwd, detector: det }); + assert.equal(loud.audit.kind, 'clean', 'the ack must survive a quiet run'); + }); + + it('keeps re-nudging with the pending ack, which is the informative one', async () => { + const file = path.join(cwd, 'a.css'); + fs.writeFileSync(file, 'noop'); + const det = fakeDetector([finding('side-tab', 1, { name: 'Side-tab' })]); + + await runHook({ stdinJson: event(file), env: {}, cwd, detector: det }); + const r2 = await runHook({ stdinJson: event(file), env: {}, cwd, detector: det }); + const r3 = await runHook({ stdinJson: event(file), env: {}, cwd, detector: det }); + assert.equal(r2.audit.kind, 'pending'); + assert.equal(r3.audit.kind, 'pending', 'the unresolved-finding nudge must not be deduped away'); + }); +}); + describe('resolveCacheCwd()', () => { let cwd; beforeEach(() => { cwd = mkTmp(); }); diff --git a/tests/lib/impeccable-config.test.js b/tests/lib/impeccable-config.test.js index bdad2051c..2fb00f28c 100644 --- a/tests/lib/impeccable-config.test.js +++ b/tests/lib/impeccable-config.test.js @@ -238,4 +238,14 @@ describe('cli/lib/impeccable-config', () => { expect(extractFindingIgnoreValue({ antipattern: 'overused-font', snippet: 'https://fonts.googleapis.com/css2?family=Alumni+Sans:wght@700' })).toBe('alumni sans'); expect(extractFindingIgnoreValue({ antipattern: 'bounce-easing', snippet: 'animation: bounce-ball 1s infinite' })).toBe('bounce-ball'); }); + + // This list is duplicated in skill/scripts/hook-lib.mjs. The two had drifted: + // font-size waivers worked in the hook but not in the CLI, so the same config + // filtered differently depending on which entry point read it. + test('extractFindingIgnoreValue covers design-system-font-size, matching the hook', () => { + expect(extractFindingIgnoreValue({ antipattern: 'design-system-font-size', ignoreValue: '0.82rem' })).toBe('0.82rem'); + expect(extractFindingIgnoreValue({ antipattern: 'design-system-radius', ignoreValue: '18px' })).toBe('18px'); + // A rule with no waivable value still extracts nothing. + expect(extractFindingIgnoreValue({ antipattern: 'side-tab', snippet: 'border-left: 4px' })).toBe(''); + }); });