fix(engine): harden ancestor-hidden video skip against mask + caller cache

Two follow-ups to the ancestor-visibility skip in `injectVideoFramesBatch`
and `syncVideoFrameVisibility`.

1. **Mask defence.** Both ancestor-hidden branches previously wrote a plain
   `img.style.visibility = "hidden"`. `applyDomLayerMask` writes the
   stylesheet rule `#${showId} *{visibility:visible !important}`, and CSS
   cascade puts important stylesheet author above non-important inline
   author — so a sub-comp host landing in the active layer's `show` set
   would revive a stale `__render_frame__` and let it bleed onto the
   layer composite. Write the hide via
   `style.setProperty("visibility", "hidden", "important")` instead;
   important inline beats important stylesheet.

2. **Caller cache hygiene.** `createVideoFrameInjector` unconditionally
   wrote `lastInjectedFrameByVideo.set(id, frameIndex)` after calling
   `injectVideoFramesBatch`, even for videos the page silently skipped due
   to a hidden visual ancestor. On the next call at the same frameIndex —
   common with source-fps < output-fps, paused source frames, or
   non-frame-aligned host starts — the cache short-circuited the second
   inject and the host's first visible frame painted blank because the
   replacement `<img>` was never created.

   Make `injectVideoFramesBatch` return `string[]` (the subset of ids it
   actually painted) and have the caller cache only those. The cli-side
   `snapshot.ts` consumer is unaffected: its local `InjectFn` types the
   return as `Promise<void>`, which is structurally compatible with
   `Promise<string[]>` under TS void-return assignment rules.

Tests: linkedom doesn't preserve `!important` in cssText, so the two new
mask-defence cases spy on the live `<img>`'s `style.setProperty` and assert
the 3-arg call shape. The cache-hygiene case stubs the page-side primitives
via `vi.mock`, drives the hook twice at the same frameIndex with a stubbed
"injected nothing" first response, and verifies the second call still
issues an inject. A counter-test pins the happy-path cache hit so a future
refactor can't trade the skip bug for a never-cache regression.

packages/engine/src/services/screenshotService.test.ts

@@ -453,4 +453,58 @@ describe("video-frame injection respects ancestor visibility", () => {
     // <img> to `visibility: visible` so it overrides the ancestor.
     expect(seededImg.style.visibility).toBe("visible");
   });
+
+  // Regression for the layered/HDR mask path: `applyDomLayerMask` writes an
+  // `!important` stylesheet rule `#${showId} *{visibility:visible !important}`
+  // which, if a sub-comp host id appears in the show set, would revive a
+  // plain (non-important) inline `visibility: hidden` on a descendant
+  // `__render_frame__` — the cascade rule is "important stylesheet author
+  // beats non-important inline author". To stay safe regardless of which
+  // layer ends up in `show`, the ancestor-hidden hide must be written with
+  // `!important` so inline `!important` beats stylesheet `!important`.
+  //
+  // linkedom strips `!important` from `cssText`/`getPropertyPriority`, so we
+  // pin the contract on the API call site instead: a `setProperty(name,
+  // value, "important")` invocation on the live `<img>`'s style.
+  it("injectVideoFramesBatch hides a stale <img> with !important so the layer mask cannot revive it", async () => {
+    const { teardown, setup } = withGlobals(setupHostHiddenScenario({ visibility: "hidden" }));
+    const seededImg = setup.document.createElement("img");
+    seededImg.classList.add("__render_frame__");
+    seededImg.style.visibility = "visible";
+    setup.video.parentNode?.insertBefore(seededImg, setup.video.nextSibling);
+    const setPropertySpy = vi.spyOn(seededImg.style, "setProperty");
+
+    try {
+      await injectVideoFramesBatch(passthroughPage(), [
+        {
+          videoId: "pip",
+          dataUri:
+            "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVR42mNkAAIAAAoAAv/lxKUAAAAASUVORK5CYII=",
+        },
+      ]);
+    } finally {
+      teardown();
+    }
+
+    expect(seededImg.style.visibility).toBe("hidden");
+    expect(setPropertySpy).toHaveBeenCalledWith("visibility", "hidden", "important");
+  });
+
+  it("syncVideoFrameVisibility hides an existing <img> with !important so the layer mask cannot revive it", async () => {
+    const { teardown, setup } = withGlobals(setupHostHiddenScenario({ visibility: "hidden" }));
+    const seededImg = setup.document.createElement("img");
+    seededImg.classList.add("__render_frame__");
+    seededImg.style.visibility = "visible";
+    setup.video.parentNode?.insertBefore(seededImg, setup.video.nextSibling);
+    const setPropertySpy = vi.spyOn(seededImg.style, "setProperty");
+
+    try {
+      await syncVideoFrameVisibility(passthroughPage(), ["pip"]);
+    } finally {
+      teardown();
+    }
+
+    expect(seededImg.style.visibility).toBe("hidden");
+    expect(setPropertySpy).toHaveBeenCalledWith("visibility", "hidden", "important");
+  });
 });

