feat(pii): add UK NINO detection and redaction to Tier 1 - #11

Merged current main into this branch, so it is no longer in conflict. Head is 97434e0.

main moved llm_shield_proxy/pii_engine.py to llm_shield_proxy/engines/pii_engine.py. Git carried the change across the rename; against current main this branch is still the same 2 files, +76/-0 — the UK_NINO entry in TIER1_PATTERNS and the two tests. The 76 added lines are byte-identical to the ones you already have (sha256 cd2b472c… over the added lines of both diffs), and nothing is removed.

Verification at the merge head

• pytest tests/test_pii_engine.py — 12 passed, including both NINO tests.
• ruff check . — clean. bandit -r llm_shield_proxy/ -s B101,B104,B311 — clean.
• pytest --ignore=tests/k8s --ignore=tests/k8s_ootb --ignore=tests/ootb, the selection CI runs — 110 passed, 1 failed. The failure is tests/test_grpc_ext_proc.py::test_ext_proc_redacts_request_body (TypeError: must be called with a dataclass type or instance). It reproduces on a detached checkout of main at a48cf32 with this branch absent, and CI on main is red for a48cf32, cb05151 and b09777a. I have not touched it — flagging it only so the red check here is not read as this branch's.

Tier 1 overhead, since CONTRIBUTING.md sets a sub-millisecond bar for compiled Tier 1 regex. Measured over 2000 rounds on an 800-character payload containing an email, a phone number, an SSN, a card number, an IP and a NINO, Tier 1 only, comparing TIER1_PATTERNS with and without the one added entry:

Separately, timing pattern.search() for the added NINO regex alone on five adversarial inputs up to 80k characters (long partial prefixes, near-miss tails, 20k repeated separators, letters only, digits only): each completed in under 0.9 ms on the same machine.

Synthetic-mode placeholder for UK_NINO is not in this PR. I can follow up with whichever you prefer.

In synthetic mode, Vault.get_or_create_token maps entity types to shape-preserving fakes — EMAIL to a fake email, SSN to a fake SSN, CREDIT_CARD to a fake card. UK_NINO matches none of those branches and falls through to else: token = fake.word(), so a NINO is replaced by an ordinary word. Redaction and rehydration still work; non-synthetic mode gives [UK_NINO_1] like every other type, which is what the tests cover. The options I can see:

1. Faker('en_GB').ssn(). On Faker 40.37.0, seeded, 200 calls produced values of the form ZZ604876T, ZZ759382T, ZZ219489T — every one prefixed ZZ and suffixed T — and 0 of the 200 matched the pattern in this PR. So the placeholder would not be re-detected by the engine, and vault.py currently holds one shared default-locale Faker().
2. Generating a format-valid NINO preserves the shape, but manufactures strings that may match a real person's identifier.
3. Leaving the generic-word fallback as it is.

Which of those you want is a call about your product rather than about the pattern, so I have left it to you.

@snowyukitty
Thanks for this, and sorry for the slow turnaround. This is a well put together PR. The HMRC exclusion rules are right, the test matrix covering all seven excluded pairs and both position-specific character sets is thorough, you measured the Tier 1 latency against the CONTRIBUTING.md bar instead of asserting it, and your follow-up comment did the diff accounting and the synthetic mode analysis properly. I especially appreciate that you traced test_ext_proc_redacts_request_body back to a detached checkout before flagging it. You were right, it was pre-existing, and it passes on current main now.

Before the findings, an apology that is squarely mine.

main was force-pushed after you merged it. Your +76/-0 against current main was accurate when you wrote it. It is not accurate any more, and that is not your doing. The commits you merged were rewritten:

Same messages, different SHAs, so your merge base no longer exists and the diff reads as 144 files. You will need to rebase onto current main, and you will hit conflicts that are the result of the rewrite rather than anything you wrote. Sorry for that. Shout if it gets messy and I will help untangle it.

Two actual findings.

1. The pattern matches ordinary English prose

This is the main one. The pattern is case insensitive and treats each separator as independently optional, so any two letter word followed by three 2-digit groups and a letter a-d matches:

"the meeting is on 10 29 38 c and then we break"  ->  matches "on 10 29 38 c"
"as 12 34 56 a rule we redact everything"         ->  matches "as 12 34 56 a"
"at 09 08 07 a"                                   ->  matches "at 09 08 07 a"

