From 76bb68e5e106f03882bae71b486b04be410832be Mon Sep 17 00:00:00 2001
From: Xuanru Li <157947275+xuanruli@users.noreply.github.com>
Date: Sun, 26 Jul 2026 06:20:52 -0700
Subject: [PATCH] fix(check): audit the composition when the motion sidecar
 will not parse
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

`runCheckPipeline` returned early on `motion.kind === "invalid"`, handing
`buildReport` an `emptyBrowserResult()`. The browser never opened, so Layout,
Runtime and Contrast printed an affirmative all-clear for sections nothing had
looked at, and `layout.duration` came back 0 for a 6s composition. Measured on
one composition under three sidecar states: 8 layout findings with a valid
sidecar, 8 with none, 0 with a typo'd one.

A sidecar says nothing about the composition — the HTML is fine, a JSON file
beside it is not — so the parse error now rides along as a motion finding while
the audit runs. Layout output for an invalid sidecar is byte-identical to the
no-sidecar path.

Nothing downstream reads the unparsed spec: both `motion.spec` accesses sit
behind a `kind === "valid"` guard, and `planMotionSampling` returns an empty
plan for anything else, so `motion.samples` stays 0 and no motion frame is
collected. `enabled`/`specPath` are unchanged from before — the early return
already passed the invalid resolution to `buildReport`.

The run was already failing (the parse error is `error`-severity), so this only
adds findings to a red report and cannot flip a passing check. Cost is one
ordinary `check`: the up-to-300-seek motion loop is exactly the part that stays
skipped.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
---
 packages/cli/src/commands/check.test.ts | 42 +++++++++++++++++++++++++
 packages/cli/src/utils/checkPipeline.ts | 25 +++++++++------
 2 files changed, 57 insertions(+), 10 deletions(-)

diff --git a/packages/cli/src/commands/check.test.ts b/packages/cli/src/commands/check.test.ts
index 8d9a07f97e..fcba738489 100644
--- a/packages/cli/src/commands/check.test.ts
+++ b/packages/cli/src/commands/check.test.ts
@@ -934,6 +934,48 @@ describe("check pipeline", () => {
     expect(browser).toHaveBeenCalledTimes(1);
   });
 
+  // An unreadable sidecar used to return before the browser opened, so Layout/Runtime/Contrast
+  // printed "0 errors" without having looked — an all-clear the same composition contradicts.
+  it("still audits layout when the motion sidecar is invalid", async () => {
+    const motion: MotionSpecResolution = {
+      kind: "invalid",
+      path: "/project/index.motion.json",
+      message: 'assertions[0] (staysInFrame): "selector" must be a non-empty string',
+    };
+    const driver = fakeDriver({
+      collectLayout: vi.fn(async (time: number) => [layoutIssue("error", { time })]),
+    });
+    const { report, browser } = await runScenario(driver, {}, { motion });
+
+    expect(browser).toHaveBeenCalledTimes(1);
+    expect(report.layout.findings.length).toBeGreaterThan(0);
+    expect(report.motion.findings).toEqual([
+      expect.objectContaining({ code: "motion_spec_invalid", severity: "error" }),
+    ]);
+    // A JSON consumer sees the spec was found but produced nothing sampleable.
+    expect(report.motion).toMatchObject({
+      enabled: true,
+      specPath: "/project/index.motion.json",
+      samples: 0,
+    });
+    expect(report.ok).toBe(false);
+  });
+
+  // Guards the new path rather than the old bug: now that the browser opens for an invalid sidecar,
+  // deleting `planMotionSampling`'s `kind !== "valid"` guard would crash on `motion.spec`.
+  it("does not sample motion for an invalid sidecar", async () => {
+    const motion: MotionSpecResolution = {
+      kind: "invalid",
+      path: "/project/index.motion.json",
+      message: "version 2 is not supported",
+    };
+    const driver = fakeDriver();
+    const { report } = await runScenario(driver, {}, { motion });
+
+    expect(report.motion.samples).toBe(0);
+    expect(driver.collectMotionFrame).not.toHaveBeenCalled();
+  });
+
   it("reports a failing appearsBy sidecar as motion_appears_late", async () => {
     const motion: MotionSpecResolution = {
       kind: "valid",
diff --git a/packages/cli/src/utils/checkPipeline.ts b/packages/cli/src/utils/checkPipeline.ts
index a02fb2fa5b..4d7bc03821 100644
--- a/packages/cli/src/utils/checkPipeline.ts
+++ b/packages/cli/src/utils/checkPipeline.ts
@@ -1063,15 +1063,20 @@ export async function runCheckPipeline(
   }
 
   const motion = dependencies.resolveMotionSpec(project.dir);
-  if (motion.kind === "invalid") {
-    const finding = findingAtRoot(
-      "motion_spec_invalid",
-      "error",
-      motion.message,
-      relative(project.dir, motion.path) || "index.motion.json",
-    );
-    return buildReport(options, lint, emptyBrowserResult(), motion, [finding], []);
-  }
+  // An unreadable sidecar only invalidates the motion stage: layout, runtime and contrast never read
+  // it. `planMotionSampling` declines to sample anything but a valid spec, so the audit runs as usual
+  // and the parse error rides along as a motion finding.
+  const specFindings =
+    motion.kind === "invalid"
+      ? [
+          findingAtRoot(
+            "motion_spec_invalid",
+            "error",
+            motion.message,
+            relative(project.dir, motion.path) || "index.motion.json",
+          ),
+        ]
+      : [];
 
   let browser: CheckBrowserResult;
   try {
@@ -1084,7 +1089,7 @@ export async function runCheckPipeline(
   const snapshotFiles = options.snapshots
     ? await writeContrastSnapshots(dependencies, project.dir, browser)
     : [];
-  const report = buildReport(options, lint, browser, motion, [], snapshotFiles);
+  const report = buildReport(options, lint, browser, motion, specFindings, snapshotFiles);
   return options.snapshots
     ? await withFindingCrops(dependencies, project, options, report)
     : report;