Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions packages/cli/src/capture/captureCompositionFrame.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ import { tmpdir } from "node:os";
import { join } from "node:path";
import { afterEach, describe, expect, it, vi } from "vitest";
import {
AUDIT_SEEK_OPTIONS,
DENSE_GEOMETRY_SEEK_OPTIONS,
captureRegionCrop,
clampCropRegion,
installPageFunctionGuard,
Expand Down Expand Up @@ -374,3 +376,15 @@ describe("installPageFunctionGuard", () => {
expect(shim(marker)).toBe(marker);
});
});

describe("DENSE_GEOMETRY_SEEK_OPTIONS", () => {
it("is genuinely geometry-only — no post-seek settle waits at the 600-sample cap", () => {
// Dense pass must not inherit AUDIT's post-seek waits — geometry is valid synchronously after setTime, and waits multiply by sample count.
expect(DENSE_GEOMETRY_SEEK_OPTIONS.animationFrameSettle).toBe("none");
expect(DENSE_GEOMETRY_SEEK_OPTIONS.waitForFontsMs).toBe(0);
expect(DENSE_GEOMETRY_SEEK_OPTIONS.settleMs).toBe(0);
// AUDIT (the full-settle path used by the base grid) must still carry the waits.
expect(AUDIT_SEEK_OPTIONS.animationFrameSettle).toBe("double");
expect(AUDIT_SEEK_OPTIONS.settleMs).toBeGreaterThan(0);
});
});
8 changes: 8 additions & 0 deletions packages/cli/src/capture/captureCompositionFrame.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,14 @@ export const AUDIT_SEEK_OPTIONS = {
settleMs: 120,
} as const;

// Geometry-only seek for the dense content_overlap grid: getBoundingClientRect is valid synchronously after setTime, so drop all post-seek waits (rAF/font/sleep) that would multiply across the dense grid.
export const DENSE_GEOMETRY_SEEK_OPTIONS = {
...AUDIT_SEEK_OPTIONS,
animationFrameSettle: "none",
waitForFontsMs: 0,
settleMs: 0,
} as const;