12 of the 23 most common two letter English words are valid NINO prefixes under the HMRC rules: as at be by he me my on or we am an. Because the proxy rewrites user prompts in flight, this would silently corrupt legitimate text in normal conversation.

Your verification would not have surfaced this, and I do not think that reflects on the work. The test suite only asserts that invalid NINOs are not matched, so there is no assertion that ordinary prose passes through untouched. Could you add a couple of cases in that direction? Honestly that class of assertion is missing for the other Tier 1 patterns too, so it would be a useful thing to establish here.

2. \b needs to become the ASCII boundary helpers

This one landed after your snapshot of main, so there was no way for you to pick it up. ede0727 moved every Tier 1 pattern off \b and onto _ASCII_LEFT_BOUNDARY / _ASCII_RIGHT_BOUNDARY (the reasoning is in the comment at llm_shield_proxy/engines/pii_engine.py:57-62). Python's \b treats CJK characters as word characters, so PII glued directly to non-Latin script does not match. The new pattern has the same gap:

"邮箱是AB123456C没有"  ->  no match with \b, matches with the ASCII boundaries

That is a leak, so the NINO pattern should use the helpers once you rebase. Your "76 lines byte-identical, nothing removed" note is exactly right, and it is why the \b survived the merge intact.

A suggested pattern

Requiring uppercase and one consistent separator (via a backreference) fixes both issues at once. I tested this against all five of your positive samples and your full invalid list, and it keeps every positive and rejects every negative:

(
    "UK_NINO",
    re.compile(
        _ASCII_LEFT_BOUNDARY
        + r"(?!(?:GB|BG|NK|KN|TN|NT|ZZ))[A-CEGHJ-PR-TW-Z][A-CEGHJ-NPR-TW-Z]([ -]?)\d{2}\1\d{2}\1\d{2}\1[A-D]"
        + _ASCII_RIGHT_BOUNDARY
    ),
),

Two known trade-offs, both of which I think are the right call, but say so if you disagree:

• All caps prose such as ON 10 29 38 C still matches. Narrowing that further needs a context keyword like "NINO" or "National Insurance" nearby, which feels like scope creep for this PR.
• Mixed separators such as AB 12-34 56C no longer match. Real NINOs do not appear that way in practice, and the backreference is what buys us the false positive fix, so I think that is a fair trade.

Use something different if you see a better approach. The two things I care about are that ordinary prose survives and that the boundary handling matches the rest of Tier 1.

On synthetic mode

Thanks for laying the three options out so clearly, and for measuring what Faker('en_GB').ssn() actually produces across 200 seeded calls rather than assuming. That is the kind of detail that makes a decision easy. I confirmed the fake.word() fallback at llm_shield_proxy/engines/vault.py:140.

Let's go with option 3 and leave the generic word fallback, and keep it out of this PR. Option 2 manufactures strings that could collide with a real person's identifier, and as you found, option 1 produces placeholders the engine cannot re-detect. Redaction and rehydration both work correctly today, which is what matters. I will open a separate issue to track the placeholder shape.

Happy to look again as soon as you have rebased. Thanks again for the care you put into this one, and sorry again about the force push.

Thanks for the review — both findings were real, and the second one I had no way to see.

Rebased onto current main and force-pushed the branch. Head is 008f79e: one commit, 2 files, +102/-0. The head tree is unchanged from what was there before the rebase, and the pull request had no review comments to orphan.

The 144-file diff. The conflicts were the ones you predicted, and they turned out to be avoidable: the three commits I had merged have trees identical to their rewritten counterparts — a48cf32/385843e, cb05151/eb7e964 and b09777a/002fe2c each pair to the same tree hash — so there was nothing to reconcile, only a base to discard. The branch is now upstream's tree plus the NINO work.

1. Prose false positives. Taken as you wrote it — uppercase, ASCII boundary helpers, one captured separator backreferenced for each group. Your three sentences pass through untouched, as do the other nine two-letter prefixes (be by he me my or we am an).

The test is stronger than a NINO check: it asserts redacted == sentence through the whole Tier 1 cascade, so it fails if any pattern rewrites ordinary prose, not just this one. That is the assertion the suite did not have, and it discriminates — the old pattern replaces on 10 29 38 c and fails the equality.

2. \b. Fixed, and 邮箱是AB123456C没有 is now a test case. It does not match with \b.

Trade-offs, so they are yours rather than hidden. Lower case and mixed separators no longer match, which you called. Beyond those, requiring the separator before the suffix also misses AB 12 34 56C and AB-12-34-56C.

