Uh oh!
There was an error while loading. Please reload this page.
fix(proxy): normalise upstream error.type to DP taxonomy, keep code/param verbatim (#327) - #328
Conversation
…/param verbatim (#327) #323 over-corrected by preserving upstream's `error.type` verbatim, which: - broke `dp-upstream-error-live.spec.ts:152` (asserting the stable `"upstream_error"` token) on every PR's e2e-playwright run from the merge forward - leaked upstream-private taxonomy to customers (mock-llm's `upstream_test_fixture`, Bedrock's `AccessDeniedException`, Anthropic's `authentication_error` etc.) - defeated the gateway's role as a normalising layer that hides the upstream zoo behind a stable, customer-SDK-actionable taxonomy This commit restores the DP-stable `error.type = "upstream_error"` contract for upstream errors, while keeping the #322/#323 preservation of `error.code` and `error.param` — those are still SDK-actionable and customer-facing-positive. Final wire contract for `ProxyError::Bridge(UpstreamStatus { .. })`: | Field | Source | |----------------|---------------------------------| | `error.type` | DP taxonomy: `"upstream_error"` | | `error.message`| upstream verbatim (4xx) / canned `"upstream returned N"` (5xx) | | `error.code` | upstream verbatim or derived per-wire (Anthropic/Bedrock/Vertex/Azure) | | `error.param` | upstream verbatim | Customer SDKs branch on `error.type == "upstream_error"` for upstream-class detection and on `error.code` for granular retry routing (`rate_limit_exceeded` vs `insufficient_quota` vs `model_not_found` etc.). Implementation: * `error_translate::render_openai_envelope` hardcodes `kind` to the new `UPSTREAM_ERROR_TYPE` constant. The translation tables (Anthropic / Bedrock / Vertex / Azure) collapse from returning `(String, Option<String>)` to just `Option<String>` — the type half is now uniformly `"upstream_error"`, so the per-wire tables only carry the derived OpenAI string code. * Generic envelope (missing parsed view) also emits `"upstream_error"` — unchanged from prior behaviour. * 5xx and `UpstreamWire::Unknown` paths in `render_bridge_upstream_envelope` already emit `"upstream_error"`; this commit makes the 4xx path consistent. Tests: * All 22 `error_translate::tests` updated: `body.kind` assertions pinned to `"upstream_error"`; code assertions unchanged. * 3 integration tests in `aisix-proxy::tests` updated and renamed: - `upstream_openai_4xx_forwards_full_envelope_per_issue_322` → `upstream_openai_4xx_forwards_code_and_param_but_normalises_type_per_issue_327` (asserts `upstream_test_fixture` does NOT leak) - `upstream_anthropic_400_passes_through_with_openai_envelope` → `upstream_anthropic_400_normalises_type_to_upstream_error` - `upstream_anthropic_rate_limit_translates_to_openai_rate_limit_exceeded` → `upstream_anthropic_rate_limit_derives_openai_rate_limit_exceeded_code` (the rate-limit `code` derivation is the customer-facing value; the `type` is normalised) Closes#327 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Free Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR normalizes upstream error type handling in the proxy: ChangesError type normalization and code derivation
🎯 3 (Moderate) | ⏱️ ~25 minutes Note 🎁 Summarized by CodeRabbit FreeYour organization has reached its limit of developer seats under the Pro Plan. For new users, CodeRabbit will generate a high-level summary and a walkthrough for each pull request. For a comprehensive line-by-line review, please add seats to your subscription by visiting https://app.coderabbit.ai/login.If you believe this is a mistake and have available seats, please assign one to the pull request author through the subscription management page using the link above. Comment |
There was a problem hiding this comment.
Pull request overview
This PR corrects an over-correction from #323: while error.code and error.param are still preserved verbatim from upstream (preserving #322's SDK retry-routing fix), error.type is now normalized to the DP-stable "upstream_error" token across all wires/paths, so upstream-private taxonomies (upstream_test_fixture, AccessDeniedException, authentication_error, etc.) no longer leak to customers. The translation tables in error_translate.rs are collapsed from (type, code) pairs to a single derived code.
Changes:
- Hardcode
kind = "upstream_error"inrender_openai_envelope; introduceUPSTREAM_ERROR_TYPEconstant. - Rename
translate_*→derive_*_codeand returnOption<String>only (the OpenAI-shapecode). - Update unit and integration tests + their docstrings/names to assert the corrected contract.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
crates/aisix-proxy/src/error_translate.rs | Drop per-wire type derivation; emit DP-stable upstream_error and only derive OpenAI code. Update module doc and tests. |
crates/aisix-proxy/src/lib.rs | Rename/retighten 3 integration tests to assert type == "upstream_error" while keeping code/param preservation assertions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Audit completed — verdict: merge, no HIGH/MEDIUM findings. Two LOW findings:
Per CLAUDE.md §8 merge gate: ready when CI passes. |
moonming
commented
May 18, 2026
Follow-up on the audit's LOW-1: traced the path, not a real bug. |
Summary
Fixes#327. PR #323 over-corrected by preserving upstream's
error.typeverbatim — this broke the stable"upstream_error"contract thatdp-upstream-error-live.spec.ts:152already pins, and leaked upstream-private taxonomy (upstream_test_fixture,AccessDeniedException,authentication_error, etc.) to customers.Contract correction
error.type"upstream_error"error.messageerror.codeerror.paramCustomer SDKs branch on
error.type == "upstream_error"for upstream-class detection and onerror.codefor granular retry routing.Implementation
error_translate::render_openai_envelope: hardcodeskindto a newUPSTREAM_ERROR_TYPEconstant ("upstream_error").(String, Option<String>)toOption<String>— the per-wire OpenAItypehalf is now uniformly"upstream_error", so the tables only carry the derived OpenAI stringcode.UpstreamWire::Unknown, and generic-envelope paths already emit"upstream_error"— this commit just makes the 4xx path consistent.Reference impl divergence (CLAUDE.md §7)
The established gateway impls in this space normalise upstream taxonomy too — LiteLLM maps to a closed set of Python exception classes, Portkey emits its own stable
typeenum. Choosing a single DP token ("upstream_error") rather than a closed OpenAI-vocabulary set (invalid_request_error,rate_limit_exceeded,server_error) is more conservative — it sidesteps the question of how Bedrock'sAccessDeniedExceptionshould map onto OpenAI'sinvalid_request_errorvsauthentication_error, since the customer just needs "upstream broke; checkcodefor granularity."Test plan
cargo check --workspace --all-targetscleancargo clippy --workspace --all-targetscleancargo fmt --checkcleancargo test --workspace— all tests greenerror_translate::testsupdated:body.kindalways"upstream_error", code assertions unchangedaisix-proxy::testsupdated and renamed to reflect the corrected contract:upstream_openai_4xx_forwards_code_and_param_but_normalises_type_per_issue_327(was..._forwards_full_envelope_per_issue_322)upstream_anthropic_400_normalises_type_to_upstream_error(was..._passes_through_with_openai_envelope)upstream_anthropic_rate_limit_derives_openai_rate_limit_exceeded_code(was..._translates_to_openai_rate_limit_exceeded)dp-upstream-error-live.spec.ts:152should go back to green once:devimage is rebuilt with this PR merged.adapter-openai-overrides-live.spec.ts:634fromupstream_test_fixturetoupstream_error) is the issue author's follow-up.Closes#327
Summary by CodeRabbit