docs: require configuration-path tests for new recognizers by omri374 · Pull Request #2211 · data-privacy-stack/presidio

omri374 · 2026-08-04T09:13:06Z

Change Description

Adds a required configuration-path test for new recognizers, and corrects several pieces of guidance that do not match current maintainer practice or current behavior.

Why

Recognizers are tested by constructing them in Python. Nothing tests the path users take, which is flipping enabled: true in a registry YAML.

Measure	Value
Entries in `default_recognizers.yaml`	86
Entries with `enabled: false`	61
Recognizer test files	108
Of those, files referencing the registry or a YAML config	8
Of those, tests that enable a predefined recognizer through config and assert detection	0

Enabling every entry in isolation, with the entry's own languages, surfaces defects that Python-only tests cannot see:

TypeError: UsMbiRecognizer.__init__() got an unexpected keyword argument 'name'
TypeError: KrBrnRecognizer.__init__() got an unexpected keyword argument 'name'
TypeError: KrDriverLicenseRecognizer.__init__() got an unexpected keyword argument 'name'

The loader passes the YAML name key to the constructor. These three classes do not accept it, so enabling any of them raises and takes down construction of the whole registry. Reproduced through RecognizerRegistryProvider. AzureAILanguageRecognizer and KrPassportRecognizer share the gap but are not reachable from the shipped config.

Two more in the same family:

31 entries declare no English support while the top-level supported_languages is ["en"]. Setting enabled: true on any of them loads nothing, with no error or warning.
KrPassportRecognizer defaults to supported_language="kr". Its four siblings use ko, and the YAML lists both codes for those siblings.

What changed

New requirement

Part 1, new section 6: every new recognizer needs a test that enables it in a YAML config and asserts detection through RecognizerRegistryProvider
Behavior must be identical across all three construction paths (direct, add_recognizer(), configuration)
Part 2, section 7: matching review-priority bullets

Corrected guidance

Enabled-by-default is framed as false-positive surface rather than geography, with explicit criteria. The previous text said only "Country-specific defaults to false"
Score bands table. The previous example assigned 0.3 as a "low base score", but the codebase uses 0.05 to 0.1 for weak patterns with (weak) / (very weak) in the pattern name
Thresholds, not hard context requirements, are the suppression mechanism. presidio-structured needs context-free detection
Context words are matched as substrings by default, so member fires on remember and auth on author. Context is also prefix-only by default
Tests must assert exact scores rather than ranges, include a lookalike negative, and exercise context enhancement
New "Backward Compatibility" section in Part 1, covering behavior changes that are not signature changes
Country directory naming (full country name, not ISO code) and ISO 639-1 language codes
Python version stated as the actual supported range rather than "all versions"
New "Configuration Issues" troubleshooting section

Bug fix in the docs

The SSN test example used 123-45-6789 as a true positive. That value is on the recognizer's sample-SSN denylist and returns no results, so the example as written fails. The "False positive prevention" case using the same value passed for the wrong reason. Replaced with 456-78-9012 and moved the denylisted value to the true-negative group

Follow-ups, not in this PR

The three broken recognizers. Each needs name: Optional[str] = None in its constructor. Small and separable.
A repo-level sweep test. This PR sets the rule for new recognizers but does nothing for the 61 already merged. A parametrized test over every YAML entry covers them; it currently reports 165 passing and 6 failing, matching the three recognizers above.
The CHANGELOG rule. The file states twice that PRs must not modify CHANGELOG.md, but open and merged PRs do. Either a CI check should enforce it or the rule should be dropped, since an unenforced rule teaches contributors to skim the file. Left unchanged here because the decision is yours.

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 (documentation only)
All unit tests and lint checks pass locally
My PR contains documentation updates / additions if required

Recognizers are tested by constructing them in Python. Nothing tests the path users take, which is enabling them in a registry YAML. 61 of the 86 entries in default_recognizers.yaml ship disabled, so their constructors are never exercised from configuration at all. Enabling every entry in isolation shows three that raise on construction (UsMbiRecognizer, KrBrnRecognizer, KrDriverLicenseRecognizer: __init__ rejects the 'name' key the loader passes), and 31 that load nothing because their languages are excluded by the top-level supported_languages filter, silently. Adds a required configuration-path test, plus guidance drawn from recurring review findings: enabled-by-default framed as false-positive surface rather than geography, score bands matching the codebase, substring context matching, exact score assertions, lookalike negatives, and a backward-compatibility section. Also corrects the SSN test example, which used 123-45-6789 as a true positive. That value is on the recognizer's sample-SSN denylist and returns no results.

github-actions · 2026-08-04T09:14:00Z

Coverage report (presidio-anonymizer)

Click to see where and how coverage changed

File	Statements	Missing	Coverage	Coverage (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}

