diff --git a/devlog/_plan/260816_wave34_closeout/090_1524_capability_preflight.md b/devlog/_plan/260816_wave34_closeout/090_1524_capability_preflight.md index 9502ccc5a5..edd165a885 100644 --- a/devlog/_plan/260816_wave34_closeout/090_1524_capability_preflight.md +++ b/devlog/_plan/260816_wave34_closeout/090_1524_capability_preflight.md @@ -106,3 +106,43 @@ This is an optimization of an already-correct chain, not the reported defect. #1 acceptance behavior ("reject candidates that cannot accept the request before retrying") holds today for both context and modality. + +### Audit round r1 correction (at `bc6019bae`) + +The section above was written at `812e7c40b`, BEFORE the real defect was found, and an adversarial +pre-merge audit showed it was describing a fix that did not work. Recorded here rather than +rewritten, because the mistake is the useful part. + +**What the earlier section got wrong.** It claimed the context-window half of #1524 was closed by +the `input_admission_refused` hop code. It was not: `core.ts` emits that refusal through +`formatErrorResponse`, which runs `classifyError` — and the message necessarily contains +"context window", because that is what it is refusing on. The generic remap rewrote our own code +to `context_length_exceeded`, so the envelope the proxy actually shipped carried no admission +marker at all. Reordering `comboFailureDecision` was necessary but changed nothing on its own. + +The reason this survived earlier review is worth naming: the test fixture hand-built an envelope +the proxy does not emit. A green test proved a shape that never reaches production. + +**What now holds, verified by running the production emitter rather than reading it:** + +- `src/lib/errors.ts:152` — `classifyError` returns `input_admission_refused` when that is the + type it was given, placed before the context-window remap. +- `src/combos/failover.ts:141` — the hop rule, matched on the structured code only and tested + before the generic stop list at `:144`. +- `src/server/responses/core.ts:2003` — the emitter. +- `tests/routing-policy-fallback.test.ts` — the fixture calls `formatErrorResponse` itself, plus a + negative case pinning that an upstream merely mentioning the token does not hop. + +Both factors are independently load-bearing: disabling the `classifyError` branch fails the +regression, and restoring the original ordering fails it too. Neither alone is sufficient, which +is exactly why the first single-factor ablation was misleading — it passed because the removed +`message.includes` arm caught the case. + +**Provenance caveat, from the reviewer.** Calling this a "local" code slightly overstates it. +Policy fallback reads `error.code` from any response (`src/server/responses/policy-fallback.ts:61`) +and combo reads the upstream nested code, so an upstream that deliberately emits +`input_admission_refused` induces a hop. That is bounded rather than dangerous: upstreams already +control other hop signals (429, 5xx), and traversal is finite — policy tries each candidate once +via `tried`, combo excludes each attempted target. The accurate description is +**structured-code-only**, not provably local, and the code comment says so. + diff --git a/src/combos/failover.ts b/src/combos/failover.ts index 3944eb2c6f..dc3578d0a3 100644 --- a/src/combos/failover.ts +++ b/src/combos/failover.ts @@ -121,19 +121,29 @@ export function comboFailureDecision( if (isCyberPolicyCode(options?.code)) return "stop"; const error = classifyError(status, "upstream_error", message); if (isCyberPolicyCode(error.code)) return "stop"; - if (["origin_rejected", "context_length_exceeded", "invalid_request_error"].includes(error.code ?? "")) { - return "stop"; - } // A local input-admission refusal (#1524) says "this candidate cannot fit the request", - // not "the request is impossible". Hopping is exactly right: the next candidate may have a - // larger context window. Read it from the structured code OR the body text, because the - // generic classifier maps 413 to its own code and would otherwise swallow this signal. - // Upstream `context_length_exceeded` above still stops. - if (options?.code === "input_admission_refused" - || error.code === "input_admission_refused" - || message.includes("input_admission_refused")) { + // not "the request is impossible": the next candidate may have a larger context window. + // + // This MUST be tested before the generic stop list below. Our own refusal message says + // "context window" -- that is what it refuses on -- and the classifier remaps that phrase, + // so checking the stop list first swallowed the signal and ended the chain. An UPSTREAM + // `context_length_exceeded` carries no admission code and still falls through to stop. + // + // Matched on the STRUCTURED code only, which classifyError now preserves for our own + // refusal. A raw substring test would additionally let any upstream override a terminal + // verdict by echoing the token in prose we do not control. + // + // Precise about what this is NOT: an upstream can still SET this code deliberately, since + // both extractors read the upstream error object. That is bounded rather than dangerous -- + // an upstream already controls other hop signals (429, 5xx), and traversal is finite: policy + // tries each candidate once via `tried`, and combo excludes each attempted target. So this is + // structured-code-only, not provably local. + if (options?.code === "input_admission_refused" || error.code === "input_admission_refused") { return "hop"; } + if (["origin_rejected", "context_length_exceeded", "invalid_request_error"].includes(error.code ?? "")) { + return "stop"; + } if ([401, 403, 404, 408, 429].includes(status) || status >= 500) return "hop"; if ([ "permission_denied", diff --git a/src/lib/errors.ts b/src/lib/errors.ts index 316002b582..c5523712dc 100644 --- a/src/lib/errors.ts +++ b/src/lib/errors.ts @@ -143,6 +143,15 @@ export function classifyError(status: number, type: string, message: string): Oc if (type === CYBER_POLICY_ERROR_CODE || isCyberPolicyMessage(text)) { return { message, type: "invalid_request_error", code: CYBER_POLICY_ERROR_CODE }; } + // A LOCAL preflight refusal keeps its own code (#1524). The message necessarily says + // "context window" -- that is what it is refusing on -- so the generic remap below would + // rewrite it to `context_length_exceeded` and make it indistinguishable from an UPSTREAM + // verdict. The two need opposite fallback handling: ours means "this candidate does not + // fit", theirs means "the request is impossible", so collapsing them ended the chain at + // the first candidate that was merely too small. + if (type === "input_admission_refused") { + return { message, type: "invalid_request_error", code: "input_admission_refused" }; + } if ( text.includes("context_length_exceeded") || text.includes("context window") || diff --git a/tests/combos.test.ts b/tests/combos.test.ts index 899118e961..70bf3a657b 100644 --- a/tests/combos.test.ts +++ b/tests/combos.test.ts @@ -360,7 +360,12 @@ describe("combo failure policy and advancement", () => { // #1524: a LOCAL input-admission refusal means "this candidate cannot fit the request", // not "the request is impossible". The next candidate may have a larger context window, // so the chain must continue instead of ending at the first incompatible target. - expect(comboFailureDecision(413, '{"code":"input_admission_refused"}')).toBe("hop"); + // + // The decision keys on the STRUCTURED code, which the proxy now preserves through + // classifyError. Matching raw text instead would let any upstream override a terminal + // verdict by echoing the token, so that shape must NOT hop. + expect(comboFailureDecision(413, 'refused', { code: 'input_admission_refused' })).toBe('hop'); + expect(comboFailureDecision(400, 'upstream mentions input_admission_refused in prose')).toBe('stop'); // An UPSTREAM context verdict still stops: retrying that elsewhere is guesswork, and a // generic 413 with no structured code keeps its existing conservative handling. expect(comboFailureDecision(400, "context_length_exceeded")).toBe("stop"); diff --git a/tests/routing-policy-fallback.test.ts b/tests/routing-policy-fallback.test.ts index a81d1ddd79..ac2fc34f28 100644 --- a/tests/routing-policy-fallback.test.ts +++ b/tests/routing-policy-fallback.test.ts @@ -1,5 +1,6 @@ import { describe, expect, test } from "bun:test"; +import { formatErrorResponse } from "../src/bridge"; import { RequestPacingQueueOverloadError } from "../src/providers/request-pacing"; import type { OcxConfig } from "../src/types"; import { beginRequestAttempt, type RequestLogContext } from "../src/server/request-log"; @@ -54,6 +55,84 @@ describe("policy candidate fallback", () => { ]); }); + test("a local input-admission refusal hops instead of ending the chain (#1524)", async () => { + // #1524: a candidate whose context window cannot fit the request used to TERMINATE the + // fallback chain. It is a local preflight verdict about ONE candidate, not about the + // request, so the next candidate -- which may have a larger window -- must still be tried. + const trace = policyTrace(); + const logCtx = { requestedModel: "policy/daily", routeDecision: trace, attempts: [] } as unknown as RequestLogContext; + const seenModels: string[] = []; + const runCore = async (req: Request, _config: OcxConfig, ctx: RequestLogContext) => { + const body = await req.clone().json() as { model?: string }; + seenModels.push(String(body.model)); + ctx.routeDecision = trace; + seedAttempt(ctx, "provider", String(body.model)); + if (seenModels.length === 1) { + // Built by the PRODUCTION emitter, not by hand. A hand-written envelope hid the real + // defect: formatErrorResponse runs classifyError, whose "context window" remap rewrote + // our own code to context_length_exceeded, so the shape the proxy actually ships never + // carried the marker the hop rule looks for. + return formatErrorResponse( + 413, + "input_admission_refused", + "Estimated input (~500000 tokens) is far past the context window of test-model (100000 tokens)." + + " Start a new session or choose a model with a larger context window.", + ); + } + return Response.json({ id: "resp", object: "response", status: "completed", output: [] }); + }; + + const response = await handleResponsesWithPolicyFallback(request(), {} as OcxConfig, logCtx, {}, { runCore }); + + expect(response.status).toBe(200); + expect(seenModels).toEqual(["policy/daily", "provider-b/model-b"]); + }); + + test("an upstream body that merely echoes the marker does not hop (#1524)", async () => { + // The refusal is ours and always carries the structured code, so the decision keys on that + // alone. Error text is provider-controlled and crosses a trust boundary: matching on it + // would let any upstream override a terminal verdict by mentioning the token. + const trace = policyTrace(); + const logCtx = { requestedModel: "policy/daily", routeDecision: trace, attempts: [] } as unknown as RequestLogContext; + const seenModels: string[] = []; + const runCore = async (req: Request, _config: OcxConfig, ctx: RequestLogContext) => { + const body = await req.clone().json() as { model?: string }; + seenModels.push(String(body.model)); + ctx.routeDecision = trace; + seedAttempt(ctx, "provider", String(body.model)); + return Response.json( + { error: { message: "upstream says: input_admission_refused is not a thing here", type: "invalid_request_error", code: "invalid_request_error" } }, + { status: 400 }, + ); + }; + + const response = await handleResponsesWithPolicyFallback(request(), {} as OcxConfig, logCtx, {}, { runCore }); + + expect(response.status).toBe(400); + expect(seenModels).toEqual(["policy/daily"]); + }); + test("an upstream context_length_exceeded still stops the chain (#1524)", async () => { + // The mirror-image contract. An upstream verdict is about the REQUEST, so retrying it + // elsewhere is guesswork -- and hopping would burn every candidate on a doomed request. + const trace = policyTrace(); + const logCtx = { requestedModel: "policy/daily", routeDecision: trace, attempts: [] } as unknown as RequestLogContext; + const seenModels: string[] = []; + const runCore = async (req: Request, _config: OcxConfig, ctx: RequestLogContext) => { + const body = await req.clone().json() as { model?: string }; + seenModels.push(String(body.model)); + ctx.routeDecision = trace; + seedAttempt(ctx, "provider", String(body.model)); + return Response.json( + { error: { message: "context length exceeded", type: "invalid_request_error", code: "context_length_exceeded" } }, + { status: 400 }, + ); + }; + + const response = await handleResponsesWithPolicyFallback(request(), {} as OcxConfig, logCtx, {}, { runCore }); + + expect(response.status).toBe(400); + expect(seenModels).toEqual(["policy/daily"]); + }); test("retries the next policy candidate and keeps distinct physical attempts", async () => { const trace = policyTrace(); const logCtx = { requestedModel: "policy/daily", routeDecision: trace, attempts: [] } as unknown as RequestLogContext;