fix(http): map non-JSON 200 responses to a typed error envelope (#166)

Davidson3556 · web-flow · commit 0de372c51d12 · 2026-07-05T12:31:15.000-07:00
A successful (200) response whose body is not JSON — a misconfigured endpoint, a proxy / captive-portal / login page returning HTML with a success status, or an empty body — caused the OK path to call raw `response.json()`, whose SyntaxError escaped to the top-level handler. That produced an opaque exit 1 and, under --output json, a bare `{"error":"<parse message>"}` that breaks the typed-envelope contract every other error honors (the non-OK path already reads defensively via safeReadJson). Wrap the OK-path parse failure in a typed INTERNAL ApiError (exit 1 unchanged) that carries the requestId, names the likely cause, and points the operator at their endpoint config; details include the HTTP status, content-type, and underlying parse message. Abort/timeout errors mid-read keep their existing classification. Fixes #94
diff --git a/src/lib/http.test.ts b/src/lib/http.test.ts
@@ -140,6 +140,53 @@ describe('HttpClient happy path', () => {
   });
 });
 
+describe('HttpClient — 200 response with a non-JSON body', () => {
+  function htmlResponse(status = 200): Response {
+    return new Response('<!DOCTYPE html><html><body>Login</body></html>', {
+      status,
+      headers: { 'content-type': 'text/html' },
+    });
+  }
+
+  it('throws a typed INTERNAL ApiError (not a raw SyntaxError) with the requestId and details', async () => {
+    const fetchImpl = vi.fn(async () => htmlResponse());
+    const client = makeClient(fetchImpl as unknown as typeof fetch);
+    let caught: unknown;
+    try {
+      await client.get('/me');
+    } catch (err) {
+      caught = err;
+    }
+    expect(caught).toBeInstanceOf(ApiError);
+    const apiErr = caught as ApiError;
+    expect(apiErr.code).toBe('INTERNAL');
+    expect(apiErr.exitCode).toBe(1);
+    expect(apiErr.requestId).toMatch(/^cli_/);
+    expect(apiErr.message).toContain('non-JSON response');
+    expect(apiErr.nextAction).toContain('TESTSPRITE_API_URL');
+    expect(apiErr.details).toMatchObject({ httpStatus: 200, contentType: 'text/html' });
+    expect(String(apiErr.details.parseError)).toContain('JSON');
+  });
+
+  it('does not retry a malformed 200 body (single fetch call)', async () => {
+    const fetchImpl = vi.fn(async () => htmlResponse());
+    const client = makeClient(fetchImpl as unknown as typeof fetch);
+    await expect(client.get('/me')).rejects.toBeInstanceOf(ApiError);
+    // A non-JSON success body is a hard config error, not a transient transport
+    // failure — it must not burn the retry budget.
+    expect(fetchImpl).toHaveBeenCalledTimes(1);
+  });
+
+  it('handles an empty 200 body the same way', async () => {
+    const fetchImpl = vi.fn(
+      async () =>
+        new Response('', { status: 200, headers: { 'content-type': 'application/json' } }),
+    );
+    const client = makeClient(fetchImpl as unknown as typeof fetch);
+    await expect(client.get('/me')).rejects.toMatchObject({ code: 'INTERNAL', exitCode: 1 });
+  });
+});
+
 describe('HttpClient error mapping', () => {
   it('does not retry AUTH_INVALID and exits 3', async () => {
     const fetchImpl = vi.fn(async () => errorEnvelopeResponse(401, 'AUTH_INVALID'));
diff --git a/src/lib/http.ts b/src/lib/http.ts
@@ -498,7 +498,13 @@ export class HttpClient {
         } catch (err) {
           // A timeout/abort can fire mid-body-read (headers received, stream stalls).
           this.rethrowIfAbort(err, timeoutSignal, options.signal, requestId);
-          throw err;
+          // Otherwise the successful response body was not valid JSON — a
+          // misconfigured endpoint, a proxy / captive-portal / login page that
+          // returns HTML with a 200 status, or an empty body. Surface a typed
+          // error carrying the requestId instead of letting the raw SyntaxError
+          // escape to index.ts, where it would print a bare `{"error":"..."}`
+          // and break the --output json envelope contract.
+          throw malformedResponseError(response, requestId, err);
         }
       }
 
@@ -692,6 +698,53 @@ async function safeReadJson(response: Response): Promise<unknown> {
   }
 }
 
+/**
+ * Build a typed error for a successful response whose body could not be parsed
+ * as JSON.
+ *
+ * The CLI expects every API response to be a JSON envelope. When a `200 OK`
+ * carries a non-JSON body — a misconfigured endpoint, a proxy / captive-portal
+ * / SSO login page returning HTML with a success status, or an empty body —
+ * `response.json()` throws a raw `SyntaxError`. Left unhandled it escapes to
+ * the top-level handler in `index.ts`, which prints a bare
+ * `{"error":"<parse message>"}` under `--output json` (breaking the
+ * typed-envelope contract every other error honors) and gives the operator no
+ * actionable context.
+ *
+ * This wraps it in a typed `INTERNAL` `ApiError` (exit 1, unchanged) that
+ * carries the `requestId`, names the likely cause, and points the operator at
+ * their endpoint configuration. `details` includes the HTTP status, the
+ * response `content-type` (when present), and the underlying parse message.
+ */
+export function malformedResponseError(
+  response: Response,
+  requestId: string,
+  cause: unknown,
+): ApiError {
+  const contentType = response.headers.get('content-type') ?? undefined;
+  const parseError = cause instanceof Error ? cause.message : String(cause);
+  const contentTypeNote = contentType ? ` (content-type: ${contentType})` : '';
+  return new ApiError(
+    {
+      code: 'INTERNAL',
+      message:
+        `The server returned a non-JSON response${contentTypeNote} for an HTTP ${response.status}. ` +
+        `This usually means the endpoint is not the TestSprite API — a proxy, captive portal, or ` +
+        `login page can return HTML with a success status.`,
+      nextAction:
+        'Check that --endpoint-url / TESTSPRITE_API_URL points at the TestSprite API ' +
+        '(default https://api.testsprite.com), then retry.',
+      requestId,
+      details: {
+        httpStatus: response.status,
+        ...(contentType ? { contentType } : {}),
+        parseError,
+      },
+    },
+    response.status,
+  );
+}
+
 export function parseRetryAfter(headerValue: string | null): number | undefined {
   if (!headerValue) return undefined;
   const numeric = Number(headerValue);
