test(audit): tighten embedding-alias + weighted-routing assertions - #142
Conversation
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Free Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR tightens an embeddings forwarding test to assert exactly one upstream POST and updates routing regressions: one adjusts the heavy/light weight split to 100/1; another adds a test that swaps the heavy weight to index 1 and verifies selection frequency over repeated trials. ChangesTest Coverage Improvements
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Note 🎁 Summarized by CodeRabbit FreeYour organization is on the Free plan. CodeRabbit will generate a high-level summary and a walkthrough for each pull request. For a comprehensive line-by-line review, please upgrade your subscription to CodeRabbit Pro by visiting https://app.coderabbit.ai/login. Comment |
There was a problem hiding this comment.
Pull request overview
Tightens the aisix-proxy test suite to prevent two audit-identified false positives: (1) ensuring the embeddings alias-rewrite test actually asserts the upstream mock was hit, and (2) strengthening weighted-routing distribution tests so a “always pick index 0” regression is caught.
Changes:
- Add a companion
weighted_pickaggregate distribution test with the heavy weight at index 1 (and prime total weight) to detect index-biased implementations. - Add
wiremock.expect(1)to the embeddings alias test to assert the mock fired exactly once.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| crates/aisix-proxy/src/routing.rs | Adds an additional aggregate-distribution test that swaps target weight ordering to catch index-biased regressions. |
| crates/aisix-proxy/src/embeddings.rs | Tightens the model-alias forwarding test by asserting the upstream mock is actually exercised via .expect(1). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Two HIGH-severity audit findings on previously-merged tests: 1. embeddings::upstream_request_uses_provider_model_name_not_display_name The mock matched only on rewritten model — but never asserted the mock fired. A regression that fell through to wiremock's default 404 could in principle bubble up as something other than 200, but the explicit `.expect(1)` makes "mock got the request" the load- bearing invariant rather than relying on the 200 status check alone. (#128 finding from independent audit pass.) 2. routing::weighted_pick_aggregate_distribution_respects_index_swap The original distribution test passed for a correctly-weighted impl AND for an "always pick index 0" regression — heavy weight was at index 0. New test swaps weights so heavy sits at index 1 and asserts the bias still kicks in, which a weight-blind impl would now fail. (#133 finding.) Total weight = 101 (prime) is deliberate: `entropy()` advances in small per-iteration steps, so a total sharing a factor with that step lands `entropy() % total` in a tiny set of residue classes and starves the boundary of samples (initial attempt with weights 1/99 saw 53.5% / 5000 vs the expected ~99%, due to mod-100 alignment with the loop's per-iteration cost). A prime total cycles through every residue and restores the expected bias. Two follow-up items NOT addressed here — both expose feature gaps in the gateway, not test gaps: - Audit's #128 finding on Retry-After: gateway does not currently forward upstream's Retry-After on 429 pass-through (only emits one for its own RateLimit). Adding the assertion would fail on main. - Audit's #129 finding on Anthropic taxonomy leak: anthropic bridge copies upstream body verbatim (truncated) into error.message, so asserting "no provider taxonomy in message" would fail on main. Both should be filed as separate feature issues. Refs #127.
e9eb4d9 to
9e34e66
Compare
Summary
Two HIGH-severity findings from an independent audit pass over recently-merged tests.
1.
embeddings::upstream_request_uses_provider_model_name_not_display_nameThe mock matched only on the rewritten model name but never asserted that the mock actually fired.
.expect(1)makes "mock got the request" the load-bearing invariant rather than relying on the 200 status check alone — a regression that fell through to wiremock's default-404 path could in principle pass without it. (Finding from #128 audit.)2.
routing::weighted_pick_aggregate_distribution_respects_index_swap(new test)The original distribution test passed for a correctly-weighted impl AND for an "always pick index 0" regression — the heavy weight sat at index 0. The new test swaps weights so heavy sits at index 1, which a weight-blind impl would now fail. (Finding from #133 audit.)
Total weight = 101 (prime) is deliberate. First attempt used weights
1/99(total 100) and got 53.5% over 5000 trials, far below the 60% threshold. Root cause:entropy()advances by roughly the per-iteration cost ofweighted_pick(a few thousand nanoseconds), and a total that shares a factor with that step landsentropy() % totalin a tiny set of residue classes — starving the 1-of-100 boundary of samples. A prime total cycles through every residue and restores the expected ~99% bias.Findings NOT addressed here (feature gaps, not test gaps)
The audit also flagged two HIGH items that surfaced gateway behavior, not test gaps:
Retry-Afternot forwarded on 429 —error.rs::retry_after_secs()only emits a header for the gateway's ownRateLimit, not forBridgeError::UpstreamStatuspass-through. Adding the audit's suggested assertion would fail onmain.error.message—aisix-provider-anthropic::map_http_errorcopies the upstream's body verbatim (truncated to 1024) intoBridgeError::UpstreamStatus.message, so a "no provider-shape strings in message" assertion would fail onmain.Both should be filed as separate feature-track issues.
Test plan
cargo test --package aisix-proxy --lib— 138 passed, 0 failed (was 137 before)cargo fmt --check— cleancargo clippy --tests -- -D warnings— zero warningsRefs #127.
Summary by CodeRabbit