From d8e17a22f3518dda965cae9dad8a320d035f07e4 Mon Sep 17 00:00:00 2001 From: Matt Wilkinson Date: Fri, 26 Jun 2026 12:49:54 -0400 Subject: [PATCH 1/2] fix(ai): persist provider error response in http-400 dumps appendRawHttpRequestDumpFor400 wrote only the sanitized request, so a 400 dump under ~/.omp/logs/http-400-requests/ could not show why the provider rejected it. The persisted payload now records the error status and message under errorResponse; request fields stay top-level for existing parsers. Refs can1357/oh-my-pi#3578 --- packages/ai/src/utils/http-inspector.ts | 25 +++++++++++++--- packages/ai/test/http-inspector.test.ts | 40 +++++++++++++++++++++++++ 2 files changed, 61 insertions(+), 4 deletions(-) create mode 100644 packages/ai/test/http-inspector.test.ts diff --git a/packages/ai/src/utils/http-inspector.ts b/packages/ai/src/utils/http-inspector.ts index 2fbe3cd7a..91f400476 100644 --- a/packages/ai/src/utils/http-inspector.ts +++ b/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,6 +23,23 @@ 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 }, + }; +} + export async function appendRawHttpRequestDumpFor400( message: string, error: unknown, @@ -33,12 +50,12 @@ export async function appendRawHttpRequestDumpFor400( 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); diff --git a/packages/ai/test/http-inspector.test.ts b/packages/ai/test/http-inspector.test.ts new file mode 100644 index 000000000..a1b28704c --- /dev/null +++ b/packages/ai/test/http-inspector.test.ts @@ -0,0 +1,40 @@ +import { describe, expect, it } from "bun:test"; +import { buildHttp400DumpPayload, type RawHttpRequestDump } 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"); + }); +}); From c0a8bff0e5ec5b574a014637d1350f5be9478f1a Mon Sep 17 00:00:00 2001 From: Matt Wilkinson Date: Sun, 28 Jun 2026 13:36:29 -0400 Subject: [PATCH 2/2] fix(ai): capture oversized-payload (413) rejections in request dumps MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A snapcompact archive that grows past the provider request-size limit gets rejected with 413 Payload Too Large and silently empties the turn, but the dump path only persisted status 400 — so the wedge left zero on-disk evidence. Persist 413 alongside 400 (both non-retryable request rejections); 5xx/429 stay out since they are retried and would spam one dump per attempt. --- packages/ai/src/utils/http-inspector.ts | 13 ++++++++++++- packages/ai/test/http-inspector.test.ts | 23 ++++++++++++++++++++++- 2 files changed, 34 insertions(+), 2 deletions(-) diff --git a/packages/ai/src/utils/http-inspector.ts b/packages/ai/src/utils/http-inspector.ts index 91f400476..be3e2aef0 100644 --- a/packages/ai/src/utils/http-inspector.ts +++ b/packages/ai/src/utils/http-inspector.ts @@ -40,13 +40,24 @@ export function buildHttp400DumpPayload( }; } +/** 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 { // 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; } diff --git a/packages/ai/test/http-inspector.test.ts b/packages/ai/test/http-inspector.test.ts index a1b28704c..6dbb8c09c 100644 --- a/packages/ai/test/http-inspector.test.ts +++ b/packages/ai/test/http-inspector.test.ts @@ -1,5 +1,9 @@ import { describe, expect, it } from "bun:test"; -import { buildHttp400DumpPayload, type RawHttpRequestDump } from "@oh-my-pi/pi-ai/utils/http-inspector"; +import { + buildHttp400DumpPayload, + type RawHttpRequestDump, + shouldDumpRejectedRequest, +} from "@oh-my-pi/pi-ai/utils/http-inspector"; class HttpError extends Error { constructor( @@ -38,3 +42,20 @@ describe("buildHttp400DumpPayload", () => { 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); + }); +});