* ci: use uv for test dependency installation + add timeout safety net The analyzer test job runs `poetry install --all-extras` with no committed lock, so Poetry performs a universal resolve of the full optional-dependencies graph on every run. That resolve backtracks for many minutes — effectively hanging — driven by the langextract extra's deep, loosely-bounded transitive tree. Reproduced locally: Poetry >6 min (never completed); uv ~2s for the same 158-package resolution. Switch the test jobs to uv: - Install uv via astral-sh/setup-uv. - Create a per-component venv with `uv venv --seed` (seed keeps pip for the wheel-build step) and expose it via $GITHUB_PATH / $VIRTUAL_ENV. - `uv pip install -e '.<extras>'` plus the test tooling that previously came from the Poetry dev group (uv does not read that group). - Add a job-level `timeout-minutes: 60` (and 30 on the install step) so a bad resolve fails fast instead of hanging. No package metadata changed — pyproject.toml is untouched; this only swaps the CI install tooling. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * ci: run tests via uv run inside the uv-managed venv Use "uv run --no-sync" for spaCy model downloads, pytest, and diff-cover instead of exposing the venv on $GITHUB_PATH. "uv sync" builds the project's .venv (resolving the analyzer --all-extras graph in seconds where Poetry's lockless universal resolve hangs), and the subsequent steps execute inside it idiomatically. The --all-extras selection stays in the job matrix, unchanged from main. Wheel-build keeps using the system interpreter, matching the previous Poetry behaviour. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * ci: install uv via pip instead of the setup-uv action The astral-sh/setup-uv action is not on the org's allowed-actions list, so adding it made the CI workflow fail at startup (startup_failure) before any job ran. Install a pinned uv from PyPI with the already present setup-python instead; this needs no new third-party action and keeps the resolver benefits that motivated the Poetry -> uv switch. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * ci: add pip to the uv venv for spaCy model downloads spaCy's `download` command shells out to `python -m pip install`, but uv-created virtualenvs ship without pip, so runtime/first-use model downloads (e.g. the presidio-cli conftest fetching en_core_web_lg) failed with SystemExit: 1. Install pip alongside the test tooling. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * ci: pre-install tomli for the wheel build on Python < 3.11 `python -m build` imports `tomli` on Python < 3.11 to read pyproject.toml, but it is not in the hashed requirements-build.txt (pip-compiled on 3.11+ where tomllib is stdlib). Poetry previously pulled tomli onto the runner as one of its own dependencies, so `pip install --require-hashes` found it already satisfied. uv has no Python dependencies, so the 3.10 wheel-build jobs began failing with "requirements must be pinned with ==". Pre-install a marker-guarded tomli to restore that condition. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * ci: build service Docker images with uv to fix the E2E resolve hang The local-build-and-E2E job builds the analyzer/anonymizer/image-redactor images via `docker compose build`. Each Dockerfile ran `poetry install --no-root --only=main -E server`, which triggered the same lockless Poetry backtracking that hangs the test jobs: the analyzer image sat on "Resolving dependencies" for ~40 minutes until the job was cancelled. Switch the dependency install to `uv pip install --system -r pyproject.toml --extra server`, which resolves the same graph in seconds (verified locally: the analyzer install layer completes in ~9s and imports spacy/flask/gunicorn while correctly omitting the project itself, matching --no-root). Drop the now-unused POETRY_VIRTUALENVS_CREATE env and switch `poetry run python install_nlp_models.py` to a direct call. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: run gunicorn directly in container entrypoints (drop poetry run) The Docker images no longer install Poetry (deps come from uv), so the `exec poetry run gunicorn ...` entrypoints crashed at startup with "exec: poetry: not found". The analyzer/anonymizer/image-redactor containers never came up, so every E2E test failed with connection refused on ports 5002/5003. uv installs gunicorn system-wide, so invoke it directly. Verified locally: the anonymizer container now starts healthy and /health returns 200. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * ci: keep uv's wheel cache on the /mnt ephemeral disk Restore the intent of the old POETRY_CACHE_DIR=/mnt/poetry_cache setup for uv: point UV_CACHE_DIR at the runner's large ephemeral disk (/mnt, ~65 GB) so the heavy analyzer --all-extras wheel cache does not fill the smaller root volume. /mnt is root-owned, so a prep step creates the directory and hands it to the runner user. UV_LINK_MODE=copy silences the cross-filesystem hardlink warning, since the project venv lives on the root volume under the checkout while the cache is on /mnt. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * docs: broaden CHANGELOG/CI comment to cover the Docker image changes Address review feedback that the PR scope grew beyond the test jobs: the CHANGELOG now notes the analyzer/anonymizer/image-redactor image builds and entrypoints also moved from Poetry to uv, and clarifies pyproject.toml metadata is unchanged. Also clarify the ci.yml install-step comment that the wheel-build step deliberately uses the system interpreter (not uv run). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * build: adopt PEP 735 dependency groups and commit uv.lock files Migrate each CI-tested package's development tooling from Poetry's [tool.poetry.group.dev.dependencies] to the standard PEP 735 [dependency-groups] table, and commit a uv.lock per package. This makes pyproject.toml the single source of truth for dev tooling and lets CI and Docker install a pinned, reproducible dependency graph instead of resolving on every run. The [project] metadata and the poetry-core build backend are unchanged, so wheels/sdists are byte-for-byte identical. Covers presidio-analyzer, presidio-anonymizer, presidio-cli, presidio-image-redactor, presidio-structured. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * ci: install from the committed uv.lock in CI and Docker builds CI now runs `uv sync --locked --group dev`, which installs exactly the locked graph and fails if pyproject.toml and uv.lock disagree rather than silently resolving new versions. The dev group provides the test tooling (and pip, which spaCy's model download shells out to), so the manual `uv pip install pytest ...` line is gone. The uv cache is persisted across runs via actions/cache keyed on uv.lock and trimmed with `uv cache prune --ci`. The analyzer/anonymizer/image-redactor images consume the same lockfile with `uv sync --locked --no-default-groups --extra server --no-install-project` into UV_PROJECT_ENVIRONMENT=/usr/local, guaranteeing images use the same dependency graph as CI. Splitting the dependency layer from the source COPY keeps it cached until pyproject/uv.lock change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * ci: allow pre-existing transformers advisories in dependency review The newly committed uv.lock files pin exact versions, so dependency-review now sees two transformers advisories that already exist on main (capped at transformers <5 by spacy-huggingface-pipelines and tracked as Dependabot alerts). They are unrelated to the Poetry->uv migration and will be addressed in separate security PRs, so allow-list the two GHSAs here to keep this PR's dependency review unblocked. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * docs: require uv.lock review and commits alongside pyproject changes Address review feedback from @SharonHart: - CODEOWNERS: require presidio-administrators approval for **/uv.lock, the same as **/pyproject.toml, so lockfile changes get dependency review. - copilot-instructions: switch the local dev commands to uv and add an explicit rule that editing pyproject.toml dependencies requires regenerating and committing that package's uv.lock in the same change, since CI installs with `uv sync --locked` and fails on drift. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * ci: reference UV_CACHE_DIR in cache path; align dev-docs to uv Address review feedback: - ci.yml: use ${{ env.UV_CACHE_DIR }} for the cache action path instead of hardcoding /mnt/uv-cache, so the cache and uv stay in sync if the dir changes. - copilot-instructions: update the "Technology Stack" bullet to name uv as the dependency/install tool (poetry-core retained only as build backend), matching the uv-based dev commands already documented. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * ci: shorten workflow comments; move transformers allow-ghsas out Trim the verbose explanatory comments in the test job to 1-3 lines each, and drop the dependency-review `allow-ghsas` entry for the transformers advisories. That suppression is a security concern rather than part of the Poetry->uv migration, so it moves to the stacked security PR alongside the .trivyignore. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * ci: drop stale pip cache; trim CHANGELOG entry Remove `cache: 'pip'` (keyed on pyproject.toml) from the test job's setup-python: it is leftover Poetry-era config. Project dependencies now use the uv cache, and the only remaining pip installs (uv, tomli, build tools) are tiny and unrelated to pyproject.toml. Also condense the unreleased CHANGELOG entry to a single concise line; the implementation detail lives in the PR description. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * small commit to trigger the pipeline again. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
26 KiB
Presidio Development 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.
Core Philosophy
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
Part 1: Implementation Guidelines
Use these guidelines when generating or writing code for Presidio.
Presidio Architecture Patterns
Data Flow (Unidirectional):
Analyzer (detect PII) → Anonymizer (transform PII) → Output
↓
nlp_engine → recognizers → context
Design Patterns:
- Registry Pattern:
RecognizerRegistryfor dynamic recognizer management - Provider Pattern:
NlpEngineProvider,RecognizerRegistryProviderfor configuration
Implementing New Recognizers
1. Choose the Right Base Class:
from presidio_analyzer import PatternRecognizer, LocalRecognizer, RemoteRecognizer
# For regex-based detection
class MyPatternRecognizer(PatternRecognizer):
pass
# For custom logic (NLP, ML)
class MyCustomRecognizer(LocalRecognizer):
def load(self): ...
def analyze(self, text, entities, nlp_artifacts): ...
# For calling remote services
class MyRemoteRecognizer(RemoteRecognizer):
def analyze(self, text, entities, nlp_artifacts): ...
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/
3. Pattern Design Best Practices:
# ❌ BAD: Too broad - matches month names as persons
pattern = r"\b[A-Z][a-z]+\b"
# ✅ 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
)
]
CONTEXT = ["ssn", "social security", "tax id"]
4. Document Pattern Sources:
"""
Recognizes US Social Security Numbers.
Pattern based on SSA Publication No. 05-10633:
https://www.ssa.gov/history/ssn/geocard.html
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)
"""
5. Required Configuration Updates:
# Update all of these:
# 1. presidio_analyzer/predefined_recognizers/__init__.py
from .country_specific.us.my_recognizer import MyRecognizer
__all__ = [..., "MyRecognizer"]
# 2. presidio_analyzer/predefined_recognizers/country_specific/us/__init__.py
from .my_recognizer import MyRecognizer
__all__ = [..., "MyRecognizer"]
# 3. presidio_analyzer/conf/default_recognizers.yaml
recognizers:
- name: MyRecognizer
supported_languages: ["en"]
type: predefined
enabled: false # Country-specific defaults to false
# 4. docs/supported_entities.md (add row to appropriate table)
# 5. CHANGELOG.md (under "Unreleased" section)
6. Comprehensive Test Coverage:
@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: 000-00-0000", 0, ()), # Invalid area
("SSN: 666-12-3456", 0, ()), # Excluded area
# 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:
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)
# ❌ 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:
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:
# ❌ 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:
# 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
# 4. Update CHANGELOG.md
Cross-Component Changes
When modifying shared interfaces:
- Identify all consumers:
# RecognizerResult is consumed by:
# - presidio-anonymizer (takes analyzer results)
# - presidio-cli (displays results)
# - presidio-structured (processes tabular data)
# - docs/samples/* (user examples)
- Update all components in same changeset:
# 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
- Respect component boundaries:
# ❌ 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:
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:
# ❌ BAD: O(2^n) on "aaaa...ab"
pattern = r"(a+)+"
# ✅ GOOD: Atomic grouping
pattern = r"(?>a+)"
3. Batch NLP Processing:
# ❌ 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:
# ✅ 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:
When adding a feature, update ALL of:
✅ CHANGELOG.md - Under "Unreleased" section
✅ 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:)
2. Pattern Source Documentation:
# 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+)+bon "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, missingload()oranalyze() - Operator design violations - Not implementing
OperatorTypeinterface correctly - Registry pattern misuse - Bypassing
RecognizerRegistry, hardcoding recognizer lists - Provider pattern violations - Not following
NlpEngineProviderorRecognizerRegistryProviderpatterns - 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.mdwith 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.,
patternfor compiled regex, should becompiled_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]vslist[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-coreis 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 formatOperatorConfig- Anonymizer operator configurationconf/default_recognizers.yaml- System-wide recognizer registrydocs/supported_entities.md- Public entity type documentation- API schemas in
docs/api-docs/
Quick Reference Commands
Local Development
# Setup (uv reads the committed uv.lock; --locked fails if it is stale)
cd presidio-analyzer # or presidio-anonymizer, presidio-cli, etc.
uv sync --locked --all-extras --group dev
uv run python -m spacy download en_core_web_lg # For analyzer/CLI only
# 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 .
Dependency changes: whenever you edit a package's
pyproject.tomldependencies (add/remove/bump[project]deps, extras, or[dependency-groups]), you MUST regenerate and commit that package'suv.lockin the same change (cd <package> && uv lock). CI installs withuv sync --lockedand fails ifpyproject.tomlanduv.lockare out of sync, so an updatedpyproject.tomlwithout its matchinguv.lockwill break the build.
Docker Testing
# 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
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- Ifuv sync --lockedfails with "lockfile needs to be updated", runuv lockin 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, onlyentity.entity_type - Hardcoded language assumptions - Use
context.languageparameter - 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:
- CHANGELOG.md - Under "Unreleased" section
- 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
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.