diff --git a/.claude/skills/recognizer-pr-review/SKILL.md b/.claude/skills/recognizer-pr-review/SKILL.md new file mode 100644 index 0000000000..e21836c9d8 --- /dev/null +++ b/.claude/skills/recognizer-pr-review/SKILL.md @@ -0,0 +1,212 @@ +--- +name: recognizer-pr-review +description: >- + Reviews Presidio pull requests for recognizer correctness, test coverage, and + backward compatibility. Use this whenever reviewing, or being asked to review, + a PR or diff in this repo — especially any change that adds or modifies a + PII recognizer (files under predefined_recognizers/, edits to + default_recognizers.yaml, new Pattern/CONTEXT/validate_result code, or a new + recognizer test). Also use it for any change to a shared analyzer/anonymizer + base class, since those ripple across the library. Trigger even when the user + just says "review this PR", "look over my changes", or "does this recognizer + look right" without naming recognizers explicitly. +--- + +# Reviewing Presidio recognizer & core PRs + +Presidio is a library. A recognizer that works when built in Python can still be +unreachable, or crash, when a user enables it in YAML — and a one-line change to a +shared base class silently alters detection results for every downstream user who +wrote no new code. This skill exists so those two failure modes get caught in +review instead of in production. + +Use it to review a diff. Work in two passes: first decide **what kind of change +this is**, then apply the matching checklist below. + +## Pass 1 — Classify the change + +Look at the changed files and answer these before commenting: + +- **Does it add or change a recognizer?** Signals: new/edited files under + `predefined_recognizers/`, new `Pattern(...)` / `PATTERNS` / `CONTEXT`, a + `validate_result` / `invalidate_result` override, a new entry (or an + `enabled:` / `supported_languages:` edit) in + `presidio_analyzer/conf/default_recognizers.yaml`, or a new + `test_*_recognizer.py`. → Apply **Recognizer checklist**. +- **Does it touch a shared class?** Anything in the analyzer/anonymizer base + classes, `RecognizerRegistry`, providers, enhancers, or `EntityRecognizer` / + `PatternRecognizer` themselves. → Apply **Backward-compatibility checklist**. + +A single PR is often both. When in doubt, run both passes. + +Keep feedback specific and actionable — cite the file and line, and give the +concrete fix, not just the concern. Let CI handle formatting; don't spend review +budget on style Ruff already enforces. + +## Recognizer checklist + +The single highest-value thing to verify is the **configuration-path test**, +because it is the gap that Python-only tests structurally cannot cover. + +### 1. Require a configuration-path test (the load-bearing rule) + +Most predefined recognizers ship `enabled: false`, so the default test run never +constructs them from configuration. Users, however, reach them exactly one way: +flipping `enabled: true` in a registry YAML. If the PR's only tests build the +recognizer directly in Python, that user path is untested. + +**Require at least one test that loads the recognizer through +`RecognizerRegistryProvider` and asserts detection.** It should write a small +YAML config with the recognizer `enabled: true`, build the registry, run +`AnalyzerEngine.analyze`, and assert the entity is returned. See +`references/config-path-testing.md` for a template and the full list of defects +this catches (constructor-signature mismatches, missing `__init__.py` exports, +`country_code` / language-filter mismatches, class defaults dropped on load). + +Real bugs this rule surfaces, reproduced through the registry: + +``` +TypeError: UsMbiRecognizer.__init__() got an unexpected keyword argument 'name' +``` + +The loader passes the YAML `name` key to the constructor; a recognizer whose +`__init__` doesn't accept `name` takes down construction of the whole registry +the moment it's enabled. Python-only tests never see it. + +### 2. Construction paths must agree + +Building the recognizer directly, adding it via `registry.add_recognizer()`, and +loading it from configuration must all yield the same recognizer. Flag any +defaulting or validation logic that runs on one path but not the others — that +divergence is a latent bug, and it's the thing config-path tests exist to expose. + +### 3. Language codes vs. country codes + +`supported_language` / the YAML `supported_languages` key take **ISO 639-1 +language codes** (`ko` for Korean), not country codes (`kr`). A mismatch produces +a recognizer that loads nothing — silently, with no error. Also check the +top-level `supported_languages` in any test config: it defaults to `["en"]` and +acts as a global filter, so a `de`-only recognizer won't load unless the test +sets it. Non-English recognizers should state the required top-level languages in +the PR description. + +### 4. Score calibration and pattern specificity + +The base score should reflect how much the pattern *alone* narrows the space, +independent of any downstream threshold. A generic pattern scored high is the +core false-positive risk. + +| Score | Use when | Name | +| --- | --- | --- | +| 0.05–0.1 | Bare digit/alphanumeric runs, no structure | `(very weak)` | +| 0.1–0.3 | Some structure: delimiters, prefix, length constraint | `(weak)` | +| 0.3–0.5 | Distinctive format, no validation | `(medium)` | +| 0.5+ | Distinctive format | `(strong)` | + +Compare against existing recognizers before accepting a score (`UsPassport` uses +0.05 for nine bare digits). A 0.3 on a pattern that also matches `covid19` or +`sha256` is overstated. + +### 5. Validation / invalidation hooks + +`validate_result` returning `True` **replaces the score with 1.0** — it is a jump +to full confidence, not a nudge. Before accepting a `return True`, ask what +fraction of arbitrary same-shape tokens would pass the check; a mod-11 check on a +17-char token passes ~9% of the time, sending ~9% of coincidental matches to 1.0 +where no threshold can filter them. + +- Return `None`, never `False`, when the check doesn't apply. `False` means + "definitely not the entity" and discards the result. +- Only promote with `True` where the check is genuinely mandatory for that value. +- No checksum is fine — ~40% of predefined recognizers don't override + `validate_result`. Don't flag a missing validator; the base score plus a + threshold is a valid design. Do flag an *invented* one that promotes weak + matches. +- Well-known sample values and reserved ranges belong in `invalidate_result`, + not buried in the regex. + +### 6. Enabled-by-default decision + +The question is **not** which country the entity belongs to — it's whether the +recognizer can produce *high-confidence* false positives. Default to +`enabled: false`; anything else needs justification in the PR description. The +disqualifier for shipping enabled is a coincidental match arriving at a score the +user cannot filter (i.e. something promoted it to a high score). + +### 7. Context words + +Context is matched as **substrings** by default (`context_matching_mode= +"substring"` in `LemmaContextAwareEnhancer`), so short words fire on unrelated +tokens: `member` matches `remember`, `auth` matches `author`. Prefer context +words long enough to be unambiguous (`member id`, `subscriber`). Context is also +prefix-only by default, so a context word *after* the match doesn't boost — tests +should cover both placements. + +Don't design a recognizer that can't fire without context: `presidio-structured` +has no surrounding text. Suppress low-confidence matches with thresholds, not by +requiring context. + +### 8. Test quality + +- **Assert exact scores, not ranges.** `assert 0.5 <= score <= 1.0` still passes + when checksum promotion or context enhancement breaks entirely. Pin it: + `assert result.score == pytest.approx(EntityRecognizer.MAX_SCORE)`. +- **Include a lookalike negative.** The false-positive surface is the point of the + test, not the happy path. Add a plausible non-PII token of the same shape (a + 17-char order ID for a VIN, a legal citation for a bank number) and assert it is + *not* flagged. +- **Exercise context enhancement.** A recognizer defining `CONTEXT` needs a test + showing the score differs between text with and without a context word. +- **Use example values the recognizer actually accepts.** Well-known samples like + `123-45-6789` are denylisted by `UsSsnRecognizer`; using one as a true positive + fails, and using it as a false-positive case passes for the wrong reason. + +### 9. Required companion updates + +A new recognizer needs all of: the export in `predefined_recognizers/__init__.py` +and the country `__init__.py`, an entry in `default_recognizers.yaml`, and a row +in `docs/supported_entities.md`. A missing export is exactly what the +config-path test catches. + +Directory naming: prefer the full lowercase country name +(`south_africa`, `philippines`). Some pre-existing dirs use short forms +(`us`, `uk`, `thai`) — don't imitate them for new directories. + +## Backward-compatibility checklist + +Because Presidio is a library, changes to shared classes alter results for users +who wrote no new code. Require the PR description to state what existing behavior +changes for anything edited outside a brand-new file. These count as behavior +changes even with no signature change: + +- Default values on shared base classes (`None` → `[]` flips truthiness for every + subclass). +- Properties on abstract interfaces — custom implementations inherit the new + default and may break. +- Scores, context lists, or patterns on *existing* recognizers. Flag any such + edit made as a side effect of adding a new recognizer; users depend on current + detection behavior. +- Anything altering which entities are returned for text that already worked. + +Two more to watch for: + +- **Surface new scoring inputs in explainability.** Anything changing how a score + is derived (context, negative context, thresholds) must show up in + `AnalysisExplanation`, or users can't tell why a result scored as it did. +- **Prefer warnings over exceptions when the caller can't fix the condition.** + Raising on a config the user didn't write turns a degraded result into a hard + failure. Prefer a property on a base class over a maintained list of class + names — lists drift, and PyPI users can't extend them. + +## Review-priority ordering + +When summarizing, lead with the highest-impact gaps in this order: + +1. No configuration-path test for a new recognizer. +2. Construction paths that disagree. +3. Undeclared backward-incompatible change to a shared class or existing recognizer. +4. `validate_result` promoting weak matches to 1.0 / range-based score assertions. +5. Missing lookalike negative or context-enhancement test. +6. Language/country-code mismatch, missing exports or doc rows. + +Terminology: say "threshold", not "cutoff"; use ISO 639-1 codes in examples. diff --git a/.claude/skills/recognizer-pr-review/references/config-path-testing.md b/.claude/skills/recognizer-pr-review/references/config-path-testing.md new file mode 100644 index 0000000000..4538774f8e --- /dev/null +++ b/.claude/skills/recognizer-pr-review/references/config-path-testing.md @@ -0,0 +1,78 @@ +# Configuration-path testing for recognizers + +Read this when reviewing (or writing) the required configuration-path test for a +new or changed recognizer. + +## Why this test is mandatory + +A recognizer has three construction paths: + +1. Direct: `MyRecognizer()` in Python. +2. Registry: `registry.add_recognizer(MyRecognizer())`. +3. Configuration: an entry in a YAML config, loaded by + `RecognizerRegistryProvider`. + +Path 3 is the one real users take to enable a predefined recognizer — they flip +`enabled: true` in a registry YAML. Most predefined recognizers ship +`enabled: false`, so the default test suite never exercises path 3. A recognizer +can pass every direct-construction test and still be unreachable, or crash the +entire registry, the moment someone enables it. + +## What the test catches that Python-only tests cannot + +- **Constructor signatures incompatible with the keys the loader passes.** The + loader forwards YAML keys (`name`, `supported_entity`, `context`, ...) into + `__init__`. A recognizer whose `__init__` doesn't accept `name` raises + `TypeError: __init__() got an unexpected keyword argument 'name'` — and because + it happens during registry construction, it takes down every other recognizer + too. +- **Class-name typos and missing `__init__.py` exports.** The loader resolves the + class by name; a missing export fails only on the configuration path. +- **`country_code` mismatch** between the class attribute and the YAML entry. +- **Class-level defaults** (thresholds, context) that configuration silently + discards because the loader sets them after construction instead of passing + them to `__init__`. +- **Language-filter exclusion.** The top-level `supported_languages` key defaults + to `["en"]` and acts as a global filter. A recognizer declaring only `de` loads + nothing, with no error or warning. + +## Template + +```python +def test_recognizer_loads_and_detects_when_enabled_in_yaml(tmp_path): + """Detection must work through the path users actually configure.""" + conf = tmp_path / "recognizers.yaml" + conf.write_text( + """ +supported_languages: + - en +recognizers: + - name: MyRecognizer + supported_languages: + - en + type: predefined + enabled: true + country_code: us +""" + ) + registry = RecognizerRegistryProvider( + conf_file=conf + ).create_recognizer_registry() + analyzer = AnalyzerEngine(registry=registry, nlp_engine=nlp_engine) + + results = analyzer.analyze("Member ID ABC123456", language="en") + + assert [result.entity_type for result in results] == ["MY_ENTITY"] +``` + +## Review notes + +- If the recognizer supports only non-English languages, the test's top-level + `supported_languages` must list those languages, and the PR description should + state the required top-level languages so users know to set them. +- The assertion should check the entity is actually returned (detection), not + merely that the registry built without raising. A recognizer that loads but + detects nothing is still broken. +- One good configuration-path test per new recognizer is enough. It complements, + not replaces, the direct-construction parametrized tests that cover + true/false positives and boundaries. diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index b887f232fe..000326e558 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -60,6 +60,14 @@ class MyRemoteRecognizer(RemoteRecognizer): - NLP/ML-based: `.../predefined_recognizers/nlp_engine_recognizers/` or `.../ner/` - Third-party: `.../predefined_recognizers/third_party/` +Directory names are the full lowercase country name (`south_africa`, `philippines`, +`canada`), not the ISO country code. Some pre-existing directories use short forms +(`us`, `uk`, `thai`); do not imitate them when adding new directories. + +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. + **3. Pattern Design Best Practices:** ```python # ❌ BAD: Too broad - matches month names as persons @@ -68,16 +76,102 @@ pattern = r"\b[A-Z][a-z]+\b" # ✅ GOOD: Specific pattern with context PATTERNS = [ Pattern( - "SSN", + "SSN (medium)", r"\b\d{3}-\d{2}-\d{4}\b", - 0.3 # Low base score, context will boost + 0.3 ) ] CONTEXT = ["ssn", "social security", "tax id"] ``` -**4. Document Pattern Sources:** +**Score bands.** The score must reflect how much the pattern alone narrows the +space, independent of any threshold applied downstream. + +| Score | Use when | Name the pattern | +| --- | --- | --- | +| 0.05 - 0.1 | Bare digit or alphanumeric runs with no structure | `"(very weak)"` | +| 0.1 - 0.3 | Some structure: delimiters, a prefix, a length constraint | `"(weak)"` | +| 0.3 - 0.5 | Distinctive format, no validation | `"(medium)"` | +| 0.5+ | Distinctive format | `"(strong)"` | + +Assigning 0.3 to a pattern that also matches `covid19` and `sha256` overstates it. +Compare against existing recognizers before choosing: `UsPassportRecognizer` uses +0.05 for nine bare digits, `UsBankRecognizer` uses a weak score for 8-17 digits. + +**Suppress with thresholds, not by requiring context.** Use `score_thresholds` to +keep low-confidence matches out of default results. Do not design a recognizer that +cannot fire without context: `presidio-structured` counts matches per column and has +no surrounding context to work with. + +**Context words are matched as substrings.** `LemmaContextAwareEnhancer` defaults to +`context_matching_mode="substring"`, so short context words fire on unrelated tokens. + +```python +# ❌ BAD: "member" matches "remember", "auth" matches "author" and "OAuth", +# "claim" matches "disclaimer" +CONTEXT = ["member", "auth", "claim"] + +# ✅ GOOD: long enough to be unambiguous +CONTEXT = ["member id", "subscriber", "prior authorization"] +``` + +Context is prefix-only by default (`context_prefix_count=5`, `context_suffix_count=0`), +so a context word appearing after the match does not boost the score. Test both +placements. + +**4. Validation and Invalidation:** + +`PatternRecognizer` gives every pattern two optional hooks. Both operate on the matched +text alone and neither is required. + +| Hook | Return | Effect on the result | +| --- | --- | --- | +| `validate_result` | `True` | Score is replaced with `MAX_SCORE` (1.0) | +| `validate_result` | `False` | Score is replaced with `MIN_SCORE` (0) and the result is dropped | +| `validate_result` | `None` | Pattern score stands unchanged | +| `invalidate_result` | `True` | Score is set to `MIN_SCORE` and the result is dropped | + +**Most patterns have no checksum, and that is fine.** About 40% of the predefined +`PatternRecognizer` subclasses do not override `validate_result` at all. Leaving it +unimplemented is the correct choice when the entity has no verifiable structure: the +base score carries the signal, and the user filters with a threshold. Do not invent a +validation rule to appear thorough. + +**Return `None`, never `False`, when the check does not apply.** `False` means "this is +definitely not the entity" and discards the result. If a checksum is mandatory for only +part of the entity's range, a value outside that range failing the check proves nothing, +so it must return `None`. + +**`True` is not a nudge, it is a jump to full confidence.** Before returning `True`, ask +what fraction of arbitrary tokens matching the same pattern would pass the check. A +17-character alphanumeric token has roughly a 1 in 11 chance of satisfying a mod-11 check +digit, so a recognizer that promotes on that check sends about 9% of coincidental matches +straight to 1.0, where no threshold can reach them. + +```python +# ❌ BAD: promotes to 1.0 on a check that random tokens pass ~9% of the time, +# and cannot invalidate, since the checksum is only mandatory regionally +def validate_result(self, pattern_text): + if self._mod11(pattern_text): + return True + return None # cannot say False: the checksum is not universal + +# ✅ GOOD: promote only where the checksum is mandatory, and say nothing elsewhere +def validate_result(self, pattern_text): + if not self._checksum_is_mandatory(pattern_text): + return None + return self._mod11(pattern_text) +``` + +If the checksum cannot be scoped that way, leave the score to the pattern and ship the +recognizer disabled by default. + +**Use `invalidate_result` for known non-entities.** Well-known sample values, reserved +ranges, and formats that collide with the pattern belong here rather than in the regex, +where they are easier to read and to test. + +**5. Document Pattern Sources:** ```python """ Recognizes US Social Security Numbers. @@ -92,7 +186,7 @@ Validation uses SSN format rules: AAA-GG-SSSS """ ``` -**5. Required Configuration Updates:** +**6. Required Configuration Updates:** ```python # Update all of these: # 1. presidio_analyzer/predefined_recognizers/__init__.py @@ -108,27 +202,97 @@ recognizers: - name: MyRecognizer supported_languages: ["en"] type: predefined - enabled: false # Country-specific defaults to false + enabled: false + country_code: us # 4. docs/supported_entities.md (add row to appropriate table) ``` -**6. Comprehensive Test Coverage:** +**Enabled by default or not.** The question is whether the recognizer can produce +*high-confidence* false positives, not which country the entity belongs to. Default to +`enabled: false` and justify anything else in the PR description. + +Coincidental matches are expected and acceptable. A generic pattern scored at 0.05 +costs the user nothing, because a confidence threshold removes it, and context or +validation can still raise it when the match is real. That is the mechanism working as +designed. + +The disqualifier is a false positive that arrives at a score the user cannot filter. +Once a coincidental match reaches 1.0, no threshold separates it from a true positive. +So the test for shipping enabled is: + +- The base score is calibrated to the pattern's specificity (see the score bands above) +- Nothing promotes a coincidental match to a high score. In particular, `validate_result` + must not return `True` for values that random tokens of the same shape pass at a + meaningful rate (see Validation and Invalidation below) + +**7. Test the Configuration Path, Not Just the Constructor:** + +Most predefined recognizers ship `enabled: false`, so the default test run never +constructs them from configuration. A recognizer that works when built in Python can +still be unreachable, or crash, when a user enables it in YAML. Every new recognizer +needs at least one test that goes through the registry. + +```python +def test_recognizer_loads_and_detects_when_enabled_in_yaml(tmp_path): + """Detection must work through the path users actually configure.""" + conf = tmp_path / "recognizers.yaml" + conf.write_text( + """ +supported_languages: + - en +recognizers: + - name: MyRecognizer + supported_languages: + - en + type: predefined + enabled: true + country_code: us +""" + ) + registry = RecognizerRegistryProvider( + conf_file=conf + ).create_recognizer_registry() + analyzer = AnalyzerEngine(registry=registry, nlp_engine=nlp_engine) + + results = analyzer.analyze("Member ID ABC123456", language="en") + + assert [result.entity_type for result in results] == ["MY_ENTITY"] +``` + +This catches, at minimum: +- Constructor signatures incompatible with the keys the loader passes (`name`, + `supported_entity`, `context`) +- Class name typos and missing `__init__.py` exports +- `country_code` mismatches between the class attribute and the YAML entry +- Class-level defaults (thresholds, context) that configuration silently discards +- A recognizer whose declared languages are excluded by the top-level + `supported_languages` filter, which loads nothing and reports no error + +⚠️ **Non-English recognizers:** the top-level `supported_languages` key in the config +acts as a global filter, and the shipped default is `["en"]`. A recognizer supporting +only `de` will not load from that config, silently. State the required top-level +languages in the PR description and cover this in the test above. + +**Behavior must not depend on how the recognizer was constructed.** Building it +directly, adding it with `registry.add_recognizer()`, and loading it from configuration +must all produce the same recognizer. Any defaulting or validation applied on one path +belongs on all of them. + +**8. Comprehensive Test Coverage:** ```python @pytest.mark.parametrize("text, expected_len, expected_positions", [ # True positives - valid formats - ("SSN: 123-45-6789", 1, ((5, 16),)), - ("My SSN is 123-45-6789", 1, ((10, 21),)), - - # True negatives - invalid formats + ("SSN: 456-78-9012", 1, ((5, 16),)), + ("My SSN is 456-78-9012", 1, ((10, 21),)), + + # True negatives - invalid formats ("SSN: 000-00-0000", 0, ()), # Invalid area ("SSN: 666-12-3456", 0, ()), # Excluded area - + ("SSN: 123-45-6789", 0, ()), # Well-known sample SSN, denylisted + # Boundary testing - embedded in text - ("Contact: 123-45-6789 for info", 1, ((9, 20),)), - - # False positive prevention - ("ISBN: 123-45-6789", 0, ()), # Different context + ("Contact: 456-78-9012 for info", 1, ((9, 20),)), ]) def test_ssn_detection(text, expected_len, expected_positions, recognizer): results = recognizer.analyze(text, ["US_SSN"]) @@ -138,6 +302,60 @@ def test_ssn_detection(text, expected_len, expected_positions, recognizer): assert result.end == end ``` +⚠️ Pick example values that the recognizer actually accepts. Well-known sample values +(`123-45-6789`, `078-05-1120`) are denylisted by `UsSsnRecognizer`, so a test using them +as true positives fails, and one using them as a false-positive case passes for the +wrong reason. + +**Assert exact scores, not ranges.** A range assertion passes even when the logic that +produces the score breaks entirely. + +```python +# ❌ BAD: still passes if checksum promotion stops working +assert 0.5 <= result.score <= 1.0 + +# ✅ GOOD: pins the behavior under test +assert result.score == pytest.approx(EntityRecognizer.MAX_SCORE) +``` + +**Include a lookalike negative.** The false-positive surface is the thing worth testing, +not the happy path. Add a case proving that a plausible non-PII token of the same shape +is not flagged: a 17-character order ID for a VIN, a legal citation for a bank account +number, a build tag for an alphanumeric member ID. + +**Exercise context enhancement.** A recognizer that defines `CONTEXT` needs a test +showing the score changes between text with and without a context word. A suite that +never triggers the enhancer does not test the context words at all. + +### Backward Compatibility + +Presidio is a library. Changes to shared classes alter results for users who have +written no new code. Before changing anything outside a new file, state in the PR +description what existing behavior changes. + +These count as behavior changes even without a signature change: +- Default values on shared base classes (`None` to `[]` changes truthiness for every + subclass) +- Properties on abstract interfaces, since custom implementations inherit the new + default and may break +- Scores, context lists, or patterns on existing recognizers +- Anything altering which entities are returned for text that previously worked + +⚠️ Do not modify an existing recognizer's patterns, scores, or context as a side effect +of adding a new one. Users depend on current detection behavior. + +**Surface new scoring inputs in explainability.** Anything that changes how a score is +derived (context, negative context, thresholds) must be reflected in +`AnalysisExplanation`, or users cannot tell why a result scored the way it did. + +**Prefer warnings over exceptions when the caller cannot fix the condition.** Raising +on a configuration a user did not write, and cannot change, turns a degraded result into +a hard failure. Where a lookup falls back to a default instead of failing, add a debug +log so the fallback is discoverable. + +**Prefer a property on the base class over a maintained list of class names.** Lists +drift as recognizers are added, and users installing from PyPI cannot extend them. + ### Implementing New Anonymizers (Operators) **1. Implement the Operator Interface:** @@ -479,6 +697,10 @@ Focus on issues in this order of importance: - **Missing multilingual tests** - Recognizers claiming multi-language support without language-specific tests - **Anonymization reversibility not tested** - No verification that anonymized data can't be de-anonymized - **Missing E2E analyzer→anonymizer tests** - Testing components in isolation without integration validation +- **No configuration-path test for a new recognizer** - Tests construct the recognizer in Python only. Recognizers shipping `enabled: false` are never exercised through `RecognizerRegistryProvider`, which is how users enable them. Require one test that enables the recognizer in a YAML config and asserts detection +- **Construction paths that disagree** - Behavior differs depending on whether a recognizer is built directly, added via `registry.add_recognizer()`, or loaded from configuration. Flag defaulting or validation logic applied on one path but not the others +- **Score assertions using ranges** - `assert 0.5 <= score <= 1.0` passes even when checksum promotion or context enhancement breaks. Require exact assertions +- **No lookalike negative** - Tests cover valid values and malformed values, but not a plausible non-PII token of the same shape, which is the actual false-positive surface **General Testing:** - Missing tests for critical business logic (PII detection, anonymization) @@ -493,6 +715,10 @@ Focus on issues in this order of importance: - Implementation must not contradict existing documentation - if conflict exists, either update docs or reconsider implementation - API documentation is auto-generated from docstrings - formatting errors break the build +**Terminology:** +- Use "threshold", not "cutoff", to match the codebase +- Use ISO 639-1 language codes in docs and configuration examples + **Docstring Quality:** - All public classes, methods, and functions must have docstrings - Docstrings must follow consistent format (Args, Returns, Raises, Examples) @@ -570,7 +796,7 @@ Use atomic grouping: (?>a+)b or possessive quantifier a++b" ## Part 3: Repository-Specific Context ### Technology Stack -- **Python** - Must support all versions +- **Python** - `requires-python = ">=3.10,<3.15"`. Code must run on every version in that range - **uv** - Dependency management and installation (not pip or Poetry). Each package commits a `uv.lock`; `poetry-core` is retained only as the build backend for now. - **Ruff** - Linting and formatting (replaces flake8, black, isort) - **spaCy** - Default NLP engine (en_core_web_lg for production), although one can use other NLP engines via provider pattern @@ -638,6 +864,11 @@ pytest -v # Run all E2E tests - **AHDS test skips** - Expected when AHDS_ENDPOINT not set - **Transformers test failures** - Expected without HuggingFace access in restricted environments +### Configuration Issues +- **Recognizer enabled in YAML but never loads** - Its declared languages are not in the top-level `supported_languages`, which defaults to `["en"]`. The loader drops it silently +- **`TypeError: unexpected keyword argument` on registry construction** - The recognizer's `__init__` does not accept a key the loader passes through from the YAML entry, most often `name` +- **Class defaults missing after loading from YAML** - Check whether the loader assigns the value after construction rather than passing it to `__init__` + ### Code Issues - **Logging PII values** - Never log `entity.text`, only `entity.entity_type` - **Hardcoded language assumptions** - Use `context.language` parameter @@ -671,4 +902,4 @@ contributions. --- -**Summary for Code Review**: Prioritize security (PII leakage), correctness (detection accuracy), and performance (regex efficiency). Ensure comprehensive testing for all recognizers. Let automated tools handle formatting. Focus on actionable, specific feedback with concrete fixes. \ No newline at end of file +**Summary for Code Review**: Prioritize security (PII leakage), correctness (detection accuracy), and performance (regex efficiency). Ensure comprehensive testing for all recognizers. Let automated tools handle formatting. Focus on actionable, specific feedback with concrete fixes. diff --git a/.github/instructions/recognizer-review.instructions.md b/.github/instructions/recognizer-review.instructions.md new file mode 100644 index 0000000000..17d10a90da --- /dev/null +++ b/.github/instructions/recognizer-review.instructions.md @@ -0,0 +1,84 @@ +--- +applyTo: >- + presidio-analyzer/presidio_analyzer/predefined_recognizers/**, + presidio-analyzer/presidio_analyzer/conf/default_recognizers.yaml, + presidio-analyzer/tests/**/test_*recognizer*.py +--- + +# Copilot code review — recognizer & core changes + +These instructions apply only to code review of recognizer and shared-analyzer +changes. Give specific, actionable feedback: cite the file and line and propose +the concrete fix. Do not comment on formatting — Ruff and CI own that. + +## Highest-priority checks (lead the review with these) + +1. **Configuration-path test is required.** Most predefined recognizers ship + `enabled: false`, so Python-only tests never construct them the way users do — + by flipping `enabled: true` in a registry YAML. If a PR adds or changes a + recognizer but only builds it directly in Python, request a test that loads it + through `RecognizerRegistryProvider` (write a small YAML config with the + recognizer `enabled: true`, build the registry, run `AnalyzerEngine.analyze`, + assert the entity is returned). This is the check that catches + `TypeError: __init__() got an unexpected keyword argument 'name'` — the loader + passes the YAML `name` key to the constructor, and a recognizer that doesn't + accept it crashes the whole registry when enabled. +2. **Construction paths must agree.** Direct construction, `add_recognizer()`, and + config loading must yield the same recognizer. Flag defaulting or validation + applied on one path but not the others. +3. **Undeclared backward-incompatible changes.** Presidio is a library; changes to + shared base classes alter results for users who wrote no new code. If the PR + edits anything outside a brand-new file (base-class defaults, interface + properties, or scores/patterns/context on an *existing* recognizer), require the + PR description to state what existing behavior changes. Flag edits to an existing + recognizer's patterns/scores/context made as a side effect of adding a new one. + +## Recognizer-specific checks + +- **Language vs. country codes.** `supported_language` and the YAML + `supported_languages` key take ISO 639-1 language codes (`ko`), not country codes + (`kr`); a mismatch loads nothing, silently. The top-level `supported_languages` + in a config defaults to `["en"]` and filters everything else out — non-English + recognizers must set it in tests and note it in the PR. +- **Score calibration.** The base score should reflect how much the pattern alone + narrows the space: ~0.05–0.1 for bare digit/alphanumeric runs, 0.1–0.3 for some + structure, 0.3–0.5 for a distinctive format, 0.5+ for strong. Flag a high score + on a generic pattern (a 0.3 that also matches `covid19`/`sha256` is overstated). +- **`validate_result` promotion.** Returning `True` replaces the score with 1.0 — + full confidence, not a nudge. Flag `return True` on a check that arbitrary + same-shape tokens pass at a meaningful rate (a mod-11 check on a 17-char token + passes ~9% of the time). Require `None` (not `False`) when the check doesn't + apply; `False` discards the result. A missing checksum is fine (~40% of + recognizers have none) — don't ask for an invented one. +- **Context words.** Context is matched as substrings by default + (`context_matching_mode="substring"` in `LemmaContextAwareEnhancer`), so short + words fire on unrelated tokens (`member`→`remember`, `auth`→`author`). Prefer + unambiguous multi-word context. Context is prefix-only by default; a word after + the match doesn't boost. Don't require context to fire — `presidio-structured` + has no surrounding text; suppress with thresholds instead. +- **Enabled-by-default.** Default to `enabled: false`. The test for shipping + enabled is whether the recognizer can produce *high-confidence* false positives + (a coincidental match at a score the user can't filter), not which country it + belongs to. +- **Companion updates.** A new recognizer needs its exports in the + `predefined_recognizers/__init__.py` and country `__init__.py`, an entry in + `default_recognizers.yaml`, and a row in `docs/supported_entities.md`. New + directories use the full lowercase country name (`south_africa`); pre-existing + short forms (`us`, `uk`, `thai`) should not be imitated. + +## Test-quality checks + +- **Assert exact scores, not ranges.** `assert 0.5 <= score <= 1.0` still passes + when checksum promotion or context enhancement breaks. Require + `== pytest.approx(...)`. +- **Require a lookalike negative** — a plausible non-PII token of the same shape + (a 17-char order ID for a VIN, a legal citation for a bank number) asserted as + *not* flagged. This is the actual false-positive surface. +- **Require a context-enhancement test** for any recognizer defining `CONTEXT`: + the score must differ between text with and without a context word. +- **Reject denylisted example values** used as true positives (e.g. `123-45-6789` + is denylisted by `UsSsnRecognizer`). + +## Terminology + +Say "threshold", not "cutoff". Use ISO 639-1 language codes in examples.