fix(combos): let a local input-admission refusal actually hop the fallback chain

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.
+

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",

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") ||

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");

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;