fix(producer): address R2 review findings — element undercount and kill-switch attribution

Two review-blocking issues from Miguel's R2 pass (both confirmed by
running the counterexamples directly):

1. countElementTags still undercounted unboundedly. The void-element fix
   covered HTML tags but SVG elements (<circle/>, <path/>, ...) are
   neither closing-tag-shaped nor in the HTML void list, so
   "<circle/>".repeat(40000) reported 0 — the same failure class as the
   original <img> counterexample, and the exact shape of comp the
   measured 1.8x regression case is made of. A 2500 ceiling cannot bound
   an error with no bound of its own. Added a third alternative matching
   any self-closing tag; verified it doesn't false-positive on the
   adversarial minified-JS case (unspaced "<b/c>", which reads like a
   tag open but never contains the literal two-char "/>" the alt requires).

2. HF_DE_SHORT_MAX_ELEMENTS=0 (the documented kill switch) still reported
   deShortBand: "skipped_elements" for every in-band render instead of
   undefined — attributing "comp too large" when the real cause was "band
   disabled," which would have polluted the DiD control cohort with
   kill-switched renders and made the post-flip read look like the
   ceiling was too tight. Extracted the attribution logic into
   resolveDeShortBand(), a pure function gated on bandEnabled
   (deShortBandMaxElements > 0) as well as decisiveness — and made it
   independently unit-testable, since the inline version could only be
   exercised by a full render pipeline run.

