test(e2e): remove the Presidio guardrail suite - #35129
Conversation
Drops tests/e2e/guardrails/test_presidio_guardrail_e2e.py and the PresidioParamsBody it was the only caller of. Both cases were red on most stage runs between 07-25 and 07-29: pre_call failed 6 of 11 runs, post_call 6 of 11, with post_call reporting the raw address reaching the caller while apply_to_output was set. The cause was propagation, not masking. GuardrailsClient.register() posts /guardrails and returns immediately with no readiness wait, unlike ProxyClient._await_model_servable or GuardrailsClient._await_team, and the data plane only picks a new guardrail up on its next periodic DB sync. Calls issued before that sync pass the raw value through. #34833 has since made both cases poll to the deadline, and on the current build each masks on the first attempt, so the suite is expected to be green now; it is being removed because it spends real provider money on every retry and because a pod replaced mid-poll still reproduces the old failure. The three guardrail.presidio.* rows stay in coverage_registry/guardrail.yaml and go uncovered on purpose, so Presidio reads as a tier-P0 gap in Grafana rather than dropping out of the denominator.
Greptile SummaryThis PR removes the flaky, provider-billed Presidio end-to-end suite and its now-unused request model.
Confidence Score: 4/5The PR appears safe to merge from a runtime perspective, but it deliberately leaves supported Presidio PII masking without end-to-end regression coverage. Production behavior is unchanged and the removed payload type has no remaining callers, while deleting both live Presidio cases means integration regressions will not be exercised by the automated suite. Files Needing Attention: tests/e2e/guardrails/test_presidio_guardrail_e2e.py
|
| Filename | Overview |
|---|---|
| tests/e2e/guardrails/test_presidio_guardrail_e2e.py | Deletes the only live-proxy Presidio masking coverage, leaving supported pre-call and post-call behavior without end-to-end regression protection. |
| tests/e2e/guardrails/guardrails_client.py | Safely removes the Presidio payload model and union member alongside their sole caller; no remaining imports or compatible callers depend on them. |
Comments Outside Diff (1)
-
tests/e2e/guardrails/test_presidio_guardrail_e2e.pyThis deletes the only live-proxy coverage of Presidio pre-call and post-call masking; the remaining isolated and mocked tests do not exercise registration, synchronization, or chat completion, so integration regressions can pass the suite while the registry only reports the already-known coverage gap.
Rule Used: What: Flag any modifications to existing tests and... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Reviews (1): Last reviewed commit: "test(e2e): remove the Presidio guardrail..." | Re-trigger Greptile
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
@greptile-apps Agreeing with the coverage concern, with one correction and a concrete alternative. I dug into whether this test can be written so it stops flipping, and there is a signal we are not using. The review says P2; these are P0. All three rows in coverage_registry/guardrail.yaml are tier: P0, and guardrail.presidio.pre_call.masks carries the rationale "PII masking pre-call; data-leak blast radius". So the severity here is higher than labelled, not lower. On whether it would pass next run. I verified both cases against a live proxy on the current build and each masked on the first attempt (My email is <EMAIL_ADDRESS>, and Please contact <EMAIL_ADDRESS> <DATE_TIME>), with the analyzer answering 200 in 0.3s at score 1.0. But I would not claim it reliably passes, because the reason it flips is still present: nothing in the test waits for the guardrail to reach the pod that serves the chat call. It only retries until masking happens to appear. Why the current shape is fragile. poll_until_blocked / _poll_until_masked cannot distinguish "the guardrail never synced to this pod" from "the guardrail synced and failed to mask". Both end the loop with the same unmasked content and the same assertion text. That has two consequences: a genuine masking regression is indistinguishable from propagation lag, and every retry is a real billed provider call. It also means anything that replaces the serving pod mid-poll reproduces the old failure, since a fresh pod starts with no guardrail until its next DB sync. What I tried and ruled out. The obvious fix is a readiness gate in register(), mirroring ProxyClient._await_model_servable and GuardrailsClient._await_team. /guardrails/list is not usable for that: on the data plane it answered {"guardrails":[]} for 40s after a successful POST /guardrails (it appears to reflect config-file guardrails, not DB-registered ones), so polling it would hang rather than gate. What does work. The proxy already returns x-litellm-applied-guardrails (set in litellm/proxy/common_utils/callback_utils.py:387). On a call opting into a freshly registered guardrail I get: applied-guardrails header: 'dbg-hdr-probe' poll until the header names the guardrail: that is propagation, retry it, and the failure message becomes "guardrail never reached the data plane", not "masking failed" I am happy to close this PR and put up that version instead; that is my recommendation over deleting the suite. Flagging that the header approach is only verified by hand on stage so far, not as a committed test. |
|
The developer's analysis is correct on every point, and the header-based approach is the right fix. A few things to add from reading the infrastructure:
The cleanest route is to use # phase 1: poll until the guardrail has synced to the serving pod
deadline = time.monotonic() + POLL_TIMEOUT
while time.monotonic() < deadline:
raw = client.proxy.transport.send(...) # StreamingResponse
applied = raw.headers.get("x-litellm-applied-guardrails", "")
if guardrail_name in applied:
break
time.sleep(POLL_INTERVAL)
else:
pytest.fail(f"guardrail {guardrail_name!r} never appeared in x-litellm-applied-guardrails within {POLL_TIMEOUT}s")
# phase 2: assert masking exactly once, no retry
content = ... # parse raw.body as JSON once
assert RAW_EMAIL not in content, ...
assert PLACEHOLDER in content, ...This keeps the two failure modes separate and eliminates the billed-retry loop entirely. Alternatively, if you want to stay on the typed path throughout, adding Bottom line: Close this PR and put up the header-based replacement. Deleting P0 PII masking coverage is the wrong trade even with the flake history, and the mechanism to fix it is already there. |
|
removing for now, rewriting the test later to make it more reliable. right now it checks a stale endpoint /guardrails/list that returns an empty list created a linear ticket to track |
5182dfa
into
litellm_internal_staging
TLDR
Problem this solves:
tests/e2e/guardrails/test_presidio_guardrail_e2e.pywas red on most stage runs between 07-25 and 07-29:pre_callfailed 6 of 11 runs,post_call6 of 11. Between them they were the two most frequently failing guardrail cases in the suiteGuardrailsClient.register()posts/guardrailsand returns immediately with no readiness wait, unlikeProxyClient._await_model_servableorGuardrailsClient._await_team. The data plane only picks a new guardrail up on its next periodic DB sync, so calls issued before that sync pass the raw value through and the test reads it as a masking failureHow it solves it:
PresidioParamsBody, which it was the only caller ofVerification, and the honest caveat
I verified both cases against a live proxy on the current build before writing this, and both pass on the first attempt:
Presidio itself is healthy: the analyzer answers 200 in 0.3s and detects
EMAIL_ADDRESSat score 1.0.So this is not removing a suite that is currently broken. #34833 landed the sync-wait after the failing runs I measured, and all of the flake history above predates it. The remaining arguments for removal are cost (a retry loop that bills a provider call per attempt) and residual fragility (a pod replaced mid-poll still reproduces the old failure, since the retry loop cannot distinguish "never synced" from "synced but did not mask"). If you would rather keep the coverage now that it is green, this PR should be closed rather than merged; I have no evidence of a current product defect here.
Relevant issues
Related: #34833 (added the presidio sync-wait).
logging_onlywas already carved out of this file under LIT-4841.Linear ticket
Pre-Submission checklist
Screenshots / Proof of Fix
Deleting tests has no runtime surface to curl, so the relevant proof is that the rest of the suite is unaffected and the removed behaviour genuinely works on a live proxy.
Suite still collects, two fewer cases:
Typing gate clean:
Live proxy,
post_callwithapply_to_output(the case whose failures looked worst), against a real Gemini deployment and the real Presidio analyzer/anonymizer:Type
✅ Test
Changes
tests/e2e/guardrails/test_presidio_guardrail_e2e.pydeleted (141 lines)tests/e2e/guardrails/guardrails_client.py: dropsPresidioParamsBodyand its arm of theGuardrailParamsBodyunion (11 lines).GuardrailModestays; it is still used byGuardrailParamsBaseThe three
guardrail.presidio.*rows stay incoverage_registry/guardrail.yamlon purpose. They are tier P0, so they now read as uncovered gaps in Grafana rather than dropping out of the denominator and flattering the coverage number.python -m coverage_registry.collector --strictonly rejects orphan markers (a marker with no registry row), and this removes markers rather than adding any, so the check is unaffected.QA runbook
cd tests/e2e && python3 -m pytest guardrails --collect-only -qshows 6 cases and no presidio entriesmake lint-e2e-basedpyrightreports 0 errorspython -m coverage_registry.collectorshows the threeguardrail.presidio.*rows as uncovered P0 gaps under Logging & GuardrailsFinal Attestation