Formal Copilot review instructions + cross-agent guidelines (recognizers, YAML config, general practices)

[omri374]

Change Description

Restructures Presidio's AI guidance into the formal layout GitHub recommends for repository custom instructions, so that (a) the Copilot PR review agent gives accurate, path-scoped feedback before a human reviewer arrives, and (b) coding agents (Claude Code, Cursor, Copilot coding agent) follow the same rules at authoring time.

File	Scope	Content
.github/copilot-instructions.md	repo-wide	General engineering practices: backward compatibility, security/PII rules, cross-component boundaries, testing and documentation standards, review posture
.github/instructions/recognizers.instructions.md	recognizer code, default_recognizers.yaml, recognizer tests	Adding/modifying PII recognizers: configuration-path testing, score calibration, validation hooks, context words, companion updates
.github/instructions/yaml-config.instructions.md	input_validation/, recognizer_registry/, conf/, config tests	The pydantic YAML→instance layer: schema/constructor sync, extra and exclude_none discipline, parse-time validation with actionable messages, schema backward compatibility
AGENTS.md (+ CLAUDE.md importing it)	coding agents	The same rules framed for authoring time — e.g. "write the configuration-path test" rather than "flag its absence"

Copilot code review reads the repo-wide file plus whichever path-scoped files match the diff, so a docs-only PR is not reviewed against recognizer checklists. Since July 2026 it reads instructions from the PR's head branch, so this PR's own Copilot review runs under these instructions — a built-in validation pass.

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.

The YAML-configuration instructions are grounded in the input_validation layer's real failure modes: PredefinedRecognizerConfig silently ignores unknown YAML keys (pydantic's extra="ignore" default), so a constructor kwarg without a schema field is dropped with no error — the gap LangExtractRecognizerConfig exists to close — and kwargs models must dump with exclude_none=True or omitted YAML fields clobber constructor defaults.

What changed

• Restructured the previous single 900-line copilot-instructions.md into the four files above; recognizer and configuration depth moved to path-scoped files, general practices stayed repo-wide. Guidance corrected in earlier revisions of this PR (score bands, substring context matching, enabled-by-default criteria, denylisted SSN sample values, exact-score assertions, backward-compatibility section) carries over into the new files.
• New: YAML/pydantic-layer instructions (no coverage existed before this PR).
• New: AGENTS.md + CLAUDE.md so coding agents get authoring-time guidance; previously the repo had none.
• Removed: .claude/skills/recognizer-pr-review/ (superseded by AGENTS.md + the instructions files, which Copilot code review and Claude Code both read).

Follow-ups, not in this PR

1. The three broken recognizers. Each needs name: Optional[str] = None in its constructor. Small and separable.
2. 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.
3. The CHANGELOG rule. The instructions state 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

[github-actions]

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]

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]

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

[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]

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]

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
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
Project Total

The report is truncated to 25 files out of 80. 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)

[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.

[Copilot]

Pull request overview

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

Suppressed comments (9)

AGENTS.md:66

• The top-level language filter emits a warning when it skips an enabled recognizer; it does not filter silently. This should match the loader behavior so authoring agents do not infer that no diagnostic is available.

   instructions file). Non-English recognizers must set the top-level
   `supported_languages` in the test config — it defaults to `["en"]` and
   silently filters everything else.

.github/instructions/recognizers.instructions.md:77

• The loader logs a warning when a recognizer's language is excluded by the global set, so describing this as silent is factually incorrect and can trigger false review feedback about missing diagnostics.

**Non-English recognizers:** the top-level `supported_languages` key acts as a
global filter and the shipped default is `["en"]`. A recognizer supporting only
`de` will not load from that config — silently. The test config and the PR
description must state the required top-level languages.

.github/instructions/recognizers.instructions.md:55

• This template fails when adapted to the required non-English case: AnalyzerEngine defaults its own languages to ["en"] and rejects a registry configured for (for example) ["de"]. Pass the registry languages explicitly so the mandated configuration-path test works for every language.

This issue also appears on line 74 of the same file.

    analyzer = AnalyzerEngine(registry=registry, nlp_engine=nlp_engine)

.github/instructions/yaml-config.instructions.md:58

• Nested config model_dump() overrides are not invoked when RecognizerRegistryConfig serializes them; the provider instead strips None centrally in _prepare_recognizer_kwargs (lines 295–299). Requiring per-model overrides would therefore produce false review findings; document the actual centralized invariant and its provider-path test.

Models whose dump is passed to a constructor override `model_dump` with
`exclude_none=True`, so a field omitted in YAML preserves the constructor
default instead of overriding it with an explicit `None`. Any new pass-through
config model must do the same; flag one that doesn't — it silently clobbers
constructor defaults, which is this layer's sneakiest backward-compatibility

AGENTS.md:47

• The language mismatch is not silent: _is_language_supported_globally emits a warning naming the recognizer and both language sets before skipping it. Correcting this avoids teaching agents to report a missing diagnostic that already exists.

This issue also appears on line 64 of the same file.

2. **Use ISO 639-1 language codes** (`ko` for Korean, never `kr`) — a
   mismatch loads nothing, silently.

AGENTS.md:51

• supported_entity is not a universally required constructor kwarg: _prepare_recognizer_kwargs deliberately adapts singular/plural entity fields to the inspected signature. Requiring every recognizer to accept the singular form conflicts with valid multi-entity constructors; list only the applicable loader fields.

3. **Make the constructor loader-compatible**: accept the YAML loader's
   kwargs (`name`, `supported_entity`, `context`, ...) and forward them to
   the base class, or the recognizer crashes the whole registry the moment a
   user enables it.

.github/copilot-instructions.md:90

• This blanket rule contradicts Presidio's supported encrypt operator and the hash operator's documented, explicit fixed-salt mode for referential integrity. It would make reviewers flag valid opt-in use cases; require a secure non-reversible default while separately scrutinizing documented reversible or deterministic opt-ins.

- **Reversible or weak anonymization** — deterministic hashing is reversible
  via rainbow tables; use random/unpredictable replacement values that don't
  preserve PII characteristics.

.github/copilot-instructions.md:165

• This E2E command fails on a clean checkout: uv sync created an unactivated analyzer environment, while e2e-tests has only its own requirements.txt, so plain pytest and its dependencies are not installed. Include the E2E environment setup documented in docs/development.md:162-176.

# E2E
docker compose up --build -d && cd e2e-tests && pytest -v

.github/copilot-instructions.md:99

• The stated adversarial input actually matches, so it does not force the nested quantifiers to exhaust alternatives. The exponential behavior appears on a long non-match such as aaaa...c; using that example is important so agents write an effective regression test.

- Avoid catastrophic regex backtracking (`(a+)+b` is O(2^n) on `aaaa...b`);
  test patterns against long adversarial strings.