Merge PR #4446: fix(ai): persist provider error response in http-400 dumps (@mattwilkinsonn)

packages/ai/src/utils/http-inspector.ts

@@ -1,5 +1,5 @@
 import * as path from "node:path";
-import { getLogsDir, isBunTestRuntime } from "@oh-my-pi/pi-utils";
+import { extractHttpStatusFromError, getLogsDir, isBunTestRuntime } from "@oh-my-pi/pi-utils";
 import * as AIError from "../error/flags";
 import { isCopilotTransientModelError } from "./retry.js";
 import { formatErrorMessageWithRetryAfter } from "./retry-after.js";
@@ -23,22 +23,50 @@ export type CapturedHttpErrorResponse = {
 
 const SENSITIVE_HEADERS = ["authorization", "x-api-key", "api-key", "cookie", "set-cookie", "proxy-authorization"];
 
+/**
+ * Build the JSON persisted for a rejected request. Request fields stay at the
+ * top level (so existing dump parsers still read `body`); the provider's error
+ * is added under `errorResponse` so a failed request is diagnosable from the
+ * dump file rather than the request alone.
+ */
+export function buildHttp400DumpPayload(
+	dump: RawHttpRequestDump,
+	error: unknown,
+	message: string,
+): RawHttpRequestDump & { errorResponse: { status: number | undefined; message: string } } {
+	return {
+		...sanitizeDump(dump),
+		errorResponse: { status: extractHttpStatusFromError(error), message },
+	};
+}
+
+/** HTTP statuses whose rejected request we persist for post-hoc diagnosis: the
+ *  request-content rejections that wedge a session. 400 (bad request) and 413
+ *  (payload too large — an oversized image / snapcompact frame payload that 413s
+ *  and empties the turn). Auth (401/403), not-found (404), rate limits and 5xx
+ *  are excluded: 429/5xx are retried, so persisting them here would write one
+ *  dump per attempt. */
+export function shouldDumpRejectedRequest(error: unknown): boolean {
+	const status = AIError.status(error);
+	return status === 400 || status === 413;
+}
+
 export async function appendRawHttpRequestDumpFor400(
 	message: string,
 	error: unknown,
 	dump: RawHttpRequestDump | undefined,
 ): Promise<string> {
 	// Never persist dumps under the test runner: providers exercise the 400 path
-	if (!dump || isBunTestRuntime() || AIError.status(error) !== 400) {
+	if (!dump || isBunTestRuntime() || !shouldDumpRejectedRequest(error)) {
 		return message;
 	}
 
-	const sanitizedDump = sanitizeDump(dump);
-	const fileName = `${Date.now()}-${Bun.hash(JSON.stringify(sanitizedDump)).toString(36)}.json`;
+	const payload = buildHttp400DumpPayload(dump, error, message);
+	const fileName = `${Date.now()}-${Bun.hash(JSON.stringify(payload)).toString(36)}.json`;
 	const filePath = path.join(getLogsDir(), "http-400-requests", fileName);
 
 	try {
-		await Bun.write(filePath, `${JSON.stringify(sanitizedDump, null, 2)}\n`);
+		await Bun.write(filePath, `${JSON.stringify(payload, null, 2)}\n`);
 		return `${message}\nraw-http-request=${filePath}`;
 	} catch (writeError) {
 		const writeMessage = writeError instanceof Error ? writeError.message : String(writeError);

packages/ai/test/http-inspector.test.ts

@@ -0,0 +1,61 @@
+import { describe, expect, it } from "bun:test";
+import {
+	buildHttp400DumpPayload,
+	type RawHttpRequestDump,
+	shouldDumpRejectedRequest,
+} from "@oh-my-pi/pi-ai/utils/http-inspector";
+
+class HttpError extends Error {
+	constructor(
+		readonly status: number,
+		message: string,
+	) {
+		super(message);
+	}
+}
+
+const dump: RawHttpRequestDump = {
+	provider: "anthropic",
+	api: "anthropic-messages",
+	model: "claude-opus-4-8",
+	method: "POST",
+	url: "https://api.anthropic.com/v1/messages",
+	headers: { "x-api-key": "secret-key", "content-type": "application/json" },
+	body: { messages: [{ role: "user", content: "hi" }] },
+};
+
+describe("buildHttp400DumpPayload", () => {
+	it("keeps request fields top-level and records the provider error response", () => {
+		const message = "400 image exceeds 5 MB limit";
+		const payload = buildHttp400DumpPayload(dump, new HttpError(400, message), message);
+
+		expect(payload.provider).toBe("anthropic");
+		expect(payload.url).toBe("https://api.anthropic.com/v1/messages");
+		expect(payload.body).toEqual({ messages: [{ role: "user", content: "hi" }] });
+		expect(payload.errorResponse).toEqual({ status: 400, message });
+	});
+
+	it("redacts sensitive request headers while keeping the rest", () => {
+		const payload = buildHttp400DumpPayload(dump, new HttpError(400, "x"), "x");
+
+		expect(payload.headers?.["x-api-key"]).toBe("[redacted]");
+		expect(payload.headers?.["content-type"]).toBe("application/json");
+	});
+});
+
+describe("shouldDumpRejectedRequest", () => {
+	it("captures request-content rejections (400 bad request, 413 payload too large)", () => {
+		expect(shouldDumpRejectedRequest(new HttpError(400, "bad request"))).toBe(true);
+		expect(shouldDumpRejectedRequest(new HttpError(413, "payload too large"))).toBe(true);
+	});
+
+	it("skips auth, not-found, rate-limit, and retried 5xx errors that would spam dumps", () => {
+		for (const status of [401, 403, 404, 429, 500, 502, 503, 504]) {
+			expect(shouldDumpRejectedRequest(new HttpError(status, "x"))).toBe(false);
+		}
+	});
+
+	it("skips errors without an HTTP status", () => {
+		expect(shouldDumpRejectedRequest(new Error("network reset"))).toBe(false);
+	});
+});