From 334982860cf04ffbb1eedc704bfe3418585f89da Mon Sep 17 00:00:00 2001 From: abrichr Date: Fri, 28 Aug 2026 11:35:07 -0400 Subject: [PATCH] fix: decide separated action text by shape, and label identifiers by recognizer Two defects, both reproduced against 1.0.3. scrub_dict mangled a plain "text" value. "text" is in SCRUB_KEYS_HTML, so _scrub_text_item passed is_separated=True for it, and scrub_text re-joined the anonymized result with ACTION_TEXT_SEP whether or not the input had ever been split. A sentence came back one character per hyphen: scrub_dict({"text": "Email: john@example.com"}, scrubber) {'text': '<-P-E-R-S-O-N->-:- -<-E-M-A-I-L-_-A-D-D-R-E-S-S->'} The split and the join are now symmetric: is_separated permits key-sequence handling, and _is_separated_form decides whether to apply it by looking at the value. Action text is exactly one typed character per separator, so "j-o-h-n" is reassembled and re-separated as before, while "follow-up", "555-123-4567" and ordinary prose are scrubbed as prose. Which keys may carry action text is now PrivacyConfig.SCRUB_KEYS_SEPARATED instead of a hard-coded tuple, and scrub_dict/scrub_list_dicts take separated_keys to set it per call. Entity labels named the wrong type, so policy keyed on them was unusable. Presidio's anonymizer settles overlapping spans by span before score, so a spaCy DATE_TIME at 0.85 covering "Card 4111111111111111" replaced a Luhn-checked CREDIT_CARD at 1.0. Same for US_BANK_NUMBER, IP_ADDRESS, and a member ID that came back as PERSON. Nothing leaked, but nothing could be routed on either. _prefer_deterministic_identifier_labels drops the statistical result where a pattern recognizer scoring at least 0.4 claims the span. It drops it only when the pattern matches cover every digit the statistical span held, so relabelling can never uncover part of an identifier: Presidio matches only the leading "A123" of "A123-456-789-012", and a wider span over the rest survives. A card number that fails the Luhn check still gets no CREDIT_CARD label, because no recognizer validates it. That is now a documented, tested limit rather than a surprise, and the README example uses a valid number. test_phi_recall.py asserted only that the identifier had disappeared, which is why this went unnoticed. Every case now pins the entity type as well. --- CLAUDE.md | 2 +- README.md | 56 ++++++--- openadapt_privacy/base.py | 41 ++++++- openadapt_privacy/config.py | 14 +++ openadapt_privacy/pipelines/dicts.py | 20 +++- openadapt_privacy/providers/presidio.py | 108 +++++++++++++++-- tests/test_phi_recall.py | 147 ++++++++++++++++++++---- tests/test_separated_action_text.py | 88 ++++++++++++++ 8 files changed, 414 insertions(+), 62 deletions(-) create mode 100644 tests/test_separated_action_text.py diff --git a/CLAUDE.md b/CLAUDE.md index 8f2184e..fb39e5e 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -85,7 +85,7 @@ scrubbed = scrub_dict(action, scrubber) | EMAIL_ADDRESS | john@example.com | | | PHONE_NUMBER | 555-123-4567 | | | US_SSN | 923-45-6789 | | -| CREDIT_CARD | 4532-1234-5678-9012 | | +| CREDIT_CARD | 4111111111111111 | | | DATE_TIME | 01/15/1985 | | | LOCATION | Toronto, ON | | diff --git a/README.md b/README.md index ec3e0d2..6de490d 100644 --- a/README.md +++ b/README.md @@ -38,15 +38,16 @@ Scrubbing is one control inside a reviewed egress process. It is not a guarantee that an artifact is free of protected data, and the evidence behind it is synthetic, not clinical. -`tests/test_phi_recall.py` is the regression gate: 24 synthetic identifiers +`tests/test_phi_recall.py` is the regression gate: 26 synthetic identifiers across names, contact details, financial identifiers, dates of birth, addresses, network identifiers, medical record numbers, member IDs, and -provider licenses. It requires 24 out of 24, and it also checks that ordinary -operational UI text comes back untouched. +provider licenses. It requires 26 out of 26, it pins the entity type each one +must produce, and it also checks that ordinary operational UI text comes back +untouched. Detection is contextual, so the same value scrubs differently depending on what -surrounds it. Every output below was produced by running 1.0.3 on 2026-08-28, -not written by hand. Some of it will surprise you: +surrounds it. Every output below was produced by running this code on +2026-08-28, not written by hand. Some of it will surprise you: ```python >>> from openadapt_privacy.providers.presidio import PresidioScrubbingProvider @@ -59,14 +60,20 @@ not written by hand. Some of it will surprise you: ': ' >>> s.scrub_text("Card 4111111111111111 on file") -' on file' +'Card on file' + +>>> s.scrub_text("Card: 4532-1234-5678-9012") +'Card: ' ``` -The last one is the important one. A card number gets redacted, but as -`DATE_TIME`, not as `CREDIT_CARD`, and the label "Card" goes with it. The -redaction holds; the entity type you get back is not the one you would predict. -Do not build a policy that keys off the placeholder name without measuring it -against your own data first. +The last two are the interesting pair. Both card numbers get redacted, but only +the first is labelled `CREDIT_CARD`, because only the first passes a Luhn +check. An identifier that no recognizer can validate falls to whatever the +spaCy model makes of it, which for a run of digits is usually `DATE_TIME`. + +So: the redaction holds either way, and a validated identifier now carries its +own type. An unvalidated one does not. Measure the placeholder names against +your own data before you route on them. For production egress: scrub a copy, verify every output file, and bind the human or policy approval to the verified artifact. A model that ran without @@ -115,10 +122,23 @@ Only the keys in `PrivacyConfig.SCRUB_KEYS_HTML` are scrubbed, and non-string values pass through, which is why the coordinates survive. Pass `scrub_all=True` to scrub every string regardless of key. -One caveat in 1.0.3: the `text` key is treated as character-separated action -text, joined by `ACTION_TEXT_SEP` (`-`). Scrubbing a plain sentence under that -key returns it hyphenated, one character at a time. Use `value` or `title` for -ordinary prose until that's fixed. +The keys in `PrivacyConfig.SCRUB_KEYS_SEPARATED` (`text` and `canonical_text`) +can also hold recorded keystrokes: one typed character per `ACTION_TEXT_SEP`, +as in `j-o-h-n-@-e-x-a-m-p-l-e-.-c-o-m`. Those are reassembled before analysis, +scrubbed, and separated again, because the PII is invisible in the split form. +Whether that happens is decided by the value, not the key, so prose under +`text` is scrubbed as prose: + +```python +scrub_dict({"text": "Email: john@example.com"}, scrubber) +# {'text': ': '} + +scrub_dict({"text": "-".join("john@example.com")}, scrubber) +# {'text': '<-E-M-A-I-L-_-A-D-D-R-E-S-S->'} +``` + +Pass `separated_keys=[]` to turn keystroke handling off for a call, or +`separated_keys=["keys"]` to move it to your own field name. ## Scrubbing screenshots @@ -183,9 +203,9 @@ PrivacyConfig( ``` The full field list is `SCRUB_CHAR`, `SCRUB_LANGUAGE`, `SCRUB_FILL_COLOR`, -`SCRUB_KEYS_HTML`, `ACTION_TEXT_NAME_PREFIX`, `ACTION_TEXT_NAME_SUFFIX`, -`ACTION_TEXT_SEP`, `SCRUB_CONFIG_TRF`, `SCRUB_PRESIDIO_IGNORE_ENTITIES`, and -`SPACY_MODEL_NAME`. +`SCRUB_KEYS_HTML`, `SCRUB_KEYS_SEPARATED`, `ACTION_TEXT_NAME_PREFIX`, +`ACTION_TEXT_NAME_SUFFIX`, `ACTION_TEXT_SEP`, `SCRUB_CONFIG_TRF`, +`SCRUB_PRESIDIO_IGNORE_ENTITIES`, and `SPACY_MODEL_NAME`. The analyzer's supported entity set comes from Presidio: `CREDIT_CARD`, `CRYPTO`, `DATE_TIME`, `EMAIL_ADDRESS`, `IBAN_CODE`, `IP_ADDRESS`, `LOCATION`, diff --git a/openadapt_privacy/base.py b/openadapt_privacy/base.py index c60d266..2c836b8 100644 --- a/openadapt_privacy/base.py +++ b/openadapt_privacy/base.py @@ -191,6 +191,7 @@ def scrub_dict( list_keys: list[str] | None = None, scrub_all: bool = False, force_scrub_children: bool = False, + separated_keys: list[str] | None = None, ) -> dict[str, Any]: """Scrub PII/PHI from a nested dictionary. @@ -204,18 +205,29 @@ def scrub_dict( scrub_all: If True, scrub all string values regardless of key. force_scrub_children: If True, use aggressive scrubbing for child values after PII is detected in parent. + separated_keys: Keys that may hold character-separated action text. + Defaults to config.SCRUB_KEYS_SEPARATED. Pass an empty list to + scrub every value as prose. Returns: Scrubbed dictionary with PII/PHI removed. """ + policy = effective_config() if list_keys is None: - list_keys = effective_config().SCRUB_KEYS_HTML + list_keys = policy.SCRUB_KEYS_HTML + if separated_keys is None: + separated_keys = policy.SCRUB_KEYS_SEPARATED scrubbed_dict: dict[str, Any] = {} for key, value in input_dict.items(): if self._should_scrub_text(key, value, list_keys, scrub_all): - scrubbed_text = self._scrub_text_item(value, key, force_scrub_children) - if key in ("text", "canonical_text") and self._is_scrubbed(value, scrubbed_text): + scrubbed_text = self._scrub_text_item( + value, + key, + force_scrub_children, + separated_keys=separated_keys, + ) + if key in separated_keys and self._is_scrubbed(value, scrubbed_text): force_scrub_children = True scrubbed_dict[key] = scrubbed_text elif isinstance(value, list): @@ -228,6 +240,7 @@ def scrub_dict( list_keys, scrub_all=list_scrub_all, force_scrub_children=force_scrub_children, + separated_keys=separated_keys, ) if self._should_scrub_list_item( item, @@ -246,6 +259,7 @@ def scrub_dict( value, list_keys, scrub_all=scrub_all or (isinstance(key, str) and key == "state"), + separated_keys=separated_keys, ) else: scrubbed_dict[key] = value @@ -256,17 +270,22 @@ def scrub_list_dicts( self, input_list: list[dict[str, Any]], list_keys: list[str] | None = None, + separated_keys: list[str] | None = None, ) -> list[dict[str, Any]]: """Scrub PII/PHI from a list of dictionaries. Args: input_list: List of dictionaries to be scrubbed. list_keys: List of keys whose values should be scrubbed. + separated_keys: Keys that may hold character-separated action text. Returns: List of scrubbed dictionaries. """ - return [self.scrub_dict(input_dict, list_keys) for input_dict in input_list] + return [ + self.scrub_dict(input_dict, list_keys, separated_keys=separated_keys) + for input_dict in input_list + ] def _should_scrub_text( self, @@ -305,6 +324,7 @@ def _scrub_text_item( value: str, key: str, force_scrub_children: bool = False, + separated_keys: list[str] | None = None, ) -> str: """Scrub a single text value. @@ -312,11 +332,16 @@ def _scrub_text_item( value: Text value to scrub. key: Dictionary key associated with the value. force_scrub_children: If True, use aggressive scrubbing. + separated_keys: Keys that may hold character-separated action text. Returns: Scrubbed text. """ - if key in ("text", "canonical_text"): + if separated_keys is None: + separated_keys = effective_config().SCRUB_KEYS_SEPARATED + if key in separated_keys: + # A permission, not an instruction: the provider applies separated + # handling only to a value that is genuinely a key sequence. return self.scrub_text(value, is_separated=True) if force_scrub_children: return self.scrub_text_all(value) @@ -351,6 +376,7 @@ def _scrub_list_item( list_keys: list[str], force_scrub_children: bool = False, scrub_all: bool = False, + separated_keys: list[str] | None = None, ) -> Any: """Scrub a single list item. @@ -360,6 +386,7 @@ def _scrub_list_item( list_keys: List of keys that should be scrubbed. force_scrub_children: If True, use aggressive scrubbing. scrub_all: If True, scrub every string at every list depth. + separated_keys: Keys that may hold character-separated action text. Returns: Scrubbed item. @@ -370,6 +397,7 @@ def _scrub_list_item( list_keys, scrub_all=scrub_all, force_scrub_children=force_scrub_children, + separated_keys=separated_keys, ) if isinstance(item, list): return [ @@ -380,6 +408,7 @@ def _scrub_list_item( list_keys, scrub_all=scrub_all, force_scrub_children=force_scrub_children, + separated_keys=separated_keys, ) if self._should_scrub_list_item( nested_item, @@ -391,7 +420,7 @@ def _scrub_list_item( ) for nested_item in item ] - return self._scrub_text_item(item, key) + return self._scrub_text_item(item, key, separated_keys=separated_keys) class ScrubbingProviderFactory: diff --git a/openadapt_privacy/config.py b/openadapt_privacy/config.py index 3f33492..c56a2ea 100644 --- a/openadapt_privacy/config.py +++ b/openadapt_privacy/config.py @@ -67,6 +67,10 @@ class PrivacyConfig: SCRUB_LANGUAGE: Language code for NLP analysis (default: "en"). SCRUB_FILL_COLOR: BGR color value for image redaction (default: blue 0x0000FF). SCRUB_KEYS_HTML: List of dict keys that should be scrubbed. + SCRUB_KEYS_SEPARATED: Dict keys whose values may hold character-separated + action text rather than prose. A value under one of these keys is + reassembled before analysis only when it is actually in separated + form; prose under the same key is scrubbed as prose. ACTION_TEXT_NAME_PREFIX: Prefix for action text names (e.g., "<"). ACTION_TEXT_NAME_SUFFIX: Suffix for action text names (e.g., ">"). ACTION_TEXT_SEP: Separator for action text sequences (e.g., "-"). @@ -103,6 +107,16 @@ class PrivacyConfig: ] ) + # Keys whose values may hold a key sequence joined by ACTION_TEXT_SEP. + # Membership here only permits separated handling; the value's own shape + # decides whether it is applied. + SCRUB_KEYS_SEPARATED: list[str] = field( + default_factory=lambda: [ + "text", + "canonical_text", + ] + ) + # Action text formatting (for handling separated text like key sequences) ACTION_TEXT_NAME_PREFIX: str = "<" ACTION_TEXT_NAME_SUFFIX: str = ">" diff --git a/openadapt_privacy/pipelines/dicts.py b/openadapt_privacy/pipelines/dicts.py index 7219744..9973c32 100644 --- a/openadapt_privacy/pipelines/dicts.py +++ b/openadapt_privacy/pipelines/dicts.py @@ -50,6 +50,7 @@ def scrub_dict( scrubber: ScrubbingProvider, list_keys: list[str] | None = None, scrub_all: bool = False, + separated_keys: list[str] | None = None, ) -> dict[str, Any]: """Scrub PII/PHI from a nested dictionary. @@ -61,6 +62,10 @@ def scrub_dict( list_keys: List of keys whose values should be scrubbed. Defaults to config.SCRUB_KEYS_HTML. scrub_all: If True, scrub all string values regardless of key. + separated_keys: Keys that may hold character-separated action text + (a key sequence joined by ACTION_TEXT_SEP). Defaults to + config.SCRUB_KEYS_SEPARATED. Pass an empty list to scrub every + value as prose. Returns: Scrubbed dictionary with PII/PHI removed. @@ -75,13 +80,19 @@ def scrub_dict( >>> scrubbed = scrub_dict(event, scrubber) """ helper = DictScrubber(scrubber) - return helper.scrub_dict(input_dict, list_keys=list_keys, scrub_all=scrub_all) + return helper.scrub_dict( + input_dict, + list_keys=list_keys, + scrub_all=scrub_all, + separated_keys=separated_keys, + ) def scrub_list_dicts( input_list: list[dict[str, Any]], scrubber: ScrubbingProvider, list_keys: list[str] | None = None, + separated_keys: list[str] | None = None, ) -> list[dict[str, Any]]: """Scrub PII/PHI from a list of dictionaries. @@ -91,6 +102,7 @@ def scrub_list_dicts( input_list: List of dictionaries to be scrubbed. scrubber: The ScrubbingProvider to use for text scrubbing. list_keys: List of keys whose values should be scrubbed. + separated_keys: Keys that may hold character-separated action text. Returns: List of scrubbed dictionaries. @@ -105,4 +117,8 @@ def scrub_list_dicts( >>> scrubbed = scrub_list_dicts(events, scrubber) """ helper = DictScrubber(scrubber) - return helper.scrub_list_dicts(input_list, list_keys=list_keys) + return helper.scrub_list_dicts( + input_list, + list_keys=list_keys, + separated_keys=separated_keys, + ) diff --git a/openadapt_privacy/providers/presidio.py b/openadapt_privacy/providers/presidio.py index 5f02ad1..afb1dcf 100644 --- a/openadapt_privacy/providers/presidio.py +++ b/openadapt_privacy/providers/presidio.py @@ -37,6 +37,24 @@ } ) +# Entity types that reach us from the spaCy pipeline. Their spans are model +# output: the model picks the boundary, so a span that overlaps a deterministic +# identifier match is the model reading an identifier plus the words near it. +_STATISTICAL_ENTITIES = frozenset( + { + "DATE_TIME", + "LOCATION", + "NRP", + "ORGANIZATION", + "PERSON", + } +) + +# Presidio scores an unvalidated, context-free digit run at 0.01-0.05. Below +# this floor a pattern recognizer is guessing too, and must not take a label +# away from the NER result. +_MIN_DETERMINISTIC_SCORE = 0.4 + class PrivacyModelUnavailable(RuntimeError): """The required, allowlisted local NLP model is unavailable or invalid.""" @@ -142,6 +160,70 @@ def _filter_automation_false_positives(text: str, analyzer_results: list) -> lis return filtered +def _covers_every_digit(text: str, result, deterministic: list) -> bool: + """Report whether pattern matches claim every digit inside `result`'s span. + + Returns False when nothing overlaps, so a statistical result that stands + alone is never touched. + """ + covered: set[int] = set() + overlaps = False + for other in deterministic: + if result.start < other.end and other.start < result.end: + overlaps = True + covered.update(range(max(result.start, other.start), min(result.end, other.end))) + if not overlaps: + return False + return not any( + text[index].isdigit() for index in range(result.start, result.end) if index not in covered + ) + + +def _prefer_deterministic_identifier_labels(text: str, analyzer_results: list) -> list: + """Give an overlapped span the label of the recognizer that validated it. + + Presidio's anonymizer settles an overlap by span before score, so a wide + spaCy `DATE_TIME` at 0.85 replaced a Luhn-checked `CREDIT_CARD` at 1.0 and + the output named the wrong entity type. A card number that reads as + `` is still redacted, but no policy can route on it. + + The statistical result is dropped only when the pattern matches cover every + digit it claimed, so this can never expose part of an identifier: where the + NER span reaches past the pattern match into more digits, the wider span + stays. + """ + deterministic = [ + result + for result in analyzer_results + if result.entity_type not in _STATISTICAL_ENTITIES + and result.score >= _MIN_DETERMINISTIC_SCORE + ] + if not deterministic: + return analyzer_results + + return [ + result + for result in analyzer_results + if not ( + result.entity_type in _STATISTICAL_ENTITIES + and _covers_every_digit(text, result, deterministic) + ) + ] + + +def _is_separated_form(text: str, separator: str) -> bool: + """Report whether `text` is a key sequence rather than prose. + + Action text is one typed character per separator: `ACTION_TEXT_SEP.join( + "john")` gives `j-o-h-n`. Prose that holds a separator ("follow-up", + "555-123-4567") has chunks longer than one character, so it is prose. + """ + if not separator or separator not in text: + return False + chunks = text.split(separator) + return len(chunks) >= 2 and all(len(chunk) <= 1 for chunk in chunks) + + def _get_analyzer_engine(): """Get or create the Presidio analyzer engine (lazy initialization).""" global _analyzer_engine, _scrubbing_entities @@ -246,8 +328,10 @@ def scrub_text(self, text: str, is_separated: bool = False) -> str: Args: text: Text to be scrubbed. - is_separated: Whether the text contains separated characters - (e.g., key sequences like "a-b-c"). + is_separated: Permit key-sequence handling (e.g. "a-b-c"). The + text is reassembled and re-separated only when it is actually + in that form; prose passes through unchanged, because splitting + prose one character at a time destroys it. Returns: Scrubbed text with PII/PHI replaced by entity type placeholders. @@ -261,11 +345,15 @@ def scrub_text(self, text: str, is_separated: bool = False) -> str: entities = _get_scrubbing_entities() # Handle separated text (e.g., key sequences) - original_text = text - if is_separated and not ( - text.startswith(policy.ACTION_TEXT_NAME_PREFIX) - or text.endswith(policy.ACTION_TEXT_NAME_SUFFIX) - ): + separated = ( + is_separated + and not ( + text.startswith(policy.ACTION_TEXT_NAME_PREFIX) + or text.endswith(policy.ACTION_TEXT_NAME_SUFFIX) + ) + and _is_separated_form(text, policy.ACTION_TEXT_SEP) + ) + if separated: text = "".join(text.split(policy.ACTION_TEXT_SEP)) # Analyze and anonymize @@ -275,6 +363,7 @@ def scrub_text(self, text: str, is_separated: bool = False) -> str: language=policy.SCRUB_LANGUAGE, ) analyzer_results = _filter_automation_false_positives(text, analyzer_results) + analyzer_results = _prefer_deterministic_identifier_labels(text, analyzer_results) logger.debug(f"analyzer_results: {analyzer_results}") @@ -288,10 +377,7 @@ def scrub_text(self, text: str, is_separated: bool = False) -> str: result_text = anonymized_results.text # Restore separator format if needed - if is_separated and not ( - original_text.startswith(policy.ACTION_TEXT_NAME_PREFIX) - or original_text.endswith(policy.ACTION_TEXT_NAME_SUFFIX) - ): + if separated: result_text = policy.ACTION_TEXT_SEP.join(result_text) return result_text diff --git a/tests/test_phi_recall.py b/tests/test_phi_recall.py index 02ac292..53892db 100644 --- a/tests/test_phi_recall.py +++ b/tests/test_phi_recall.py @@ -1,4 +1,8 @@ -"""Quantitative synthetic PHI recall and clean-text regression gate.""" +"""Quantitative synthetic PHI recall and clean-text regression gate. + +Recall alone is not enough. A caller that routes on the entity type needs the +type to be right, so every case below pins the placeholder it must produce. +""" from __future__ import annotations @@ -20,47 +24,64 @@ pytest.skip("Presidio dependencies not installed", allow_module_level=True) +# (category, text, identifier that must disappear, entity type that must fire) PHI_CASES = [ - ("person", "Patient John Smith is ready.", "John Smith"), - ("person", "Schedule Amelia Earhart for follow-up.", "Amelia Earhart"), - ("person", "The chart belongs to Aisha Rahman.", "Aisha Rahman"), - ("person", "Emergency contact is José Alvarez.", "José Alvarez"), - ("person", "Seen by physician Chidi Okafor today.", "Chidi Okafor"), - ("person", "Send the referral to Mei Lin.", "Mei Lin"), - ("person", "Patient Olivia O'Connor arrived.", "Olivia O'Connor"), - ("person", "Discuss results with François Dupont.", "François Dupont"), - ("email", "Email jane.doe@example.com with results.", "jane.doe@example.com"), + ("person", "Patient John Smith is ready.", "John Smith", "PERSON"), + ("person", "Schedule Amelia Earhart for follow-up.", "Amelia Earhart", "PERSON"), + ("person", "The chart belongs to Aisha Rahman.", "Aisha Rahman", "PERSON"), + ("person", "Emergency contact is José Alvarez.", "José Alvarez", "PERSON"), + ("person", "Seen by physician Chidi Okafor today.", "Chidi Okafor", "PERSON"), + ("person", "Send the referral to Mei Lin.", "Mei Lin", "PERSON"), + ("person", "Patient Olivia O'Connor arrived.", "Olivia O'Connor", "PERSON"), + ("person", "Discuss results with François Dupont.", "François Dupont", "PERSON"), + ("email", "Email jane.doe@example.com with results.", "jane.doe@example.com", "EMAIL_ADDRESS"), ( "email", "Portal contact: care-team+east@clinic.example.org", "care-team+east@clinic.example.org", + "EMAIL_ADDRESS", ), - ("phone", "Call the patient at 555-123-4567.", "555-123-4567"), - ("phone", "Mobile: +1 (416) 555-0199", "+1 (416) 555-0199"), - ("ssn", "Social security number 923-45-6789.", "923-45-6789"), + ("phone", "Call the patient at 555-123-4567.", "555-123-4567", "PHONE_NUMBER"), + ("phone", "Mobile: +1 (416) 555-0199", "+1 (416) 555-0199", "PHONE_NUMBER"), + ("ssn", "Social security number 923-45-6789.", "923-45-6789", "US_SSN"), ( "credit_card", - "Card 4532-1234-5678-9012 is on file.", - "4532-1234-5678-9012", + "Card 4532-1234-5678-9014 is on file.", + "4532-1234-5678-9014", + "CREDIT_CARD", ), - ("dob", "Date of birth: 01/15/1985.", "01/15/1985"), - ("dob", "DOB is January 5, 1974.", "January 5, 1974"), + ( + "credit_card", + "Card 4111111111111111 on file", + "4111111111111111", + "CREDIT_CARD", + ), + ( + "bank_account", + "My bank account number is 635526789012.", + "635526789012", + "US_BANK_NUMBER", + ), + ("dob", "Date of birth: 01/15/1985.", "01/15/1985", "DATE_TIME"), + ("dob", "DOB is January 5, 1974.", "January 5, 1974", "DATE_TIME"), ( "address", "Home address: 123 Main Street, Boston, MA 02110.", "123 Main Street", + "STREET_ADDRESS", ), - ("address", "Mail to 88 King St W, Toronto, ON M5H 1J9.", "88 King St W"), - ("ip", "Last login from 192.168.10.22.", "192.168.10.22"), + ("address", "Mail to 88 King St W, Toronto, ON M5H 1J9.", "88 King St W", "STREET_ADDRESS"), + ("ip", "Last login from 192.168.10.22.", "192.168.10.22", "IP_ADDRESS"), ( "url", "Portal is https://patient.example.org/chart/42", "https://patient.example.org/chart/42", + "URL", ), - ("mrn", "Medical record number MRN: 00123456.", "00123456"), - ("mrn", "Patient ID: AB-902771.", "AB-902771"), - ("member_id", "Health plan member ID ZXQ-443-991.", "ZXQ-443-991"), - ("license", "Provider license number ME123456.", "ME123456"), + ("mrn", "Medical record number MRN: 00123456.", "00123456", "MEDICAL_RECORD_NUMBER"), + ("mrn", "Patient ID: AB-902771.", "AB-902771", "MEDICAL_RECORD_NUMBER"), + ("member_id", "Health plan member ID ZXQ-443-991.", "ZXQ-443-991", "MEDICAL_RECORD_NUMBER"), + ("license", "Provider license number ME123456.", "ME123456", "US_DRIVER_LICENSE"), ] CLEAN_CASES = [ @@ -83,7 +104,7 @@ def scrubber() -> PresidioScrubbingProvider: def test_synthetic_phi_identifier_recall_is_complete(scrubber) -> None: misses = [] - for category, text, identifier in PHI_CASES: + for category, text, identifier, _entity in PHI_CASES: scrubbed = scrubber.scrub_text(text) if identifier in scrubbed: misses.append((category, identifier, scrubbed)) @@ -95,6 +116,84 @@ def test_synthetic_phi_identifier_recall_is_complete(scrubber) -> None: ) +def test_synthetic_phi_entity_labels_are_correct(scrubber) -> None: + """The placeholder must name the entity a policy would route on. + + Asserting only that the identifier disappeared hid a systematic + mislabelling: a card number came back as ````. + """ + wrong = [] + for category, text, _identifier, entity in PHI_CASES: + scrubbed = scrubber.scrub_text(text) + if f"<{entity}>" not in scrubbed: + wrong.append((category, entity, scrubbed)) + + assert not wrong, f"{len(wrong)}/{len(PHI_CASES)} cases carry the wrong entity type: {wrong}" + + @pytest.mark.parametrize("text", CLEAN_CASES) def test_clean_operational_text_is_not_redacted(scrubber, text: str) -> None: assert scrubber.scrub_text(text) == text + + +def test_a_card_number_that_fails_the_luhn_check_is_still_redacted(scrubber) -> None: + """Documented limit: no checksum, no ``CREDIT_CARD`` label. + + ``4532-1234-5678-9012`` is not a valid card number, so Presidio's card + recognizer invalidates it. Something else claims the span, and the digits + still leave the output, but the type is not ``CREDIT_CARD``. + """ + scrubbed = scrubber.scrub_text("Card: 4532-1234-5678-9012") + + assert "4532-1234-5678-9012" not in scrubbed + assert "" not in scrubbed + + +def test_a_wider_ner_span_survives_when_it_holds_digits_the_pattern_missed() -> None: + """Relabelling must never shrink what gets redacted. + + Presidio's driver-license recognizer matches only the leading ``A123`` of + ``A123-456-789-012``. If an overlapping ``ORGANIZATION`` span covering the + whole identifier were dropped in favour of that partial match, the trailing + digits would leave the scrubber unredacted. + """ + from presidio_analyzer import RecognizerResult + + from openadapt_privacy.providers.presidio import _prefer_deterministic_identifier_labels + + text = "A123-456-789-012" + partial = RecognizerResult(entity_type="US_DRIVER_LICENSE", start=0, end=4, score=0.65) + whole = RecognizerResult(entity_type="ORGANIZATION", start=0, end=16, score=0.85) + + kept = _prefer_deterministic_identifier_labels(text, [partial, whole]) + + assert whole in kept + + +def test_a_validated_pattern_takes_the_label_from_an_overlapping_ner_span() -> None: + from presidio_analyzer import RecognizerResult + + from openadapt_privacy.providers.presidio import _prefer_deterministic_identifier_labels + + text = "Card 4111111111111111 on file" + card = RecognizerResult(entity_type="CREDIT_CARD", start=5, end=21, score=1.0) + date = RecognizerResult(entity_type="DATE_TIME", start=0, end=21, score=0.85) + + kept = _prefer_deterministic_identifier_labels(text, [card, date]) + + assert kept == [card] + + +def test_a_low_confidence_pattern_does_not_take_a_label() -> None: + """An unvalidated 0.05 digit-run match is a guess, not an identifier.""" + from presidio_analyzer import RecognizerResult + + from openadapt_privacy.providers.presidio import _prefer_deterministic_identifier_labels + + text = "Card 4111111111111111 on file" + weak = RecognizerResult(entity_type="US_BANK_NUMBER", start=5, end=21, score=0.05) + date = RecognizerResult(entity_type="DATE_TIME", start=0, end=21, score=0.85) + + kept = _prefer_deterministic_identifier_labels(text, [weak, date]) + + assert date in kept diff --git a/tests/test_separated_action_text.py b/tests/test_separated_action_text.py new file mode 100644 index 0000000..201e672 --- /dev/null +++ b/tests/test_separated_action_text.py @@ -0,0 +1,88 @@ +"""Separated action text must be decided by the value's shape, not the key name. + +``SCRUB_KEYS_HTML`` contains ``text``, which is also the most common key name in +an arbitrary dict. Treating every ``text`` value as a hyphen-joined key sequence +turned a plain sentence into one character per hyphen. +""" + +from __future__ import annotations + +import pytest + +try: + import spacy + + from openadapt_privacy.config import config + + if not spacy.util.is_package(config.SPACY_MODEL_NAME): + pytest.skip( + f"SpaCy model {config.SPACY_MODEL_NAME} not installed", + allow_module_level=True, + ) + + from openadapt_privacy.pipelines.dicts import scrub_dict + from openadapt_privacy.providers.presidio import PresidioScrubbingProvider +except ImportError: # pragma: no cover - exercised only without the extra + pytest.skip("Presidio dependencies not installed", allow_module_level=True) + + +@pytest.fixture(scope="module") +def scrubber() -> PresidioScrubbingProvider: + return PresidioScrubbingProvider() + + +PROSE_UNDER_TEXT_KEY = "Email: john@example.com" + + +def test_plain_prose_under_the_text_key_is_not_character_separated(scrubber) -> None: + """A sentence under ``text`` comes back as a sentence.""" + result = scrub_dict({"text": PROSE_UNDER_TEXT_KEY}, scrubber)["text"] + + assert "john@example.com" not in result + assert "" in result + assert "-E-M-A-I-L" not in result + assert result.count("-") == PROSE_UNDER_TEXT_KEY.count("-") + + +def test_hyphenated_prose_under_the_text_key_keeps_its_words(scrubber) -> None: + """A single hyphen in ordinary prose does not make the value action text.""" + result = scrub_dict({"text": "Book the follow-up for jane.doe@example.com"}, scrubber)["text"] + + assert "follow-up" in result + assert "" in result + + +def test_separated_action_text_still_round_trips(scrubber) -> None: + """A genuine key sequence is still joined before analysis and re-separated.""" + typed = "-".join("john@example.com") + + result = scrubber.scrub_text(typed, is_separated=True) + + assert "".join(result.split("-")) == "" + + +def test_separated_action_text_under_the_text_key_still_finds_pii(scrubber) -> None: + """The dict path must not lose the separated analysis that hides PII.""" + typed = "-".join("john@example.com") + + result = scrub_dict({"text": typed}, scrubber)["text"] + + assert "john@example.com" not in "".join(result.split("-")) + assert "".join(result.split("-")) == "" + + +def test_separated_handling_can_be_turned_off_per_call(scrubber) -> None: + """``separated_keys`` makes the behaviour explicit instead of implied.""" + typed = "-".join("john@example.com") + + result = scrub_dict({"text": typed}, scrubber, separated_keys=[])["text"] + + assert result == typed + + +def test_a_phone_number_under_the_text_key_survives_as_a_phone_number(scrubber) -> None: + """Hyphen groups of more than one character are not a key sequence.""" + result = scrub_dict({"text": "Call 555-123-4567 now"}, scrubber)["text"] + + assert "555-123-4567" not in result + assert "" in result