fix: address review nits — refactor fallbacks, add unit tests (FAR-122)

- Merge both one-shot log fallbacks into a single conditional block using a
  cheap string-scan guard (`stdout.includes('"type":"result"')`) to avoid
  calling parseClaudeStreamJson twice and prevent double readPodLogs calls
  when the first fallback already ran.

- Extract error-message logic into `buildPartialRunError(exitCode, model, stdout)`
  (exported for tests) so the `!parsed` branch is a one-liner and the logic
  is independently testable.

- Export `isK8s404` for tests.

- Add execute.test.ts with 15 unit tests covering:
  - isK8s404: v0.x response.statusCode, v1.0+ response.status, direct
    statusCode, message-based detection, non-404 codes
  - buildPartialRunError: exitCode=0 path, empty stdout, init-only output
    (model surfaced), first non-system content line, null exitCode (-1),
    multiple consecutive system events

Co-Authored-By: Paperclip <noreply@paperclip.ing>

src/server/execute.test.ts

@@ -0,0 +1,108 @@
+import { describe, it, expect } from "vitest";
+import { isK8s404, buildPartialRunError } from "./execute.js";
+
+describe("isK8s404", () => {
+  it("returns false for non-Error values", () => {
+    expect(isK8s404(null)).toBe(false);
+    expect(isK8s404(undefined)).toBe(false);
+    expect(isK8s404("string error")).toBe(false);
+    expect(isK8s404(404)).toBe(false);
+  });
+
+  it("returns false for unrelated errors", () => {
+    expect(isK8s404(new Error("something went wrong"))).toBe(false);
+    expect(isK8s404(new Error("HTTP-Code: 500 Message: Internal Server Error"))).toBe(false);
+  });
+
+  it("detects 404 from v1.0+ message format", () => {
+    const err = new Error("HTTP-Code: 404 Message: Unknown API Status Code! Body: ...");
+    expect(isK8s404(err)).toBe(true);
+  });
+
+  it("detects 404 from v0.x response.statusCode", () => {
+    const err = Object.assign(new Error("Not Found"), {
+      response: { statusCode: 404 },
+    });
+    expect(isK8s404(err)).toBe(true);
+  });
+
+  it("detects 404 from v1.0+ response.status", () => {
+    const err = Object.assign(new Error("Not Found"), {
+      response: { status: 404 },
+    });
+    expect(isK8s404(err)).toBe(true);
+  });
+
+  it("detects 404 from direct statusCode property", () => {
+    const err = Object.assign(new Error("Not Found"), { statusCode: 404 });
+    expect(isK8s404(err)).toBe(true);
+  });
+
+  it("does not match non-404 status codes on response", () => {
+    const err = Object.assign(new Error("Forbidden"), {
+      response: { statusCode: 403 },
+    });
+    expect(isK8s404(err)).toBe(false);
+  });
+});
+
+describe("buildPartialRunError", () => {
+  const initLine = JSON.stringify({
+    type: "system",
+    subtype: "init",
+    model: "claude-sonnet-4-6",
+    session_id: "sess_abc",
+  });
+
+  it("returns parse-failure message when exitCode is 0", () => {
+    expect(buildPartialRunError(0, "", "")).toBe("Failed to parse Claude JSON output");
+    expect(buildPartialRunError(0, "claude-sonnet-4-6", initLine)).toBe(
+      "Failed to parse Claude JSON output",
+    );
+  });
+
+  it("returns generic exit message when stdout is empty", () => {
+    expect(buildPartialRunError(1, "", "")).toBe("Claude exited with code 1");
+    expect(buildPartialRunError(null, "", "")).toBe("Claude exited with code -1");
+  });
+
+  it("skips system/init events and returns generic message when only init captured", () => {
+    const msg = buildPartialRunError(1, "claude-sonnet-4-6", initLine);
+    expect(msg).toBe(
+      "Claude started but did not produce a result (model: claude-sonnet-4-6) — check API credentials, model support, and adapter config",
+    );
+  });
+
+  it("includes model from parsedStream when stdout is init-only", () => {
+    const msg = buildPartialRunError(null, "MiniMax-M2.7", initLine);
+    expect(msg).toContain("MiniMax-M2.7");
+    expect(msg).not.toContain("type");
+    expect(msg).not.toContain("system");
+  });
+
+  it("uses first non-system line as content when present", () => {
+    const stdout = [initLine, "Error: no API key configured"].join("\n");
+    const msg = buildPartialRunError(1, "claude-sonnet-4-6", stdout);
+    expect(msg).toBe("Claude exited with code 1: Error: no API key configured");
+  });
+
+  it("uses first non-system JSON event as content", () => {
+    const resultLike = JSON.stringify({ type: "result", subtype: "error", result: "rate limit" });
+    const stdout = [initLine, resultLike].join("\n");
+    const msg = buildPartialRunError(2, "claude-sonnet-4-6", stdout);
+    expect(msg).toContain("rate limit");
+    expect(msg).toContain("code 2");
+  });
+
+  it("null exitCode renders as -1 in message", () => {
+    const msg = buildPartialRunError(null, "", "Some plain error text");
+    expect(msg).toBe("Claude exited with code -1: Some plain error text");
+  });
+
+  it("skips multiple consecutive system events", () => {
+    const anotherSystem = JSON.stringify({ type: "system", subtype: "other" });
+    const stdout = [initLine, anotherSystem, "real error line"].join("\n");
+    const msg = buildPartialRunError(1, "model-x", stdout);
+    expect(msg).toBe("Claude exited with code 1: real error line");
+  });
+});