export interface SeekCompositionTimelineOptions {
fallbackToBridgeAndTimelines?: boolean;
waitForPreferredSeekTargetMs?: number;
Expand Down
41 changes: 41 additions & 0 deletions packages/cli/src/commands/check.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,9 @@ function fakeDriver(overrides: Partial<CheckAuditDriver> = {}): CheckAuditDriver
getCanvas: vi.fn(async () => ({ width: 1920, height: 1080 })),
findAmbiguousSelectors: vi.fn(async (_selectors: string[]) => []),
seek: vi.fn(async (_time: number) => undefined),
seekGeometry: vi.fn(async (_time: number) => undefined),
collectLayout: vi.fn(async (_time: number, _tolerance: number) => []),
collectOverlap: vi.fn(async (_time: number) => []),
collectLayoutGeometry: vi.fn(async () => `geometry-${geometryCallCount++}`),
collectRotationSample: vi.fn(async (_time: number) => []),
collectOffPivotRotationSample: vi.fn(async (time: number) => ({ time, samples: [] })),
Expand Down Expand Up @@ -1343,3 +1345,42 @@ describe("contrast candidate round-trip", () => {
expect(source).not.toMatch(/prepared\.map\(\(entry\) => entry\.candidate\)/);
});
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 The "surfaces a held content_overlap" test doesn't exercise the dense-vs-sparse distinction it claims to demonstrate.

collectOverlap: vi.fn(async (time: number) => [
  layoutIssue("warning", { time, code: "content_overlap" }),
]),

This returns a content_overlap warning at EVERY dense sample time (~72 samples for a 9s fake duration at 8fps). The collapse step sees ~72 occurrences → persistence tier trivially promotes to error via the occurrences >= 2 shortcut (see the sibling comment on layoutAudit.ts:324). The test would pass equally well against a broken dense pass that simply promoted every finding — it never asserts occurrences count or firstSeen/lastSeen span.

More importantly: the test doesn't reproduce the target defect (transient crossing seen only by 2 adjacent dense samples that the sparse grid missed). Test 2 (:1364-1373) correctly validates the static-composition gate — that one is fine.

Fix: add a third test where collectOverlap returns a finding at exactly two adjacent dense sample times and nothing anywhere else. Assert (a) driver.collectOverlap.mock.calls.length matches the dense grid size, (b) the surviving finding has occurrences === 2, (c) severity resolves to what the intended semantic is (once the sibling comment's design question is decided).

Also: buildOverlapSampleTimes(duration) at checkPipeline.ts:441 is a pure function and would take ~5 lines to unit-test for the count/bounds/cap-at-120/quantization behavior. Catches density-math regressions the corpus can't.

Review by Rames D Jusso

describe("dense motion-overlap re-sampling", () => {
// Collision lives inside (3.5, 4.5), a gap the sparse base grid seeks past; only the 8fps dense pass observes it.
const inBetweenGridWindow = (time: number): boolean => time >= 3.6 && time <= 4.4;

it("detects a content_overlap that occurs ONLY between two sparse grid samples", async () => {
const driver = fakeDriver({
// Sparse base grid sees nothing at any base sample time.
collectLayout: vi.fn(async (_time: number) => []),
// The transient exists only strictly between base samples 3.5 and 4.5.
collectOverlap: vi.fn(async (time: number) =>
inBetweenGridWindow(time)
? [layoutIssue("warning", { time, code: "content_overlap" })]
: [],
),
});
const { report } = await runScenario(driver);
expect(driver.collectOverlap).toHaveBeenCalled();
expect(report.layout.findings.some((f) => f.code === "content_overlap")).toBe(true);
// Held ~750ms across the dense grid (>= the 500ms floor) -> promoted.
expect(report.layout.errorCount).toBeGreaterThan(0);
});

it("runs the dense pass even when sparse fingerprints are identical (aliased motion)", async () => {
// Aliased motion has identical fingerprints yet still collides between samples — the false-negative the removed gate caused.
const driver = fakeDriver({
collectLayoutGeometry: vi.fn(async () => "static"),
collectLayout: vi.fn(async (_time: number) => []),
collectOverlap: vi.fn(async (time: number) =>
inBetweenGridWindow(time)
? [layoutIssue("warning", { time, code: "content_overlap" })]
: [],
),
});
const { report } = await runScenario(driver);
expect(driver.collectOverlap).toHaveBeenCalled();
expect(report.layout.findings.some((f) => f.code === "content_overlap")).toBe(true);
});
});
10 changes: 10 additions & 0 deletions packages/cli/src/commands/layout-audit.browser.js
Original file line number Diff line number Diff line change
Expand Up @@ -1425,6 +1425,16 @@
return issues;
};

// Reruns only the overlap detector (same threshold, no new surface) on a fine grid for the dense motion re-sampling pass.
window.__hyperframesOverlapAudit = function auditOverlap(options) {
const time = options && typeof options.time === "number" ? options.time : 0;
const root =
document.querySelector("[data-composition-id][data-width][data-height]") ||
document.querySelector("[data-composition-id]") ||
document.body;
return contentOverlapIssues(root, time);
};

// Frozen-sweep guard (#U10, checkPipeline.ts): a compact per-sample
// fingerprint of every visible element's box + opacity, in DOM order. Node
// calls this once per seeked grid point and compares the strings across the
Expand Down
19 changes: 19 additions & 0 deletions packages/cli/src/utils/checkBrowser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { join, resolve } from "node:path";
import type { Page } from "puppeteer-core";
import {
AUDIT_SEEK_OPTIONS,
DENSE_GEOMETRY_SEEK_OPTIONS,
DEFAULT_ZOOM_PADDING_PX,
DEFAULT_ZOOM_SCALE,
captureRegionCrop,
Expand Down Expand Up @@ -348,7 +349,12 @@ function createPageDriver(page: Page, setTime: (time: number) => void): CheckAud
setTime(time);
await seekCompositionTimeline(page, time, AUDIT_SEEK_OPTIONS);
},
seekGeometry: async (time) => {
setTime(time);
await seekCompositionTimeline(page, time, DENSE_GEOMETRY_SEEK_OPTIONS);
},
collectLayout: (time, tolerance) => collectLayout(page, time, tolerance),
collectOverlap: (time) => collectOverlap(page, time),
collectLayoutGeometry: () => collectLayoutGeometry(page),
collectRotationSample: (time) => collectRotationSample(page, time),
collectOffPivotRotationSample: (time) => collectOffPivotRotationSample(page, time),
Expand Down Expand Up @@ -464,6 +470,19 @@ async function collectLayout(
return anchorLayoutIssues(page, raw.flatMap(parseLayoutIssue));
}

async function collectOverlap(page: Page, time: number): Promise<AnchoredLayoutIssue[]> {
const raw = await page.evaluate(
(options: { time: number }) => {
const audit = Reflect.get(window, "__hyperframesOverlapAudit");
if (typeof audit !== "function") return [];
const result = Reflect.apply(audit, window, [options]);
return Array.isArray(result) ? result : [];
},
{ time },
);
return anchorLayoutIssues(page, raw.flatMap(parseLayoutIssue));
}

async function collectLayoutGeometry(page: Page): Promise<string> {
return page.evaluate(() => {
const geometry = Reflect.get(window, "__hyperframesLayoutGeometry");
Expand Down
33 changes: 33 additions & 0 deletions packages/cli/src/utils/checkPipeline.ts
Original file line number Diff line number Diff line change
Expand Up @@ -417,9 +417,42 @@ async function collectGridSamples(
collected.screenshots.push({ time, pngBase64: capture.pngBase64 });
}
}
await collectMotionOverlapSamples(driver, grid, collected);
return collected;
}

// Dense grid catches mid-motion text collisions the sparse layout grid seeks past; text-only overlap collection is cheap enough to afford it.
const OVERLAP_SAMPLE_FPS = 8;
// Ceiling that bounds dense seeks; past ~75s the grid degrades below 8fps rather than growing the seek budget without limit.
const OVERLAP_MAX_SAMPLES = 600;

function buildOverlapSampleTimes(duration: number): number[] {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 20s+ compositions silently degrade to sub-8fps sampling under the 120-cap.

const count = Math.min(
  OVERLAP_MAX_SAMPLES,
  Math.max(2, Math.ceil(duration * OVERLAP_SAMPLE_FPS) + 1),
);
const step = duration / (count - 1);

For duration = 20s, Math.ceil(20*8)+1 = 161 → capped to 120 → step = 20/119 ≈ 0.168s → effective ~5.94fps. For duration = 30s, step ≈ 0.253s → ~3.95fps. The comment at :422-430 sells the design as "8fps (~0.125s spacing) lands >= 2 samples inside a window that narrow", but that guarantee holds only for duration <= 14.875s.

Failure mode: long-form compositions (explainer clips, marketing walk-throughs) fall back to a sparser grid than the design claims. Density guarantee erodes silently — the occurrences >= 2 promotion machinery loses its sub-250ms transient coverage past ~15s.

HF's fuzz corpus is short so this may not bite in CI, but production comp durations trend longer.

Fix options: (a) preserve density and drop the tail past 15s: const step = 1 / OVERLAP_SAMPLE_FPS; const count = Math.min(OVERLAP_MAX_SAMPLES, Math.floor(duration * OVERLAP_SAMPLE_FPS) + 1); (b) accept the degradation but return a truncatedAtSeconds signal so downstream consumers can flag long comps. Minimum: document the effective-fps table in the const block so future authors don't assume 8fps everywhere.

Review by Rames D Jusso

if (!Number.isFinite(duration) || duration <= 0) return [];
const count = Math.min(
OVERLAP_MAX_SAMPLES,
Math.max(2, Math.ceil(duration * OVERLAP_SAMPLE_FPS) + 1),
);
const step = duration / (count - 1);
return mergeSampleTimes(
Array.from({ length: count }, (_, index) => Math.round(index * step * 1000) / 1000),
);
}

/** Reruns content_overlap on a fine grid, unconditionally rather than gated on fingerprint change, since an animation aliased to the sparse grid collides between identical-fingerprint samples. */
async function collectMotionOverlapSamples(
driver: CheckAuditDriver,
grid: SampleGrid,
collected: GridSamples,
): Promise<void> {
const baseTimes = new Set(grid.layoutSamples);
for (const time of buildOverlapSampleTimes(grid.duration)) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 Design-contract question — these dense-pass findings enter the persistence tier via a shortcut that assumed sparse-grid timing.

for (const time of buildOverlapSampleTimes(grid.duration)) {
  if (baseTimes.has(time)) continue;
  await driver.seek(time);
  collected.layoutIssues.push(...(await driver.collectOverlap(time)));
}

The findings pushed here flow into collapseStaticLayoutIssuesisContentOverlapHeldLongEnough (layoutAudit.ts:323-330, unmodified by this PR):

function isContentOverlapHeldLongEnough(issue: LayoutIssue, occurrences: number): boolean {
  if (occurrences >= HELD_ACROSS_SAMPLES_MIN_OCCURRENCES) return true;   // ← short-circuits
  const firstSeen = issue.firstSeen ?? issue.time;
  const lastSeen = issue.lastSeen ?? issue.time;
  const heldMs = (lastSeen - firstSeen) * 1000;
  return heldMs >= CONTENT_OVERLAP_HELD_ERROR_MS;
}

Design comment at layoutAudit.ts:186-198 frames the mapping: "At the default 9-sample grid over a multi-second composition ... two collapsed occurrences are already >= one sample-to-sample gap, which is well past 500ms — so 'held under 250ms' reduces to occurrences <= 1 and 'held >= 500ms' reduces to occurrences >= 2." That mapping holds for the sparse grid (~1s between adjacent samples).

At the new 8fps dense grid, two adjacent samples span ~125ms. So occurrences === 2 → promoted straight to error — well under the 500ms design floor and even under the 250ms ignore floor.

The design comment DOES anticipate this — it says "with the literal ms span (CONTENT_OVERLAP_HELD_ERROR_MS) kept as a fallback for callers whose samples really are spaced close enough together for the ms floor to matter on its own (dense --at/--at-transitions runs)". But the code short-circuits on occurrences FIRST — the ms-fallback branch only runs when occurrences < 2, so it never gates the dense-pass 2-occurrence case.

Your PR-body reviewer note acknowledges this: "At 8fps the occurrences >= 2 persistence shortcut spans ~125ms (vs ~500ms on the sparse grid), so brief-but-real collisions now gate at error. Every inspected instance was real, but if softer severity is preferred, the heldMs >= 500 path is the alternative knob."

The request is: make the choice explicit either in code or in the comment. Three options:

  • (a) Tighten isContentOverlapHeldLongEnough to ANDreturn occurrences >= 2 && (lastSeen - firstSeen) * 1000 >= CONTENT_OVERLAP_HELD_ERROR_MS. Still passes the sparse-grid case (samples ≥1s apart → heldMs ≥1000 ≥ 500) and correctly gates the dense case. Cleanest — one source of truth.
  • (b) Tag dense-pass findings with a source: 'dense' marker HERE and skip the occurrences shortcut only for that source in the persistence-tier code.
  • (c) Update the design comment at layoutAudit.ts:186-198 to say the ms floor is intentionally relaxed to ~125ms for dense-pass findings, with a rationale from the corpus ("81/81 samples of the corpus showed 125ms crossings were real defects").

Any of the three closes the contract question. Right now the comment and the code disagree; the disagreement widens by 375ms every time the dense pass fires.

Review by Rames D Jusso

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 Dense loop has neither early-terminate nor try/catch — cost + flake risk.

for (const time of buildOverlapSampleTimes(grid.duration)) {
  if (baseTimes.has(time)) continue;
  await driver.seek(time);
  collected.layoutIssues.push(...(await driver.collectOverlap(time)));
}

Cost: serial loop of up to ~120 seek + evaluate roundtrips per composition. Each seek waits for GSAP/CSS timeline settle. Once persistence tier is going to promote a finding at occurrences >= 2, additional samples of the same key add zero signal but still pay the cost. The perf claim text-only so it's cheap (comment at :428-430) is defensible per-sample but not per-run.

Flake: neither driver.seek(time) nor driver.collectOverlap(time) is guarded. If either throws mid-loop (transient page-evaluate error, browser blip, timeout on a specific frame), the entire dense pass — and by inheritance the surrounding collectGridSamples await — rejects, aborting the whole check run. Under the sparse grid the same shape existed with 9 chances; now there are ~120, so the throw budget multiplies with the sample count.

Fix: (a) wrap the loop body in try/catch, record the throw as a soft dense_overlap_probe_failed info-level finding rather than aborting. (b) early-terminate when every observed key has already crossed a threshold (say 5 occurrences). (c) at minimum, add a per-sample timeout so a hung seek doesn't stall the whole pass. Neither is a merge blocker — but the const block should carry a worst-case cost note.

Review by Rames D Jusso

if (baseTimes.has(time)) continue;
// Settle-free seek: collectOverlap reads getBoundingClientRect geometry, valid synchronously after setTime, so the dense pass skips the per-seek paint settle.
await driver.seekGeometry(time);
collected.layoutIssues.push(...(await driver.collectOverlap(time)));
}
}

// Frozen-sweep guard (#U10): compositions this short can legitimately hold a
// single static frame the whole time (a title card) — never flag those.
const SWEEP_STATIC_MIN_DURATION_SEC = 3;
Expand Down
4 changes: 4 additions & 0 deletions packages/cli/src/utils/checkTypes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,11 @@ export interface CheckAuditDriver {
getCanvas(): Promise<Canvas>;
findAmbiguousSelectors(selectors: string[]): Promise<AnchoredLayoutIssue[]>;
seek(time: number): Promise<void>;
/** Settle-free seek for the geometry-only dense content_overlap pass; only collectOverlap consumes it, and getBoundingClientRect is valid synchronously after setTime. */
seekGeometry(time: number): Promise<void>;
collectLayout(time: number, tolerance: number): Promise<AnchoredLayoutIssue[]>;
/** content_overlap only, for the dense re-sampling grid — catches transient text collisions the sparse grid seeks past. */
collectOverlap(time: number): Promise<AnchoredLayoutIssue[]>;
/** Frozen-sweep guard (#U10): an opaque per-sample geometry+opacity
* fingerprint of the current seeked state, for detecting a timeline that
* never advances under seek. See layout-audit.browser.js. */
Expand Down
41 changes: 41 additions & 0 deletions packages/cli/src/utils/layoutAudit.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,47 @@ describe("persistence-tiered severity (#U10)", () => {
expect(collapsed[0]).toMatchObject({ severity: "error", occurrences: 2 });
});

it("keeps a content_overlap that spans under the 500ms floor as a warning, even with 2 occurrences", () => {
// Two dense-pass occurrences ~125ms apart are under the held-duration floor, so occurrences>=2 alone must not promote to error.
const collapsed = collapseStaticLayoutIssues(
[
{ ...issue("content_overlap", "warning"), time: 4.0 },
{ ...issue("content_overlap", "warning"), time: 4.125 },
],
73,
);

expect(collapsed).toHaveLength(1);
expect(collapsed[0]).toMatchObject({ severity: "warning", occurrences: 2 });
});

it("does NOT promote content_overlap whose two occurrences span exactly 499ms (under the floor)", () => {
// Boundary: a span one millisecond short of the 500ms floor stays a warning — guards the AND-tighten for sparse callers.
const collapsed = collapseStaticLayoutIssues(
[
{ ...issue("content_overlap", "warning"), time: 4.0 },
{ ...issue("content_overlap", "warning"), time: 4.499 },
],
73,
);

expect(collapsed).toHaveLength(1);
expect(collapsed[0]).toMatchObject({ severity: "warning", occurrences: 2 });
});

it("promotes content_overlap whose two occurrences span exactly 500ms (at the floor)", () => {
const collapsed = collapseStaticLayoutIssues(
[
{ ...issue("content_overlap", "warning"), time: 4.0 },
{ ...issue("content_overlap", "warning"), time: 4.5 },
],
73,
);

expect(collapsed).toHaveLength(1);
expect(collapsed[0]).toMatchObject({ severity: "error", occurrences: 2 });
});

it("promotes a held, canvas-scale canvas_overflow breach from info to warning", () => {
const breach = {
...issue("canvas_overflow", "info"),
Expand Down
23 changes: 4 additions & 19 deletions packages/cli/src/utils/layoutAudit.ts
Original file line number Diff line number Diff line change
Expand Up @@ -181,21 +181,7 @@ export function dedupeLayoutIssues(issues: LayoutIssue[]): LayoutIssue[] {
return result;
}

// Persistence-tier thresholds (#U10, adapted from Adam Rosler's visual-linter
// design). The approach doc frames these as held-duration floors — ignore
// under ~250ms, re-promote content_overlap at >= ~500ms — measured against
// the SAME firstSeen/lastSeen span this collapse step already tracks. At the
// default 9-sample grid over a multi-second composition, a single collapsed
// occurrence is held 0ms (one entrance/exit transient sample) and two
// collapsed occurrences are already >= one sample-to-sample gap, which is
// well past 500ms — so "held under 250ms" reduces to `occurrences <= 1` and
// "held >= 500ms" reduces to `occurrences >= 2`. Tiering below is written in
// those sample-count terms (the mapping the approach doc asks to document),
// with the literal ms span (CONTENT_OVERLAP_HELD_ERROR_MS) kept as a fallback
// for callers whose samples really are spaced close enough together for the
// ms floor to matter on its own (dense `--at`/`--at-transitions` runs). The
// ~250ms ignore floor needs no separate constant — see the occurrences <= 1
// branch below.
// Persistence-tier thresholds (#U10): occurrences>=2 alone can't imply the 500ms floor under dense 8fps sampling, so content_overlap promotion requires a literal firstSeen..lastSeen span >= 500ms — stricter for sparse callers too.
const CONTENT_OVERLAP_HELD_ERROR_MS = 500;
const HELD_ACROSS_SAMPLES_MIN_OCCURRENCES = 2;

Expand Down Expand Up @@ -317,11 +303,10 @@ function isCanvasBreachHeldLarge(issue: LayoutIssue, occurrences: number): boole
return overlapX > 0 && overlapY > 0;
}

// Split out of applyPersistenceTier so the two independent "held long enough"
// signals (sample count vs. wall-clock span) read as one boolean question
// instead of adding a third compound branch to the tiering ladder above.
// Split out of applyPersistenceTier so the compound "held long enough" test reads as one boolean question.
function isContentOverlapHeldLongEnough(issue: LayoutIssue, occurrences: number): boolean {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 The AND-tighten fixes the dense-pass over-promotion, but SILENTLY REGRESSES external sparse-grid callers whose intent was promote-on-any-2-occurrences.

function isContentOverlapHeldLongEnough(issue: LayoutIssue, occurrences: number): boolean {
  if (occurrences < HELD_ACROSS_SAMPLES_MIN_OCCURRENCES) return false;
  const firstSeen = issue.firstSeen ?? issue.time;
  const lastSeen = issue.lastSeen ?? issue.time;
  const heldMs = (lastSeen - firstSeen) * 1000;
  return heldMs >= CONTENT_OVERLAP_HELD_ERROR_MS;
}

The R1 concern was that the old shortcut occurrences >= 2 => promote fires at ~125ms under the new 8fps dense pass, violating the design's 500ms wall-clock floor. The R2 AND-tighten fixes that — good. But it changes semantics for callers who were RELYING on the old OR:

  • User-specified --samples 20 on a 3s composition: adjacent-sample gap = 150ms. A genuine held collision hitting 2 sparse occurrences at 150ms apart previously promoted to error; now stays warning.
  • --at / --at-transitions dense runs: the OLD design comment at layoutAudit.ts:186-198 explicitly called these out — quoting the pre-feat(lint): dense motion re-sampling for content_overlap #2746 version: "with the literal ms span kept as a fallback for callers whose samples really are spaced close enough together for the ms floor to matter on its own (dense --at/--at-transitions runs)". So the R1 design ALREADY anticipated dense sparse-caller sampling. The AND-tighten regresses those callers.
  • Short compositions: 1.5s composition at 9 samples → 167ms adjacent-gap → same regression.

The R2 commit updates the design comment at :186-198 to say "honored regardless of sampling density" — so the intent IS that these external callers no longer promote at <500ms. That may be the right call! But the change from OR to AND is a semantics migration that:

  1. Is not called out in the PR title ("dense motion re-sampling" reads as an additive change).
  2. Is not called out in the commit body's user-facing summary (only mentioned as "honor 500ms floor").
  3. Has no boundary test at the 499ms/500ms crossing (a future >/>= typo lands green).
  4. Has no explicit note in the changelog / migration path for the --at/--samples callers who now silently see fewer error promotions.

Fix options:

  • (a) Explicit design decision + test: add a case at layoutAudit.test.ts where occurrences=2, heldMs=499 stays warning AND heldMs=500 promotes — proves the boundary contract and locks it in. Update the PR title/body to call out the semantics change explicitly.
  • (b) Preserve OR for external callers, apply AND only to the dense-pass source: tag dense-pass findings with an internal source: 'motion-dense' marker at checkPipeline.ts:474 and branch here on that marker. Keeps the sparse-grid caller behavior unchanged.
  • (c) Deprecation path: log a warning when a content_overlap finding hits occurrences >= 2 but not the ms floor, mentioning the semantics change and pointing to the ms floor constant.

(a) is probably right — the design change looks intentional and (b) adds an internal-vs-external branch that's harder to reason about. But regardless of which, please add the boundary test.

Review by Rames D Jusso

if (occurrences >= HELD_ACROSS_SAMPLES_MIN_OCCURRENCES) return true;
// Two samples measure a span, but under dense 8fps sampling that span must still clear the wall-clock floor.
if (occurrences < HELD_ACROSS_SAMPLES_MIN_OCCURRENCES) return false;
const firstSeen = issue.firstSeen ?? issue.time;
const lastSeen = issue.lastSeen ?? issue.time;
const heldMs = (lastSeen - firstSeen) * 1000;
Expand Down
Loading