I checked whether making just that separator optional (\1?[A-D]) was a free win. It is not. It picks up those two forms, but it also widens the all-caps prose class you already accepted: ON 10 29 38C and AS 12 34 56A match under \1? and do not today. It does not reach AB123456 C either, since that is a second separator rather than an omitted one. I have left the pattern exactly as you wrote it.

Synthetic mode is untouched — option 3, generic word fallback, as you decided. The diff is TIER1_PATTERNS and the tests, nothing else.

Validation at 008f79e.

• ruff check . — All checks passed. bandit -r llm_shield_proxy/ pii-leak-benchmark/pii_leak_benchmark/ -s B101,B104,B311 — no issues identified across 10,671 lines.
• pytest --ignore=tests/k8s --ignore=tests/k8s_ootb --ignore=tests/ootb — 684 passed, 43 skipped, 13 xfailed, 0 failed. You were right that test_ext_proc_redacts_request_body passes on current main; run on its own here it passes too.
• Tier 1 overhead, one run of 2000 rounds over a 573-character payload, Tier 1 only, with and without the one added pattern: median 0.0596 → 0.0652 ms (+0.0056 ms), p95 0.0619 → 0.0679 ms. Python 3.12.10, Windows AMD64.
• The backreference is a single optional character with no nested quantifier, so it cannot backtrack pathologically, but I measured it: UK_NINO alone against long valid prefix runs, near-miss tails, 40,000 characters of repeated separators, letters only and digits only, each input up to 80,000 characters — worst single search 0.953 ms.

I also checked the character classes against HMRC's NIM39110 letter by letter: [A-CEGHJ-PR-TW-Z] and [A-CEGHJ-NPR-TW-Z] admit exactly the permitted letters, with O correctly allowed in the first position and excluded from the second.

/review

@greptileai

Re-reviewed at the rebased head (008f79e), diff is TIER1_PATTERNS (+14) and tests/test_pii_engine.py (+88), nothing removed.

Both findings from the earlier review round are fixed in this diff:

1. Prose false positives. The pattern now requires uppercase and backreferences a single captured separator (([ -]?)\d{2}\1\d{2}\1\d{2}\1), so the independently-optional-separator shape that matched ordinary two-letter words + digit groups + a-d ("the meeting is on 10 29 38 c") no longer applies. test_pii_tier1_uk_nino_leaves_ordinary_prose_untouched asserts redacted == sentence for all 13 prose cases through the whole Tier 1 cascade, not just a "no UK_NINO span" check.
2. CJK boundary leak. The pattern uses _ASCII_LEFT_BOUNDARY / _ASCII_RIGHT_BOUNDARY instead of \b, matching the rest of Tier 1 (ede0727). test_pii_tier1_uk_nino_matches_when_glued_to_non_latin_script covers 邮箱是AB123456C没有.

I independently re-derived the two character classes against HMRC NIM39110 ([A-CEGHJ-PR-TW-Z] excludes D,F,I,Q,U,V; [A-CEGHJ-NPR-TW-Z] additionally excludes O) and the seven reserved-pair exclusions in the lookahead — both check out letter by letter. The capturing group is read back via match.group(0) (the whole match), not the backreferenced group, so span/token minting is unaffected. The single optional-character backreference has no nested quantifier, so it isn't a new ReDoS surface on the streaming hot path, consistent with the adversarial timing already posted in this thread.

No new findings in the personal-data/secrets, fail-open, hot-path, or measurement-claim classes. classify_tier1_match correctly leaves UK_NINO on the default (True, "medium") path, same as every other Tier 1 type without a checksum.

Verdict: MERGEABLE

@greptileai please review

@snowyukitty thank you, and sorry this sat for five weeks after you fixed everything. That delay was on me.

I checked both findings against the current code:

• Ordinary prose: all 13 sentences in your new test pass through unchanged, and the pattern found nothing across about 4.2 million characters of the repository's own text outside the tests.
• Text glued to Chinese characters: 邮箱是AB123456C没有 is caught.

I merged current main into the branch so CI ran on the real result; it is green. Merging now.

One small thing for a possible follow-up, not a blocker: a number written with spaces in the digits but none before the letter (AB 12 34 56C) is not matched, because the separator has to be the same everywhere. If you want to take that on, a separate PR is welcome.

Thanks again for the careful work, especially the prose test. That kind of assertion was missing for the other Tier 1 patterns too.