src/server/execute.ts

@@ -19,8 +19,9 @@ const MAX_LOG_RECONNECT_ATTEMPTS = 50;
 /**
  * Detect a Kubernetes 404 (Not Found) error from @kubernetes/client-node.
  * Works for both v0.x (response.statusCode) and v1.0+ (response.status, message).
+ * Exported for unit tests.
  */
-function isK8s404(err: unknown): boolean {
+export function isK8s404(err: unknown): boolean {
   if (!(err instanceof Error)) return false;
   const e = err as unknown as Record<string, unknown>;
   const resp = e.response as Record<string, unknown> | undefined;
@@ -29,6 +30,45 @@ function isK8s404(err: unknown): boolean {
   return /HTTP-Code:\s*404\b/.test(err.message);
 }
 
+/**
+ * Build the error message when Claude's stdout contains no result event.
+ * Skips system/init event lines so the UI doesn't display the raw init JSON.
+ * Exported for unit tests.
+ */
+export function buildPartialRunError(
+  exitCode: number | null,
+  model: string,
+  stdout: string,
+): string {
+  if (exitCode === 0) return "Failed to parse Claude JSON output";
+
+  // Walk stdout lines, skip system events, return the first real content line.
+  const firstContentLine = stdout.split(/\r?\n/)
+    .map((l) => l.trim())
+    .find((l) => {
+      if (!l) return false;
+      try {
+        const obj = JSON.parse(l);
+        if (typeof obj === "object" && obj !== null && (obj as Record<string, unknown>).type === "system") return false;
+      } catch {
+        // not JSON — treat as content
+      }
+      return true;
+    }) ?? "";
+
+  // If we only have system/init events and nothing else, surface the model
+  // name so the operator can diagnose missing credentials or unsupported model.
+  const initOnlyOutput = stdout.trim() !== "" && model !== "" && !firstContentLine;
+  if (initOnlyOutput) {
+    const modelHint = model ? ` (model: ${model})` : "";
+    return `Claude started but did not produce a result${modelHint} — check API credentials, model support, and adapter config`;
+  }
+
+  return firstContentLine
+    ? `Claude exited with code ${exitCode ?? -1}: ${firstContentLine}`
+    : `Claude exited with code ${exitCode ?? -1}`;
+}
+
 /**
  * Wait for the Job's pod to reach a terminal or running state.
  * Returns the pod name once logs can be streamed, or throws on failure.
@@ -640,26 +680,27 @@ export async function execute(ctx: AdapterExecutionContext): Promise<AdapterExec
       stdout = logResult.value;
     }
 
-    // If the follow stream missed output (container exited quickly), do a
-    // one-shot log read as fallback before the pod is cleaned up.
-    if (!stdout.trim()) {
-      await onLog("stdout", `[paperclip] Log stream returned empty — reading pod logs directly...\n`);
-      stdout = await readPodLogs(namespace, podName, kubeconfigPath);
-      if (stdout.trim()) {
-        await onLog("stdout", stdout);
+    // One-shot log fallback: handles two failure modes with a single read.
+    // Mode 1 — empty stream: the follow stream returned nothing (fast exit before connection).
+    // Mode 2 — partial stream: we have some output but no result event (follow stream raced
+    //   with container exit and captured only the init line before the connection dropped).
+    // A one-shot readPodLogs is more reliable for already-terminated containers and reads
+    // from the beginning of the log, giving us the full output.
+    // We use a cheap string scan for the result-event guard (avoids a full JSON parse here;
+    // the authoritative parse happens once below after all fallbacks complete).
+    const hasResultEvent = stdout.includes('"type":"result"');
+    const needsOneShot = !stdout.trim() || (stdout.trim() && !hasResultEvent);
+    if (needsOneShot) {
+      if (!stdout.trim()) {
+        await onLog("stdout", `[paperclip] Log stream returned empty — reading pod logs directly...\n`);
       }
-    }
-
-    // Second fallback: if we got some output but it contains no result event,
-    // the follow stream may have raced with a fast container exit (capturing
-    // only the init line before the connection dropped).  A one-shot read of
-    // the full log is more reliable for already-terminated containers and may
-    // return the complete output.
-    if (stdout.trim() && !parseClaudeStreamJson(stdout).resultJson) {
-      const fullLogs = await readPodLogs(namespace, podName, kubeconfigPath);
-      if (fullLogs && fullLogs.length > stdout.length) {
+      const oneShotLogs = await readPodLogs(namespace, podName, kubeconfigPath);
+      if (!stdout.trim() && oneShotLogs.trim()) {
+        stdout = oneShotLogs;
+        await onLog("stdout", stdout);
+      } else if (oneShotLogs && oneShotLogs.length > stdout.length) {
         await onLog("stdout", `[paperclip] Log stream captured partial output — supplemental one-shot read returned more content.\n`);
-        stdout = fullLogs;
+        stdout = oneShotLogs;
       }
     }
 
@@ -752,39 +793,11 @@ export async function execute(ctx: AdapterExecutionContext): Promise<AdapterExec
   }
 
   if (!parsed) {
-    // Find the first stdout line that is NOT a system/init event.
-    // Using the system/init JSON blob as the error message produces a huge,
-    // unreadable error in the UI.  Skip those and use the first real content line.
-    const firstContentLine = stdout.split(/\r?\n/)
-      .map((l) => l.trim())
-      .find((l) => {
-        if (!l) return false;
-        try {
-          const obj = JSON.parse(l);
-          if (typeof obj === "object" && obj !== null && (obj as Record<string, unknown>).type === "system") return false;
-        } catch {
-          // not JSON — treat as content
-        }
-        return true;
-      }) ?? "";
-
-    // If we got an init event but nothing else, give a specific message that
-    // names the model so it is easier to diagnose (e.g. unsupported model,
-    // missing API credentials).
-    const initOnlyOutput = stdout.trim() !== "" && parsedStream.model !== "" && !firstContentLine;
-    const modelHint = parsedStream.model ? ` (model: ${parsedStream.model})` : "";
-
     return {
       exitCode,
       signal: null,
       timedOut: false,
-      errorMessage: exitCode === 0
-        ? "Failed to parse Claude JSON output"
-        : initOnlyOutput
-          ? `Claude started but did not produce a result${modelHint} — check API credentials, model support, and adapter config`
-          : firstContentLine
-            ? `Claude exited with code ${exitCode ?? -1}: ${firstContentLine}`
-            : `Claude exited with code ${exitCode ?? -1}`,
+      errorMessage: buildPartialRunError(exitCode, parsedStream.model, stdout),
       resultJson: { stdout },
     };
   }