…#2248)
* test(analyzer): add base-class contract conformance test
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
data-privacy-stack/presidio-product-core#139.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014cYn8gUDL2txRa5V7r4vLZ
* fix(analyzer): accept context in AzureHealthDeidRecognizer, AzureOpenAILangExtractRecognizer, 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
data-privacy-stack/presidio-product-core#139.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014cYn8gUDL2txRa5V7r4vLZ
* test(analyzer): add shipped configuration field-reach test
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
data-privacy-stack/presidio-product-core#139.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014cYn8gUDL2txRa5V7r4vLZ
* test(analyzer): add per-class round-trip and no-silent-drop tests
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
data-privacy-stack/presidio-product-core#139.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014cYn8gUDL2txRa5V7r4vLZ
* fix(analyzer): drop context for recognizers that do not accept it, revert 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>
* fix(analyzer): gate dropped-entity-key warning on MRO reachability
_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
* fix(analyzer): base loader warnings on MRO-reachable constructor parameters
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>
* fix(analyzer): address review nits on dropped-key warning wording
- 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
* fix(analyzer): clarify dropped-vs-ineffective entity-key wording
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
* fix(analyzer): assert empty supported_entities, rename ineffective-key wording
Addresses the remaining Copilot review comments.
- test_shipped_entry_fields_reach_constructed_recognizer gated the
supported_entities assertion on truthiness, so an entry that explicitly
set `supported_entities: []` was treated as "not set" and skipped. Gate
on key presence instead, and give the assertion a failure message.
- The entity-key branch of _prepare_recognizer_kwargs warns but does not
remove the key (a class accepting **kwargs still receives it). Rename
`dropped_keys` to `ineffective_keys`, reword the comment, and rename
test_dropped_entity_key_warns_for_class_defining_its_own_entities to
test_ineffective_entity_key_warns_for_class_defining_its_own_entities
so the names match what the code and the assertions actually do.
No behavior change: no shipped entry sets supported_entities, and the
warning and kwargs contents are unchanged.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(analyzer): skip conformance cases missing optional extras, keep context for **kwargs leaves
Addresses the second Copilot review round.
- The conformance suite hard-failed with ImportError on a core/dev install
without `--all-extras`: six recognizer classes import transformers, the
Azure SDKs or langextract in their constructor. Add
OPTIONAL_DEPENDENCY_MODULES and skip those parametrized cases when the
module is absent. The skip probes for the module rather than catching the
exception, so an unexpected ImportError from any class still fails.
Verified with the modules hidden: the 7 previously failing cases now skip.
- _prepare_recognizer_kwargs dropped `context` for any class that could not
reach a `context` parameter, including leaf constructors accepting
**kwargs, which could previously read it straight out of kwargs. That was
a silent, backward-incompatible loss for out-of-tree recognizers. Scope
the drop to strict leaf signatures, the only ones that raise TypeError on
an unexpected keyword. All three in-repo classes that fail the
reachability check have strict signatures, so the shipped set is
unchanged. Regression test added.
- _entry_language_configs did not mirror the loader on the
no-supported_languages path: it returned None instead of the entry-level
`context`, so those 11 shipped entries would stop being checked if one
ever set the key. It now carries the entry context, and rejects an empty
language list explicitly instead of treating it as omitted.
- Document, with a test, that entry-level `context` is silently discarded
when supported_languages is a bare list of codes. The test pins today's
behavior rather than changing the loader, which would alter detection.
- Correct the comment claiming this branch leaves the entity key in kwargs:
`supported_entity` is always filtered out later when not declared.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(analyzer): revert unsound context **kwargs carve-out; probe torch separately
Three Copilot findings, verified before fixing:
- The has_var_kw carve-out that kept `context` in kwargs for a leaf
constructor accepting **kwargs was unsound and could crash registry
construction, exactly the failure mode this whole rule exists to
prevent. _reachable_init_param_names already assumes **kwargs is
forwarded up the MRO while computing reachability, so "unreachable"
means some class in that forwarding chain has no **kwargs of its own
and does not declare `context` -- e.g. the existing
ChildForwardsKwargs -> StrictParent test double, which really does
raise TypeError when constructed with `context` left in kwargs
(reproduced directly: `ChildForwardsKwargs(context=["zeta"])` raises
"unexpected keyword argument 'context'"). Reverted to unconditionally
dropping `context` when unreachable, regardless of the leaf's own
**kwargs. Rewrote the regression test to construct the class (not
just inspect the prepared dict) so this class of bug fails loudly.
- OPTIONAL_DEPENDENCY_MODULES mapped HuggingFaceNerRecognizer and
MedicalNERRecognizer to only "transformers", but both constructors
also require torch, which the transformers extra does not install.
A partial environment (transformers without torch) would reach the
constructor and fail instead of skipping. Now probes a tuple of
modules per class. Verified with only torch hidden (transformers
left importable): both cases now skip instead of failing.
- test_entry_context_is_dropped_for_bare_language_list asserted
`instance.context != entry_context`, which passes for any wrong
value, not just the documented drop. Asserts the exact expected
default (CreditCardRecognizer's own built-in context list, not an
empty list -- verified directly) instead.
Copilot's third finding on this pass (LocalRecognizer supposedly
breaking test_not_loadable_as_predefined_entry_names_only_real_classes
via inspect.isabstract) does not reproduce: LocalRecognizer implements
its inherited abstract methods and inspect.isabstract returns False
for it, confirmed directly and via the passing guard test -- no change
needed there.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014cYn8gUDL2txRa5V7r4vLZ
* fix(analyzer): base entity-key conversion/filter on reachable params, not leaf signature
Fourth Copilot review round, both posted findings verified by reproduction
before fixing; the two remaining findings were pre-existing test-robustness
gaps in the same optional-dependency-skip mechanism just added.
- The plural->singular normalize step (Story unrelated, pre-existing code)
and the singular-drop safety filter both checked only the leaf __init__
signature, not the **kwargs-forwarding reachable set the entity-key
WARNING already uses. A leaf accepting **kwargs and forwarding to a
strict parent that declares only `supported_entity` therefore kept the
unconverted `supported_entities` (leaf has **kwargs, so the plural-kept
filter let it through) and crashed there. Reproduced directly:
`ChildForwardsKwargsToSingularParent(supported_entities=["PERSON"])`
raised "unexpected keyword argument 'supported_entities'". Both steps
now key off `_reachable_init_param_names`, matching what the warning
logic already does; the plural-kept-on-**kwargs filter is intentionally
left as is (established, tested compat behavior for a class that reads
the value from **kwargs without declaring it, e.g.
BasicLangExtractRecognizer). Verified no current concrete recognizer
hits this combination either way. Added
test_plural_converted_to_singular_when_only_singular_is_reachable,
which -- like the context regression test -- actually constructs the
class rather than only inspecting the prepared dict.
- The per-class round-trip test's own `accepts_context` still included
`or leaf_accepts_var_kw`, a leftover from before the **kwargs carve-out
for context was reverted last commit -- it no longer matched what the
loader does, so it could not have caught this class of regression.
Simplified to match production exactly.
- OPTIONAL_DEPENDENCY_MODULES: AzureAILanguageRecognizer's try block also
imports azure.core.credentials, and AzureHealthDeidRecognizer's
get_azure_credential() call additionally requires azure.identity (both
confirmed by reading the source) -- a partial install of only the
first package in either pair reached the constructor instead of
skipping. Both now probe every module their constructor actually needs.
Verified with each secondary module hidden while its sibling stays
importable.
- test_unknown_key_is_not_silent's `except ValueError: raised = True`
counted any ValueError as the gap being closed, including one unrelated
to `no_such_key`. Now checks the message and re-raises anything else.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014cYn8gUDL2txRa5V7r4vLZ
* fix(analyzer): build recognizers from bare-string YAML entries
`recognizers: [CreditCardRecognizer]` is the shorthand the configuration
schema accepts for a predefined recognizer with no further settings, but
`_split_recognizers` required a mapping for the predefined list and excluded
strings from the custom list, so the entry matched neither and no recognizer
was built. The registry came back empty with no error, and a mixed list
silently lost only its string entries.
Normalize a bare string to {"name": <str>, "type": "predefined"} in
`_split_recognizers`, the single place every entry passes through before
anything is constructed. The custom list now keys off the same mapping check
rather than "not a string".
Behavior changes:
- A bare-string entry now constructs the named recognizer instead of being
silently discarded. Registries using this form gain the recognizers they
already asked for.
- A bare-string entry naming an unknown class now raises
PredefinedRecognizerNotFoundError, matching what a `name:` in a mapping
entry already did, instead of yielding an empty registry with no
diagnostic.
- Dict entries are unaffected. The shipped default_recognizers.yaml has no
bare-string entries, so the default registry is unchanged.
Tests: five for the bare-string path (split normalization, construction,
one instance per registry language, mixed with dict entries, unknown name
raising), all verified to fail without this change. Four more pin that
custom YAML-defined recognizers are untouched by the predefined-path rules
this PR added: fields survive and no "ignoring" warning fires, an entry
without a `type` key is still custom, and a multi-language custom entry
keeps per-language context.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(analyzer): trim comment prose in _prepare_recognizer_kwargs
Several comments added across this PR's review rounds restated what the
code already says, or explained the fix history (test-double names, why
an earlier approach was reverted) rather than a non-obvious "why". Cut
each to the one fact a reader actually needs; behavior is unchanged.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014cYn8gUDL2txRa5V7r4vLZ
* fix(analyzer): correct context-warning wording; reuse loader's MRO walk in tests
- The dropped-context log message said context boosts "every result a
recognizer emits"; it only boosts a result's score when a context word
actually matches nearby text.
- test_recognizer_config_conformance.py duplicated
RecognizerListLoader._reachable_init_param_names; import it from
production code instead of maintaining a second copy.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014cYn8gUDL2txRa5V7r4vLZ
* refactor(analyzer): declare optional-dependency modules on the recognizer class
Per review: replace the test-file OPTIONAL_DEPENDENCY_MODULES dict (which
would go stale as new optional-dependency recognizers are added) with a
EntityRecognizer.OPTIONAL_DEPENDENCY_MODULES class attribute that each
recognizer needing a third-party extra declares on itself. A subclass with
the same requirement (MedicalNERRecognizer, AzureOpenAILangExtractRecognizer,
BasicLangExtractRecognizer) inherits the value via normal Python attribute
lookup instead of needing its own entry. The conformance suite now reads
cls.OPTIONAL_DEPENDENCY_MODULES directly.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014cYn8gUDL2txRa5V7r4vLZ
* test(analyzer): drop redundant guard test, prove NOT_LOADABLE_AS_PREDEFINED_ENTRY
- Removed test_known_contract_gaps_names_only_real_classes: with
KNOWN_CONTRACT_GAPS empty, the check was always trivially true, and a
stale/renamed entry is already caught by
test_recognizer_accepts_registry_injected_keys's own assertion message.
- Added test_not_loadable_as_predefined_entry_actually_fails, which
constructs each of LocalRecognizer/PatternRecognizer/ZaPhoneNumberRecognizer
via a real type:predefined registry entry and asserts the specific error
the exclusion's comment claims, instead of relying on the comment alone.
None of the three are abstract in the inspect.isabstract sense; each is
excluded for a distinct, now-proven structural reason.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014cYn8gUDL2txRa5V7r4vLZ
* fix(analyzer): fix empty-entity-list, cross-check untested kwargs, default languages
Three findings from the latest Copilot review, each verified by direct
reproduction before fixing:
- _prepare_recognizer_kwargs: an explicitly empty supported_entities: []
survived the plural->singular conversion's truthiness check and was kept
in kwargs by the **kwargs-compat filter, then forwarded to a strict
singular-only parent, raising TypeError. Now always pops the plural key
once singular-only is reachable, only defaulting the singular value when
the list is non-empty.
- test_context_dropped_for_leaf_forwarding_kwargs_to_a_strict_parent
stripped supported_language from the prepared kwargs before constructing,
because StrictParent didn't declare it -- masking a regression in that
key's handling. StrictParent now declares every registry-injected key
(still not context, the key under test), so the test constructs with the
unmodified, realistic prepared kwargs.
- RecognizerRegistryProvider.create_recognizer_registry passed a None
supported_languages (a raw dict config supplying recognizers +
global_regex_flags but omitting supported_languages skips the defaults
merge) straight into RecognizerListLoader.get, which iterates it and
raised TypeError -- reproduced with both a bare-string and a mapping-form
predefined entry. RecognizerRegistry itself already falls back to ["en"],
but only after that crash; now the same default applies before the
recognizers are built.
Also fixed test_unknown_key_is_not_silent's exception-matching: it only
checked str(exc), but ConfigurationValidator wraps the underlying pydantic
ValidationError in a generic "Invalid recognizer registry configuration"
ValueError, so the offending key name survives only on __cause__. Verified
by simulating turn 06's extra="forbid" fix locally: the old check stayed
False (masking the fix forever, keeping the xfail marker in place), the
fixed check correctly flips to True.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014cYn8gUDL2txRa5V7r4vLZ
* fix(analyzer): only default supported_languages when omitted, not when empty
The None-defaulting fix from the previous round used a falsy check
(`or ["en"]`), which also coerced an explicit `supported_languages: []` --
a distinct, deliberately preserved configuration
(RecognizerRegistryConfig keeps it as [], not None) -- into ["en"],
silently building a CreditCardRecognizer the caller never asked for.
Checks `is None` specifically now, so an explicit empty list still
resolves to zero recognizers.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014cYn8gUDL2txRa5V7r4vLZ
---------
Co-authored-by: Claude <noreply@anthropic.com>
Co-authored-by: Omri Mendels <omri@mindware.health>
Change Description
Describe your changes
Issue reference
Fixes #XX
Checklist