Skip to content

Confit hardening PRD: Recognizer configuration conformance suite (#1) - #2248

Open
omri374 wants to merge 9 commits into
mainfrom
ralph/config-02-conformance-suite
Open

Confit hardening PRD: Recognizer configuration conformance suite (#1)#2248
omri374 wants to merge 9 commits into
mainfrom
ralph/config-02-conformance-suite

Conversation

@omri374

@omri374 omri374 commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator

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 keys RecognizerListLoader injects from a registry YAML entry (name, supported_language, context, and one of supported_entity/supported_entities), a field-reach test over every entry of the shipped conf/default_recognizers.yaml, and a per-class round-trip test that builds a synthetic registry entry for every concrete recognizer class and asserts name/context/score_thresholds actually reach the constructed instance. It also fixes, in the loader, the registry-build crash the first of these tests found (a context entry on a recognizer whose constructor does not accept it), and adds logger.warning calls for two previously-silent cases the tests surfaced.

Behavior changes

None to detection results or scores. Two new logger.warning lines, and one TypeError that becomes a warning:

  • RecognizerListLoader._prepare_recognizer_kwargs now drops context when the recognizer class does not accept it, and logs a WARNING naming the class and the key. Before this PR the same entry raised TypeError: __init__() got an unexpected keyword argument 'context' and the whole registry failed to build. Nothing changes for classes that accept context.
  • The loader already silently dropped supported_entity/supported_entities for 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 context is handled in the loader, not the constructors

EntityRecognizer.context is 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, AzureHealthDeidRecognizer and the LangExtract recognizers would boost a DATE as much as a NAME. Those classes deliberately do not accept context, and this PR does not add it to them.

What was broken is that a registry entry setting context for such a class crashed the whole registry build. The fix is one rule in the loader: a context key the class does not accept is dropped with a warning naming the class. The conformance contract is therefore name, supported_language and an entity key; context is not part of it.

What's in each commit / story

  1. Story 1test_recognizer_accepts_registry_injected_keys, parametrized over every concrete EntityRecognizer subclass, walking each constructor's __init__ MRO to compute which registry-injected keys are reachable. KNOWN_CONTRACT_GAPS regression-locks the three gaps found (all context); ENTITIES_FROM_OWN_CONFIG documents the LangExtract family's legitimate exemption from the entity-key requirement.
  2. Story 2 — the loader-side context rule 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 in test_recognizers_loader_utils.py. KNOWN_CONTRACT_GAPS is empty: every concrete recognizer already meets the name / supported_language / entity-key contract.
  3. Story 3test_shipped_entry_fields_reach_constructed_recognizer, parametrized over every entry of conf/default_recognizers.yaml: loads each through RecognizerRegistryProvider and asserts language count, supported_language, per-language context, name, and (when set) supported_entities/score_thresholds all reach the constructed instance(s). Reuses NOT_LOADABLE_FROM_SHIPPED_ENTRY from tests/test_recognizers_loader_utils.py (imported, not duplicated).
  4. Story 4test_synthetic_entry_round_trips_to_every_concrete_class, parametrized over every concrete recognizer class with a synthetic conf_<Class> entry; REQUIRED_KWARGS/REQUIRED_ENV supply what a handful of classes need to construct (an Azure endpoint/credentials, HuggingFaceNerRecognizer's model_name); NOT_LOADABLE_AS_PREDEFINED_ENTRY documents three classes that structurally cannot be built through a type: predefined entry at all (LocalRecognizer, PatternRecognizer, ZaPhoneNumberRecognizer — all subclassing-only base classes); for a class that does not accept context the test asserts the entry still loads, the instance keeps the base default [], and the loader WARNING names the class; CONTEXT_NOT_APPLIED documents BasicLangExtractRecognizer's pre-existing (unchanged) "accepts but doesn't apply context" behavior. test_unknown_key_is_not_silent documents today's silent-drop gap for an unrecognized YAML key and is marked xfail(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 a2106de added context to three constructors; commit 45c8a3b reverts that and moves the fix into the loader, per review. The net diff contains no constructor changes.

  • Commits 7cd44e3 and 31fa9b0 address the Copilot review comment: both loader warnings and the context drop now key off RecognizerListLoader._reachable_init_param_names (constructor parameters reachable through **kwargs forwarding along the MRO), so StanzaRecognizer and TransformersRecognizer, which forward to SpacyRecognizer, are not reported as ignoring keys they apply.

  • uv run ruff format --check . flags 26 pre-existing files unrelated to this PR (confirmed identical on origin/main before this branch's changes); the actual CI lint gate (.github/workflows/ci.yml) only runs ruff 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 in test_recognizer_registry_provider.py, one test_analyzer_engine_provider.py case) fail with a ProxyError/403, confirmed identical on origin/main before 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

  • I have reviewed the contribution guidelines
  • I agree to follow this project's Code of Conduct
  • I confirm that I have the right to submit this contribution and that it does not knowingly contain proprietary or confidential code.
  • My code includes unit tests
  • All unit tests and lint checks pass locally
  • My PR contains documentation updates / additions if required (none needed: no new entities, no schema changes)

🤖 Generated with Claude Code

https://claude.ai/code/session_014cYn8gUDL2txRa5V7r4vLZ


Generated by Claude Code

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
Copilot AI lite review requested due to automatic review settings September 10, 2026 14:44
@github-actions

Copy link
Copy Markdown
Contributor

Coverage report (presidio-anonymizer)

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)
Lines missing
  presidio-anonymizer/presidio_anonymizer
  __init__.py
  anonymizer_engine.py
  presidio-anonymizer/presidio_anonymizer/entities/engine
  pii_entity.py
  presidio-anonymizer/presidio_anonymizer/entities/engine/result
  operator_result.py
  presidio-anonymizer/presidio_anonymizer/operators
  custom.py
Project Total  

This report was generated by python-coverage-comment-action

Comment thread presidio-analyzer/tests/test_recognizer_config_conformance.py
@github-actions

Copy link
Copy Markdown
Contributor

Coverage report (presidio-cli)

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)
Lines missing
  presidio-cli/presidio_cli
  cli.py
Project Total  

This report was generated by python-coverage-comment-action

@github-actions

Copy link
Copy Markdown
Contributor

Coverage report (presidio-image-redactor)

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)
Lines missing
  presidio-image-redactor/presidio_image_redactor
  dicom_image_pii_verify_engine.py
  document_intelligence_ocr.py
  image_analyzer_engine.py
Project Total  

This report was generated by python-coverage-comment-action

@github-actions

Copy link
Copy Markdown
Contributor

Coverage report (presidio-structured)

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)
Lines missing
  presidio-structured/presidio_structured/data
  data_processors.py
