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 <noreply@anthropic.com>

* 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 <noreply@anthropic.com>

* 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 <rule> <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 <noreply@anthropic.com>

* 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 <noreply@anthropic.com>

* 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 <noreply@anthropic.com>

---------

Co-authored-by: Claude <noreply@anthropic.com>

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,