mirror of
https://github.com/data-privacy-stack/presidio.git
synced 2026-09-21 05:27:53 -05:00
Formal Copilot review instructions + cross-agent guidelines (recognizers, YAML config, general practices) (#2211)
This commit is contained in:
+133
-637
@@ -1,674 +1,170 @@
|
||||
# Presidio Development Instructions
|
||||
# Presidio Development & Review Instructions
|
||||
|
||||
Presidio is a Python-based data protection and de-identification SDK with multiple components for detecting and anonymizing PII (Personally Identifiable Information) in text and images.
|
||||
Presidio is a Python-based data protection and de-identification SDK with
|
||||
multiple components for detecting and anonymizing PII (Personally Identifiable
|
||||
Information) in text and images.
|
||||
|
||||
Domain-specific rules live in path-scoped instruction files and apply on top of
|
||||
this file when a change touches the matching paths:
|
||||
|
||||
- `.github/instructions/recognizers.instructions.md` — adding or modifying PII
|
||||
recognizers.
|
||||
- `.github/instructions/yaml-config.instructions.md` — the pydantic layer that
|
||||
translates YAML configuration into Presidio instances.
|
||||
|
||||
## Core Philosophy
|
||||
|
||||
**Data Privacy is Paramount** - This is a PII detection and anonymization system used in sensitive contexts. Security and correctness are non-negotiable.
|
||||
**Data privacy is paramount.** This is a PII detection and anonymization system
|
||||
used in sensitive contexts; security and correctness are non-negotiable.
|
||||
|
||||
**Key Principles:**
|
||||
- **Accuracy First**: False negatives (missed PII) and false positives (incorrect detections) both damage trust
|
||||
- **Security by Default**: Never log PII values, use non-reversible anonymization, validate all inputs
|
||||
- **Cross-Component Awareness**: Presidio is a multi-component system - changes ripple across boundaries
|
||||
- **Stateless Design**: Presidio is designed for scalability - avoid adding unnecessary state
|
||||
- **Documentation Integrity**: Code and docs must stay synchronized - outdated docs are dangerous
|
||||
- **Accuracy first**: false negatives (missed PII) and false positives
|
||||
(incorrect detections) both damage trust.
|
||||
- **Security by default**: never log PII values, use non-reversible
|
||||
anonymization, validate all inputs.
|
||||
- **Presidio is a library**: changes to shared code alter results for users who
|
||||
wrote no new code. Backward compatibility is the prime directive.
|
||||
- **Stateless design**: modules that process records are stateless for
|
||||
scalability — avoid adding state.
|
||||
- **Cross-component awareness**: Presidio is a multi-component system; changes
|
||||
ripple across boundaries.
|
||||
- **Documentation integrity**: code and docs must stay synchronized — outdated
|
||||
docs are dangerous.
|
||||
|
||||
---
|
||||
## Backward Compatibility
|
||||
|
||||
## Part 1: Implementation Guidelines
|
||||
Before changing anything outside a brand-new file, the PR description must
|
||||
state what existing behavior changes. These count as behavior changes even
|
||||
without a signature change:
|
||||
|
||||
Use these guidelines when **generating or writing code** for Presidio.
|
||||
- Default values on shared base classes (`None` to `[]` changes truthiness for
|
||||
every subclass).
|
||||
- Properties on abstract interfaces — custom implementations inherit the new
|
||||
default and may break.
|
||||
- Anything altering which entities are returned, or their scores, for text that
|
||||
previously worked.
|
||||
|
||||
### Presidio Architecture Patterns
|
||||
Prefer additive changes: new parameters get defaults preserving current
|
||||
behavior; public APIs are never broken without a deprecation path.
|
||||
|
||||
**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 as it did.
|
||||
|
||||
**Prefer warnings over exceptions when the caller cannot fix the condition.**
|
||||
Raising on a configuration a user did not write turns a degraded result into a
|
||||
hard failure. Where a lookup falls back to a default, add a debug log so the
|
||||
fallback is discoverable.
|
||||
|
||||
**Data Flow (Unidirectional):**
|
||||
```
|
||||
Analyzer (detect PII) → Anonymizer (transform PII) → Output
|
||||
↓
|
||||
nlp_engine → recognizers → context
|
||||
```
|
||||
**Prefer a property on the base class over a maintained list of class names.**
|
||||
Lists drift as classes are added, and users installing from PyPI cannot extend
|
||||
them.
|
||||
|
||||
**Design Patterns:**
|
||||
- **Registry Pattern**: `RecognizerRegistry` for dynamic recognizer management
|
||||
- **Provider Pattern**: `NlpEngineProvider`, `RecognizerRegistryProvider` for configuration
|
||||
## Cross-Component Changes
|
||||
|
||||
### Implementing New Recognizers
|
||||
Data flows one way: Analyzer → Anonymizer → Output. Downstream components
|
||||
(CLI, structured, image-redactor) consume analyzer/anonymizer, never the
|
||||
reverse.
|
||||
|
||||
**1. Choose the Right Base Class:**
|
||||
```python
|
||||
from presidio_analyzer import PatternRecognizer, LocalRecognizer, RemoteRecognizer
|
||||
- Shared data models (`RecognizerResult`, `OperatorConfig`) are contracts —
|
||||
changes require coordinated updates across all consumers, in the same
|
||||
changeset.
|
||||
- Reuse by importing from shared modules, not by copying code across
|
||||
components. If multiple components need a feature, extract it to a common
|
||||
location.
|
||||
- Respect boundaries: a component imports another's public interface, never its
|
||||
internals (e.g. the anonymizer must not import
|
||||
`presidio_analyzer.predefined_recognizers`).
|
||||
- Registry and provider patterns exist to decouple components — bypassing them
|
||||
creates hidden dependencies.
|
||||
- Test the complete integration path (unit, integration, and e2e), not just
|
||||
isolated components.
|
||||
|
||||
# For regex-based detection
|
||||
class MyPatternRecognizer(PatternRecognizer):
|
||||
pass
|
||||
## Security & Privacy
|
||||
|
||||
# For custom logic (NLP, ML)
|
||||
class MyCustomRecognizer(LocalRecognizer):
|
||||
def load(self): ...
|
||||
def analyze(self, text, entities, nlp_artifacts): ...
|
||||
Always flag:
|
||||
|
||||
# For calling remote services
|
||||
class MyRemoteRecognizer(RemoteRecognizer):
|
||||
def analyze(self, text, entities, nlp_artifacts): ...
|
||||
```
|
||||
- **PII leakage in logs, errors, or debug output** — log entity types and
|
||||
positions, never `entity.text`.
|
||||
- **Reversible or weak anonymization** — deterministic hashing is reversible
|
||||
via rainbow tables; use random/unpredictable replacement values that don't
|
||||
preserve PII characteristics.
|
||||
- Regex injection: user-provided patterns must be validated before compilation.
|
||||
- Hardcoded secrets or credentials; unsafe deserialization (pickle, untrusted
|
||||
models); command injection; path traversal; missing input validation on API
|
||||
endpoints (including unbounded input sizes).
|
||||
|
||||
**2. Predefined Recognizers Location Matters:**
|
||||
- Country-specific: `presidio-analyzer/presidio_analyzer/predefined_recognizers/country_specific/{country}/`
|
||||
- Generic patterns: `.../predefined_recognizers/generic/`
|
||||
- NLP/ML-based: `.../predefined_recognizers/nlp_engine_recognizers/` or `.../ner/`
|
||||
- Third-party: `.../predefined_recognizers/third_party/`
|
||||
## Performance
|
||||
|
||||
**3. Pattern Design Best Practices:**
|
||||
```python
|
||||
# ❌ BAD: Too broad - matches month names as persons
|
||||
pattern = r"\b[A-Z][a-z]+\b"
|
||||
- Avoid catastrophic regex backtracking (`(a+)+b` is O(2^n) on `aaaa...b`);
|
||||
test patterns against long adversarial strings.
|
||||
- Cache compiled regexes; don't recompile per call.
|
||||
- Batch NLP processing (`nlp.pipe`) instead of per-text calls; load models
|
||||
once and reuse.
|
||||
- Flag O(n²) where O(n) exists, blocking I/O on API paths, and loading entire
|
||||
datasets into memory.
|
||||
|
||||
# ✅ GOOD: Specific pattern with context
|
||||
PATTERNS = [
|
||||
Pattern(
|
||||
"SSN",
|
||||
r"\b\d{3}-\d{2}-\d{4}\b",
|
||||
0.3 # Low base score, context will boost
|
||||
)
|
||||
]
|
||||
## Testing Standards
|
||||
|
||||
CONTEXT = ["ssn", "social security", "tax id"]
|
||||
```
|
||||
- Test names describe behavior:
|
||||
`test_when_invalid_checksum_then_no_match`, not `test_case2`.
|
||||
- Assert exact expected values, not ranges — a range assertion passes even when
|
||||
the logic producing the value breaks.
|
||||
- Cover true positives, true negatives, edge cases, and values embedded in
|
||||
surrounding text; validate exact boundaries, not just types.
|
||||
- Fix random seeds for non-deterministic NLP/ML tests.
|
||||
- Test behavior, not implementation details.
|
||||
|
||||
**4. Document Pattern Sources:**
|
||||
```python
|
||||
"""
|
||||
Recognizes US Social Security Numbers.
|
||||
## Documentation
|
||||
|
||||
Pattern based on SSA Publication No. 05-10633:
|
||||
https://www.ssa.gov/history/ssn/geocard.html
|
||||
When adding features, update all that apply: `docs/supported_entities.md` (new
|
||||
entity types), `docs/api-docs/api-docs.yml` (API changes), `README.md` (major
|
||||
features), docstrings (all public classes/methods, reST format — `:param:`,
|
||||
`:return:`, `:raises:`), and `docs/samples/` (complex features).
|
||||
|
||||
Validation uses SSN format rules: AAA-GG-SSSS
|
||||
- AAA: Area number (001-899, excluding 666)
|
||||
- GG: Group number (01-99)
|
||||
- SSSS: Serial number (0001-9999)
|
||||
"""
|
||||
```
|
||||
Do not update `CHANGELOG.md` in a PR: current-release entries are generated
|
||||
from merged PRs before each version bump, and per-PR edits create merge
|
||||
conflicts.
|
||||
|
||||
**5. Required Configuration Updates:**
|
||||
```python
|
||||
# Update all of these:
|
||||
# 1. presidio_analyzer/predefined_recognizers/__init__.py
|
||||
from .country_specific.us.my_recognizer import MyRecognizer
|
||||
__all__ = [..., "MyRecognizer"]
|
||||
Terminology: use "threshold", not "cutoff"; use ISO 639-1 language codes in
|
||||
docs and configuration examples.
|
||||
|
||||
# 2. presidio_analyzer/predefined_recognizers/country_specific/us/__init__.py
|
||||
from .my_recognizer import MyRecognizer
|
||||
__all__ = [..., "MyRecognizer"]
|
||||
## Code Review Guidelines
|
||||
|
||||
# 3. presidio_analyzer/conf/default_recognizers.yaml
|
||||
recognizers:
|
||||
- name: MyRecognizer
|
||||
supported_languages: ["en"]
|
||||
type: predefined
|
||||
enabled: false # Country-specific defaults to false
|
||||
- Only comment with HIGH CONFIDENCE (>80%) that an issue exists; be concise and
|
||||
actionable — cite the line and propose the concrete fix.
|
||||
- Check for existing patterns and helpers in the codebase before suggesting new
|
||||
approaches.
|
||||
- Severity: 🔴 security/PII leakage and correctness → 🟡 performance,
|
||||
cross-component breaks, missing tests → 💡 code quality.
|
||||
|
||||
# 4. docs/supported_entities.md (add row to appropriate table)
|
||||
```
|
||||
**Do not flag** (automated tools own these): formatting, line length, import
|
||||
order (Ruff); type-hint style (`List[str]` vs `list[str]`); style preferences
|
||||
or speculative abstractions that don't fix bugs or improve accuracy.
|
||||
|
||||
**6. 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),)),
|
||||
## Repository Context
|
||||
|
||||
# True negatives - invalid formats
|
||||
("SSN: 000-00-0000", 0, ()), # Invalid area
|
||||
("SSN: 666-12-3456", 0, ()), # Excluded area
|
||||
- **Python** `>=3.10,<3.15` — code must run on every version in range.
|
||||
- **uv** for dependency management (not pip/Poetry); each package commits a
|
||||
`uv.lock`. Whenever a package's `pyproject.toml` dependencies change,
|
||||
regenerate and commit that package's `uv.lock` in the same change
|
||||
(`cd <package> && uv lock`) — CI installs with `uv sync --locked` and fails
|
||||
on drift.
|
||||
- **Ruff** for linting and formatting; **spaCy** as the default NLP engine
|
||||
(swappable via provider pattern); **Docker** images on
|
||||
`ghcr.io/data-privacy-stack`.
|
||||
|
||||
# Boundary testing - embedded in text
|
||||
("Contact: 123-45-6789 for info", 1, ((9, 20),)),
|
||||
|
||||
# False positive prevention
|
||||
("ISBN: 123-45-6789", 0, ()), # Different context
|
||||
])
|
||||
def test_ssn_detection(text, expected_len, expected_positions, recognizer):
|
||||
results = recognizer.analyze(text, ["US_SSN"])
|
||||
assert len(results) == expected_len
|
||||
for result, (start, end) in zip(results, expected_positions):
|
||||
assert result.start == start
|
||||
assert result.end == end
|
||||
```
|
||||
|
||||
### Implementing New Anonymizers (Operators)
|
||||
|
||||
**1. Implement the Operator Interface:**
|
||||
```python
|
||||
from presidio_anonymizer.operators import Operator, OperatorType
|
||||
|
||||
class MyOperator(Operator):
|
||||
"""Custom anonymization operator."""
|
||||
|
||||
def operate(self, text: str, params: dict = None) -> str:
|
||||
"""Transform the detected PII."""
|
||||
# Ensure non-reversible transformation
|
||||
import uuid
|
||||
return f"<{params.get('entity_type', 'REDACTED')}_{uuid.uuid4().hex[:8]}>"
|
||||
|
||||
def validate(self, params: dict = None) -> None:
|
||||
"""Validate operator parameters before use."""
|
||||
if params and 'entity_type' not in params:
|
||||
raise ValueError("entity_type is required")
|
||||
|
||||
def operator_name(self) -> str:
|
||||
return "my_operator"
|
||||
|
||||
def operator_type(self) -> OperatorType:
|
||||
return OperatorType.Anonymize
|
||||
```
|
||||
|
||||
**2. Security Checklist:**
|
||||
- ✅ **Non-reversible**: Cannot recover original PII from anonymized output
|
||||
- ✅ **Entropy**: Uses random/unpredictable values (not deterministic hashing)
|
||||
- ✅ **No PII leakage**: Doesn't preserve PII characteristics (length, format)
|
||||
|
||||
```python
|
||||
# ❌ BAD: Reversible via rainbow tables
|
||||
def operate(self, text, params):
|
||||
return hashlib.md5(text.encode()).hexdigest()
|
||||
|
||||
# ✅ GOOD: Non-reversible with entropy
|
||||
def operate(self, text, params):
|
||||
return f"<{params['entity_type']}_{uuid.uuid4().hex[:8]}>"
|
||||
```
|
||||
|
||||
**3. Test Anonymization Quality:**
|
||||
```python
|
||||
def test_operator_is_non_reversible():
|
||||
"""Verify same input produces different output."""
|
||||
operator = MyOperator()
|
||||
result1 = operator.operate("John Doe", {"entity_type": "PERSON"})
|
||||
result2 = operator.operate("John Doe", {"entity_type": "PERSON"})
|
||||
assert result1 != result2 # Different each time
|
||||
|
||||
def test_operator_preserves_structure():
|
||||
"""Verify anonymized text maintains sentence structure."""
|
||||
text = "Email: john@example.com, Phone: 555-1234"
|
||||
# After anonymization
|
||||
expected = "Email: <EMAIL_xxx>, Phone: <PHONE_yyy>"
|
||||
# Structure preserved, PII replaced
|
||||
```
|
||||
|
||||
### API Development
|
||||
|
||||
**1. Maintain Backward Compatibility:**
|
||||
```python
|
||||
# ❌ BAD: Breaking change
|
||||
def analyze(text: str, language: str, entities: List[str]):
|
||||
...
|
||||
|
||||
# ✅ GOOD: Optional parameter with default
|
||||
def analyze(
|
||||
text: str,
|
||||
language: str,
|
||||
entities: Optional[List[str]] = None
|
||||
) -> List[RecognizerResult]:
|
||||
...
|
||||
```
|
||||
|
||||
**2. Required Updates for API Changes:**
|
||||
```bash
|
||||
# 1. Update OpenAPI schema
|
||||
docs/api-docs/api-docs.yml
|
||||
|
||||
# 2. Add E2E tests
|
||||
e2e-tests/tests/test_new_endpoint.py
|
||||
|
||||
# 3. Add usage example
|
||||
docs/samples/python/new_feature_example.ipynb
|
||||
|
||||
```
|
||||
|
||||
### Cross-Component Changes
|
||||
|
||||
**When modifying shared interfaces:**
|
||||
|
||||
1. **Identify all consumers:**
|
||||
```python
|
||||
# RecognizerResult is consumed by:
|
||||
# - presidio-anonymizer (takes analyzer results)
|
||||
# - presidio-cli (displays results)
|
||||
# - presidio-structured (processes tabular data)
|
||||
# - docs/samples/* (user examples)
|
||||
```
|
||||
|
||||
2. **Update all components in same changeset:**
|
||||
```python
|
||||
# If adding field to RecognizerResult:
|
||||
# 1. presidio-analyzer: Add field and populate
|
||||
# 2. presidio-anonymizer: Handle new field (or ignore safely)
|
||||
# 3. presidio-cli: Display new field (optional)
|
||||
# 4. Tests: Update expectations
|
||||
# 5. Docs: Document new field
|
||||
# 6. e2e-tests: Add integration test for new field
|
||||
```
|
||||
|
||||
3. **Respect component boundaries:**
|
||||
```python
|
||||
# ❌ BAD: Anonymizer importing analyzer internals
|
||||
from presidio_analyzer.predefined_recognizers import UsSsnRecognizer
|
||||
|
||||
# ✅ GOOD: Use public interfaces only
|
||||
from presidio_analyzer import RecognizerResult
|
||||
```
|
||||
|
||||
### Performance Optimization
|
||||
|
||||
**1. Cache Compiled Regexes:**
|
||||
```python
|
||||
from functools import lru_cache
|
||||
|
||||
@lru_cache(maxsize=128)
|
||||
def _compile_pattern(pattern_str: str) -> re.Pattern:
|
||||
return re.compile(pattern_str, re.IGNORECASE)
|
||||
```
|
||||
|
||||
**2. Avoid Catastrophic Backtracking:**
|
||||
```python
|
||||
# ❌ BAD: O(2^n) on "aaaa...ab"
|
||||
pattern = r"(a+)+"
|
||||
|
||||
# ✅ GOOD: Atomic grouping
|
||||
pattern = r"(?>a+)"
|
||||
```
|
||||
|
||||
**3. Batch NLP Processing:**
|
||||
```python
|
||||
# ❌ BAD: Process one at a time
|
||||
for text in texts:
|
||||
doc = nlp(text)
|
||||
...
|
||||
|
||||
# ✅ GOOD: Use spaCy pipe for batching
|
||||
for doc in nlp.pipe(texts, batch_size=50):
|
||||
...
|
||||
```
|
||||
|
||||
### Testing Requirements
|
||||
|
||||
**Test Naming Convention:**
|
||||
```python
|
||||
# ✅ GOOD: Descriptive, intention-revealing
|
||||
def test_when_valid_ssn_then_detect_with_correct_boundaries()
|
||||
def test_when_invalid_checksum_then_no_match()
|
||||
def test_when_context_missing_then_low_confidence()
|
||||
|
||||
# ❌ BAD: Non-descriptive
|
||||
def test_ssn_1()
|
||||
def test_case2()
|
||||
```
|
||||
|
||||
### Documentation Requirements
|
||||
|
||||
**1. Required Documentation Updates:**
|
||||
```markdown
|
||||
When adding a feature, update ALL of:
|
||||
|
||||
✅ docs/supported_entities.md - For new entity types
|
||||
✅ docs/api-docs/api-docs.yml - For API changes
|
||||
✅ README.md - For major features
|
||||
✅ Docstrings - All public classes/methods
|
||||
✅ docs/samples/ - Usage examples for complex features
|
||||
✅ Update docstrings based on the reST docstring format (:param:, :return:, :raises:, :example:)
|
||||
```
|
||||
|
||||
Do not update `CHANGELOG.md` in a PR. Before each version bump, changelog
|
||||
entries for the current release are generated from merged PRs. Per-PR changelog
|
||||
edits create unnecessary merge conflicts.
|
||||
|
||||
**2. Pattern Source Documentation:**
|
||||
```python
|
||||
# In recognizer docstring or comments
|
||||
"""
|
||||
Pattern based on Royal Mail PAF specification:
|
||||
https://www.royalmail.com/find-a-postcode
|
||||
|
||||
UK postcodes follow 6 formats:
|
||||
- A9 9AA (e.g., M1 1AA)
|
||||
- A99 9AA (e.g., M60 1NW)
|
||||
- AA9 9AA (e.g., CR2 6XH)
|
||||
- AA99 9AA (e.g., DN55 1PT)
|
||||
- A9A 9AA (e.g., W1A 1HQ)
|
||||
- AA9A 9AA (e.g., EC1A 1BB)
|
||||
|
||||
Plus special case: GIR 0AA
|
||||
"""
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Part 2: Code Review Guidelines
|
||||
|
||||
Use these guidelines when **reviewing pull requests** for Presidio.
|
||||
|
||||
### Review Philosophy
|
||||
|
||||
* Only comment when you have HIGH CONFIDENCE (>80%) that an issue exists
|
||||
* Be concise: one sentence per comment when possible
|
||||
* Focus on actionable feedback, not observations
|
||||
* Data privacy is paramount - this is a PII detection/anonymization system
|
||||
* All modules in Presidio which process records are stateless - avoid suggesting stateful solutions
|
||||
* Presidio is a multi-component system - consider cross-component impacts of changes
|
||||
* Don't reinvent the wheel - check for existing patterns, functions and best practices in the codebase before suggesting new approaches
|
||||
|
||||
### Review Priorities
|
||||
|
||||
Focus on issues in this order of importance:
|
||||
|
||||
### 🔴 CRITICAL (Always Flag)
|
||||
|
||||
#### 1. Security & Privacy Vulnerabilities
|
||||
|
||||
**PII-Specific Risks:**
|
||||
- **PII leakage in logs, error messages, or debug output** - Never log detected PII values, only entity types and positions
|
||||
- **Regex injection vulnerabilities** - User-provided patterns must be validated before compilation
|
||||
- **Inadequate anonymization** - Reversible transformations, weak masking, deterministic fake data without proper context
|
||||
- **Side-channel leaks** - Timing attacks revealing PII presence, cache-based information disclosure
|
||||
|
||||
**General Security:**
|
||||
- Hardcoded secrets, API keys, credentials (especially for NLP model endpoints, cloud services)
|
||||
- Command injection (especially in CLI component)
|
||||
- Unsafe deserialization (pickle files, untrusted NLP models)
|
||||
- Missing input validation on API endpoints (analyzer, anonymizer, image-redactor)
|
||||
- Path traversal in file operations
|
||||
- Insecure random number generation for fake data
|
||||
|
||||
#### 2. Correctness & Logic Errors
|
||||
|
||||
**PII Detection Accuracy:**
|
||||
- **False positives** - Overly broad regex patterns matching non-PII or other entity types with med/high confidence
|
||||
- **False negatives** - Missing valid cases for a given entity type, especially edge cases or common variations
|
||||
- **Incorrect entity boundaries** - Off-by-one errors in start/end positions causing malformed anonymization
|
||||
- **Confidence score miscalculation** - Scores outside [0.0, 1.0], incorrect aggregation of multiple detection methods
|
||||
|
||||
**General Logic:**
|
||||
- Race conditions in multi-threaded analysis
|
||||
- Resource leaks (NLP models not released, file handles, network connections)
|
||||
- Null/None handling in entity detection chains
|
||||
- Incorrect error handling that silently fails to detect PII
|
||||
- Adding state where unnecessary (Presidio is designed to be stateless for scalability)
|
||||
|
||||
#### 3. Performance Issues
|
||||
|
||||
**PII Detection Specific:**
|
||||
- **Inefficient regex patterns** - Catastrophic backtracking (e.g., `(a+)+b` on "aaaa...a")
|
||||
- **Redundant passes** - Running the same logic multiple times on same text
|
||||
- **Unbounded batch processing** - Loading entire datasets into memory
|
||||
- **Missing regex compilation caching** - Recompiling patterns on every call
|
||||
- **Unnecessary model loads** - Loading the same model multiple times instead of reusing instances
|
||||
|
||||
**General Performance:**
|
||||
- O(n²) or worse algorithms when O(n) exists
|
||||
- Blocking I/O on critical API paths
|
||||
- Missing database indexes for entity result storage
|
||||
- Inefficient image processing (loading full image when bounding box would suffice)
|
||||
|
||||
### 🟡 IMPORTANT (Flag if Significant)
|
||||
|
||||
#### 4. Cross-Component Alignment & Integration
|
||||
|
||||
**Respect the Natural Data Flow:**
|
||||
- Presidio follows a unidirectional flow: Analyzer → Anonymizer → Output
|
||||
- Downstream components (CLI, structured, image-redactor) consume analyzer/anonymizer, never the reverse
|
||||
- Breaking this flow creates circular dependencies and tight coupling
|
||||
- Changes should propagate forward through the data pipeline, not backward
|
||||
|
||||
**Module Reuse Guidelines:**
|
||||
- Reuse code by importing from shared modules, not by copying code across components
|
||||
- Shared data models (RecognizerResult, OperatorConfig) should be treated as contracts - changes require coordinated updates across all consumers
|
||||
- When adding functionality, check if it belongs in an existing shared module rather than duplicating logic
|
||||
- If multiple components need the same feature, extract it to a common location rather than implementing it multiple times
|
||||
- Backward compatibility is critical when modifying shared modules - ensure existing consumers continue to work without changes
|
||||
|
||||
**Avoid Cross-Component Side Effects:**
|
||||
- Changes to internal implementation should not affect other components' behavior
|
||||
- Modifying shared configuration files requires understanding impact on all components that consume them
|
||||
- Registry and provider patterns exist to decouple components - bypassing them creates hidden dependencies
|
||||
- Component boundaries must be respected: anonymizer should never import from analyzer internals, only public interfaces
|
||||
- Providing a solution specific to one component in a shared module instead of providing a general solution that can be used by multiple components creates tight coupling and maintenance challenges
|
||||
|
||||
**When Making Changes Across Components:**
|
||||
- Identify all components that consume the interface you're modifying
|
||||
- Update dependent components in the same changeset to maintain system consistency
|
||||
- Ensure configuration files, API schemas, and documentation stay synchronized
|
||||
- Test the complete integration path, not just individual components in isolation in unit tests, integration tests, and the e2e test suite
|
||||
- Communicate changes clearly in the PR description, especially if they affect multiple components or require coordinated
|
||||
|
||||
#### 5. Architecture & Design
|
||||
|
||||
**Presidio Patterns:**
|
||||
- **Recognizer design violations** - Not inheriting from `EntityRecognizer`, missing `load()` or `analyze()`
|
||||
- **Operator design violations** - Not implementing `OperatorType` interface correctly
|
||||
- **Registry pattern misuse** - Bypassing `RecognizerRegistry`, hardcoding recognizer lists
|
||||
- **Provider pattern violations** - Not following `NlpEngineProvider` or `RecognizerRegistryProvider` patterns
|
||||
- **Tight coupling** - Recognizers depending on specific NLP engine implementation details
|
||||
|
||||
**General Design:**
|
||||
- Circular dependencies between modules
|
||||
- Missing abstraction for third-party service integrations
|
||||
- Breaking existing public APIs without deprecation warnings
|
||||
- Inconsistent error handling strategies (mixing exceptions and error codes)
|
||||
|
||||
#### 6. Data Integrity & Validation
|
||||
|
||||
**Input Validation:**
|
||||
- Missing validation of user-provided entity types
|
||||
- Accepting arbitrary regex patterns without safety checks
|
||||
- No length limits on input text (DoS via memory exhaustion)
|
||||
- Missing validation of parameters
|
||||
- Unchecked file uploads
|
||||
|
||||
**Output Validation:**
|
||||
- Confidence scores outside valid range
|
||||
- Overlapping entity spans not handled correctly
|
||||
- Missing entity type in anonymization results
|
||||
|
||||
#### 7. Testing Requirements
|
||||
|
||||
**Presidio-Specific Testing:**
|
||||
- **Missing tests for new recognizers** - Must include: true positives, true negatives, edge cases, false positive scenarios, entity within larger context
|
||||
- **No validation of entity boundaries** - Tests only check entity type, not exact start/end positions
|
||||
- **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
|
||||
|
||||
**General Testing:**
|
||||
- Missing tests for critical business logic (PII detection, anonymization)
|
||||
- Flaky tests due to non-deterministic NLP/ML models (use fixed random seeds)
|
||||
- Tests that don't validate behavior (checking implementation details instead)
|
||||
- Missing regex pattern edge cases (empty strings, special characters, unicode)
|
||||
|
||||
#### 8. Documentation Requirements
|
||||
|
||||
**Code-Documentation Consistency:**
|
||||
- Code changes must be reflected in documentation - outdated docs are misleading and dangerous
|
||||
- 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
|
||||
|
||||
**Docstring Quality:**
|
||||
- All public classes, methods, and functions must have docstrings
|
||||
- Docstrings must follow consistent format (Args, Returns, Raises, Examples)
|
||||
- No formatting issues that break API doc generation (malformed RST/Markdown, incorrect indentation)
|
||||
- Include type information in docstrings when not obvious from type hints
|
||||
|
||||
**Documentation for New Features:**
|
||||
- New recognizers must document pattern sources - link to official standards, government specifications, or authoritative references
|
||||
- Complex additions require usage examples in `docs/samples/` - show common use cases, not just API reference
|
||||
- New entity types must be added to `docs/supported_entities.md` with description and example
|
||||
- API changes require updates to `docs/api-docs/api-docs.yml` (OpenAPI schema)
|
||||
|
||||
**Pattern Recognizer Documentation:**
|
||||
- Explain the logic source: "Based on ISO standard X", "Follows format defined by Y government agency"
|
||||
- Document regex pattern rationale - why specific character classes, lookaheads, or groups are needed
|
||||
- Include references to validation algorithms (e.g., "Luhn checksum validation per ISO/IEC 7812")
|
||||
- Note any limitations or known edge cases in the pattern
|
||||
|
||||
### 💡 OPTIONAL (Low Priority)
|
||||
|
||||
#### 9. Code Quality (only if impacts maintainability)
|
||||
|
||||
- Overly complex recognizer logic (>50 lines in `analyze()` method, >3 nesting levels)
|
||||
- Misleading variable names (e.g., `pattern` for compiled regex, should be `compiled_pattern`)
|
||||
- Missing docstrings on public recognizer/operator classes
|
||||
- Incomplete type hints on public APIs (especially `analyze()`, `anonymize()` signatures)
|
||||
|
||||
### What NOT to Flag
|
||||
|
||||
**DO NOT comment on these (handled by automated tools):**
|
||||
|
||||
- ❌ Code formatting, line length, indentation (handled by `ruff format`)
|
||||
- ❌ Import ordering (handled by `ruff check --select I`)
|
||||
- ❌ Trailing commas, whitespace (handled by `ruff`)
|
||||
- ❌ Type hint style preferences (`List[str]` vs `list[str]` - both valid for Python 3.9-3.12 support)
|
||||
|
||||
**DO NOT comment on style preferences that don't affect correctness:**
|
||||
|
||||
- Personal preferences for syntax variations
|
||||
- Subjective naming when current name is clear in PII context
|
||||
- Minor refactoring suggestions that don't fix bugs or improve accuracy
|
||||
- Unnecessary abstractions "for future flexibility" in recognizers
|
||||
|
||||
### Review Examples
|
||||
|
||||
**Be specific and actionable:**
|
||||
```
|
||||
✅ GOOD: "🔴 CRITICAL: Line 45 logs detected PII value. Change logger.info(f'Found: {entity.text}')
|
||||
to logger.info(f'Found entity type: {entity.entity_type}')"
|
||||
|
||||
❌ BAD: "Don't log PII"
|
||||
```
|
||||
|
||||
**Provide context:**
|
||||
```
|
||||
✅ GOOD: "🟡 Important: This regex has catastrophic backtracking on input 'aaaaaa...b' (O(2^n) time).
|
||||
Use atomic grouping: (?>a+)b or possessive quantifier a++b"
|
||||
|
||||
❌ BAD: "This regex is slow"
|
||||
```
|
||||
|
||||
**Differentiate severity:**
|
||||
- **🔴 CRITICAL** - Security, data leakage, correctness bugs affecting PII detection accuracy
|
||||
- **🟡 Important** - Performance issues, cross-component breaks, missing tests for new recognizers
|
||||
- **💡 Suggestion** - Code quality improvements, better error messages, optimization opportunities
|
||||
|
||||
**Acknowledge good practices:**
|
||||
- Well-tested recognizers with comprehensive edge case coverage
|
||||
- Proper use of context validation (NLP + regex)
|
||||
- Good error handling with informative messages
|
||||
- Performance optimizations (regex caching, batch processing)
|
||||
|
||||
---
|
||||
|
||||
## Part 3: Repository-Specific Context
|
||||
|
||||
### Technology Stack
|
||||
- **Python** - Must support all versions
|
||||
- **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
|
||||
- **Docker** - Deployment via GitHub Container Registry (`ghcr.io/data-privacy-stack`)
|
||||
|
||||
|
||||
### Critical Files for Cross-Component Changes
|
||||
- `RecognizerResult` - Shared analyzer output format
|
||||
- `OperatorConfig` - Anonymizer operator configuration
|
||||
- `conf/default_recognizers.yaml` - System-wide recognizer registry
|
||||
- `docs/supported_entities.md` - Public entity type documentation
|
||||
- API schemas in `docs/api-docs/`
|
||||
|
||||
## Quick Reference Commands
|
||||
|
||||
### Local Development
|
||||
```bash
|
||||
# Setup (uv reads the committed uv.lock; --locked fails if it is stale)
|
||||
cd presidio-analyzer # or presidio-anonymizer, presidio-cli, etc.
|
||||
# Setup and test (per package)
|
||||
cd presidio-analyzer
|
||||
uv sync --locked --all-extras --group dev
|
||||
uv run python -m spacy download en_core_web_lg # For analyzer/CLI only
|
||||
uv run python -m spacy download en_core_web_lg # analyzer/CLI only
|
||||
uv run pytest -xvv
|
||||
uv run ruff check . && uv run ruff format .
|
||||
|
||||
# Run tests
|
||||
uv run pytest -xvv # Stop on first failure with verbose output
|
||||
uv run pytest tests/test_us_ssn_recognizer.py -k "test_valid" # Specific test
|
||||
|
||||
# Lint
|
||||
uv run ruff check .
|
||||
uv run ruff format .
|
||||
# E2E
|
||||
docker compose up --build -d && cd e2e-tests && pytest -v
|
||||
```
|
||||
|
||||
> **Dependency changes:** whenever you edit a package's `pyproject.toml`
|
||||
> dependencies (add/remove/bump `[project]` deps, extras, or
|
||||
> `[dependency-groups]`), you MUST regenerate and commit that package's
|
||||
> `uv.lock` in the same change (`cd <package> && uv lock`). CI installs with
|
||||
> `uv sync --locked` and fails if `pyproject.toml` and `uv.lock` are out of
|
||||
> sync, so an updated `pyproject.toml` without its matching `uv.lock` will
|
||||
> break the build.
|
||||
|
||||
### Docker Testing
|
||||
```bash
|
||||
# Quick test with pre-built images
|
||||
docker pull ghcr.io/data-privacy-stack/presidio-analyzer:latest
|
||||
docker run -d -p 5002:3000 --name analyzer ghcr.io/data-privacy-stack/presidio-analyzer:latest
|
||||
curl http://localhost:5002/health
|
||||
|
||||
# Full build from source (takes 15+ minutes)
|
||||
docker compose up --build -d
|
||||
```
|
||||
|
||||
### E2E Testing
|
||||
```bash
|
||||
docker-compose up -d # Start all services
|
||||
cd e2e-tests
|
||||
python -m venv venv && source venv/bin/activate
|
||||
pip install -r requirements.txt
|
||||
pytest -v # Run all E2E tests
|
||||
```
|
||||
|
||||
## Common Issues to Watch For
|
||||
|
||||
### Build/Test Issues
|
||||
- **Stale `uv.lock`** - If `uv sync --locked` fails with "lockfile needs to be updated", run `uv lock` in that package and commit the result.
|
||||
- **Missing spaCy models** - Download en_core_web_lg before running tests
|
||||
- **AHDS test skips** - Expected when AHDS_ENDPOINT not set
|
||||
- **Transformers test failures** - Expected without HuggingFace access in restricted environments
|
||||
|
||||
### Code Issues
|
||||
- **Logging PII values** - Never log `entity.text`, only `entity.entity_type`
|
||||
- **Hardcoded language assumptions** - Use `context.language` parameter
|
||||
- **Missing None checks** - NLP engines return None for empty/invalid text
|
||||
- **Unbounded regex backtracking** - Test patterns with long strings
|
||||
- **Confidence score > 1.0** - Validate score normalization logic
|
||||
|
||||
## Documentation Requirements Checklist
|
||||
|
||||
**See section 8 in Review Priorities above for comprehensive documentation guidelines.**
|
||||
|
||||
When adding features, update:
|
||||
- **docs/supported_entities.md** - For new entity types
|
||||
- **docs/api-docs/api-docs.yml** - For API changes
|
||||
- **README.md** - For major features
|
||||
- **Docstrings** - All public classes and methods (ensure proper formatting for API doc generation)
|
||||
- **docs/samples/** - Add usage examples for complex new features
|
||||
|
||||
Do not update `CHANGELOG.md` in a PR. Its current-release entries are generated
|
||||
from merged PRs before each version bump, avoiding conflicts between concurrent
|
||||
contributions.
|
||||
|
||||
## Reference Documentation
|
||||
|
||||
**Consult these for detailed guidance:**
|
||||
- **CONTRIBUTING.md** - PR process, CLA, code of conduct
|
||||
- **docs/development.md** - Build process, testing, CI/CD
|
||||
- **docs/analyzer/developing_recognizers.md** - Recognizer best practices
|
||||
- **docs/analyzer/adding_recognizers.md** - Step-by-step recognizer guide
|
||||
- **docs/anonymizer/adding_operators.md** - Operator development guide
|
||||
|
||||
---
|
||||
|
||||
**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.
|
||||
Reference docs: `CONTRIBUTING.md`, `docs/development.md`,
|
||||
`docs/analyzer/adding_recognizers.md`, `docs/analyzer/developing_recognizers.md`,
|
||||
`docs/anonymizer/adding_operators.md`.
|
||||
|
||||
@@ -0,0 +1,189 @@
|
||||
---
|
||||
applyTo: "presidio-analyzer/presidio_analyzer/predefined_recognizers/**,presidio-analyzer/presidio_analyzer/conf/default_recognizers.yaml,presidio-analyzer/tests/test_*recognizer*.py,docs/supported_entities.md"
|
||||
---
|
||||
|
||||
# Recognizer changes
|
||||
|
||||
Rules for adding or modifying PII recognizers. When reviewing, lead with the
|
||||
highest-impact gaps in this order:
|
||||
|
||||
1. **Pattern accuracy.** The pattern is as specific as the format allows, the
|
||||
score is calibrated to the pattern alone, the context words are the right
|
||||
ones, the checksum is correct where one exists (and none is invented where
|
||||
it doesn't — `validate_result` must not promote weak matches to 1.0), and
|
||||
the logic's source is documented, preferably an official specification.
|
||||
2. **Proper testing.** A configuration-path test through
|
||||
`RecognizerRegistryProvider` (the load-bearing rule below), exact-score
|
||||
assertions rather than ranges, a lookalike negative, and
|
||||
context-enhancement coverage.
|
||||
3. Construction paths that disagree (direct vs. `add_recognizer()` vs. YAML).
|
||||
4. Changes to an *existing* recognizer's patterns, scores, or context made as a
|
||||
side effect of adding a new one — users depend on current detection behavior.
|
||||
5. Language/country-code mismatch; missing exports, YAML entry, or docs row.
|
||||
|
||||
Give specific, actionable feedback: cite the file and line and propose the
|
||||
concrete fix. Do not comment on formatting — Ruff and CI own that.
|
||||
|
||||
## The load-bearing rule: test the configuration path
|
||||
|
||||
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. A recognizer that works when built
|
||||
in Python can still be unreachable — or crash — when enabled in YAML.
|
||||
|
||||
**Every new or changed recognizer needs at least one test that loads it through
|
||||
`RecognizerRegistryProvider` and asserts detection:**
|
||||
|
||||
```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`) — e.g.
|
||||
`TypeError: __init__() got an unexpected keyword argument 'name'`, which takes
|
||||
down construction of the whole registry the moment the recognizer is enabled.
|
||||
- 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 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.
|
||||
|
||||
**Construction paths must agree.** Building the recognizer directly, adding it
|
||||
via `registry.add_recognizer()`, and loading it from configuration must all
|
||||
produce the same recognizer. Flag defaulting or validation logic applied on one
|
||||
path but not the others.
|
||||
|
||||
## Placement and naming
|
||||
|
||||
- Country-specific: `predefined_recognizers/country_specific/{country}/`;
|
||||
generic patterns: `predefined_recognizers/generic/`; NLP/ML-based:
|
||||
`nlp_engine_recognizers/` or `ner/`; third-party: `third_party/`.
|
||||
- New country directories use the full lowercase country name (`south_africa`,
|
||||
`philippines`, `canada`). Some pre-existing directories use short forms
|
||||
(`us`, `uk`, `thai`); do not imitate them.
|
||||
- `supported_language` and the YAML `supported_languages` key take **ISO 639-1
|
||||
language codes** (`ko` for Korean), not country codes (`kr`). A mismatch
|
||||
produces a recognizer that never loads, with no error.
|
||||
|
||||
## Pattern scores
|
||||
|
||||
The base score must reflect how much the pattern *alone* narrows the space,
|
||||
independent of any downstream threshold:
|
||||
|
||||
| Score | Use when | Name the pattern |
|
||||
| --- | --- | --- |
|
||||
| 0.05–0.1 | Bare digit or alphanumeric runs, 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)"` |
|
||||
|
||||
Compare against existing recognizers before accepting a score
|
||||
(`UsPassportRecognizer` uses 0.05 for nine bare digits). A 0.3 on a pattern that
|
||||
also matches `covid19` or `sha256` is overstated. Coincidental matches at a low
|
||||
score are the mechanism working as designed — a threshold filters them, and
|
||||
context or validation raises the real ones.
|
||||
|
||||
Suppress low-confidence matches with `score_thresholds`, not by requiring
|
||||
context: `presidio-structured` has no surrounding text to draw context from.
|
||||
|
||||
## Context words
|
||||
|
||||
`LemmaContextAwareEnhancer` defaults to `context_matching_mode="substring"`, so
|
||||
short context words fire on unrelated tokens: `member` matches `remember`,
|
||||
`auth` matches `author` and `OAuth`. Prefer context long enough to be
|
||||
unambiguous (`member id`, `subscriber`, `prior authorization`).
|
||||
|
||||
Context is prefix-only by default (`context_prefix_count=5`,
|
||||
`context_suffix_count=0`), so a context word *after* the match does not boost
|
||||
the score. Tests should cover both placements.
|
||||
|
||||
## Validation and invalidation hooks
|
||||
|
||||
| Hook | Return | Effect on the result |
|
||||
| --- | --- | --- |
|
||||
| `validate_result` | `True` | Score replaced with `MAX_SCORE` (1.0) |
|
||||
| `validate_result` | `False` | Score set to `MIN_SCORE` and result dropped |
|
||||
| `validate_result` | `None` | Pattern score stands unchanged |
|
||||
| `invalidate_result` | `True` | Score set to `MIN_SCORE` and result dropped |
|
||||
|
||||
- **`True` is a jump to full confidence, not a nudge.** Ask what fraction of
|
||||
arbitrary same-shape tokens would pass: a mod-11 check on a 17-character token
|
||||
passes ~9% of the time, sending ~9% of coincidental matches to 1.0 where no
|
||||
threshold can filter them. Only promote where the check is genuinely mandatory
|
||||
for that value.
|
||||
- **Return `None`, never `False`, when the check does not apply.** `False` means
|
||||
"definitely not the entity" and discards the result.
|
||||
- **No checksum is fine.** About 40% of predefined recognizers do not override
|
||||
`validate_result`; the base score plus a threshold is a valid design. Do not
|
||||
request an invented validator — but do flag one that promotes weak matches.
|
||||
- Well-known sample values and reserved ranges belong in `invalidate_result`,
|
||||
not buried in the regex.
|
||||
|
||||
## Enabled by default or not
|
||||
|
||||
Global (non-country-specific) recognizers — credit card, email, IBAN, IP, URL —
|
||||
generally ship `enabled: true`, provided their false-positive rate is low.
|
||||
Country-specific recognizers default to `enabled: false`. In both cases the
|
||||
deciding question is whether the recognizer can produce **high-confidence false
|
||||
positives**; shipping enabled requires justification in the PR description and
|
||||
both of:
|
||||
|
||||
- The base score is calibrated to the pattern's specificity (bands above).
|
||||
- Nothing promotes a coincidental match to a score the user cannot filter.
|
||||
|
||||
## 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** — a plausible non-PII token of the same shape
|
||||
(a 17-character order ID for a VIN, a legal citation for a bank account
|
||||
number) asserted as *not* flagged. This is the actual false-positive surface.
|
||||
- **Exercise context enhancement.** A recognizer defining `CONTEXT` needs a test
|
||||
showing the score differs between text with and without a context word.
|
||||
- **Assert entity boundaries** (exact start/end), not just entity type — and
|
||||
include values embedded in surrounding text.
|
||||
- **Use example values the recognizer actually accepts.** Well-known samples
|
||||
like `123-45-6789` are denylisted by `UsSsnRecognizer`: as a true positive the
|
||||
test fails, and as a false-positive case it passes for the wrong reason.
|
||||
|
||||
## Required companion updates
|
||||
|
||||
A new recognizer needs all of:
|
||||
|
||||
1. Export in `presidio_analyzer/predefined_recognizers/__init__.py` **and** the
|
||||
country/category `__init__.py`.
|
||||
2. An entry in `presidio_analyzer/conf/default_recognizers.yaml` (normally
|
||||
`enabled: false`).
|
||||
3. A row in `docs/supported_entities.md`.
|
||||
4. A docstring citing the pattern's source: the official standard, government
|
||||
specification, or authoritative reference the format comes from, plus the
|
||||
validation algorithm if any (e.g. "Luhn checksum per ISO/IEC 7812").
|
||||
@@ -0,0 +1,108 @@
|
||||
---
|
||||
applyTo: "presidio-analyzer/presidio_analyzer/input_validation/**,presidio-analyzer/presidio_analyzer/recognizer_registry/**,presidio-analyzer/presidio_analyzer/conf/**,presidio-analyzer/presidio_analyzer/nlp_engine/ner_model_configuration.py,presidio-analyzer/tests/test_yaml_recognizer_models.py,presidio-analyzer/tests/test_recognizer_registry_provider.py,presidio-analyzer/tests/test_configuration_validator.py,presidio-analyzer/tests/test_config_loader.py,presidio-analyzer/tests/test_recognizer_registry.py,presidio-analyzer/tests/test_ner_model_configuration.py"
|
||||
---
|
||||
|
||||
# YAML configuration & pydantic validation layer
|
||||
|
||||
Rules for the layer that translates YAML configuration into Presidio instances:
|
||||
the pydantic models in `presidio_analyzer/input_validation/`
|
||||
(`yaml_recognizer_models.py`, `schemas.py`), the loaders in
|
||||
`recognizer_registry/`, and the shipped configs in `conf/`.
|
||||
|
||||
This layer is a public contract. YAML files written by users years ago must keep
|
||||
parsing, and every field a user can write must actually reach the object it
|
||||
configures. When reviewing, lead with:
|
||||
|
||||
1. A YAML-reachable field that silently goes nowhere (schema/constructor drift).
|
||||
2. A change that makes existing YAML files stop parsing or change meaning.
|
||||
3. A validation failure surfacing as a distant `TypeError` instead of a parse-time
|
||||
error with an actionable message.
|
||||
|
||||
## Schema/constructor sync
|
||||
|
||||
Every constructor parameter that should be settable from YAML needs a matching
|
||||
pydantic field. In every contribution, check that constructor parameters and
|
||||
schema fields have not drifted apart — a mismatch means a value a user writes
|
||||
in YAML never reaches the object, or reaches it unvalidated. As of today the
|
||||
consequence is silent: `PredefinedRecognizerConfig` ignores unknown YAML keys,
|
||||
so a constructor kwarg without a schema field is dropped without any error and
|
||||
the recognizer falls back to its defaults (the failure
|
||||
`LangExtractRecognizerConfig` exists to prevent; see its docstring). Even if
|
||||
that `extra` behavior changes, the no-mismatch rule stands.
|
||||
|
||||
- A recognizer whose constructor takes model-specific kwargs needs a dedicated
|
||||
config model registered in `CONFIG_MODEL_MAP` (keyed by `class_name` or
|
||||
`name`), following `HuggingFaceRecognizerConfig` / `GLiNERRecognizerConfig` /
|
||||
`LangExtractRecognizerConfig`.
|
||||
- When a PR adds a constructor parameter to a recognizer that already has a
|
||||
config model, require the matching field in that model — otherwise YAML users
|
||||
cannot set it and get no error telling them so.
|
||||
|
||||
## `extra` must be a deliberate choice
|
||||
|
||||
- `extra="forbid"` for closed configs (`TextChunkerConfig`,
|
||||
`RecognizerRegistryConfig`): typos fail fast at parse time with a clear
|
||||
message.
|
||||
- `extra="allow"` for pass-through configs whose kwargs flow to a constructor
|
||||
(HuggingFace, GLiNER, LangExtract).
|
||||
- Flag a new model that leaves pydantic's default (`extra="ignore"`) without
|
||||
justification — silent ignoring is almost never the intended behavior.
|
||||
|
||||
## `exclude_none` discipline on kwargs models
|
||||
|
||||
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
|
||||
trap.
|
||||
|
||||
## Fail early, with actionable messages
|
||||
|
||||
Validation belongs at parse time, in the pydantic model, phrased so the user
|
||||
knows how to fix their YAML — not as a distant `TypeError` during registry
|
||||
construction. House style to hold new code to:
|
||||
|
||||
- Class existence checked at parse time
|
||||
(`validate_predefined_recognizer_exists` → "Predefined recognizer 'X' not
|
||||
found"), and custom/predefined name conflicts rejected with the fix spelled
|
||||
out ("Either use type: 'predefined' or choose a different name").
|
||||
- Mutually exclusive fields enforced in a `model_validator` naming both fields
|
||||
("Cannot specify both 'supported_language' and 'supported_languages'"), with
|
||||
an example of the correct form where the fix isn't obvious (see the global
|
||||
context validator).
|
||||
- Parameters checked against the selected mode (`TextChunkerConfig` rejects
|
||||
`max_tokens` on a character chunker by name, listing the allowed fields).
|
||||
- Prefer warnings over exceptions when the caller cannot fix the condition;
|
||||
raising on a config the user didn't write turns a degraded result into a hard
|
||||
failure.
|
||||
|
||||
## Backward compatibility of the schema
|
||||
|
||||
Existing user YAML must keep working. Each of these is a breaking change and
|
||||
must be called out explicitly in the PR description:
|
||||
|
||||
- A new **required** field, a renamed field, or a removed field.
|
||||
- A tightened validator that rejects previously-accepted YAML.
|
||||
- A changed default (including a changed `enabled`, score, or language default).
|
||||
- Dropping support for the legacy singular forms. `supported_language` /
|
||||
`supported_languages` and `supported_entity` / `supported_entities` both stay
|
||||
supported, with mutual exclusivity enforced — do not remove the legacy form.
|
||||
- Removing accepted input shapes: bare-string recognizer entries and dict
|
||||
entries with inferred `type` (`patterns`/`deny_list` ⇒ custom) are all valid
|
||||
today and must remain so.
|
||||
|
||||
## Required tests for changes in this layer
|
||||
|
||||
- **Round-trip through the provider**: load a config through
|
||||
`RecognizerRegistryProvider` and assert on the constructed registry — not
|
||||
only on the validated pydantic model. Model-level tests miss dump/loader
|
||||
drift.
|
||||
- The shipped `conf/default_recognizers.yaml` must validate against the models;
|
||||
a change to either side needs `test_recognizer_registry_provider.py` /
|
||||
`test_yaml_recognizer_models.py` updated in the same PR.
|
||||
- New validators need both directions tested: valid YAML passes, invalid YAML
|
||||
fails **with the expected message** (assert on the message — it is part of
|
||||
the user experience).
|
||||
- A new schema field needs a test proving the value actually reaches the
|
||||
constructed object, not just that validation accepts it.
|
||||
@@ -0,0 +1,116 @@
|
||||
# Presidio — Agent Guidelines
|
||||
|
||||
Presidio is a Python SDK for detecting (presidio-analyzer) and anonymizing
|
||||
(presidio-anonymizer) PII in text and images, plus CLI, structured-data, and
|
||||
image-redaction components. It is a widely used **library**: users depend on
|
||||
current detection behavior, and configuration files written years ago must keep
|
||||
working. Correctness and backward compatibility outrank cleverness.
|
||||
|
||||
The review-side versions of these rules — which the Copilot PR review agent
|
||||
also enforces — live in `.github/copilot-instructions.md` and
|
||||
`.github/instructions/*.instructions.md`. Follow them at authoring time so the
|
||||
review finds nothing.
|
||||
|
||||
## Working in this repo
|
||||
|
||||
```bash
|
||||
cd presidio-analyzer # or presidio-anonymizer, presidio-cli, ...
|
||||
uv sync --locked --all-extras --group dev
|
||||
uv run python -m spacy download en_core_web_lg # analyzer/CLI only
|
||||
uv run pytest -xvv
|
||||
uv run ruff check . && uv run ruff format .
|
||||
```
|
||||
|
||||
- Python `>=3.10,<3.15`; code must run on every version in range.
|
||||
- Dependencies are managed with **uv**, not pip/Poetry. If you touch a
|
||||
package's `pyproject.toml` dependencies, run `uv lock` in that package and
|
||||
commit the updated `uv.lock` in the same change — CI fails on drift.
|
||||
- Do not edit `CHANGELOG.md`; release entries are generated from merged PRs.
|
||||
- Never log PII values (`entity.text`) — only entity types and positions.
|
||||
- Modules that process records are stateless; do not add state.
|
||||
- Terminology: "threshold", not "cutoff"; ISO 639-1 language codes everywhere.
|
||||
|
||||
## Adding a PII recognizer
|
||||
|
||||
The full rulebook — score bands, context-word rules, validation-hook
|
||||
semantics, the configuration-path test template, and the test-quality bar —
|
||||
is `.github/instructions/recognizers.instructions.md`. **Read it before
|
||||
starting**; those rules apply at authoring time, not just in review. The
|
||||
workflow, in order:
|
||||
|
||||
1. **Place and name it** under `predefined_recognizers/`: full lowercase
|
||||
country name for new country directories (`south_africa`, not `za`;
|
||||
don't imitate the pre-existing short forms `us`/`uk`/`thai`), or
|
||||
`generic/`, `nlp_engine_recognizers/`, `ner/`, `third_party/` as
|
||||
appropriate.
|
||||
2. **Use ISO 639-1 language codes** (`ko` for Korean, never `kr`) — a
|
||||
mismatch loads nothing, silently.
|
||||
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.
|
||||
4. **Design the pattern for accuracy first** — this is the top review
|
||||
priority: as specific as the format allows, score calibrated to the
|
||||
pattern alone, unambiguous context words, the correct checksum if one
|
||||
exists (and none invented if it doesn't), and the pattern's source
|
||||
documented in the docstring, preferably an official specification.
|
||||
5. **Register it everywhere**: exports in `predefined_recognizers/__init__.py`
|
||||
*and* the country/category `__init__.py`; an entry in
|
||||
`conf/default_recognizers.yaml` (country-specific ships `enabled: false`);
|
||||
a row in `docs/supported_entities.md`.
|
||||
6. **Write the configuration-path test** — the most-missed step and the one
|
||||
that matters most: enable the recognizer in a YAML config, load it through
|
||||
`RecognizerRegistryProvider`, and assert detection (template in the
|
||||
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.
|
||||
|
||||
When **modifying** an existing recognizer: changed patterns, scores, or context
|
||||
change detection results for existing users — state that in the PR description,
|
||||
and never change an existing recognizer as a side effect of adding a new one.
|
||||
|
||||
## Changing the YAML configuration layer
|
||||
|
||||
The rulebook for the pydantic models in `presidio_analyzer/input_validation/`
|
||||
is `.github/instructions/yaml-config.instructions.md` — **read it before
|
||||
touching the layer**. The short version:
|
||||
|
||||
- Constructor parameters and schema fields must never drift apart: a
|
||||
YAML-settable kwarg without a matching pydantic field is silently dropped
|
||||
today. Model-specific kwargs need a dedicated config model registered in
|
||||
`CONFIG_MODEL_MAP`.
|
||||
- Choose `extra` deliberately (`forbid` fails fast, `allow` passes through);
|
||||
pass-through models dump with `exclude_none=True` so YAML omissions keep
|
||||
constructor defaults.
|
||||
- Validate at parse time with actionable messages, and never break existing
|
||||
YAML — legacy singular fields, bare-string entries, and inferred `type` all
|
||||
stay supported.
|
||||
- Test through `RecognizerRegistryProvider`, not just the pydantic model:
|
||||
prove new fields reach the constructed object and assert error *messages*
|
||||
for invalid YAML.
|
||||
|
||||
## General engineering rules
|
||||
|
||||
- **Declare behavior changes.** Any edit outside a brand-new file needs the PR
|
||||
description to say what existing behavior changes. Defaults on shared base
|
||||
classes, properties on abstract interfaces, and anything altering returned
|
||||
entities or scores all count, even with no signature change.
|
||||
- **Explainability**: anything that changes how a score is derived must be
|
||||
reflected in `AnalysisExplanation`.
|
||||
- **Component boundaries**: data flows Analyzer → Anonymizer → Output; import
|
||||
public interfaces, never another component's internals; shared models
|
||||
(`RecognizerResult`, `OperatorConfig`) are contracts — update all consumers
|
||||
in the same changeset.
|
||||
- **Anonymizer operators**: non-reversible by default — no deterministic
|
||||
hashing (rainbow tables), unpredictable replacement values, no preserved PII
|
||||
characteristics. Deterministic or format-preserving output is sometimes a
|
||||
hard requirement (e.g. referential integrity across a dataset); support it as
|
||||
an explicit, documented opt-in, never as the default behavior.
|
||||
- **Security**: never log PII values; validate untrusted inputs before use
|
||||
(user-supplied regexes before compilation, file paths, API payloads); no
|
||||
hardcoded secrets; no unsafe deserialization of untrusted models or pickles.
|
||||
- **Performance**: no catastrophic regex backtracking (test long adversarial
|
||||
inputs); cache compiled regexes; batch NLP with `nlp.pipe`.
|
||||
- **Docs move with code**: `docs/supported_entities.md` for entities,
|
||||
`docs/api-docs/api-docs.yml` for API changes, reST docstrings on public
|
||||
APIs, samples for complex features.
|
||||
Reference in New Issue
Block a user