packages/engine/src/services/screenshotService.ts

@@ -369,13 +369,22 @@ export async function removeDomLayerMask(page: Page, extraHideIds: string[]): Pr
   );
 }
 
+/**
+ * Returns the subset of `updates.videoId`s that were actually painted in
+ * this call. Videos skipped because of a hidden visual ancestor are NOT
+ * included — the caller relies on this to avoid recording a `lastInjected`
+ * cache entry for a frame that never reached the page, which would otherwise
+ * short-circuit the next inject at the same frameIndex and leave the host's
+ * first visible frame blank.
+ */
 export async function injectVideoFramesBatch(
   page: Page,
   updates: Array<{ videoId: string; dataUri: string }>,
-): Promise<void> {
-  if (updates.length === 0) return;
-  await page.evaluate(
+): Promise<string[]> {
+  if (updates.length === 0) return [];
+  return await page.evaluate(
     async (items: Array<{ videoId: string; dataUri: string }>, visualProperties: string[]) => {
+      const injectedIds: string[] = [];
       const pendingDecodes: Array<Promise<void>> = [];
       const replacementLayoutProperties = new Set([
         "width",
@@ -435,7 +444,13 @@ export async function injectVideoFramesBatch(
           // Don't paint a frame over a hidden host — if an existing replacement
           // <img> is still around from when the host was visible, hide it so it
           // doesn't bleed through a sibling host that *is* visible on this seek.
-          if (hasImg && img) img.style.visibility = "hidden";
+          //
+          // Use `!important` so the inline hide survives `applyDomLayerMask`'s
+          // stylesheet `#${showId} *{visibility:visible !important}` when the
+          // sub-comp host happens to land in the active layer's `show` set —
+          // important stylesheet beats non-important inline, but important
+          // inline beats important stylesheet.
+          if (hasImg && img) img.style.setProperty("visibility", "hidden", "important");
           continue;
         }
 
@@ -522,10 +537,12 @@ export async function injectVideoFramesBatch(
         // GSAP-controlled value.
         video.style.setProperty("visibility", "hidden", "important");
         video.style.setProperty("pointer-events", "none", "important");
+        injectedIds.push(item.videoId);
       }
       if (pendingDecodes.length > 0) {
         await Promise.all(pendingDecodes);
       }
+      return injectedIds;
     },
     updates,
     [...MEDIA_VISUAL_STYLE_PROPERTIES],
@@ -577,11 +594,16 @@ export async function syncVideoFrameVisibility(
       } else {
         // Inactive (or ancestor-hidden) video: hide both. Use visibility only
         // (never opacity) so we never clobber GSAP-controlled inline opacity.
+        // Use `!important` on the <img> hide so `applyDomLayerMask`'s
+        // important stylesheet rule (`#${showId} *{visibility:visible !important}`)
+        // cannot revive a stale frame when the sub-comp host lands in the
+        // active layer's `show` set — same mask-defense reasoning as the
+        // `isVisualAncestorHidden` branch in `injectVideoFramesBatch`.
         video.style.removeProperty("display");
         video.style.setProperty("visibility", "hidden", "important");
         video.style.setProperty("pointer-events", "none", "important");
         if (hasImg) {
-          img.style.visibility = "hidden";
+          img.style.setProperty("visibility", "hidden", "important");
         }
       }
     }

packages/engine/src/services/videoFrameInjector.test.ts

@@ -1,9 +1,30 @@
 // @vitest-environment node
-import { describe, it, expect, beforeEach, afterEach } from "vitest";
+import { describe, it, expect, beforeEach, afterEach, vi } from "vitest";
 import { mkdtempSync, rmSync, writeFileSync } from "node:fs";
 import { tmpdir } from "node:os";
 import { join } from "node:path";
-import { __testing } from "./videoFrameInjector.js";
+import { type Page } from "puppeteer-core";
+
+// Hoist mocks before importing the module under test so the mock factory wins.
+// The cache-hygiene block exercises createVideoFrameInjector against stubbed
+// page-side primitives so we can assert on Node-side state (cache poisoning)
+// without standing up a real browser.
+const { injectVideoFramesBatchMock, syncVideoFrameVisibilityMock } = vi.hoisted(() => ({
+  injectVideoFramesBatchMock: vi.fn<
+    (page: Page, updates: Array<{ videoId: string; dataUri: string }>) => Promise<string[]>
+  >(async (_page, updates) => updates.map((u) => u.videoId)),
+  syncVideoFrameVisibilityMock: vi.fn<(page: Page, ids: string[]) => Promise<void>>(
+    async () => undefined,
+  ),
+}));
+
+vi.mock("./screenshotService.js", () => ({
+  injectVideoFramesBatch: injectVideoFramesBatchMock,
+  syncVideoFrameVisibility: syncVideoFrameVisibilityMock,
+}));
+
+import { __testing, createVideoFrameInjector } from "./videoFrameInjector.js";
+import { type FrameLookupTable } from "./videoFrameExtractor.js";
 import { DEFAULT_CONFIG } from "../config.js";
 
 const { createFrameSourceCache } = __testing;
@@ -143,3 +164,91 @@ describe("frame source cache eviction", () => {
     expect(cache.stats()).toMatchObject({ ...SHARED_STATS, entries: 0, bytes: 0 });
   });
 });
+
+describe("createVideoFrameInjector cache hygiene against page-side skips", () => {
+  // Build a minimal FrameLookupTable stand-in that returns one fixed payload
+  // for every time so we can drive the hook deterministically. The real
+  // table is exercised exhaustively in videoFrameExtractor.test.ts.
+  function fakeTable(payload: { videoId: string; framePath: string; frameIndex: number }) {
+    return {
+      getActiveFramePayloads: () =>
+        new Map([
+          [payload.videoId, { framePath: payload.framePath, frameIndex: payload.frameIndex }],
+        ]),
+    } as unknown as FrameLookupTable;
+  }
+
+  // Bypass the on-disk frame cache by handing back a synthetic data URI.
+  function inlineResolver(framePath: string): string {
+    return `data:image/png;base64,fake-${framePath}`;
+  }
+
+  beforeEach(() => {
+    injectVideoFramesBatchMock.mockReset();
+    syncVideoFrameVisibilityMock.mockReset();
+    syncVideoFrameVisibilityMock.mockResolvedValue(undefined);
+  });
+
+  it("does not poison the lastInjected cache when the page reports zero ids injected", async () => {
+    // Regression for the agentic-finecut scenario after PR #1028's ancestor
+    // skip: when injectVideoFramesBatch silently drops a video (its sub-comp
+    // host is hidden), the caller used to record `lastInjectedFrame[v] = N`
+    // anyway. On the next frame, if the source frameIndex is unchanged
+    // (low-fps source, multiple output frames per source frame, or
+    // non-frame-aligned host start), the cache short-circuits the second
+    // call and the host's first visible frame paints blank because the
+    // replacement <img> was never created.
+    //
+    // Pin the contract: when the page returns `[]` (no ids actually
+    // injected), the cache must not record those frameIndexes, so a follow-
+    // up call at the same frameIndex still issues an inject.
+    const fakePage = {} as Page;
+    const hook = createVideoFrameInjector(
+      fakeTable({ videoId: "pip", framePath: "/p", frameIndex: 5 }),
+      {
+        frameSrcResolver: inlineResolver,
+      },
+    );
+    expect(hook).not.toBeNull();
+
+    // First call: simulate the ancestor-hidden skip — page-side reports it
+    // injected nothing.
+    injectVideoFramesBatchMock.mockResolvedValueOnce([]);
+    await hook!(fakePage, 0);
+    expect(injectVideoFramesBatchMock).toHaveBeenCalledTimes(1);
+    expect(injectVideoFramesBatchMock).toHaveBeenLastCalledWith(fakePage, [
+      { videoId: "pip", dataUri: "data:image/png;base64,fake-/p" },
+    ]);
+
+    // Second call: same frameIndex, but the previous call did not really
+    // paint. The cache must NOT short-circuit; the inject must run again.
+    injectVideoFramesBatchMock.mockResolvedValueOnce(["pip"]);
+    await hook!(fakePage, 0);
+    expect(injectVideoFramesBatchMock).toHaveBeenCalledTimes(2);
+    expect(injectVideoFramesBatchMock).toHaveBeenLastCalledWith(fakePage, [
+      { videoId: "pip", dataUri: "data:image/png;base64,fake-/p" },
+    ]);
+  });
+
+  it("does cache normally when the page reports the id as injected", async () => {
+    // Counter-test: when injection succeeds for a videoId, the cache must
+    // record it and a second call at the same frameIndex must short-circuit.
+    // This pins the happy path so a future refactor can't trade the skip
+    // bug for a never-cache regression.
+    const fakePage = {} as Page;
+    const hook = createVideoFrameInjector(
+      fakeTable({ videoId: "pip", framePath: "/p", frameIndex: 5 }),
+      {
+        frameSrcResolver: inlineResolver,
+      },
+    );
+
+    injectVideoFramesBatchMock.mockResolvedValueOnce(["pip"]);
+    await hook!(fakePage, 0);
+    expect(injectVideoFramesBatchMock).toHaveBeenCalledTimes(1);
+
+    await hook!(fakePage, 0);
+    // Cache hit — no second inject for the same frameIndex.
+    expect(injectVideoFramesBatchMock).toHaveBeenCalledTimes(1);
+  });
+});

packages/engine/src/services/videoFrameInjector.ts

@@ -197,12 +197,22 @@ export function createVideoFrameInjector(
 
     await syncVideoFrameVisibility(page, Array.from(activeIds));
     if (updates.length > 0) {
-      await injectVideoFramesBatch(
-        page,
-        updates.map((u) => ({ videoId: u.videoId, dataUri: u.dataUri })),
+      // Only record cache entries for videos the page actually painted.
+      // injectVideoFramesBatch skips any video whose visual ancestor is
+      // hidden (sub-comp host out-of-window) and returns the subset of ids
+      // it really wrote — recording the rest would short-circuit the next
+      // call at the same frameIndex and leave the host's first visible
+      // frame blank.
+      const injectedIds = new Set(
+        await injectVideoFramesBatch(
+          page,
+          updates.map((u) => ({ videoId: u.videoId, dataUri: u.dataUri })),
+        ),
       );
       for (const update of updates) {
-        lastInjectedFrameByVideo.set(update.videoId, update.frameIndex);
+        if (injectedIds.has(update.videoId)) {
+          lastInjectedFrameByVideo.set(update.videoId, update.frameIndex);
+        }
       }
     }
   };