fix(feedback-lint): tighten census value scoping and defect-keyword matching

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.
2026-09-08 02:36:10 +00:00 · 2026-07-17 06:22:00 +00:00
parent 8f90fd9ec1
commit 490642b78a
4 changed files with 244 additions and 34 deletions
@@ -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", () => {
@@ -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:
@@ -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([]);
+  });
 });
@@ -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);
 }