test(e2e): prove the global guardrail is live before asserting the team bypass - #34641
test(e2e): prove the global guardrail is live before asserting the team bypass#34641devin-ai-integration[bot] wants to merge 2 commits into
Conversation
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
|
|
Greptile SummaryThis PR strengthens two end-to-end regression tests.
Confidence Score: 3/5This PR should not merge until the cache-settings test preserves and restores the complete original cache configuration. The cache endpoint replaces omitted non-secret settings, while the changed test posts and restores only four fields, allowing existing settings to be cleared and an originally absent namespace to remain changed. Files Needing Attention: tests/e2e/management/test_config_misc_endpoints_e2e.py
|
| Filename | Overview |
|---|---|
| tests/e2e/management/test_config_misc_endpoints_e2e.py | Strengthens cache persistence validation, but its partial update and restoration payload can destructively alter the shared proxy's cache configuration. |
| tests/e2e/guardrails/test_team_disable_global_guardrail_e2e.py | Adds a valid non-team baseline that proves the global guardrail is active before testing the team-level bypass. |
Reviews (1): Last reviewed commit: "test(e2e): make cache-settings and guard..." | Re-trigger Greptile
| resources.defer(lambda: self._write_settings(client, captured)) | ||
|
|
||
| namespace = f"e2e-cache-ns-{unique_marker()}" | ||
| target = CacheSettingsValue(type=captured.type, host=captured.host, port=captured.port, namespace=namespace) |
There was a problem hiding this comment.
Partial update clears cache settings
When the shared proxy has cache settings beyond type, host, port, and namespace, this four-field update clears the omitted non-secret fields because /cache/settings replaces them rather than preserving them. The deferred restore uses the same partial model, so settings such as TTL, database, SSL, or Sentinel configuration remain removed; when the original namespace is null, serialization also omits it and leaves the generated namespace active, changing cache connectivity or key behavior for later tests.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
TLDR
Problem this solves:
How it solves it:
Relevant issues
Linear ticket
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
@greptileaito re-request a review after pushing changes)Delays in PR merge?
If you're seeing a delay in your PR being merged, ping the LiteLLM Team on Slack (#pr-review).
Screenshots / Proof of Fix
The test needs a live proxy with gemini credentials, which I don't have provisioned in this environment, so I have not run it live; the QA runbook below is the manual curl sequence it mirrors. Local checks at ac25510:
make lint-e2e-basedpyrightreports 0 errors andmake pre-commitpassesType
✅ Test
Changes
test_team_with_disable_flag_bypasses_global_guardrailasserted only that a key on the opted-out team got a completion. A guardrail that was never registered, never propagated to the worker, or silently broken produces the same pass, so the test could not distinguish "the opt-out works" from "there was nothing to opt out of". It now runs the existing_assert_eventually_blockedhelper againstscoped_keyfirstThe baseline doubles as the propagation wait the bypass request previously raced, since the helper polls to a 40s deadline while the new guardrail syncs to every worker
This PR originally also reworked
TestCacheSettings::test_update_persists_cache_backend_to_get(which wrote back identical settings, so a no-op/cache/settingspassed) to write a freshnamespaceand read it back. #34664 landed onlitellm_internal_stagingfirst and deleted that test outright, on the grounds that no round-trip through this route is safe on a shared proxy: GET cannot express the effective config, so any write assembled from it can dropssl/redis_startup_nodesand downgrade a TLS cluster to a plaintext node. A partial write with a changednamespacehas exactly that hazard, so I took the deletion in the merge rather than resurrecting the test; the registry row stays deliberately uncovered pending LIT-4816QA runbook
litellm_content_filterguardrail banning a unique keyword viaPOST /guardrails(needs gemini credentials for the chat calls)POST /v1/chat/completionswith a plain key and a prompt containing that keyword; expect a 400 naming the content block (retry for up to 40s while the guardrail syncs to every worker)metadata.disable_global_guardrailsset, generate a key in it, and send the same prompt; expect a 200 with choicesFinal Attestation
Link to Devin session: https://app.devin.ai/sessions/16e9ea5475e44d1d9d6745b2436d8bf0