Project Total  

This report was generated by python-coverage-comment-action

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.py to assert constructor contract conformance and configuration field reachability across shipped and synthetic registry entries.
  • Fix constructor compatibility for context in AzureHealthDeidRecognizer, MedicalNERRecognizer, and AzureOpenAILangExtractRecognizer, plus targeted regression tests.
  • Add a logger.warning when supported_entity/supported_entities is 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.

@omri374 omri374 closed this Sep 10, 2026
@github-actions

github-actions Bot commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

Coverage report (presidio-analyzer)

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)
Lines missing
  presidio-analyzer/presidio_analyzer
  analyzer_engine.py
  entity_recognizer.py
  presidio-analyzer/presidio_analyzer/chunkers
  character_based_text_chunker.py
  text_chunker_provider.py
  presidio-analyzer/presidio_analyzer/context_aware_enhancers
  lemma_context_aware_enhancer.py
  presidio-analyzer/presidio_analyzer/input_validation
  schemas.py
  yaml_recognizer_models.py
  presidio-analyzer/presidio_analyzer/llm_utils
  config_loader.py
  presidio-analyzer/presidio_analyzer/nlp_engine
  __init__.py
  nlp_engine_provider.py
  presidio-analyzer/presidio_analyzer/predefined_recognizers
  __init__.py
  presidio-analyzer/presidio_analyzer/predefined_recognizers/country_specific/finland
  fi_personal_identity_code_recognizer.py
  presidio-analyzer/presidio_analyzer/predefined_recognizers/country_specific/germany
  de_bsnr_recognizer.py
  de_id_card_recognizer.py
  de_lanr_recognizer.py
  de_passport_recognizer.py
  de_social_security_recognizer.py
  de_vat_id_recognizer.py
  presidio-analyzer/presidio_analyzer/predefined_recognizers/country_specific/poland
  pl_pesel_recognizer.py
  presidio-analyzer/presidio_analyzer/predefined_recognizers/ner
  gliner_recognizer.py
  huggingface_ner_recognizer.py
  presidio-analyzer/presidio_analyzer/predefined_recognizers/third_party
  azure_ai_language.py
  presidio-analyzer/presidio_analyzer/recognizer_registry
  recognizer_registry.py
  recognizer_registry_provider.py
  recognizers_loader_utils.py 291-292
Project Total  

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>
@omri374 omri374 reopened this Sep 10, 2026
@omri374 omri374 changed the title test(analyzer): recognizer configuration conformance suite (config hardening, turn 02) test(analyzer): recognizer configuration conformance suite (config hardening PRD) Sep 10, 2026
@omri374 omri374 changed the title test(analyzer): recognizer configuration conformance suite (config hardening PRD) Confit hardening PRD: Recognizer configuration conformance suite (#1) Sep 10, 2026
_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
Copilot AI review requested due to automatic review settings September 10, 2026 15:04
…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>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 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_flags to 26 even though this module already imports GLOBAL_REGEX_FLAGS from the shipped default_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 in kwargs; 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_flags to 26. Using GLOBAL_REGEX_FLAGS keeps 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

Copilot AI review requested due to automatic review settings September 10, 2026 15:09
- 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

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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

Comment thread presidio-analyzer/tests/test_recognizers_loader_utils.py
Comment thread presidio-analyzer/tests/test_recognizers_loader_utils.py Outdated
Copilot AI review requested due to automatic review settings September 10, 2026 15:16
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

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 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

Copilot AI review requested due to automatic review settings September 10, 2026 15:22

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 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_entities assertions 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 None instead so empty lists are still asserted.
  • Files reviewed: 3/3 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants