refactor: span-level advisor, recogniser base, new recognisers, MCP entrypoint - #32
Merged
Conversation
…efault, docker port - sorted(certain + kept, key=e.start) — preserve span ordering across certain/kept merge - catch IndexError — empty choices list no longer bypasses fail-open - strip provider prefix in build_judge_model when base_url set (was already done in _judge) - judge_score_threshold 0.99 → 0.85 — 0.99 sent nearly everything to judge, defeating latency goal - docker-compose: vllm base_url uses container port 8000 not host-mapped 8001; drop openai: prefix - module docstring documents false-negative capability trade-off explicitly
Presidio's recognisers emit 0.85 as their default confidence floor. With threshold=0.85, nearly every detection is classified as 'certain' and the judge is never called. 0.7 ensures default-confidence spans (PERSON, LOCATION, DATE_TIME) go through the judge for verification.
Logic: score >= threshold → certain (skip judge), score < threshold → uncertain (send to judge). 0.85 and 0.7 had it backwards — they made PERSON at Presidio's 0.85 floor 'certain', so the judge was never called. 0.9 sends NER detections (0.85) to the judge while keeping 1.0-confidence pattern matches (AU_TFN, EMAIL) as certain.
…ities; correct threshold to 0.9 - _judge() was stripping on ':' treating Ollama's name:tag as provider:model (qwen2.5:3b → 3b). Custom endpoint receives model name as-is; stripping belongs in assessor path only. - Fail-open return now sorted by e.start, consistent with success path. - judge_score_threshold default 0.7 → 0.9: higher threshold = more entities sent to judge. 0.9 makes Presidio's typical 0.85 NER detections uncertain; 1.0 pattern matches stay certain.
- app.py, mcp/server.py: change startup condition from 'judge_model or judge_base_url' to 'judge_model' — build_assessor_agent requires judge_model; base_url alone would raise ValueError at startup - tests/conftest.py: _PassThroughRefiner uses object.__init__ instead of pass, avoiding violated base class invariants (_client/_settings unset) - build_refiner: document OpenAI-compatible-only constraint; non-OpenAI provider prefixes (anthropic:, google:) are not supported and will fail-open - docker-compose.yml: pin vllm image to v0.9.2 instead of :latest - .env.example: clarify refiner/assessor provider split, correct threshold default to 0.9, swap example model to openai:gpt-4o-mini
…s, MCP entrypoint
There was a problem hiding this comment.
Pull request overview
This PR refactors the “judge/refiner” layer into an “advisor” architecture, replaces the Presidio+spaCy detection pipeline with an internal recogniser stack (regex + optional GLiNER2), and adds runnable entrypoints for both the FastAPI service and MCP server. It also updates tests, Docker/dev tooling, and bumps the package version to 0.4.0 to reflect the architectural shift.
Changes:
- Replace Presidio AnalyzerEngine/spaCy with
BaseRecogniser-driven detection (regex + optional GLiNER2) and introduce span-levelSpanAdvisor. - Rename
judge→advisorthroughout API/MCP surfaces (modes, settings, deps) and addadvisor_appliedmetadata. - Add new generic recognisers (email/phone/credit-card/person/location/date_time), new module entrypoints, and update Docker/dev configuration accordingly.
Reviewed changes
Copilot reviewed 54 out of 58 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/test_refiner.py | New unit tests for SpanAdvisor behavior (routing, timeout fail-open, keep list filtering). |
| tests/unit/test_recognisers.py | Adjust tests to new recogniser base validation hook (_validate). |
| tests/unit/test_pseudonymiser.py | Update pseudonymiser tests to use operator configs derived from recognisers. |
| tests/unit/test_properties.py | Switch property tests from ENTITY_CLASSIFICATION to recogniser-based completeness checks. |
| tests/unit/test_judge_model.py | Rename tests to build_advisor_model and update env var expectations. |
| tests/unit/test_entities.py | Update entity model tests for new verification field and recogniser-driven validation. |
| tests/unit/test_assessor.py | Update assessor tests to advisor module path and new entity construction requirements. |
| tests/unit/test_analyser.py | Update analyser tests to new Span/BaseRecogniser conversion and regex-only detection. |
| tests/integration/test_refine.py | Integration coverage for advisor/fast modes across /detect and /pseudonymise. |
| tests/integration/test_pseudonymise.py | Update integration test expectations for request/response mode metadata rename. |
| tests/integration/test_detect.py | Update integration test expectations for request/response mode metadata rename. |
| tests/conftest.py | Rewire fixtures to new analyser/pseudonymiser/advisor stack and provide test doubles. |
| src/priveil/settings.py | Replace judge/spaCy settings with advisor + optional GLiNER2 config. |
| src/priveil/recognisers/registry.py | Introduce recogniser registry returning ordered BaseRecogniser list + operator config builder. |
| src/priveil/recognisers/phone.py | Add generic international phone regex recogniser. |
| src/priveil/recognisers/person.py | Add GLiNER2-backed PERSON recogniser. |
| src/priveil/recognisers/location.py | Add GLiNER2-backed LOCATION recogniser. |
| src/priveil/recognisers/email.py | Add email regex recogniser. |
| src/priveil/recognisers/date_time.py | Add GLiNER2-backed DATE_TIME recogniser with advisor verification routing. |
| src/priveil/recognisers/credit_card.py | Add credit card regex recogniser with Luhn validation and mask operator defaults. |
| src/priveil/recognisers/base.py | New shared recogniser base classes (Span, BaseRecogniser, RegexRecogniser, GLiNERRecogniser). |
| src/priveil/recognisers/au_tfn.py | Port TFN recogniser to new regex base with checksum validation. |
| src/priveil/recognisers/au_phone.py | Port AU phone recogniser to new regex base. |
| src/priveil/recognisers/au_medicare.py | Port Medicare recogniser to new regex base with checksum validation. |
| src/priveil/recognisers/au_bsb.py | Port BSB recogniser to new regex base and route verification via advisor. |
| src/priveil/recognisers/au_acn.py | Port ACN recogniser to new regex base with checksum validation. |
| src/priveil/recognisers/au_abn.py | Port ABN recogniser to new regex base with checksum validation. |
| src/priveil/mcp/tools.py | Update MCP tools to use advisor flow and surface advisor_applied in responses. |
| src/priveil/mcp/server.py | Update MCP server lifespan to load GLiNER2 optionally and wire advisor/assessor. |
| src/priveil/mcp/main.py | Add runnable MCP entrypoint (python -m priveil.mcp). |
| src/priveil/mcp/init.py | Update MCP package exports/import side effects to align with new entrypoint. |
| src/priveil/judge/refiner.py | Remove legacy refiner implementation. |
| src/priveil/judge/prompts/refiner.md | Remove legacy refiner prompt. |
| src/priveil/engine/pseudonymiser.py | Make operator config injectable and derive defaults from recognisers. |
| src/priveil/engine/analyser.py | Replace Presidio+spaCy analyser with concurrent recogniser execution + global deduplication. |
| src/priveil/domain/pseudonymisation.py | Rename mode to advisor and add advisor_applied field to response data. |
| src/priveil/domain/entities.py | Remove classification map and add verification field on Entity. |
| src/priveil/domain/detection.py | Rename mode to advisor and add advisor_applied field to response data. |
| src/priveil/app.py | Rewire app lifespan to initialise recogniser stack, pseudonymiser configs, and optional advisor. |
| src/priveil/api/routes/pseudonymise.py | Replace refiner wiring with advisor wiring and surface advisor_applied. |
| src/priveil/api/routes/detect.py | Replace refiner wiring with advisor wiring and surface advisor_applied. |
| src/priveil/api/routes/assess.py | Update assess route to advisor assessor module and new env var wording. |
| src/priveil/api/models.py | Update request/response meta mode Literals to advisor. |
| src/priveil/api/deps.py | Replace refiner dependency with advisor dependency and update assess error message. |
| src/priveil/advisor/span_advisor.py | New span-level advisor implementation with timeout + fail-open behavior. |
| src/priveil/advisor/prompts/span_advisor.md | New advisor prompt for span keep/drop decisions. |
| src/priveil/advisor/prompts/assessor.md | New assessor prompt aligned with advisor layer. |
| src/priveil/advisor/model.py | New advisor model factory supporting provider strings and OpenAI-compatible endpoints. |
| src/priveil/advisor/assessor.py | Update assessor prompt construction + breakdown logic and wire to advisor model factory. |
| src/priveil/advisor/init.py | Add advisor package init. |
| src/priveil/main.py | Add runnable API entrypoint (python -m priveil). |
| pyproject.toml | Bump version to 0.4.0, add gliner optional extra, update scripts/dep set. |
| Dockerfile | Restructure stages and add a “local” target with gliner + mcp extras. |
| docker-compose.yml | Add vLLM service and wire API container to it via advisor settings. |
| docker-compose.local.yml | Add local dev stack (Ollama + API + MCP over SSE). |
| .env.example | Update env var documentation to advisor terminology and new tuning knobs. |
| .cursor/skills/conduit-py.md | Update repository guidance to reflect new advisor/recogniser architecture and entrypoints. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 54 out of 58 changed files in this pull request and generated 7 comments.
Comments suppressed due to low confidence (2)
src/priveil/app.py:88
- Warmup still references
app.state.refiner, but the refiner was removed/renamed toadvisor. As written, this will raiseAttributeErrorat startup and also misses warming the advisor for this text.
await app.state.analyser.analyse(DetectionRequest(text="Warmup: Jane Smith, TFN 123 456 782", mode="fast"))
if app.state.refiner is not None:
with suppress(Exception):
warmup = await app.state.analyser.analyse(DetectionRequest(text="Warmup Jane Smith", mode="fast"))
await app.state.refiner.refine("Warmup Jane Smith", warmup.entities)
tests/unit/test_recognisers.py:143
- This cross-cutting section still refers to Presidio's
validate_resultsemantics, but the recogniser API is now_validate()and the engine no longer uses Presidio. Renaming the test/docstring and fixing the assertion message will keep the intent clear.
- Fix critical app.py hasattr→is None bug (engines were never init in prod) - Delete stale app.state.refiner warmup block (refiner removed) - test_refine.py: rename refined_client→advised_client, fix docstring, test names - test_assess.py: PRIVEIL_JUDGE_MODEL→PRIVEIL_ADVISOR_MODEL, drop PERSON assertion - test_pseudonymise.py: operator_override_redact uses email (no GLiNER2 in tests) - test_tools.py: PERSON→email/TFN tests, fix assess match string, rename tests - test_refiner.py: rename misleading test name - detection/pseudonymisation.py: fix mode description (judge→advisor env var) - recognisers/base.py: Three→Four public classes in docstring - .env.example: fix mode='judge' refs, fix wrong raw-OpenAI-client claim
- verification: ClassVar → ClassVar[Literal['trust','advisor']] in all 12 recognisers - Add Literal to typing imports in each recogniser - Fix context word matching: substring 'cw in window' → re.search with \b boundaries to prevent false positives (e.g. 'tel' matching 'hotel')
…endpoints - mode='judge' → mode='advisor' throughout - PRIVEIL_JUDGE_* → PRIVEIL_ADVISOR_* in config table - /anonymise → /pseudonymise endpoint - Add Detection Stack section (GLiNER2 + regex layers) - Remove spaCy references; update MCP install instructions - Update project structure (judge/ → advisor/, full recognisers/ tree) - JSON examples show new meta/data response envelope - Configuration table: full advisor settings + GLiNER2 model setting - Remove AU_ACCOUNT_NUMBER (not in current recogniser registry)
Comment on lines
+36
to
+45
| try: | ||
| from gliner2 import GLiNER2 | ||
| logger.info("Loading GLiNER2 model '%s'…", settings.gliner2_model) | ||
| gliner_model = GLiNER2.from_pretrained(settings.gliner2_model) | ||
| logger.info("GLiNER2 model loaded.") | ||
| except ImportError: | ||
| logger.warning( | ||
| "gliner2 package not installed — NER recognisers (PERSON, LOCATION, DATE_TIME) " | ||
| "are disabled. Install with: uv sync --extra gliner" | ||
| ) |
Comment on lines
+55
to
+59
| try: | ||
| from gliner2 import GLiNER2 | ||
| gliner_model = GLiNER2.from_pretrained(settings.gliner2_model) | ||
| logger.info("GLiNER2 model loaded for MCP server.") | ||
| except ImportError: |
Comment on lines
149
to
153
| @@ -151,7 +151,7 @@ def test_validate_result_returns_false_not_none_on_invalid( | |||
| Returning None would tell presidio 'no validation performed' and the | |||
| match would be kept at its original score — a critical correctness bug. | |||
| """ | |||
Comment on lines
+1
to
+16
| """Generic (international) phone number recogniser.""" | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| import re | ||
| from typing import ClassVar, Literal | ||
|
|
||
| from priveil.domain.entities import EntityType, Sensitivity | ||
| from priveil.recognisers.base import RegexRecogniser | ||
|
|
||
|
|
||
| class PhoneRecogniser(RegexRecogniser): | ||
| """Detect North American and international phone numbers. | ||
|
|
||
| Complements AUPhoneRecogniser with broader international patterns. | ||
| """ |
- docker-compose.yml test service: remove bare 'pytest' command override; Dockerfile CMD already uses 'uv run pytest' which resolves through the managed env — bare 'pytest' is not on $PATH - docker-compose.local.yml api/mcp: add --no-dev to 'uv run' so it does not attempt to sync dev dependencies (pytest etc.) that are absent from the local image (no-dev + gliner + mcp extras only)
…ark suite - tests/unit/test_detection_accuracy.py: 52 parametrized tests covering obvious detections (email, TFN, ABN, ACN, Medicare, AU phone, credit card, BSB), checksum/Luhn true-negatives, entity metadata assertions, and mixed-document scenarios. Documents the \b anchoring gap for +61 and (02) phone formats as known limitations. - tests/unit/test_span_advisor_routing.py: 17 unit tests for SpanAdvisor.advise() — trust bypass, score-threshold bypass (0.9), LLM invocation with drop semantics, fail-open via mocked agent.run, and HTTP-level integration using detect_client / advised_client fixtures. - benchmarks/: standalone suite requiring a live API (make serve). Auto-skips when unreachable. 7 data-driven scenarios in benchmarks/data/ (single email, customer notification, AU loan application, financial report, bulk records, dense PII, advisor-ambiguous BSB). Benchmarks cover per-scenario latency, concurrent burst throughput (5/10/25 requests), and fast-vs-advisor mode comparison. - benchmarks/generate_report.py: post-processor that strips raw timing arrays, fetches /detect for each scenario, embeds results into results.json and index.html (so the dashboard works via file://). - benchmarks/index.html: static dashboard with overview cards, latency table with inline bar charts, concurrent throughput, mode comparison, and a span viewer modal — click any scenario to see the text with detected entity spans colour-coded by type, hover tooltips, and an entity detail table. - Makefile: bench target (make serve first, then make bench); test target unchanged. pyproject.toml: pytest-benchmark added to dev deps.
Comment on lines
42
to
+46
| "pytest>=9.1.1,<10.0", | ||
| "pytest-asyncio>=1.4.0", | ||
| "pytest-cov>=7.1.0,<8", | ||
| "httpx>=0.28.1", | ||
| "hypothesis>=6.155.7", | ||
| "pytest-benchmark>=5.1.0", |
Comment on lines
+25
to
+27
| patterns: ClassVar[list[re.Pattern[str]]] = [ | ||
| re.compile(r"\b(?:\+?1[-. ]?)?\(?\d{3}\)?[-. ]?\d{3}[-. ]?\d{4}\b"), | ||
| ] |
Comment on lines
+66
to
+86
| def detect(self, text: str) -> list[Span]: | ||
| spans: list[Span] = [] | ||
| text_lower = text.lower() | ||
|
|
||
| for pattern in self.patterns: | ||
| for match in pattern.finditer(text): | ||
| matched_text = match.group() | ||
|
|
||
| if self._validate(matched_text) is False: | ||
| continue | ||
|
|
||
| score = self._BASE_SCORE | ||
|
|
||
| # Context boost: search a window of ±_CONTEXT_WINDOW chars around the match. | ||
| window_start = max(0, match.start() - self._CONTEXT_WINDOW) | ||
| window_end = min(len(text), match.end() + self._CONTEXT_WINDOW) | ||
| window = text_lower[window_start:window_end] | ||
|
|
||
| if any(re.search(r"\b" + re.escape(cw) + r"\b", window) for cw in self.context_words): | ||
| score = min(1.0, score + self._CONTEXT_BOOST) | ||
|
|
Comment on lines
149
to
157
| result = recogniser._validate(invalid_text) # type: ignore[union-attr] | ||
| assert result is False, ( | ||
| f"{type(recogniser).__name__}.validate_result({invalid_text!r}) returned " | ||
| f"{result!r}; expected False so presidio invalidates the match" |
'routing' is too generic — matches network/system routing terminology and pushes the score to 0.95, bypassing the advisor threshold (0.9). The LLM is then never consulted, causing false positives like routing codes in technical documentation being flagged as bank BSBs. Removed 'routing' and added 'account', 'transfer', 'payment' as replacement context words that are unambiguously banking-specific. Score for non-banking contexts stays at 0.8, correctly routing to the LLM advisor for contextual verification.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 73 out of 77 changed files in this pull request and generated 6 comments.
Comments suppressed due to low confidence (1)
tests/unit/test_recognisers.py:158
- The assertion message still mentions
validate_resultand Presidio. Since tests now call_validate(), the message should match the method name and the new engine behaviour (discard span onFalse).
Comment on lines
+38
to
+39
| # validate_result must return True (not None) so presidio keeps the match | ||
| assert self.recogniser.validate_result("123 456 782") is True | ||
| assert self.recogniser._validate("123 456 782") is True |
Comment on lines
41
to
45
| def test_invalid_returns_false_not_none(self) -> None: | ||
| # The spike bug: returning None here means presidio keeps the match at | ||
| # its original score. We must return False to invalidate. | ||
| result = self.recogniser.validate_result("123 456 789") | ||
| result = self.recogniser._validate("123 456 789") | ||
| assert result is False, f"Expected False, got {result!r} — validate_result must not return None on failure" |
Comment on lines
149
to
153
| @@ -151,7 +151,7 @@ def test_validate_result_returns_false_not_none_on_invalid( | |||
| Returning None would tell presidio 'no validation performed' and the | |||
| match would be kept at its original score — a critical correctness bug. | |||
| """ | |||
Comment on lines
+8
to
+10
| When the LLM advisor is unavailable (PRIVEIL_ADVISOR_MODEL not set) the advisor | ||
| tier degrades conservatively — spans are kept rather than silently dropped. | ||
|
|
Comment on lines
+22
to
+24
| patterns: ClassVar[list[re.Pattern[str]]] = [ | ||
| re.compile(r"\b[\w.+-]+@[\w-]+\.[a-zA-Z]{2,}\b", re.IGNORECASE), | ||
| ] |
Comment on lines
+25
to
+27
| patterns: ClassVar[list[re.Pattern[str]]] = [ | ||
| re.compile(r"\b(?:\+?1[-. ]?)?\(?\d{3}\)?[-. ]?\d{3}[-. ]?\d{4}\b"), | ||
| ] |
Remove stray blank line that ruff's isort rule flagged as an unsorted/unformatted import block, failing the Lint & type-check job.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 73 out of 77 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
src/priveil/recognisers/email.py:24
- The email regex only allows a single dot-separated TLD segment (e.g. "example.com"), so addresses like "[email protected]" will be partially matched as "[email protected]". In /pseudonymise this can leave the trailing ".au" in the output, leaking part of the address and producing an incorrect entity_map key.
src/priveil/recognisers/phone.py:27 - The phone regex is wrapped in word boundaries (\b). Because '+' is a non-word character, numbers that start with a country prefix like "+1 212 555 1212" will not match at the start boundary, despite the docstring claiming international support.
Addresses outstanding review feedback on #32 that was never actioned. Correctness: - email: domain allowed a single label, so [email protected] redacted to <EMAIL>.au. Allow one or more labels. - phone: leading \b cannot match before '+', so '+1 212 555 0123' lost its '+' and '+61 2 9374 4000' was not detected at all. Replace with lookarounds and add an E.164 pattern. Match parens as a balanced pair so '(212) 555-0123' no longer yields the malformed span '212) 555-0123'. - analyser: on an overlap score tie, prefer the longer span. AU_BSB matched '212-555' inside a phone number and won, redacting to 'XXX-XXX-0123'. - person/location: declared verification='trust', so SpanAdvisor never submitted them and span_advisor.md's PERSON/LOCATION false-positive guidance was unreachable. Route them to the advisor tier. - app/mcp: GLiNER2 load caught only ImportError, so an installed-but- unloadable model killed startup. Degrade to regex-only instead. Docs/tests: - Drop stale presidio/validate_result wording for the _validate contract. - Correct span_advisor and phone docstrings. - Add span-exactness, international, dedup and NER-routing regressions.
Comment on lines
+121
to
+125
| prev = result[-1] | ||
| if span.start < prev.end: # Overlapping | ||
| if span.score > prev.score: | ||
| result[-1] = span | ||
| # else: keep prev, discard span |
| advisor_spans.append(entity) | ||
|
|
||
| if not advisor_spans: | ||
| return AdvisorResult(entities=tuple(certain), advisor_applied=False) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changed
judge→advisor— module moved tosrc/priveil/advisor/; assessor and span-advisor (formerly refiner) preserved with corrected logic (prefix strip, threshold 0.9, fail-open sort).src/priveil/recognisers/base.pystandardises the interface; all AU recognisers updated.src/priveil/recognisers/.src/priveil/mcp/__main__.pyandsrc/priveil/__main__.pyadded for direct module execution.pyproject.tomlanduv.lockupdated.Why
Addresses review feedback: judge terminology was confusing; generic recognisers were missing; MCP server lacked a runnable entrypoint.