Skip to content

Commit 5e4bf38

Browse files
committed
fix(server): record recovery kind and de-duplicate Responses param shaping
CodeRabbit follow-up on the Copilot Responses-wire work. The new passthrough recovery loop called noteAttemptSend without the third `recovery` argument, so a request that silently self-healed via OAuth refresh or key rotation showed an inflated sendCount with no recorded reason — exactly the case this path exists to make debuggable. Thread the existing AttemptRecoveryKind values ("oauth-401" / "key-429") through, matching what rebuildAndRefetch does in the normal adapter loop. Both translate-and-replay inbound handlers also carried identical copies of the Responses sampling-param block, and this PR had to patch the two in lockstep — the drift risk in concrete form. Extract `stripUnsupportedResponsesSamplingParams`. `store` stays inline: only the Chat Completions handler has a non-Responses default for it. Full suite: 6069 pass, 6 fail — the same six that fail on the unmodified base.
1 parent 27a2b4e commit 5e4bf38

4 files changed

Lines changed: 42 additions & 28 deletions

File tree

src/server/chat-completions.ts

Lines changed: 2 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ import { resolveClientRetryAfter } from "../lib/retry-after";
2020
import { estimateTokens } from "../lib/token-estimate";
2121
import { routeModel } from "../router";
2222
import { resolveWireProtocolOverride } from "./adapter-resolve";
23+
import { stripUnsupportedResponsesSamplingParams } from "./responses-wire-params";
2324
import type { OcxConfig } from "../types";
2425
import { readJsonRequestBody } from "./request-decompress";
2526
import {
@@ -88,19 +89,7 @@ export async function handleChatCompletions(
8889
directRoute = route.codexAccountMode === "direct";
8990
// ChatGPT backend rejects store:true and unsupported sampling knobs.
9091
internalBody.store = false;
91-
// Forward mode is the only route that 400s on max_output_tokens, and the adapter's
92-
// own stripUnsupportedForwardParams already drops it there on the same condition.
93-
// A routed Responses gateway (Copilot, api.openai.com, ...) honors the field, and
94-
// dropping it there silently ignores the caller's max_tokens.
95-
if (route.provider.authMode === "forward") delete internalBody.max_output_tokens;
96-
// temperature/top_p/stop/user stay stripped on every Responses route: `stop` is not
97-
// a Responses parameter at all, reasoning models reject temperature/top_p, and the
98-
// passthrough adapter relays the body verbatim — `noTemperatureModels` and friends
99-
// are honored only by the openai-chat adapter, so there is no per-model filter here.
100-
delete internalBody.temperature;
101-
delete internalBody.top_p;
102-
delete internalBody.stop;
103-
delete internalBody.user;
92+
stripUnsupportedResponsesSamplingParams(internalBody, route.provider);
10493
} else if (internalBody.store === undefined) {
10594
internalBody.store = false;
10695
}

src/server/claude-messages.ts

Lines changed: 2 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ import { clearableDeadline, idleDeadline } from "../lib/abort";
2727
import { estimateTokens } from "../lib/token-estimate";
2828
import { routeModel } from "../router";
2929
import { resolveWireProtocolOverride } from "./adapter-resolve";
30+
import { stripUnsupportedResponsesSamplingParams } from "./responses-wire-params";
3031
import type { OcxConfig } from "../types";
3132
import { readJsonRequestBody } from "./request-decompress";
3233
import { addFinalRequestLog, httpStatusForTerminalStatus, recordFirstOutput, type RequestLogContext, type RequestLogEntry } from "./request-log";
@@ -599,20 +600,7 @@ export async function handleClaudeMessages(
599600
route.provider = resolveWireProtocolOverride(route.providerName, route.modelId, route.provider);
600601
if (route.provider.adapter === "openai-responses") {
601602
nativeRoute = true;
602-
// Forward mode is the only route that 400s on max_output_tokens ("Unsupported
603-
// parameter", verified live 2026-07-11), and the adapter's own
604-
// stripUnsupportedForwardParams already drops it there on the same condition. A routed
605-
// Responses gateway (Copilot, api.openai.com, ...) honors the field, and dropping it
606-
// there silently ignores the caller's max_tokens.
607-
if (route.provider.authMode === "forward") delete internalBody.max_output_tokens;
608-
// temperature/top_p/stop/user stay stripped on every Responses route: `stop` is not
609-
// a Responses parameter at all, reasoning models reject temperature/top_p, and the
610-
// passthrough adapter relays the body verbatim — `noTemperatureModels` and friends
611-
// are honored only by the openai-chat adapter, so there is no per-model filter here.
612-
delete internalBody.temperature;
613-
delete internalBody.top_p;
614-
delete internalBody.stop;
615-
delete internalBody.user;
603+
stripUnsupportedResponsesSamplingParams(internalBody, route.provider);
616604
}
617605
// Estimated-usage adapters (cursor/kiro) report no per-turn input tokens; stash a
618606
// request-side estimate so the log's in:0 rows get a floor. NEVER set this for
Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
import type { OcxProviderConfig } from "../types";
2+
3+
/**
4+
* Shape a translated Responses body for the wire the route actually uses.
5+
*
6+
* Shared by both translate-and-replay inbound handlers (Chat Completions and Anthropic
7+
* Messages). They previously carried identical copies of this block and had to be patched
8+
* in lockstep when the Copilot routed-gateway case appeared, which is exactly how the two
9+
* drift apart.
10+
*
11+
* `store` is deliberately not handled here — only the Chat Completions handler has a
12+
* non-Responses default for it.
13+
*/
14+
export function stripUnsupportedResponsesSamplingParams(
15+
body: Record<string, unknown>,
16+
provider: Pick<OcxProviderConfig, "authMode">,
17+
): void {
18+
// Forward mode is the only route that 400s on max_output_tokens ("Unsupported parameter",
19+
// verified live 2026-07-11), and the adapter's own stripUnsupportedForwardParams already
20+
// drops it there on the same condition. A routed Responses gateway (Copilot,
21+
// api.openai.com, ...) honors the field, and dropping it there silently ignores the
22+
// caller's max_tokens.
23+
if (provider.authMode === "forward") delete body.max_output_tokens;
24+
// temperature/top_p/stop/user stay stripped on every Responses route: `stop` is not a
25+
// Responses parameter at all, reasoning models reject temperature/top_p, and the
26+
// passthrough adapter relays the body verbatim — `noTemperatureModels` and friends are
27+
// honored only by the openai-chat adapter, so there is no per-model filter here.
28+
delete body.temperature;
29+
delete body.top_p;
30+
delete body.stop;
31+
delete body.user;
32+
}

src/server/responses/core.ts

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1524,8 +1524,12 @@ export async function handleResponses(
15241524
// Bounded: at most one token refresh, then one replay per remaining pool key.
15251525
for (;;) {
15261526
let nextProvider: OcxProviderConfig | undefined;
1527+
// Recorded on the replay's send so the request log explains WHY the send count grew,
1528+
// exactly as the normal loop's rebuildAndRefetch does.
1529+
let recoveryKind: AttemptRecoveryKind | undefined;
15271530
if (upstreamResponse.status === 401 && isOAuth401ReplayProvider && sentOAuthSnapshot && !oauth401Replayed) {
15281531
oauth401Replayed = true;
1532+
recoveryKind = "oauth-401";
15291533
// Release the rejected response's socket before the refresh round-trip, not just
15301534
// before the replay — a failed refresh returns without reaching the shared cancel.
15311535
try { void upstreamResponse.body?.cancel().catch(() => {}); } catch { /* already consumed/closed */ }
@@ -1544,6 +1548,7 @@ export async function handleResponses(
15441548
route.providerName === "github-copilot" ? getOAuthCredentialApiBaseUrl(route.providerName) : undefined,
15451549
);
15461550
} else if (upstreamResponse.status === 429 && hasKeyPoolFailover(route.provider)) {
1551+
recoveryKind = "key-429";
15471552
nextProvider = rotateProviderTransportOn429(config, route.providerName, route.provider, {
15481553
retryAfter: upstreamResponse.headers.get("retry-after"),
15491554
now: Date.now(),
@@ -1563,7 +1568,7 @@ export async function handleResponses(
15631568
request = await passthroughAdapter.buildRequest(parsed, { headers: selectedForwardHeaders });
15641569
// Keep the request log's attempt count honest: a replay is a second upstream send,
15651570
// exactly as the transient-5xx retry above records one.
1566-
noteAttemptSend(logCtx.activeAttempt, passthroughEstimate);
1571+
noteAttemptSend(logCtx.activeAttempt, passthroughEstimate, recoveryKind);
15671572
try {
15681573
upstreamResponse = await fetchWithHeaderTimeout(request.url, {
15691574
method: request.method, headers: request.headers, body: request.body,

0 commit comments

Comments
 (0)