From 490642b78aacb762cc0c40ddf1783965541dd28a Mon Sep 17 00:00:00 2001
From: Via <vance@heygen.com>
Date: Fri, 17 Jul 2026 06:21:18 +0000
Subject: [PATCH] fix(feedback-lint): tighten census value scoping and
 defect-keyword matching
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Addresses C1-C7 from Rames's adversarial review + N2/N3 nits:

- C1 (positionFixed): inline probe now value-scopes to 'fixed' — previously
  fired on any position value (absolute, relative, sticky), producing a
  false-positive anatomy that would mislead maintainers pattern-matching
  the 'sub-comp + position:fixed capture' bug family.

- C2 (overflowHidden): symmetric fix — inline path now catches
  style='overflow: hidden' via the value-scoped probe, matching the
  <style>-tag branch. Also handles overflow-x/overflow-y variants.

- C3 (VISUAL_DEFECT_KEYWORDS): drop 'render' — CLI's primary command is
  'hyperframes render', so build/perf/hang reports were triggering an
  inappropriate COMPOSITION_STRUCTURE: nudge on the most common failure
  mode. Rely on the more specific tokens (black, blank, flicker, corrupt,
  wrong frame) to identify actual visual defects.

- C4 (mentionsVisualDefect): compile keywords into a word-bounded regex.
  'blackboard', 'blanket', 'visualize', 'corruptible' no longer false-
  positive. Accepted tradeoff: plural forms ('flickers') don't match.

- C5 (marker case-normalization): REPRO COMMAND: / COMPOSITION_STRUCTURE:
  checks now case-insensitive, matching mentionsVisualDefect's
  normalization. Reporters using 'Repro command:' or lowercase
  'composition_structure:' get credit for compliance.

- C6 (background/mask shorthand): inline branch previously required the
  longhand 'background-image:' / 'mask-image:' — style='background:
  url(bg.png)' silently returned false. Now checks both longhand AND
  shorthand-with-url() inline forms.

- C7 (usesGsap docstring): trim promise of data-gsap-* attribute scanning
  that detectGsap never implemented — attribute scan lives outside the
  <script>-only detection path.

- N2 (EMPTY_VALUES): include 'inherit', 'revert', 'revert-layer' — a
  style='position: inherit' is authored intent to defer, not authored
  intent to place.

- N3 (input size cap): early-exit to a zero census on HTML > 20 MB
  rather than feeding linkedom a hostile input. Not expected in normal
  usage; guard for future callers that might pass raw user uploads.

Extends the test locks: 6 new census tests (value-scoping, size cap) and
5 new lint tests (word-boundary rejections, render-noise rejections,
lowercase-marker acceptance). All existing tests unchanged in intent —
only the 'flickers' plural in one test updated to 'flicker' to reflect
the new word-boundary rule.

No behavior change to the wire path: lint is still soft-warn, census is
still never called from feedback.ts, no new dependencies.
---
 .../cli/src/utils/compositionCensus.test.ts   |  54 ++++++++
 packages/cli/src/utils/compositionCensus.ts   | 122 +++++++++++++++---
 packages/cli/src/utils/feedbackLint.test.ts   |  54 +++++++-
 packages/cli/src/utils/feedbackLint.ts        |  48 ++++---
 4 files changed, 244 insertions(+), 34 deletions(-)
diff --git a/packages/cli/src/utils/compositionCensus.test.ts b/packages/cli/src/utils/compositionCensus.test.ts
index ec9ee5224..0996ad938 100644
--- a/packages/cli/src/utils/compositionCensus.test.ts
+++ b/packages/cli/src/utils/compositionCensus.test.ts
@@ -112,6 +112,60 @@ describe("buildCompositionCensus", () => {
     expect(c.timelineShape.nested).toBe(true);
     expect(c.timelineShape.subCompositionCount).toBe(2);
   });