github-actions · 2026-08-04T09:14:26Z

Coverage report (presidio-structured)

Click to see where and how coverage changed

File	Statements	Missing	Coverage	Coverage (new stmts)	Lines missing
presidio-structured/presidio_structured/data
data_processors.py
Project Total

_{This report was generated by python-coverage-comment-action}

github-actions · 2026-08-04T09:14:38Z

Coverage report (presidio-cli)

Click to see where and how coverage changed

File	Statements	Missing	Coverage	Coverage (new stmts)	Lines missing
presidio-cli/presidio_cli
cli.py
Project Total

_{This report was generated by python-coverage-comment-action}

Clarified scoring criteria for strong patterns in the instructions.

Copilot

Pull request overview

Updates the repository’s Copilot contributor guidance for Presidio recognizers to require configuration-path testing (enabling recognizers via YAML and loading through RecognizerRegistryProvider) and to align recognizer/testing guidance with current loader behavior and maintainer practices.

Changes:

Adds a new requirement that new recognizers include at least one test which enables the recognizer in a YAML registry config and asserts detection through RecognizerRegistryProvider.
Refines recognizer design/testing guidance (score bands, thresholds vs hard context requirements, substring context matching behavior, and exact-score assertions).
Fixes the SSN documentation example to avoid denylisted sample SSNs and adds troubleshooting/backward-compatibility guidance.

github-actions · 2026-08-04T09:18:34Z

Coverage report (presidio-image-redactor)

Click to see where and how coverage changed

