From 828fb3e9090080782091f452c6790bb521bfb2a3 Mon Sep 17 00:00:00 2001 From: Yuneng Jiang Date: Mon, 7 Sep 2026 20:57:22 -0700 Subject: [PATCH] test(guardrails): tighten the presidio contract assertions Review follow-ups on the contract suite. The restore case only checked the email, so a regression that left in the answer passed. It now requires every masked value back and rejects any numbered token by shape rather than by name. Its poll returned as soon as an email placeholder appeared, which read as accepting an unrestored response. The condition is now "the model echoed the line in either form", and the docstring says why that is the right stopping point: the restore runs per request, so a token that survived it is a verdict and not something a retry fixes. The unreachable case claimed to check that the failure does not name the endpoint and never asserted it. It does now, and the unreachable base moved from a loopback address to a host in the reserved .invalid domain, because the proxy already rewrites IPs out of error messages: against 127.0.0.1 that assertion could not fail even with the sanitizer removed. Verified by putting the raw exception back into the analyzer error, which now turns the case red and previously did not. A hostname is also what a real deployment looks like. The registry entries used assertion names outside the documented guardrail grammar. Rather than rename behaviors that are genuinely distinct, the grammar now groups the names in use, including logs_masked_entities, which was already in the registry and already outside it. The exclusion pattern is back to two explicit filenames, so a future self-contained presidio suite is not silently kept off the lane. --- .github/e2e-stack/select_tests.py | 3 +- tests/e2e/CLAUDE.md | 16 ++++++- .../guardrails/test_presidio_contract_e2e.py | 46 ++++++++++++++----- 3 files changed, 50 insertions(+), 15 deletions(-) diff --git a/.github/e2e-stack/select_tests.py b/.github/e2e-stack/select_tests.py index 633ee6d8a23..52013994381 100644 --- a/.github/e2e-stack/select_tests.py +++ b/.github/e2e-stack/select_tests.py @@ -7,7 +7,8 @@ UNSUPPORTED: Final = re.compile( r"^tests/e2e/(ui|claude_code|load)/" r"|^tests/e2e/llm_translation/realtime/test_realtime_pipecat_audio_e2e\.py$" r"|^tests/e2e/batches/test_managed_files_enforcement_e2e\.py$" - r"|^tests/e2e/guardrails/test_presidio_.*_e2e\.py$" + r"|^tests/e2e/guardrails/test_presidio_masking_e2e\.py$" + r"|^tests/e2e/guardrails/test_presidio_contract_e2e\.py$" ) HARNESS: Final = re.compile( r"^tests/e2e/[A-Za-z0-9_.-]+\.(py|ini)$" diff --git a/tests/e2e/CLAUDE.md b/tests/e2e/CLAUDE.md index 89c04208d65..a0c4835f732 100644 --- a/tests/e2e/CLAUDE.md +++ b/tests/e2e/CLAUDE.md @@ -203,11 +203,23 @@ logging... guardrail... provider : presidio | lakera | bedrock | aporia | ... - hook_point : pre_call | post_call | during | logging_only - assertion : blocks | masks | allows + hook_point : pre_call | post_call | during | logging_only | apply_endpoint + assertion : blocks | masks | allows + masks_only_configured_entities | masks_each_entity_in_place + | restores_masked_values + logs_masked_entities + fails_closed_when_unreachable e.g. guardrail.presidio.pre_call.masks exercised_on=[chat_completions] + guardrail.presidio.pre_call.fails_closed_when_unreachable exercised_on=[chat_completions] ``` +The verdict names say what the guardrail decided, and are the only ones a new +provider normally needs. The rest exist because a guardrail that talks to an +outside service has a contract with it beyond the verdict: which entity types it +asked for, where the spans it got back were applied, what it recorded for an +operator, and what it does when that service is unreachable. Reach for one of +those only when the case pins that contract rather than the decision + Other - holding pen (endpoint or behavior) ``` diff --git a/tests/e2e/guardrails/test_presidio_contract_e2e.py b/tests/e2e/guardrails/test_presidio_contract_e2e.py index 4ce1a7377ce..e9c973a4fdf 100644 --- a/tests/e2e/guardrails/test_presidio_contract_e2e.py +++ b/tests/e2e/guardrails/test_presidio_contract_e2e.py @@ -24,6 +24,7 @@ as secrets; see presidio_env. from __future__ import annotations +import re import time from collections.abc import Callable from typing import Final, Literal @@ -53,10 +54,16 @@ RAW_PHONE = "+1 415-555-0134" RAW_CARD = "4111 1111 1111 1111" PII_SENTENCE = f"Reach Dana at {RAW_EMAIL} or on {RAW_PHONE} about card {RAW_CARD} today." -UNREACHABLE_BASE = "http://127.0.0.1:9/" +UNREACHABLE_BASE = "http://presidio-unreachable.invalid/" +_UNREACHABLE_HOST: Final = "presidio-unreachable.invalid" MAX_ECHO_TOKENS = 128 +_NUMBERED_TOKEN: Final = re.compile(r"<[A-Z_]+_\d+>") +_ECHOED_THE_LINE: Final = re.compile( + rf"{re.escape(RAW_EMAIL)}|{re.escape(RAW_PHONE)}|<(?:EMAIL_ADDRESS|PHONE_NUMBER)_\d+>" +) + def _register( client: GuardrailsClient, @@ -267,11 +274,11 @@ class TestPresidioOutputParseContract: ) restored: Final = _poll_until_restored(client, scoped_key, name, _echo_prompt(unique_marker())) - assert RAW_EMAIL in restored, ( - "the caller must get the real address back, not the placeholder the model saw; a token left " - f"in the answer means the mapping was lost between the two hooks: {scrub(restored)!r}" + assert RAW_EMAIL in restored and RAW_PHONE in restored, ( + "the caller must get every masked value back, not the placeholders the model saw; a token " + f"left in the answer means the mapping was lost between the two hooks: {scrub(restored)!r}" ) - assert "EMAIL_ADDRESS_1" not in restored, ( + assert not _NUMBERED_TOKEN.search(restored), ( f"no numbered token may survive into the caller's response: {scrub(restored)!r}" ) @@ -297,6 +304,11 @@ def _poll_until_restored(client: GuardrailsClient, key: str, name: str, prompt: The applied-guardrails header is the liveness gate: without it a response carrying the raw address would be indistinguishable from one the guardrail never touched, and the restore assertion would pass vacuously. + + Polling stops as soon as the model has echoed the line in either form, + restored or still carrying a token, because the restore runs per request and + a token that survived it is a verdict rather than something a retry fixes. + Only a model that never echoed at all is worth waiting out. """ deadline: Final = time.monotonic() + POLL_TIMEOUT last = "" # rebind-ok: last-observation accumulator for the failure message @@ -304,7 +316,7 @@ def _poll_until_restored(client: GuardrailsClient, key: str, name: str, prompt: outcome = client.chat_raw(key, MODEL, prompt, guardrails=[name], max_tokens=MAX_ECHO_TOKENS) if outcome.ok and name in outcome.headers.get("x-litellm-applied-guardrails", ""): last = _first_content(ChatResponse.model_validate_json(outcome.body)) - if RAW_EMAIL in last or "EMAIL_ADDRESS" in last: + if _ECHOED_THE_LINE.search(last): return last else: last = f"" @@ -326,11 +338,17 @@ class TestPresidioUnreachableContract: ) -> None: """A Presidio outage must not degrade into "no masking today". - The guardrail is pointed at a closed local port so the analyzer call - cannot connect. With PII entities configured the guardrail fails closed - and the prompt never reaches the model with the address still in it. The - error goes straight back to the caller, so it is also asserted to carry - neither the raw address nor the endpoint it failed to reach. + The guardrail is pointed at a host in the reserved .invalid domain, so + the analyzer call fails to resolve immediately. With PII entities + configured the guardrail fails closed and the prompt never reaches the + model with the address still in it. + + The error goes straight back to the caller, so it is also asserted to + carry neither the raw address nor the endpoint it failed to reach. That + second assertion is why the unreachable base is a hostname rather than a + loopback address: the proxy already rewrites IPs out of error messages, + which would make an endpoint-disclosure assertion pass no matter what, + and a real deployment is addressed by hostname anyway. """ name: Final = f"e2e-presidio-unreachable-{unique_marker()}" _register( @@ -353,7 +371,11 @@ class TestPresidioUnreachableContract: ) body: Final = scrub(str(outcome)) assert RAW_EMAIL not in body, f"the failure must not echo the prompt's PII back to the caller: {body}" + assert UNREACHABLE_BASE not in body and _UNREACHABLE_HOST not in body, ( + "the failure must not name the endpoint the guardrail could not reach; on a real deployment " + f"that address is a secret and this body is rendered straight back to the caller: {body}" + ) assert "Presidio" in body, ( - f"the refusal must say the guardrail could not run, or an operator cannot tell an outage " + "the refusal must say the guardrail could not run, or an operator cannot tell an outage " f"from a model error: {body}" )