Confit hardening PRD: Recognizer configuration conformance suite (#1) - #2248
Confit hardening PRD: Recognizer configuration conformance suite (#1)#2248omri374 wants to merge 9 commits into
Conversation
Adds tests/test_recognizer_config_conformance.py, parametrized over every concrete EntityRecognizer subclass, asserting each constructor accepts the keys RecognizerListLoader injects from a registry YAML entry (name, supported_language, context, and one of supported_entity/supported_entities) -- unless the class is in ENTITIES_FROM_OWN_CONFIG (the LangExtract family, which derives entities from its config_path file instead). KNOWN_CONTRACT_GAPS regression-locks the three constructors that don't yet satisfy the contract, verified against the current signatures: AzureHealthDeidRecognizer, AzureOpenAILangExtractRecognizer and MedicalNERRecognizer are all missing `context`. Each currently crashes registry construction with a TypeError the moment a user enables it in YAML with a context list -- caught here in CI instead. Closing these gaps is turn 02's Story 2, in the next commit. Part of the ADR at https://github.com/data-privacy-stack/presidio-product-core/issues/139. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014cYn8gUDL2txRa5V7r4vLZ
…AILangExtractRecognizer, MedicalNERRecognizer Closes the three constructor-contract gaps the previous commit's conformance test locked in, all missing `context`: - AzureHealthDeidRecognizer: forwards `context` to RemoteRecognizer, which already accepts it. - MedicalNERRecognizer: forwards `context` to HuggingFaceNerRecognizer, which already accepts it. - AzureOpenAILangExtractRecognizer: its immediate base, LangExtractRecognizer, has no `context` parameter and no **kwargs, so it cannot be forwarded through super().__init__(); the value is instead stored directly on the instance after the super() call, matching EntityRecognizer's own "falsy -> []" default. Behavior only newly exists for this class -- passing `context` previously raised TypeError -- so there is no prior behavior to preserve. KNOWN_CONTRACT_GAPS is now empty; the conformance test still passes with every concrete recognizer's constructor accepting the full set of registry-injected keys. Each changed class gets a direct-construction test asserting `context=["x"]` reaches `.context` (test_ahds_recognizer.py, test_medical_ner_recognizer.py, test_azure_openai_langextract_recognizer.py). Also: RecognizerListLoader._prepare_recognizer_kwargs now emits a logger.warning (never logging PII) when a registry entry sets supported_entity/supported_entities for a class that accepts neither -- naming the class and the dropped key -- instead of silently discarding the value, since such a class (e.g. BasicLangExtractRecognizer) defines its entities from its own configuration. No other behavior change: the value was already dropped/ignored before this commit for such classes; this only adds visibility. Covered by test_dropped_entity_key_warns_for_class_defining_its_own_entities and test_no_warning_when_class_accepts_the_entity_key in test_recognizers_loader_utils.py. Part of the ADR at https://github.com/data-privacy-stack/presidio-product-core/issues/139. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014cYn8gUDL2txRa5V7r4vLZ
Adds test_shipped_entry_fields_reach_constructed_recognizer, parametrized over every entry of conf/default_recognizers.yaml: builds a single-recognizer registry config from the entry (enabled forced true, using the file's global_regex_flags), loads it through RecognizerRegistryProvider with load() patched to a no-op on every concrete recognizer class, and asserts one instance per declared language, that each instance's supported_language and name match the entry, that per-language context reaches the instance when the entry sets one, and that supported_entities / score_thresholds reach the instance when the entry sets them (normalized via normalize_score_thresholds). Reuses NOT_LOADABLE_FROM_SHIPPED_ENTRY from test_recognizers_loader_utils.py rather than redefining it, so exactly one such set exists in the suite. Part of the ADR at https://github.com/data-privacy-stack/presidio-product-core/issues/139. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014cYn8gUDL2txRa5V7r4vLZ
Adds test_synthetic_entry_round_trips_to_every_concrete_class, parametrized over every concrete recognizer class: builds a synthetic "conf_<Class>"-named registry entry with per-language context and score_thresholds, loads it through RecognizerRegistryProvider (load() patched to a no-op), and asserts name/supported_language/context/ score_thresholds all reached the constructed instance -- not just that the constructor accepts the keys (Story 1/2 covers that at the signature level). REQUIRED_KWARGS/REQUIRED_ENV supply what a handful of classes need beyond the synthetic entry (a config_path default, an Azure endpoint/credentials env var, HuggingFaceNerRecognizer's model_name). NOT_LOADABLE_AS_PREDEFINED_ ENTRY documents three classes that cannot be built through a `type: predefined` entry at all -- LocalRecognizer and PatternRecognizer (subclassing-only base classes; PatternRecognizer's required `patterns` can only be set on a `type: custom` entry, which the schema enforces) and ZaPhoneNumberRecognizer (requires `target_classification`, positional, with no schema field to set it from). CONTEXT_NOT_APPLIED documents BasicLangExtractRecognizer, whose constructor already accepts `context` but does not apply it -- pre-existing, unchanged behavior this turn. test_unknown_key_is_not_silent asserts a CreznameCardRecognizer... (typo guard) -- a CreditCardRecognizer entry with an unrecognized `no_such_key` either raises ValueError or logs a WARNING naming it. Neither happens today (PredefinedRecognizerConfig silently drops unknown keys), so the test is marked xfail(strict=True, reason="flipped in turn 06 (derived schema)"): it documents today's silent-drop gap and will force removal of the marker the moment turn 06 closes it. Part of the ADR at https://github.com/data-privacy-stack/presidio-product-core/issues/139. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014cYn8gUDL2txRa5V7r4vLZ
Coverage report (presidio-anonymizer)Click to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Coverage report (presidio-cli)Click to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||
Coverage report (presidio-image-redactor)Click to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||||||||
Coverage report (presidio-structured)Click to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||
There was a problem hiding this comment.
🟡 Changes recommended
The new warning in _prepare_recognizer_kwargs can be misleading for subclasses that forward **kwargs to superclasses which do accept/apply supported_entities, causing false-positive “ignoring” warnings.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR adds a comprehensive conformance test suite for Presidio Analyzer’s recognizer registry/YAML configuration layer, ensuring registry-injected keys are accepted by every concrete recognizer and that shipped/synthetic YAML entries actually round-trip key fields into constructed recognizer instances. It also fixes three recognizer constructors to accept the registry-injected context key and introduces a new warning when entity keys are configured for recognizers that define entities outside the registry entry.
Changes:
- Add
tests/test_recognizer_config_conformance.pyto assert constructor contract conformance and configuration field reachability across shipped and synthetic registry entries. - Fix constructor compatibility for
contextinAzureHealthDeidRecognizer,MedicalNERRecognizer, andAzureOpenAILangExtractRecognizer, plus targeted regression tests. - Add a
logger.warningwhensupported_entity/supported_entitiesis configured for a recognizer class that (per loader inference) doesn’t accept those keys.
File summaries
| File | Description |
|---|---|
| presidio-analyzer/tests/test_recognizers_loader_utils.py | Adds tests covering the new warning behavior around dropped entity keys. |
| presidio-analyzer/tests/test_recognizer_config_conformance.py | Introduces the new registry/YAML conformance suite across all concrete recognizers and shipped config entries. |
| presidio-analyzer/tests/test_medical_ner_recognizer.py | Adds regression test ensuring context reaches MedicalNERRecognizer instances. |
| presidio-analyzer/tests/test_azure_openai_langextract_recognizer.py | Adds regression test ensuring context reaches AzureOpenAILangExtractRecognizer instances. |
| presidio-analyzer/tests/test_ahds_recognizer.py | Adds regression test ensuring context reaches AzureHealthDeidRecognizer instances. |
| presidio-analyzer/presidio_analyzer/recognizer_registry/recognizers_loader_utils.py | Adds warning when entity keys are configured for recognizers inferred not to accept them. |
| presidio-analyzer/presidio_analyzer/predefined_recognizers/third_party/azure_openai_langextract_recognizer.py | Accepts context and stores it on the instance (cannot be forwarded to base). |
| presidio-analyzer/presidio_analyzer/predefined_recognizers/third_party/ahds_recognizer.py | Adds context parameter and forwards it to RemoteRecognizer. |
| presidio-analyzer/presidio_analyzer/predefined_recognizers/ner/medical_ner_recognizer.py | Adds context parameter and forwards it to HuggingFaceNerRecognizer. |
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Coverage report (presidio-analyzer)Click to see where and how coverage changed
The report is truncated to 25 files out of 81. To see the full report, please visit the workflow summary page. This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
…vert constructor changes Reverts the `context` kwarg added to AzureHealthDeidRecognizer, AzureOpenAILangExtractRecognizer and MedicalNERRecognizer. `context` is one flat word list applied to every result a recognizer emits, which only makes sense for single-entity recognizers; multi-entity recognizers (NER models, remote PHI services, LLM extractors) deliberately do not accept it. The registry-build crash is fixed in the loader instead: RecognizerListLoader._prepare_recognizer_kwargs drops `context` when the class signature does not accept it and logs a WARNING naming the class and the key, so a registry entry with context for such a class loads instead of raising TypeError. The conformance contract is `name`, `supported_language` and an entity key; `context` is no longer required. The per-class round-trip test asserts that a class which does not accept context loads with the base default `[]` and that the warning is logged, and that a class which accepts it receives it with no warning. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
_prepare_recognizer_kwargs's new WARNING (added in the previous commit) checked only the leaf __init__ signature, so a class that forwards **kwargs to a base class which does declare supported_entity(ies) (e.g. TransformersRecognizer/StanzaRecognizer forwarding to SpacyRecognizer) was reported as "ignoring" a value that a base class actually applies. Walk the **kwargs-forwarding MRO chain (mirroring the conformance suite's own reachability check) and only warn when the key is unreachable anywhere in that chain. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014cYn8gUDL2txRa5V7r4vLZ
…meters The dropped-key warnings in RecognizerListLoader._prepare_recognizer_kwargs keyed off the leaf __init__ signature only. A subclass that forwards **kwargs to a parent that accepts the key (StanzaRecognizer and TransformersRecognizer via SpacyRecognizer) was reported as "ignoring supported_entities" while the key was in fact applied, and the new context rule let context pass through **kwargs to a parent that does not accept it. Adds RecognizerListLoader._reachable_init_param_names, the union of constructor parameter names along the MRO stopping at the first __init__ without **kwargs, and bases both warnings and the context drop on that set. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
There was a problem hiding this comment.
🟢 Approval recommended
The changes are additive and improve config-layer robustness with tests, with only minor wording/maintainability follow-ups noted in review comments.
Review details
Suppressed comments (3)
Previously missed (1) — in code that hasn't changed since the last review.
presidio-analyzer/tests/test_recognizer_config_conformance.py:460
- These tests hardcode
global_regex_flagsto 26 even though this module already importsGLOBAL_REGEX_FLAGSfrom the shippeddefault_recognizers.yaml. Using the constant makes the conformance suite resilient if the shipped default flags ever change.
This issue also appears on line 524 of the same file.
presidio-analyzer/presidio_analyzer/recognizer_registry/recognizers_loader_utils.py:322
- The docstring says the entity key is “dropped”, but for classes which accept
**kwargs(e.g. LangExtract-based recognizers)_prepare_recognizer_kwargs()still returns the key inkwargs; the key is effectively ignored by the recognizer, not necessarily removed here. Adjusting the wording avoids misleading future maintainers about what this function actually does.
This function adapts supported_entity/supported_entities based on the
recognizer class __init__ signature to avoid passing unexpected kwargs.
- If recognizer accepts only supported_entity (singular), convert
presidio-analyzer/tests/test_recognizer_config_conformance.py:527
- Same as above: this test also hardcodes
global_regex_flagsto 26. UsingGLOBAL_REGEX_FLAGSkeeps the test aligned with the shipped configuration contract it’s validating.
configuration = {
"global_regex_flags": 26,
"supported_languages": ["en"],
"recognizers": [
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
- Reword the entity-key WARNING from "does not accept" to "does not apply", since the key can still be technically accepted via **kwargs while never being consumed by any class in the MRO chain (that's exactly the case this warning now targets). - Clarify _prepare_recognizer_kwargs's docstring: a key can remain in the returned kwargs while still being effectively ignored by the constructed recognizer. - Use the already-imported GLOBAL_REGEX_FLAGS constant instead of a hardcoded 26 in two conformance-suite test configurations. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014cYn8gUDL2txRa5V7r4vLZ
There was a problem hiding this comment.
🟡 Changes recommended
A few user-facing warning/doc/test messages are currently inconsistent with actual kwarg acceptance semantics and should be corrected for accuracy and long-term maintainability.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (3)
Previously missed (1) — in code that hasn't changed since the last review.
presidio-analyzer/tests/test_recognizer_config_conformance.py:460
- These conformance tests already import GLOBAL_REGEX_FLAGS from the existing loader-utils test module; hardcoding 26 here makes the suite brittle if the project’s default regex flags change. Prefer using the shared constant for consistency.
This issue also appears on line 524 of the same file.
presidio-analyzer/presidio_analyzer/recognizer_registry/recognizers_loader_utils.py:412
- The WARNING message claims the class "does not accept" the entity-key kwargs, but some recognizers (e.g. BasicLangExtractRecognizer) accept arbitrary **kwargs and will therefore accept these keys even though they are ignored. Rewording this warning to describe the real contract (the keys have no effect) avoids confusing users who see the warning and then inspect the constructor signature.
RecognizerListLoader.SUPPORTED_ENTITIES,
)
if key in kwargs
]
if dropped_keys:
presidio-analyzer/tests/test_recognizer_config_conformance.py:526
- Same as above: prefer GLOBAL_REGEX_FLAGS over the hardcoded 26 to avoid test drift if the repo changes its default regex flags.
configuration = {
"global_regex_flags": GLOBAL_REGEX_FLAGS,
"supported_languages": ["en"],
- Files reviewed: 3/3 changed files
- Comments generated: 3
- Review effort level: Lite
Docstrings and a test assertion/failure message still said the entity key is "dropped" or that the class "accepts neither" key, which is only true for a strict-signature class -- a **kwargs-accepting class (e.g. BasicLangExtractRecognizer) keeps the key in the prepared kwargs and simply never applies it. Reworded to "unreachable"/"has no effect" so the docs and failure output match what the code actually does. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014cYn8gUDL2txRa5V7r4vLZ
There was a problem hiding this comment.
🟢 Approval recommended
The changes are well-scoped with strong regression coverage, and the only remaining feedback is minor docstring wording alignment (no functional blockers).
Review details
Suppressed comments (2)
presidio-analyzer/presidio_analyzer/recognizer_registry/recognizers_loader_utils.py:340
- The _prepare_recognizer_kwargs docstring says the entity key is "dropped", but the implementation only logs a warning and may still pass the key through (e.g., for classes accepting **kwargs). Rewording this avoids implying the kwarg is always removed from the constructor call.
If the key is unreachable anywhere in the class's constructor chain (it
defines its supported entities from its own configuration, e.g. a
LangExtract config file) and the entry set one anyway, a
``logger.warning`` names the class and the key that has no effect,
presidio-analyzer/tests/test_recognizers_loader_utils.py:294
- This test’s docstring says the key is “dropped” / “silently discarding the value”, but the expected behavior (and the assertion below) is that the kwarg may still be passed (via **kwargs) and is simply not applied by the recognizer. Updating the docstring makes the test’s intent match the behavior it verifies.
"""A class that has neither supported_entity nor supported_entities
reachable anywhere in its constructor chain (it defines its entities from
its own configuration, e.g. a LangExtract config file) still loads when
the entry sets supported_entities -- but a WARNING naming the class and
the ineffective key is logged instead of staying silent about it.
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
The new shipped-entry reachability test currently skips assertions when supported_entities is explicitly set to an empty list, leaving a correctness gap in the conformance suite.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
presidio-analyzer/tests/test_recognizer_config_conformance.py:325
supported_entitiesassertions are skipped when the shipped YAML explicitly sets an empty list (e.g.supported_entities: []), because the check uses truthiness (if entry.get("supported_entities")). If an empty list is a meaningful configured value, this test would incorrectly treat it as “not set” and miss regressions. Consider checking for key presence /is not Noneinstead so empty lists are still asserted.
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Lite
Change Description
Adds
tests/test_recognizer_config_conformance.py, the conformance suite for the recognizer registry / YAML configuration layer (ADR module M0): a parametrized check that every concrete recognizer's constructor accepts the keysRecognizerListLoaderinjects from a registry YAML entry (name,supported_language,context, and one ofsupported_entity/supported_entities), a field-reach test over every entry of the shippedconf/default_recognizers.yaml, and a per-class round-trip test that builds a synthetic registry entry for every concrete recognizer class and assertsname/context/score_thresholdsactually reach the constructed instance. It also fixes, in the loader, the registry-build crash the first of these tests found (acontextentry on a recognizer whose constructor does not accept it), and addslogger.warningcalls for two previously-silent cases the tests surfaced.Behavior changes
None to detection results or scores. Two new
logger.warninglines, and oneTypeErrorthat becomes a warning:RecognizerListLoader._prepare_recognizer_kwargsnow dropscontextwhen the recognizer class does not accept it, and logs a WARNING naming the class and the key. Before this PR the same entry raisedTypeError: __init__() got an unexpected keyword argument 'context'and the whole registry failed to build. Nothing changes for classes that acceptcontext.supported_entity/supported_entitiesfor a class whose constructor accepts neither (e.g.BasicLangExtractRecognizer, which derives its entities from its own config file). That case now logs a WARNING naming the class and the dropped key instead of staying silent.No entity or PII values are logged, only class names and key names. No constructor signature changes.
Why
contextis handled in the loader, not the constructorsEntityRecognizer.contextis one flat list of context words applied to every result the recognizer emits. That is meaningful for a recognizer that detects a single entity type (the pattern recognizers), and meaningless for one that detects several:MedicalNERRecognizer,AzureHealthDeidRecognizerand the LangExtract recognizers would boost a DATE as much as a NAME. Those classes deliberately do not acceptcontext, and this PR does not add it to them.What was broken is that a registry entry setting
contextfor such a class crashed the whole registry build. The fix is one rule in the loader: acontextkey the class does not accept is dropped with a warning naming the class. The conformance contract is thereforename,supported_languageand an entity key;contextis not part of it.What's in each commit / story
test_recognizer_accepts_registry_injected_keys, parametrized over every concreteEntityRecognizersubclass, walking each constructor's__init__MRO to compute which registry-injected keys are reachable.KNOWN_CONTRACT_GAPSregression-locks the three gaps found (allcontext);ENTITIES_FROM_OWN_CONFIGdocuments the LangExtract family's legitimate exemption from the entity-key requirement.contextrule described above plus its two tests (test_context_dropped_with_warning_for_class_not_accepting_it,test_context_kept_without_warning_for_class_accepting_it), and the dropped-entity-key warning plus its two tests (test_dropped_entity_key_warns_for_class_defining_its_own_entities,test_no_warning_when_class_accepts_the_entity_key), all intest_recognizers_loader_utils.py.KNOWN_CONTRACT_GAPSis empty: every concrete recognizer already meets thename/supported_language/ entity-key contract.test_shipped_entry_fields_reach_constructed_recognizer, parametrized over every entry ofconf/default_recognizers.yaml: loads each throughRecognizerRegistryProviderand asserts language count,supported_language, per-languagecontext,name, and (when set)supported_entities/score_thresholdsall reach the constructed instance(s). ReusesNOT_LOADABLE_FROM_SHIPPED_ENTRYfromtests/test_recognizers_loader_utils.py(imported, not duplicated).test_synthetic_entry_round_trips_to_every_concrete_class, parametrized over every concrete recognizer class with a syntheticconf_<Class>entry;REQUIRED_KWARGS/REQUIRED_ENVsupply what a handful of classes need to construct (an Azure endpoint/credentials,HuggingFaceNerRecognizer'smodel_name);NOT_LOADABLE_AS_PREDEFINED_ENTRYdocuments three classes that structurally cannot be built through atype: predefinedentry at all (LocalRecognizer,PatternRecognizer,ZaPhoneNumberRecognizer— all subclassing-only base classes); for a class that does not acceptcontextthe test asserts the entry still loads, the instance keeps the base default[], and the loader WARNING names the class;CONTEXT_NOT_APPLIEDdocumentsBasicLangExtractRecognizer's pre-existing (unchanged) "accepts but doesn't applycontext" behavior.test_unknown_key_is_not_silentdocuments today's silent-drop gap for an unrecognized YAML key and is markedxfail(strict=True, reason="flipped in turn 06 (derived schema)")— it is expected to fail today; that is the point, and it will force the marker's removal the moment turn 06 closes the gap.Notes / known limitations
Commit
a2106deaddedcontextto three constructors; commit45c8a3breverts that and moves the fix into the loader, per review. The net diff contains no constructor changes.Commits
7cd44e3and31fa9b0address the Copilot review comment: both loader warnings and thecontextdrop now key offRecognizerListLoader._reachable_init_param_names(constructor parameters reachable through**kwargsforwarding along the MRO), soStanzaRecognizerandTransformersRecognizer, which forward toSpacyRecognizer, are not reported as ignoring keys they apply.uv run ruff format --check .flags 26 pre-existing files unrelated to this PR (confirmed identical onorigin/mainbefore this branch's changes); the actual CI lint gate (.github/workflows/ci.yml) only runsruff check, which is fully green. No file touched by this PR needs reformatting.The full local test run has pre-existing failures unrelated to this PR: this sandbox's network egress blocks
huggingface.co, so tests that need a real model download (test_gliner_recognizer.py,test_stanza_*,test_transformers_recognizer.py, the tokenizer-chunker tests intest_recognizer_registry_provider.py, onetest_analyzer_engine_provider.pycase) fail with aProxyError/403, confirmed identical onorigin/mainbefore this branch's changes. Every test this PR added or touched passes.Issue reference
Part of the ADR at https://github.com/data-privacy-stack/presidio-product-core/issues/139.
Turn 02 of 6. Turn 01 is PR #2210.
Checklist
🤖 Generated with Claude Code
https://claude.ai/code/session_014cYn8gUDL2txRa5V7r4vLZ
Generated by Claude Code