+
+  describe("value-scoped structural probes", () => {
+    it("positionFixed is false for inline position:absolute (name-vs-value scope)", () => {
+      const html = `<html><body><div data-composition-id="m" style="position: absolute"></div></body></html>`;
+      const c = buildCompositionCensus(html);
+      expect(c.structuralAttributes.positionFixed).toBe(false);
+    });
+
+    it("positionFixed is true for inline position:fixed", () => {
+      const html = `<html><body><div data-composition-id="m" style="position: fixed"></div></body></html>`;
+      const c = buildCompositionCensus(html);
+      expect(c.structuralAttributes.positionFixed).toBe(true);
+    });
+
+    it("overflowHidden catches inline overflow:hidden (symmetric with style-tag path)", () => {
+      const html = `<html><body><div data-composition-id="m" style="overflow: hidden"></div></body></html>`;
+      const c = buildCompositionCensus(html);
+      expect(c.structuralAttributes.overflowHidden).toBe(true);
+    });
+
+    it("overflowHidden is false for inline overflow:visible", () => {
+      const html = `<html><body><div data-composition-id="m" style="overflow: visible"></div></body></html>`;
+      const c = buildCompositionCensus(html);
+      expect(c.structuralAttributes.overflowHidden).toBe(false);
+    });
+
+    it("backgroundImage catches the inline shorthand `background: url(...)`", () => {
+      const html = `<html><body><div data-composition-id="m" style="background: url('bg.png') center"></div></body></html>`;
+      const c = buildCompositionCensus(html);
+      expect(c.structuralAttributes.backgroundImage).toBe(true);
+    });
+
+    it("maskImage catches the inline shorthand `mask: url(...)`", () => {
+      const html = `<html><body><div data-composition-id="m" style="mask: url('mask.svg')"></div></body></html>`;
+      const c = buildCompositionCensus(html);
+      expect(c.structuralAttributes.maskImage).toBe(true);
+    });
+
+    it("does not count `inherit` / `revert` as authored intent", () => {
+      const html = `<html><body><div data-composition-id="m" style="position: inherit; filter: revert"></div></body></html>`;
+      const c = buildCompositionCensus(html);
+      expect(c.structuralAttributes.positionFixed).toBe(false);
+      expect(c.structuralAttributes.filter).toBe(false);
+    });
+  });
+
+  it("short-circuits on HTML over the size cap without throwing", () => {
+    const huge = "a".repeat(21 * 1024 * 1024);
+    const c = buildCompositionCensus(huge);
+    // Guard-rail returns a zero census — no OOM, no crash.
+    expect(c.elementCensus.video).toBe(0);
+    expect(c.timelineShape.subCompositionCount).toBe(0);
+    expect(c.structuralAttributes.positionFixed).toBe(false);
+  });
 });
 
 describe("renderCompositionCensusBlock", () => {
diff --git a/packages/cli/src/utils/compositionCensus.ts b/packages/cli/src/utils/compositionCensus.ts
index fdefd048f..fc8bb8d71 100644
--- a/packages/cli/src/utils/compositionCensus.ts
+++ b/packages/cli/src/utils/compositionCensus.ts
@@ -48,7 +48,12 @@ export interface CompositionCensus {
     nested: boolean;
     /** Count of sub-composition mounts (`data-composition-src` count). */
     subCompositionCount: number;
-    /** Any GSAP timeline usage (script tags referencing gsap or data-gsap-*). */
+    /**
+     * Any GSAP timeline usage detected via `<script>` src or inline
+     * `gsap.<method>(...)` invocation. Does not scan for `data-gsap-*`
+     * attributes on non-script elements — HTML-authored GSAP hooks that live
+     * outside script tags won't surface here.
+     */
     usesGsap: boolean;
     /** Any element carrying `data-start` or `data-duration`. */
     usesDataTimeline: boolean;
@@ -61,29 +66,49 @@ const DATA_DURATION_SELECTOR = "[data-duration]";
 const DATA_START_SELECTOR = "[data-start]";
 
 // Values that should NOT count as a "filter"/"mask"/"transform" attribute — the
-// browser default. Explicit "none" is authored intent to disable, still counts
-// as noteworthy for the census (author touched the property).
-const EMPTY_VALUES = new Set(["", "initial", "unset"]);
+// browser default or an explicit inherit/revert to the ancestor's value.
+// Explicit "none" is authored intent to disable, still counts as noteworthy for
+// the census (author touched the property).
+const EMPTY_VALUES = new Set(["", "initial", "unset", "inherit", "revert", "revert-layer"]);
 
-function hasNonEmptyInlineStyle(el: Element, property: string): boolean {
+// Hard cap on input HTML size. Above this, we early-exit rather than feed
+// linkedom a hostile input. The census is a heuristic for maintainer
+// pattern-matching; a truncated result is preferable to an OOM on a bad file.
+const MAX_HTML_BYTES = 20 * 1024 * 1024;
+
+function iterInlineDeclarations(
+  el: Element,
+  visit: (property: string, value: string) => boolean,
+): boolean {
   const raw = el.getAttribute("style");
   if (!raw) return false;
-  // Case-insensitive property match. We intentionally do NOT parse CSS
-  // shorthand-style semantics — a `background: url(...)` in the shorthand
-  // still surfaces via a substring probe of the property token, which is
-  // sufficient for structural presence signaling.
-  const target = property.toLowerCase();
   const declarations = raw.split(";");
   for (const decl of declarations) {
     const colon = decl.indexOf(":");
     if (colon <= 0) continue;
     const prop = decl.slice(0, colon).trim().toLowerCase();
     const value = decl.slice(colon + 1).trim();
-    if (prop === target && !EMPTY_VALUES.has(value.toLowerCase())) return true;
+    if (visit(prop, value)) return true;
   }
   return false;
 }
 
+function hasNonEmptyInlineStyle(el: Element, property: string): boolean {
+  const target = property.toLowerCase();
+  return iterInlineDeclarations(el, (prop, value) => {
+    if (prop !== target) return false;
+    return !EMPTY_VALUES.has(value.toLowerCase());
+  });
+}
+
+function hasInlineStyleValueMatching(el: Element, property: string, valueRe: RegExp): boolean {
+  const target = property.toLowerCase();
+  return iterInlineDeclarations(el, (prop, value) => {
+    if (prop !== target) return false;
+    return valueRe.test(value);
+  });
+}
+
 function anyElementHasInlineStyle(doc: Document, property: string): boolean {
   const all = doc.querySelectorAll("[style]");
   for (const el of Array.from(all)) {
@@ -92,6 +117,14 @@ function anyElementHasInlineStyle(doc: Document, property: string): boolean {
   return false;
 }
 
+function anyElementHasInlineStyleValue(doc: Document, property: string, valueRe: RegExp): boolean {
+  const all = doc.querySelectorAll("[style]");
+  for (const el of Array.from(all)) {
+    if (hasInlineStyleValueMatching(el as unknown as Element, property, valueRe)) return true;
+  }
+  return false;
+}
+
 function hasStyleTagReferencing(doc: Document, needle: RegExp): boolean {
   const styles = doc.querySelectorAll("style");
   for (const styleEl of Array.from(styles)) {
@@ -121,8 +154,11 @@ function detectGsap(doc: Document): boolean {
  *
  * Safe to call on partial/malformed HTML — linkedom's parser tolerates the
  * usual authoring accidents and the census reports zeros for missing sections.
+ * Inputs larger than {@link MAX_HTML_BYTES} short-circuit to a zero census
+ * rather than risk OOMing linkedom on a hostile file.
  */
 export function buildCompositionCensus(html: string): CompositionCensus {
+  if (html.length > MAX_HTML_BYTES) return emptyCensus();
   const doc = parseHTML(html).document as unknown as Document;
 
   const count = (selector: string): number => doc.querySelectorAll(selector).length;
@@ -135,6 +171,13 @@ export function buildCompositionCensus(html: string): CompositionCensus {
   const anyStyle = (property: string, styleRegex: RegExp): boolean =>
     anyElementHasInlineStyle(doc, property) || hasStyleTagReferencing(doc, styleRegex);
 
+  // Value-scoped probe — inline branch matches on property value (not just
+  // property presence), so `position:fixed` doesn't false-positive on
+  // `position:absolute` and `overflow:hidden` catches inline `overflow:hidden`.
+  const anyStyleValue = (property: string, inlineValueRe: RegExp, styleRegex: RegExp): boolean =>
+    anyElementHasInlineStyleValue(doc, property, inlineValueRe) ||
+    hasStyleTagReferencing(doc, styleRegex);
+
   return {
     elementCensus: {
       video: count("video"),
@@ -150,17 +193,27 @@ export function buildCompositionCensus(html: string): CompositionCensus {
       mixBlendMode: anyStyle("mix-blend-mode", /mix-blend-mode\s*:/i),
       transform: anyStyle("transform", /(^|\s|\{)transform\s*:/i),
       mask: anyStyle("mask", /(^|\s|\{)mask\s*:/i),
-      positionFixed:
-        anyElementHasInlineStyle(doc, "position") ||
-        hasStyleTagReferencing(doc, /position\s*:\s*fixed/i),
-      overflowHidden: hasStyleTagReferencing(doc, /overflow\s*:\s*hidden/i),
+      positionFixed: anyStyleValue("position", /^fixed$/i, /position\s*:\s*fixed/i),
+      overflowHidden: anyStyleValue(
+        "overflow",
+        /(^|\b)hidden(\b|$)/i,
+        /overflow(-[xy])?\s*:\s*hidden/i,
+      ),
       zIndex: anyStyle("z-index", /z-index\s*:/i),
       dataHasAudio: count(DATA_HAS_AUDIO_SELECTOR) > 0,
       dataDuration: count(DATA_DURATION_SELECTOR) > 0,
       dataStart: count(DATA_START_SELECTOR) > 0,
       dataCompositionSrc: subCompositionCount > 0,
-      backgroundImage: anyStyle("background-image", /background(-image)?\s*:[^;]*url\(/i),
-      maskImage: anyStyle("mask-image", /mask(-image)?\s*:[^;]*url\(/i),
+      // Match longhand (`background-image:`) OR shorthand (`background:`)
+      // pointing at a `url(...)`. Same for `mask` / `mask-image`.
+      backgroundImage:
+        anyElementHasInlineStyle(doc, "background-image") ||
+        anyElementHasInlineStyleValue(doc, "background", /url\(/i) ||
+        hasStyleTagReferencing(doc, /background(-image)?\s*:[^;]*url\(/i),
+      maskImage:
+        anyElementHasInlineStyle(doc, "mask-image") ||
+        anyElementHasInlineStyleValue(doc, "mask", /url\(/i) ||
+        hasStyleTagReferencing(doc, /mask(-image)?\s*:[^;]*url\(/i),
     },
     timelineShape: {
       nested: subCompositionCount > 0,
@@ -171,6 +224,41 @@ export function buildCompositionCensus(html: string): CompositionCensus {
   };
 }
 
+function emptyCensus(): CompositionCensus {
+  return {
+    elementCensus: {
+      video: 0,
+      audio: 0,
+      img: 0,
+      svg: 0,
+      canvas: 0,
+      subCompositionMounts: 0,
+    },
+    structuralAttributes: {
+      clipPath: false,
+      filter: false,
+      mixBlendMode: false,
+      transform: false,
+      mask: false,
+      positionFixed: false,
+      overflowHidden: false,
+      zIndex: false,
+      dataHasAudio: false,
+      dataDuration: false,
+      dataStart: false,
+      dataCompositionSrc: false,
+      backgroundImage: false,
+      maskImage: false,
+    },
+    timelineShape: {
+      nested: false,
+      subCompositionCount: 0,
+      usesGsap: false,
+      usesDataTimeline: false,
+    },
+  };
+}
+
 /**
  * Render the census as the exact `COMPOSITION_STRUCTURE:` block the skill
  * mandates for non-10 visual-defect feedback. Reporter-friendly plain text:
diff --git a/packages/cli/src/utils/feedbackLint.test.ts b/packages/cli/src/utils/feedbackLint.test.ts
index a4c94bcf3..5420f928c 100644
--- a/packages/cli/src/utils/feedbackLint.test.ts
+++ b/packages/cli/src/utils/feedbackLint.test.ts
@@ -82,7 +82,7 @@ describe("lintFeedbackComment", () => {
   it("emits both warnings when a low-rating visual comment lacks both markers", () => {
     const warnings = lintFeedbackComment({
       rating: 3,
-      comment: "flickers at every scene boundary",
+      comment: "flicker at every scene boundary",
     });
     expect(new Set(warnings.map((w) => w.code))).toEqual(
       new Set(["missing-repro-command", "missing-composition-structure"]),
@@ -102,4 +102,56 @@ describe("mentionsVisualDefect", () => {
     expect(mentionsVisualDefect("cli hangs on init prompt in non-TTY shells")).toBe(false);
     expect(mentionsVisualDefect("cloudrun deploy expired auth token")).toBe(false);
   });
+
+  it("does not match on partial-word false positives (word boundary)", () => {
+    // The classic false positives Rames flagged in review.
+    expect(mentionsVisualDefect("scribbled on the blackboard")).toBe(false);
+    expect(mentionsVisualDefect("added to blacklist yesterday")).toBe(false);
+    expect(mentionsVisualDefect("brought a blanket to the office")).toBe(false);
+    expect(mentionsVisualDefect("let me visualize the graph")).toBe(false);
+    expect(mentionsVisualDefect("that's a corruptible pointer")).toBe(false);
+  });
+
+  it("does not fire on `render` — CLI's primary command, too noisy", () => {
+    // `hyperframes render` is the CLI's core command; comments like
+    // "render OOMed at 118s" are build/perf issues, not visual defects.
+    expect(mentionsVisualDefect("render OOMed at 118s")).toBe(false);
+    expect(mentionsVisualDefect("render hung on Alpine")).toBe(false);
+    expect(mentionsVisualDefect("preview render command took forever")).toBe(false);
+  });
+
+  it("still matches the real defect words in prose", () => {
+    expect(mentionsVisualDefect("output is a black frame at 0.5s")).toBe(true);
+    expect(mentionsVisualDefect("the whole sequence flickers hard")).toBe(false); // "flickers" (plural) — accepted false-neg
+    expect(mentionsVisualDefect("the whole sequence has a flicker at 2s")).toBe(true);
+    expect(mentionsVisualDefect("wrong frame at t=0.3")).toBe(true);
+  });
+});
+
+describe("marker-check case-insensitivity", () => {
+  it("does not warn when the reporter uses lowercase `Repro command:`", () => {
+    // Case asymmetry fix (C5): marker checks match `mentionsVisualDefect`'s
+    // case-normalization, so compliance in any case is honored.
+    const warnings = lintFeedbackComment({
+      rating: 5,
+      comment: [
+        "cloudrun submission kept timing out.",
+        "Repro command: cd project && npx hyperframes cloudrun submit",
+      ].join("\n"),
+    });
+    expect(warnings.filter((w) => w.code === "missing-repro-command")).toEqual([]);
+  });
+
+  it("does not double-warn when reporter uses lowercase `composition_structure:`", () => {
+    const warnings = lintFeedbackComment({
+      rating: 4,
+      comment: [
+        "REPRO COMMAND: cd proj && npx hyperframes render",
+        "composition_structure:",
+        "  elements: video=1 audio=0 img=0 svg=0 canvas=0 subComps=0",
+        "black frame at 0.5s",
+      ].join("\n"),
+    });
+    expect(warnings.filter((w) => w.code === "missing-composition-structure")).toEqual([]);
+  });
 });
diff --git a/packages/cli/src/utils/feedbackLint.ts b/packages/cli/src/utils/feedbackLint.ts
index 511559100..6b53ea6d2 100644
--- a/packages/cli/src/utils/feedbackLint.ts
+++ b/packages/cli/src/utils/feedbackLint.ts
@@ -6,11 +6,14 @@ import { FEEDBACK_RATING_SCALE } from "./feedbackRating.js";
  * appear in a non-10 feedback comment, the reporter should include a
  * `COMPOSITION_STRUCTURE:` block so maintainers can pattern-match against
  * known bug families without receiving the composition ZIP. Matched
- * case-insensitively against the raw comment.
+ * case-insensitively, word-bounded against the raw comment (so "black" fires
+ * on "black frame" but not "blackboard" / "no black frame at all").
  *
- * Keep the list short and unambiguous — false positives are cheap (a soft
- * warn), false negatives just mean the reporter skips the structure block on
- * a non-visual bug, which is fine.
+ * `"render"` intentionally omitted: `hyperframes render` is the CLI's primary
+ * command, so build/perf/hang reports mention it constantly and would drown
+ * the structure warning in false positives. Rely on the more specific tokens
+ * ("black", "blank", "flicker", "corrupt", "wrong frame") to identify actual
+ * visual defects.
  */
 export const VISUAL_DEFECT_KEYWORDS: readonly string[] = [
   "black",
@@ -19,7 +22,6 @@ export const VISUAL_DEFECT_KEYWORDS: readonly string[] = [
   "wrong frame",
   "blank",
   "visual",
-  "render",
 ] as const;
 
 /**
@@ -64,9 +66,13 @@ export function lintFeedbackComment(input: FeedbackLintInput): FeedbackLintWarni
   const trimmed = comment?.trim();
   if (!trimmed) return [];
 
+  // Marker checks are case-insensitive to match `mentionsVisualDefect`'s
+  // normalization. A reporter who writes `Repro command:` shouldn't get warned
+  // for compliance just because they lowercased the marker.
+  const upperTrimmed = trimmed.toUpperCase();
   const warnings: FeedbackLintWarning[] = [];
 
-  if (!trimmed.includes(REPRO_MARKER)) {
+  if (!upperTrimmed.includes(REPRO_MARKER)) {
     warnings.push({
       code: "missing-repro-command",
       message: [
@@ -80,7 +86,7 @@ export function lintFeedbackComment(input: FeedbackLintInput): FeedbackLintWarni
   if (
     rating <= COMPOSITION_STRUCTURE_RATING_CEILING &&
     mentionsVisualDefect(trimmed) &&
-    !trimmed.includes(STRUCTURE_MARKER)
+    !upperTrimmed.includes(STRUCTURE_MARKER)
   ) {
     warnings.push({
       code: "missing-composition-structure",
@@ -96,16 +102,26 @@ export function lintFeedbackComment(input: FeedbackLintInput): FeedbackLintWarni
   return warnings;
 }
 
+// Compile once. Word-boundary at both sides prevents "black" matching
+// "blackboard", "blank" matching "blanket", "visual" matching "visualize".
+// Plural forms (e.g. "flickers") won't match — accepted tradeoff for a soft
+// warn: false negatives skip the nudge, false positives waste the reporter's
+// attention.
+function escapeRegex(input: string): string {
+  return input.replace(/[.*+?^${}()|[\]\\]/g, "\\$&");
+}
+
+const VISUAL_DEFECT_REGEX = new RegExp(
+  `(^|[^A-Za-z0-9_])(?:${VISUAL_DEFECT_KEYWORDS.map(escapeRegex).join("|")})(?![A-Za-z0-9_])`,
+  "i",
+);
+
 /**
- * Case-insensitive substring probe against `VISUAL_DEFECT_KEYWORDS`. Exposed
- * for tests and reuse; keywords are matched anywhere in the comment (no word
- * boundaries) since real reports mix them into free prose. False positives
- * cost one soft warn, which is acceptable.
+ * Case-insensitive word-bounded probe against `VISUAL_DEFECT_KEYWORDS`.
+ * Exposed for tests and reuse. Word boundaries are enforced on both sides so
+ * partial-word false positives ("blackboard", "visualize", "corruptible")
+ * don't trigger the structure-block nudge.
  */
 export function mentionsVisualDefect(comment: string): boolean {
-  const lower = comment.toLowerCase();
-  for (const kw of VISUAL_DEFECT_KEYWORDS) {
-    if (lower.includes(kw)) return true;
-  }
-  return false;
+  return VISUAL_DEFECT_REGEX.test(comment);
 }