File	Statements	Missing	Coverage	Coverage (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 · 2026-08-04T09:20:31Z

Coverage report (presidio-analyzer)

Click to see where and how coverage changed

File	Statements	Missing	Coverage	Coverage (new stmts)	Lines missing
presidio-analyzer/presidio_analyzer
analyzer_engine.py
entity_recognizer.py
presidio-analyzer/presidio_analyzer/chunkers
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
__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
Project Total

The report is truncated to 25 files out of 79. To see the full report, please visit the workflow summary page.

_{This report was generated by python-coverage-comment-action}

Copilot

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Suppressed comments (5)

.github/copilot-instructions.md:65

The repo already has a non-full-name country directory (presidio_analyzer/predefined_recognizers/country_specific/thai/), so stating that only us/uk are exceptions is inaccurate and can confuse contributors. Either list thai as an existing exception or relax the rule to match current layout.

Directory names are the full lowercase country name (`south_africa`, `philippines`,
`canada`), not the ISO country code. The only exceptions are the pre-existing `us`
and `uk` directories. Do not add new abbreviated directories.

.github/copilot-instructions.md:108

The LemmaContextAwareEnhancer constructor parameter is context_matching_mode, not matching_mode; using the wrong name here will lead to incorrect guidance/snippets.

**Context words are matched as substrings.** `LemmaContextAwareEnhancer` defaults to
`matching_mode="substring"`, so short context words fire on unrelated tokens.

.github/copilot-instructions.md:69

This language-code guidance conflicts with current code/config: default_recognizers.yaml includes kr in supported_languages for Korean recognizers and KrPassportRecognizer defaults supported_language="kr". Clarify that kr is a legacy literal tag and that ISO 639-1 (ko) should be used to avoid recognizers being filtered out when users pass standard language codes.

Language codes are different: `supported_language` and the YAML `supported_languages`
key take ISO 639-1 language codes (`ko` for Korean), not country codes (`kr`). A
mismatch here produces a recognizer that never loads.

.github/copilot-instructions.md:100

UsBankRecognizer uses score 0.05 for its 8-17 digit pattern, which falls in the table’s “very weak” band; calling it a “weak score” here is misleading given the new score-band guidance. Consider stating the exact score instead to keep the example consistent.

Compare against existing recognizers before choosing: `UsPassportRecognizer` uses
0.05 for nine bare digits, `UsBankRecognizer` uses a weak score for 8-17 digits.

.github/copilot-instructions.md:202

The configuration-path test example references an undefined nlp_engine variable, so it won’t run as written. Defining a lightweight engine (e.g., NoOpNlpEngine) in the snippet makes it copy/pasteable and avoids requiring spaCy model downloads for pattern-only recognizers.

    registry = RecognizerRegistryProvider(
        conf_file=conf
    ).create_recognizer_registry()
    analyzer = AnalyzerEngine(registry=registry, nlp_engine=nlp_engine)

Replaces the enabled-by-default criteria, which used the presence of a checksum as a gate. Most patterns have no checksum: about 40% of the predefined PatternRecognizer subclasses do not override validate_result, and that is the right choice when the entity has no verifiable structure. Coincidental matches are also not the problem. A generic pattern scored at 0.05 costs nothing, because a threshold removes it while context or validation can still lift a real match. The disqualifier for shipping enabled is a coincidental match that arrives at a score no threshold can separate from a true positive. Adds a section covering the hooks as a generic capability: True replaces the score with MAX_SCORE, False drops the result, None leaves the pattern score alone. Spells out that a check which is only mandatory across part of the entity's range can promote but never invalidate, so it inflates coincidental matches to full confidence while genuine lookalikes keep the base score. The previous wording compressed this into one unclear sentence.

Copilot

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Suppressed comments (1)

.github/copilot-instructions.md:65

The guidance says the only abbreviated country-specific directory names are us and uk, but the current codebase also includes country_specific/thai/ (e.g., th_tnin_recognizer.py), so the statement is inaccurate and could mislead contributors.

Directory names are the full lowercase country name (`south_africa`, `philippines`,
`canada`), not the ISO country code. The only exceptions are the pre-existing `us`
and `uk` directories. Do not add new abbreviated directories.

Adds a repo-local code review skill that captures the recognizer testing, scoring, and backward-compatibility practices established in PR #2211. The skill classifies a diff (recognizer change vs. shared-class change) and applies matching checklists. Its load-bearing rule: any PR adding or changing a recognizer must include a configuration-path test that loads the recognizer through RecognizerRegistryProvider and asserts detection, since predefined recognizers ship enabled: false and are otherwise never exercised on the path users actually take. A references file provides the test template and the full list of defects the test catches. Also covers score-band calibration, validate_result promotion pitfalls, context-substring matching, exact-score/lookalike-negative test requirements, and backward-compatibility review for changes to shared library classes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013ck825cANTDYre5UCfmpYE

Adds .github/instructions/recognizer-review.instructions.md, a review-only, path-scoped GitHub Copilot custom-instructions file. It applies to recognizer sources, default_recognizers.yaml, and recognizer tests, and directs Copilot code review to require a RecognizerRegistryProvider configuration-path test, check construction-path agreement and backward compatibility, and enforce the score-calibration, validate_result, context-word, and test-quality rules captured in the recognizer-pr-review skill. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013ck825cANTDYre5UCfmpYE

- Use context_matching_mode (the actual LemmaContextAwareEnhancer constructor parameter) instead of matching_mode. - Correct the directory-naming note: thai/ (and us/uk) are pre-existing short forms not to imitate, rather than claiming only us/uk are exceptions. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013ck825cANTDYre5UCfmpYE

Copilot AI lite review requested due to automatic review settings August 4, 2026 09:13

Copilot started reviewing on behalf of omri374 August 4, 2026 09:14 View session

Update scoring criteria for strong patterns

0ae3660

Clarified scoring criteria for strong patterns in the instructions.

Copilot AI reviewed Aug 4, 2026

View reviewed changes

Comment thread .github/copilot-instructions.md Outdated

Comment thread .github/copilot-instructions.md Outdated

Copilot AI review requested due to automatic review settings August 4, 2026 09:16

Copilot started reviewing on behalf of omri374 August 4, 2026 09:17 View session

Copilot AI reviewed Aug 4, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings August 4, 2026 09:34

Copilot started reviewing on behalf of omri374 August 4, 2026 09:36 View session

Copilot AI reviewed Aug 4, 2026

View reviewed changes

Merge branch 'main' into docs/copilot-instructions-yaml-testing

f594c3d

omri374 force-pushed the docs/copilot-instructions-yaml-testing branch from 59a7f1e to f594c3d Compare August 8, 2026 05:30

claude added 3 commits August 8, 2026 06:12

Navigation Menu

Uh oh!

docs: require configuration-path tests for new recognizers - #2211

docs: require configuration-path tests for new recognizers#2211
omri374 wants to merge 7 commits into
mainfrom
docs/copilot-instructions-yaml-testing

omri374 commented Aug 4, 2026

Uh oh!

github-actions Bot commented Aug 4, 2026

Uh oh!

github-actions Bot commented Aug 4, 2026

Uh oh!

github-actions Bot commented Aug 4, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Aug 4, 2026

Uh oh!

github-actions Bot commented Aug 4, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

omri374 commented Aug 4, 2026

Change Description

Why

What changed

Follow-ups, not in this PR

Checklist

Uh oh!

github-actions Bot commented Aug 4, 2026

Coverage report (presidio-anonymizer)

Uh oh!

github-actions Bot commented Aug 4, 2026

Coverage report (presidio-structured)

Uh oh!

github-actions Bot commented Aug 4, 2026

Coverage report (presidio-cli)

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Aug 4, 2026

Coverage report (presidio-image-redactor)

Uh oh!

github-actions Bot commented Aug 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Coverage report (presidio-analyzer)

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions Bot commented Aug 4, 2026 •

edited

Loading