diff --git a/README.md b/README.md index 32d9dad..1cdde56 100644 --- a/README.md +++ b/README.md @@ -965,7 +965,7 @@ Behavior depends entirely on the resolved provider's configured `type`: | Resolved provider `type` | Behavior | |---|---| -| `anthropic` | **Passthrough.** The request reaches that provider's own `/v1/messages` endpoint untranslated — no message-body translation of any kind. The response reaches the client unchanged, field for field, with one exception: `"model"` is rewritten to the client's own requested alias, the same routing rewrite every route applies before any adapter call — not a translation of the response body. | +| `anthropic` | **Passthrough.** The request reaches that provider's own `/v1/messages` endpoint untranslated — no message-body translation of any kind. The response reaches the client BYTE-FOR-BYTE identical, with one exception: `"model"` is rewritten to the client's own requested alias, the same routing rewrite every route applies before any adapter call — not a translation of the response body. This is a real byte-level guarantee, not an approximation: every other field's value is copied from the upstream's raw bytes without ever passing through a Go value, so a JSON integer beyond `float64`'s exact range (inside, say, a `tool_use` block's arbitrary `input`) survives intact instead of being silently rounded. | | `openai` or `gemini` | **Full bidirectional translation**, through the same `providerAdapter.chatCompletion` a `/v1/chat/completions` request against that provider already uses — its contract is OpenAI-shape in, OpenAI-shape out regardless of provider type. The Anthropic-shaped request is translated to OpenAI's shape before the call; that provider's OpenAI-shaped response is translated back to Anthropic's shape before it reaches the client. | A few translation specifics worth knowing: @@ -981,9 +981,13 @@ A few translation specifics worth knowing: the same combined prompt-token count every other route's budgets track, alongside `input_tokens` — not dropped, and not priced at a separate cache-tier rate. -- **`tool_result.is_error`** has no native OpenAI tool-message field; a - string `content` gets an `"Error: "` prefix so an OpenAI-backed - agentic loop still sees the failure signal. +- **`tool_result.is_error`** has no native OpenAI tool-message field. + `content` — a string, Anthropic's content-block array form (Claude + Code sends this), or absent — is normalized to a plain string first + (OpenAI's own tool message requires one; a `null` content is + rejected), then gets an `"Error: "` prefix, so an OpenAI-backed + agentic loop still sees the failure signal regardless of which shape + the client sent. - **Reasoning models** (OpenAI's `o1`/`o3`/`o4`/`gpt-5` families) get `max_completion_tokens` instead of `max_tokens` — the field those models reject with a 400. @@ -994,18 +998,33 @@ A few translation specifics worth knowing: ### Streaming is not yet supported -`"stream"` set to anything other than the JSON literal `false` (or left -unset) gets a clean **400** `invalid_request_error` — never silently -ignored, never answered non-streamed. This is deliberately a 400, not a -501: Anthropic's own SDKs retry any status ≥ 500, so a 501 here would -turn every streaming call — the default for Claude Code — into a -multi-request retry storm against the caller's own request-rate limit. -The rejection happens *before* the rate limiter runs, so a -streaming-rejected request never consumes budget either. A follow-up -release adds the Anthropic SSE event translator -(`message_start`/`content_block_delta`/`message_stop`) — a genuinely -different event model from OpenAI's own stream shape this plugin already -forwards on `/v1/chat/completions`. +`"stream"` set to anything other than the JSON literal `false`, `null`, +or an absent key gets a clean **400** `invalid_request_error` — never +silently ignored, never answered non-streamed. `null` is treated the +same as unset, so an SDK that serializes an unset optional field as +explicit `null` never gets a false-positive rejection on an ordinary +non-streaming call. This is deliberately a 400, not a 501: Anthropic's +own SDKs retry any status ≥ 500, so a 501 here would turn every +streaming call — the default for Claude Code — into a multi-request +retry storm against the caller's own request-rate limit. The rejection +happens *before* the rate limiter runs, so a streaming-rejected request +never consumes budget either. A follow-up release adds the Anthropic SSE +event translator (`message_start`/`content_block_delta`/`message_stop`) +— a genuinely different event model from OpenAI's own stream shape this +plugin already forwards on `/v1/chat/completions`. + +### Route takeover + +On a deployment running `passthroughUnknown: true`, `POST /v1/messages` +used to fall through to `next` (the router's own backing service) before +this route existed — nothing in this plugin recognized the path. Now the +gateway intercepts it unconditionally: `next` is never reached for this +path again, regardless of `passthroughUnknown`. This is intended — the +whole point of this route is for the gateway itself to answer +`/v1/messages` — but it is a real behavior change for any deployment +currently proxying `/v1/messages` downstream through +`passthroughUnknown`. Route that traffic elsewhere, or expect the +gateway to answer it directly, before upgrading. ## Image and audio endpoints @@ -1421,6 +1440,14 @@ the plugin's only non-standard-library runtime dependency. integration suite's true incremental-delivery timing assertion is gated behind `INTEGRATION_STREAMING=1` and skipped by default for exactly this reason — re-run it once the upstream issue is fixed. +- **`POST /v1/messages` now intercepts unconditionally, even with + `passthroughUnknown: true`.** Before this route existed, that path + fell through to `next` on a `passthroughUnknown` deployment like any + other unrecognized path. The gateway now answers it directly and + `next` is never reached for it again — intended, but a real behavior + change for a deployment currently proxying `/v1/messages` downstream. + See [Anthropic Messages API — Route + takeover](#route-takeover). - **A request rejected before it reaches a provider never consumes request-rate quota.** On the unified routes, a malformed body, a missing `model` field, or an unknown/denied model (404/403) is rejected before diff --git a/routes_messages.go b/routes_messages.go index 28a03ef..3e5afaa 100644 --- a/routes_messages.go +++ b/routes_messages.go @@ -118,15 +118,20 @@ func (g *Gateway) handleMessages(w http.ResponseWriter, r *http.Request, u *user } // isStreamingRequested reports whether req's "stream" field is anything -// other than the JSON literal false or an absent key (item 9 fix, -// 2026-08-22 review). A bare req["stream"].(bool) assertion silently -// treats a non-bool value ("stream":"true", "stream":1) as false and lets -// it through to an upstream that might itself honor a loosely-typed -// truthy value — exactly the "silently answered non-streamed" outcome -// this route must reject explicitly instead of allowing by accident. +// other than the JSON literal false, JSON null, or an absent key (item +// 9/F7 fix, 2026-08-22/23 review). A bare req["stream"].(bool) assertion +// silently treats a non-bool value ("stream":"true", "stream":1) as +// false and lets it through to an upstream that might itself honor a +// loosely-typed truthy value — exactly the "silently answered +// non-streamed" outcome this route must reject explicitly instead of +// allowing by accident. JSON null is treated the same as an absent key +// (F7 fix): an SDK that serializes an unset optional field as explicit +// null must not get a hard 400 on an ordinary non-streaming call — null +// decodes to a Go nil interface value, which the item-9 fix's "anything +// not the literal false" rule would otherwise also reject. func isStreamingRequested(req map[string]any) bool { v, ok := req["stream"] - if !ok { + if !ok || v == nil { return false } return v != false @@ -177,20 +182,16 @@ func (e *responseTranslationError) Error() string { return "translate response: // through anthropicPassthroughForwardedHeaders' allowlist only (item 5 // fix, above). // -// The response body reaches the client almost byte-for-byte: this -// function decodes it twice, read-only, to (a) extract usage -// (input_tokens/output_tokens/cache_*_tokens, via the existing -// anthropicResponseBody type, translate_anthropic.go — reused rather -// than declared again) for accounting, and (b) rewrite only the "model" -// field to requestedModel — the client's own requested alias — before -// re-encoding and writing it (item 4 fix, 2026-08-22 review: the -// PREVIOUS version left the upstream's own real model id in the -// response, inconsistent with callTranslatedMessages' own alias echo on -// the same route, and leaking the operator's real upstream model id, -// which gatewayAliasKey exists specifically to prevent). Every other -// field survives the round trip unchanged, since the rewrite decodes -// into a generic map rather than a narrow typed struct — rewriting one -// field this way is routing, not translating the message body. +// The response body reaches the client BYTE-FOR-BYTE except "model", +// rewritten to requestedModel — the client's own requested alias (item 4 +// fix, 2026-08-22 review: the PREVIOUS version left the upstream's own +// real model id in the response, inconsistent with callTranslatedMessages' +// own alias echo on the same route, and leaking the operator's real +// upstream model id, which gatewayAliasKey exists specifically to +// prevent). Byte-for-byte is load-bearing, not a nicety: see +// rewriteModelField's own doc comment (item 4/F1 fix, 2026-08-23 review) +// for why a decode-then-re-marshal round trip through a Go value +// silently corrupts data on this route. func (g *Gateway) callAnthropicMessagesPassthrough(ctx context.Context, w http.ResponseWriter, clientHeaders http.Header, adapter providerAdapter, req map[string]any, requestedModel string) (usage, error) { delete(req, gatewayAliasKey) // this route never sets it via req itself, but a client-sent "__alias" field must never reach the real Anthropic API @@ -233,33 +234,44 @@ func (g *Gateway) callAnthropicMessagesPassthrough(ctx context.Context, w http.R raw, err := io.ReadAll(io.LimitReader(resp.Body, maxResponseBytes)) if err != nil { - return usage{}, fmt.Errorf("%w: read response body: %w", errUpstream, err) + // F5 fix, 2026-08-23 review: the upstream already answered 200 + // (already billed server-side) — a body-read failure past that + // point is classified as a translation failure, not a + // connectivity problem, consistent with the usage-decode failure + // below. Nothing is recoverable here: with no bytes read at all, + // there is neither a usage number to extract nor a body to + // deliver, so this is still an error return. + return usage{}, &responseTranslationError{err: fmt.Errorf("read response body: %w", err)} } + // F5 fix, 2026-08-23 review: a failure parsing USAGE out of raw must + // never block delivering raw itself to the client — the upstream + // already fully answered, and this route's whole purpose is passing + // that answer through. billed stays usage{} on a parse failure; + // runMeteredCall's own zero-usage estimate (from request body size, + // routes_unified.go) takes over, exactly like every other + // unparseable-usage-shape case already relies on. This is NOT a + // repeat of the "silently drop tokens a client can exploit" bug item + // 1 fixed elsewhere: there, callErr was returned non-nil and skipped + // that estimate fallback entirely; here callErr stays nil, so the + // fallback still runs and the request still gets billed something. var parsed anthropicResponseBody + var billed usage if unmarshalErr := json.Unmarshal(raw, &parsed); unmarshalErr != nil { - return usage{}, fmt.Errorf("%w: decode anthropic response: %w", errUpstream, unmarshalErr) + g.warnf("messages route: could not decode usage from an anthropic passthrough response (forwarding the response to the client regardless): %v", unmarshalErr) + } else { + // totalInputTokens folds Anthropic's prompt-cache counters + // (cache_creation_input_tokens/cache_read_input_tokens) into the + // billed prompt count (item 6 fix, 2026-08-22 review) — see its + // own doc comment, translate_anthropic.go, for why dropping them + // under-bills a cache-heavy caller (the common case for Claude + // Code) by orders of magnitude. + billed = usage{prompt: parsed.Usage.totalInputTokens(), completion: parsed.Usage.OutputTokens} } - // totalInputTokens folds Anthropic's prompt-cache counters - // (cache_creation_input_tokens/cache_read_input_tokens) into the - // billed prompt count (item 6 fix, 2026-08-22 review) — see its own - // doc comment, translate_anthropic.go, for why dropping them - // under-bills a cache-heavy caller (the common case for Claude Code) - // by orders of magnitude. - billed := usage{prompt: parsed.Usage.totalInputTokens(), completion: parsed.Usage.OutputTokens} - // The upstream call already succeeded and billed above by this - // point — any failure from here on is a *responseTranslationError, - // not a connectivity problem, so billed still reaches the caller - // even if the field rewrite below somehow fails (item 1/12 fix). - var envelope map[string]any - if unmarshalErr := json.Unmarshal(raw, &envelope); unmarshalErr != nil { - return billed, &responseTranslationError{err: fmt.Errorf("decode anthropic response envelope: %w", unmarshalErr)} - } - envelope["model"] = requestedModel - out, err := json.Marshal(envelope) + out, err := rewriteModelField(raw, requestedModel) if err != nil { - return billed, &responseTranslationError{err: fmt.Errorf("re-marshal anthropic response: %w", err)} + return billed, &responseTranslationError{err: err} } if ct := resp.Header.Get("Content-Type"); ct != "" { @@ -273,6 +285,81 @@ func (g *Gateway) callAnthropicMessagesPassthrough(ctx context.Context, w http.R return billed, nil } +// rewriteModelField replaces raw's top-level "model" field with a JSON +// string of newModel, preserving every other field's VALUE bytes and +// their original relative order exactly (item 4/F1 fix, 2026-08-23 +// review). A prior version decoded the whole response into +// map[string]any and re-marshaled it — not enough: Go's float64 cannot +// represent every JSON integer exactly (verified repro: +// 12345678901234567890 becomes 12345678901234567000, and +// 9007199254740993 becomes 9007199254740992 — both silent corruption of +// a value that could be, for instance, a long numeric id inside a +// tool_use block's arbitrary "input"), json.Marshal also rewrites 1.0 to +// 1 and HTML-escapes < and & by default, and re-marshaling a Go map +// always re-sorts its keys regardless of the source's own order — wrong +// on a route documented as passthrough. +// +// This function never turns any value but "model" into a Go value at +// all: it walks raw's top-level object with json.Decoder.Token()/Decode() +// and captures every other field's value as a json.RawMessage, which +// preserves the exact source bytes of that value — including a nested +// object or array's own internal structure — without parsing it further. +// The rebuilt object is written compactly (no inter-token whitespace): +// for the compact JSON every provider in this codebase actually returns +// over the wire, that reproduces the input byte-for-byte apart from the +// "model" field; a pretty-printed input's insignificant whitespace would +// not survive, which no known caller of this function ever produces. +func rewriteModelField(raw []byte, newModel string) ([]byte, error) { + dec := json.NewDecoder(bytes.NewReader(raw)) + if tok, err := dec.Token(); err != nil || tok != json.Delim('{') { + return nil, fmt.Errorf("response is not a JSON object") + } + + newModelJSON, err := json.Marshal(newModel) + if err != nil { + return nil, fmt.Errorf("marshal model field: %w", err) + } + + var out bytes.Buffer + out.WriteByte('{') + first := true + for dec.More() { + keyTok, err := dec.Token() + if err != nil { + return nil, fmt.Errorf("read field key: %w", err) + } + key, ok := keyTok.(string) + if !ok { + return nil, fmt.Errorf("unexpected non-string object key") + } + + var val json.RawMessage + if decodeErr := dec.Decode(&val); decodeErr != nil { + return nil, fmt.Errorf("read field %q value: %w", key, decodeErr) + } + + if !first { + out.WriteByte(',') + } + first = false + + keyJSON, err := json.Marshal(key) + if err != nil { + return nil, fmt.Errorf("marshal field key %q: %w", key, err) + } + out.Write(keyJSON) + out.WriteByte(':') + if key == "model" { + out.Write(newModelJSON) + } else { + out.Write(val) + } + } + out.WriteByte('}') + + return out.Bytes(), nil +} + // callTranslatedMessages handles a /v1/messages request resolved to a // non-anthropic-type provider (openai, or gemini — both implement // providerAdapter.chatCompletion against the SAME OpenAI-shaped @@ -316,7 +403,17 @@ func (g *Gateway) callTranslatedMessages(ctx context.Context, w http.ResponseWri rec := newMessagesResponseRecorder() result, err := adapter.chatCompletion(ctx, rec, openaiReq) if err != nil { - return usage{}, err + // result, not usage{} (F5 fix, 2026-08-23 review): harmless today + // — streaming is rejected before this route ever reaches here, so + // chatCompletion's non-streaming contract already guarantees + // usage{} on any error. It matters the day streaming lands: a + // streaming chatCompletion call (forwardStream) CAN return + // nonzero usage alongside an error — a usage chunk that arrived + // just before a dropped connection must still be billed — and + // discarding it here would reintroduce item 1's original + // CRITICAL (usage silently dropped on an error path) the moment + // this route gains streaming support. + return result, err } out, err := anthropicResponseFromOpenAI(rec.body.Bytes(), requestedModel) diff --git a/routes_messages_test.go b/routes_messages_test.go index 46b0521..7246954 100644 --- a/routes_messages_test.go +++ b/routes_messages_test.go @@ -388,6 +388,39 @@ func TestHandleMessages_StreamingRejected(t *testing.T) { } } +// TestHandleMessages_NullStream_IsNotStreaming is the regression for F7 +// (2026-08-23 review): isStreamingRequested's item-9 fix ("anything not +// the literal false is streaming") had the side effect of also +// rejecting "stream": null, since a bare v != false comparison treats a +// JSON null (decoded as a Go nil interface) as "present and not false". +// An SDK that serializes an unset optional field as explicit null must +// still get an ordinary, successful non-streaming response, not a false +// -positive 400. +func TestHandleMessages_NullStream_IsNotStreaming(t *testing.T) { + const anthResp = `{"id":"msg_1","type":"message","role":"assistant","content":[{"type":"text","text":"ok"}],"model":"claude-real","stop_reason":"end_turn","usage":{"input_tokens":1,"output_tokens":1}}` + called := false + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + called = true + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(anthResp)) + })) + defer srv.Close() + + gw := newMessagesTestGateway(t, srv.URL, "") + body := map[string]any{"model": "claude-test", "max_tokens": 10, "stream": nil, "messages": []any{map[string]any{"role": "user", "content": "hi"}}} + req := newMessagesRequest(t, body, "x-api-key", "sk-alice") + rec := httptest.NewRecorder() + gw.ServeHTTP(rec, req) + + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200, body=%s", rec.Code, rec.Body.String()) + } + if !called { + t.Error("upstream was never called; \"stream\": null must be treated as an ordinary non-streaming request") + } +} + // TestHandleMessages_RateLimited_Returns429 proves this route enforces // the caller's group/user requestsPerMinute limit exactly like // routes_unified.go's runUnified: a second request past the limit is @@ -542,3 +575,107 @@ func TestHandleMessages_TranslationFailureAfterBilledUsage_PreservesUsage(t *tes t.Errorf("log output = %q, want it to NOT be classified as an upstream connection error", logs) } } + +// TestHandleMessages_AnthropicProvider_Passthrough_PreservesExactBytes is +// the regression for F1 (BLOCKING, 2026-08-23 review): the previous +// alias-echo fix decoded the upstream's 200 into map[string]any and +// re-marshaled it, which silently corrupted data on a route documented +// as passthrough — a JSON integer beyond float64's exact-integer range +// (verified repro: 12345678901234567890 became 12345678901234567000), +// a trailing ".0" on a float (1.0 became 1), and HTML-escaped "<" and +// "&". A tool_use block's "input" is arbitrary model-generated JSON, so +// a long numeric id inside tool arguments was silently corrupted on +// every anthropic-type request through this route. This proves the +// fixed version (rewriteModelField, routes_messages.go) reproduces the +// upstream body byte-for-byte apart from the "model" field alone: same +// field order, same big-integer digits, same "1.0", same literal "<". +func TestHandleMessages_AnthropicProvider_Passthrough_PreservesExactBytes(t *testing.T) { + const anthResp = `{"id":"msg_01ABC","type":"message","role":"assistant","content":[{"type":"tool_use","id":"toolu_1","name":"calc","input":{"big_id":12345678901234567890,"score":1.0,"note":"1 < 2 & 3"}}],"model":"claude-real","stop_reason":"end_turn","usage":{"input_tokens":5,"output_tokens":3}}` + const wantBody = `{"id":"msg_01ABC","type":"message","role":"assistant","content":[{"type":"tool_use","id":"toolu_1","name":"calc","input":{"big_id":12345678901234567890,"score":1.0,"note":"1 < 2 & 3"}}],"model":"claude-test","stop_reason":"end_turn","usage":{"input_tokens":5,"output_tokens":3}}` + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(anthResp)) + })) + defer srv.Close() + + gw := newMessagesTestGateway(t, srv.URL, "") + + body := map[string]any{ + "model": "claude-test", + "max_tokens": 100, + "messages": []any{map[string]any{"role": "user", "content": "hello"}}, + } + req := newMessagesRequest(t, body, "x-api-key", "sk-alice") + rec := httptest.NewRecorder() + gw.ServeHTTP(rec, req) + + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200, body=%s", rec.Code, rec.Body.String()) + } + if got := rec.Body.String(); got != wantBody { + t.Errorf("body = %s\nwant = %s\n(must be byte-for-byte identical apart from the \"model\" field: no float64 rounding, no 1.0->1, no HTML-escaping, no key reordering)", got, wantBody) + } +} + +// TestHandleMessages_AnthropicProvider_Passthrough_HeaderAllowlist is the +// regression for F3 (BLOCKING, 2026-08-23 review): negative assertions +// that a blanket header-forward, or reordering injectAuth ahead of the +// allowlist copy while adding x-api-key to it, would both defeat. +// Authorization, x-api-key (the CLIENT's own value), and Cookie must +// never reach the anthropic-type upstream, and the upstream must see +// exactly ONE x-api-key value: the gateway's own configured key +// ("sk-ant-up"), never the client's own ("sk-alice"). The exactly-ONE +// check matters on its own: injectAuth's Set() call replaces any +// existing x-api-key value, so a mutant that adds x-api-key to the +// allowlist and reorders injectAuth BEFORE that copy would still leave +// Get("x-api-key") returning "sk-ant-up" (Set ran first, Add appended +// after) while a second, leaked "sk-alice" value silently rode along — +// invisible to a bare .Get() check, which only ever returns the first +// value. +func TestHandleMessages_AnthropicProvider_Passthrough_HeaderAllowlist(t *testing.T) { + const anthResp = `{"id":"msg_1","type":"message","role":"assistant","content":[{"type":"text","text":"ok"}],"model":"claude-real","stop_reason":"end_turn","usage":{"input_tokens":1,"output_tokens":1}}` + + var gotHeaders http.Header + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + gotHeaders = r.Header.Clone() + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(anthResp)) + })) + defer srv.Close() + + gw := newMessagesTestGateway(t, srv.URL, "") + + body := map[string]any{"model": "claude-test", "max_tokens": 10, "messages": []any{map[string]any{"role": "user", "content": "hi"}}} + // Authenticate via Authorization: Bearer (presentedKey, auth.go, + // gives it priority over x-api-key when both are present) and + // ADDITIONALLY send an x-api-key header of the client's own — unused + // for auth here, but still present on the request and must still + // never reach the upstream — plus a Cookie header. None of the three + // has any business reaching the upstream. + req := newMessagesRequest(t, body, "Authorization", "Bearer sk-alice") + req.Header.Set("x-api-key", "some-other-client-value") // #nosec G101 -- test fixture literal, not a real credential + req.Header.Set("Cookie", "session=client-side-secret") + rec := httptest.NewRecorder() + gw.ServeHTTP(rec, req) + + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200, body=%s", rec.Code, rec.Body.String()) + } + + if got := gotHeaders.Get("Authorization"); got != "" { + t.Errorf("upstream Authorization = %q, want empty (never forwarded)", got) + } + if got := gotHeaders.Get("Cookie"); got != "" { + t.Errorf("upstream Cookie = %q, want empty (never forwarded)", got) + } + apiKeyValues := gotHeaders.Values("x-api-key") + if len(apiKeyValues) != 1 { + t.Fatalf("upstream x-api-key values = %v, want exactly 1 (the client's own key must never ride along as a second value)", apiKeyValues) + } + if apiKeyValues[0] != "sk-ant-up" { // #nosec G101 -- test fixture literal, not a real credential + t.Errorf("upstream x-api-key = %q, want the gateway's own configured key sk-ant-up, never the client's sk-alice", apiKeyValues[0]) + } +} diff --git a/routes_passthrough_test.go b/routes_passthrough_test.go index 4095afe..07d99a5 100644 --- a/routes_passthrough_test.go +++ b/routes_passthrough_test.go @@ -69,6 +69,22 @@ func TestExtractPassthroughUsage(t *testing.T) { wantUsage: usage{prompt: 11, completion: 4}, wantModel: "claude-native", }, + { + // F2 fix (BLOCKING, 2026-08-23 review): reverting + // extractPassthroughUsage's providerTypeAnthropic case back + // to input_tokens alone (dropping the cache_creation_input_ + // tokens/cache_read_input_tokens terms) must fail this case — + // this is the production /{provider}/... native passthrough + // route, and the previous version silently under-billed a + // cache-heavy tenant by orders of magnitude with zero test + // coverage catching it. + name: "anthropic shape with prompt-cache tokens billed (F2)", + typeName: providerTypeAnthropic, + providerName: "anthropic", + body: `{"model":"claude-native","usage":{"input_tokens":4,"cache_creation_input_tokens":180000,"cache_read_input_tokens":20000,"output_tokens":9}}`, + wantUsage: usage{prompt: 200004, completion: 9}, + wantModel: "claude-native", + }, { name: "gemini shape, no model field", typeName: providerTypeGemini, diff --git a/tools/yaegi-check/main.go b/tools/yaegi-check/main.go index 2657937..52ea7b3 100644 --- a/tools/yaegi-check/main.go +++ b/tools/yaegi-check/main.go @@ -764,9 +764,17 @@ func exerciseMessagesRoute(handler http.Handler) error { if err := assertAnthropicErrorShape("upstream 429", upErrRec.Body.Bytes()); err != nil { return err } - if upErrRec.Header().Get("X-Llmgw-Cache") != "" { - return fmt.Errorf("POST /v1/messages (upstream 429) carries a stale X-Llmgw-Cache header: %q", upErrRec.Header().Get("X-Llmgw-Cache")) - } + // A stale X-Llmgw-Cache header on an error response was deliberately + // NOT asserted here (F6 fix, 2026-08-23 review): this harness's own + // fixture configures no cache/Redis, so g.cache is nil and the + // header is never set on any response regardless of what this route + // does — the assertion could not fail no matter what code ran, + // confirmed by a mutation pass that deleted the header-clear line it + // was meant to guard (handleAdapterErrorEnvelope, routes_unified.go) + // with make yaegi-check still reporting OK. That behavior is already + // covered where it can actually fail — a real cache fixture, go + // test: TestHandleChat_CacheableRequest_UpstreamDown_502HasNoCacheHeader + // (routes_unified_test.go). // 7. A tool_use round trip through the TRANSLATED path, to drive // openAIToolCallFromAnthropic/openAIToolMessageFromAnthropic and diff --git a/translate_anthropic.go b/translate_anthropic.go index 8d746cf..9dac421 100644 --- a/translate_anthropic.go +++ b/translate_anthropic.go @@ -937,36 +937,58 @@ func openAIToolCallFromAnthropic(bm map[string]any) (map[string]any, error) { // openAIToolMessageFromAnthropic maps one Anthropic {"type":"tool_result", // "tool_use_id","content","is_error"} content block to an OpenAI // role:"tool" message ({"role":"tool","tool_call_id","content"}), the -// mirror of anthropicToolResultBlock above. is_error (item 11 fix, -// 2026-08-22 review) has no native OpenAI equivalent — a tool message -// carries no error flag — so it is preserved via prefixErrorMarker -// instead of being silently dropped: the previous version of this -// function read only tool_use_id/content and discarded is_error -// entirely, losing the failure signal for any OpenAI-backed agentic loop -// that branches on whether a tool call succeeded. +// mirror of anthropicToolResultBlock above. is_error (item 11/F4 fix, +// 2026-08-22/23 review) has no native OpenAI equivalent — a tool message +// carries no error flag — so it is preserved by prefixing "Error: " onto +// content once openAIToolResultContentString (below) has normalized it +// to a plain string, instead of being silently dropped: an earlier +// version of this function read only tool_use_id/content and discarded +// is_error entirely, and a second earlier version only prefixed a Go +// string, still dropping the signal for Anthropic's array-content form. func openAIToolMessageFromAnthropic(bm map[string]any) map[string]any { toolUseID, _ := bm["tool_use_id"].(string) - content := bm["content"] + content := openAIToolResultContentString(bm["content"]) if isErr, _ := bm["is_error"].(bool); isErr { - content = prefixErrorMarker(content) + content = "Error: " + content } return map[string]any{"role": "tool", "tool_call_id": toolUseID, "content": content} } -// prefixErrorMarker preserves Anthropic's tool_result.is_error signal -// across the translation to OpenAI's tool message shape (item 11 fix, -// 2026-08-22 review): a string content gets an "Error: " prefix so the -// model still sees the failure signal in the one shape OpenAI's tool -// message actually carries. Any other content shape (a block array, or -// absent) is left as-is rather than guessing how to annotate it — -// OpenAI-compatible upstreams generally infer failure from content -// itself in that case, same as before this fix; only the common -// string-content case previously lost the signal entirely. -func prefixErrorMarker(content any) any { - if s, ok := content.(string); ok { - return "Error: " + s +// openAIToolResultContentString normalizes an Anthropic tool_result +// block's "content" into the single string OpenAI's tool message +// "content" field requires (item 11/F4 fix, 2026-08-22/23 review). Three +// source shapes all collapse to a string: a plain string passes through +// unchanged; a content-block array (Anthropic permits this, and Claude +// Code sends it for a tool_result carrying more than plain text) has its +// text blocks concatenated; an absent/nil content becomes "". Handling +// all three uniformly, here, is what lets openAIToolMessageFromAnthropic's +// is_error prefix apply regardless of the source shape — the PREVIOUS +// version only prefixed a Go string and left the array form unmarked +// entirely, silently dropping the failure signal for real Claude Code +// traffic (item 11 only covered half of it). It also fixes a second, +// separate bug the array-only gap masked: an absent content previously +// passed through as a literal Go nil, which this function's caller wrote +// out as the JSON literal null — OpenAI rejects a null tool-message +// content outright, where "" is accepted. +func openAIToolResultContentString(content any) string { + switch c := content.(type) { + case string: + return c + case []any: + var parts []string + for _, block := range c { + bm, ok := block.(map[string]any) + if !ok { + continue + } + if text, ok := bm["text"].(string); ok { + parts = append(parts, text) + } + } + return strings.Join(parts, "") + default: + return "" } - return content } // openAIMessagesFromAnthropic maps one Anthropic message ({"role", diff --git a/translate_anthropic_test.go b/translate_anthropic_test.go index 475f3d9..a378311 100644 --- a/translate_anthropic_test.go +++ b/translate_anthropic_test.go @@ -724,11 +724,15 @@ func TestOpenAIMessagesFromAnthropic_InterspersedToolResults(t *testing.T) { } // TestOpenAIToolMessageFromAnthropic_PreservesIsError is the regression -// for item 11a (IMPORTANT, 2026-08-22 review): Anthropic's -// tool_result.is_error has no native OpenAI tool-message field, so the -// previous version of this function silently dropped it. A string -// content now gets an "Error: " prefix so the failure signal survives -// into the one shape OpenAI's tool message actually carries. +// for item 11a and F4 (IMPORTANT/BLOCKING-adjacent, 2026-08-22/23 +// review): Anthropic's tool_result.is_error has no native OpenAI +// tool-message field, so a prior version of this function silently +// dropped it — and a second prior version prefixed only a Go string, +// silently dropping the signal again for the array-content form Claude +// Code actually sends. All content shapes now normalize to a string +// (openAIToolResultContentString) before the "Error: " prefix applies, +// and an absent content becomes "" rather than the JSON null OpenAI +// rejects for a tool message. func TestOpenAIToolMessageFromAnthropic_PreservesIsError(t *testing.T) { cases := []struct { content any @@ -738,13 +742,22 @@ func TestOpenAIToolMessageFromAnthropic_PreservesIsError(t *testing.T) { }{ {name: "string content with is_error prefixes Error:", content: "boom", isError: true, wantContent: "Error: boom"}, {name: "string content without is_error is unchanged", content: "ok", isError: false, wantContent: "ok"}, - {name: "non-string content is left as-is even with is_error", content: []any{"x"}, isError: true, wantContent: []any{"x"}}, + {name: "array-form content with is_error is normalized to a string and prefixed (F4)", content: []any{map[string]any{"type": "text", "text": "boom"}}, isError: true, wantContent: "Error: boom"}, + {name: "array-form content without is_error joins its text blocks (F4)", content: []any{map[string]any{"type": "text", "text": "part1"}, map[string]any{"type": "text", "text": "part2"}}, isError: false, wantContent: "part1part2"}, + {name: "absent content with is_error still prefixes onto an empty string, never null (F4)", content: nil, isError: true, wantContent: "Error: "}, + {name: "absent content without is_error is an empty string, never null (F4)", content: nil, isError: false, wantContent: ""}, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { - bm := map[string]any{"tool_use_id": "call_1", "content": tc.content, "is_error": tc.isError} + bm := map[string]any{"tool_use_id": "call_1", "is_error": tc.isError} + if tc.content != nil { + bm["content"] = tc.content + } got := openAIToolMessageFromAnthropic(bm) assert.Equal(t, tc.wantContent, got["content"]) + if _, ok := got["content"].(string); !ok { + t.Errorf("content = %#v (%T), want a string (OpenAI rejects a null/non-string tool message content)", got["content"], got["content"]) + } }) } }