fix(coach): never 500 on a provider failure; fix Codex tool-call wire

A Codex backend 400 on the richer Coach tool-call request (and the daily
briefing) propagated out of the provider chain runner unwrapped: a 4xx other
than 401/403/429 was not classified as a hard provider failure, so it bypassed
AllProvidersFailedError and rethrew from the chat route as an HTTP 500. Plain
status-card completions, which never send a tools array, were unaffected.

Harden the failure path so a provider error can never surface as a 500:
- Treat any 4xx a provider wire returns as a hard failure so it cascades and is
  wrapped in AllProvidersFailedError. InsightSchemaError (the strict-JSON 422
  surface) stays the only non-hard class, so that diagnostic is preserved.
- The chat route now also classifies a tagged provider error that reaches its
  catch unwrapped and emits the graceful coach.provider.* SSE frame instead of
  rethrowing. Only an untagged error keeps the 500 + error-reporter path.

Make the Codex 400 diagnosable: fold the redacted upstream response body into
the thrown error message and the chain wide-event, so the field the backend
rejects is visible in logs (it was previously dropped).

Fix the Codex request shape behind the tool-call 400:
- Send the required strict flag on each Responses-API function tool.
- Replay prior assistant turns as output_text, not input_text, per the
  Responses API multi-turn contract — the violation only surfaced on round two
  of the tool loop, which is the path that broke.

Codex-only; the OpenAI, Anthropic, local and mock providers are untouched.
This commit is contained in:
Marc-André Bombeck committed 2026-06-27 10:26:10 +02:00
1 parent ee704cacab
commit 8b7648bdd0
6 files changed
+321 -30

No files matched your search

@@ -261,6 +261,34 @@ describe("coach chat — tool-mode routing (F1)", () => {
);
});
it("surfaces a graceful provider-error stream (NOT an HTTP 500) when the tool loop throws a tagged provider error", async () => {
// v1.21.3 — RCA for the live incident. A Codex 400 raised inside the tool
// loop used to bubble out un-wrapped and rethrow as an HTTP 500. The route
// must now classify any tagged provider error (`upstream` + `httpStatus`)
// and return the graceful 200 SSE error frame the drawer decodes.
resolveProviderChain.mockResolvedValue([
{ providerType: "codex", instance: {} },
]);
(runCoachToolLoop as ReturnType<typeof vi.fn>).mockRejectedValueOnce(
Object.assign(new Error("Codex request failed (400): {…}"), {
httpStatus: 400,
upstream: "codex",
}),
);
const res = await post(chatReq({ message: "How is my BP?" }));
// No throw → no 500. The provider-error frame streams over a 200.
expect(res.status).toBe(200);
// The full reservation is refunded (no tokens billed on a failed turn).
expect(reconcileSpend).toHaveBeenCalledWith(
"u1",
3000,
0,
expect.anything(),
);
});
it("persists the tool trace onto provenance", async () => {
resolveProviderChain.mockResolvedValue([
{ providerType: "anthropic", instance: {} },
+54
View File
@@ -672,6 +672,30 @@ Reply now as the assistant, in ${locale === "de" ? "German" : "English"}. Fetch
: "coach.provider.unavailable",
});
}
// v1.21.3 — defence in depth. The chain runner wraps every hard provider
// failure in `AllProvidersFailedError`, but a provider client can still
// throw a tagged wire error that reaches here un-wrapped (e.g. a Codex 400
// raised mid tool-loop on a path the chain runner did not catch). Such an
// error is a PROVIDER failure, not a server bug — surface the same graceful
// `coach.provider.*` frame the chain path uses rather than rethrowing into
// an HTTP 500 (the bug that took the live Coach down for codex users). Only
// a genuinely unexpected error (no upstream tag, no httpStatus) keeps the
// 500 + GlitchTip path so real defects stay visible.
const providerError = classifyBubblingProviderError(err);
if (providerError) {
annotate({
action: { name: "insights.coach.providerFailed" },
meta: {
attempts: 1,
firstStatus: providerError.httpStatus,
credentialExpired: providerError.code === "credential_expired",
unwrapped: true,
},
});
return streamProviderError({
code: `coach.provider.${providerError.code}`,
});
}
throw err;
}
@@ -1073,6 +1097,36 @@ async function streamRefusal(args: {
return new Response(stream, { status: 200, headers: SSE_HEADERS });
}
/**
* v1.21.3 — classify a provider error that bubbled out of the chain runner
* un-wrapped (i.e. not an `AllProvidersFailedError`). The provider clients tag
* their thrown errors with `upstream` + `httpStatus`; a tagged error is a
* provider failure that must surface a graceful `coach.provider.*` frame rather
* than rethrow into an HTTP 500. Returns `null` for anything that is NOT a
* recognisable provider error (a real server bug), so those keep the 500 +
* GlitchTip path. Status mapping mirrors `AllProvidersFailedError`: 401/403 →
* credential_expired, 429 → rate_limited, everything else → unavailable.
*/
function classifyBubblingProviderError(
err: unknown,
): {
code: "credential_expired" | "rate_limited" | "unavailable";
httpStatus: number | null;
} | null {
if (err === null || typeof err !== "object") return null;
const e = err as { upstream?: unknown; httpStatus?: unknown };
const hasUpstreamTag = typeof e.upstream === "string";
const status = typeof e.httpStatus === "number" ? e.httpStatus : null;
// Require the wire tag — a bare `{ httpStatus }` from unrelated code must not
// be swallowed as a provider outage.
if (!hasUpstreamTag) return null;
if (status === 401 || status === 403) {
return { code: "credential_expired", httpStatus: status };
}
if (status === 429) return { code: "rate_limited", httpStatus: status };
return { code: "unavailable", httpStatus: status };
}
function streamProviderError(args: { code: string }): Response {
const stream = createSseStream((controller) => {
controller.enqueue(