feat(KB-180): add planning route diagnostics and harden JSON error handling

- Add API diagnostics to planning routes for better debugging - Harden API JSON error handling with improved validation - Add comprehensive tests for planning route JSON responses - Update server routes and API layer for robustness
2026-03-31 01:38:43 -07:00
parent 9e03c47b91
commit aef82c2df9
5 changed files with 144 additions and 53 deletions
--- a/packages/dashboard/app/api.test.ts
+++ b/packages/dashboard/app/api.test.ts
@@ -16,6 +16,8 @@ import {
  fetchWorkspaceFileList,
  fetchWorkspaceFileContent,
  saveWorkspaceFileContent,
+  startPlanningStreaming,
+  fetchTasks,
 } from "./api";
 import type { Task, TaskDetail, BatchStatusResponse } from "@kb/core";

@@ -1109,7 +1111,7 @@ describe("API Error Handling", () => {
  });

  describe("Non-JSON success responses", () => {
-    it("throws error for successful non-JSON response", async () => {
+    it("throws a descriptive HTML fallback error including the endpoint URL", async () => {
      globalThis.fetch = vi.fn().mockReturnValue(
        Promise.resolve({
          ok: true,
@@ -1124,11 +1126,10 @@ describe("API Error Handling", () => {
        } as unknown as Response)
      );

-      await expect(fetchTasks()).rejects.toThrow("Unexpected response format: expected JSON, got text/html");
+      await expect(fetchTasks()).rejects.toThrow("API returned HTML instead of JSON for /api/tasks");
    });

-    it("includes response preview for unexpected success format", async () => {
-      const htmlResponse = "<html><body>SPA Fallback</body></html>";
+    it("includes planning endpoint URL and status when HTML is returned", async () => {
      globalThis.fetch = vi.fn().mockReturnValue(
        Promise.resolve({
          ok: true,
@@ -1139,16 +1140,18 @@ describe("API Error Handling", () => {
              name.toLowerCase() === "content-type" ? "text/html" : null,
          },
          json: () => Promise.reject(new Error("JSON parse error")),
-          text: () => Promise.resolve(htmlResponse),
+          text: () => Promise.resolve("<!DOCTYPE html><html><body>SPA Fallback</body></html>"),
        } as unknown as Response)
      );

-      await expect(fetchTasks()).rejects.toThrow("(Response: <html><body>SPA Fallback</body></html>)");
+      await expect(startPlanningStreaming("Build auth")).rejects.toThrow(
+        "API returned HTML instead of JSON for /api/planning/start-streaming. The endpoint may not be properly configured. (200 OK)"
+      );
    });
  });

  describe("JSON parsing edge cases", () => {
-    it("handles invalid JSON in error response with JSON content-type", async () => {
+    it("reports invalid JSON with the endpoint URL", async () => {
      globalThis.fetch = vi.fn().mockReturnValue(
        Promise.resolve({
          ok: false,
@@ -1163,9 +1166,9 @@ describe("API Error Handling", () => {
        } as unknown as Response)
      );

-      // Should fall back to text-based error when JSON parsing fails
-      await expect(fetchTasks()).rejects.toThrow("Request failed: 500 Internal Server Error");
-      await expect(fetchTasks()).rejects.toThrow("Response:");
+      await expect(fetchTasks()).rejects.toThrow(
+        "API returned invalid JSON for /api/tasks. (500 Internal Server Error)"
+      );
    });
  });
 });
--- a/packages/dashboard/app/api.ts
+++ b/packages/dashboard/app/api.ts
@@ -16,60 +16,55 @@ import type {
 import type { PlanningQuestion, PlanningSummary, PlanningResponse } from "@kb/core";
 import type { ScheduledTask, ScheduledTaskCreateInput, ScheduledTaskUpdateInput, AutomationRunResult } from "@kb/core";

+function looksLikeHtml(body: string): boolean {
+  const trimmed = body.trim();
+  return trimmed.startsWith("<!DOCTYPE") || trimmed.startsWith("<html") || trimmed.startsWith("<HTML");
+}
+
+function buildApiUrl(path: string): string {
+  return `/api${path}`;
+}
+
 async function api<T = unknown>(path: string, opts: RequestInit = {}): Promise<T> {
-  const res = await fetch(`/api${path}`, {
+  const url = buildApiUrl(path);
+  const res = await fetch(url, {
    headers: { "Content-Type": "application/json" },
    ...opts,
  });

-  // Check if response is JSON before attempting to parse
  const contentType = res.headers.get("content-type") ?? "";
+  const bodyText = await res.text();
  const isJson = contentType.includes("application/json");
-  const isHtml = contentType.includes("text/html");
+  const isHtml = contentType.includes("text/html") || looksLikeHtml(bodyText);
+
+  if (isHtml) {
+    throw new Error(
+      `API returned HTML instead of JSON for ${url}. ` +
+      `The endpoint may not be properly configured. (${res.status} ${res.statusText})`
+    );
+  }
+
+  if (!isJson) {
+    const preview = bodyText.length > 160 ? `${bodyText.slice(0, 160)}...` : bodyText;
+    throw new Error(
+      `API returned ${contentType || "an unknown content type"} instead of JSON for ${url}. ` +
+      `(${res.status} ${res.statusText})${preview ? ` Response: ${preview}` : ""}`
+    );
+  }
+
+  let data: unknown;
+  try {
+    data = bodyText ? JSON.parse(bodyText) : null;
+  } catch {
+    throw new Error(
+      `API returned invalid JSON for ${url}. (${res.status} ${res.statusText})`
+    );
+  }

  if (!res.ok) {
-    // For error responses, read as text first
-    const text = await res.text();
-    
-    // Try to extract error from JSON if content-type indicates JSON
-    if (isJson) {
-      try {
-        const data = JSON.parse(text) as { error?: string };
-        throw new Error(data.error || `Request failed: ${res.status} ${res.statusText}`);
-      } catch {
-        // JSON parsing failed - fall through to text-based error below
-      }
-    }
-    
-    // HTML error response (SPA fallback case) - show user-friendly message
-    if (isHtml || text.trim().startsWith("<!DOCTYPE") || text.trim().startsWith("<html")) {
-      throw new Error("Server returned an unexpected response. Please refresh and try again.");
-    }
-    
-    // Non-JSON error response: use status text with truncated body preview
-    const textPreview = text.length > 100 ? text.slice(0, 100) + "..." : text;
-    const message = textPreview 
-      ? `Request failed: ${res.status} ${res.statusText} (Response: ${textPreview})`
-      : `Request failed: ${res.status} ${res.statusText}`;
-    throw new Error(message);
+    throw new Error((data as { error?: string } | null)?.error || `Request failed for ${url}: ${res.status} ${res.statusText}`);
  }

-  // For successful responses, parse as JSON if content-type indicates JSON
-  if (!isJson) {
-    // Unexpected non-JSON success response (likely HTML from SPA fallback)
-    const text = await res.text();
-    
-    // Check if it looks like HTML
-    if (isHtml || text.trim().startsWith("<!DOCTYPE") || text.trim().startsWith("<html")) {
-      throw new Error("Server returned an unexpected response. Please refresh and try again.");
-    }
-    
-    // Other non-JSON response
-    const textPreview = text.length > 100 ? text.slice(0, 100) + "..." : text;
-    throw new Error(`Unexpected response format: expected JSON, got ${contentType || "unknown"}${textPreview ? ` (Response: ${textPreview})` : ""}`);
-  }
-
-  const data = await res.json();
  return data as T;
 }

--- a/packages/dashboard/src/routes.ts
+++ b/packages/dashboard/src/routes.ts
@@ -565,6 +565,18 @@ function pushGitBranch(): GitPushResult {

 export function createApiRoutes(store: TaskStore, options?: ServerOptions): Router {
  const router = Router();
+
+  if (process.env.KB_DEBUG_PLANNING_ROUTES === "1") {
+    const planningRoutes = [
+      "POST /planning/start",
+      "POST /planning/start-streaming",
+      "POST /planning/respond",
+      "POST /planning/cancel",
+      "POST /planning/create-task",
+      "GET /planning/:sessionId/stream",
+    ];
+    console.debug("[planning:routes:registered]", planningRoutes);
+  }
  const sessionFilesCache = new Map<string, { files: string[]; expiresAt: number }>();

  // Get GitHub token from options or env
--- a/packages/dashboard/src/server.test.ts
+++ b/packages/dashboard/src/server.test.ts
@@ -50,6 +50,38 @@ async function GET(app: express.Express, path: string): Promise<{ status: number
  });
 }

+async function REQUEST(
+  app: express.Express,
+  method: string,
+  path: string,
+  body?: string,
+  headers?: Record<string, string>,
+): Promise<{ status: number; body: unknown; headers: http.IncomingHttpHeaders }> {
+  return new Promise((resolve, reject) => {
+    const server = app.listen(0, () => {
+      const addr = server.address() as { port: number };
+      const req = http.request(
+        { hostname: "127.0.0.1", port: addr.port, path, method, headers },
+        (res) => {
+          let data = "";
+          res.on("data", (chunk) => (data += chunk));
+          res.on("end", () => {
+            server.close();
+            try {
+              resolve({ status: res.statusCode!, body: JSON.parse(data), headers: res.headers });
+            } catch {
+              resolve({ status: res.statusCode!, body: data, headers: res.headers });
+            }
+          });
+        },
+      );
+      req.on("error", (err) => { server.close(); reject(err); });
+      if (body) req.write(body);
+      req.end();
+    });
+  });
+}
+
 describe("API Error Handling Middleware", () => {
  let store: TaskStore;

@@ -110,4 +142,40 @@ describe("API Error Handling Middleware", () => {
      }
    });
  });
+
+  describe("planning API route content types", () => {
+    it("returns JSON for all POST planning endpoints instead of falling through to SPA HTML", async () => {
+      const endpoints = [
+        "/api/planning/start",
+        "/api/planning/start-streaming",
+        "/api/planning/respond",
+        "/api/planning/cancel",
+        "/api/planning/create-task",
+      ];
+
+      for (const path of endpoints) {
+        const app = createServer(store);
+        const res = await REQUEST(app, "POST", path, JSON.stringify({}), {
+          "Content-Type": "application/json",
+        });
+
+        expect(res.headers["content-type"]).toContain("application/json");
+        if (typeof res.body === "string") {
+          expect(res.body).not.toContain("<!DOCTYPE html>");
+          expect(res.body).not.toContain("<html");
+        }
+      }
+    });
+
+    it("returns JSON 404s for unmatched planning API routes", async () => {
+      const app = createServer(store);
+      const res = await REQUEST(app, "POST", "/api/planning/not-a-route", JSON.stringify({}), {
+        "Content-Type": "application/json",
+      });
+
+      expect(res.status).toBe(404);
+      expect(res.headers["content-type"]).toContain("application/json");
+      expect(res.body).toEqual({ error: "Not found" });
+    });
+  });
 });
--- a/packages/dashboard/src/server.ts
+++ b/packages/dashboard/src/server.ts
@@ -166,6 +166,19 @@ export function createServer(store: TaskStore, options?: ServerOptions): ReturnT
  // Rate limiting — mutation endpoints (POST/PUT/PATCH/DELETE)
  app.use("/api", rateLimit(RATE_LIMITS.api));

+  // Planning route diagnostics for production/runtime debugging. Disabled by default.
+  if (process.env.KB_DEBUG_PLANNING_ROUTES === "1") {
+    app.use("/api/planning", (req, _res, next) => {
+      console.debug("[planning:request]", {
+        method: req.method,
+        path: req.path,
+        originalUrl: req.originalUrl,
+        contentType: req.headers["content-type"],
+      });
+      next();
+    });
+  }
+
  // REST API
  app.use("/api", createApiRoutes(store, options));