Uh oh!
There was an error while loading. Please reload this page.
test(e2e): C7 /v1/rerank + /v1/images/generations forward + model translation - #167
Merged
Conversation
…nslation (#151) Two more endpoints from #151's C7 row, both following the same "verbatim-forward with model rewrite" pattern per docs: - `/v1/rerank` (docs §4.7): "Cohere-style rerank. Routed to `{base}/v1/rerank`. The Model's provider supplies the API key; the request body is forwarded verbatim after rewriting the `model` field." - `/v1/images/generations` (docs §4.9): "OpenAI Images API. Forwarded with the `model` field rewritten." Prior to this PR the gateway had **zero** e2e coverage on either endpoint. Both are real production surfaces: - Rerank is the standard relevance-scoring step in modern RAG pipelines. A regression that mangled scores or reordered results would silently corrupt RAG ranking quality across every customer using rerank. - Image generation (DALL-E / GPT-Image) is increasingly common in agent-style workflows. A regression that dropped the image url or revised_prompt fields would leave callers with no signal about what was generated. Two cases pinned, one per endpoint: Rerank: caller POSTs Cohere-shape `{model, query, documents, top_n}` to /v1/rerank. Asserts: - Caller-side response: `id`, results array (3 items), each item's `index` + `relevance_score` byte-for-byte from upstream, `meta` present - Upstream-side: hit `/v1/rerank` (NOT chat-completions), `Bearer sk-mock` auth, `model` rewritten to upstream model_name, query + documents + top_n forwarded verbatim Images: caller POSTs OpenAI-shape `{model, prompt, n, size, response_format}` to /v1/images/generations. Asserts: - Caller-side response: `created` numeric, `data[0].url` and `data[0].revised_prompt` byte-for-byte from upstream - Upstream-side: hit `/v1/images/generations` (NOT another route), `Bearer sk-mock` auth, `model` rewritten, prompt + n + size + response_format forwarded verbatim References: - Gateway's own /v1/rerank contract: `docs/api-proxy.md` §4.7 - Gateway's own /v1/images/generations contract: §4.9 - Cohere Rerank API spec: <https://docs.cohere.com/reference/rerank> - OpenAI Images API spec: <https://platform.openai.com/docs/api-reference/images/create> Refs #151
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Free Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis pull request adds two independent end-to-end test suites for the AI gateway. The first tests the ChangesImages Generations E2E Test
Rerank E2E Test
Estimated code review effort🎯 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
Adds end-to-end (source-blind) regression coverage for two previously uncovered proxy endpoints—/v1/rerank and /v1/images/generations—verifying the gateway’s “verbatim forward with model rewrite” contract and correct upstream dispatch.
Changes:
- Introduce an e2e test for
/v1/rerankthat asserts upstream path, upstream auth injection,modeltranslation, and response passthrough. - Introduce an e2e test for
/v1/images/generationsthat asserts upstream path, upstream auth injection,modeltranslation, and response passthrough (includingurl+revised_prompt).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tests/e2e/src/cases/rerank-e2e.test.ts | New e2e coverage for /v1/rerank forward + model rewrite + strict upstream/response assertions. |
| tests/e2e/src/cases/images-generations-e2e.test.ts | New e2e coverage for /v1/images/generations forward + model rewrite + strict upstream/response assertions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This was referenced May 9, 2026
Uh oh!
There was an error while loading. Please reload this page.
moonming added a commit
that referenced
this pull request
May 9, 2026
Closes#157 (test-infra concern, not a product bug). Issue: the e2e suite's `guardrail-keyword-e2e.test.ts` has flaked three times on CI in the past 24 hours (#157 first occurrence, plus reruns required on PR #165 and #167). Failure mode is the same: `waitConfigPropagation: condition not met within 5s`. Root cause: vitest is configured with `maxForks: 4` (per `vitest.config.ts`), so up to 4 test files run in parallel, each spawning its own `aisix` binary against a SHARED etcd. Each binary opens watches and writes resources via the admin API concurrently with the others. Under that load, etcd watch dispatch latency for the LAST resource in a multi-resource batch (e.g. a Guardrail rule following Model + ApiKey + ProviderKey writes) can exceed the 5s budget. The 5s budget was sized when the suite had ~9 files. The suite is now 20+ files (#161, #163, #165, #167 added embeddings, responses, passthrough, rerank, images). The growth in parallelism load wasn't matched by a budget bump. Fix: raise the deadline to 10s. This: - Eliminates the recurring rerun churn on feature PRs - Preserves the "fail loudly on a genuinely stuck snapshot" property — 10s is still a generous floor; a real bug where propagation hangs indefinitely would still fail clearly - Doesn't change the happy-path latency at all (the helper polls every 50ms and returns as soon as the condition is met — bumping the deadline only affects the sad path) This is a test-infra-only change; no product behavior is affected. The ≤500ms spec target for in-process propagation is unchanged; this is purely the CI test harness's wait budget for slow runners under concurrent load. If 10s proves insufficient as the suite grows further, the next escalation would be to reduce `maxForks` from 4 to 2 (slower wall time, less etcd pressure) — tracked in #157 as a fallback.
moonming added a commit
that referenced
this pull request
May 9, 2026
… (#169) * test(harness): raise waitConfigPropagation budget 5s → 10s (#157) Closes#157 (test-infra concern, not a product bug). Issue: the e2e suite's `guardrail-keyword-e2e.test.ts` has flaked three times on CI in the past 24 hours (#157 first occurrence, plus reruns required on PR #165 and #167). Failure mode is the same: `waitConfigPropagation: condition not met within 5s`. Root cause: vitest is configured with `maxForks: 4` (per `vitest.config.ts`), so up to 4 test files run in parallel, each spawning its own `aisix` binary against a SHARED etcd. Each binary opens watches and writes resources via the admin API concurrently with the others. Under that load, etcd watch dispatch latency for the LAST resource in a multi-resource batch (e.g. a Guardrail rule following Model + ApiKey + ProviderKey writes) can exceed the 5s budget. The 5s budget was sized when the suite had ~9 files. The suite is now 20+ files (#161, #163, #165, #167 added embeddings, responses, passthrough, rerank, images). The growth in parallelism load wasn't matched by a budget bump. Fix: raise the deadline to 10s. This: - Eliminates the recurring rerun churn on feature PRs - Preserves the "fail loudly on a genuinely stuck snapshot" property — 10s is still a generous floor; a real bug where propagation hangs indefinitely would still fail clearly - Doesn't change the happy-path latency at all (the helper polls every 50ms and returns as soon as the condition is met — bumping the deadline only affects the sad path) This is a test-infra-only change; no product behavior is affected. The ≤500ms spec target for in-process propagation is unchanged; this is purely the CI test harness's wait budget for slow runners under concurrent load. If 10s proves insufficient as the suite grows further, the next escalation would be to reduce `maxForks` from 4 to 2 (slower wall time, less etcd pressure) — tracked in #157 as a fallback. * test(harness): also reduce maxForks 4→2 (#157 fallback escalation) After the timeout bump landed, #169's own CI run still flaked with the same `condition not met within 10s` error — meaning etcd watch dispatch latency genuinely exceeds 10s under maxForks=4 with the current suite size (20+ files). The audit on #169 had flagged this as the "product-side hypothesis still open" — confirmed. Apply the documented fallback from #157: cut maxForks from 4 to 2. This halves the concurrent-watcher count against the shared etcd and brings dispatch latency back inside the budget. Trade-off: wall time grows ~1.5-2× per CI run (locally measured ~14s @ maxForks=4 vs ~30s @ maxForks=2, so faster than expected because the extra parallelism wasn't fully utilized anyway — contention dominated). Net for CI is "predictable green" vs "fast but constantly-rerunning". The 10s `waitConfigPropagation` budget from the prior commit stays in place as belt-and-suspenders — even with maxForks=2 the original 5s would still be tight on slow runners. Combined this PR now does: 1. waitConfigPropagation deadline 5s → 10s (`harness/admin.ts`) 2. maxForks 4 → 2 (`vitest.config.ts`) The product-side hypothesis ("etcd watch dispatch degrades non-linearly with concurrent watchers") is now strongly supported and worth a separate product-side investigation tracked in #157 follow-up.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two more endpoints from #151's C7 row, both following the same "verbatim-forward with `model` rewrite" pattern per gateway docs:
Prior to this PR the gateway had zero e2e coverage on either endpoint.
What's pinned
Why these matter
Source-blind discipline
Every assertion derives from external contracts:
No internal Rust paths or struct field names referenced.
Independent audit
Per CLAUDE.md §8, an independent audit agent reviewed commit 84bbd8e. Result: 0 HIGH, 0 MEDIUM blockers, 2 LOW informational notes (consistent with merged precedent), 1 MEDIUM-INFO gateway/product gap surfaced for separate tracking — filed as #168 (§4.7/§4.9 over-promise provider matrix; only OpenAI-compat upstreams actually have these routes). Per CLAUDE.md §8, gateway-product gaps are filed separately rather than blocking merge.
Test plan
Refs #151.