Also from this review round: the inversion log line could report
"400 frames >= 900" for a band-routed inversion; it now names the floor
that actually decided the render. Tightened shortBand's type to match its
peer fields' unions (workerInversion, parallelRouter) instead of a bare
string. Clarified the tween-count merge docblock, which claimed workers
always agree (semantically true) while the code takes a defensive max
(in case one doesn't) — the two aren't in conflict, but the comment read
as if they were.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

packages/producer/src/services/render/perfSummary.ts

@@ -83,7 +83,7 @@ export interface DrawElementPerfInput {
   /** Rough compiled-composition element count — gate variable for the short-comp inversion band. */
   compositionElementCount?: number;
   /** Short-comp band decision when the band was DECISIVE: "applied" (inverts once HF_DE_SHORT_BAND_ROUTE is on; counterfactual in the baseline release) | "skipped_elements" (element ceiling was the only blocker); unset when the band could not have affected this render. */
-  shortBand?: string;
+  shortBand?: "applied" | "skipped_elements";
   parallelRouter?: "routed" | "reverted";
   /** Auto-resolved worker count before the router pinned it to 3 (set only when the router fired). */
   preRouterWorkers?: number;

packages/producer/src/services/renderOrchestrator.test.ts

@@ -37,6 +37,7 @@ import {
   countElementTags,
   envInt,
   mergeWorkerInitObservability,
+  resolveDeShortBand,
   shouldPreferParallelDrawElement,
   shouldPreferSingleWorkerDrawElement,
   shouldStreamParallelCapture,
@@ -1762,6 +1763,67 @@ describe("shouldPreferSingleWorkerDrawElement (DE priority inversion)", () => {
     });
   });
 
+  describe("resolveDeShortBand", () => {
+    it("reports applied only when decisive and the element ceiling cleared", () => {
+      expect(
+        resolveDeShortBand({
+          invertAtBaseFloor: false,
+          invertAtBandFloor: true,
+          bandEnabled: true,
+          bandOpen: true,
+        }),
+      ).toBe("applied");
+    });
+
+    it("reports skipped_elements when decisive but the comp is oversized", () => {
+      expect(
+        resolveDeShortBand({
+          invertAtBaseFloor: false,
+          invertAtBandFloor: true,
+          bandEnabled: true,
+          bandOpen: false,
+        }),
+      ).toBe("skipped_elements");
+    });
+
+    it("is undefined when the base floor already inverts — the band changed nothing", () => {
+      expect(
+        resolveDeShortBand({
+          invertAtBaseFloor: true,
+          invertAtBandFloor: true,
+          bandEnabled: true,
+          bandOpen: true,
+        }),
+      ).toBeUndefined();
+    });
+
+    it("is undefined when neither floor inverts — the render was ineligible for some other reason", () => {
+      expect(
+        resolveDeShortBand({
+          invertAtBaseFloor: false,
+          invertAtBandFloor: false,
+          bandEnabled: true,
+          bandOpen: true,
+        }),
+      ).toBeUndefined();
+    });
+
+    // Review finding (R1 + R2, both reviewers): HF_DE_SHORT_MAX_ELEMENTS=0
+    // must read as "band disabled" (undefined), never "comp too large"
+    // (skipped_elements) — the latter would poison the DiD control cohort by
+    // mislabeling a kill-switch event as a real oversize measurement.
+    it("HF_DE_SHORT_MAX_ELEMENTS=0 (bandEnabled=false) reports undefined even when the render would otherwise be decisive", () => {
+      expect(
+        resolveDeShortBand({
+          invertAtBaseFloor: false,
+          invertAtBandFloor: true, // an otherwise-eligible in-band render
+          bandEnabled: false, // the kill switch
+          bandOpen: false, // deShortBandOpen also false when the switch is off
+        }),
+      ).toBeUndefined();
+    });
+  });
+
   describe("mergeWorkerInitObservability", () => {
     it("max-merges across workers and ignores workers that reported nothing", () => {
       expect(
@@ -1789,6 +1851,32 @@ describe("shouldPreferSingleWorkerDrawElement (DE priority inversion)", () => {
       expect(countElementTags('<img src="a.png"><IMG SRC="b.png">')).toBe(2);
     });
 
+    // Review-flagged blocker (v1): SVG elements are neither closing-tag-shaped
+    // nor in the HTML void list, so a self-closing-SVG-heavy comp read as
+    // element count 0 — an UNBOUNDED undercount, the same failure class as
+    // the original <img> counterexample, and the exact shape of comp the
+    // measured 1.8x regression case is made of. The ceiling cannot bound an
+    // error that has no bound of its own.
+    it("counts self-closing SVG elements — the 40k-node regression case must not read as empty", () => {
+      expect(countElementTags("<circle/>".repeat(40000))).toBe(40000);
+      expect(countElementTags('<path d="M0 0 L1 1" stroke="red" />')).toBe(1);
+      expect(countElementTags("<feGaussianBlur stdDeviation='2'/>")).toBe(1);
+    });
+
+    it("does not double-count a self-closed void element (still just 1)", () => {
+      expect(countElementTags('<img src="a.png"/>')).toBe(1);
+      expect(countElementTags('<img src="a.png" />')).toBe(1);
+    });
+
+    it("does not false-positive on minified JS division-after-comparison (the self-closing alt's real risk)", () => {
+      // Unspaced "<b/c>" is the adversarial case: "<" IS immediately
+      // followed by a letter, so the generic self-closing alt gets as far as
+      // starting a match — but it still requires the literal two-char "/>"
+      // sequence, and here a "c" sits between the "/" and the ">", so
+      // backtracking never finds one and it correctly fails to match.
+      expect(countElementTags("if(a<b/c>d){}")).toBe(0);
+    });
+
     it("does not false-positive on inline-script comparisons or void-prefixed words", () => {
       // Only the </script> closer counts: "<breadth" and "<imgWidth" hit the
       // br/img alternatives but fail the \b word boundary (next char is a

packages/producer/src/services/renderOrchestrator.ts

@@ -495,7 +495,7 @@ export interface RenderPerfSummary {
     /** Rough compiled-composition element count — the variable the short-comp inversion band is gated on. Always set. */
     compositionElementCount?: number;
     /** Short-comp band attribution: "applied" | "skipped_elements"; unset when the frame count made the band irrelevant. */
-    shortBand?: string;
+    shortBand?: "applied" | "skipped_elements";
     /** DE parallel-router outcome: "routed" (fired, held), "reverted" (fired, self-verify retry rolled back), "none". Mutually exclusive with workerInversion. */
     parallelRouter?: string;
     /** Worker count the auto-resolution chose BEFORE the router pinned it to 3 — the single-worker-inversion counterfactual. Only set when the router fired. */
@@ -1248,12 +1248,27 @@ export function envInt(name: string, fallback: number): number {
  * matter here — the 20k-40k node ones — is the most expensive case. Precision
  * is not needed. It feeds a threshold whose measured crossover is ~3.9k and
  * whose default sits at 2500, so tag counting is comfortably inside the
- * margin. Closing tags plus HTML void elements (`<img>`, `<br>`, …) — voids
- * matter because they skew EXPENSIVE to paint (images), and counting only
- * closers would read an image gallery as a tiny comp and open the band on
- * exactly the content most likely to lose it. Opening tags are deliberately
- * NOT counted: compiled comps embed inline scripts where `a < b` would
- * false-positive. Self-closing SVG children still undercount; acceptable.
+ * margin — PROVIDED the count is not unboundedly low for some real content
+ * shape. Three sources are counted, each catching a case the others miss:
+ *
+ *   1. Closing tags (`</div>`) — the base count for ordinary HTML.
+ *   2. Named HTML void elements (`<img>`, `<br>`, …), bare or self-closed —
+ *      voids matter because they skew EXPENSIVE to paint (images), and
+ *      counting only closers would read an image gallery as a tiny comp and
+ *      open the band on exactly the content most likely to lose it.
+ *   3. Any self-closing tag (`<circle/>`, `<path d="…"/>`) — SVG's own
+ *      elements are neither closing-tag-shaped nor in the void list, so
+ *      without this a self-closing-SVG-heavy composition (`<circle/>` x 40k)
+ *      counted as ZERO — an unbounded undercount, not a rounding error, and
+ *      exactly the shape of comp the 1.8x regression case is made of
+ *      (review finding: the ceiling cannot compensate for an error with no
+ *      bound).
+ *
+ * Opening (non-self-closing, non-void) tags are deliberately NOT counted:
+ * compiled comps embed inline scripts, and `a < b` or `x <breadth` would
+ * false-positive on a bare `<letter` scan. All three counted forms require a
+ * literal closing marker (`</`, a void name at a word boundary, or `/>`), so
+ * ordinary JS comparisons and divisions don't qualify — verified by test.
  *
  * These semantics are FROZEN while the short-band baseline is being read —
  * the fleet distribution recorded by the baseline release must be measured
@@ -1261,7 +1276,7 @@ export function envInt(name: string, fallback: number): number {
  */
 export function countElementTags(html: string): number {
   const matches = html.match(
-    /<\/[a-zA-Z]|<(?:img|br|hr|input|source|track|area|base|col|embed|link|meta|param|wbr)\b/gi,
+    /<\/[a-zA-Z]|<(?:img|br|hr|input|source|track|area|base|col|embed|link|meta|param|wbr)\b|<[a-zA-Z][-a-zA-Z0-9]*\b[^>]*\/>/gi,
   );
   return matches === null ? 0 : matches.length;
 }
@@ -1271,8 +1286,9 @@ export function countElementTags(html: string): number {
  * success-path channel for PARALLEL renders, whose worker console buffers
  * (and so the `[FrameCapture:INIT]` line) only propagate on failure. Max
  * matches summarizeInitObservability's own multi-session semantics: keep the
- * worst observed startup cost, and tween count is per-composition so any
- * worker's reading is the reading.
+ * worst observed startup cost for duration. Tween count is per-composition,
+ * so workers should agree — max is a defensive read against a worker that
+ * initializes before the timeline is fully wired, not an expected disagreement.
  */
 export function mergeWorkerInitObservability(
   perfs: ReadonlyArray<{ initDurationMs?: number; initTweenCount?: number }>,
@@ -1295,6 +1311,34 @@ export function mergeWorkerInitObservability(
   return { initDurationMs, tweenCount };
 }
 
+/**
+ * The short-comp band's attribution decision, extracted as a pure function so
+ * the gating fixes below are independently testable rather than living inline
+ * where only a full render pipeline run could exercise them.
+ *
+ * "applied" / "skipped_elements" are emitted ONLY when the band is DECISIVE —
+ * every other inversion-eligibility condition already passed (both floor
+ * evaluations agree on everything except which floor they used) and the band
+ * floor alone flipped the answer. `bandEnabled` gates that decisiveness
+ * itself: `HF_DE_SHORT_MAX_ELEMENTS=0` is a documented kill switch (symmetric
+ * with `HF_DE_SHORT_MIN_FRAMES=0`, which already disables via the predicate's
+ * own `minFrames > 0` guard), and without this gate a fired kill switch left
+ * every in-band render decisive against a real floor comparison — reporting
+ * "skipped_elements" (comp too large) instead of undefined (band disabled)
+ * and corrupting the DiD control cohort with kill-switched renders (review
+ * finding).
+ */
+export function resolveDeShortBand(args: {
+  invertAtBaseFloor: boolean;
+  invertAtBandFloor: boolean;
+  bandEnabled: boolean;
+  bandOpen: boolean;
+}): "applied" | "skipped_elements" | undefined {
+  const decisive = args.bandEnabled && args.invertAtBandFloor && !args.invertAtBaseFloor;
+  if (!decisive) return undefined;
+  return args.bandOpen ? "applied" : "skipped_elements";
+}
+
 /**
  * DE priority inversion predicate: should an AUTO-resolved multi-worker render
  * drop to single-worker verified drawElement streaming?
@@ -2496,9 +2540,17 @@ async function executeRenderPipeline(input: {
     const deShortBandMinFrames = envInt("HF_DE_SHORT_MIN_FRAMES", 250);
     const deShortBandMaxElements = envInt("HF_DE_SHORT_MAX_ELEMENTS", 2500);
     const compositionElementCount = countElementTags(compiled.html);
+    // HF_DE_SHORT_MAX_ELEMENTS=0 is the documented kill switch (symmetric
+    // with HF_DE_SHORT_MIN_FRAMES=0, which disables via the predicate's own
+    // minFrames > 0 guard). Gated explicitly here too — without it, a fired
+    // max-elements kill switch left every in-band render decisive against a
+    // real floor comparison, so it reported "skipped_elements" (comp too
+    // large) instead of undefined (band disabled), corrupting the DiD
+    // control cohort with kill-switched renders (review finding).
+    const deShortBandEnabled = deShortBandMaxElements > 0;
     const deShortBandOpen =
+      deShortBandEnabled &&
       deShortBandMinFrames > 0 &&
-      deShortBandMaxElements > 0 &&
       compositionElementCount <= deShortBandMaxElements;
     // Baseline-first sequencing: this release EVALUATES the band on every
     // render and emits the decision, but only routes on it when
@@ -2544,19 +2596,21 @@ async function executeRenderPipeline(input: {
       ...deInversionArgs,
       minFrames: Math.min(deSingleMinFrames, deShortBandMinFrames),
     });
-    // The band is DECISIVE only when it alone flips the decision — every
-    // other eligibility condition already passed and only the floor differed.
-    // Renders the band could not have affected (ineligible for any other
-    // reason, or already inverting at 900+) stay unset, so the telemetry
-    // cohort contains exactly the renders whose routing this change decides.
-    const deShortBandDecisive = invertAtBandFloor && !invertAtBaseFloor;
-    const deShortBand = deShortBandDecisive
-      ? deShortBandOpen
-        ? ("applied" as const)
-        : ("skipped_elements" as const)
-      : undefined;
+    const deShortBand = resolveDeShortBand({
+      invertAtBaseFloor,
+      invertAtBandFloor,
+      bandEnabled: deShortBandEnabled,
+      bandOpen: deShortBandOpen,
+    });
     const deInversionEligible =
       deShortBandRoute && deShortBand === "applied" ? invertAtBandFloor : invertAtBaseFloor;
+    // The floor that actually decided this render — for the human-facing log
+    // below, so it never claims e.g. "400 frames >= 900" for a band-routed
+    // inversion (review finding).
+    const deInversionEffectiveMinFrames =
+      deShortBandRoute && deShortBand === "applied"
+        ? Math.min(deSingleMinFrames, deShortBandMinFrames)
+        : deSingleMinFrames;
     // DE parallel-router eligibility — see shouldPreferParallelDrawElement.
     // Default-off (HF_DE_PARALLEL_ROUTER); HF_DE_PARALLEL_MIN_FRAMES default
     // 700, re-calibrated 2026-07-27 from the original safe-high 2000. A
@@ -2790,7 +2844,7 @@ async function executeRenderPipeline(input: {
       log.info(
         "[Render] Fast capture: single-worker drawElement streaming preferred over " +
           `${workerCount}-worker screenshot capture (${totalFrames} frames >= ` +
-          `${deSingleMinFrames}; verified path, measured faster at every worker count). ` +
+          `${deInversionEffectiveMinFrames}; verified path, measured faster at every worker count). ` +
           "Set HF_DE_SINGLE_MIN_FRAMES=0 or --workers N to override.",
       );
       workerCount = 1;