From 6e5f460a0c63f8cc2b07e28f8ec80cee7fefce37 Mon Sep 17 00:00:00 2001
From: Vance Ingalls <vance@heygen.com>
Date: Wed, 23 Sep 2026 03:44:04 -0700
Subject: [PATCH] fix(studio): split the empty-read reason and stop blaming the
 composition (#4325)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Two residuals from the cutover-dashboard work, both found in the data the
first pass shipped (0.8.61/0.8.62).

`absent_or_empty` was one label over two different failures: the route
answers `content: ""` both from the `optional=1` shim (nothing resolved at
that path) and for a real 0-byte file. The route now marks the shim with
`missing: true`, and the read path with `missing: false` so the field's
PRESENCE is what tells the client this server draws the distinction — a new
server's empty file would otherwise look exactly like either case from an
old one. Client splits them into `absent` and `empty_file`; a server without
the field still reports `absent_or_empty`, so the existing series stays
honest instead of silently folding into one of the new ones.

Why it is worth splitting: on 0.8.62, 75 tabs hit this class and not one of
them ever landed an SDK edit afterwards — it is terminal for the tab, while
the tab keeps playing back and navigating, so nothing surfaces to the user.
Every measured case with a loaded tree carries `path_in_tree: true`: a file
the tree lists and this read cannot get. Which of the two causes that is
decides the fix, and until now the event could not say.

`res.json()` at the end of the read was unguarded, so a 200 carrying HTML —
an SPA fallback or a proxy answering in the route's place — rejected out of
the function entirely and the effect's outer catch filed it as
`stage: "open"`, which asserts the COMPOSITION failed to parse. Observed in
production on 0.8.62: `Unexpected token '<', "<!-- /*!"... is not valid
JSON`. That is the third label to make the same wrong claim, after the
fetch-rejection split in #4240 and #4269. Now `reason: "invalid_json"` with
the response's content-type.

`path_in_tree` now rides all three empty-read reasons, not just the combined
one, so the split keeps the signal that justified it.

Tests: 4 new (absent / empty_file / combined-label fallback / non-JSON 200)
plus a server test pinning `missing` on both answers. 1172 tests green
across studio hooks + studio-server routes; oxlint, oxfmt, tsc clean.

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
---
 .../studio-server/src/routes/files.test.ts    | 26 +++++-
 packages/studio-server/src/routes/files.ts    | 20 ++++-
 .../hooks/useSdkSession.lifecycle.test.tsx    | 79 ++++++++++++++++-
 packages/studio/src/hooks/useSdkSession.ts    | 88 ++++++++++++++++---
 4 files changed, 196 insertions(+), 17 deletions(-)

diff --git a/packages/studio-server/src/routes/files.test.ts b/packages/studio-server/src/routes/files.test.ts
index 266b4e44b0..45e4cc5428 100644
--- a/packages/studio-server/src/routes/files.test.ts
+++ b/packages/studio-server/src/routes/files.test.ts
@@ -255,11 +255,35 @@ describe("registerFileRoutes", () => {
     const response = await app.request(
       "http://localhost/projects/demo/files/missing-file.txt?optional=1",
     );
-    const payload = (await response.json()) as { filename?: string; content?: string };
+    const payload = (await response.json()) as {
+      filename?: string;
+      content?: string;
+      missing?: boolean;
+    };
 
     expect(response.status).toBe(200);
     expect(payload.filename).toBe("missing-file.txt");
     expect(payload.content).toBe("");
+    expect(payload.missing).toBe(true);
+  });
+
+  // The shim and a real 0-byte file both answer `content: ""`. `missing` is
+  // the only thing separating them, and it has to be on BOTH answers — its
+  // presence is what tells a caller this server draws the distinction at all.
+  it("marks a real zero-byte file as present, not missing", async () => {
+    const projectDir = createProjectDir();
+    writeFileSync(join(projectDir, "empty.html"), "");
+    const app = new Hono();
+    registerFileRoutes(app, createAdapter(projectDir));
+
+    const response = await app.request(
+      "http://localhost/projects/demo/files/empty.html?optional=1",
+    );
+    const payload = (await response.json()) as { content?: string; missing?: boolean };
+
+    expect(response.status).toBe(200);
+    expect(payload.content).toBe("");
+    expect(payload.missing).toBe(false);
   });
 
   it("still returns 404 for other missing files", async () => {
diff --git a/packages/studio-server/src/routes/files.ts b/packages/studio-server/src/routes/files.ts
index ed68ee328f..7a6f20ec65 100644
--- a/packages/studio-server/src/routes/files.ts
+++ b/packages/studio-server/src/routes/files.ts
@@ -2278,7 +2278,14 @@ export function registerFileRoutes(api: Hono, adapter: StudioApiAdapter): void {
 
     if (!existsSync(res.absPath)) {
       if (c.req.query("optional") === "1") {
-        return c.json({ filename: res.filePath, content: "" });
+        // `missing: true` separates the absent-file shim from a genuinely
+        // 0-byte file — both answer `content: ""`, and the caller could not
+        // tell them apart. That ambiguity hid the largest remaining class of
+        // SDK-session failures: a composition the file tree lists but this
+        // read answers empty for is either a placeholder nobody has written
+        // yet, or a path that does not resolve here at all, and those need
+        // different fixes. Additive, so an older client ignores it.
+        return c.json({ filename: res.filePath, content: "", missing: true });
       }
       return c.json({ error: "not found" }, 404);
     }
@@ -2286,7 +2293,16 @@ export function registerFileRoutes(api: Hono, adapter: StudioApiAdapter): void {
     const content = readFileSync(res.absPath);
     const version = fileContentVersion(content);
     c.header("ETag", version);
-    return c.json({ filename: res.filePath, content: content.toString("utf-8"), version });
+    // `missing: false` on the read path too, so its PRESENCE is what tells a
+    // caller this server distinguishes the two empty answers at all. Without
+    // it here, a real 0-byte file from a new server looks exactly like either
+    // case from an old one, and the split above buys nothing.
+    return c.json({
+      filename: res.filePath,
+      content: content.toString("utf-8"),
+      version,
+      missing: false,
+    });
   });
 
   // ── Write (overwrite) ──
diff --git a/packages/studio/src/hooks/useSdkSession.lifecycle.test.tsx b/packages/studio/src/hooks/useSdkSession.lifecycle.test.tsx
index 146ba5db5d..7894614166 100644
--- a/packages/studio/src/hooks/useSdkSession.lifecycle.test.tsx
+++ b/packages/studio/src/hooks/useSdkSession.lifecycle.test.tsx
@@ -440,7 +440,9 @@ describe("useSdkSession unavailable telemetry", () => {
   // so this is what "the composition genuinely is not there" looks like on the
   // wire — previously indistinguishable from a broken request. The shim and a
   // real 0-byte file are the same response, hence the name.
-  it("separates a file that is not on disk", async () => {
+  // An older server sends no `missing` field, so the combined label stays —
+  // rather than guessing one of the two and quietly corrupting the series.
+  it("keeps the combined label when the server does not say which empty this is", async () => {
     vi.stubGlobal(
       "fetch",
       vi.fn(async () => ({ ok: true, json: async () => ({ content: "" }) }) as Response),
@@ -459,6 +461,81 @@ describe("useSdkSession unavailable telemetry", () => {
     await act(async () => root.unmount());
   });
 
+  // `missing: true` is the route's shim — nothing resolved at that path.
+  it("reports a file the server could not find as absent", async () => {
+    vi.stubGlobal(
+      "fetch",
+      vi.fn(
+        async () => ({ ok: true, json: async () => ({ content: "", missing: true }) }) as Response,
+      ),
+    );
+    const root = createRoot(document.createElement("div"));
+    await act(async () => root.render(<Probe projectId="project-a" />));
+    await flushAsyncEffects();
+
+    expect(trackMock).toHaveBeenCalledWith("sdk_session_unavailable", {
+      stage: "read",
+      reason: "absent",
+      path_in_tree: null,
+    });
+    await act(async () => root.unmount());
+  });
+
+  // `missing: false` with empty content is a real 0-byte file on disk — a
+  // placeholder somebody created and has not written yet, not a bad path.
+  it("reports a real zero-byte file separately from an absent one", async () => {
+    vi.stubGlobal(
+      "fetch",
+      vi.fn(
+        async () => ({ ok: true, json: async () => ({ content: "", missing: false }) }) as Response,
+      ),
+    );
+    const root = createRoot(document.createElement("div"));
+    await act(async () => root.render(<Probe projectId="project-a" />));
+    await flushAsyncEffects();
+
+    expect(trackMock).toHaveBeenCalledWith("sdk_session_unavailable", {
+      stage: "read",
+      reason: "empty_file",
+      path_in_tree: null,
+    });
+    await act(async () => root.unmount());
+  });
+
+  // A 200 carrying HTML is an SPA fallback or a proxy answering in the route's
+  // place. Before this, `res.json()` rejected outside any catch and the outer
+  // catch filed it as `stage: "open"` — blaming the user's composition for a
+  // response the composition had nothing to do with.
+  it("reports a non-JSON 200 as a read failure, not a composition parse failure", async () => {
+    vi.stubGlobal(
+      "fetch",
+      vi.fn(
+        async () =>
+          ({
+            ok: true,
+            headers: { get: () => "text/html; charset=utf-8" },
+            json: async () => {
+              throw new SyntaxError(`Unexpected token '<', "<!-- /*!"... is not valid JSON`);
+            },
+          }) as unknown as Response,
+      ),
+    );
+    const root = createRoot(document.createElement("div"));
+    await act(async () => root.render(<Probe projectId="project-a" />));
+    await flushAsyncEffects();
+
+    expect(trackMock).toHaveBeenCalledWith("sdk_session_unavailable", {
+      stage: "read",
+      reason: "invalid_json",
+      content_type: "text/html; charset=utf-8",
+    });
+    expect(trackMock).not.toHaveBeenCalledWith(
+      "sdk_session_unavailable",
+      expect.objectContaining({ stage: "open" }),
+    );
+    await act(async () => root.unmount());
+  });
+
   // The graveyard-refuted fix's proposed next step: instrument, don't act.
   // `path_in_tree` separates "genuinely not in the loaded tree" from
   // "concurrent/duplicate open" without deciding anything on Studio's behalf.
diff --git a/packages/studio/src/hooks/useSdkSession.ts b/packages/studio/src/hooks/useSdkSession.ts
index 92067657f5..df327fe39a 100644
--- a/packages/studio/src/hooks/useSdkSession.ts
+++ b/packages/studio/src/hooks/useSdkSession.ts
@@ -35,16 +35,46 @@ import { addExternalFileReloadListener } from "./externalFileReloadBus";
  * before it left the browser — CSP `connect-src`, Private Network Access, an
  * extension rewriting `fetch` (rejects near-instantly, tab stays visible and
  * alive). Telemetry before this change could not tell the two apart.
+ *
+ * `absent` and `empty_file` split what `absent_or_empty` could not: the route
+ * answers `content: ""` both for a file it cannot find (the `optional=1` shim)
+ * and for a real 0-byte one, so the single label covered a composition nobody
+ * has written yet AND a path that does not resolve on this server. The route
+ * now marks the shim with `missing: true`. A server without that field still
+ * lands in `absent_or_empty`, so the old series stays honest rather than
+ * silently folding into one of the new ones.
+ *
+ * Measured on 0.8.62 before the split: 75 tabs hit this class and not one of
+ * them ever landed an SDK edit afterwards — it is terminal for the tab, while
+ * the tab itself keeps playing back and navigating, so nothing surfaces.
+ *
+ * `invalid_json` is a 200 whose body is not JSON at all — in production, an
+ * HTML page (`Unexpected token '<', "<!-- /*!"...`), i.e. an SPA fallback or a
+ * proxy answering in the route's place. `res.json()` rejected outside any
+ * catch until now, so it escaped this function and the effect's outer catch
+ * filed it as `stage: "open"` — which asserts the COMPOSITION failed to parse.
+ * Third label to make that same wrong claim, after `network` (#4240) and the
+ * fetch-rejection split; this one is the response shape, not the composition.
  */
 type ProjectFileReadFailure =
   | { ok: false; reason: "unsafe_path" }
   | { ok: false; reason: "http_error"; status: number; why?: string }
   | { ok: false; reason: "missing_content" }
   | { ok: false; reason: "absent_or_empty" }
+  | { ok: false; reason: "absent" }
+  | { ok: false; reason: "empty_file" }
+  | { ok: false; reason: "invalid_json"; contentType: string }
   | { ok: false; reason: "network"; elapsedMs: number; hidden: boolean };
 
 type ProjectFileReadResult = { ok: true; content: string } | ProjectFileReadFailure;
 
+/** The three ways a 200 can carry no composition — old combined label plus its split. */
+const EMPTY_READ_REASONS = new Set<ProjectFileReadFailure["reason"]>([
+  "absent_or_empty",
+  "absent",
+  "empty_file",
+]);
+
 /**
  * Record a read that produced no usable content, and answer which project — if
  * any — the failure identifies as unreachable.
@@ -73,12 +103,15 @@ function reportReadFailure(
     reason: read.reason,
     ...(read.reason === "http_error" ? { status: read.status, why: read.why } : {}),
     ...(read.reason === "network" ? { elapsed_ms: read.elapsedMs, hidden: read.hidden } : {}),
-    // Only meaningful for `absent_or_empty` — the graveyard-refuted "clear
+    ...(read.reason === "invalid_json" ? { content_type: read.contentType } : {}),
+    // Only meaningful for the empty-read reasons — the graveyard-refuted "clear
     // activeCompPath when it's not in the tree" fix's proposed next step,
     // scoped to instrumentation only. `null` while the tree hasn't loaded
     // yet: a false-negative there would read as "genuinely absent" when it is
-    // really "haven't looked".
-    ...(read.reason === "absent_or_empty" ? { path_in_tree: pathInTree } : {}),
+    // really "haven't looked". Carried on all three so the split keeps the
+    // signal that made it worth splitting: every measured case so far is
+    // `path_in_tree: true`, a file the tree lists and this read cannot get.
+    ...(EMPTY_READ_REASONS.has(read.reason) ? { path_in_tree: pathInTree } : {}),
   });
   if (read.reason !== "http_error") return null;
   return read.status === 404 ? projectId : null;
@@ -118,6 +151,30 @@ async function httpReadFailureWhy(res: Response): Promise<string | undefined> {
   return typeof body?.why === "string" ? body.why : undefined;
 }
 
+interface ProjectFileBody {
+  content?: string;
+  /** Present only from a server that separates its shim from a real 0-byte file. */
+  missing?: boolean;
+}
+
+/**
+ * Decide what a 200 with a JSON body actually delivered.
+ *
+ * `optional=1` answers a missing file with 200 + `content: ""`, so a non-string
+ * content is a response shape we did not expect, not absence. An empty string
+ * parses into a session with no elements, which declines every edit wholesale —
+ * not a session worth opening — and `missing` is the route saying which of the
+ * two empties it was. Without that field (older server) the combined label
+ * stands rather than guessing one and corrupting the series.
+ */
+function classifyProjectFileBody(data: ProjectFileBody): ProjectFileReadResult {
+  if (typeof data.content !== "string") return { ok: false, reason: "missing_content" };
+  if (data.content !== "") return { ok: true, content: data.content };
+  if (data.missing === true) return { ok: false, reason: "absent" };
+  if (data.missing === false) return { ok: false, reason: "empty_file" };
+  return { ok: false, reason: "absent_or_empty" };
+}
+
 /**
  * Read a project file's content (optional read — a missing file is not an
  * error). Replaces the removed SDK http adapter's `read()` — the only thing
@@ -146,16 +203,21 @@ async function readProjectFileOptional(
     const why = await httpReadFailureWhy(res);
     return { ok: false, reason: "http_error", status: res.status, ...(why ? { why } : {}) };
   }
-  const data = (await res.json()) as { content?: string };
-  // `optional=1` answers a missing file with 200 + `content: ""`, so a
-  // non-string here means a response shape we did not expect, not absence.
-  if (typeof data.content !== "string") return { ok: false, reason: "missing_content" };
-  // An empty body parses into a session with no elements, which declines every
-  // edit wholesale — not a session worth opening. The absent-file shim and a
-  // genuinely 0-byte file are the same 200 on the wire and cannot be told
-  // apart here, hence the name; for a composition it is always the former.
-  if (data.content === "") return { ok: false, reason: "absent_or_empty" };
-  return { ok: true, content: data.content };
+  // A 200 that is not JSON is a different failure from a JSON body missing its
+  // field: it means something other than this route answered — an SPA fallback
+  // or a proxy. Left unguarded, it rejected out of this function entirely and
+  // the effect's outer catch blamed the composition. See the type's comment.
+  let data: ProjectFileBody;
+  try {
+    data = (await res.json()) as ProjectFileBody;
+  } catch {
+    return {
+      ok: false,
+      reason: "invalid_json",
+      contentType: res.headers.get("content-type") ?? "",
+    };
+  }
+  return classifyProjectFileBody(data);
 }
 
 /**